| Message ID | 20170314151638.23132-5-javier@osg.samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Mar 14, 2017 at 5:16 PM, Javier Martinez Canillas <javier@osg.samsung.com> 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. I'm bot sure this patch does something useful right now. Can we survive without it? I think we may. > drivers/misc/eeprom/at24.c | 189 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 188 insertions(+), 1 deletion(-) It's a huge! It will increase not only driver code base but memory footprint for almost no benefit.
Hello Andy, On 03/14/2017 07:59 PM, Andy Shevchenko wrote: > On Tue, Mar 14, 2017 at 5:16 PM, Javier Martinez Canillas > <javier@osg.samsung.com> 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. > > I'm bot sure this patch does something useful right now. Can we > survive without it? I think we may. > Yes, we can survive without it for now. But the problem is that with current I2C core, DT-only I2C drivers must have an I2C device table in order to have module auto-load working. That's because the core always reports as modalias i2c:<foo> regardless if the device was registered via DT or legacy mechanism. And some maintainers don't accept patches doing this duplication and instead ask for the core to be fixed, i.e [0]. But to make sure that fixing the core won't add regressions in drivers that are relying in the current behavior, patches like $SUBJECT are needed. So there isn't an agreement if is better to just rely in the current behavior (and have a superfluous I2C device ID table) or fix the I2C core (and need a OF device ID table). I personally prefer the latter since that means that an DT-only driver will only need a OF table, and only drivers that supports both will need both tables. >> drivers/misc/eeprom/at24.c | 189 ++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 188 insertions(+), 1 deletion(-) > > It's a huge! It will increase not only driver code base but memory > footprint for almost no benefit. > Indeed, but these all are compatible strings used by DTS in mainline and so should be in the OF device ID table in order to be matched and the proper modalias reported (once the I2C core is fixed). One option is to add #ifdef CONFIG_OF guards for the OF device table definition but again there's no agreement on that one since some maintainers say the it is better to always build the OF ID table than having #ifdefery in C code... [0]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1321026.html Best regards,
> So there isn't an agreement if is better to just rely in the current behavior > (and have a superfluous I2C device ID table) or fix the I2C core (and need a > OF device ID table). For at24, the i2c_device_id table is not superfluous! It is used outside the DT world as well. > Indeed, but these all are compatible strings used by DTS in mainline and so > should be in the OF device ID table in order to be matched and the proper > modalias reported (once the I2C core is fixed). I'd think we should fix the DTS files instead to contain a fallback we agree on. Say, we agree on "atmel,at24c01" as a the generic fallback, the DTS should contain: compatible = "<your_vendor>,<your_type>", "atmel,at24c01" And we shall only keep compatible values in the source file which differ in behaviour fromt the generic case. > One option is to add #ifdef CONFIG_OF guards for the OF device table definition > but again there's no agreement on that one since some maintainers say the it is > better to always build the OF ID table than having #ifdefery in C code... I don't like the #ifdeffery as well.
Hello Wolfram, On 03/15/2017 04:58 AM, Wolfram Sang wrote: > >> So there isn't an agreement if is better to just rely in the current behavior >> (and have a superfluous I2C device ID table) or fix the I2C core (and need a >> OF device ID table). > > For at24, the i2c_device_id table is not superfluous! It is used outside > the DT world as well. > Yes, I know. I was trying to explain to Andy what's the problem that I want to solve in general, not talking about this particular driver. Sorry if that was confusing. >> Indeed, but these all are compatible strings used by DTS in mainline and so >> should be in the OF device ID table in order to be matched and the proper >> modalias reported (once the I2C core is fixed). > > I'd think we should fix the DTS files instead to contain a fallback we > agree on. Say, we agree on "atmel,at24c01" as a the generic fallback, > the DTS should contain: > > compatible = "<your_vendor>,<your_type>", "atmel,at24c01" > > And we shall only keep compatible values in the source file which differ > in behaviour fromt the generic case. > Do you know who that's familiar with this device and driver can do this? I've been trying to fix all the drivers that are relying on having an I2C ID table but whose devices are registered via DT and I've now posted patches for all. I wanted to do that so this patch can finally land [0] but to be honest I'm about to give up on this... it seems is causing a lot of churn to maintainers and many don't see the benefit on having the I2C core to report a proper OF modalias, and don't think the current behavior is particularly bad. Unfortunately some maintainers do and don't accept patches adding I2C tables only to have module autoloading working so I still think it should be fixed. [0]: https://patchwork.kernel.org/patch/6903991/ Best regards,
On Wed, Mar 15, 2017 at 12:58 PM, Javier Martinez Canillas <javier@osg.samsung.com> wrote: > On 03/15/2017 04:58 AM, Wolfram Sang wrote: > Unfortunately some maintainers do and don't accept patches adding I2C tables > only to have module autoloading working so I still think it should be fixed. Wait, how does it work for now?! Sounds for me you are trying to solve non-existing issue. > [0]: https://patchwork.kernel.org/patch/6903991/
Hello Andy, On 03/15/2017 08:21 AM, Andy Shevchenko wrote: > On Wed, Mar 15, 2017 at 12:58 PM, Javier Martinez Canillas > <javier@osg.samsung.com> wrote: >> On 03/15/2017 04:58 AM, Wolfram Sang wrote: > >> Unfortunately some maintainers do and don't accept patches adding I2C tables >> only to have module autoloading working so I still think it should be fixed. > > Wait, how does it work for now?! > It only works if you have an I2C device ID table, but that may not be the case for DT-only drivers that could only have an OF device ID table. In the latter case module autoload won't work. > Sounds for me you are trying to solve non-existing issue. > It's an existing issue. You _must_ have an I2C device ID table if you want to autload a device driver which is superfluous for DT-only drivers. In other words, if you register an I2C device using OF the modalias will be: $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias i2c:maxtouch While the correct thing to report should be: $ cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias of:NtrackpadT<NULL>Catmel,maxtouch >> [0]: https://patchwork.kernel.org/patch/6903991/ > Best regards,
On Wed, Mar 15, 2017 at 1:39 PM, Javier Martinez Canillas <javier@osg.samsung.com> wrote: > Hello Andy, > > On 03/15/2017 08:21 AM, Andy Shevchenko wrote: >> On Wed, Mar 15, 2017 at 12:58 PM, Javier Martinez Canillas >> <javier@osg.samsung.com> wrote: >>> On 03/15/2017 04:58 AM, Wolfram Sang wrote: >> >>> Unfortunately some maintainers do and don't accept patches adding I2C tables >>> only to have module autoloading working so I still think it should be fixed. >> >> Wait, how does it work for now?! > It only works if you have an I2C device ID table, but that may not be the case > for DT-only drivers that could only have an OF device ID table. In the latter > case module autoload won't work. OK. >> Sounds for me you are trying to solve non-existing issue. > It's an existing issue. You _must_ have an I2C device ID table if you want to > autload a device driver which is superfluous for DT-only drivers. Okay, can you scope only affected drivers then? Looking to spread patches from you over all drivers I dunno they are all affected right now. P.S. Personally I agree with maintainers who do *not* apply this. Sorry.
Hello Andy, On 03/15/2017 07:43 PM, Andy Shevchenko wrote: > On Wed, Mar 15, 2017 at 1:39 PM, Javier Martinez Canillas > <javier@osg.samsung.com> wrote: >> Hello Andy, >> >> On 03/15/2017 08:21 AM, Andy Shevchenko wrote: >>> On Wed, Mar 15, 2017 at 12:58 PM, Javier Martinez Canillas >>> <javier@osg.samsung.com> wrote: >>>> On 03/15/2017 04:58 AM, Wolfram Sang wrote: >>> >>>> Unfortunately some maintainers do and don't accept patches adding I2C tables >>>> only to have module autoloading working so I still think it should be fixed. >>> >>> Wait, how does it work for now?! > >> It only works if you have an I2C device ID table, but that may not be the case >> for DT-only drivers that could only have an OF device ID table. In the latter >> case module autoload won't work. > > OK. > >>> Sounds for me you are trying to solve non-existing issue. > >> It's an existing issue. You _must_ have an I2C device ID table if you want to >> autload a device driver which is superfluous for DT-only drivers. > > Okay, can you scope only affected drivers then? Looking to spread > patches from you over all drivers I dunno they are all affected right > now. > That's what I did. I've only posted patches for drivers that have DT support but don't have an OF device ID table since module autoload will be broken for those if the I2C core is fixed to report a proper OF modalias. And drivers/misc/eeprom/at24.c is one of those drivers. > P.S. Personally I agree with maintainers who do *not* apply this. Sorry. > So what's your suggestion to solve the issue then? When I said that some maintainers don't want a superfluous device table to be added I was talking about I2C device ID table for DT-only drivers, but $SUBJECT is the opposite. I've the impression that you are nacking $SUBJECT without fully understanding what the problem is and how this patch + the patch for I2C core are fixing it. Best regards,
> > I'd think we should fix the DTS files instead to contain a fallback we > > agree on. Say, we agree on "atmel,at24c01" as a the generic fallback, > > the DTS should contain: > > > > compatible = "<your_vendor>,<your_type>", "atmel,at24c01" > > > > And we shall only keep compatible values in the source file which differ > > in behaviour fromt the generic case. > > > > Do you know who that's familiar with this device and driver can do this? I've Sorry, I don't understand your question. What do you mean?
Hello Wolfram, On 03/16/2017 10:07 AM, Wolfram Sang wrote: > >>> I'd think we should fix the DTS files instead to contain a fallback we >>> agree on. Say, we agree on "atmel,at24c01" as a the generic fallback, >>> the DTS should contain: >>> >>> compatible = "<your_vendor>,<your_type>", "atmel,at24c01" >>> >>> And we shall only keep compatible values in the source file which differ >>> in behaviour fromt the generic case. >>> >> >> Do you know who that's familiar with this device and driver can do this? I've > > Sorry, I don't understand your question. What do you mean? > Sorry, for not explaining myself correctly. I meant to ask who can do what you suggested before. I'm certainly not familiar with this driver to identify what is the minimum set of compatible strings that can be used as generic fallback. Best regards,
> Sorry, for not explaining myself correctly. I meant to ask who can do what you > suggested before. I'm certainly not familiar with this driver to identify what > is the minimum set of compatible strings that can be used as generic fallback. Well, I am the maintainer of this driver :) But we should definately get Rob into the boat if he is OK with updating all DTS files having such an EEPROM.
Hello Wolfram, On 03/16/2017 10:36 AM, Wolfram Sang wrote: > >> Sorry, for not explaining myself correctly. I meant to ask who can do what you >> suggested before. I'm certainly not familiar with this driver to identify what >> is the minimum set of compatible strings that can be used as generic fallback. > > Well, I am the maintainer of this driver :) But we should definately get Oh right, silly me :) > Rob into the boat if he is OK with updating all DTS files having such an > EEPROM. > Agreed, are you going to take care of that? To be honest I think I'll just give up on this task, it has been a big time sink and I had to explain over and over to different people what the problem is with the I2C modalias uevent reporting. I've posted patches for all the drivers that could be affected when reporting a proper OF modalias by the core and also the RFC patch to properly report it [0]. But it seems that for many maintainers this is just an unnecessary churn and they don't think there's an issue with the current behaviour. So it feels I'm causing more harm than good by keep pushing this. [0]: https://lkml.org/lkml/2015/7/30/494 Best regards,
> > Rob into the boat if he is OK with updating all DTS files having such an > > EEPROM. > > > > Agreed, are you going to take care of that? Nope, sorry, no bandwidth. You might ask Lee, he was very interested in getting proper I2C OF support upstream. > up on this task, it has been a big time sink and I had to explain over and over > to different people what the problem is with the I2C modalias uevent reporting. Next time, maybe do a wiki page and point people to it? That being said, we should probably create a wiki page on the I2C wiki anyhow. Documenting the current state of affairs. That I would do when I finally get around to brush up I2C wiki. > But it seems that for many maintainers this is just an unnecessary churn and they > don't think there's an issue with the current behaviour. So it feels I'm causing > more harm than good by keep pushing this. I understand somehow. They probably were reluctant to change something that is working, even if it is not pretty. I'm not saying it's not worth it, yet one needs energy and motivation to push it through.
Hello Wolfram, On 03/16/2017 12:05 PM, Wolfram Sang wrote: > >>> Rob into the boat if he is OK with updating all DTS files having such an >>> EEPROM. >>> >> >> Agreed, are you going to take care of that? > > Nope, sorry, no bandwidth. You might ask Lee, he was very interested in > getting proper I2C OF support upstream. > Ok, understandable. Adding Lee and Kieran to cc who were also interested on this. >> up on this task, it has been a big time sink and I had to explain over and over >> to different people what the problem is with the I2C modalias uevent reporting. > > Next time, maybe do a wiki page and point people to it? That being said, > we should probably create a wiki page on the I2C wiki anyhow. > Documenting the current state of affairs. That I would do when I finally > get around to brush up I2C wiki. > Agreed, I can help writing such a wiki page if you want. I've already requested for an account at https://i2c.wiki.kernel.org. >> But it seems that for many maintainers this is just an unnecessary churn and they >> don't think there's an issue with the current behaviour. So it feels I'm causing >> more harm than good by keep pushing this. > > I understand somehow. They probably were reluctant to change something Yes, I don't blame them. It's kind of corner case that most people don't hit it. One problem though is that this implementation detail leaks into the DTS and DT binding documents, as we saw people using compatible strings without a vendor prefix just because they could. > that is working, even if it is not pretty. I'm not saying it's not worth > it, yet one needs energy and motivation to push it through. > Yeah, I think I've the energy and motivation but unfortunately also not enough time :) And likely to have even less time in the near future. Best regards,
Hello, On 03/16/2017 12:39 PM, Javier Martinez Canillas wrote: > On 03/16/2017 12:05 PM, Wolfram Sang wrote: [snip] >> >> Next time, maybe do a wiki page and point people to it? That being said, >> we should probably create a wiki page on the I2C wiki anyhow. >> Documenting the current state of affairs. That I would do when I finally >> get around to brush up I2C wiki. >> > > Agreed, I can help writing such a wiki page if you want. I've already requested for > an account at https://i2c.wiki.kernel.org. > FYI, I've added a page to the wiki explaining the current issues with the I2C core MODALIAS uevent reporting and OF match: https://i2c.wiki.kernel.org/index.php/OF_Modalias And also linked to the main page in the work-in-progress section: https://i2c.wiki.kernel.org/index.php/Main_Page#Work_in_progress Please feel free to add/remove/change anything that you think is missing or isn't correct for you. Best regards,
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index 764ff5df0dbc..b67b804855f9 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,188 @@ 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 = "at24,24c02", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_SERIAL | AT24_FLAG_READONLY) + }, + { + .compatible = "atmel,24c02", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_SERIAL | AT24_FLAG_READONLY) + }, + { + .compatible = "microchip,24c02", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_SERIAL | AT24_FLAG_READONLY) + }, + { + .compatible = "nxp,24c02", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_SERIAL | AT24_FLAG_READONLY) + }, + { + .compatible = "renesas,24c02", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_SERIAL | AT24_FLAG_READONLY) + }, + { + .compatible = "renesas,r1ex24002", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_SERIAL | AT24_FLAG_READONLY) + }, + { + .compatible = "at24,spd", + .data = (void *)AT24_DEVICE_MAGIC(2048 / 8, + AT24_FLAG_READONLY | AT24_FLAG_IRUGO) + }, + { + .compatible = "at,24c04", + .data = (void *)AT24_DEVICE_MAGIC(4096 / 8, 0) + }, + { + .compatible = "at24,24c04", + .data = (void *)AT24_DEVICE_MAGIC(4096 / 8, 0) + }, + { + .compatible = "atmel,24c04", + .data = (void *)AT24_DEVICE_MAGIC(4096 / 8, 0) + }, + { + .compatible = "at,24c08", + .data = (void *)AT24_DEVICE_MAGIC(8192 / 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 = "at,24c32", + .data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "at24,24c32", + .data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c32", + .data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "catalyst,24c32", + .data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "microchip,24c32", + .data = (void *)AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "at,24c64", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_ADDR16 | + AT24_FLAG_SERIAL | + AT24_FLAG_READONLY) + }, + { + .compatible = "at24,24c64", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_ADDR16 | + AT24_FLAG_SERIAL | + AT24_FLAG_READONLY) + }, + { + .compatible = "atmel,24c64", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_ADDR16 | + AT24_FLAG_SERIAL | + AT24_FLAG_READONLY) + }, + { + .compatible = "microchip,24c64", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_ADDR16 | + AT24_FLAG_SERIAL | + AT24_FLAG_READONLY) + }, + { + .compatible = "ramtron,24c64", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_ADDR16 | + AT24_FLAG_SERIAL | + AT24_FLAG_READONLY) + }, + { + .compatible = "st,24c64", + .data = (void *)AT24_DEVICE_MAGIC(16, + AT24_FLAG_ADDR16 | + AT24_FLAG_SERIAL | + AT24_FLAG_READONLY) + }, + { + .compatible = "atmel,24c128", + .data = (void *)AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "renesas,24c128", + .data = (void *)AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "at,24c256", + .data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "at24,24c256", + .data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c256", + .data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "st,24c256", + .data = (void *)AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "at24,24c512", + .data = (void *)AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c512", + .data = (void *)AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "microchip,24c512", + .data = (void *)AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "at,24c1024", + .data = (void *)AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "atmel,24c1024", + .data = (void *)AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16) + }, + { + .compatible = "st,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 +781,10 @@ 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) { + if (client->dev.of_node) { + 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 +1000,7 @@ static int at24_remove(struct i2c_client *client) static struct i2c_driver at24_driver = { .driver = { .name = "at24", + .of_match_table = of_match_ptr(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. Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> --- Hello, I added in the OF device ID table all the manufacturer,model combinations that I found on either a DT bindings doc or Device Tree source files, but the Documentation/devicetree/bindings/eeprom/eeprom.txt docs says that: - compatible : should be "<manufacturer>,<type>", like these: And give some examples. I wonder if such a lax definition for compatibles is valid or if all possible combinations should be documented in the doc. I would had expected the latter and keep both the OF device ID table and bindings document updated as compatible EEPROM from a new vendor is used. Best regards, Javier drivers/misc/eeprom/at24.c | 189 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 188 insertions(+), 1 deletion(-)