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

[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.
>
>
>
> 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.
>
>

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
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
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?

>
>
> >
> > >
> > >
> > >
> > >> +     }
> > >> +     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