linux-kernel - Re: [PATCH 1/5] iio: imu: adis: Add custom ops struct

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]

Message-ID: <20241028165910.0000171c@Huawei.com>
Date: Mon, 28 Oct 2024 16:59:10 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Robert Budai <robert.budai@...log.com>
CC: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
	<Michael.Hennerich@...log.com>, Nuno Sa <nuno.sa@...log.com>, "Ramona
 Gradinariu" <ramona.gradinariu@...log.com>, Antoniu Miclaus
	<antoniu.miclaus@...log.com>, Jonathan Cameron <jic23@...nel.org>, "Rob
 Herring" <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, Jagath Jog J
	<jagathjog1996@...il.com>, <linux-iio@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <robi_budai@...oo.com>
Subject: Re: [PATCH 1/5] iio: imu: adis: Add custom ops struct

On Mon, 28 Oct 2024 14:25:33 +0200
Robert Budai <robert.budai@...log.com> wrote:

> From: Nuno Sá <nuno.sa@...log.com>
> 
> This patch introduces a custom ops struct letting users define
> custom read and write functions. Some adis devices might define
> a completely different spi protocol from the one used in the
> default implementation.

Also needs to mention the reset as that is nothing to do with
bus access.

> 
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Signed-off-by: Nuno Sá <nuno.sa@...log.com>

Minor comments inline

Jonathan

>  
> @@ -339,8 +339,11 @@ int __adis_reset(struct adis *adis)
>  	int ret;
>  	const struct adis_timeout *timeouts = adis->data->timeouts;
>  
> -	ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
> -				 ADIS_GLOB_CMD_SW_RESET);
> +	if (adis->ops->reset)

This one looks to be unrelated to the read / write path and
isn't mentioned in the patch description. Perhaps better to add
it in a separate patch where you can talk about why is it is needed. 

> +		ret = adis->ops->reset(adis);
> +	else
> +		ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
> +					 ADIS_GLOB_CMD_SW_RESET);
>  	if (ret) {
>  		dev_err(&adis->spi->dev, "Failed to reset device: %d\n", ret);
>  		return ret;

>  /**
>   * adis_init() - Initialize adis device structure
>   * @adis:	The adis device
> @@ -517,6 +525,9 @@ int adis_init(struct adis *adis, struct iio_dev *indio_dev,
>  
>  	adis->spi = spi;
>  	adis->data = data;
> +	if (!adis->ops->write || !adis->ops->read)
> +		adis->ops = &adis_default_ops;

If only write or read is specified, error out, don't replace with
the default ops as that clearly indicates a bug.


> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index e6a75356567a..7b589cc83380 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -94,6 +94,21 @@ struct adis_data {
>  	unsigned int burst_max_speed_hz;
>  };
>  
> +/**
> + * struct adis_ops: Custom ops for adis devices.
> + * @write: Custom spi write implementation.
> + * @read: Custom spi read implementation.
> + * @reset: Custom sw reset implementation. The custom implementation does not
> + *	   need to sleep after the reset. It's done by the library already.
> + */
> +struct adis_ops {
> +	int (*write)(struct adis *adis, unsigned int reg, unsigned int value,
> +		     unsigned int size);
> +	int (*read)(struct adis *adis, unsigned int reg, unsigned int *value,
> +		    unsigned int size);
> +	int (*reset)(struct adis *adis);
> +};
> +
>  /**
>   * struct adis - ADIS device instance data
>   * @spi: Reference to SPI device which owns this ADIS IIO device
> @@ -117,6 +132,7 @@ struct adis {
>  
>  	const struct adis_data	*data;
>  	unsigned int		burst_extra_len;
> +	const struct adis_ops	*ops;

Docs?  This structure has kernel-doc that needs updating to cover this new element.
Also, whilst you are here, please can you fix that doc in general (as 
precursor patch preferably).

At least one element in the docs doesn't seem to exist in the structure.

>  	/**
>  	 * The state_lock is meant to be used during operations that require
>  	 * a sequence of SPI R/W in order to protect the SPI transfer