| Message ID | 20190710193918.31135-1-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] modpost: Support I2C Aliases from OF tables | expand |
Hello Kieran, On 7/10/19 9:39 PM, Kieran Bingham wrote: > I2C drivers match against an I2C ID table, an OF table, and an ACPI > table. It is now also possible to match against an OF table entry > without the vendor prefix to support backwards compatibility, and allow > simplification of the i2c probe functions. > > As part of this matching, the probe function is being converted to > remove the need to specify the i2c_device_id table, but to support > module aliasing, we still require to have the MODULE_DEVICE_TABLE entry. > My opinion on this is that I2C drivers should register the tables of the firmware interfaces that support. That is, if a driver is only used in a platform that supports OF then it should only require an OF device table. But if the driver supports devices that can also be present in platforms that use ACPI, then should also require to have an ACPI device ID table. So there should be consistency about what table is used for both matching a device with a driver and reporting a modalias to user-space for module auto-loading. If a I2C device was instantiated by OF, then the OF table should be used for both reporting a modalias uevent and matching a driver. Now, the i2c_of_match_device() function attempts to match by first calling of_match_device() and if fails fallbacks to i2c_of_match_device_sysfs(). The latter attempts to match the I2C device by striping the vendor prefix of the compatible strings on the OF device ID table. That means that you could instantiate an I2C device ID through the sysfs interface and the OF table would be used to match the driver. But i2c_device_uevent() would had reported an I2C modalias and not an OF modalias (since the registered device won't have an associated of_node) so there's inconsistency in that case since a table is used to match but no to report modaliases. > Facilitate generating the I2C aliases directly from the of_device_id > tables, by stripping the vendor prefix prefix from the compatible string > and using that as an alias just as the i2c-core supports. > I see two ways to solve this issue. One is with $SUBJECT since we can argue that if a OF-only driver is able to match devices that were instantiated through the sysfs interface that only have a device name, then a modalias of the form i2c:<foo> is needed. Since the compatible strings without the vendor prefix is used to match, then I think that makes sense to also use them without the vendor prefix to populate I2C modaliases as $SUBJECT does. The other option is to remove i2c_of_match_device() and don't make OF match to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI case, since i2c_device_match() just calls acpi_driver_match_device() directly and doesn't have a wrapper function that fallbacks to sysfs matching. In this case an I2C device ID table would be required if the devices have to be instantiated through sysfs. That way the I2C table would be used both for auto-loading and also to match the device when it doesn't have an of_node. If the former is the correct way to solve this then the patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Best regards,
Hi Javier, thank you for providing the extra information. (And Kieran, thanks for the patch!) > The other option is to remove i2c_of_match_device() and don't make OF match > to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI > case, since i2c_device_match() just calls acpi_driver_match_device() directly > and doesn't have a wrapper function that fallbacks to sysfs matching. > > In this case an I2C device ID table would be required if the devices have to > be instantiated through sysfs. That way the I2C table would be used both for > auto-loading and also to match the device when it doesn't have an of_node. That would probably mean that only a minority of drivers will not add an I2C device ID table because it is easy to add an you get the sysfs feature? Then we are back again with the situation that most drivers will have multiple tables. With the minor change that the I2C device id table is not required anymore by the core, but it will be just very useful to have? Or? > If the former is the correct way to solve this then the patch looks good to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> For this actual patch from Kieran, I'd like to hear an opinion from the people maintaining modpost. The aproach looks okay to me, yet I can't tell how "easy" we are with adding new types like 'i2c_of'. Thanks everyone, Wolfram
Hi. On Thu, Aug 1, 2019 at 4:44 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > Hi Javier, > > thank you for providing the extra information. > > (And Kieran, thanks for the patch!) > > > The other option is to remove i2c_of_match_device() and don't make OF match > > to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI > > case, since i2c_device_match() just calls acpi_driver_match_device() directly > > and doesn't have a wrapper function that fallbacks to sysfs matching. > > > > In this case an I2C device ID table would be required if the devices have to > > be instantiated through sysfs. That way the I2C table would be used both for > > auto-loading and also to match the device when it doesn't have an of_node. > > That would probably mean that only a minority of drivers will not add an I2C > device ID table because it is easy to add an you get the sysfs feature? > > Then we are back again with the situation that most drivers will have > multiple tables. With the minor change that the I2C device id table is > not required anymore by the core, but it will be just very useful to > have? Or? > > > If the former is the correct way to solve this then the patch looks good to me. > > > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > For this actual patch from Kieran, I'd like to hear an opinion from the > people maintaining modpost. As you see 'git log scripts/mod/file2alias.c', this file is touched by every subsystem. So, the decision is up to you, Wolfram. And, you can pick this to your tree if you like. The implementation is really trivial. As Javier pointed out, this discussion comes down to "do we want to fall back to i2c_of_match_device_sysfs()?" If a driver supports DT and devices are instantiated via DT, in which situation is this useful? Do legacy non-DT platforms need this? > The aproach looks okay to me, yet I can't > tell how "easy" we are with adding new types like 'i2c_of'. As far as I understood, this patch provides a shorthand. You can save one table, but still get the same MODULE_ALIAS in the *.mod.c file. You need to add two MODULE_DEVICE_TABLE() though. MODULE_DEVICE_TABLE(of, si4713_of_match); MODULE_DEVICE_TABLE(i2c_of, si4713_of_match);
Hello Wolfram, On 7/31/19 9:44 PM, Wolfram Sang wrote: > Hi Javier, > > thank you for providing the extra information. > > (And Kieran, thanks for the patch!) > >> The other option is to remove i2c_of_match_device() and don't make OF match >> to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI >> case, since i2c_device_match() just calls acpi_driver_match_device() directly >> and doesn't have a wrapper function that fallbacks to sysfs matching. >> >> In this case an I2C device ID table would be required if the devices have to >> be instantiated through sysfs. That way the I2C table would be used both for >> auto-loading and also to match the device when it doesn't have an of_node. > > That would probably mean that only a minority of drivers will not add an I2C > device ID table because it is easy to add an you get the sysfs feature? > I believe so yes. > Then we are back again with the situation that most drivers will have > multiple tables. With the minor change that the I2C device id table is > not required anymore by the core, but it will be just very useful to > have? Or? > Yes, it won't be needed anymore if you are only instantiating all your devices from your firmware interface (e.g: OF, ACPI). >> If the former is the correct way to solve this then the patch looks good to me. >> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > For this actual patch from Kieran, I'd like to hear an opinion from the > people maintaining modpost. The aproach looks okay to me, yet I can't > tell how "easy" we are with adding new types like 'i2c_of'. > As Masahiro-san mentioned, this approach will still require to add a new macro MODULE_DEVICE_TABLE(i2c_of, bar_of_match) so the OF device table is used twice. One to expose the "of:N*T*Cfoo,bar" and another one to expose it as "i2c:bar". I expect that many developers would miss adding this macro for new drivers that are DT-only and so sysfs instantiation would not work there. So whatever is the approach taken we should clearly document all this so drivers authors are aware. > Thanks everyone, > > Wolfram > Best regards,
Hello Masahiro-san, On 8/1/19 4:17 AM, Masahiro Yamada wrote: > Hi. > > On Thu, Aug 1, 2019 at 4:44 AM Wolfram Sang <wsa@the-dreams.de> wrote: >> >> Hi Javier, >> >> thank you for providing the extra information. >> >> (And Kieran, thanks for the patch!) >> >>> The other option is to remove i2c_of_match_device() and don't make OF match >>> to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI >>> case, since i2c_device_match() just calls acpi_driver_match_device() directly >>> and doesn't have a wrapper function that fallbacks to sysfs matching. >>> >>> In this case an I2C device ID table would be required if the devices have to >>> be instantiated through sysfs. That way the I2C table would be used both for >>> auto-loading and also to match the device when it doesn't have an of_node. >> >> That would probably mean that only a minority of drivers will not add an I2C >> device ID table because it is easy to add an you get the sysfs feature? >> >> Then we are back again with the situation that most drivers will have >> multiple tables. With the minor change that the I2C device id table is >> not required anymore by the core, but it will be just very useful to >> have? Or? >> >>> If the former is the correct way to solve this then the patch looks good to me. >>> >>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >> >> For this actual patch from Kieran, I'd like to hear an opinion from the >> people maintaining modpost. > > > As you see 'git log scripts/mod/file2alias.c', > this file is touched by every subsystem. > > So, the decision is up to you, Wolfram. > And, you can pick this to your tree if you like. > > > The implementation is really trivial. > > > As Javier pointed out, this discussion comes down to > "do we want to fall back to i2c_of_match_device_sysfs()?" > Yes, I think that's the crux of the matter. Basically the matching logic should be consistent with the modalias uevent exposed to user-space to auto-load modules. So I think that we should either: a) take Kieran's patch or b) remove the i2c_of_match_device_sysfs() fallback for OF and require an I2C device table for sysfs instantiation and matching. > If a driver supports DT and devices are instantiated via DT, > in which situation is this useful? Is useful if you don't have all the I2C devices described in the DT. For example a daughterboard with an I2C device is connected to a board through an expansion slot or an I2C device connected directly to I2C pins exposed in a machine. In these cases your I2C devices won't be static so users might want to use the sysfs user-space interface to instantiate the I2C devices, i.e: # echo eeprom 0x50 > /sys/bus/i2c/devices/i2c-3/new_device as explained in https://github.com/torvalds/linux/blob/master/Documentation/i2c/instantiating-devices#L207 > Do legacy non-DT platforms need this? Yes, can also be used by non-DT platforms. But in this case isn't a problem because drivers for these platform will already have an I2C device ID table. > > > >> The aproach looks okay to me, yet I can't >> tell how "easy" we are with adding new types like 'i2c_of'. > > As far as I understood, this patch provides a shorthand. > You can save one table, but still get the > same MODULE_ALIAS in the *.mod.c file. > You need to add two MODULE_DEVICE_TABLE() though. > > MODULE_DEVICE_TABLE(of, si4713_of_match); > MODULE_DEVICE_TABLE(i2c_of, si4713_of_match); > That's my understanding as well. Best regards,
Hi Javier, On Tue, Aug 6, 2019 at 12:25 AM Javier Martinez Canillas <javierm@redhat.com> wrote: > On 7/31/19 9:44 PM, Wolfram Sang wrote: > > Hi Javier, > >> The other option is to remove i2c_of_match_device() and don't make OF match > >> to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI > >> case, since i2c_device_match() just calls acpi_driver_match_device() directly > >> and doesn't have a wrapper function that fallbacks to sysfs matching. > >> > >> In this case an I2C device ID table would be required if the devices have to > >> be instantiated through sysfs. That way the I2C table would be used both for > >> auto-loading and also to match the device when it doesn't have an of_node. > > > > That would probably mean that only a minority of drivers will not add an I2C > > device ID table because it is easy to add an you get the sysfs feature? > > > > I believe so yes. > As Masahiro-san mentioned, this approach will still require to add a new macro > MODULE_DEVICE_TABLE(i2c_of, bar_of_match) so the OF device table is used twice. > > One to expose the "of:N*T*Cfoo,bar" and another one to expose it as "i2c:bar". > > I expect that many developers would miss adding this macro for new drivers that > are DT-only and so sysfs instantiation would not work there. So whatever is the > approach taken we should clearly document all this so drivers authors are aware. You could add a new I2C_MODULE_DEVICE_TABLE() that adds both, right? Makes it a little bit easier to check/enforce this. Gr{oetje,eeting}s, Geert
On Tue, Aug 6, 2019 at 12:48 AM Javier Martinez Canillas <javierm@redhat.com> wrote: > On 8/1/19 4:17 AM, Masahiro Yamada wrote: > So I think that we should either: > > a) take Kieran's patch or b) remove the i2c_of_match_device_sysfs() fallback > for OF and require an I2C device table for sysfs instantiation and matching. > > > If a driver supports DT and devices are instantiated via DT, > > in which situation is this useful? > > Is useful if you don't have all the I2C devices described in the DT. For example > a daughterboard with an I2C device is connected to a board through an expansion > slot or an I2C device connected directly to I2C pins exposed in a machine. > > In these cases your I2C devices won't be static so users might want to use the > sysfs user-space interface to instantiate the I2C devices, i.e: > > # echo eeprom 0x50 > /sys/bus/i2c/devices/i2c-3/new_device > > as explained in https://github.com/torvalds/linux/blob/master/Documentation/i2c/instantiating-devices#L207 Does this actually work with DT names, too? E.g. # echo atmel,24c02 > /sys/bus/i2c/devices/i2c-3/new_device Still leaves us with legacy names for backwards compatibility. Gr{oetje,eeting}s, Geert
Hello Geert, On 8/6/19 9:22 AM, Geert Uytterhoeven wrote: > Hi Javier, > > On Tue, Aug 6, 2019 at 12:25 AM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> On 7/31/19 9:44 PM, Wolfram Sang wrote: >>> Hi Javier, >>>> The other option is to remove i2c_of_match_device() and don't make OF match >>>> to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI >>>> case, since i2c_device_match() just calls acpi_driver_match_device() directly >>>> and doesn't have a wrapper function that fallbacks to sysfs matching. >>>> >>>> In this case an I2C device ID table would be required if the devices have to >>>> be instantiated through sysfs. That way the I2C table would be used both for >>>> auto-loading and also to match the device when it doesn't have an of_node. >>> >>> That would probably mean that only a minority of drivers will not add an I2C >>> device ID table because it is easy to add an you get the sysfs feature? >>> >> >> I believe so yes. > >> As Masahiro-san mentioned, this approach will still require to add a new macro >> MODULE_DEVICE_TABLE(i2c_of, bar_of_match) so the OF device table is used twice. >> >> One to expose the "of:N*T*Cfoo,bar" and another one to expose it as "i2c:bar". >> >> I expect that many developers would miss adding this macro for new drivers that >> are DT-only and so sysfs instantiation would not work there. So whatever is the >> approach taken we should clearly document all this so drivers authors are aware. > > You could add a new I2C_MODULE_DEVICE_TABLE() that adds both, right? > Makes it a little bit easier to check/enforce this. > Right, we could add a macro for that. Although it should probably be called I2C_OF_MODULE_DEVICE_TABLE() or something like that since is specific to OF. > Gr{oetje,eeting}s, > > Geert > Best regards,
Hello Geert, On 8/6/19 9:30 AM, Geert Uytterhoeven wrote: > On Tue, Aug 6, 2019 at 12:48 AM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> On 8/1/19 4:17 AM, Masahiro Yamada wrote: >> So I think that we should either: >> >> a) take Kieran's patch or b) remove the i2c_of_match_device_sysfs() fallback >> for OF and require an I2C device table for sysfs instantiation and matching. >> >>> If a driver supports DT and devices are instantiated via DT, >>> in which situation is this useful? >> >> Is useful if you don't have all the I2C devices described in the DT. For example >> a daughterboard with an I2C device is connected to a board through an expansion >> slot or an I2C device connected directly to I2C pins exposed in a machine. >> >> In these cases your I2C devices won't be static so users might want to use the >> sysfs user-space interface to instantiate the I2C devices, i.e: >> >> # echo eeprom 0x50 > /sys/bus/i2c/devices/i2c-3/new_device >> >> as explained in https://github.com/torvalds/linux/blob/master/Documentation/i2c/instantiating-devices#L207 > > Does this actually work with DT names, too? E.g. > > # echo atmel,24c02 > /sys/bus/i2c/devices/i2c-3/new_device > My understanding is that it does. If I'm reading the code correctly the i2c_of_match_device_sysfs() function first attempts to match using both the vendor and device part of the compatible string and if that fails, it strips the vendor part and try to match only using the device part. So you could use any of the following: # echo 24c02 0x50 > /sys/bus/i2c/devices/i2c-3/new_device # echo atmel,24c02 0x50 > /sys/bus/i2c/devices/i2c-3/new_device Best regards,
On 06.08.19 19:12, Javier Martinez Canillas wrote: > Right, we could add a macro for that. Although it should probably be called > I2C_OF_MODULE_DEVICE_TABLE() or something like that since is specific to OF. At that point it should be completely noop when OF is disabled, so we also can get rid of many ifdef's. I've got some patch somewhere for introducing a MODULE_OF_TABLE() macro as replacement for many MODULE_DEVICE_TABLE(of, ...) cases, which noops when CONFIG_OF is disabled. (and similar ones for other table types). by the way: haven't followed whole discussion, just picked up some proposed table changes ... does that require userland upgrades ? --mtx
On Thu, Aug 08, 2019 at 03:12:47PM +0200, Enrico Weigelt, metux IT consult wrote: > On 06.08.19 19:12, Javier Martinez Canillas wrote: > > > Right, we could add a macro for that. Although it should probably be called > > I2C_OF_MODULE_DEVICE_TABLE() or something like that since is specific to OF. > > At that point it should be completely noop when OF is disabled, so we > also can get rid of many ifdef's. Why? > I've got some patch somewhere for introducing a MODULE_OF_TABLE() macro > as replacement for many MODULE_DEVICE_TABLE(of, ...) cases, which noops > when CONFIG_OF is disabled. (and similar ones for other table types). It's simple wrong to have #ifdef CONFIG_OF without counterpart of_match_ptr(). And taking into consideration that ID table itself doesn't depend to OF at all, why not simple drop that #ifdef and of_match_ptr() all together?
On 08.08.19 15:24, Andy Shevchenko wrote: > On Thu, Aug 08, 2019 at 03:12:47PM +0200, Enrico Weigelt, metux IT consult wrote: >> On 06.08.19 19:12, Javier Martinez Canillas wrote: >> >>> Right, we could add a macro for that. Although it should probably be called >>> I2C_OF_MODULE_DEVICE_TABLE() or something like that since is specific to OF. >> >> At that point it should be completely noop when OF is disabled, so we >> also can get rid of many ifdef's. > > Why? For cases where drivers work w/ or w/o oftree. Not sure whether it applies to i2c specifically, but there're other places where we still need nasty ifdef's (eg. gpio-keyboard). >> I've got some patch somewhere for introducing a MODULE_OF_TABLE() macro >> as replacement for many MODULE_DEVICE_TABLE(of, ...) cases, which noops >> when CONFIG_OF is disabled. (and similar ones for other table types). > > It's simple wrong to have #ifdef CONFIG_OF without counterpart of_match_ptr(). Of course, but that's just a part of the story. (actually I'd prefer using it everywhere, even if the driver only supports oftree). > And taking into consideration that ID table itself doesn't depend to OF at all, > why not simple drop that #ifdef and of_match_ptr() all together? Consumes less space. Yes, it isn't much, but in some scenarios one needs to heavily reduce the kernel size. And I wouldn't like to use of_match_ptr() inside a MODULE_DEVICE_TABLE() call :o --mtx
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c index e17a29ae2e97..16776f624d3a 100644 --- a/scripts/mod/file2alias.c +++ b/scripts/mod/file2alias.c @@ -379,6 +379,35 @@ static void do_of_table(void *symval, unsigned long size, do_of_entry_multi(symval + i, mod); } +static int do_i2c_of_entry(void *symval, struct module *mod) +{ + const char *alias; + DEF_FIELD_ADDR(symval, of_device_id, compatible); + + alias = strrchr(*compatible, ','); + + buf_printf(&mod->dev_table_buf, "MODULE_ALIAS(\"%s%s\");\n", + I2C_MODULE_PREFIX, alias ? alias + 1 : *compatible); + + return 1; +} + +/* Reuse OF tables to generate I2C aliases */ +static void do_i2c_of_table(void *symval, unsigned long size, + struct module *mod) +{ + unsigned int i; + const unsigned long id_size = SIZE_of_device_id; + + device_id_check(mod->name, "i2c_of", size, id_size, symval); + + /* Leave last one: it's the terminator. */ + size -= id_size; + + for (i = 0; i < size; i += id_size) + do_i2c_of_entry(symval + i, mod); +} + /* Looks like: hid:bNvNpN */ static int do_hid_entry(const char *filename, void *symval, char *alias) @@ -1452,6 +1481,8 @@ void handle_moddevtable(struct module *mod, struct elf_info *info, do_usb_table(symval, sym->st_size, mod); if (sym_is(name, namelen, "of")) do_of_table(symval, sym->st_size, mod); + else if (sym_is(name, namelen, "i2c_of")) + do_i2c_of_table(symval, sym->st_size, mod); else if (sym_is(name, namelen, "pnp")) do_pnp_device_entry(symval, sym->st_size, mod); else if (sym_is(name, namelen, "pnp_card"))
I2C drivers match against an I2C ID table, an OF table, and an ACPI table. It is now also possible to match against an OF table entry without the vendor prefix to support backwards compatibility, and allow simplification of the i2c probe functions. As part of this matching, the probe function is being converted to remove the need to specify the i2c_device_id table, but to support module aliasing, we still require to have the MODULE_DEVICE_TABLE entry. Facilitate generating the I2C aliases directly from the of_device_id tables, by stripping the vendor prefix prefix from the compatible string and using that as an alias just as the i2c-core supports. Drivers which remove the i2c_device_id table can then register against the of_device_id table by adding an extra MODULE_DEVICE_TABLE registration as shown by the following example: /* si4713_i2c_driver - i2c driver interface */ -static const struct i2c_device_id si4713_id[] = { - { "si4713" , 0 }, - { }, -}; -MODULE_DEVICE_TABLE(i2c, si4713_id); static const struct of_device_id si4713_of_match[] = { { .compatible = "silabs,si4713" }, { }, }; MODULE_DEVICE_TABLE(of, si4713_of_match); +MODULE_DEVICE_TABLE(i2c_of, si4713_of_match); Several drivers have had their i2c_device_id tables removed entirely which will lead to their module aliases being limited, during the following patches: 0f21700ac40c ("rtc: pcf85063: switch to probe_new") 3a4f4f2963f4 ("ASoC: rt5677: Convert I2C driver to ->probe_new()") 511cb17448d9 ("mfd: tps65217: Introduce dependency on CONFIG_OF") 8597c0920d6f ("NFC: fdp: Convert I2C driver to ->probe_new()") b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new() call-back type") The following patches might require I2C aliases to be generated from their ACPI tables: e19c92059a70 ("media: staging: atomisp: Switch i2c drivers to use ->probe_new()") f758eb2363ec ("media: dw9714: Remove ACPI match tables, convert to use probe_new") Cc: Lee Jones <lee.jones@linaro.org> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: Javier Martinez Canillas <javierm@redhat.com> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Mark Brown <broonie@kernel.org> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> --- scripts/mod/file2alias.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)