Re: [PATCH v4 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 26 May 2023 16:30:46 +0200
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:

> Add the chip_info structure to the driver's private data to hold all
> the device specific infos.
> Refactor the kx022a driver implementation to make it more generic and
> extensible.
> 
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>

Hi Mehdi,

Just one trivial comment from me about missing docs / unused element.

Thanks,

Jonathan

...

> diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> index 12424649d438..c23b6f03409e 100644
> --- a/drivers/iio/accel/kionix-kx022a.h
> +++ b/drivers/iio/accel/kionix-kx022a.h
> @@ -76,7 +76,56 @@
>  
>  struct device;
>  
> -int kx022a_probe_internal(struct device *dev);
> -extern const struct regmap_config kx022a_regmap;
> +/**
> + * struct kx022a_chip_info - Kionix accelerometer chip specific information
> + *
> + * @name:		name of the device
> + * @regmap_config:	pointer to register map configuration
> + * @channels:		pointer to iio_chan_spec array
> + * @num_channels:	number of iio_chan_spec channels
> + * @fifo_length:	number of 16-bit samples in a full buffer
> + * @who:		WHO_AM_I register
> + * @id:			WHO_AM_I register value
> + * @cntl:		control register 1
> + * @cntl2:		control register 2
> + * @odcntl:		output data control register
> + * @buf_cntl1:		buffer control register 1
> + * @buf_cntl2:		buffer control register 2
> + * @buf_clear:		buffer clear register
> + * @buf_status1:	buffer status register 1

Missing entry for buf_smp_lvl_mask.

Mind you it's also not used in this patch and only seems to be used in the
get_fifo_bytes() function in patch 7, so might not be needed at all?
If it is, then introduce it in patch 7 where we can see how it is used.

> + * @buf_read:		buffer read register
> + * @inc1:		interrupt control register 1
> + * @inc4:		interrupt control register 4
> + * @inc5:		interrupt control register 5
> + * @inc6:		interrupt control register 6
> + * @xout_l:		x-axis output least significant byte
> + */
> +struct kx022a_chip_info {
> +	const char *name;
> +	const struct regmap_config *regmap_config;
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
> +	unsigned int fifo_length;
> +	u8 who;
> +	u8 id;
> +	u8 cntl;
> +	u8 cntl2;
> +	u8 odcntl;
> +	u8 buf_cntl1;
> +	u8 buf_cntl2;
> +	u8 buf_clear;
> +	u8 buf_status1;
> +	u16 buf_smp_lvl_mask;
> +	u8 buf_read;
> +	u8 inc1;
> +	u8 inc4;
> +	u8 inc5;
> +	u8 inc6;
> +	u8 xout_l;
> +};
> +
> +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
> +
> +extern const struct kx022a_chip_info kx022a_chip_info;
>  
>  #endif