Re: [PATCH v4 2/2] iio: light: isl76682: Add ISL76682 driver

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 11/23/23 02:26, Marek Vasut wrote:
> On 11/22/23 13:17, Matti Vaittinen wrote:
>> On 11/21/23 05:10, Marek Vasut wrote:
>>> The ISL76682 is very basic ALS which only supports ALS or IR mode
>>> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any
>>> other fancy functionality.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---

...

>>
>> I like this table-based look-up for write (and read) of scales. 
>> Looking at this I see an analogy to some of the regulator stuff, like 
>> for example the ramp-up values. What I do very much like in the 
>> regulator subsystem is the drivers/regulator/helpers.c
>>
>> I wonder if similar approach would be usable in IIO as well? I mean, 
>> providing readily written iio_regmap_read/write_raw_<functionality>() 
>> and iio_available_*() helpers for the simple devices where we just 
>> have value-register mapping? I mean, driver would just populate 
>> something like:
>>
>> struct iio_scale_desc {
>>      int *scale_val_table;
>>      int *scale_val2_table;
>>      int num_scales;
> 
> You'd also need type here (fractional, int+micro, ...), right ?

Well, my thinking was to go with baby-steps. Eg, start by supporting 
just int+micro - but yes. As I wrote below, this can be expanded by 
allowing specifying the type.

>>      int scale_reg_addr;
>>      int scale_reg_mask;
>> };
>>
>> and call helper like
>> int iio_regmap_read_raw_scale(struct iio_dev *idev,
>>                    struct iio_scale_desc *sd, int *val,
>>                    int *val2)"
>> provided by IIO framework.
>>
>> Similar helper for writing new scales and getting available scales.
>>
>> Later this could be expanded by allowing specifying the type of 
>> provided values (in the example case, IIO_VAL_INT_PLUS_x - but maybe 
>> this would be extensible (and worth) to support also the other options?)
>>
>> I know it's a bit much to be done in the context of this series. Hence 
>> I am definitely not insisting this to be done here! OTOH, the embedded 
>> Linux is not in EU next year so maybe Marek would forgive me before we 
>> meet next time :pondering:
> 
> toffee-- forgive++ , hehehe , no worries.

At least I know what I need to pack in the suitcase! ;) Oh, by the way, 
the wafers were great!

>> Anyways - does this sound like a sensible thing to do? I guess it 
>> could help simplifying some drivers a little.
> 
> The only thing I would wonder about is, should such a thing go into 
> regmap so it can be reused cross-subsystem instead of making this iio 
> specific ?

I definitely think a relation "register value" <=> "item from a table" 
is very much used also outside the IIO. So yes, a generic regmap helper 
for doing write as a "look value from table and write corresponding 
value to a register" and "read value from register and return me a 
corresponding item from a table" would be very usable.

There is a tradeoff when doing a generic one instead of making it 
targeted for IIO use. Supporting different types of data is likely to 
make the code a bit hairy. Also, the IIO way of having these IIO_VAL_* 
flags does probably require IIO - specific wrappers in any case.

>> Oh. Only after writing of this I noticed the range is written in HW 
>> only together with the 'start' command. I guess this is how the IC 
>> operates - 
> 
> The IC is just simple, a few bits in command register to control it and 
> nothing fancy, so it is just easier to write the 8 bits at a time 
> instead of doing RMW .
> 
>> you need to write all configs together with starting the measurement? 
>> Or is that just an optimization to avoid extra writes? If it's the 
>> first, then a suggested iio_regmap_*() -helper wouldn't work here. I 
>> might've added a comment explaining why range is written in 
>> isl76682_get() and not here to the isl76682_get().
> 
> It is just easier and cheaper to write it all at once instead of RMW .
> It also isn't like RMW would win anything here, rather the opposite.

Yep. Kind of makes sense. It's just me who is already used to see the 
regmap writes straight in the scale setting function - and RMW provided 
by regmap is really pretty simple when regmap lock is used. But this is 
very much Ok for me just like you wrote it.

Yours,
	-- Matti

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

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