[U-Boot] [PATCH v3 01/10] dm: i2c: Add a uclass for I2C

[Simon Glass]

Hi Masahiro,


On 3 December 2014 at 06:24, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
> A little more comments from me.
>
>
>
>
> On Mon, 24 Nov 2014 11:57:15 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
>> +int i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
>> +{
>> +     struct dm_i2c_ops *ops = i2c_get_ops(bus);
>> +     struct dm_i2c_bus *i2c = bus->uclass_priv;
>> +     int ret;
>> +
>> +     if (ops->set_bus_speed) {
>> +             ret = ops->set_bus_speed(bus, speed);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +     i2c->speed_hz = speed;
>> +
>> +     return 0;
>> +}
>
>
> This looks odd.
>
>
> If each driver does not have .set_bus_speed handler,
> we cannot change the bus speed
> because changing the bus speed involves some hardware
> register(s) setting.
>
> We should not change i2c->speed_hz without changing the
> actual speed.
>
> I think the code should be:
>
>
>         if (ops->set_bus_speed) {
>                 ret = ops->set_bus_speed(bus, speed);
>                 if (ret)
>                         return ret;
>                 i2c->speed_hz = speed;
>         }
>
>

I'll add a comment. The idea is that the driver can check the speed
and give an error here rather than on the next xfer(). Also if it
wants to change the clocks here then it can do so. But otherwise it is
OK to deal with the speed change on the next xfer.

[snip]

Regards,
Simon