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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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