[U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C

[Heiko Schocher]

Hello Simon, Masahiro,

Am 20.11.2014 15:04, schrieb Simon Glass:
> + A few more people to cc
>
> Hi Masahiro,
>
> On 20 November 2014 06:05, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
>>
>> Hi Simon,
>>
>>
>>
>> On Wed, 19 Nov 2014 10:24:47 +0000
>> Simon Glass <sjg at chromium.org> wrote:
>>
>>>>
>>>>
>>>> BTW, address_offset within the chip and data are treated in the same way in I2C bus.
>>>> Should we pass them separately to each driver?
>>>>
>>>> I mean, can we put the offset address and data in the buffer?
>>>>
>>>>
>>>
>>> I'm not sure what you mean by this sorry.
>>
>>
>> Let's assume we want to write some data to a EEPROM chip connected to i2c bus.
>>
>> We generally send
>>
>>   [byte 0] SLAVE_ADDR (7bit) + W flag
>>   [byte 1] Offset address in EEPROM where you want to start writing
>>   [byte 2] WData0
>>   [byte 3] WData1
>>     ...
>>
>>
>>  From the perspective of I2C protocol,  [byte 1], [byte 2], [byte 3], ... are all data.
>>
>> I2C itself deos not (should not) know which byte is the offset_address in the chip
>> and which is the *real* data to be written.
>>
>>
>>
>>>>> +     return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,
>>>>> +                       len);
>>
>> In this implementation, the offset_address is treated with "addr"
>> and the *real* data is handled with "buffer".
>>
>> It seems odd from the perspective of I2C protocol, I think.

Yes.

>>
>>
>>
>> Likewise, when we read data from a EEPROM chip connected to i2c bus,
>>
>> We generally send/receive
>>    [byte 0] SLAVE_ADDR (7bit) + W flag
>>    [byte 1] Offset address in EEPROM where you want to start reading
>>    [byte 2] SLAVE_ADDR (7bit) + R flag
>>    [byte 3] RData 0
>>    [byte 4] Rdata 1
>>
>>
>> [byte 1], [byte 3], [byte 4] are data written/read via I2C bus.
>>
>> In my view, each I2C driver should handle [byte 0] and [byte 1] in its ".write" operation
>> and [byte 2], [byte3], [byte 4], .... in its ".read" operation.

Yes, but this changes the current U-Boot API ...

>>
>>
>
> We could certainly change this. I'm not sure that I have a strong
> opinion either way.
>
> I haven't to date seen an I2C chip where we don't have an address as
> the first byte. If we change the API at the driver level, which I
> think is what we are discussing, then we would need to move to a
> message array format. The read transaction would become two elements:
> a write (for the address) then a read (to get the data).
>
> If we want to change it, we should do it now. My question is, what is
> the point? Will we really want >2 elements in the message array, do we

Do we need more than 2 elements? But of course, if we go into this direction
we should support n elements ...

> want more control over how transactions are done (repeated start,
> etc.)? I'm not sure. Still it would be a fairly low-impact change I

Thats the point ... do we need all this stuff in U-Boot?

> feel. We are changing the drivers anyway, so changing the
> uclass-to-driver API would be feasible. One advantage perhaps it that
> it would match Linux more closely.
>
> Perhaps Heiko can share an opinion here?

This implements that we must adapt each i2c driver "a little bit more"
right? But I think, as we go with this approach more into the linux
direction it sounds good to me (maybe we can directly use linux i2c
drivers? ... that sounds good, and maybe should be a goal too?). I could
not really say how many work this would be, but as we do this change step
by step it is worth to go in this direction, as we can cleanup here and
there (especially the eeprom driver) some "suboptimal" code ...

Thinking about it ... maybe we start from scratch with i2c drivers for
DM and try to use linux i2c drivers?

bye,
Heiko

>
>>
>>
>>>
>>>>
>>>>
>>>>
>>>>> +     }
>>>>> +     debug("not found\n");
>>>>> +     return i2c_bind_driver(bus, chip_addr, devp);
>>>>> +}
>>>>
>>>>
>>>>
>>>> If no chip-device is found at the specified chip_addr,
>>>> the last line calls i2c_bind_driver().  Why?
>>>>
>>>> The i2c_bind_driver() tries to create a "generic" chip.
>>>> What is this "generic" chip?
>>>>
>>>> Besides, i2c_bind_driver() tries to probe the created generic chip,
>>>> but it always fails in i2c_chip_ofdata_to_platdata()
>>>> because the generic chip does not have "reg" property
>>>>
>>>> I could not understand at all what this code wants to do.
>>>
>>> This actually creates the device. A generic I2C device is something
>>> that has no specific driver, but you can use read()/write() on it.
>>>
>>> As an example, if we have an EEPROM we might add a special driver for
>>> it with functions like 'erase' and 'lock'. In that case we would bind
>>> the EEPROM driver to this address on the I2C bus. But for many cases
>>> we don't have/need a special driver, and can just fall back to one
>>> that supports simple read() and write() only.
>>
>>
>> Sorry, I could not parse you here.
>>
>> I2C is not a hot-plugged bus.
>> I could not understand why such a dummy device is created on run time.
>> Is it related to 'erase' or 'lock' functions?
>>
>
> If we cannot write to the chip (i.e. it does not ACK when we send it
> its address) then we won't be able to talk to it, so there is no point
> in creating a device.
>
> With driver model / device tree we could just blindly add the device
> and use it, but I chose to duplicate the current behaviour since this
> is expected.
>
>>
>>
>>
>>
>>
>>
>>
>> BTW, sandbox_i2c_read() is 400KHz tolerate:
>>
>>
>>          /* For testing, don't allow reading above 400KHz */
>>          if (i2c->speed_hz > 400000 || alen != 1)
>>                  return -EINVAL;
>>
>>
>>
>> but sandbox_i2c_write() only allows 100KHz:
>>
>>          /* For testing, don't allow writing above 100KHz */
>>          if (i2c->speed_hz > 100000 || alen != 1)
>>                  return -EINVAL;
>>
>>
>>
>>
>> Because the clock-frequency is set to <400000> in the sandbox DTS,
>> writing to I2C fails unless we change the I2C speed.
>>
>> Is this intentional?
>>
>> Personally, I like everything to work on the mail line.
>
> This is test code, as it says in the comment. I'm considering
> splitting sandbox into two boards, one with the test code and one
> without. But let's see how this develops.
>
> Regards,
> Simon
>

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany