[U-Boot] [PATCH v2 01/26] dm: i2c: Provide an offset length parameter where needed

[Masahiro YAMADA]

Hi Simon,


2015-01-22 1:12 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Bin,
>
> On 21 January 2015 at 03:45, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
>> Hi Simon,
>>
>>
>>
>> On Mon, 19 Jan 2015 20:12:30 -0700
>> Simon Glass <sjg at chromium.org> wrote:
>>
>>
>>> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
>>> index 1e500fb..7c3ad00 100644
>>> --- a/common/cmd_i2c.c
>>> +++ b/common/cmd_i2c.c
>>> @@ -168,7 +168,7 @@ static int i2c_get_cur_bus_chip(uint chip_addr, struct udevice **devp)
>>>       if (ret)
>>>               return ret;
>>>
>>> -     return i2c_get_chip(bus, chip_addr, devp);
>>> +     return i2c_get_chip(bus, chip_addr, 1, devp);
>>>  }
>>
>> The i2c command calls
>> [1] i2c_get_cur_bus_chip()   = set the offset len to 1
>> [2] i2c_set_chip_offset_len  = change the offset
>>
>>
>> Now we can do [1] and [2] at the same time, right?
>>
>>
>> If we set the offset address when we get the device,
>> we won't need i2c_set_chip_offset_len(), I think.
>>
>> The offset_len for each device does not change.
>> Chainging it on the way makes no sense.
>
> Except that I inserted this patch into the series I sent a week ago,
> which I want to apply today/tomorrow. So I wanted to limit the scope
> of the change. I was not able to convince myself that there would be
> no side-effects with the cmd_i2c.c change. But if you have had a look
> too then perhaps we are OK.
>
> I'll send a follow-up patch to clean this up (still will be in this
> merge window).

Sorry, I noticed a side-effect.

If you mistype the offset length on the first run of the I2C command,
a generic chip is created and bound with the wrong offset length.
You cannot correct this because on the next run, i2c_get_chip()
find the chip from the bound-devices list.

We still need i2c_set_chip_offset_len
to set the offset length explicitly.

I take back my former comment.  Sorry.





>>
>>
>>
>>
>>>  }
>>>
>>> -int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp)
>>> +int i2c_get_chip(struct udevice *bus, uint chip_addr, uint offset_len,
>>> +              struct udevice **devp)
>>
>>
>> If the device tree for the child device is not found
>> (i.e. i2c_bind_driver() is called), the new generic chip
>> will be given with the offset_len.
>>
>> On the other hand, if the device tree is found,
>> offset_len is default to 1 because
>> i2c_chip_ofdata_to_platdata() always set chip->offset_len to 1.
>>
>> It is a pity.
>>
>> I wonder if it would not be possible to
>> get the default offset_len from the device tree node of the child device.
>>
>>
>> For example, the EEPROM on my board expects chip->offset_len == 2.
>>
>> It would be nice if we could have the offset property for the EEPROM device node.
>>
>> I dug into Documentation/devicetree/bindings/, but I could not find the one.
>
> I have a local patch which adds a 'u-boot,i2c-offset-length' property,
> but in the case of cros_ec I just changed the value in the probe
> function. So it wasn't essential.
>
> I would prefer to add device tree support though. Since it seems you
> agree I will go ahead with this. Any preference on property naming?


I checked Documentation/devicetree/bindings/eeprom.txt,
but nothing is mentioned about the offset length.

So, I am OK with your idea.




-- 
Best Regards
Masahiro Yamada