Hi Heiko, > Hello Chuanhua Han, > > Am 22.05.2019 um 14:45 schrieb Chuanhua Han: > > > > > >> -----Original Message----- > >> From: Lukasz Majewski <[email protected]> > >> Sent: 2019年5月22日 19:32 > >> To: Chuanhua Han <[email protected]> > >> Cc: [email protected]; [email protected]; Biwen Li <[email protected]>; > >> [email protected]; Stefano Babic <[email protected]> > >> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read > >> wrong time > >> > >> On Wed, 22 May 2019 09:31:35 +0000 > >> Chuanhua Han <[email protected]> wrote: > >> > >>>> -----Original Message----- > >>>> From: Lukasz Majewski <[email protected]> > >>>> Sent: 2019年5月22日 16:41 > >>>> To: Chuanhua Han <[email protected]> > >>>> Cc: [email protected]; [email protected]; Biwen Li > >>>> <[email protected]>; [email protected]; Stefano Babic > >>>> <[email protected]> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: > >>>> fix bug that read wrong time > >>>> > >>>> Hi Chuanhua, > >>>> > >>>>>> -----Original Message----- > >>>>>> From: Lukasz Majewski <[email protected]> > >>>>>> Sent: 2019年5月22日 15:16 > >>>>>> To: Chuanhua Han <[email protected]> > >>>>>> Cc: [email protected]; [email protected]; Biwen Li > >>>>>> <[email protected]>; [email protected] > >>>>>> Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read > >>>>>> wrong time > >>>>>> > >>>>>> Hi Chuanhua, > >>>>>> > >>>>>>> Because i2c driver does not generate a stop bit when reading > >>>>>>> registers of pcf2127 > >>>>>> > >>>>>> The change (in the common i2c code) is rather large when > >>>>>> considering the description above. > >>>>>> > >>>>>> Could you write a more detailed patch description? Is this only > >>>>>> the problem with i2c in DM? > >>>>> OK, This is a problem with the i2c transport framework. After > >>>>> writing the register address that you want to read, the stop > >>>>> signal is missing before reading the register data. > >>>>>> > >>>>>> Is the same code (as introduced in this commit) available in > >>>>>> Linux kernel? > >>>>> The kernel does not have such a problem > >>>> > >>>> Do you know maybe why such issue is not present on Linux > >>>> kernel? > >>> The kernel code is encapsulated when reading the pcf2127 register, > >>> which separates the read and write, thus generating a stop signal > >>> before reading the register. Eg: Here is the wrapper made by the > >>> kernel code: static int pcf2127_i2c_read(void *context, const void > >>> *reg, size_t reg_size, void *val, size_t val_size) { > >>> struct device *dev = context; > >>> struct i2c_client *client = to_i2c_client(dev); > >>> int ret; > >>> > >>> if (WARN_ON(reg_size != 1)) > >>> return -EINVAL; > >>> > >>> ret = i2c_master_send(client, reg, 1); > >>> if (ret != 1) > >>> return ret < 0 ? ret : -EIO; > >>> > >>> ret = i2c_master_recv(client, val, val_size); > >>> if (ret != val_size) > >>> return ret < 0 ? ret : -EIO; > >>> > >>> return 0; > >>> } > >> > >> That was my point - the same shall be done in the pcf2127 driver. > >> This is a slow device, so we can afford for this. > > There is no similar api in the uboot code to split the read data > > into the address of the send register and receive the data. Uboot > > encapsulates the transmit register address and the read data, so no > > stop signal is generated. > > If this API is missing, please introduce it. But not in one big patch > instead split it into: > > - introduce I2C_M_RD_NEED_STOP_BIT > > - support flag I2C_M_RD_NEED_STOP_BIT in driver drivers/i2c/mxc_i2c.c > > or may we need a more common approach to this? > > - pcf2127 driver changes > > Also, as Lukasz mentioned, provide commit messages with more > information, what you do and why.
Verbose commit messages help with understanding the _real_ problem
being solved by the patch. Also are helpful in the future as a
documentation and reference.
>
> >>
> >>>>
> >>>>>> How the error is reproduced? What are the symptoms of it?
> >>>>> You can use the i2c command to read the register data of
> >>>>> pcf2127.
> >>>>
> >>>> So the problem is with using "i2c ..." commands from U-Boot
> >>>> prompt?
> >>> Yes,but adding debugging to the rtc driver is also the same
> >>> problem
> >>>>
> >>>>> You will find that you want to read the address 0 of the
> >>>>> register, but the data of the 1 address is read, and the data
> >>>>> read later will be offset.
> >>>>
> >>>> As fair as I can tell the pcf2127 has its own DM aware driver at
> >>>> driver/rtc/pcf2127.c.
> >>>>
> >>>> Is this driver broken so you need to modify the generic i.MX I2C
> >>>> code? Have you tried to test this driver on your setup?
> >>> Pcf2127 driver also has problems, has been modified, need i2c
> >>> general code to support
> >>
> >> Just one remark the mxc_i2c.c is IMX specific I2C code. Not the
> >> generic one.
> > ok
> >>
> >> Moreover, it looks like the same approach (first send, then read)
> >> is performed in the pcf2127 driver at pcf2127_rtc_get() function.
> >>
> >> I think that the driver shall be first thoroughly checked, then
> >> fixes shall be added to it.
>
> I have no such device, so hard to say ... and as Lukasz alread
> mentioned the driver seems to make such an approach:
>
> 52 static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time
> *tm) 53 {
> 54 int ret = 0;
> 55 uchar buf[10] = { PCF2127_REG_CTRL1 };
> 56
> 57 ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
> 58 if (ret < 0)
> 59 return ret;
> 60 ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf,
> sizeof(buf)); 61 if (ret < 0)
> 62 return ret;
>
I would prefer to fix the issue in the driver itself. Only when it is
not possible we shall introduce extra flags and modify the common I2C
code.
>
> It seems there are currently no real users of this driver:
>
> pollux:u-boot hs [master] $ grep -lr RTC_PCF2127 .
> ./drivers/rtc/Kconfig
> ./drivers/rtc/Makefile
> pollux:u-boot hs [master] $
>
> I added Meng Yi to cc, as he is the author of this driver. May he
> can say here more... at last I hope, the driver worked for him.
>
> bye,
> Heiko
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: [email protected]
pgpQTqjAFPFEX.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
