| Message ID | 20171001104948.21663-1-javierm@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v6] eeprom: at24: Add OF device ID table | expand |
Hi, > + { > + .compatible = "atmel,spd", > + .data = (void *)AT24_DEVICE_MAGIC(2048 / 8, > + AT24_FLAG_READONLY | AT24_FLAG_IRUGO) > + }, checkpatch reported this one as un-documented. And come to think of it, since this is solely for EEPROMs on RAM modules, I think we can drop a DT binding for it. Could you agree? I can do it locally, no need to resend. I'll do HW testing later, but wanted to check on your opinion already. Thanks, Wolfram
On Tue, Oct 17, 2017 at 4:40 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > Hi, > >> + { >> + .compatible = "atmel,spd", >> + .data = (void *)AT24_DEVICE_MAGIC(2048 / 8, >> + AT24_FLAG_READONLY | AT24_FLAG_IRUGO) >> + }, > > checkpatch reported this one as un-documented. And come to think of it, > since this is solely for EEPROMs on RAM modules, I think we can drop a > DT binding for it. Could you agree? I can do it locally, no need to > resend. I'll do HW testing later, but wanted to check on your opinion > already. It is used by arch/powerpc/boot/dts/mpc8349emitx.dts. Rob
On Tue, Oct 17, 2017 at 04:43:14PM -0500, Rob Herring wrote: > On Tue, Oct 17, 2017 at 4:40 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > > Hi, > > > >> + { > >> + .compatible = "atmel,spd", > >> + .data = (void *)AT24_DEVICE_MAGIC(2048 / 8, > >> + AT24_FLAG_READONLY | AT24_FLAG_IRUGO) > >> + }, > > > > checkpatch reported this one as un-documented. And come to think of it, > > since this is solely for EEPROMs on RAM modules, I think we can drop a > > DT binding for it. Could you agree? I can do it locally, no need to > > resend. I'll do HW testing later, but wanted to check on your opinion > > already. > > It is used by arch/powerpc/boot/dts/mpc8349emitx.dts. Thanks!
Hi Wolfram, On Tue, Oct 17, 2017 at 11:40 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> + { >> + .compatible = "atmel,spd", >> + .data = (void *)AT24_DEVICE_MAGIC(2048 / 8, >> + AT24_FLAG_READONLY | AT24_FLAG_IRUGO) >> + }, > > checkpatch reported this one as un-documented. And come to think of it, > since this is solely for EEPROMs on RAM modules, I think we can drop a > DT binding for it. Could you agree? I can do it locally, no need to > resend. I'll do HW testing later, but wanted to check on your opinion > already. Why? No one has placeholders for memory modules in DT? In addition, I can imagine a DT overlay for a memory module, adding the memory node and the EEPROM. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
On Tue, Oct 17, 2017 at 11:40 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > Hi, > >> + { >> + .compatible = "atmel,spd", >> + .data = (void *)AT24_DEVICE_MAGIC(2048 / 8, >> + AT24_FLAG_READONLY | AT24_FLAG_IRUGO) >> + }, > > checkpatch reported this one as un-documented. And come to think of it, > since this is solely for EEPROMs on RAM modules, I think we can drop a > DT binding for it. Could you agree? I can do it locally, no need to As mentioned by Rob, it's already used by a DTS in mainline. I think the problem is in how the DT binding for this driver is described. Most DT bindings describes the complete list of compatible strings supported by a driver and that's what checkpatch expects. But Documentation/devicetree/bindings/eeprom/eeprom.txt describes it as the cartesian product of the atmel (and also the deprecated at and at24) manufacturer and a list of devices. To save you a lookup: If there is no specific driver for <manufacturer>, a generic device with <type> and manufacturer "atmel" should be used. Possible types are: "24c00", "24c01", "24c02", "24c04", "24c08", "24c16", "24c32", "24c64", "24c128", "24c256", "24c512", "24c1024", "spd" > resend. I'll do HW testing later, but wanted to check on your opinion > already. > Great, thanks! I hope I got it right this time. Adding a DTS snippet I can see that the entry .data is correctly used but I don't a device to check if is working correctly after $SUBJECT. > Thanks, > > Wolfram >
On Sun, Oct 01, 2017 at 12:49:48PM +0200, Javier Martinez Canillas wrote: > The driver doesn't have a struct of_device_id table but supported devices > are registered via Device Trees. This is working on the assumption that a > I2C device registered via OF will always match a legacy I2C device ID and > that the MODALIAS reported will always be of the form i2c:<device>. > > But this could change in the future so the correct approach is to have an > OF device ID table if the devices are registered via OF. > > To maintain backward compatibility with old Device Trees, only use the OF > device ID table .data if the device was registered via OF and the OF node > compatible matches an entry in the OF device ID table. > > Suggested-by: Wolfram Sang <wsa@the-dreams.de> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Applied to for-next, thanks for keeping at it!
Hello Wolfram, On 11/05/2017 10:53 PM, Wolfram Sang wrote: > On Sun, Oct 01, 2017 at 12:49:48PM +0200, Javier Martinez Canillas wrote: >> The driver doesn't have a struct of_device_id table but supported devices >> are registered via Device Trees. This is working on the assumption that a >> I2C device registered via OF will always match a legacy I2C device ID and >> that the MODALIAS reported will always be of the form i2c:<device>. >> >> But this could change in the future so the correct approach is to have an >> OF device ID table if the devices are registered via OF. >> >> To maintain backward compatibility with old Device Trees, only use the OF >> device ID table .data if the device was registered via OF and the OF node >> compatible matches an entry in the OF device ID table. >> >> Suggested-by: Wolfram Sang <wsa@the-dreams.de> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> > > Applied to for-next, thanks for keeping at it! > Great, thanks a lot for your feedback and suggestions on this series. Best regards,
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index 764ff5df0dbc..0fbb19bc54aa 100644 --- a/drivers/misc/eeprom/at24.c +++ b/drivers/misc/eeprom/at24.c @@ -12,6 +12,7 @@ #include <linux/kernel.h> #include <linux/init.h> #include <linux/module.h> +#include <linux/of_device.h> #include <linux/slab.h> #include <linux/delay.h> #include <linux/mutex.h> @@ -175,6 +176,64 @@ static const struct i2c_device_id at24_ids[] = { }; MODULE_DEVICE_TABLE(i2c, at24_ids); +static const struct of_device_id at24_of_match[] = { + { + .compatible = "atmel,24c00", + .data = (void *)AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) + }, + { + .compatible = "atmel,24c01", + .data = (void *)AT24_DEVICE_MAGIC(1024 / 8, 0) + }, + { + .compatible = "atmel,24c02", + .data = (void *)AT24_DEVICE_MAGIC(2048 / 8, 0) + }, + { + .compatible = "atmel,spd", + .data = (void *)AT24_DEVICE_MAGIC(2048 / 8, + AT24_FLAG_READONLY | AT24_FLAG_IRUGO) + }, + { + .compatible = "atmel,24c04", + .data = (void *)AT24_DEVICE_MAGIC(4096 / 8, 0) + }, + { + .compatible = "atmel,24c08", + .data = (void *)AT24_DEVICE_MAGIC(8192 / 8, 0) + }, + { + .compatible = "atmel,24c16", + .data = (void *)AT24_DEVICE_MAGIC(16384 / 8, 0) + }, + { + .compatible = "atmel,24c32", + .data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c64", + .data = (void *)AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c128", + .data = (void *)AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c256", + .data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c512", + .data = (void *)AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c1024", + .data = (void *)AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16) + }, + { }, +}; +MODULE_DEVICE_TABLE(of, at24_of_match); + static const struct acpi_device_id at24_acpi_ids[] = { { "INT3499", AT24_DEVICE_MAGIC(8192 / 8, 0) }, { } @@ -598,7 +657,16 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) if (client->dev.platform_data) { chip = *(struct at24_platform_data *)client->dev.platform_data; } else { - if (id) { + /* + * The I2C core allows OF nodes compatibles to match against the + * I2C device ID table as a fallback, so check not only if an OF + * node is present but also if it matches an OF device ID entry. + */ + if (client->dev.of_node && + of_match_device(at24_of_match, &client->dev)) { + magic = (kernel_ulong_t) + of_device_get_match_data(&client->dev); + } else if (id) { magic = id->driver_data; } else { const struct acpi_device_id *aid; @@ -814,6 +882,7 @@ static int at24_remove(struct i2c_client *client) static struct i2c_driver at24_driver = { .driver = { .name = "at24", + .of_match_table = at24_of_match, .acpi_match_table = ACPI_PTR(at24_acpi_ids), }, .probe = at24_probe,
The driver doesn't have a struct of_device_id table but supported devices are registered via Device Trees. This is working on the assumption that a I2C device registered via OF will always match a legacy I2C device ID and that the MODALIAS reported will always be of the form i2c:<device>. But this could change in the future so the correct approach is to have an OF device ID table if the devices are registered via OF. To maintain backward compatibility with old Device Trees, only use the OF device ID table .data if the device was registered via OF and the OF node compatible matches an entry in the OF device ID table. Suggested-by: Wolfram Sang <wsa@the-dreams.de> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- Wolfram, This version shouldn't cause a regression if is applied before the DT changes, that's why I'm not posting as a part of a series like in the previous versions. It also maintains backward compatibility, since as pointed out by Geert, old DTB should be supported. The driver will only attempt to use the OF entry's .data if the device was registered via OF *and* was matched using the OF device ID table. Best regards, Javier Changes in v6: - Check if the OF node matches an OF device ID table entry or if it was matched against the I2C device ID table to know from where to get the data. This will maintain backward copatibility for old DTBs (Geert Uytterhoeven). Changes in v5: - None Changes in v4: - None Changes in v3: - Fix wrong .data values for "atmel,24c02" and "atmel,24c64" entries. Changes in v2: - Only add a single OF device ID entry for each device type (Wolfram Sang). drivers/misc/eeprom/at24.c | 71 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-)