[PATCH 1/4] iio: adc: rcar-gyroadc: mark OF related data as maybe unused

[PATCH 1/4] iio: adc: rcar-gyroadc: mark OF related data as maybe unused

[Krzysztof Kozlowski]

The driver can be compile tested with !CONFIG_OF making certain data
unused:

  drivers/iio/adc/rcar-gyroadc.c:286:34: error: ‘rcar_gyroadc_child_match’ defined but not used [-Werror=unused-const-variable=]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/iio/adc/rcar-gyroadc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
index 27d9e147b4b7..b8972f673c9d 100644
--- a/drivers/iio/adc/rcar-gyroadc.c
+++ b/drivers/iio/adc/rcar-gyroadc.c
@@ -283,7 +283,7 @@ static const struct of_device_id rcar_gyroadc_match[] = {
 
 MODULE_DEVICE_TABLE(of, rcar_gyroadc_match);
 
-static const struct of_device_id rcar_gyroadc_child_match[] = {
+static const struct of_device_id rcar_gyroadc_child_match[] __maybe_unused = {
 	/* Mode 1 ADCs */
 	{
 		.compatible	= "fujitsu,mb88101a",
-- 
2.34.1

[PATCH 2/4] iio: dac: ad5755: mark OF related data as maybe unused

[Krzysztof Kozlowski]

The driver can be compile tested with !CONFIG_OF making certain data
unused (of_device_id is not used for device matching):

  drivers/iio/dac/ad5755.c:865:34: error: ‘ad5755_of_match’ defined but not used [-Werror=unused-const-variable=]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/iio/dac/ad5755.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c
index beadfa938d2d..a6dc8a0bfc29 100644
--- a/drivers/iio/dac/ad5755.c
+++ b/drivers/iio/dac/ad5755.c
@@ -862,7 +862,7 @@ static const struct spi_device_id ad5755_id[] = {
 };
 MODULE_DEVICE_TABLE(spi, ad5755_id);
 
-static const struct of_device_id ad5755_of_match[] = {
+static const struct of_device_id ad5755_of_match[] __maybe_unused = {
 	{ .compatible = "adi,ad5755" },
 	{ .compatible = "adi,ad5755-1" },
 	{ .compatible = "adi,ad5757" },
-- 
2.34.1

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

[Krzysztof Kozlowski]

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(-)

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

[PATCH 4/4] iio: proximity: sx9500: Mark ACPI and OF related data as maybe unused

[Krzysztof Kozlowski]

The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making
certain data unused:

  drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/iio/proximity/sx9500.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index 8794e75e5bf9..840db1953998 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -1036,13 +1036,13 @@ static const struct acpi_device_id sx9500_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match);
 
-static const struct of_device_id sx9500_of_match[] = {
+static const struct of_device_id sx9500_of_match[] __maybe_unused = {
 	{ .compatible = "semtech,sx9500", },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sx9500_of_match);
 
-static const struct i2c_device_id sx9500_id[] = {
+static const struct i2c_device_id sx9500_id[] __maybe_unused = {
 	{"sx9500", 0},
 	{ },
 };
-- 
2.34.1

Re: [PATCH 2/4] iio: dac: ad5755: mark OF related data as maybe unused

[Jonathan Cameron]

On Sat, 11 Mar 2023 12:14:55 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> The driver can be compile tested with !CONFIG_OF making certain data
> unused (of_device_id is not used for device matching):

It should be used for device matching I think, so I'd rather see
it assigned for that purpose than hiding the issue.


> 
>   drivers/iio/dac/ad5755.c:865:34: error: ‘ad5755_of_match’ defined but not used [-Werror=unused-const-variable=]
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/iio/dac/ad5755.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c
> index beadfa938d2d..a6dc8a0bfc29 100644
> --- a/drivers/iio/dac/ad5755.c
> +++ b/drivers/iio/dac/ad5755.c
> @@ -862,7 +862,7 @@ static const struct spi_device_id ad5755_id[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, ad5755_id);
>  
> -static const struct of_device_id ad5755_of_match[] = {
> +static const struct of_device_id ad5755_of_match[] __maybe_unused = {
>  	{ .compatible = "adi,ad5755" },
>  	{ .compatible = "adi,ad5755-1" },
>  	{ .compatible = "adi,ad5757" },

Re: [PATCH 1/4] iio: adc: rcar-gyroadc: mark OF related data as maybe unused

[Jonathan Cameron]

On Sat, 11 Mar 2023 12:14:54 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> The driver can be compile tested with !CONFIG_OF making certain data
> unused:
> 
>   drivers/iio/adc/rcar-gyroadc.c:286:34: error: ‘rcar_gyroadc_child_match’ defined but not used [-Werror=unused-const-variable=]
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Whilst in general I'd prefer to see these converted to generic fw properties, this
one is not trivial. So applied to the togreg branch of iio.git.

It would be nice if anyone has time to circle back to this and get rid of
the of specific stuff.

Jonathan


> ---
>  drivers/iio/adc/rcar-gyroadc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
> index 27d9e147b4b7..b8972f673c9d 100644
> --- a/drivers/iio/adc/rcar-gyroadc.c
> +++ b/drivers/iio/adc/rcar-gyroadc.c
> @@ -283,7 +283,7 @@ static const struct of_device_id rcar_gyroadc_match[] = {
>  
>  MODULE_DEVICE_TABLE(of, rcar_gyroadc_match);
>  
> -static const struct of_device_id rcar_gyroadc_child_match[] = {
> +static const struct of_device_id rcar_gyroadc_child_match[] __maybe_unused = {
>  	/* Mode 1 ADCs */
>  	{
>  		.compatible	= "fujitsu,mb88101a",

Re: [PATCH 2/4] iio: dac: ad5755: mark OF related data as maybe unused

[Krzysztof Kozlowski]

On 11/03/2023 13:22, Jonathan Cameron wrote:
> On Sat, 11 Mar 2023 12:14:55 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> The driver can be compile tested with !CONFIG_OF making certain data
>> unused (of_device_id is not used for device matching):
> 
> It should be used for device matching I think, so I'd rather see
> it assigned for that purpose than hiding the issue.

That would require testing and changes. The device matching is via SPI
table which has device data. Probably adding OF matching would require
bigger changes to for handling the match data.

This was intentional design in this driver, so we are not hiding here
anything.

> 
> 
>>
>>   drivers/iio/dac/ad5755.c:865:34: error: ‘ad5755_of_match’ defined but not used [-Werror=unused-const-variable=]

> 

Best regards,
Krzysztof

Re: [PATCH 1/4] iio: adc: rcar-gyroadc: mark OF related data as maybe unused

[Krzysztof Kozlowski]

On 11/03/2023 13:23, Jonathan Cameron wrote:
> On Sat, 11 Mar 2023 12:14:54 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> The driver can be compile tested with !CONFIG_OF making certain data
>> unused:
>>
>>   drivers/iio/adc/rcar-gyroadc.c:286:34: error: ‘rcar_gyroadc_child_match’ defined but not used [-Werror=unused-const-variable=]
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Whilst in general I'd prefer to see these converted to generic fw properties, this
> one is not trivial. So applied to the togreg branch of iio.git.
> 
> It would be nice if anyone has time to circle back to this and get rid of
> the of specific stuff.

This is device ID table. What do you want to remove here? What OF part?

Best regards,
Krzysztof

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

[Jonathan Cameron]

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

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

[Krzysztof Kozlowski]

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

Re: [PATCH 4/4] iio: proximity: sx9500: Mark ACPI and OF related data as maybe unused

[Jonathan Cameron]

On Sat, 11 Mar 2023 12:14:57 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making
> certain data unused:
> 
>   drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=]
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Hi Krysztof

Thanks for looking at these warnings. 

Drop the protection macros instead.  The tables are trivial in size and
the of_match_ptr() breaks some ways this driver can be used.
ACPI_PTR() isn't as bad, but is pretty much pointless given this size of
the array. 

Jonathan


> ---
>  drivers/iio/proximity/sx9500.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index 8794e75e5bf9..840db1953998 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -1036,13 +1036,13 @@ static const struct acpi_device_id sx9500_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match);
>  
> -static const struct of_device_id sx9500_of_match[] = {
> +static const struct of_device_id sx9500_of_match[] __maybe_unused = {
>  	{ .compatible = "semtech,sx9500", },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, sx9500_of_match);
>  
> -static const struct i2c_device_id sx9500_id[] = {
> +static const struct i2c_device_id sx9500_id[] __maybe_unused = {
>  	{"sx9500", 0},
>  	{ },
>  };

Re: [PATCH 4/4] iio: proximity: sx9500: Mark ACPI and OF related data as maybe unused

[Krzysztof Kozlowski]

On 11/03/2023 13:28, Jonathan Cameron wrote:
> On Sat, 11 Mar 2023 12:14:57 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making
>> certain data unused:
>>
>>   drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=]
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Hi Krysztof
> 
> Thanks for looking at these warnings. 
> 
> Drop the protection macros instead.  The tables are trivial in size and
> the of_match_ptr() breaks some ways this driver can be used.
> ACPI_PTR() isn't as bad, but is pretty much pointless given this size of
> the array. 
> 

For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF
platform, OF table is used.

What usage exactly is broken here? What ways?

Best regards,
Krzysztof

Re: [PATCH 2/4] iio: dac: ad5755: mark OF related data as maybe unused

[Jonathan Cameron]

On Sat, 11 Mar 2023 13:25:33 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 11/03/2023 13:22, Jonathan Cameron wrote:
> > On Sat, 11 Mar 2023 12:14:55 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> >> The driver can be compile tested with !CONFIG_OF making certain data
> >> unused (of_device_id is not used for device matching):  
> > 
> > It should be used for device matching I think, so I'd rather see
> > it assigned for that purpose than hiding the issue.  
> 
> That would require testing and changes. The device matching is via SPI
> table which has device data. Probably adding OF matching would require
> bigger changes to for handling the match data.
> 
> This was intentional design in this driver, so we are not hiding here
> anything.

I doubt it was intentional. Mostly people do this because the magic
fallbacks to find the spi_device_id entry work.

If we'd noticed at review time it would not have gone in like this.
Note that the spi_match_id() use of_modalias_node() which has stripped the
vendor id off the compatible then matches against the spi_device_id
table.

So it 'should' just work.  Now ideally we'd switch to
spi_get_device_match_data() but that needs more significant changes.
Though simple enough ones that review would be enough.

Just need to use pointers to the ad75755_chip_info_tbl entries
rather than the enum in both the spi id table and the of one - this
avoids the issue with the enum value of 0 counting as a failed match.

> 
> > 
> >   
> >>
> >>   drivers/iio/dac/ad5755.c:865:34: error: ‘ad5755_of_match’ defined but not used [-Werror=unused-const-variable=]  
> 
> >   
> 
> Best regards,
> Krzysztof
> 

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

[Jonathan Cameron]

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
> 

Re: [PATCH 4/4] iio: proximity: sx9500: Mark ACPI and OF related data as maybe unused

[Jonathan Cameron]

On Sat, 11 Mar 2023 13:30:01 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 11/03/2023 13:28, Jonathan Cameron wrote:
> > On Sat, 11 Mar 2023 12:14:57 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> >> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making
> >> certain data unused:
> >>
> >>   drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=]
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>  
> > 
> > Hi Krysztof
> > 
> > Thanks for looking at these warnings. 
> > 
> > Drop the protection macros instead.  The tables are trivial in size and
> > the of_match_ptr() breaks some ways this driver can be used.
> > ACPI_PTR() isn't as bad, but is pretty much pointless given this size of
> > the array. 
> >   
> 
> For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF
> platform, OF table is used.

So you would think, but nope.. That's not how it works (I was surprised
when I came across this the first time too)
 
PRP0001 is magic and requires no specific support in an individual
driver beyond not using that of_match_ptr() macro!

https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L754
Docs here
https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst#L450
> 
> What usage exactly is broken here? What ways?
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/4] iio: adc: rcar-gyroadc: mark OF related data as maybe unused

[Jonathan Cameron]

On Sat, 11 Mar 2023 13:26:12 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 11/03/2023 13:23, Jonathan Cameron wrote:
> > On Sat, 11 Mar 2023 12:14:54 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> >> The driver can be compile tested with !CONFIG_OF making certain data
> >> unused:
> >>
> >>   drivers/iio/adc/rcar-gyroadc.c:286:34: error: ‘rcar_gyroadc_child_match’ defined but not used [-Werror=unused-const-variable=]
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>  
> > 
> > Whilst in general I'd prefer to see these converted to generic fw properties, this
> > one is not trivial. So applied to the togreg branch of iio.git.
> > 
> > It would be nice if anyone has time to circle back to this and get rid of
> > the of specific stuff.  
> 
> This is device ID table. What do you want to remove here? What OF part?

All of it (well not the table, but the code that directly accesses the
table).  As a general rule I'd like to see all driver in IIO
using the generic firmware accessors in property.h so that they work
with PRP0001 and other firmware types (see reply to later patch)

It's understandable that a given driver is only used on dt based
platforms, but that doesn't mean I wouldn't prefer generic code
if we can do it easily.  It becomes easier to review and better as a source
of code that might get copied into future drivers.

Jonathan

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/4] iio: dac: ad5755: mark OF related data as maybe unused

[Krzysztof Kozlowski]

On 11/03/2023 19:31, Jonathan Cameron wrote:
> On Sat, 11 Mar 2023 13:25:33 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 11/03/2023 13:22, Jonathan Cameron wrote:
>>> On Sat, 11 Mar 2023 12:14:55 +0100
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>   
>>>> The driver can be compile tested with !CONFIG_OF making certain data
>>>> unused (of_device_id is not used for device matching):  
>>>
>>> It should be used for device matching I think, so I'd rather see
>>> it assigned for that purpose than hiding the issue.  
>>
>> That would require testing and changes. The device matching is via SPI
>> table which has device data. Probably adding OF matching would require
>> bigger changes to for handling the match data.
>>
>> This was intentional design in this driver, so we are not hiding here
>> anything.
> 
> I doubt it was intentional. Mostly people do this because the magic
> fallbacks to find the spi_device_id entry work.
> 
> If we'd noticed at review time it would not have gone in like this.
> Note that the spi_match_id() use of_modalias_node() which has stripped the
> vendor id off the compatible then matches against the spi_device_id
> table.
> 
> So it 'should' just work.  Now ideally we'd switch to
> spi_get_device_match_data() but that needs more significant changes.
> Though simple enough ones that review would be enough.
> 
> Just need to use pointers to the ad75755_chip_info_tbl entries
> rather than the enum in both the spi id table and the of one - this
> avoids the issue with the enum value of 0 counting as a failed match.

It's not that simple change... maybe you are right that adding match
data to OF table would not break anything, but to me it is something
substantial and requiring testing, which obviously I cannot do.
Therefore I am going to skip this one (thus the error stays).

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

Re: [PATCH 4/4] iio: proximity: sx9500: Mark ACPI and OF related data as maybe unused

[Krzysztof Kozlowski]

On 11/03/2023 19:44, Jonathan Cameron wrote:
> On Sat, 11 Mar 2023 13:30:01 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 11/03/2023 13:28, Jonathan Cameron wrote:
>>> On Sat, 11 Mar 2023 12:14:57 +0100
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>   
>>>> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making
>>>> certain data unused:
>>>>
>>>>   drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=]
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>  
>>>
>>> Hi Krysztof
>>>
>>> Thanks for looking at these warnings. 
>>>
>>> Drop the protection macros instead.  The tables are trivial in size and
>>> the of_match_ptr() breaks some ways this driver can be used.
>>> ACPI_PTR() isn't as bad, but is pretty much pointless given this size of
>>> the array. 
>>>   
>>
>> For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF
>> platform, OF table is used.
> 
> So you would think, but nope.. That's not how it works (I was surprised
> when I came across this the first time too)
>  
> PRP0001 is magic and requires no specific support in an individual
> driver beyond not using that of_match_ptr() macro!

I know, we talk about ACPI table.

> 
> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L754
> Docs here
> https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst#L450

The code is compile when CONFIG_ACPI is defined, right? Then you have
ACPI table, so what for ACPI platform is missing? ACPI platform has ACPI
table.

Best regards,
Krzysztof

Re: [PATCH 4/4] iio: proximity: sx9500: Mark ACPI and OF related data as maybe unused

[Jonathan Cameron]

On Sun, 12 Mar 2023 11:17:05 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 11/03/2023 19:44, Jonathan Cameron wrote:
> > On Sat, 11 Mar 2023 13:30:01 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> >> On 11/03/2023 13:28, Jonathan Cameron wrote:  
> >>> On Sat, 11 Mar 2023 12:14:57 +0100
> >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>>     
> >>>> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making
> >>>> certain data unused:
> >>>>
> >>>>   drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=]
> >>>>
> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>    
> >>>
> >>> Hi Krysztof
> >>>
> >>> Thanks for looking at these warnings. 
> >>>
> >>> Drop the protection macros instead.  The tables are trivial in size and
> >>> the of_match_ptr() breaks some ways this driver can be used.
> >>> ACPI_PTR() isn't as bad, but is pretty much pointless given this size of
> >>> the array. 
> >>>     
> >>
> >> For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF
> >> platform, OF table is used.  
> > 
> > So you would think, but nope.. That's not how it works (I was surprised
> > when I came across this the first time too)
> >  
> > PRP0001 is magic and requires no specific support in an individual
> > driver beyond not using that of_match_ptr() macro!  
> 
> I know, we talk about ACPI table.

I'm not sure I follow.   I thought by ACPI table you meant the acpi_device_id
table pointed to by acpi_match_table in struct device_driver.

That one is not needed for PRP0001.  It is irrelevant if there is one or not.

Maybe the confusion is that you think the presence of an acpi_match table means
we don't also check PRP0001?  As you can see here
https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L886
it is checked in all cases.

If you meant the DSDT table being provide by the firmware I don't see the relevance
to this discussion.

> 
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L754
> > Docs here
> > https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst#L450  
> 
> The code is compile when CONFIG_ACPI is defined, right? Then you have
> ACPI table, so what for ACPI platform is missing? ACPI platform has ACPI
> table.

I don't follow.  A given ACPI platform may provide in DSDT one of two choices
to bind to this driver.

Either it provides an entry from the acpi_device_id table, or it must
use PRP0001 and a compatible entry from the of_device_id table.  That only works
if of_match_ptr() is not used.

As a side note, both the IDs in the ACPI match table are not valid IDs for use
in DSDT.  We are supporting them only because they have been used on shipping devices.
Semtech does have a PNP ID of STH but that's not the one used.

Anyhow, to be clear. For IIO drivers, don't use of_match_ptr()
or ACPI_PTR(). There are some legacy cases that we haven't cleaned up
yet, but I'm not taking patches that add any new ones without a very very
strong argument in favour and so far no one has successfully made one.

Jonathan


> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 4/4] iio: proximity: sx9500: Mark ACPI and OF related data as maybe unused

[Krzysztof Kozlowski]

On 12/03/2023 15:14, Jonathan Cameron wrote:
> On Sun, 12 Mar 2023 11:17:05 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 11/03/2023 19:44, Jonathan Cameron wrote:
>>> On Sat, 11 Mar 2023 13:30:01 +0100
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>   
>>>> On 11/03/2023 13:28, Jonathan Cameron wrote:  
>>>>> On Sat, 11 Mar 2023 12:14:57 +0100
>>>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>>>     
>>>>>> The driver can be compile tested with !CONFIG_OF or !CONFIG_ACPI making
>>>>>> certain data unused:
>>>>>>
>>>>>>   drivers/iio/proximity/sx9500.c:1039:34: error: ‘sx9500_of_match’ defined but not used [-Werror=unused-const-variable=]
>>>>>>
>>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>    
>>>>>
>>>>> Hi Krysztof
>>>>>
>>>>> Thanks for looking at these warnings. 
>>>>>
>>>>> Drop the protection macros instead.  The tables are trivial in size and
>>>>> the of_match_ptr() breaks some ways this driver can be used.
>>>>> ACPI_PTR() isn't as bad, but is pretty much pointless given this size of
>>>>> the array. 
>>>>>     
>>>>
>>>> For ACPI platform, ACPI table is used, so nothing for PRP0001. For OF
>>>> platform, OF table is used.  
>>>
>>> So you would think, but nope.. That's not how it works (I was surprised
>>> when I came across this the first time too)
>>>  
>>> PRP0001 is magic and requires no specific support in an individual
>>> driver beyond not using that of_match_ptr() macro!  
>>
>> I know, we talk about ACPI table.
> 
> I'm not sure I follow.   I thought by ACPI table you meant the acpi_device_id
> table pointed to by acpi_match_table in struct device_driver.
> 
> That one is not needed for PRP0001.  It is irrelevant if there is one or not.
> 
> Maybe the confusion is that you think the presence of an acpi_match table means
> we don't also check PRP0001?  As you can see here
> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L886
> it is checked in all cases.
> 
> If you meant the DSDT table being provide by the firmware I don't see the relevance
> to this discussion.
> 
>>
>>>
>>> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/bus.c#L754
>>> Docs here
>>> https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst#L450  
>>
>> The code is compile when CONFIG_ACPI is defined, right? Then you have
>> ACPI table, so what for ACPI platform is missing? ACPI platform has ACPI
>> table.
> 
> I don't follow.  A given ACPI platform may provide in DSDT one of two choices
> to bind to this driver.

OK, I understand your point. I assumed we do not care at all about
PRP0001 if ACPI is enabled, because then we simply use ACPI table. But
indeed they might for example be not in sync...

> 
> Either it provides an entry from the acpi_device_id table, or it must
> use PRP0001 and a compatible entry from the of_device_id table.  That only works
> if of_match_ptr() is not used.
> 
> As a side note, both the IDs in the ACPI match table are not valid IDs for use
> in DSDT.  We are supporting them only because they have been used on shipping devices.
> Semtech does have a PNP ID of STH but that's not the one used.
> 
> Anyhow, to be clear. For IIO drivers, don't use of_match_ptr()
> or ACPI_PTR(). There are some legacy cases that we haven't cleaned up
> yet, but I'm not taking patches that add any new ones without a very very
> strong argument in favour and so far no one has successfully made one.


Best regards,
Krzysztof