Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Mehdi Djait <mehdi.djait.k@gmail.com>]

Hello Matti,

On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote:
> > > > +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.
> > 
> > I agree, this seems to be the better solution. I will look into this.
> > 
> 
> Thanks for going the extra mile :)

I am reconsidering the renaming of the fields 

1. inc{1,4,5,6} get assigned once to data->{ien_reg,inc_reg} in the probe 
function and then never used again

2. buf_cntl2 is used for enabling the buffer and changing the resolution
to 16bits, so which name is better than buf_cntl ? 

3. cntl seems the most appropriate name, again different functions and the same 
reg

4. who, id, xout_l, odcntl, buf_{clear, status, read} are quite straightforward

Anyway this is my opinion, what do you think ? 

--
Kind Regards,
Mehdi Djait