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@xxxxxxxxxxxxx> 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@xxxxxxxxxx> >> >> 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, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
