| Message ID | 20230311111457.251475-3-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/4] iio: adc: rcar-gyroadc: mark OF related data as maybe unused | expand |
On Sat, 11 Mar 2023 12:14:56 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > The driver currently matches only via i2c_device_id, but also has > of_device_id table: > > drivers/iio/light/max44009.c:545:34: error: ‘max44009_of_match’ defined but not used [-Werror=unused-const-variable=] > > Fixes: 6aef699a7d7e ("iio: light: add driver for MAX44009") > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Don't use of_match_ptr() unless you are absolutely sure no other firmware route will make use of the of_match_table. In this particular case ACPI using PRP0001 is broken by that macro. So good to set the of_match_table, but make sure to always set it and hence you don't need the __maybe_unused. Jonathan > --- > drivers/iio/light/max44009.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/light/max44009.c b/drivers/iio/light/max44009.c > index 3dadace09fe2..274e0b679ca2 100644 > --- a/drivers/iio/light/max44009.c > +++ b/drivers/iio/light/max44009.c > @@ -527,6 +527,12 @@ static int max44009_probe(struct i2c_client *client) > return devm_iio_device_register(&client->dev, indio_dev); > } > > +static const struct of_device_id max44009_of_match[] __maybe_unused = { > + { .compatible = "maxim,max44009" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max44009_of_match); > + > static const struct i2c_device_id max44009_id[] = { > { "max44009", 0 }, > { } > @@ -536,18 +542,13 @@ MODULE_DEVICE_TABLE(i2c, max44009_id); > static struct i2c_driver max44009_driver = { > .driver = { > .name = MAX44009_DRV_NAME, > + .of_match_table = of_match_ptr(max44009_of_match), > }, > .probe_new = max44009_probe, > .id_table = max44009_id, > }; > module_i2c_driver(max44009_driver); > > -static const struct of_device_id max44009_of_match[] = { > - { .compatible = "maxim,max44009" }, > - { } > -}; > -MODULE_DEVICE_TABLE(of, max44009_of_match); > - > MODULE_AUTHOR("Robert Eshleman <bobbyeshleman@gmail.com>"); > MODULE_LICENSE("GPL v2"); > MODULE_DESCRIPTION("MAX44009 ambient light sensor driver");
On 11/03/2023 13:26, Jonathan Cameron wrote: > On Sat, 11 Mar 2023 12:14:56 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> The driver currently matches only via i2c_device_id, but also has >> of_device_id table: >> >> drivers/iio/light/max44009.c:545:34: error: ‘max44009_of_match’ defined but not used [-Werror=unused-const-variable=] >> >> Fixes: 6aef699a7d7e ("iio: light: add driver for MAX44009") >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Don't use of_match_ptr() unless you are absolutely sure no other firmware > route will make use of the of_match_table. > > In this particular case ACPI using PRP0001 is broken by that macro. It's not broken because there was no matching via PRP0001 due to missing table. > > So good to set the of_match_table, but make sure to always set it > and hence you don't need the __maybe_unused. So you want to add PRP0001? We can, the fix is for different issue, though. Best regards, Krzysztof
On Sat, 11 Mar 2023 13:28:17 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 11/03/2023 13:26, Jonathan Cameron wrote: > > On Sat, 11 Mar 2023 12:14:56 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >> The driver currently matches only via i2c_device_id, but also has > >> of_device_id table: > >> > >> drivers/iio/light/max44009.c:545:34: error: ‘max44009_of_match’ defined but not used [-Werror=unused-const-variable=] > >> > >> Fixes: 6aef699a7d7e ("iio: light: add driver for MAX44009") > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > Don't use of_match_ptr() unless you are absolutely sure no other firmware > > route will make use of the of_match_table. > > > > In this particular case ACPI using PRP0001 is broken by that macro. > > It's not broken because there was no matching via PRP0001 due to missing > table. > > > > > So good to set the of_match_table, but make sure to always set it > > and hence you don't need the __maybe_unused. > > So you want to add PRP0001? We can, the fix is for different issue, though. There is nothing to add. You need to do less than you have done in this patch. Drop the of_match_ptr() and the __maybe_unused and PRP0001 based matching will just work. The PRP0001 path just uses the of_device_id table and needs no specific support in a driver - it doesn't need an ACPI id table or anything like that. It's a long story, but hindsight says that of_match_ptr() should never have existed as it only serves to stop things working that otherwise work for free. Jonathan > > Best regards, > Krzysztof >
On 11/03/2023 19:35, Jonathan Cameron wrote: > On Sat, 11 Mar 2023 13:28:17 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 11/03/2023 13:26, Jonathan Cameron wrote: >>> On Sat, 11 Mar 2023 12:14:56 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> The driver currently matches only via i2c_device_id, but also has >>>> of_device_id table: >>>> >>>> drivers/iio/light/max44009.c:545:34: error: ‘max44009_of_match’ defined but not used [-Werror=unused-const-variable=] >>>> >>>> Fixes: 6aef699a7d7e ("iio: light: add driver for MAX44009") >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>> >>> Don't use of_match_ptr() unless you are absolutely sure no other firmware >>> route will make use of the of_match_table. >>> >>> In this particular case ACPI using PRP0001 is broken by that macro. >> >> It's not broken because there was no matching via PRP0001 due to missing >> table. >> >>> >>> So good to set the of_match_table, but make sure to always set it >>> and hence you don't need the __maybe_unused. >> >> So you want to add PRP0001? We can, the fix is for different issue, though. > > There is nothing to add. You need to do less than you have done in this patch. > Drop the of_match_ptr() and the __maybe_unused and PRP0001 based matching will just > work. The PRP0001 path just uses the of_device_id table and needs no Sure, but that's *adding a feature*. You said that "ACPI using PRP0001 is broken", but it was never here in the first place. PRP0001 *was* already broken here, not *is*. The patch does not decrease the functionality. > specific support in a driver - it doesn't need an ACPI id table or anything like > that. > > It's a long story, but hindsight says that of_match_ptr() should never have > existed as it only serves to stop things working that otherwise work for free. Sure, I can go with ID table always present. Best regards, Krzysztof
diff --git a/drivers/iio/light/max44009.c b/drivers/iio/light/max44009.c index 3dadace09fe2..274e0b679ca2 100644 --- a/drivers/iio/light/max44009.c +++ b/drivers/iio/light/max44009.c @@ -527,6 +527,12 @@ static int max44009_probe(struct i2c_client *client) return devm_iio_device_register(&client->dev, indio_dev); } +static const struct of_device_id max44009_of_match[] __maybe_unused = { + { .compatible = "maxim,max44009" }, + { } +}; +MODULE_DEVICE_TABLE(of, max44009_of_match); + static const struct i2c_device_id max44009_id[] = { { "max44009", 0 }, { } @@ -536,18 +542,13 @@ MODULE_DEVICE_TABLE(i2c, max44009_id); static struct i2c_driver max44009_driver = { .driver = { .name = MAX44009_DRV_NAME, + .of_match_table = of_match_ptr(max44009_of_match), }, .probe_new = max44009_probe, .id_table = max44009_id, }; module_i2c_driver(max44009_driver); -static const struct of_device_id max44009_of_match[] = { - { .compatible = "maxim,max44009" }, - { } -}; -MODULE_DEVICE_TABLE(of, max44009_of_match); - MODULE_AUTHOR("Robert Eshleman <bobbyeshleman@gmail.com>"); MODULE_LICENSE("GPL v2"); MODULE_DESCRIPTION("MAX44009 ambient light sensor driver");
The driver currently matches only via i2c_device_id, but also has of_device_id table: drivers/iio/light/max44009.c:545:34: error: ‘max44009_of_match’ defined but not used [-Werror=unused-const-variable=] Fixes: 6aef699a7d7e ("iio: light: add driver for MAX44009") Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/iio/light/max44009.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)