Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure

From: Mehdi Djait <mehdi.djait.k@gmail.com>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: jic23@kernel.org, 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 2/3] iio: accel: kionix-kx022a: Add chip_info structure
Date: Tue, 21 Mar 2023 16:56:43 +0100	[thread overview]
Message-ID: <ZBnTuykAqse5vBhO@carbian> (raw)
In-Reply-To: <4c28925d-c07c-61b7-8863-9c00e6846687@gmail.com>

Hello Matti,

> > +static int kx022a_get_fifo_bytes(struct kx022a_data *data)
> > +{
> > +	struct device *dev = regmap_get_device(data->regmap);
> > +	__le16 buf_status;
> > +	int ret, fifo_bytes;
> > +
> > +	ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
> > +	if (ret) {
> > +		dev_err(dev, "Error reading buffer status\n");
> > +		return ret;
> > +	}
> > +
> > +	buf_status &= data->chip_info->buf_smp_lvl_mask;
> > +	fifo_bytes = le16_to_cpu(buf_status);
> > +
> > +	/*
> > +	 * The KX022A has FIFO which can store 43 samples of HiRes data from 2
> > +	 * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
> > +	 * 258 bytes of sample data. The quirk to know is that the amount of bytes in
> > +	 * the FIFO is advertised via 8 bit register (max value 255). The thing to note
> > +	 * is that full 258 bytes of data is indicated using the max value 255.
> > +	 */
> > +	if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
> > +		fifo_bytes = KX022A_FIFO_MAX_BYTES;
> > +
> > +	if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
> > +		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
> > +
> > +	return fifo_bytes;
> > +}
> 
> I like adding this function. Here I agree with Jonathan - having a device
> specific functions would clarify this a bit. The KX022A "quirk" is a bit
> confusing. You could then get rid of the buf_smp_lvl_mask.

my bad here, I should have made a separate patch and explained more ...
buf_smp_lvl_mask is essential because kionix products use different
number of bits to report "the number of data bytes that have been stored in the 
sample buffer" using the registers BUF_STATUS_1 and BUF_STATUS_2

kx022a: 8bits
kx132: 10bits
kx12x: 11bits
kx126: 12bits

I think this function is quite generic and can be used for different
kionix devices: 

- It reads BUF_STATUS_1 and BUF_STATUS_2 and then uses a chip specific
mask 
- It takes care of the quirk of kx022a which is just a simple if statement 

> 
> > +
> >   static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> >   {
> >   	/*
> > @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> >   	 */
> >   	data->timestamp = 0;
> > -	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
> > +	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[data->chip_info->fifo_length * 3];
> 
> I don't like this. Having the length of an array decided at run-time is not
> something I appreciate. Maybe you could just always reserve the memory so
> that the largest FIFO gets supported. I am just wondering how large arrays
> we can safely allocate from the stack?

I was stupid enough to ignore the warnings... 
I will take care of it in the v2

> 
> 
> > @@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
> >   		goto unlock_out;
> >   	/* Enable buffer */
> > -	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
> > -			      KX022A_MASK_BUF_EN);
> > +	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
> > +			      KX_MASK_BUF_EN);
> >   	if (ret)
> >   		goto unlock_out;
> > -	data->state |= KX022A_STATE_FIFO;
> > +	data->state |= KX_STATE_FIFO;
> >   	ret = regmap_set_bits(data->regmap, data->ien_reg,
> > -			      KX022A_MASK_WMI);
> > +			      KX_MASK_WMI);
> 
> I think this fits to one line now. (even on my screen)
> 
> >   	if (ret)
> >   		goto unlock_out;
> 
> > -int kx022a_probe_internal(struct device *dev)
> > +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
> 
> As mentioned elsewhere, this might also work if the chip-type enum was
> passed here as parameter. That way the bus specific part would not need to
> know about the struct chip_info...
> 
> >   {
> >   	static const char * const regulator_names[] = {"io-vdd", "vdd"};
> >   	struct iio_trigger *indio_trig;
> > @@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev)
> >   		return -ENOMEM;
> >   	data = iio_priv(idev);
> > +	data->chip_info = chip_info;
> 
> ...Here you could then pick the correct chip_info based on the chip-type
> enum. In that case I'd like to get the regmap_config(s) in own file. Not
> sure how that would look like though.
> 
> All in all, I like how this looks like. Nice job!

Thank you for the feedback :)

--
Kind Regards 
Mehdi Djait