[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset
Lukasz Majewski
lukma at denx.de
Mon Jun 17 07:32:18 UTC 2019
Hi Chuanhua,
> > -----Original Message-----
> > From: Lukasz Majewski <lukma at denx.de>
> > Sent: 2019年6月4日 16:19
> > To: Chuanhua Han <chuanhua.han at nxp.com>
> > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li at nxp.com>
> > Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag
> > that not call i2c_setup_offset
> >
> > On Tue, 4 Jun 2019 07:59:44 +0000
> > Chuanhua Han <chuanhua.han at nxp.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Lukasz Majewski <lukma at denx.de>
> > > > Sent: 2019年6月4日 15:56
> > > > To: Chuanhua Han <chuanhua.han at nxp.com>
> > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > <biwen.li at nxp.com> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2]
> > > > dm: i2c: Add a flag that not call i2c_setup_offset
> > > >
> > > > On Tue, 4 Jun 2019 06:48:59 +0000
> > > > Chuanhua Han <chuanhua.han at nxp.com> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Lukasz Majewski <lukma at denx.de>
> > > > > > Sent: 2019年6月4日 14:46
> > > > > > To: Chuanhua Han <chuanhua.han at nxp.com>
> > > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > > <biwen.li at nxp.com> Subject: Re: [EXT] Re: [U-Boot] [PATCH
> > > > > > 1/2] dm: i2c: Add a flag that not call i2c_setup_offset
> > > > > >
> > > > > > On Tue, 4 Jun 2019 02:20:49 +0000 Chuanhua Han
> > > > > > <chuanhua.han at nxp.com> wrote:
> > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Lukasz Majewski <lukma at denx.de>
> > > > > > > > Sent: 2019年6月4日 6:08
> > > > > > > > To: Chuanhua Han <chuanhua.han at nxp.com>
> > > > > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > > > > <biwen.li at nxp.com> Subject: [EXT] Re: [U-Boot] [PATCH
> > > > > > > > 1/2] dm: i2c: Add a flag that not call i2c_setup_offset
> > > > > > > >
> > > > > > > > On Thu, 30 May 2019 18:31:48 +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_RD_NO_I2C_SETUP_OFFSET
> > > > > > > > > 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>
> > > > > > > > > ---
> > > > > > > > > Changes in v3:
> > > > > > > > > - Use the new flag
> > > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > > > >
> > > > > > > > > Changes in v2:
> > > > > > > > > - Split the original patch into 3 patches
> > > > > > > > > - Add detailed description information for
> > > > > > > > > each patch
> > > > > > > > >
> > > > > > > > > drivers/i2c/i2c-uclass.c | 6 ++++--
> > > > > > > > > include/i2c.h | 1 +
> > > > > > > > > 2 files changed, 5 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > > > > > b/drivers/i2c/i2c-uclass.c index
> > > > > > > > > e47abf1833..9804b5e8c7 100644 ---
> > > > > > > > > a/drivers/i2c/i2c-uclass.c +++
> > > > > > > > > b/drivers/i2c/i2c-uclass.c @@ -135,8 +135,10 @@ int
> > > > > > > > > dm_i2c_read(struct udevice *dev, uint offset, uint8_t
> > > > > > > > > *buffer, int len) if (chip->flags &
> > > > > > > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev,
> > > > > > > > > offset, buffer, len); ptr = msg;
> > > > > > > > > - if (!i2c_setup_offset(chip, offset,
> > > > > > > > > offset_buf, ptr))
> > > > > > > > > - ptr++;
> > > > > > > > > + if (!(chip->flags &
> > > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> > > > > > > > > + if (!i2c_setup_offset(chip, offset,
> > > > > > > > > offset_buf, ptr))
> > > > > > > > > + ptr++;
> > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > if (len) {
> > > > > > > > > ptr->addr = chip->chip_addr; diff
> > > > > > > > > --git a/include/i2c.h b/include/i2c.h index
> > > > > > > > > a5c760c711..3123cbf280 100644
> > > > > > > > > --- a/include/i2c.h
> > > > > > > > > +++ b/include/i2c.h
> > > > > > > > > @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
> > > > > > > > > DM_I2C_CHIP_10BIT = 1 << 0,
> > > > > > > > > /* Use 10-bit addressing */
> > > > > > > > > DM_I2C_CHIP_RD_ADDRESS = 1 << 1,
> > > > > > > > > /* Send address for each read byte */
> > > > > > > > > DM_I2C_CHIP_WR_ADDRESS = 1 << 2,
> > > > > > > > > /* Send address for each write byte */
> > > > > > > >
> > > > > > > > Aren't those two above flags describe exactly what you
> > > > > > > > need? They send address for each read/written byte. (or
> > > > > > > > do you need to send more than a single byte)?
> > > > > > > This is not what I need. I need to generate a stop signal
> > > > > > > after setting the register address,
> > > > > >
> > > > > > Please correct me if I'm wrong, but the above flag says
> > > > > > that you need to send the I2C stop after you send one byte?
> > > > > >
> > > > > > Would it be possible to send address with I2C stop and then
> > > > > > read data (single byte) with I2C stop?
> > > > > >
> > > > > > > so I need the following flag.
> > > > > > > >
> > > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET = 1 << 3,
> > > > > >
> > > > > > I do find this name as a bit misleading - is the
> > > > > > RD_NO_I2C_SETUP_OFFSET name reflecting the purpose (to send
> > > > > > I2C stop after sending address)?
> > > > > The purpose is to send a stop signal after setting the slave
> > > > > register address.
> > > >
> > > > What I'm trying to point out is that the above description
> > > > doesn't match the flag name. Maybe there is a better, more
> > > > descriptive name?
> > > Because the stop signal is generated by not calling the
> > > i2c_setup_offset function, this flag is used.
> >
> > But the name of the flag shall be not based on particular function
> > name. It shall be a generic description - like presented above:
> >
> > DM_I2C_CHIP_WR_ADDRESS = 1 << 2,
> > /* Send address for each write byte */
> >
> > With such generic names - there is no problem if we refactor the
> > code and change the function name.
> >
> > I've looked closer into those two flags:
> > git grep -n "DM_I2C_CHIP_WR_ADDRESS"
> >
> > drivers/rtc/ds1307.c:319:
> > drivers/rtc/isl1208.c:173:
> > drivers/rtc/rv3029.c:469:
> >
> > It looks like those above rtc devices use it - so maybe the issue
> > described for pcf2127 is also present on those devices (so those
> > flags have been introduced) ?
> In this way, although each byte will send the slave address, this
> does not correspond to the timing of pcf2127. Also, there is no stop
> signal after setting the register address. The chip also doesn't work.
I don't mind to add new flag - It is OK.
Please only name it properly - like DM_I2C_CHIP_ADDR_STOP - to be
generic enough so other IC's with similar issue can re-use it.
> >
> > > >
> > > > Also - what is byte length of send address to this device? One
> > > > byte ?
> > > Yes,one byte
> > > >
> > > > > >
> > > > > > Maybe there would be a better name?
> > > > > >
> > > > > > > > > /* No i2c_setup_offset*/ };
> > > > > > > > >
> > > > > > > > > struct udevice;
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Best regards,
> > > > > > > >
> > > > > > > > Lukasz Majewski
> > > > > > > >
> > > > > > > > --
> > > > > > > >
> > > > > > > > DENX Software Engineering GmbH, Managing Director:
> > > > > > > > Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5,
> > > > > > > > D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59
> > > > > > > > Fax: (+49)-8142-66989-80 Email: lukma at denx.de
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Best regards,
> > > > > >
> > > > > > Lukasz Majewski
> > > > > >
> > > > > > --
> > > > > >
> > > > > > DENX Software Engineering GmbH, Managing Director:
> > > > > > Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5,
> > > > > > D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
> > > > > > (+49)-8142-66989-80 Email: lukma at denx.de
> > > >
> > > >
> > > >
> > > >
> > > > Best regards,
> > > >
> > > > Lukasz Majewski
> > > >
> > > > --
> > > >
> > > > DENX Software Engineering GmbH, Managing Director: Wolfgang
> > > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> > > > Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
> > > > (+49)-8142-66989-80 Email: lukma at denx.de
> >
> >
> >
> >
> > Best regards,
> >
> > Lukasz Majewski
> >
> > --
> >
> > DENX Software Engineering GmbH, Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma at denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190617/5d4d3e39/attachment.sig>
More information about the U-Boot
mailing list