diff mbox series

[1/5] iio: addac: ad74413r: add spi_device_id table

Message ID 20221111143921.742194-2-linux@rasmusvillemoes.dk
State New
Headers
Series iio: addac: ad74413r: various fixups |

Commit Message

Rasmus Villemoes Nov. 11, 2022, 2:39 p.m. UTC 
  Silence the run-time warning

  SPI driver ad74413r has no spi_device_id for adi,ad74412r

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Jonathan Cameron Nov. 12, 2022, 4:50 p.m. UTC | #1 
On Fri, 11 Nov 2022 15:39:17 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Silence the run-time warning
> 
>   SPI driver ad74413r has no spi_device_id for adi,ad74412r
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/iio/addac/ad74413r.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index 899bcd83f40b..37485be88a63 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -1457,12 +1457,20 @@ static const struct of_device_id ad74413r_dt_id[] = {
>  };
>  MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
>  
> +static const struct spi_device_id ad74413r_spi_id[] = {
> +	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
> +	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
> +	{},
Trivial, but prefer not to have a comma after a "NULL" terminator like this.
It would never make sense to add anything after it in the array.
Now you are matching existing driver style, but I'd still rather not see more
instances of this added.

Also, driver_data is not currently used. It should be because adding this
spi_id table means the driver can be probed via various routes where
device_get_match_data() == NULL. 

Hence, alongside this change you need to have a fallback to cover that case.
Something along the lines of...

	st->chip_info = device_get_match_data(..);
	if (!st->chip_info) {
		struct spi_device_id *id = spi_get_device_id();
		if (!id)
			return -EINVAL;

		st->chip_info = (void *)id->driver_data;
		//or better yet cast to the correct type I'm just too lazy to look it up ;)
		if (!st->chip_info)
			return -EINVAL;

	}
> +};
> +MODULE_DEVICE_TABLE(spi, ad74413r_spi_id);
> +
>  static struct spi_driver ad74413r_driver = {
>  	.driver = {
>  		   .name = "ad74413r",
>  		   .of_match_table = ad74413r_dt_id,
>  	},
>  	.probe = ad74413r_probe,
> +	.id_table = ad74413r_spi_id,
>  };
>  
>  module_driver(ad74413r_driver,
Rasmus Villemoes Nov. 14, 2022, 8:02 a.m. UTC | #2 
On 12/11/2022 17.50, Jonathan Cameron wrote:
> On Fri, 11 Nov 2022 15:39:17 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> Silence the run-time warning
>>
>>   SPI driver ad74413r has no spi_device_id for adi,ad74412r
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  drivers/iio/addac/ad74413r.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
>> index 899bcd83f40b..37485be88a63 100644
>> --- a/drivers/iio/addac/ad74413r.c
>> +++ b/drivers/iio/addac/ad74413r.c
>> @@ -1457,12 +1457,20 @@ static const struct of_device_id ad74413r_dt_id[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
>>  
>> +static const struct spi_device_id ad74413r_spi_id[] = {
>> +	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
>> +	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
>> +	{},
> Trivial, but prefer not to have a comma after a "NULL" terminator like this.
> It would never make sense to add anything after it in the array.

I agree and wouldn't have added it if it weren't for the existing case
in the other table.

> Now you are matching existing driver style, but I'd still rather not see more
> instances of this added.

Sure.

> Also, driver_data is not currently used. It should be because adding this
> spi_id table means the driver can be probed via various routes where
> device_get_match_data() == NULL. 

That makes sense, I think I thought that that would somehow happen
automatically. Looking through the history of similar fixes, I see that
for example 3f8dd0a7dc does indeed add code as you suggest, but
855fe49984 does not (and also doesn't add the corresponding .driver_data
initializers in the spi table). They may very well both be correct, but
looping in Oleksij for good measure.

> Hence, alongside this change you need to have a fallback to cover that case.
> Something along the lines of...
> 
> 	st->chip_info = device_get_match_data(..);
> 	if (!st->chip_info) {
> 		struct spi_device_id *id = spi_get_device_id();
> 		if (!id)
> 			return -EINVAL;
> 
> 		st->chip_info = (void *)id->driver_data;
> 		//or better yet cast to the correct type I'm just too lazy to look it up ;)
> 		if (!st->chip_info)
> 			return -EINVAL;
> 
> 	}

Thanks,
Rasmus
Jonathan Cameron Nov. 14, 2022, 7:39 p.m. UTC | #3 
On Mon, 14 Nov 2022 09:02:44 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 12/11/2022 17.50, Jonathan Cameron wrote:
> > On Fri, 11 Nov 2022 15:39:17 +0100
> > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >   
> >> Silence the run-time warning
> >>
> >>   SPI driver ad74413r has no spi_device_id for adi,ad74412r
> >>
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> ---
> >>  drivers/iio/addac/ad74413r.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> >> index 899bcd83f40b..37485be88a63 100644
> >> --- a/drivers/iio/addac/ad74413r.c
> >> +++ b/drivers/iio/addac/ad74413r.c
> >> @@ -1457,12 +1457,20 @@ static const struct of_device_id ad74413r_dt_id[] = {
> >>  };
> >>  MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
> >>  
> >> +static const struct spi_device_id ad74413r_spi_id[] = {
> >> +	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
> >> +	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
> >> +	{},  
> > Trivial, but prefer not to have a comma after a "NULL" terminator like this.
> > It would never make sense to add anything after it in the array.  
> 
> I agree and wouldn't have added it if it weren't for the existing case
> in the other table.
> 
> > Now you are matching existing driver style, but I'd still rather not see more
> > instances of this added.  
> 
> Sure.
> 
> > Also, driver_data is not currently used. It should be because adding this
> > spi_id table means the driver can be probed via various routes where
> > device_get_match_data() == NULL.   
> 
> That makes sense, I think I thought that that would somehow happen
> automatically. Looking through the history of similar fixes, I see that
> for example 3f8dd0a7dc does indeed add code as you suggest, but
> 855fe49984 does not (and also doesn't add the corresponding .driver_data
> initializers in the spi table). They may very well both be correct, but
> looping in Oleksij for good measure.

It depends a bit on whether there is any plausible chance of anyone making
use of say greybus with the device (I think that still relies on the spi id
though not checked recently).  If not and no board files exist, chances are
that all is needed is the id table (to make autoprobing of modules work).
Still I'd not leave the opening for the unexpected to happen given users
have an annoying habit of finding the corner cases :)

Jonathan

> 
> > Hence, alongside this change you need to have a fallback to cover that case.
> > Something along the lines of...
> > 
> > 	st->chip_info = device_get_match_data(..);
> > 	if (!st->chip_info) {
> > 		struct spi_device_id *id = spi_get_device_id();
> > 		if (!id)
> > 			return -EINVAL;
> > 
> > 		st->chip_info = (void *)id->driver_data;
> > 		//or better yet cast to the correct type I'm just too lazy to look it up ;)
> > 		if (!st->chip_info)
> > 			return -EINVAL;
> > 
> > 	}  
> 
> Thanks,
> Rasmus
>
diff mbox series

Patch

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 899bcd83f40b..37485be88a63 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -1457,12 +1457,20 @@  static const struct of_device_id ad74413r_dt_id[] = {
 };
 MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
 
+static const struct spi_device_id ad74413r_spi_id[] = {
+	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
+	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, ad74413r_spi_id);
+
 static struct spi_driver ad74413r_driver = {
 	.driver = {
 		   .name = "ad74413r",
 		   .of_match_table = ad74413r_dt_id,
 	},
 	.probe = ad74413r_probe,
+	.id_table = ad74413r_spi_id,
 };
 
 module_driver(ad74413r_driver,