[v6] eeprom: at24: Add OF device ID table

Message ID 20171001104948.21663-1-javierm@redhat.com
State Under Review
Headers show
Series
  • [v6] eeprom: at24: Add OF device ID table
Related show

Commit Message

Javier Martinez Canillas Oct. 1, 2017, 10:49 a.m.
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(-)

Comments

Wolfram Sang Oct. 17, 2017, 9:40 p.m. | #1
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
Rob Herring Oct. 17, 2017, 9:43 p.m. | #2
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
Wolfram Sang Oct. 17, 2017, 9:47 p.m. | #3
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!
Geert Uytterhoeven Oct. 18, 2017, 6:39 a.m. | #4
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

Patch

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,