| Message ID | 20210922151014.49719-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backlight: hx8357: Add SPI device ID table | expand |
On Wed, Sep 22, 2021 at 04:10:14PM +0100, Mark Brown wrote: > Currently autoloading for SPI devices does not use the DT ID table, it uses > SPI modalises. Supporting OF modalises is going to be difficult if not > impractical, an attempt was made but has been reverted, so ensure that > module autoloading works for this driver by adding a SPI device ID table. > > Fixes: 96c8395e2166 ("spi: Revert modalias changes") > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > drivers/video/backlight/hx8357.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c > index 9b50bc96e00f..c64b1fbe027f 100644 > --- a/drivers/video/backlight/hx8357.c > +++ b/drivers/video/backlight/hx8357.c > @@ -565,6 +565,19 @@ static struct lcd_ops hx8357_ops = { > .get_power = hx8357_get_power, > }; > > +static const struct spi_device_id hx8357_spi_ids[] = { > + { > + .name = "hx8357", > + .driver_data = (kernel_ulong_t)hx8357_lcd_init, Based on this I had expected to find spi_get_device_id() and a ->driver_data somewhere in this patch. Where will this .driver_data be consumed? Daniel. > + }, > + { > + .name = "hx8369", > + .driver_data = (kernel_ulong_t)hx8369_lcd_init, > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(spi, hx8357_spi_ids); > + > static const struct of_device_id hx8357_dt_ids[] = { > { > .compatible = "himax,hx8357", > @@ -672,6 +685,7 @@ static struct spi_driver hx8357_driver = { > .name = "hx8357", > .of_match_table = hx8357_dt_ids, > }, > + .id_table = hx8357_spi_ids, > }; > > module_spi_driver(hx8357_driver); > -- > 2.20.1 >
On Mon, Sep 27, 2021 at 10:42:00AM +0100, Daniel Thompson wrote: > Based on this I had expected to find spi_get_device_id() and a ->driver_data > somewhere in this patch. > Where will this .driver_data be consumed? It will never be consumed unless someone writes a board file - the runtime match will still happen based on the DT compatible, this is only there for the modalias.
On Mon, Sep 27, 2021 at 12:47:27PM +0100, Mark Brown wrote: > On Mon, Sep 27, 2021 at 10:42:00AM +0100, Daniel Thompson wrote: > > > Based on this I had expected to find spi_get_device_id() and a ->driver_data > > somewhere in this patch. > > > Where will this .driver_data be consumed? > > It will never be consumed unless someone writes a board file - the > runtime match will still happen based on the DT compatible, this is only > there for the modalias. Ok... so I'm not going mad. In that case what is the point of including unconsumed driver data? Most DT centric drivers that included this for modalias reasons leave it NULL. I reviewed quite a few drivers this morning and I haven't seen a single one that includes unreachable driver data in this manner. Unless there's a good reason I'd prefer backlight to follow the prior art! Daniel.
On Mon, Sep 27, 2021 at 02:24:17PM +0100, Daniel Thompson wrote: > In that case what is the point of including unconsumed driver data? Most > DT centric drivers that included this for modalias reasons leave it > NULL. It's mainly there because it's generated fairly mechanically from the OF ID table - there's no great reason for it to be there while all instantiation is done via DT.
On Mon, Sep 27, 2021 at 02:30:56PM +0100, Mark Brown wrote: > On Mon, Sep 27, 2021 at 02:24:17PM +0100, Daniel Thompson wrote: > > > In that case what is the point of including unconsumed driver data? Most > > DT centric drivers that included this for modalias reasons leave it > > NULL. > > It's mainly there because it's generated fairly mechanically from the OF > ID table - there's no great reason for it to be there while all > instantiation is done via DT. Ok. If there's no plan to change the driver to pick it up from the table then let's remove the redundant values. They just make it look like somebody forgot something in the probe method (instead of it being a deliberate choice). Daniel.
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c index 9b50bc96e00f..c64b1fbe027f 100644 --- a/drivers/video/backlight/hx8357.c +++ b/drivers/video/backlight/hx8357.c @@ -565,6 +565,19 @@ static struct lcd_ops hx8357_ops = { .get_power = hx8357_get_power, }; +static const struct spi_device_id hx8357_spi_ids[] = { + { + .name = "hx8357", + .driver_data = (kernel_ulong_t)hx8357_lcd_init, + }, + { + .name = "hx8369", + .driver_data = (kernel_ulong_t)hx8369_lcd_init, + }, + {}, +}; +MODULE_DEVICE_TABLE(spi, hx8357_spi_ids); + static const struct of_device_id hx8357_dt_ids[] = { { .compatible = "himax,hx8357", @@ -672,6 +685,7 @@ static struct spi_driver hx8357_driver = { .name = "hx8357", .of_match_table = hx8357_dt_ids, }, + .id_table = hx8357_spi_ids, }; module_spi_driver(hx8357_driver);
Currently autoloading for SPI devices does not use the DT ID table, it uses SPI modalises. Supporting OF modalises is going to be difficult if not impractical, an attempt was made but has been reverted, so ensure that module autoloading works for this driver by adding a SPI device ID table. Fixes: 96c8395e2166 ("spi: Revert modalias changes") Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/video/backlight/hx8357.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)