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

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 5/7/23 23:45, Mehdi Djait wrote:
> 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

I think that for the clarity and re-usability we would have fields for 
functions. We could for example have separate fields for buffer enable 
and resolution even though it means assigning the same register to both. 
This, however, is somewhat wasteful (memory vise).

Problem with buf_cntl1 and buf_cntl2 is that the 'cntl' is too broad to 
really tell what the control is. Also, what is the difference of 
buf_cntl1 and buf_cntl2?

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

I agree. These look pretty clear to me, although 'status' is also 
somewhat ambiguous. (Is it sample level? Is it some corruption 
detection? Can the buffer be 'busy'?).

> Anyway this is my opinion, what do you think ?

I can currently live with both of these approaches. We may need to 
review this if/when adding support to other sensor(s) though. I will 
leave the decision to you - just make the code best you can ;)

Yours,
	-- Matti

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

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