diff mbox series

backlight: hx8357: Add SPI device ID table

Message ID 20210922151014.49719-1-broonie@kernel.org (mailing list archive)
State Awaiting Upstream
Headers show
Series backlight: hx8357: Add SPI device ID table | expand

Commit Message

Mark Brown Sept. 22, 2021, 3:10 p.m. UTC
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(+)

Comments

Daniel Thompson Sept. 27, 2021, 9:42 a.m. UTC | #1
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
>
Mark Brown Sept. 27, 2021, 11:47 a.m. UTC | #2
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.
Daniel Thompson Sept. 27, 2021, 1:24 p.m. UTC | #3
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.
Mark Brown Sept. 27, 2021, 1:30 p.m. UTC | #4
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.
Daniel Thompson Oct. 1, 2021, 2:20 p.m. UTC | #5
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 mbox series

Patch

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);