Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
Hello Geert, On Wed, Aug 30, 2017 at 10:15 PM, Geert Uytterhoevenwrote: > Hi Javier, > > On Wed, Aug 30, 2017 at 9:57 PM, Javier Martinez Canillas > wrote: >>> I think we should talk about the same case: Let me repeat what I did: >>> >>> 1) I added your patch "eeprom: at24: Add OF device ID table" >>> 2) I added an EEPROM node to an I2C >>> >>> + eeprom@50 { >>> + compatible = "renesas,24c01"; >>> + reg = <0x50>; >>> + }; >>> >>> -> no at24 binding to the device >>> >>> 3) I revert your patch >>> >>> -> at24 binding to the device >>> >> >> I've tested this and you are right, it fails... >> >> The problem is that the patch also changes how the driver obtains the >> EEPROM parameters (the magic value in the entry's data field). >> >> So even when module autoload and device / driver matching works, the >> driver probe function fails because if (client->dev.of_node) the >> driver attempts to get the entry data using >> of_device_get_match_data(), which is obviously wrong since the >> compatible string in the dev node isn't present in the OF table. >> >> The id->driver_data from the I2C table should be used instead since >> that's the table that matches in this case. >> >> One option is to fallback to id->driver_data if >> of_device_get_match_data() fails, but that's just an (ugly) >> workaround. So I agree with you that the best option is to wait for >> the DTS patches to land first. > > Which means new kernels won't work with old DTBs. Oops... > I'm afraid that needs to be fixed. People care about DTB backward > compatibility on many platforms. > Right, I've yet to find one of those mythical platforms that ship old DTBs with new kernels, but I agree with you since people seem to care about backward compatibility (at least on theory). So I see two options then: 1) Use the workaround I mentioned and lookup the I2C device ID table entry data if of_device_get_match_data() fails 2) Only call of_device_get_match_data() if (dev->of_node && of_match_device(dev->driver->of_match_table, dev)) Not sure what's the preferred idiom for these cases. To good thing about keeping backward compatibility is that Wolfram would be able to pick the driver patch even before the DTS patches land. Best regards, Javier
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
Hi Javier, On Wed, Aug 30, 2017 at 9:57 PM, Javier Martinez Canillaswrote: >> I think we should talk about the same case: Let me repeat what I did: >> >> 1) I added your patch "eeprom: at24: Add OF device ID table" >> 2) I added an EEPROM node to an I2C >> >> + eeprom@50 { >> + compatible = "renesas,24c01"; >> + reg = <0x50>; >> + }; >> >> -> no at24 binding to the device >> >> 3) I revert your patch >> >> -> at24 binding to the device >> > > I've tested this and you are right, it fails... > > The problem is that the patch also changes how the driver obtains the > EEPROM parameters (the magic value in the entry's data field). > > So even when module autoload and device / driver matching works, the > driver probe function fails because if (client->dev.of_node) the > driver attempts to get the entry data using > of_device_get_match_data(), which is obviously wrong since the > compatible string in the dev node isn't present in the OF table. > > The id->driver_data from the I2C table should be used instead since > that's the table that matches in this case. > > One option is to fallback to id->driver_data if > of_device_get_match_data() fails, but that's just an (ugly) > workaround. So I agree with you that the best option is to wait for > the DTS patches to land first. Which means new kernels won't work with old DTBs. Oops... I'm afraid that needs to be fixed. People care about DTB backward compatibility on many platforms. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
> > I think we should talk about the same case: Let me repeat what I did: > > 1) I added your patch "eeprom: at24: Add OF device ID table" > 2) I added an EEPROM node to an I2C > > + eeprom@50 { > + compatible = "renesas,24c01"; > + reg = <0x50>; > + }; > > -> no at24 binding to the device > > 3) I revert your patch > > -> at24 binding to the device > I've tested this and you are right, it fails... The problem is that the patch also changes how the driver obtains the EEPROM parameters (the magic value in the entry's data field). So even when module autoload and device / driver matching works, the driver probe function fails because if (client->dev.of_node) the driver attempts to get the entry data using of_device_get_match_data(), which is obviously wrong since the compatible string in the dev node isn't present in the OF table. The id->driver_data from the I2C table should be used instead since that's the table that matches in this case. One option is to fallback to id->driver_data if of_device_get_match_data() fails, but that's just an (ugly) workaround. So I agree with you that the best option is to wait for the DTS patches to land first. It worked for me on my previous tests because the tested drivers didn't use a table entry data, I'm so sorry for missing this :( Best regards, Javier
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
On Wed, Aug 30, 2017 at 06:19:02PM +0200, Javier Martinez Canillas wrote: > Hello Wolfram, > > On Tue, Aug 29, 2017 at 10:48 AM, Wolfram Sangwrote: > > > >> I don't have a DT based system at hand now, but I'll test it again and > >> let you know probably tomorrow. > > > > I will try again today, too. Thanks! > > > > Ok, I had some time to do some tests again. I used an ARM Chromebook > (Exynos Peach Pi) that has an I2C touchpad (Atmel maXTouch). I tried again as well and it still fails for me. > Tested the following cases: I think we should talk about the same case: Let me repeat what I did: 1) I added your patch "eeprom: at24: Add OF device ID table" 2) I added an EEPROM node to an I2C + eeprom@50 { + compatible = "renesas,24c01"; + reg = <0x50>; + }; -> no at24 binding to the device 3) I revert your patch -> at24 binding to the device I think you should be able to test this DTS snipplet even without a real eeprom. Especially after applying this to the at24 driver. diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index 79c5c39be29cac..f9f547680c53db 100644 --- a/drivers/misc/eeprom/at24.c +++ b/drivers/misc/eeprom/at24.c @@ -805,11 +805,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) * Perform a one-byte test read to verify that the * chip is functional. */ - err = at24_read(at24, 0, _byte, 1); - if (err) { - err = -ENODEV; - goto err_clients; - } at24->nvmem_config.name = dev_name(>dev); at24->nvmem_config.dev = >dev; Can you check this? Thanks, Wolfram signature.asc Description: PGP signature
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
Hello Wolfram, On Tue, Aug 29, 2017 at 10:48 AM, Wolfram Sangwrote: > >> I don't have a DT based system at hand now, but I'll test it again and >> let you know probably tomorrow. > > I will try again today, too. Thanks! > Ok, I had some time to do some tests again. I used an ARM Chromebook (Exynos Peach Pi) that has an I2C touchpad (Atmel maXTouch). Tested the following cases: 1) Driver without OF device ID table (only a I2C table with a "maxtouch" entry) and DTS defining a device node with a "atmel,maxtouch" compatible string. This is the case without any of the patches in this series. $ modinfo drivers/input/touchscreen/atmel_mxt_ts.ko | grep maxtouch alias: i2c:maxtouch $ grep maxtouch /sys/devices/platform/soc/12e0.i2c/i2c-8/8-004b/uevent OF_COMPATIBLE_0=atmel,maxtouch MODALIAS=i2c:maxtouch 2) Driver without OF device ID table (only a I2C table with a "maxtouch" entry) and DTS defining a device node with a "atmel,maxtouch", "generic,maxtouch" compatible string. This is the case when platform maintainers merge the DTS patches without the driver patch. $ modinfo drivers/input/touchscreen/atmel_mxt_ts.ko | grep maxtouch alias: i2c:maxtouch $ grep maxtouch /sys/devices/platform/soc/12e0.i2c/i2c-8/8-004b/uevent OF_COMPATIBLE_0=atmel,maxtouch OF_COMPATIBLE_1=generic,maxtouch MODALIAS=i2c:maxtouch 3) Driver with an OF device ID table (with a "generic,maxtouch" entry) and DTS defining a device node with a "atmel,maxtouch" compatible string. This is the case when the driver patch is merged without the DTS patches. $ modinfo drivers/input/touchscreen/atmel_mxt_ts.ko | grep maxtouch alias: of:N*T*Cgeneric,maxtouchC* alias: of:N*T*Cgeneric,maxtouch alias: i2c:maxtouch $ grep maxtouch /sys/devices/platform/soc/12e0.i2c/i2c-8/8-004b/uevent OF_COMPATIBLE_0=atmel,maxtouch MODALIAS=i2c:maxtouch 4) Driver with an OF device ID table (with a "generic,maxtouch" entry) and DTS defining a device node with a "atmel,maxtouch", "generic,maxtouch" compatible string. This is the case when both the DTS and driver patches are merged. $ modinfo drivers/input/touchscreen/atmel_mxt_ts.ko | grep maxtouch alias: of:N*T*Cgeneric,maxtouchC* alias: of:N*T*Cgeneric,maxtouch alias: i2c:maxtouch $ grep maxtouch /sys/devices/platform/soc/12e0.i2c/i2c-8/8-004b/uevent OF_COMPATIBLE_0=atmel,maxtouch OF_COMPATIBLE_1=generic,maxtouch MODALIAS=i2c:maxtouch For all cases module autoload, driver probe and evtest worked for me. You said that (3) doesn't work but I don't understand why is failing for you. Probably I'm missing something. Best regards, Javier
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
> I don't have a DT based system at hand now, but I'll test it again and > let you know probably tomorrow. I will try again today, too. Thanks! signature.asc Description: PGP signature
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
Hello Wolfram, On Mon, Aug 28, 2017 at 6:01 PM, Wolfram Sangwrote: > >> > But there is a dependency, no? If I apply the driver patch, >> > non-converted device trees will not find their eeproms anymore. So, I >> >> I don't think that's correct. If you apply this patch before the DTS >> changes, the driver will still match using the I2C device ID table >> like it has been doing it until today. > > My tests do not confirm this. If I add a node with a "renesas,24c01" > compatible to my board, it works before your patch, but not after. If I > change it to "atmel,24c01" it works even after your patch. I haven't > looked into it, though, maybe i2c_of_match_device_sysfs() is stepping on > our foots here? > > Did you test and did it work for you? > I would swear that I tested both combinations (driver patch without DT changes and DTS changes without driver patch), but it was months ago when I first posted the patches so I may misremembering. I don't have a DT based system at hand now, but I'll test it again and let you know probably tomorrow. Best regards, Javier
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
> > But there is a dependency, no? If I apply the driver patch, > > non-converted device trees will not find their eeproms anymore. So, I > > I don't think that's correct. If you apply this patch before the DTS > changes, the driver will still match using the I2C device ID table > like it has been doing it until today. My tests do not confirm this. If I add a node with a "renesas,24c01" compatible to my board, it works before your patch, but not after. If I change it to "atmel,24c01" it works even after your patch. I haven't looked into it, though, maybe i2c_of_match_device_sysfs() is stepping on our foots here? Did you test and did it work for you? signature.asc Description: PGP signature
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
Hello Wolfram, On Mon, Jul 31, 2017 at 5:30 PM, Wolfram Sangwrote: > >> Patches can be applied independently since the DTS changes without driver >> changes are no-op and the OF table won't be used without the DTS changes. > > But there is a dependency, no? If I apply the driver patch, > non-converted device trees will not find their eeproms anymore. So, I I don't think that's correct. If you apply this patch before the DTS changes, the driver will still match using the I2C device ID table like it has been doing it until today. IOW, this is what will happen: 1- an OF device is registered with the wrong compatible (not found in the OF table) 2- the I2C core strips the vendor part and fills the struct i2c_client .name with the device part. 3- i2c_device_match() will be called since a new device has been registered 4- i2c_of_match_device() will fail because there's no OF entry that matches the device compatible 5- the I2C core fallbacks to i2c_match_id() and matches using the I2C device ID table. So no noticeable difference AFAICT in that case. Best regards, Javier
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
> Patches can be applied independently since the DTS changes without driver > changes are no-op and the OF table won't be used without the DTS changes. But there is a dependency, no? If I apply the driver patch, non-converted device trees will not find their eeproms anymore. So, I need to wait until all DTS patches are upstream, right? I can pick patch 1, though. We can already document it. signature.asc Description: PGP signature
Re: [RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
Hello Wolfram, On Thu, Jun 15, 2017 at 8:54 PM, Javier Martinez Canillaswrote: > > This series is a follow-up to patch [0] that added an OF device ID table > to the at24 EEPROM driver. As you suggested [1], this version instead of > adding entries for every used tuple, only adds a single > entry for each chip type using the "atmel" vendor as a generic fallback. > > The first patch documents in the DT binding what's the correct vendor to > use and what are the ones that are being deprecated. The second one adds > the OF device ID table for the at24 driver and the next patches use this > vendor in the compatible string to each DTS that defines a compatible I2C > EEPROM device node. > > Patches can be applied independently since the DTS changes without driver > changes are no-op and the OF table won't be used without the DTS changes. > > [0]: https://lkml.org/lkml/2017/3/14/589 > [1]: https://lkml.org/lkml/2017/3/15/99 > Are you planning to pick this series? It has been in the list for months and were resent many times... Best regards, Javier
[RESEND PATCH v5 00/16] eeprom: at24: Add OF device ID table
Hello Wolfram, This series is a follow-up to patch [0] that added an OF device ID table to the at24 EEPROM driver. As you suggested [1], this version instead of adding entries for every usedtuple, only adds a single entry for each chip type using the "atmel" vendor as a generic fallback. The first patch documents in the DT binding what's the correct vendor to use and what are the ones that are being deprecated. The second one adds the OF device ID table for the at24 driver and the next patches use this vendor in the compatible string to each DTS that defines a compatible I2C EEPROM device node. Patches can be applied independently since the DTS changes without driver changes are no-op and the OF table won't be used without the DTS changes. [0]: https://lkml.org/lkml/2017/3/14/589 [1]: https://lkml.org/lkml/2017/3/15/99 Best regards, Javier Changes in v5: - Only deprecate the atmel variants at25 and at (Geert Uytterhoeven). - Only replace atmel variant but keep other EEPROM vendors (Geert Uytterhoeven). - Only replace atmel variant but keep other EEPROM vendors (Geert Uytterhoeven). - Only replace atmel variant but keep other EEPROM vendors (Geert Uytterhoeven). - Only replace atmel variant but keep other EEPROM vendors (Geert Uytterhoeven). - Only replace atmel variant but keep other EEPROM vendors (Geert Uytterhoeven). - Only replace atmel variant but keep other EEPROM vendors (Geert Uytterhoeven). - Only replace atmel variant but keep other EEPROM vendors (Geert Uytterhoeven). - Only replace atmel variant but keep other EEPROM vendors (Geert Uytterhoeven). Changes in v4: - Document the manufacturers that have been deprecated (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). - Only use the atmel manufacturer in the compatible string instead of keeping the deprecated ones (Rob Herring). Changes in v3: - Fix wrong .data values for "atmel,24c02" and "atmel,24c64" entries. - Add Geert Uytterhoeven reviewed-by tag. - Add Geert Uytterhoeven reviewed-by tag. Changes in v2: - Only add a single OF device ID entry for each device type (Wolfram Sang). Javier Martinez Canillas (16): dt-bindings: i2c: eeprom: Document vendor to be used and deprecated ones eeprom: at24: Add OF device ID table ARM: dts: efm32: Add generic compatible string for I2C EEPROM ARM: dts: keystone: Add generic compatible string for I2C EEPROM ARM: dts: lpc18xx: Add generic compatible string for I2C EEPROM ARM: dts: r7s72100: Add generic compatible string for I2C EEPROM ARM: dts: koelsch: Add generic compatible string for I2C EEPROM ARM: dts: socfpga: Add generic compatible string for I2C EEPROM ARM: dts: uniphier: Add generic compatible string for I2C EEPROM ARM: dts: zynq: Add generic compatible string for I2C EEPROM arm64: zynqmp: Add generic compatible string for I2C EEPROM powerpc/5200: Add generic compatible string for I2C EEPROM powerpc/fsl: Add generic compatible string for I2C EEPROM powerpc/512x: Add generic compatible string for I2C EEPROM powerpc/83xx: Add generic compatible string for I2C EEPROM powerpc/44x: Add generic compatible string for I2C EEPROM .../devicetree/bindings/eeprom/eeprom.txt | 6 +- arch/arm/boot/dts/efm32gg-dk3750.dts | 2 +- arch/arm/boot/dts/keystone-k2e-evm.dts | 2 +- arch/arm/boot/dts/keystone-k2hk-evm.dts| 2 +- arch/arm/boot/dts/keystone-k2l-evm.dts | 2 +- arch/arm/boot/dts/lpc4337-ciaa.dts | 6 +-