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:   Mon, 24 Apr 2023 00:05:09 +0200
From:   Mehdi Djait <mehdi.djait.k@gmail.com>
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     mazziesaccount@gmail.com, krzysztof.kozlowski+dt@linaro.org,
        andriy.shevchenko@linux.intel.com, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v2 4/5] iio: accel: kionix-kx022a: Add a function to
 retrieve number of bytes in buffer
Message-ID: <ZEWrleoBekBYkhYy@carbian>
References: <cover.1682019544.git.mehdi.djait.k@gmail.com>
 <2c5d71e37fc7e000091189b3c5d66ede1a6015d2.1682019544.git.mehdi.djait.k@gmail.com>
 <20230422184653.7ae28d5a@jic23-huawei>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230422184653.7ae28d5a@jic23-huawei>
Precedence: bulk

Hello everyone,

On Sat, Apr 22, 2023 at 06:46:53PM +0100, Jonathan Cameron wrote:
> On Thu, 20 Apr 2023 22:22:04 +0200
> Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> 
> > Since Kionix accelerometers use various numbers of bits to report data, a
> > device-specific function is required.
> > Move the driver's private data to the header file to support the new function.
> > Make the allocation of the "buffer" array in the fifo_flush function dynamic
> > and more generic.
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> 
> This results in some fifo_length changes in here and some in the previous patch.
> Either keep it fixed for first patch, then make those changes and the ones you have
> here in a single patch, or if that's hard maybe a single patch is cleaner.
> 
> I'd only expect the stuff about bytes in buffer to be in this patch.
> 
> Jonathan

I will come up with a better solution in the v3

> 
> 
> 
> > ---
> > v2:
> > - separated this change from the chip_info introduction and made it a patch in v2 
> > - changed the function from generic implementation for to device-specific one
> > - removed blank lines pointed out by checkpatch
> > - changed the allocation of the "buffer" array in __kx022a_fifo_flush
> > 
> >  drivers/iio/accel/kionix-kx022a.c | 72 +++++++++++++------------------
> >  drivers/iio/accel/kionix-kx022a.h | 37 ++++++++++++++++
> >  2 files changed, 66 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> > index 7f9a2c29790b..1c81ea1657b9 100644
> > --- a/drivers/iio/accel/kionix-kx022a.c
> > +++ b/drivers/iio/accel/kionix-kx022a.c
> > @@ -150,36 +150,6 @@ static const struct regmap_config kx022a_regmap_config = {
> >  	.cache_type = REGCACHE_RBTREE,
> >  };
> >  
> > -struct kx022a_data {
> > -	struct regmap *regmap;
> > -	struct iio_trigger *trig;
> > -	struct device *dev;
> > -	struct iio_mount_matrix orientation;
> > -	int64_t timestamp, old_timestamp;
> > -
> > -	int irq;
> > -	int inc_reg;
> > -	int ien_reg;
> > -
> > -	unsigned int state;
> > -	unsigned int odr_ns;
> > -
> > -	bool trigger_enabled;
> > -	/*
> > -	 * Prevent toggling the sensor stby/active state (PC1 bit) in the
> > -	 * middle of a configuration, or when the fifo is enabled. Also,
> > -	 * protect the data stored/retrieved from this structure from
> > -	 * concurrent accesses.
> > -	 */
> > -	struct mutex mutex;
> > -	u8 watermark;
> > -
> > -	/* 3 x 16bit accel data + timestamp */
> > -	__le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
> > -	struct {
> > -		__le16 channels[3];
> > -		s64 ts __aligned(8);
> > -	} scan;
> >  };
> >  
> >  static const struct iio_mount_matrix *
> > @@ -340,7 +310,6 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
> >  		dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
> >  
> >  	return ret;
> > -
> 
> Grumpy maintainer hat on:  I don't want to see white space changes in unrelated code
> in a patch doing anything other than white space cleanup.

I will make a separate patch.

> 
> >  }
> >  
> >  static int kx022a_turn_off_lock(struct kx022a_data *data)
> > @@ -595,34 +564,50 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> >  	return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
> >  }
> > 
> 
> 
> >  static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> >  			       bool irq)
> >  {
> >  	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;
> >  
> > -	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> > -	if (ret) {
> > -		dev_err(dev, "Error reading buffer status\n");
> > -		return ret;
> > -	}
> > +	/* 3 axis, 2 bytes of data for each of the axis */
> > +	buffer = kmalloc(data->chip_info->fifo_length * 6, GFP_KERNEL);
> 
> Split that 6 up into sizeof(*buffer) * 3 
> Then just comment on 3 axis.  Or use a define for number of axis and drop the
> comment entirely.
> 
> Also, this feels like it fitst better in previous patch.

I will add this change to the previous patch

> 
> 
> > +	if (!buffer)
> > +		return -ENOMEM;
> >  
> > -	/* Let's not overflow if we for some reason get bogus value from i2c */
> > -	if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
> > -		fifo_bytes = KX022A_FIFO_MAX_BYTES;
> > +	fifo_bytes = data->chip_info->get_fifo_bytes(data);
> 
> Only this small part is what this patch claims to do.. (+ the callback of course).
> 
> 
> >  
> >  	if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
> >  		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
> > @@ -704,6 +689,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> >  	if (renable)
> >  		enable_irq(data->irq);
> >  
> > +	kfree(buffer);
> >  	return ret;
> >  }
> >  
> > @@ -1016,6 +1002,7 @@ const struct kx022a_chip_info kx022a_chip_info = {
> >  	.inc5		  = KX022A_REG_INC5,
> >  	.inc6		  = KX022A_REG_INC6,
> >  	.xout_l		  = KX022A_REG_XOUT_L,
> > +	.get_fifo_bytes	  = kx022a_get_fifo_bytes,
> >  };
> >  EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
> >  
> > @@ -1143,7 +1130,6 @@ int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chi
> >  	if (ret)
> >  		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
> >  
> > -
> 
> Another one.  If those predate your series, feel free to clean them up either
> at the start or end of this series.

I will

--
Kind Regards
Mehdi Djait