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;
Message-ID: <867ac7b4-b666-854f-69f7-2d7d7d92c94e@gmail.com>
Date:   Tue, 25 Apr 2023 09:50:11 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Content-Language: en-US, en-GB
To:     Mehdi Djait <mehdi.djait.k@gmail.com>, jic23@kernel.org
Cc:     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
References: <cover.1682373451.git.mehdi.djait.k@gmail.com>
 <bf0269aff66483f2323914170707203749b33f0f.1682373451.git.mehdi.djait.k@gmail.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add
 chip_info structure
In-Reply-To: <bf0269aff66483f2323914170707203749b33f0f.1682373451.git.mehdi.djait.k@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/25/23 01:22, Mehdi Djait 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>
> ---
> v3:
> - added the change of the buffer's allocation in the __kx022a_fifo_flush
>    to this patch
> - added the chip_info to the struct kx022a_data
> 
> v2:
> - mentioned the introduction of the i2c_device_id table in the commit
> - get i2c_/spi_get_device_id only when device get match fails
> - removed the generic KX_define
> - removed the kx022a_device_type enum
> - added comments for the chip_info struct elements
> - fixed errors pointed out by the kernel test robot
> 
>   drivers/iio/accel/kionix-kx022a-i2c.c |  15 +++-
>   drivers/iio/accel/kionix-kx022a-spi.c |  15 +++-
>   drivers/iio/accel/kionix-kx022a.c     | 114 +++++++++++++++++---------
>   drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++-
>   4 files changed, 147 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> index 8f23631a1fd3..ce299d0446f7 100644
> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -15,6 +15,7 @@

...


>   static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> @@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>   {
>   	struct kx022a_data *data = iio_priv(idev);
>   	struct device *dev = regmap_get_device(data->regmap);
> -	__le16 buffer[KX022A_FIFO_LENGTH * 3];
> +	__le16 *buffer;
>   	uint64_t sample_period;
>   	int count, fifo_bytes;
>   	bool renable = false;
>   	int64_t tstamp;
>   	int ret, i;
>   
> +	buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;

Do you think we could get rid of allocating and freeing the buffer for 
each flush? I feel it is a bit wasteful, and with high sampling 
frequencies this function can be called quite often. Do you think there 
would be a way to either use stack (always reserve big enough buffer no 
matter which chip we have - or is the buffer too big to be safely taken 
from the stack?), or a buffer stored in private data and allocated at 
probe or buffer enable?

Also, please avoid such long lines. I know many people don't care about 
the line length - but for example I tend to have 3 terminal windows open 
side-by-side on my laptop screen. Hence long lines tend to be harder to 
read for me.

> +
>   	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
>   	if (ret) {
>   		dev_err(dev, "Error reading buffer status\n");
> @@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>   		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
>   
>   	count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
> -	if (!count)
> +	if (!count) {
> +		kfree(buffer);
>   		return 0;
> +	}
>   
>   	/*
>   	 * If we are being called from IRQ handler we know the stored timestamp
> @@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>   	}
>   
>   	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
> -	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
> +	ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
>   				&buffer[0], fifo_bytes);
>   	if (ret)
>   		goto renable_out;
> @@ -704,6 +710,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>   	if (renable)
>   		enable_irq(data->irq);
>   
> +	kfree(buffer);
>   	return ret;
>   }
> 
...

>   
> -int kx022a_probe_internal(struct device *dev)
> +const struct kx022a_chip_info kx022a_chip_info = {
> +	.name		  = "kx022-accel",
> +	.regmap_config	  = &kx022a_regmap_config,
> +	.channels	  = kx022a_channels,
> +	.num_channels	  = ARRAY_SIZE(kx022a_channels),
> +	.fifo_length	  = KX022A_FIFO_LENGTH,
> +	.who		  = KX022A_REG_WHO,
> +	.id		  = KX022A_ID,
> +	.cntl		  = KX022A_REG_CNTL,
> +	.cntl2		  = KX022A_REG_CNTL2,
> +	.odcntl		  = KX022A_REG_ODCNTL,
> +	.buf_cntl1	  = KX022A_REG_BUF_CNTL1,
> +	.buf_cntl2	  = KX022A_REG_BUF_CNTL2,
> +	.buf_clear	  = KX022A_REG_BUF_CLEAR,
> +	.buf_status1	  = KX022A_REG_BUF_STATUS_1,
> +	.buf_read	  = KX022A_REG_BUF_READ,
> +	.inc1		  = KX022A_REG_INC1,
> +	.inc4		  = KX022A_REG_INC4,
> +	.inc5		  = KX022A_REG_INC5,
> +	.inc6		  = KX022A_REG_INC6,
> +	.xout_l		  = KX022A_REG_XOUT_L,
> +};
> +EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);

Do you think the fields (or at least some of them) in this struct could 
be named based on the (main) functionality being used, not based on the 
register name? Something like "watermark_reg", "buf_en_reg", 
"reset_reg", "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", 
"int2_pinconf_reg", "int1_src_reg" ...

I would not be at all surprized to see for example some IRQ control to 
be shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be 
moved to cntl<Y> or to buf_cntl<Y> for next sensor we want to support. 
Especially when new cool feature is added to next sensor, resulting also 
adding a new cntl<Z> or buf_cntl<Z> or INC<Z>.

I, however, believe the _functionality_ will be there (in some register) 
- at least for the ICs for which we can re-use this driver. Hence, it 
might be nice - and if you can think of better names for these fields - 
to rename them based on the _functionality_ we use.

Another benefit would be added clarity to the code. Writing a value to 
"buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to 
me) than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". 
Especially if you don't have a datasheet at your hands.

I am not "demanding" this (at least not for now :]) because it seems 
these two Kionix sensors have been pretty consistent what comes to 
maintaining the same functionality in the registers with same naming - 
but I believe this is something that may in any case be lurking around 
the corner.


All in all, looks nice and clean to me! Good job.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~