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

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

[Krzysztof Kozlowski]

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 },
 	{}
 };
-- 
2.34.1

[PATCH 2/3] platform: classmate: mark SPI related data as maybe unused

[Krzysztof Kozlowski]

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

  drivers/platform/x86/classmate-laptop.c:1137:36: error: ‘cmpc_device_ids’ defined but not used [-Werror=unused-const-variable=]

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

diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c
index 8b6a14611859..2edaea2492df 100644
--- a/drivers/platform/x86/classmate-laptop.c
+++ b/drivers/platform/x86/classmate-laptop.c
@@ -1134,7 +1134,7 @@ static void cmpc_exit(void)
 module_init(cmpc_init);
 module_exit(cmpc_exit);
 
-static const struct acpi_device_id cmpc_device_ids[] = {
+static const struct acpi_device_id cmpc_device_ids[] __maybe_unused = {
 	{CMPC_ACCEL_HID, 0},
 	{CMPC_ACCEL_HID_V4, 0},
 	{CMPC_TABLET_HID, 0},
-- 
2.34.1

Re: [PATCH 2/3] platform: classmate: mark SPI related data as maybe unused

[Hans de Goede]

Hi,

On 3/12/23 14:26, Krzysztof Kozlowski wrote:
> The driver can be compile tested as built-in making certain data unused:
> 
>   drivers/platform/x86/classmate-laptop.c:1137:36: error: ‘cmpc_device_ids’ defined but not used [-Werror=unused-const-variable=]
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thank you for your patch, I've applied this patch and patch 3/3
to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> ---
>  drivers/platform/x86/classmate-laptop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c
> index 8b6a14611859..2edaea2492df 100644
> --- a/drivers/platform/x86/classmate-laptop.c
> +++ b/drivers/platform/x86/classmate-laptop.c
> @@ -1134,7 +1134,7 @@ static void cmpc_exit(void)
>  module_init(cmpc_init);
>  module_exit(cmpc_exit);
>  
> -static const struct acpi_device_id cmpc_device_ids[] = {
> +static const struct acpi_device_id cmpc_device_ids[] __maybe_unused = {
>  	{CMPC_ACCEL_HID, 0},
>  	{CMPC_ACCEL_HID_V4, 0},
>  	{CMPC_TABLET_HID, 0},

[PATCH 3/3] platform: sony: mark SPI related data as maybe unused

[Krzysztof Kozlowski]

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

  drivers/platform/x86/sony-laptop.c:3290:36: error: ‘sony_device_ids’ defined but not used [-Werror=unused-const-variable=]

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

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 537d6a2d0781..9569f11dec8c 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -3287,7 +3287,7 @@ static void sony_nc_remove(struct acpi_device *device)
 	dprintk(SONY_NC_DRIVER_NAME " removed.\n");
 }
 
-static const struct acpi_device_id sony_device_ids[] = {
+static const struct acpi_device_id sony_device_ids[] __maybe_unused = {
 	{SONY_NC_HID, 0},
 	{SONY_PIC_HID, 0},
 	{"", 0},
-- 
2.34.1

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

[Hans de Goede]

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

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

[Krzysztof Kozlowski]

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

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

[Hans de Goede]

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

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

[Krzysztof Kozlowski]

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

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

[Javier Martinez Canillas]

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.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat