[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that need generate stop bit

[Simon Glass]

Hi Chuanhua,

On Sun, 23 Jun 2019 at 22:48, Chuanhua Han <chuanhua.han at nxp.com> wrote:
>
> + Simon Glass
>
> > -----Original Message-----
> > From: Lukasz Majewski <lukma at denx.de>
> > Sent: 2019年6月18日 16:07
> > To: Chuanhua Han <chuanhua.han at nxp.com>
> > Cc: hs at denx.de; sjg at chromium.org; Biwen Li <biwen.li at nxp.com>;
> > u-boot at lists.denx.de
> > Subject: [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that need generate stop bit
> >
> > On Mon, 17 Jun 2019 21:12:40 +0800
> > Chuanhua Han <chuanhua.han at nxp.com> wrote:
> >
> > > Usually the i2c bus needs to write the address of the register before
> > > reading the internal register data of the device (ignoring the
> > > transmission of the slave address).
> > >
> > > Generally, the stop signal is not needed before the register is read,
> > > but there is a special chip that needs this stop signal (such as
> > > pcf2127). However, in the current i2c general code, the dm_i2c_read
> > > api encapsulates two messages, the first time is to set the register
> > > address message, the second time is a message to read the register
> > > data, so that no stop signal is generated.
> > >
> > > This patch uses the DM_I2C_CHIP_ADDR_STOP flag for specific i2c chips,
> > > so if the i2c slave requires a stop signal, chips driver can set this
> > > flag, then call the dm_i2c_write to set the register address (a stop
> > > signal is generated after this API call), then call dm_i2c_read to
> > > read the register data.
> > >
> > > Signed-off-by: Chuanhua Han <chuanhua.han at nxp.com>
> >
> > Reviewed-by: Lukasz Majewski <lukma at denx.de>

I already asked why you cannot use dm_i2c_xfer() to do what you want,
but I did not see your response. Can you please send your response
again here on this thread? The dm_i2c_xfer() function is designed to
handle any sorts of quirk that are needed. I'd like to avoid quirks in
the core code if possible.

If you really want to add a quirk, then I would like to put it behind
a CONFIG_I2C_QUIRKS option, and use:

  if (IS_ENABLED(CONFIG_I2C_QUIRKS) || !(chip->flags & DM_I2C_CHIP_ADDR_STOP)) {
           if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
                   ptr++;

so we don't affect code size on other platforms. After all this
feature is just for one broken chip and should not negatively affect
everyone else that follows the spec correctly.

I am worried about all the SPL-affecting changing that add a few bytes
here and there, but really add up.

But my first preference would be to use i2c_xfer() if possible.

Regards,
Simon