linux-kernel - Re: [PATCH 1/5] iio: addac: ad74413r: add spi_device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 12 Nov 2022 16:50:49 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Cosmin Tanislav <cosmin.tanislav@...log.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] iio: addac: ad74413r: add spi_device_id table

On Fri, 11 Nov 2022 15:39:17 +0100
Rasmus Villemoes <linux@...musvillemoes.dk> wrote:

> Silence the run-time warning
> 
>   SPI driver ad74413r has no spi_device_id for adi,ad74412r
> 
> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.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,