Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 28 May 2023 17:55:50 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Mehdi Djait <mehdi.djait.k@gmail.com>
Cc:     mazziesaccount@gmail.com, krzysztof.kozlowski+dt@linaro.org,
        andriy.shevchenko@linux.intel.com, robh+dt@kernel.org,
        lars@metafoo.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v4 5/7] iio: accel: kionix-kx022a: Refactor driver and
 add chip_info structure
Message-ID: <20230528175550.4e58f90f@jic23-huawei>
In-Reply-To: <de588a5a3ca311f6dc3a543bfa5cea7b590ae44c.1685109507.git.mehdi.djait.k@gmail.com>
References: <cover.1685109507.git.mehdi.djait.k@gmail.com>
        <de588a5a3ca311f6dc3a543bfa5cea7b590ae44c.1685109507.git.mehdi.djait.k@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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