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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 15 Dec 2023 14:06:32 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 11/23/23 09:24, Matti Vaittinen wrote:
> > 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:  
> 
> ..snip
> 
> >>> 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?)
> >>>  
> 
> ... snip
> 
> >>
> >> 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.  
> 
> I had some spare time so drafted following:
> 
> +struct reg_val_table {
> +       int *reg_vals;
> +       int *vals;
> +       int num_vals;
> +};
> 
> ...
> 
> +/**
> + * regtable_find_val - find a value matching register setting
> + *
> + * Search given table for value mathcing a register setting.
> + *
> + * @table:     Table from which the register setting - value pairs are
> + *             searched.
> + * @reg:       Register value for which the matching physical value is
> + *             searched.
> + * @val:       Pointer to location where the found value will be stored.
> + *
> + * returns:    0 on success, negative errno if table is invalid or match is
> + *             not found.
> + */
> +int regtable_find_val(const struct reg_val_table *table, int reg, int *val)
> 
> 
> +/**
> + * regtable_find_reg - find a register setting matching given value.
> + *
> + * Search given table for a register setting matching a value.
> + *
> + * @table:     Table from which the register setting - value pairs are
> + *             searched.
> + * @val:       Value for which the matching register setting is searched.
> + * @reg:       Pointer to location where the found register value will be
> + *             stored.
> + *
> + * returns:    0 on success, negative errno if table is invalid or match is
> + *             not found.
> + */
> +int regtable_find_reg(const struct reg_val_table *table, int val, int *reg)
> 
> 
> +/**
> + * regtable_find_greater_than_val - find the closest greater val and reg
Maybe use rounding terminology rather than greater than?

regtable_find_val_roundup()?

> + *
> + * Search given table for the smallest value which is still greater than
> + * the given value. Both the found value and corresponding register
> + * setting are returned unless given pointers are NULL.
> + *
> + * @table:     Table from which the register setting - value pairs are
> + *             searched.
> + * @val_cmp:   Value to which the values stored in table are compared to.
> + * @reg:       NULL or pointer to location where the matching register
> + *             setting value will be stored.
> + * @val:       NULL or pointer to location where the found value will be
> + *             stored.
> + *
> + * returns:    0 on success, negative errno if table is invalid or match is
> + *             not found.
> + */
> +int regtable_find_greater_than_val(const struct reg_val_table *table, 
> int val_cmp,
> +                                  int *reg, int *val)
> 
> 

regtable_find_val_rounddown()?

> +/**
> + * regtable_find_smaller_than_val - find the closest smaller val and reg
> + *
> + * Search given table for the greatest value which is still smaller than
> + * the given value. Both the found value and corresponding register
> + * setting are returned unless given pointers are NULL.
> + *
> + * @table:     Table from which the register setting - value pairs are
> + *             searched.
> + * @val_cmp:   Value to which the values stored in table are compared to.
> + * @reg:       NULL or pointer to location where the matching register
> + *             setting value will be stored.
> + * @val:       NULL or pointer to location where the found value will be
> + *             stored.
> + *
> + * returns:    0 on success, negative errno if table is invalid or match is
> + *             not found.
> + */
> +int regtable_find_smaller_than_val(const struct reg_val_table *table,
> +                                  int val_cmp, int *reg, int *val)
> 
> 
> and
> 
> +struct regmap_regval_table {
> +       const struct reg_val_table table;
> +       int reg;
> +       int mask;
> +};
> 
> +/**
> + * regmap_table_value_set - update register to match 
> human-understandable value
> + * @map:       Register map
> + * @table:     Table describing register-value, human-readable value 
> relation
> + * value:      Human understandable value to configure in hardware.
> + *
> + * Return:     0 on success, negative errno on error.
> + */
> +int regmap_table_value_set(struct regmap *map,
> +                          const struct regmap_regval_table *table, int 
> value)
> 
> 
> +/**
> + * regmap_table_value_get - return human-understandable configuration
> + *
> + * Reads hardware or regmap cache for current hardware configuration and
> + * converts the read register value to human understandable entity.
> + * @map:       Register map
> + * @table:     Table describing register-value, human-readable value 
> relation
> + * value:      Human understandable value to configure in hardware.
> + *
> + * Return:     0 on success, negative errno on error.
> + */
> +int regmap_table_value_get(struct regmap *map,
> +                          const struct regmap_regval_table *table, int 
> *value)
> 
> 
> (for anyone interested, whole thing + tests can be found from:
> https://github.com/M-Vaittinen/linux/commits/regtable/
> Just last 3 commits.)
> 
> I am however having difficulties in seeing how this could be utilized by 
> IIO, which tends to rely on values being represented by two integers 
> (val and val2).

Two integers and a type to make it harder still... IIO_VAL_INT_PLUS_MICRO etc
though I guess that might not need representing as generally the caller
would know what that was.  Fixed point (ish) is a pain, but not come up with a better
presentation yet :(


> 
> Any suggestions regarding this idea? I'm wondering if I should just 
> scrap this and try seeing if I can make an IIO-specific helper(s) - or 
> if someone sees this would bring additional value worth an proper RFC? I 
> don't want to sen an RFC for people to properly review if this idea is 
> just plain stupid :)

It seems useful in general but I guess it's a question of whether you can find
enough users to justify it.

> 
> Yours,
> 	-- Matti
>