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@xxxxxxx>
---
...
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! ~~
