Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Mehdi Djait]

Hello Jonathan,

On Sat, May 06, 2023 at 05:46:51PM +0100, Jonathan Cameron wrote:
> On Fri, 5 May 2023 20:11:33 +0200
> Mehdi Djait <mehdi.djait.k@xxxxxxxxx> wrote:
> 
> > Hello Jonathan,
> > 
> > On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:
> > > On Tue, 25 Apr 2023 00:22:27 +0200
> > > Mehdi Djait <mehdi.djait.k@xxxxxxxxx> wrote:
> > >   
> > > > Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> > > > ranges from ±2G to ±16G, digital output through I²C/SPI.
> > > > Add support for basic accelerometer features such as reading acceleration
> > > > via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
> > > > 
> > > > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@xxxxxxxxx>  
> > > 
> > > Two tiny things inline.  
> > >   
> > > > +static int kx132_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;
> > > > +	}
> > > > +
> > > > +	fifo_bytes = le16_to_cpu(buf_status);
> > > > +	fifo_bytes &= data->chip_info->buf_smp_lvl_mask;  
> > > 
> > > Slight preference for FIELD_GET() as it saves me checking the mask includes
> > > lowest bits.  
> > 
> > This will mean I have the remove the chip_info member buf_smp_lvl_mask
> > and use KX132_MASK_BUF_SMP_LVL directly because otherwise the
> > __builtin_constant_p function will cause an error when building. 
> > Check: https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h#L65
> > 
> > I can change it to FIELD_GET() if you want to.
> 
> Good point.  You could use le16_get_bits() though which I'm fairly sure will take
> a variable just fine.
> 

I don't think it will work. 

>From the commit log introducing the {type}_get_bits to <linux/bitfield.h>
"    Field specification must be a constant; __builtin_constant_p() doesn't
    have to be true for it, but compiler must be able to evaluate it at
    build time.  If it cannot or if the value does not encode any bitfield,
    the build will fail. "

Actually Geert Uytterhoeven tried to solve excatly this issue, but it
seems that the patch was not accepted. 
Check: https://lore.kernel.org/linux-iio/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@xxxxxxxxx/


So which solution would be the best:

1. Use directly KX132_MASK_BUF_SMP_LVL, the only reason I was trying to
avoid this was to make this function generic enough for other chip
variants

2. Copy the field_get() definition from drivers/clk/at91 or from the commit
of Geert and use it in this driver

3. leave it as it is ? 

4. another solution ?

> >
> > > 
> > >   
> > > > +
> > > > +	return fifo_bytes;
> > > > +}
> > > > +
> > > >  static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > > >  			       bool irq)
> > > >  {
> > > > @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
> > > >  };
> > > >  EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
> > > >  
> > > > +const struct kx022a_chip_info kx132_chip_info = {
> > > > +	.name		  = "kx132-1211",
> > > > +	.regmap_config	  = &kx132_regmap_config,
> > > > +	.channels	  = kx132_channels,
> > > > +	.num_channels	  = ARRAY_SIZE(kx132_channels),
> > > > +	.fifo_length	  = KX132_FIFO_LENGTH,
> > > > +	.who		  = KX132_REG_WHO,
> > > > +	.id		  = KX132_ID,
> > > > +	.cntl		  = KX132_REG_CNTL,
> > > > +	.cntl2		  = KX132_REG_CNTL2,
> > > > +	.odcntl		  = KX132_REG_ODCNTL,
> > > > +	.buf_cntl1	  = KX132_REG_BUF_CNTL1,
> > > > +	.buf_cntl2	  = KX132_REG_BUF_CNTL2,
> > > > +	.buf_clear	  = KX132_REG_BUF_CLEAR,
> > > > +	.buf_status1	  = KX132_REG_BUF_STATUS_1,
> > > > +	.buf_smp_lvl_mask = KX132_MASK_BUF_SMP_LVL,  
> > > 
> > > There are some things in here (typically where the define isn't used
> > > anywhere else) where I think it would be easier to follow if the
> > > value was listed here.  Masks and IDs for example. 
> > >   
> > 
> > After removing buf_smp_lvl_mask, which members will be easier to understand (besides id) ? 
> 
> I haven't gone through them.  Length seems an obvious one.  It's a number not a magic value.
> Register addresses might also be simpler if they aren't used elsewhere.
> 
> Not really about understanding just about a define that adds nothing if all we do is
> assign it to a variable of the same name.

Do you have a strong opinion about this ? 

I would really prefer to leave it like this, for the following reasons:

1. If I directly use values here, I have to do it also in the previous
patch where I introduce the chip_info for the kx022a -> this means I
will have defines in the h file which are not used at all -> the defines should
be deleted -> the patch will get unnecessarily bigger. I received
multiple comments about removing unnecessary changes and reducing of the
patch size when possible.

2. Consistency: having all the defines in one place really seems to be
better for understanding IMO. I find the mix of values and defines in 
the chip-info a bit confusing, e.g., I will use the direct value for 
KX132_REG_CNTL but not for KX132_REG_CNTL2 because KX132_REG_CNTL2 is
referenced in a regmap_range. 

--
Kind Regards
Mehdi Djait