diff mbox series

[3/4] iio: light: max44009: add missing OF device matching

Message ID 20230311111457.251475-3-krzysztof.kozlowski@linaro.org
State New
Headers
Series [1/4] iio: adc: rcar-gyroadc: mark OF related data as maybe unused |

Commit Message

Krzysztof Kozlowski March 11, 2023, 11:14 a.m. UTC 
  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(-)

Comments

Jonathan Cameron March 11, 2023, 12:26 p.m. UTC | #1 
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");
Krzysztof Kozlowski March 11, 2023, 12:28 p.m. UTC | #2 
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
Jonathan Cameron March 11, 2023, 6:35 p.m. UTC | #3 
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
>
Krzysztof Kozlowski March 12, 2023, 10:15 a.m. UTC | #4 
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 mbox series

Patch

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");