[U-Boot] [RFC PATCH 2/2] dm: i2c: support 10bit addressing in I2C uclass layer

[Simon Glass]

Hi Masahiro,

On 20 December 2014 at 00:25, Masahiro YAMADA <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
> 2014-12-20 6:34 GMT+09:00 Simon Glass <sjg at chromium.org>:
>> Hi Masahiro,
>>
>> On 19 December 2014 at 11:34, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
>>> Master send to / receive from 10-bit addressed slave devices
>>> can be supported by software layer without any hardware change
>>> because the LSB 8bit of the slave address is treated as data part.
>>>
>>> Master Send to a 10bit-addressed slave chip is performed like this:
>>>
>>>  DIR    Format
>>>  M->S   11110 + address[9:8] + R/W(0)
>>>  M->S   address[7:0]
>>>  M->S   data0
>>>  M->S   data1
>>>       ...
>>>
>>> Master Receive from a 10bit-addressed slave chip is like this:
>>>
>>>  DIR    Format
>>>  M->S   11110 + address[9:8] + R/W(0)
>>>  M->S   address[7:0]
>>>         (Restart)
>>>  M->S   111110 + address[9:8] + R/W(1)
>>>  S->M   data0
>>>  S->M   data1
>>>       ...
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.m at jp.panasonic.com>
>>> Cc: Heiko Schocher <hs at denx.de>
>>> Cc: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>>  drivers/i2c/i2c-uclass.c | 80 +++++++++++++++++++++++++++++++-----------------
>>>  include/i2c.h            |  4 +++
>>>  2 files changed, 56 insertions(+), 28 deletions(-)
>>
>> Seems like a good idea if we can make it work...
>>
>> But this is driver-specific. Some drivers have hardware to send the
>> address and it isn't part of the message. For example see the tegra
>> driver.
>>
>> So what you have here feels a bit like a hack to me. Can't the driver
>> implement it? If you are trying to avoid driver work to support 10-bit
>> addresses, maybe it should be an option that we can enable for each
>> driver, so we don't break the other drivers?
>
>
> I was writing two I2C drivers on DM,
> both of which have no dedicated hardware support for 10bit addressing.
>
> Of course, the driver could implement it, but it means
> I put the completely the same code in each of driver.
>
> For write transaction, for example, we create a new buffer and copy
> offset-address + data in Uclass layer.
>
> Do I have to create a new buffer again, in my driver,
> and copy  lower-slave-address + offset-address + data ?

I see your point!

>
> Perhaps, is it a good idea to have it optional?
>
> DM_I2C_FLAG_SW_TENBIT  - if set, uclass takes care of 10bit addressing
> by software
>                                                     if unset, each
> driver is responsible to handle I2C_M_TEN correctly
>
> altough I do think 10bit support on U-Boot is urgent necessity...

I've thought about this quite a bit. We have only just changed the API
to be the same as Linux (the list of messages). It seems wrong to now
hack it around, such that the address field only stores the first part
of the address in 10-bit mode. Did we like the API or not?

I see two options that are somewhat sane and defensible:

- Add a helper function in the uclass that the driver can call to turn
the address + message into a single stream of bytes (we can avoid a
second malloc() by reserving space for the address bytes before the
message or similar similar, so long is it is clearly documented)
- Allow the driver to request that the message bytes should always
contain the address, which would remove the address-processing code
from the driver.

I think this needs a little more discussion before we decide what to do.

Regards,
Simon