On Fri, 15 Dec 2023 14:06:32 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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
>
