drm/ssd130x: Set SPI .id_table to prevent an SPI core warning

[PATCH] drm/ssd130x: Set SPI .id_table to prevent an SPI core warning

Posted by Javier Martinez Canillas 1 year, 4 months ago

The only reason for the ssd130x-spi driver to have an spi_device_id table
is that the SPI core always reports an "spi:" MODALIAS, even when the SPI
device has been registered via a Device Tree Blob.

Without spi_device_id table information in the module's metadata, module
autoloading would not work because there won't be an alias that matches
the MODALIAS reported by the SPI core.

This spi_device_id table is not needed for device matching though, since
the of_device_id table is always used in this case. For this reason, the
struct spi_driver .id_table field is currently not set in the SPI driver.

Because the spi_device_id table is always required for module autoloading,
the SPI core checks during driver registration that both an of_device_id
table and a spi_device_id table are present and that they contain the same
entries for all the SPI devices.

Not setting the .id_table field in the driver then confuses the core and
leads to the following warning when the ssd130x-spi driver is registered:

  [   41.091198] SPI driver ssd130x-spi has no spi_device_id for sinowealth,sh1106
  [   41.098614] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1305
  [   41.105862] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1306
  [   41.113062] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1307
  [   41.120247] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1309
  [   41.127449] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1322
  [   41.134627] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1325
  [   41.141784] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1327
  [   41.149021] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1331

To prevent the warning, set the .id_table even though it's not necessary.

Since the check is done even for built-in drivers, drop the condition to
only define the ID table when the driver is built as a module. Finally,
rename the variable to use the "_spi_id" convention used for ID tables.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/solomon/ssd130x-spi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c b/drivers/gpu/drm/solomon/ssd130x-spi.c
index 08334be38694..7c935870f7d2 100644
--- a/drivers/gpu/drm/solomon/ssd130x-spi.c
+++ b/drivers/gpu/drm/solomon/ssd130x-spi.c
@@ -151,7 +151,6 @@ static const struct of_device_id ssd130x_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ssd130x_of_match);
 
-#if IS_MODULE(CONFIG_DRM_SSD130X_SPI)
 /*
  * The SPI core always reports a MODALIAS uevent of the form "spi:<dev>", even
  * if the device was registered via OF. This means that the module will not be
@@ -160,7 +159,7 @@ MODULE_DEVICE_TABLE(of, ssd130x_of_match);
  * To workaround this issue, add a SPI device ID table. Even when this should
  * not be needed for this driver to match the registered SPI devices.
  */
