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

[Jonathan Cameron <jic23@kernel.org>]

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
>