| Message ID | 20241231114516.2063201-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/ssd130x: Set SPI .id_table to prevent an SPI core warning | expand |
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(-) >
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!
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 >
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!
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,
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(-)