-static const struct spi_device_id ssd130x_spi_table[] = {
+static const struct spi_device_id ssd130x_spi_id[] = {
 	/* ssd130x family */
 	{ "sh1106",  SH1106_ID },
 	{ "ssd1305", SSD1305_ID },
@@ -175,14 +174,14 @@ static const struct spi_device_id ssd130x_spi_table[] = {
 	{ "ssd1331", SSD1331_ID },
 	{ /* sentinel */ }
 };
-MODULE_DEVICE_TABLE(spi, ssd130x_spi_table);
-#endif
+MODULE_DEVICE_TABLE(spi, ssd130x_spi_id);
 
 static struct spi_driver ssd130x_spi_driver = {
 	.driver = {
 		.name = DRIVER_NAME,
 		.of_match_table = ssd130x_of_match,
 	},
+	.id_table = ssd130x_spi_id,
 	.probe = ssd130x_spi_probe,
 	.remove = ssd130x_spi_remove,
 	.shutdown = ssd130x_spi_shutdown,
-- 
2.47.1

Re: [PATCH] drm/ssd130x: Set SPI .id_table to prevent an SPI core warning

Posted by Dmitry Baryshkov 1 year, 4 months ago

On Tue, Dec 31, 2024 at 12:44:58PM +0100, Javier Martinez Canillas wrote:
> The only reason for the ssd130x-spi driver to have an spi_device_id table
> is that the SPI core always reports an "spi:" MODALIAS, even when the SPI
> device has been registered via a Device Tree Blob.
> 
> Without spi_device_id table information in the module's metadata, module
> autoloading would not work because there won't be an alias that matches
> the MODALIAS reported by the SPI core.
> 
> This spi_device_id table is not needed for device matching though, since
> the of_device_id table is always used in this case. For this reason, the
> struct spi_driver .id_table field is currently not set in the SPI driver.
> 
> Because the spi_device_id table is always required for module autoloading,
> the SPI core checks during driver registration that both an of_device_id
> table and a spi_device_id table are present and that they contain the same
> entries for all the SPI devices.
> 
> Not setting the .id_table field in the driver then confuses the core and
> leads to the following warning when the ssd130x-spi driver is registered:
> 
>   [   41.091198] SPI driver ssd130x-spi has no spi_device_id for sinowealth,sh1106
>   [   41.098614] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1305
>   [   41.105862] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1306
>   [   41.113062] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1307
>   [   41.120247] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1309
>   [   41.127449] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1322
>   [   41.134627] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1325
>   [   41.141784] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1327
>   [   41.149021] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1331
> 
> To prevent the warning, set the .id_table even though it's not necessary.
> 
> Since the check is done even for built-in drivers, drop the condition to
> only define the ID table when the driver is built as a module. Finally,
> rename the variable to use the "_spi_id" convention used for ID tables.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Fixes: 74373977d2ca ("drm/solomon: Add SSD130x OLED displays SPI support")

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> ---
> 
>  drivers/gpu/drm/solomon/ssd130x-spi.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH] drm/ssd130x: Set SPI .id_table to prevent an SPI core warning

Posted by Javier Martinez Canillas 1 year, 4 months ago

Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:

Hello Dmitry,

> On Tue, Dec 31, 2024 at 12:44:58PM +0100, Javier Martinez Canillas wrote:
>> The only reason for the ssd130x-spi driver to have an spi_device_id table
>> is that the SPI core always reports an "spi:" MODALIAS, even when the SPI
>> device has been registered via a Device Tree Blob.
>> 
>> Without spi_device_id table information in the module's metadata, module
>> autoloading would not work because there won't be an alias that matches
>> the MODALIAS reported by the SPI core.
>> 
>> This spi_device_id table is not needed for device matching though, since
>> the of_device_id table is always used in this case. For this reason, the
>> struct spi_driver .id_table field is currently not set in the SPI driver.
>> 
>> Because the spi_device_id table is always required for module autoloading,
>> the SPI core checks during driver registration that both an of_device_id
>> table and a spi_device_id table are present and that they contain the same
>> entries for all the SPI devices.
>> 
>> Not setting the .id_table field in the driver then confuses the core and
>> leads to the following warning when the ssd130x-spi driver is registered:
>> 
>>   [   41.091198] SPI driver ssd130x-spi has no spi_device_id for sinowealth,sh1106
>>   [   41.098614] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1305
>>   [   41.105862] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1306
>>   [   41.113062] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1307
>>   [   41.120247] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1309
>>   [   41.127449] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1322
>>   [   41.134627] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1325
>>   [   41.141784] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1327
>>   [   41.149021] SPI driver ssd130x-spi has no spi_device_id for solomon,ssd1331
>> 
>> To prevent the warning, set the .id_table even though it's not necessary.
>> 
>> Since the check is done even for built-in drivers, drop the condition to
>> only define the ID table when the driver is built as a module. Finally,
>> rename the variable to use the "_spi_id" convention used for ID tables.
>> 
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>
> Fixes: 74373977d2ca ("drm/solomon: Add SSD130x OLED displays SPI support")
>

I was on the fence about adding a Fixes: tag due a) the issue being there
from the beginning as you pointed out and b) the warning being harmless.

But I'll add it to v2 or just before pushing it to drm-misc-next.

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>

