Message-ID: <f9408067-ee3c-9f5b-d1b3-7017113c8026@linaro.org>
Date:   Thu, 16 Mar 2023 15:07:10 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH 1/3] platform: olpc: mark SPI related data as maybe unused
Content-Language: en-US
To:     Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>,
        Daniel Oliveira Nascimento <don@syst.com.br>,
        Mattia Dongili <malattia@linux.it>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230312132624.352703-1-krzysztof.kozlowski@linaro.org>
 <63814b1b-1297-da4d-79fe-8f2e58e30482@redhat.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Javier Martinez Canillas <javierm@redhat.com>
In-Reply-To: <63814b1b-1297-da4d-79fe-8f2e58e30482@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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