| Message ID | 1478522866-29620-11-git-send-email-kieran@bingham.xyz |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello Kieran, On 11/07/2016 09:47 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 f87342c211bc..7d8c5e4136e2 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) > { This is OK... > struct as3722 *as3722; > unsigned long irq_flags; > @@ -441,12 +440,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); > - ... but I don't think this is correct. Otherwise you will break module autoload for this driver since modpost needs the I2C device ID table info to fill the i2c modalias in the drivers' module. Remember that i2c_device_uevent() always reports modalias of the form MODALIAS=i2c:<foo> even when your series allows to match without a I2C device ID table. Best regards,
Thanks for your reviews again Javier, On 07/11/16 19:20, Javier Martinez Canillas wrote: > Hello Kieran, > > On 11/07/2016 09:47 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 f87342c211bc..7d8c5e4136e2 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) >> { > > This is OK... > >> struct as3722 *as3722; >> unsigned long irq_flags; >> @@ -441,12 +440,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); >> - > > ... but I don't think this is correct. Otherwise you will break module > autoload for this driver since modpost needs the I2C device ID table > info to fill the i2c modalias in the drivers' module. > > Remember that i2c_device_uevent() always reports modalias of the form > MODALIAS=i2c:<foo> even when your series allows to match without a I2C > device ID table. > Ok - Thanks for the reminder. I'll try to bear this in mind when we start updating drivers. For now we can consider this patch dropped from the series I think. > Best regards, >
> Remember that i2c_device_uevent() always reports modalias of the form > MODALIAS=i2c:<foo> even when your series allows to match without a I2C > device ID table. Not always. Can't we do something similar like ACPI does with acpi_device_uevent_modalias()? I mean the whole point of this series is to remove the need of having an I2C device ID table...
Hello Kieran, On 11/07/2016 07:05 PM, Kieran Bingham wrote: > Thanks for your reviews again Javier, > Thanks to you for keep pushing this series. > On 07/11/16 19:20, Javier Martinez Canillas wrote: >> Hello Kieran, >> >> On 11/07/2016 09:47 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 f87342c211bc..7d8c5e4136e2 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) >>> { >> >> This is OK... >> >>> struct as3722 *as3722; >>> unsigned long irq_flags; >>> @@ -441,12 +440,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); >>> - >> >> ... but I don't think this is correct. Otherwise you will break module >> autoload for this driver since modpost needs the I2C device ID table >> info to fill the i2c modalias in the drivers' module. >> >> Remember that i2c_device_uevent() always reports modalias of the form >> MODALIAS=i2c:<foo> even when your series allows to match without a I2C >> device ID table. >> > > Ok - Thanks for the reminder. I'll try to bear this in mind when we > start updating drivers. > > For now we can consider this patch dropped from the series I think. > Yes, or you could just do the change that uses probe_new for now but leave the MODULE_DEVICE_TABLE(). >> Best regards, >> > Best regards,
Hello Wolfram, On 11/07/2016 08:09 PM, Wolfram Sang wrote: > >> Remember that i2c_device_uevent() always reports modalias of the form >> MODALIAS=i2c:<foo> even when your series allows to match without a I2C >> device ID table. > > Not always. Can't we do something similar like ACPI does with Right, I meant that it always report a platform I2C modalias for both OF and legacy platform data I2C device registration mechanisms. > acpi_device_uevent_modalias()? > Yes, doing that is trivial. I posted a RFC patch about a year ago that changes i2c_device_uevent() to report a proper OF modalias [0] as a part of a series that fixed module autoload in a bunch of I2C drivers [1]. What's tricky is to make sure that the change won't introduce regressions in current I2C drivers. I enumerated the possible issues if the I2C core starts reporting OF modaliases and fixed some of them in the same series. > I mean the whole point of this series is to remove the need of having an > I2C device ID table... > Agreed, one of the issues was that the I2C table was needed anyways for the core to match and pass a struct i2c_device_id to the probe's function. This series solves that so once it lands, I plan to address the possible issues in the I2C drivers and re-send [0] as a proper patch. [0]: https://patchwork.kernel.org/patch/6903991/ [1]: https://lkml.org/lkml/2015/7/30/519 Best regards,
diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c index f87342c211bc..7d8c5e4136e2 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; @@ -441,12 +440,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) }; @@ -457,8 +450,7 @@ static struct i2c_driver as3722_i2c_driver = { .of_match_table = as3722_of_match, .pm = &as3722_pm_ops, }, - .probe = as3722_i2c_probe, - .id_table = as3722_i2c_id, + .probe_new = as3722_i2c_probe, }; module_i2c_driver(as3722_i2c_driver);