| Message ID | 1462374888-22888-9-git-send-email-kieran@bingham.xyz |
|---|---|
| State | Superseded |
| Headers | show |
On 05/04/2016 11:14 AM, Kieran Bingham wrote: > From: Lee Jones <lee.jones@linaro.org> > > Also remove unused second probe() parameter 'i2c_device_id'. > > Acked-by: Grant Likely <grant.likely@linaro.org> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > Signed-off-by: Kieran Bingham <kieran@bingham.xyz> > > --- > Changes since v4 > - Rename .probe2 to probe_new > --- > drivers/mfd/as3722.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) > > diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c > index e1f597f97f86..28fc62af6861 100644 > --- a/drivers/mfd/as3722.c > +++ b/drivers/mfd/as3722.c > @@ -354,8 +354,7 @@ static int as3722_i2c_of_probe(struct i2c_client *i2c, > return 0; > } > > -static int as3722_i2c_probe(struct i2c_client *i2c, > - const struct i2c_device_id *id) > +static int as3722_i2c_probe(struct i2c_client *i2c) > { > struct as3722 *as3722; > unsigned long irq_flags; > @@ -453,12 +452,6 @@ static const struct of_device_id as3722_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, as3722_of_match); > > -static const struct i2c_device_id as3722_i2c_id[] = { > - { "as3722", 0 }, > - {}, > -}; > -MODULE_DEVICE_TABLE(i2c, as3722_i2c_id); > - Just a note that this can only be made because the driver's Kconfig symbol is bool and not tristate. Since for drivers that can be built as a module, the I2C core always reports a MODALIAS of the form "i2c:as3722" and so the i2c_device_id array and the MODULE_DEVICE_TABLE() are needed even when not used by the driver. As mentioned the change is correct for this driver but I just wanted to point out in case other authors try to do the same change for drivers that can be built as a module and so breaking module auto-loading. Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com> Best regards,
On Tue, 10 May 2016, Javier Martinez Canillas wrote: > On 05/04/2016 11:14 AM, Kieran Bingham wrote: > > From: Lee Jones <lee.jones@linaro.org> > > > > Also remove unused second probe() parameter 'i2c_device_id'. > > > > Acked-by: Grant Likely <grant.likely@linaro.org> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > Signed-off-by: Kieran Bingham <kieran@bingham.xyz> > > > > --- > > Changes since v4 > > - Rename .probe2 to probe_new > > --- > > drivers/mfd/as3722.c | 12 ++---------- > > 1 file changed, 2 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c > > index e1f597f97f86..28fc62af6861 100644 > > --- a/drivers/mfd/as3722.c > > +++ b/drivers/mfd/as3722.c > > @@ -354,8 +354,7 @@ static int as3722_i2c_of_probe(struct i2c_client *i2c, > > return 0; > > } > > > > -static int as3722_i2c_probe(struct i2c_client *i2c, > > - const struct i2c_device_id *id) > > +static int as3722_i2c_probe(struct i2c_client *i2c) > > { > > struct as3722 *as3722; > > unsigned long irq_flags; > > @@ -453,12 +452,6 @@ static const struct of_device_id as3722_of_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, as3722_of_match); > > > > -static const struct i2c_device_id as3722_i2c_id[] = { > > - { "as3722", 0 }, > > - {}, > > -}; > > -MODULE_DEVICE_TABLE(i2c, as3722_i2c_id); > > - > > Just a note that this can only be made because the driver's Kconfig symbol > is bool and not tristate. Since for drivers that can be built as a module, > the I2C core always reports a MODALIAS of the form "i2c:as3722" and so the > i2c_device_id array and the MODULE_DEVICE_TABLE() are needed even when not > used by the driver. > > As mentioned the change is correct for this driver but I just wanted to > point out in case other authors try to do the same change for drivers that > can be built as a module and so breaking module auto-loading. Sounds like a subsequent patch might be required to fix that use-case too. I'll add it to my TODO. :) > Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com> > > Best regards,
Hello Lee, On 05/10/2016 03:33 AM, Lee Jones wrote: > On Tue, 10 May 2016, Javier Martinez Canillas wrote: [snip] >>> >>> -static const struct i2c_device_id as3722_i2c_id[] = { >>> - { "as3722", 0 }, >>> - {}, >>> -}; >>> -MODULE_DEVICE_TABLE(i2c, as3722_i2c_id); >>> - >> >> Just a note that this can only be made because the driver's Kconfig symbol >> is bool and not tristate. Since for drivers that can be built as a module, >> the I2C core always reports a MODALIAS of the form "i2c:as3722" and so the >> i2c_device_id array and the MODULE_DEVICE_TABLE() are needed even when not >> used by the driver. >> >> As mentioned the change is correct for this driver but I just wanted to >> point out in case other authors try to do the same change for drivers that >> can be built as a module and so breaking module auto-loading. > > Sounds like a subsequent patch might be required to fix that use-case > too. I'll add it to my TODO. :) > Well, the fix is really trivial and I've posted it as an RFC patch a [0] a long time ago. That is what Kieran and my used to test module autoload with this patch series. The problem is that a lot of I2C drivers are relying on how the subsystem behave (always match using i2c_device_id table and report i2c: modalias) and so OF drivers don't have an of_device_id table since was not necessary. So if the RFC patch lands [0], that will break a lot of drivers since after that, the I2C devices registered via OF will report a of: modalias but will not have a OF aliases in their modules. We need a flag day to change the I2C subsystem behaviour and that can only happen after all the in-tree I2C drivers have proper exported of_device_id. I posted a patch series almost a year ago [1] trying to fix the I2C drivers that I could find using a script but then found that removing the I2C table was not possible also due how the subsystem did the matching. Fortunately your patch series fixed this :) So after your series land, I plan to do the same investigation again and post patches to fix all the remaining I2C drivers so the modalias patch can finally land and the I2C subsystem report modalias like other subsystems do. [0]: https://patchwork.ozlabs.org/patch/502201/ [1]: https://lkml.org/lkml/2015/7/30/519 Best regards,
On Tue, 10 May 2016, Javier Martinez Canillas wrote: > Hello Lee, > > On 05/10/2016 03:33 AM, Lee Jones wrote: > > On Tue, 10 May 2016, Javier Martinez Canillas wrote: > > [snip] > > >>> > >>> -static const struct i2c_device_id as3722_i2c_id[] = { > >>> - { "as3722", 0 }, > >>> - {}, > >>> -}; > >>> -MODULE_DEVICE_TABLE(i2c, as3722_i2c_id); > >>> - > >> > >> Just a note that this can only be made because the driver's Kconfig symbol > >> is bool and not tristate. Since for drivers that can be built as a module, > >> the I2C core always reports a MODALIAS of the form "i2c:as3722" and so the > >> i2c_device_id array and the MODULE_DEVICE_TABLE() are needed even when not > >> used by the driver. > >> > >> As mentioned the change is correct for this driver but I just wanted to > >> point out in case other authors try to do the same change for drivers that > >> can be built as a module and so breaking module auto-loading. > > > > Sounds like a subsequent patch might be required to fix that use-case > > too. I'll add it to my TODO. :) > > > > Well, the fix is really trivial and I've posted it as an RFC patch a [0] a > long time ago. That is what Kieran and my used to test module autoload with > this patch series. > > The problem is that a lot of I2C drivers are relying on how the subsystem > behave (always match using i2c_device_id table and report i2c: modalias) > and so OF drivers don't have an of_device_id table since was not necessary. > > So if the RFC patch lands [0], that will break a lot of drivers since after > that, the I2C devices registered via OF will report a of: modalias but will > not have a OF aliases in their modules. > > We need a flag day to change the I2C subsystem behaviour and that can only > happen after all the in-tree I2C drivers have proper exported of_device_id. > > I posted a patch series almost a year ago [1] trying to fix the I2C drivers > that I could find using a script but then found that removing the I2C table > was not possible also due how the subsystem did the matching. Fortunately > your patch series fixed this :) > > So after your series land, I plan to do the same investigation again and > post patches to fix all the remaining I2C drivers so the modalias patch can > finally land and the I2C subsystem report modalias like other subsystems do. Sounds perfect. Thanks for the explanation. Wolfram, Things are looking up for the subsystem, please do your thing, so we can ensure awesomeness. :) > [0]: https://patchwork.ozlabs.org/patch/502201/ > [1]: https://lkml.org/lkml/2015/7/30/519
diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c index e1f597f97f86..28fc62af6861 100644 --- a/drivers/mfd/as3722.c +++ b/drivers/mfd/as3722.c @@ -354,8 +354,7 @@ static int as3722_i2c_of_probe(struct i2c_client *i2c, return 0; } -static int as3722_i2c_probe(struct i2c_client *i2c, - const struct i2c_device_id *id) +static int as3722_i2c_probe(struct i2c_client *i2c) { struct as3722 *as3722; unsigned long irq_flags; @@ -453,12 +452,6 @@ static const struct of_device_id as3722_of_match[] = { }; MODULE_DEVICE_TABLE(of, as3722_of_match); -static const struct i2c_device_id as3722_i2c_id[] = { - { "as3722", 0 }, - {}, -}; -MODULE_DEVICE_TABLE(i2c, as3722_i2c_id); - static const struct dev_pm_ops as3722_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(as3722_i2c_suspend, as3722_i2c_resume) }; @@ -469,9 +462,8 @@ static struct i2c_driver as3722_i2c_driver = { .of_match_table = as3722_of_match, .pm = &as3722_pm_ops, }, - .probe = as3722_i2c_probe, + .probe_new = as3722_i2c_probe, .remove = as3722_i2c_remove, - .id_table = as3722_i2c_id, }; module_i2c_driver(as3722_i2c_driver);