Thanks for your review!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/ssd130x: Set SPI .id_table to prevent an SPI core warning

Posted by Dmitry Baryshkov 1 year, 4 months ago

On Tue, Dec 31, 2024 at 04:34:34PM +0100, Javier Martinez Canillas wrote:
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:
> 
> Hello Dmitry,
> 
> > On Tue, Dec 31, 2024 at 12:44:58PM +0100, Javier Martinez Canillas wrote:

[...]

> >> Since the check is done even for built-in drivers, drop the condition to
> >> only define the ID table when the driver is built as a module. Finally,
> >> rename the variable to use the "_spi_id" convention used for ID tables.
> >> 
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >
> > Fixes: 74373977d2ca ("drm/solomon: Add SSD130x OLED displays SPI support")
> >
> 
> I was on the fence about adding a Fixes: tag due a) the issue being there
> from the beginning as you pointed out and b) the warning being harmless.
> 
> But I'll add it to v2 or just before pushing it to drm-misc-next.

Just before pushing is enough. dim b4-shazam can do that for you.

> 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> 
> Thanks for your review!
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

-- 
With best wishes
Dmitry

Re: [PATCH] drm/ssd130x: Set SPI .id_table to prevent an SPI core warning

Posted by Javier Martinez Canillas 1 year, 4 months ago

Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:

> On Tue, Dec 31, 2024 at 04:34:34PM +0100, Javier Martinez Canillas wrote:
>> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:
>> 
>> Hello Dmitry,
>> 
>> > On Tue, Dec 31, 2024 at 12:44:58PM +0100, Javier Martinez Canillas wrote:
>
> [...]
>
>> >> Since the check is done even for built-in drivers, drop the condition to
>> >> only define the ID table when the driver is built as a module. Finally,
>> >> rename the variable to use the "_spi_id" convention used for ID tables.
>> >> 
>> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> >
>> > Fixes: 74373977d2ca ("drm/solomon: Add SSD130x OLED displays SPI support")
>> >
>> 
>> I was on the fence about adding a Fixes: tag due a) the issue being there
>> from the beginning as you pointed out and b) the warning being harmless.
>> 
>> But I'll add it to v2 or just before pushing it to drm-misc-next.
>
> Just before pushing is enough. dim b4-shazam can do that for you.
>

Yeah, I meant in case that I post a v2. I'll wait a few days before
pushing in case someone else chimes in. Thanks again!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/ssd130x: Set SPI .id_table to prevent an SPI core warning

Posted by Javier Martinez Canillas 1 year, 4 months ago

Javier Martinez Canillas <javierm@redhat.com> writes:

> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:
>
>> On Tue, Dec 31, 2024 at 04:34:34PM +0100, Javier Martinez Canillas wrote:
>>> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:
>>> 
>>> Hello Dmitry,
>>> 
>>> > On Tue, Dec 31, 2024 at 12:44:58PM +0100, Javier Martinez Canillas wrote:
>>
>> [...]
>>
>>> >> Since the check is done even for built-in drivers, drop the condition to
>>> >> only define the ID table when the driver is built as a module. Finally,
>>> >> rename the variable to use the "_spi_id" convention used for ID tables.
>>> >> 
>>> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>> >
>>> > Fixes: 74373977d2ca ("drm/solomon: Add SSD130x OLED displays SPI support")
>>> >
>>> 
>>> I was on the fence about adding a Fixes: tag due a) the issue being there
>>> from the beginning as you pointed out and b) the warning being harmless.
>>> 
>>> But I'll add it to v2 or just before pushing it to drm-misc-next.
>>
>> Just before pushing is enough. dim b4-shazam can do that for you.
>>
>
> Yeah, I meant in case that I post a v2. I'll wait a few days before
> pushing in case someone else chimes in. Thanks again!
>

Pushed to drm-misc (drm-misc-next). Thanks!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat