[1/3] platform: olpc: mark SPI related data as maybe unused

  The driver can be compile tested as built-in making certain data unused:

  drivers/platform/olpc/olpc-xo175-ec.c:737:35: error: ‘olpc_xo175_ec_id_table’ defined but not used [-Werror=unused-const-variable=]

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

Hi Krzysztof,

On 3/12/23 14:26, Krzysztof Kozlowski wrote:
> The driver can be compile tested as built-in making certain data unused:
> 
>   drivers/platform/olpc/olpc-xo175-ec.c:737:35: error: ‘olpc_xo175_ec_id_table’ defined but not used [-Werror=unused-const-variable=]
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/platform/olpc/olpc-xo175-ec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/olpc/olpc-xo175-ec.c b/drivers/platform/olpc/olpc-xo175-ec.c
> index 4823bd2819f6..04573495ef5a 100644
> --- a/drivers/platform/olpc/olpc-xo175-ec.c
> +++ b/drivers/platform/olpc/olpc-xo175-ec.c
> @@ -734,7 +734,7 @@ static const struct of_device_id olpc_xo175_ec_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, olpc_xo175_ec_of_match);
>  
> -static const struct spi_device_id olpc_xo175_ec_id_table[] = {
> +static const struct spi_device_id olpc_xo175_ec_id_table[] __maybe_unused = {
>  	{ "xo1.75-ec", 0 },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(spi, olpc_xo175_ec_id_table);

The whole presence of the olpc_xo175_ec_id_table[] seems to make little sense.

This should be referenced by:

static struct spi_driver olpc_xo175_ec_spi_driver = {

Like this:

        .probe          = olpc_xo175_ec_probe,
        .remove         = olpc_xo175_ec_remove,
+       .id_table       = olpc_xo175_ec_id_table,

Otherwise those ids cannot be used to load the driver the non DT/of way. Since the driver assumingly does actually bind already this means that it is only used the DT/of way and it seems to me that the whole olpc_xo175_ec_id_table[] can be removed entirely.

Exposing modaliases for a non supported way of binding the driver does not really seem useful ?

Patches 2/3 and 3/3 do make sense, I'll merge those soonish.

Regards,

Hans
  

On 16/03/2023 13:50, Hans de Goede wrote:
> Hi Krzysztof,
> 
> On 3/12/23 14:26, Krzysztof Kozlowski wrote:
>> The driver can be compile tested as built-in making certain data unused:
>>
>>   drivers/platform/olpc/olpc-xo175-ec.c:737:35: error: ‘olpc_xo175_ec_id_table’ defined but not used [-Werror=unused-const-variable=]
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  drivers/platform/olpc/olpc-xo175-ec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/olpc/olpc-xo175-ec.c b/drivers/platform/olpc/olpc-xo175-ec.c
>> index 4823bd2819f6..04573495ef5a 100644
>> --- a/drivers/platform/olpc/olpc-xo175-ec.c
>> +++ b/drivers/platform/olpc/olpc-xo175-ec.c
>> @@ -734,7 +734,7 @@ static const struct of_device_id olpc_xo175_ec_of_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, olpc_xo175_ec_of_match);
>>  
>> -static const struct spi_device_id olpc_xo175_ec_id_table[] = {
>> +static const struct spi_device_id olpc_xo175_ec_id_table[] __maybe_unused = {
>>  	{ "xo1.75-ec", 0 },
>>  	{}
>>  };
>>  MODULE_DEVICE_TABLE(spi, olpc_xo175_ec_id_table);
> 
> The whole presence of the olpc_xo175_ec_id_table[] seems to make little sense.
> 
> This should be referenced by:
> 
> static struct spi_driver olpc_xo175_ec_spi_driver = {
> 
> Like this:
> 
>         .probe          = olpc_xo175_ec_probe,
>         .remove         = olpc_xo175_ec_remove,
> +       .id_table       = olpc_xo175_ec_id_table,

Yes, it should.

> 
> Otherwise those ids cannot be used to load the driver the non DT/of way. Since the driver assumingly does actually bind already this means that it is only used the DT/of way and it seems to me that the whole olpc_xo175_ec_id_table[] can be removed entirely.
> 
> Exposing modaliases for a non supported way of binding the driver does not really seem useful ?

However binding the device and module loading (uevent) uses sometimes
different pieces. Maybe something changed in kernel, but sometime ago
certain buses where sending uevent for module loading with one ID (e.g.
platform or spi bus) but device matching would be according to OF. Thus
if you did not have entries in spi_device_id, the module would not autoload.

It was exactly the case for example here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c46ed2281bbe4b84e6f3d4bdfb0e4e9ab813fa9d&context=30&ignorews=0&dt=0

You needed spi_device_id for proper module autoloading.

Unless something change in between in the kernel?

(+CC Javier, author of that commit)

Best regards,
Krzysztof
  

Hi,

On 3/16/23 15:07, Krzysztof Kozlowski wrote:
> On 16/03/2023 13:50, Hans de Goede wrote:
>> Hi Krzysztof,
>>
>> On 3/12/23 14:26, Krzysztof Kozlowski wrote:
>>> The driver can be compile tested as built-in making certain data unused:
>>>
>>>   drivers/platform/olpc/olpc-xo175-ec.c:737:35: error: ‘olpc_xo175_ec_id_table’ defined but not used [-Werror=unused-const-variable=]
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> ---
>>>  drivers/platform/olpc/olpc-xo175-ec.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/olpc/olpc-xo175-ec.c b/drivers/platform/olpc/olpc-xo175-ec.c
>>> index 4823bd2819f6..04573495ef5a 100644
>>> --- a/drivers/platform/olpc/olpc-xo175-ec.c
>>> +++ b/drivers/platform/olpc/olpc-xo175-ec.c
>>> @@ -734,7 +734,7 @@ static const struct of_device_id olpc_xo175_ec_of_match[] = {
>>>  };
>>>  MODULE_DEVICE_TABLE(of, olpc_xo175_ec_of_match);
>>>  
>>> -static const struct spi_device_id olpc_xo175_ec_id_table[] = {
>>> +static const struct spi_device_id olpc_xo175_ec_id_table[] __maybe_unused = {
>>>  	{ "xo1.75-ec", 0 },
>>>  	{}
>>>  };
>>>  MODULE_DEVICE_TABLE(spi, olpc_xo175_ec_id_table);
>>
>> The whole presence of the olpc_xo175_ec_id_table[] seems to make little sense.
>>
>> This should be referenced by:
>>
>> static struct spi_driver olpc_xo175_ec_spi_driver = {
>>
>> Like this:
>>
>>         .probe          = olpc_xo175_ec_probe,
>>         .remove         = olpc_xo175_ec_remove,
>> +       .id_table       = olpc_xo175_ec_id_table,
> 
> Yes, it should.
> 
>>
>> Otherwise those ids cannot be used to load the driver the non DT/of way. Since the driver assumingly does actually bind already this means that it is only used the DT/of way and it seems to me that the whole olpc_xo175_ec_id_table[] can be removed entirely.
>>
>> Exposing modaliases for a non supported way of binding the driver does not really seem useful ?
> 
> However binding the device and module loading (uevent) uses sometimes
> different pieces. Maybe something changed in kernel, but sometime ago
> certain buses where sending uevent for module loading with one ID (e.g.
> platform or spi bus) but device matching would be according to OF. Thus
> if you did not have entries in spi_device_id, the module would not autoload.
> 
> It was exactly the case for example here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c46ed2281bbe4b84e6f3d4bdfb0e4e9ab813fa9d&context=30&ignorews=0&dt=0
> 
> You needed spi_device_id for proper module autoloading.
> 
> Unless something change in between in the kernel?

Looks like your right, the spi_uevent() code always emits "spi:xxxxxxx" style modalias even for dt/of enumerated devices:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c#n398

So the table needs to stay.

Can you do a v2 (of just this patch) adding an id_table entry to olpc_xo175_ec_spi_driver rather then using __maybe_unused ?

Regards,

Hans
  

On 16/03/2023 15:13, Hans de Goede wrote:
> 
> Looks like your right, the spi_uevent() code always emits "spi:xxxxxxx" style modalias even for dt/of enumerated devices:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c#n398
> 
> So the table needs to stay.
> 
> Can you do a v2 (of just this patch) adding an id_table entry to olpc_xo175_ec_spi_driver rather then using __maybe_unused ?

Yes, sure.


Best regards,
Krzysztof
  

Hans de Goede <hdegoede@redhat.com> writes:

[...]

>>> Exposing modaliases for a non supported way of binding the driver does not really seem useful ?
>> 
>> However binding the device and module loading (uevent) uses sometimes
>> different pieces. Maybe something changed in kernel, but sometime ago
>> certain buses where sending uevent for module loading with one ID (e.g.
>> platform or spi bus) but device matching would be according to OF. Thus
>> if you did not have entries in spi_device_id, the module would not autoload.
>> 
>> It was exactly the case for example here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c46ed2281bbe4b84e6f3d4bdfb0e4e9ab813fa9d&context=30&ignorews=0&dt=0
>> 
>> You needed spi_device_id for proper module autoloading.
>> 
>> Unless something change in between in the kernel?
>
> Looks like your right, the spi_uevent() code always emits "spi:xxxxxxx" style modalias even for dt/of enumerated devices:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c#n398
>
> So the table needs to stay.
>

Yeah, and in fact dropping the spi_device_id table will cause the kernel
to warn that an spi_device_id entry doesn't exist for a given of_device_id
entry since commit 5fa6863ba692 ("spi: Check we have a spi_device_id for
each DT compatible").

Fixing that is not trivial because a lot of drivers are rely on current
behaviour of the SPI core always returning a spi:<dev> modalias. So don't
even have an OF table, even when the SPI devices are instantiated by DT.