Re: [PATCH v1 1/2] spi: deduplicate spi_match_id() in __spi_register_driver()

Re: [PATCH v1 1/2] spi: deduplicate spi_match_id() in __spi_register_driver()

[Jon Hunter]

Hi Andy,

On 19/11/2021 17:37, Andy Shevchenko wrote:
> The same logic is used in spi_match_id() and in the __spi_register_driver().
> By switching the former from taking struct spi_device * to const char * as
> the second parameter we may deduplicate the code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/spi/spi.c | 17 ++++++-----------
>   1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index b23e675953e1..dfa06103150e 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -315,11 +315,10 @@ static void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
>    * and the sysfs version makes coldplug work too.
>    */
>   
> -static const struct spi_device_id *spi_match_id(const struct spi_device_id *id,
> -						const struct spi_device *sdev)
> +static const struct spi_device_id *spi_match_id(const struct spi_device_id *id, const char *name)
>   {
>   	while (id->name[0]) {
> -		if (!strcmp(sdev->modalias, id->name))
> +		if (!strcmp(name, id->name))
>   			return id;
>   		id++;
>   	}
> @@ -330,7 +329,7 @@ const struct spi_device_id *spi_get_device_id(const struct spi_device *sdev)
>   {
>   	const struct spi_driver *sdrv = to_spi_driver(sdev->dev.driver);
>   
> -	return spi_match_id(sdrv->id_table, sdev);
> +	return spi_match_id(sdrv->id_table, sdev->modalias);
>   }
>   EXPORT_SYMBOL_GPL(spi_get_device_id);
>   
> @@ -352,7 +351,7 @@ static int spi_match_device(struct device *dev, struct device_driver *drv)
>   		return 1;
>   
>   	if (sdrv->id_table)
> -		return !!spi_match_id(sdrv->id_table, spi);
> +		return !!spi_match_id(sdrv->id_table, spi->modalias);
>   
>   	return strcmp(spi->modalias, drv->name) == 0;
>   }
> @@ -474,12 +473,8 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv)
>   			if (sdrv->id_table) {
>   				const struct spi_device_id *spi_id;
>   
> -				for (spi_id = sdrv->id_table; spi_id->name[0];
> -				     spi_id++)
> -					if (strcmp(spi_id->name, of_name) == 0)
> -						break;
> -
> -				if (spi_id->name[0])
> +				spi_id = spi_match_id(sdrv->id_table, of_name);
> +				if (!spi_id)
>   					continue;
>   			} else {
>   				if (strcmp(sdrv->driver.name, of_name) == 0)
> 


Following this change I am seeing the following warnings again although 
most of these have now been fixed ...

  WARNING KERN SPI driver mtd_dataflash has no spi_device_id for atmel,at45
  WARNING KERN SPI driver mtd_dataflash has no spi_device_id for 
atmel,dataflash
  WARNING KERN SPI driver spi-nor has no spi_device_id for jedec,spi-nor
  WARNING KERN SPI driver mmc_spi has no spi_device_id for mmc-spi-slot
  WARNING KERN SPI driver cros-ec-spi has no spi_device_id for 
google,cros-ec-spi

I have not looked any further yet, but this appears to cause the SPI ID 
match to fail.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v1 1/2] spi: deduplicate spi_match_id() in __spi_register_driver()

[Andy Shevchenko]

On Tue, Nov 23, 2021 at 03:22:38PM +0000, Jon Hunter wrote:
> On 19/11/2021 17:37, Andy Shevchenko wrote:

> > @@ -474,12 +473,8 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv)
> >   			if (sdrv->id_table) {
> >   				const struct spi_device_id *spi_id;
> > -				for (spi_id = sdrv->id_table; spi_id->name[0];
> > -				     spi_id++)
> > -					if (strcmp(spi_id->name, of_name) == 0)
> > -						break;

> > -				if (spi_id->name[0])
> > +				spi_id = spi_match_id(sdrv->id_table, of_name);
> > +				if (!spi_id)

Seems I have inverted condition here. Shouldn't it be

				if (spi_id)

instead?

> >   					continue;
> >   			} else {
> >   				if (strcmp(sdrv->driver.name, of_name) == 0)
> > 
> 
> 
> Following this change I am seeing the following warnings again although most
> of these have now been fixed ...
> 
>  WARNING KERN SPI driver mtd_dataflash has no spi_device_id for atmel,at45
>  WARNING KERN SPI driver mtd_dataflash has no spi_device_id for
> atmel,dataflash
>  WARNING KERN SPI driver spi-nor has no spi_device_id for jedec,spi-nor
>  WARNING KERN SPI driver mmc_spi has no spi_device_id for mmc-spi-slot
>  WARNING KERN SPI driver cros-ec-spi has no spi_device_id for
> google,cros-ec-spi
> 
> I have not looked any further yet, but this appears to cause the SPI ID
> match to fail.

Ah, thanks for testing!
Can you try above?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] spi: deduplicate spi_match_id() in __spi_register_driver()

[Andy Shevchenko]

On Tue, Nov 23, 2021 at 03:22:38PM +0000, Jon Hunter wrote:
> On 19/11/2021 17:37, Andy Shevchenko wrote:

> Following this change I am seeing the following warnings again although most
> of these have now been fixed ...
> 
>  WARNING KERN SPI driver mtd_dataflash has no spi_device_id for atmel,at45
>  WARNING KERN SPI driver mtd_dataflash has no spi_device_id for
> atmel,dataflash
>  WARNING KERN SPI driver spi-nor has no spi_device_id for jedec,spi-nor
>  WARNING KERN SPI driver mmc_spi has no spi_device_id for mmc-spi-slot
>  WARNING KERN SPI driver cros-ec-spi has no spi_device_id for
> google,cros-ec-spi

> I have not looked any further yet, but this appears to cause the SPI ID
> match to fail.

Looking into the code it should be harmless warning. I.o.w. it shouldn't
prevent driver registration. In any case I'm about to send a fix, thanks
for the report!

-- 
With Best Regards,
Andy Shevchenko