diff mbox series

[v7,03/10] lib: add linear ranges helpers

Message ID c4cd52979ec187c942fa5794aab11e6c7f944cbb.1585656143.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive)
State New, archived
Headers show
Series Support ROHM BD99954 charger IC | expand

Commit Message

Vaittinen, Matti March 31, 2020, 12:23 p.m. UTC
Many devices have control registers which control some measurable
property. Often a register contains control field so that change in
this field causes linear change in the controlled property. It is not
a rare case that user wants to give 'meaningful' control values and
driver needs to convert them to register field values. Even more
often user wants to 'see' the currently set value - again in
meaningful units - and driver needs to convert the values it reads
from register to these meaningful units. Examples of this include:

- regulators, voltage/current configurations
- power, voltage/current configurations
- clk(?) NCOs

and maybe others I can't think of right now.

Provide a linear_range helper which can do conversion from user value
to register value 'selector'.

The idea here is stolen from regulator framework and patches refactoring
the regulator helpers to use this are following.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---

No changes since v6

 include/linux/linear_range.h |  48 +++++++
 lib/Kconfig                  |   3 +
 lib/Makefile                 |   1 +
 lib/linear_ranges.c          | 246 +++++++++++++++++++++++++++++++++++
 4 files changed, 298 insertions(+)
 create mode 100644 include/linux/linear_range.h
 create mode 100644 lib/linear_ranges.c

Comments

Andy Shevchenko March 31, 2020, 2:05 p.m. UTC | #1
On Tue, Mar 31, 2020 at 03:23:03PM +0300, Matti Vaittinen wrote:
> Many devices have control registers which control some measurable
> property. Often a register contains control field so that change in
> this field causes linear change in the controlled property. It is not
> a rare case that user wants to give 'meaningful' control values and
> driver needs to convert them to register field values. Even more
> often user wants to 'see' the currently set value - again in
> meaningful units - and driver needs to convert the values it reads
> from register to these meaningful units. Examples of this include:
> 
> - regulators, voltage/current configurations
> - power, voltage/current configurations
> - clk(?) NCOs
> 
> and maybe others I can't think of right now.
> 
> Provide a linear_range helper which can do conversion from user value
> to register value 'selector'.
> 
> The idea here is stolen from regulator framework and patches refactoring
> the regulator helpers to use this are following.

...

> +/*
> + * linear_ranges.c -- helpers to map values in a linear range to range index

File name inside file can bring an unnecessary churn in the future in case we
would like to rename it (by some reason). So, better to remove.

> + *
> + * Original idea borrowed from regulator framework
> + *

> + * It might be useful if we could support also inversely proportional ranges?

Looks like remark that should not be here, rather in commit message or even in cover letter.

> + * Copyright 2020 ROHM Semiconductors
> + */

...

> +/**
> + * linear_range_get_value - fetch a value from given range

> + *

This blank line is not needed. Can we drop them everywhere?

> + * @r:		pointer to linear range where value is looked from
> + * @selector:	selector for which the value is searched
> + * @val:	address where found value is updated
> + *
> + * Search given ranges for value which matches given selector.
> + *
> + * Return: 0 on success, -EINVAL given selector is not found from any of the
> + * ranges.
> + */

...

> +int linear_range_get_selector_low(const struct linear_range *r,
> +				  unsigned int val, unsigned int *selector,
> +				  bool *found)
> +{
> +	*found = false;
> +
> +	if (r->min > val)
> +		return -EINVAL;
> +

> +	if (linear_range_get_max_value(r) >= val)
> +		*found = true;

As far as I can see this is a bit different from _high counterpart. So, if we
even not found the range we still check r->step. Can we make them symmetrical
(to some extend)?

> +	if (!r->step)

Why not positive conditional?

> +		*selector = r->min_sel;
> +	else
> +		*selector = (val - r->min) / r->step + r->min_sel;

> +	return 0;
> +}

...

> +int linear_range_get_selector_low_array(const struct linear_range *r,
> +					int ranges, unsigned int val,
> +					unsigned int *selector, bool *found)
> +{
> +	int i;
> +	int ret = -EINVAL;
> +
> +	for (i = 0; i < ranges; i++) {
> +		int tmpret;
> +
> +		tmpret = linear_range_get_selector_low(&r[i], val, selector,
> +						       found);
> +
> +		if (!tmpret)
> +			ret = 0;
> +
> +		if (*found)
> +			break;
> +	}
> +
> +	return ret;
> +}

Can we refactor this?

	int i;
	int ret = -EINVAL;

	for (i = 0; i < ranges; i++) {
		ret = linear_range_get_selector_low(&r[i], val, selector, found);
		if (*found)
			break;
	}

	return break;

This will unshadow the error code returned by the loop body.

Or if ranges is guaranteed to be always positive number, convert this to do {} while.

...

> +int linear_range_get_selector_high(const struct linear_range *r,
> +				   unsigned int val, unsigned int *selector,
> +				   bool *found)
> +{
> +	*found = false;
> +
> +	if (linear_range_get_max_value(r) < val)
> +		return -EINVAL;
> +

> +	if (r->min <= val) {
> +		*found = true;
> +	} else {
> +		*selector = r->min_sel;
> +		return 0;
> +	}

	if (r->min > val) {
		*selector = r->min_sel;
		return 0;
	}

	...see below...

> +	if (!r->step)

Positive conditional?

> +		*selector = r->max_sel;
> +	else
> +		*selector = DIV_ROUND_UP(val - r->min, r->step) + r->min_sel;


	*found = true;

> +	return 0;
> +}
Vaittinen, Matti April 1, 2020, 10:09 a.m. UTC | #2
Hello Andy,

Thanks for the review again. I'll send v8 later this week :) 

On Tue, 2020-03-31 at 17:05 +0300, Andy Shevchenko wrote:
> On Tue, Mar 31, 2020 at 03:23:03PM +0300, Matti Vaittinen wrote:
> > Many devices have control registers which control some measurable
> > property. Often a register contains control field so that change in
> > this field causes linear change in the controlled property. It is
> > not
> > a rare case that user wants to give 'meaningful' control values and
> > driver needs to convert them to register field values. Even more
> > often user wants to 'see' the currently set value - again in
> > meaningful units - and driver needs to convert the values it reads
> > from register to these meaningful units. Examples of this include:
> > 
> > - regulators, voltage/current configurations
> > - power, voltage/current configurations
> > - clk(?) NCOs
> > 
> > and maybe others I can't think of right now.
> > 
> > Provide a linear_range helper which can do conversion from user
> > value
> > to register value 'selector'.
> > 
> > The idea here is stolen from regulator framework and patches
> > refactoring
> > the regulator helpers to use this are following.
> 
> ...
> 
> > +/*
> > + * linear_ranges.c -- helpers to map values in a linear range to
> > range index
> 
> File name inside file can bring an unnecessary churn in the future in
> case we
> would like to rename it (by some reason). So, better to remove.

Agree.

> > + *
> > + * Original idea borrowed from regulator framework
> > + *
> > + * It might be useful if we could support also inversely
> > proportional ranges?
> 
> Looks like remark that should not be here, rather in commit message
> or even in cover letter.

I think this is a good place so that anyone who opens this file will
see what could be done to improve this. No one is looking at the old
commit messages unless they face some problems. And cover letters fade
away even faster - although the cover letter and commit messages may
catch some attention during the patch submissions.

So maybe you would agree with me if I added this also in cover letter
and commit message :)

> 
> > + * Copyright 2020 ROHM Semiconductors
> > + */
> 
> ...
> 
> > +/**
> > + * linear_range_get_value - fetch a value from given range
> > + *
> 
> This blank line is not needed. Can we drop them everywhere?

Yep. We can. I see that is recommended way in Documentation/doc-
guide/kernel-doc.rst. (Although for my eye it looks clearer with the
empty line)

> 
> > + * @r:		pointer to linear range where value is looked
> > from
> > + * @selector:	selector for which the value is searched
> > + * @val:	address where found value is updated
> > + *
> > + * Search given ranges for value which matches given selector.
> > + *
> > + * Return: 0 on success, -EINVAL given selector is not found from
> > any of the
> > + * ranges.
> > + */
> 
> ...
> 
> > +int linear_range_get_selector_low(const struct linear_range *r,
> > +				  unsigned int val, unsigned int
> > *selector,
> > +				  bool *found)
> > +{
> > +	*found = false;
> > +
> > +	if (r->min > val)
> > +		return -EINVAL;
> > +
> > +	if (linear_range_get_max_value(r) >= val)
> > +		*found = true;
> 
> As far as I can see this is a bit different from _high counterpart. 

Yes. Because the logic is different. _low accepts selector for a
closest value which is smaller or equal to given. _high accepts
selector for a closest value which is higher or equal to given. This
mean that both the _high and _low accept also input value which is not
in the range - _low accepts input which is higher than range, _high
accepts input which is lower than the range.

Both functions bail out right away if input is not "acceptable". 

_low if:
+	if (r->min > val)
> > +		return -EINVAL;

_high if:
+	if (linear_range_get_max_value(r) < val)
> > +		return -EINVAL;
> > 

After this check, if the input is within the other end of range, then
we can set found to true.

for _low:
> > +	if (linear_range_get_max_value(r) >= val)
> > +		*found = true;

for _high:
> > +	if (r->min <= val) {
> > +		*found = true;
> > 

And I agree with your later comment - inversing this check for makes
the function cleaner.

> So, if we
> even not found the range we still check r->step. Can we make them
> symmetrical
> (to some extend)?

Yes we can. For _low we can do:

if (linear_range_get_max_value(r) < val) {
	*selector = r->max_sel;
	return 0;
}

if (!r->step)
	*selector = r->max_sel;
else
	*selector = DIV_ROUND_UP(val - r->min, r->step) + r - min_sel;
*found = true;
 
return 0;
 


> 
> > +	if (!r->step)
> 
> Why not positive conditional?

because we really want to test if step is zero. I can write it as
if (r->step == 0) if that is preferred. But we definitely want to chek
if r->step is zero and handle it as exception case. (We want to protect
from division by zero - and in that case all selectors are Ok as range
is constant)


> > +		*selector = r->min_sel;
> > +	else
> > +		*selector = (val - r->min) / r->step + r->min_sel;
> > +	return 0;
> > +}
> 
> ...
> 
> > +int linear_range_get_selector_low_array(const struct linear_range
> > *r,
> > +					int ranges, unsigned int val,
> > +					unsigned int *selector, bool
> > *found)
> > +{
> > +	int i;
> > +	int ret = -EINVAL;
> > +
> > +	for (i = 0; i < ranges; i++) {
> > +		int tmpret;
> > +
> > +		tmpret = linear_range_get_selector_low(&r[i], val,
> > selector,
> > +						       found);
> > +
> > +		if (!tmpret)
> > +			ret = 0;
> > +
> > +		if (*found)
> > +			break;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> Can we refactor this?
> 
> 	int i;
> 	int ret = -EINVAL;
> 
> 	for (i = 0; i < ranges; i++) {
> 		ret = linear_range_get_selector_low(&r[i], val,
> selector, found);
> 		if (*found)
> 			break;
> 	}
> 
> 	return break;
> 
> This will unshadow the error code returned by the loop body.

What is the return break; doing here? I don't understand the syntax.
And we want the logic to be such that we return zero from this function
if _any_ of the calls to linear_range_get_selector_low returned zero.
(Even if further calls would return non-zero value). But we wan't to
keep looking for better (in-range) match if *found is not true.


> Or if ranges is guaranteed to be always positive number, convert this
> to do {} while.

I don't see the benefit :/

> 
> ...
> 
> > +int linear_range_get_selector_high(const struct linear_range *r,
> > +				   unsigned int val, unsigned int
> > *selector,
> > +				   bool *found)
> > +{
> > +	*found = false;
> > +
> > +	if (linear_range_get_max_value(r) < val)
> > +		return -EINVAL;
> > +
> > +	if (r->min <= val) {
> > +		*found = true;
> > +	} else {
> > +		*selector = r->min_sel;
> > +		return 0;
> > +	}
> 
> 	if (r->min > val) {
> 		*selector = r->min_sel;
> 		return 0;
> 	}

I agree. Inversing this condition makes it cleaner.


Best Regards
	Matti
Andy Shevchenko April 1, 2020, 12:39 p.m. UTC | #3
On Wed, Apr 1, 2020 at 1:09 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
> On Tue, 2020-03-31 at 17:05 +0300, Andy Shevchenko wrote:
> > On Tue, Mar 31, 2020 at 03:23:03PM +0300, Matti Vaittinen wrote:

...

> > > + * It might be useful if we could support also inversely
> > > proportional ranges?
> >
> > Looks like remark that should not be here, rather in commit message
> > or even in cover letter.
>
> I think this is a good place so that anyone who opens this file will
> see what could be done to improve this. No one is looking at the old
> commit messages unless they face some problems. And cover letters fade
> away even faster - although the cover letter and commit messages may
> catch some attention during the patch submissions.
>
> So maybe you would agree with me if I added this also in cover letter
> and commit message :)

OK

...

> > > +int linear_range_get_selector_low(const struct linear_range *r,
> > > +                             unsigned int val, unsigned int
> > > *selector,
> > > +                             bool *found)

> > As far as I can see this is a bit different from _high counterpart.
>
> Yes. Because the logic is different. _low accepts selector for a
> closest value which is smaller or equal to given. _high accepts
> selector for a closest value which is higher or equal to given. This
> mean that both the _high and _low accept also input value which is not
> in the range - _low accepts input which is higher than range, _high
> accepts input which is lower than the range.

Thanks for elaboration.

> > > +   if (!r->step)
> >
> > Why not positive conditional?
>
> because we really want to test if step is zero.

> I can write it as
> if (r->step == 0) if that is preferred.

Yes, please. Then we will see that 0 has special meaning here.

>  But we definitely want to chek
> if r->step is zero and handle it as exception case. (We want to protect
> from division by zero - and in that case all selectors are Ok as range
> is constant)
>
>
> > > +           *selector = r->min_sel;
> > > +   else
> > > +           *selector = (val - r->min) / r->step + r->min_sel;
> > > +   return 0;
> > > +}

...

> > > +int linear_range_get_selector_low_array(const struct linear_range
> > > *r,
> > > +                                   int ranges, unsigned int val,
> > > +                                   unsigned int *selector, bool
> > > *found)
> > > +{
> > > +   int i;
> > > +   int ret = -EINVAL;
> > > +
> > > +   for (i = 0; i < ranges; i++) {
> > > +           int tmpret;
> > > +
> > > +           tmpret = linear_range_get_selector_low(&r[i], val,
> > > selector,
> > > +                                                  found);
> > > +
> > > +           if (!tmpret)
> > > +                   ret = 0;
> > > +
> > > +           if (*found)
> > > +                   break;
> > > +   }
> > > +
> > > +   return ret;
> > > +}
> >
> > Can we refactor this?
> >
> >       int i;
> >       int ret = -EINVAL;
> >
> >       for (i = 0; i < ranges; i++) {
> >               ret = linear_range_get_selector_low(&r[i], val,
> > selector, found);
> >               if (*found)
> >                       break;
> >       }
> >
> >       return break;
> >
> > This will unshadow the error code returned by the loop body.
>
> What is the return break; doing here? I don't understand the syntax.

Typo. return ret; should be.

> And we want the logic to be such that we return zero from this function
> if _any_ of the calls to linear_range_get_selector_low returned zero.
> (Even if further calls would return non-zero value). But we wan't to
> keep looking for better (in-range) match if *found is not true.

won't or want to? I'm confused with the last.
Is this logic described in the description? Sorry, I forgot, although
I had read it.
If no, perhaps it needs to be added either to description or as a
comment to the loop.

> > Or if ranges is guaranteed to be always positive number, convert this
> > to do {} while.
>
> I don't see the benefit :/

I see.
 do {
   ret = ...
 } while (++i < ranges && *found == false);

much better to read.
Vaittinen, Matti April 1, 2020, 1:18 p.m. UTC | #4
On Wed, 2020-04-01 at 15:39 +0300, Andy Shevchenko wrote:
> On Wed, Apr 1, 2020 at 1:09 PM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > On Tue, 2020-03-31 at 17:05 +0300, Andy Shevchenko wrote:
> > > On Tue, Mar 31, 2020 at 03:23:03PM +0300, Matti Vaittinen wrote:
> > > > +int linear_range_get_selector_low_array(const struct
> > > > linear_range
> > > > *r,
> > > > +                                   int ranges, unsigned int
> > > > val,
> > > > +                                   unsigned int *selector,
> > > > bool
> > > > *found)
> > > > +{
> > > > +   int i;
> > > > +   int ret = -EINVAL;
> > > > +
> > > > +   for (i = 0; i < ranges; i++) {
> > > > +           int tmpret;
> > > > +
> > > > +           tmpret = linear_range_get_selector_low(&r[i], val,
> > > > selector,
> > > > +                                                  found);
> > > > +
> > > > +           if (!tmpret)
> > > > +                   ret = 0;
> > > > +
> > > > +           if (*found)
> > > > +                   break;
> > > > +   }
> > > > +
> > > > +   return ret;
> > > > +}
> > > 
> > > Can we refactor this?
> > > 
> > >       int i;
> > >       int ret = -EINVAL;
> > > 
> > >       for (i = 0; i < ranges; i++) {
> > >               ret = linear_range_get_selector_low(&r[i], val,
> > > selector, found);
> > >               if (*found)
> > >                       break;
> > >       }
> > > 
> > >       return break;
> > > 
> > > This will unshadow the error code returned by the loop body.
> > 
> > What is the return break; doing here? I don't understand the
> > syntax.
> 
> Typo. return ret; should be.
> 
> > And we want the logic to be such that we return zero from this
> > function
> > if _any_ of the calls to linear_range_get_selector_low returned
> > zero.
> > (Even if further calls would return non-zero value). But we wan't
> > to
> > keep looking for better (in-range) match if *found is not true.
> 
> won't or want to? I'm confused with the last.

want :) Typo here too.

> Is this logic described in the description? Sorry, I forgot, although
> I had read it.

 * Scan array of ranges for selector which which range value matches
given
 * input value. Value is matching if it is equal or smaller than
given
 * value. If given value is found to be in a range scannins is
stopped and
 * @found is set true. If a range with values smaller than
given value is found
 * but the range max is being smaller than given
value, then the ranges
 * biggest selector is updated to @selector but
scanning ranges is continued
 * and @found is set to false.

I think it is but I am open to all suggestions how to improve doc!

> > > Or if ranges is guaranteed to be always positive number, convert
> > > this
> > > to do {} while.
> > 
> > I don't see the benefit :/
> 
> I see.
>  do {
>    ret = ...
>  } while (++i < ranges && *found == false);
> 
> much better to read.

Huh?
Compared to:
for (i = 0; i < ranges; i++) {
	ret = ...
}

I wouldn't say so.

As I explained, we need to have "temporary" return value in any case
because we need to return 0 if any of the calls to
linear_range_get_selector_low() returned 0. Return value 0 from
linear_range_get_selector_low() means we found "matching" value (lower
than input) and selector was updated (although input value was not in
range).


Br,
	Matti Vaittinen
Andy Shevchenko April 1, 2020, 2:42 p.m. UTC | #5
On Wed, Apr 1, 2020 at 4:18 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
> On Wed, 2020-04-01 at 15:39 +0300, Andy Shevchenko wrote:
> > On Wed, Apr 1, 2020 at 1:09 PM Vaittinen, Matti
> > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > On Tue, 2020-03-31 at 17:05 +0300, Andy Shevchenko wrote:
> > > > On Tue, Mar 31, 2020 at 03:23:03PM +0300, Matti Vaittinen wrote:
> > > > > +int linear_range_get_selector_low_array(const struct
> > > > > linear_range
> > > > > *r,
> > > > > +                                   int ranges, unsigned int
> > > > > val,
> > > > > +                                   unsigned int *selector,
> > > > > bool
> > > > > *found)
> > > > > +{
> > > > > +   int i;
> > > > > +   int ret = -EINVAL;
> > > > > +
> > > > > +   for (i = 0; i < ranges; i++) {
> > > > > +           int tmpret;
> > > > > +
> > > > > +           tmpret = linear_range_get_selector_low(&r[i], val,
> > > > > selector,
> > > > > +                                                  found);
> > > > > +
> > > > > +           if (!tmpret)
> > > > > +                   ret = 0;
> > > > > +
> > > > > +           if (*found)
> > > > > +                   break;
> > > > > +   }
> > > > > +
> > > > > +   return ret;
> > > > > +}

Looked again at the code of the callee.
So, *found becomes true if and only if the return is 0 (or other way
around if you prefer).
Now I'm wondering why you need 'found' at all?

It means above may be as simple as

      int i;
      int ret = -EINVAL;

      for (i = 0; i < ranges; i++) {
               ret = linear_range_get_selector_low(&r[i], val, selector, found);
               if (*found)
                       break;
      }

      return ret;

or assuming 'found' will gone

  int i
  int ret = -EINVAL;

  for (i = 0; i < ranges && ret; i++) {
      ret = linear_range_get_selector_low(&r[i], val, selector);
  }
  return ret;

...

>  * value. If given value is found to be in a range scannins is

> I think it is but I am open to all suggestions how to improve doc!

Thanks. At least fix a typo: scannins -> scannings

...

> Compared to:
> for (i = 0; i < ranges; i++) {
>         ret = ...
> }
>
> I wouldn't say so.
>
> As I explained, we need to have "temporary" return value in any case
> because we need to return 0 if any of the calls to
> linear_range_get_selector_low() returned 0. Return value 0 from
> linear_range_get_selector_low() means we found "matching" value (lower
> than input) and selector was updated (although input value was not in
> range).

See above.

--
With Best Regards,
Andy Shevchenko
Vaittinen, Matti April 2, 2020, 4:03 a.m. UTC | #6
Hello Andy,

On Wed, 2020-04-01 at 17:42 +0300, Andy Shevchenko wrote:
> On Wed, Apr 1, 2020 at 4:18 PM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > On Wed, 2020-04-01 at 15:39 +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 1, 2020 at 1:09 PM Vaittinen, Matti
> > > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > > On Tue, 2020-03-31 at 17:05 +0300, Andy Shevchenko wrote:
> > > > > On Tue, Mar 31, 2020 at 03:23:03PM +0300, Matti Vaittinen
> > > > > wrote:
> > > > > > +int linear_range_get_selector_low_array(const struct
> > > > > > linear_range
> > > > > > *r,
> > > > > > +                                   int ranges, unsigned
> > > > > > int
> > > > > > val,
> > > > > > +                                   unsigned int *selector,
> > > > > > bool
> > > > > > *found)
> > > > > > +{
> > > > > > +   int i;
> > > > > > +   int ret = -EINVAL;
> > > > > > +
> > > > > > +   for (i = 0; i < ranges; i++) {
> > > > > > +           int tmpret;
> > > > > > +
> > > > > > +           tmpret = linear_range_get_selector_low(&r[i],
> > > > > > val,
> > > > > > selector,
> > > > > > +                                                  found);
> > > > > > +
> > > > > > +           if (!tmpret)
> > > > > > +                   ret = 0;
> > > > > > +
> > > > > > +           if (*found)
> > > > > > +                   break;
> > > > > > +   }
> > > > > > +
> > > > > > +   return ret;
> > > > > > +}
> 
> Looked again at the code of the callee.
> So, *found becomes true if and only if the return is 0

Yes. If found is true, then ret is 0.

>  (or other way
> around if you prefer).

No. Not other way around. Ret can be 0 and found false. This is what we
need to handle. Ret means we found range value smaller than given
input. Found means also the given input was in range.

> Now I'm wondering why you need 'found' at all?

To separate case where given input was in range.

> It means above may be as simple as
> 
>       int i;
>       int ret = -EINVAL;
> 
>       for (i = 0; i < ranges; i++) {
>                ret = linear_range_get_selector_low(&r[i], val,
> selector, found);
>                if (*found)
>                        break;
>       }

No. It can't. Here we get wrong return value if one of the calls to
linear_range_get_selector_low() return zero with found being false and
subsequent calls return non zero ret. Then we actually found lower-
than-given-input value from one of the ranges and updated the selector
(that would be usable for example for regulator voltage control) but
end up returning non zero ret as the subsequent calls to
linear_range_get_selector_low() do overwrite the ret.

OTOH, we should not stop for first range having zero ret if found is
false because some of the ranges may have range where given input is
in-range (and this is considered better value).

> 
>       return ret;
> 
> or assuming 'found' will gone

No. We should not drop the 'found'.

> 
>   int i
>   int ret = -EINVAL;
> 
>   for (i = 0; i < ranges && ret; i++) {
>       ret = linear_range_get_selector_low(&r[i], val, selector);
>   }
>   return ret;
> 
> ...
> 
> >  * value. If given value is found to be in a range scannins is
> > I think it is but I am open to all suggestions how to improve doc!
> 
> Thanks. At least fix a typo: scannins -> scannings
> 

Thanks :) I'll fix it

> ...
> 
> > Compared to:
> > for (i = 0; i < ranges; i++) {
> >         ret = ...
> > }
> > 
> > I wouldn't say so.
> > 
> > As I explained, we need to have "temporary" return value in any
> > case
> > because we need to return 0 if any of the calls to
> > linear_range_get_selector_low() returned 0. Return value 0 from
> > linear_range_get_selector_low() means we found "matching" value
> > (lower
> > than input) and selector was updated (although input value was not
> > in
> > range).
> 
> See above.


Best Regards
	--Matti
diff mbox series

Patch

diff --git a/include/linux/linear_range.h b/include/linux/linear_range.h
new file mode 100644
index 000000000000..17b5943727d5
--- /dev/null
+++ b/include/linux/linear_range.h
@@ -0,0 +1,48 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2020 ROHM Semiconductors */
+
+#ifndef LINEAR_RANGE_H
+#define LINEAR_RANGE_H
+
+#include <linux/types.h>
+
+/**
+ * struct linear_range - table of selector - value pairs
+ *
+ * Define a lookup-table for range of values. Intended to help when looking
+ * for a register value matching certaing physical measure (like voltage).
+ * Usable when increment of one in register always results a constant increment
+ * of the physical measure (like voltage).
+ *
+ * @min:  Lowest value in range
+ * @min_sel: Lowest selector for range
+ * @max_sel: Highest selector for range
+ * @step: Value step size
+ */
+struct linear_range {
+	unsigned int min;
+	unsigned int min_sel;
+	unsigned int max_sel;
+	unsigned int step;
+};
+
+unsigned int linear_range_values_in_range(const struct linear_range *r);
+unsigned int linear_range_values_in_range_array(const struct linear_range *r,
+						int ranges);
+unsigned int linear_range_get_max_value(const struct linear_range *r);
+
+int linear_range_get_value(const struct linear_range *r, unsigned int selector,
+			   unsigned int *val);
+int linear_range_get_value_array(const struct linear_range *r, int ranges,
+				 unsigned int selector, unsigned int *val);
+int linear_range_get_selector_low(const struct linear_range *r,
+				  unsigned int val, unsigned int *selector,
+				  bool *found);
+int linear_range_get_selector_high(const struct linear_range *r,
+				   unsigned int val, unsigned int *selector,
+				   bool *found);
+int linear_range_get_selector_low_array(const struct linear_range *r,
+					int ranges, unsigned int val,
+					unsigned int *selector, bool *found);
+
+#endif
diff --git a/lib/Kconfig b/lib/Kconfig
index bc7e56370129..411ab2d2d800 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -19,6 +19,9 @@  config RAID6_PQ_BENCHMARK
 	  Benchmark all available RAID6 PQ functions on init and choose the
 	  fastest one.
 
+config LINEAR_RANGES
+	tristate
+
 config PACKING
 	bool "Generic bitfield packing and unpacking"
 	default n
diff --git a/lib/Makefile b/lib/Makefile
index 611872c06926..18c3d313872e 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -122,6 +122,7 @@  obj-$(CONFIG_DEBUG_LIST) += list_debug.o
 obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
 
 obj-$(CONFIG_BITREVERSE) += bitrev.o
+obj-$(CONFIG_LINEAR_RANGES) += linear_ranges.o
 obj-$(CONFIG_PACKING)	+= packing.o
 obj-$(CONFIG_CRC_CCITT)	+= crc-ccitt.o
 obj-$(CONFIG_CRC16)	+= crc16.o
diff --git a/lib/linear_ranges.c b/lib/linear_ranges.c
new file mode 100644
index 000000000000..52246325f9a9
--- /dev/null
+++ b/lib/linear_ranges.c
@@ -0,0 +1,246 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * linear_ranges.c -- helpers to map values in a linear range to range index
+ *
+ * Original idea borrowed from regulator framework
+ *
+ * It might be useful if we could support also inversely proportional ranges?
+ * Copyright 2020 ROHM Semiconductors
+ */
+
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/linear_range.h>
+
+/**
+ * linear_range_values_in_range - return the amount of values in a range
+ *
+ * @r:		pointer to linear range where values are counted
+ *
+ * Compute the amount of values in range pointed by @r. Note, values can
+ * be all equal - range with selectors 0,...,2 with step 0 still contains
+ * 3 values even though they are all equal.
+ *
+ * Return: the amount of values in range pointed by @r
+ */
+unsigned int linear_range_values_in_range(const struct linear_range *r)
+{
+	if (!r)
+		return 0;
+	return r->max_sel - r->min_sel + 1;
+}
+EXPORT_SYMBOL_GPL(linear_range_values_in_range);
+
+/**
+ * linear_range_values_in_range_array - return the amount of values in ranges
+ *
+ * @r:		pointer to array of linear ranges where values are counted
+ * @ranges:	amount of ranges we include in computation.
+ *
+ * Compute the amount of values in ranges pointed by @r. Note, values can
+ * be all equal - range with selectors 0,...,2 with step 0 still contains
+ * 3 values even though they are all equal.
+ *
+ * Return: the amount of values in first @ranges ranges pointed by @r
+ */
+unsigned int linear_range_values_in_range_array(const struct linear_range *r,
+						int ranges)
+{
+	int i, values_in_range = 0;
+
+	for (i = 0; i < ranges; i++) {
+		int values;
+
+		values = linear_range_values_in_range(&r[i]);
+		if (!values)
+			return values;
+
+		values_in_range += values;
+	}
+	return values_in_range;
+}
+EXPORT_SYMBOL_GPL(linear_range_values_in_range_array);
+
+/**
+ * linear_range_get_max_value - return the largest value in a range
+ *
+ * @r:		pointer to linear range where value is looked from
+ *
+ * Return: the largest value in the given range
+ */
+unsigned int linear_range_get_max_value(const struct linear_range *r)
+{
+	return r->min + (r->max_sel - r->min_sel) * r->step;
+}
+EXPORT_SYMBOL_GPL(linear_range_get_max_value);
+
+/**
+ * linear_range_get_value - fetch a value from given range
+ *
+ * @r:		pointer to linear range where value is looked from
+ * @selector:	selector for which the value is searched
+ * @val:	address where found value is updated
+ *
+ * Search given ranges for value which matches given selector.
+ *
+ * Return: 0 on success, -EINVAL given selector is not found from any of the
+ * ranges.
+ */
+int linear_range_get_value(const struct linear_range *r, unsigned int selector,
+			   unsigned int *val)
+{
+	if (r->min_sel > selector || r->max_sel < selector)
+		return -EINVAL;
+
+	*val = r->min + (selector - r->min_sel) * r->step;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(linear_range_get_value);
+
+/**
+ * linear_range_get_value_array - fetch a value from array of ranges
+ *
+ * @r:		pointer to array of linear ranges where value is looked from
+ * @ranges:	amount of ranges in an array
+ * @selector:	selector for which the value is searched
+ * @val:	address where found value is updated
+ *
+ * Search through an array of ranges for value which matches given selector.
+ *
+ * Return: 0 on success, -EINVAL given selector is not found from any of the
+ * ranges.
+ */
+int linear_range_get_value_array(const struct linear_range *r, int ranges,
+				 unsigned int selector, unsigned int *val)
+{
+	int i;
+
+	for (i = 0; i < ranges; i++)
+		if (r[i].min_sel <= selector && r[i].max_sel >= selector)
+			return linear_range_get_value(&r[i], selector, val);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(linear_range_get_value_array);
+
+/**
+ * linear_range_get_selector_low - return linear range selector for value
+ *
+ * @r:		pointer to linear range where selector is looked from
+ * @val:	value for which the selector is searched
+ * @selector:	address where found selector value is updated
+ * @found:	flag to indicate that given value was in the range
+ *
+ * Return selector which which range value is closest match for given
+ * input value. Value is matching if it is equal or smaller than given
+ * value. If given value is in the range, then @found is set true.
+ *
+ * Return: 0 on success, -EINVAL if range is invalid or does not contain
+ * value smaller or equal to given value
+ */
+int linear_range_get_selector_low(const struct linear_range *r,
+				  unsigned int val, unsigned int *selector,
+				  bool *found)
+{
+	*found = false;
+
+	if (r->min > val)
+		return -EINVAL;
+
+	if (linear_range_get_max_value(r) >= val)
+		*found = true;
+
+	if (!r->step)
+		*selector = r->min_sel;
+	else
+		*selector = (val - r->min) / r->step + r->min_sel;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(linear_range_get_selector_low);
+
+/**
+ * linear_range_get_selector_low_array - return linear range selector for value
+ *
+ * @r:		pointer to array of linear ranges where selector is looked from
+ * @ranges:	amount of ranges to scan from array
+ * @val:	value for which the selector is searched
+ * @selector:	address where found selector value is updated
+ * @found:	flag to indicate that given value was in the range
+ *
+ * Scan array of ranges for selector which which range value matches given
+ * input value. Value is matching if it is equal or smaller than given
+ * value. If given value is found to be in a range scannins is stopped and
+ * @found is set true. If a range with values smaller than given value is found
+ * but the range max is being smaller than given value, then the ranges
+ * biggest selector is updated to @selector but scanning ranges is continued
+ * and @found is set to false.
+ *
+ * Return: 0 on success, -EINVAL if range array is invalid or does not contain
+ * range with a value smaller or equal to given value
+ */
+int linear_range_get_selector_low_array(const struct linear_range *r,
+					int ranges, unsigned int val,
+					unsigned int *selector, bool *found)
+{
+	int i;
+	int ret = -EINVAL;
+
+	for (i = 0; i < ranges; i++) {
+		int tmpret;
+
+		tmpret = linear_range_get_selector_low(&r[i], val, selector,
+						       found);
+
+		if (!tmpret)
+			ret = 0;
+
+		if (*found)
+			break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(linear_range_get_selector_low_array);
+
+/**
+ * linear_range_get_selector_high - return linear range selector for value
+ *
+ * @r:		pointer to linear range where selector is looked from
+ * @val:	value for which the selector is searched
+ * @selector:	address where found selector value is updated
+ * @found:	flag to indicate that given value was in the range
+ *
+ * Return selector which which range value is closest match for given
+ * input value. Value is matching if it is equal or higher than given
+ * value. If given value is in the range, then @found is set true.
+ *
+ * Return: 0 on success, -EINVAL if range is invalid or does not contain
+ * value greater or equal to given value
+ */
+int linear_range_get_selector_high(const struct linear_range *r,
+				   unsigned int val, unsigned int *selector,
+				   bool *found)
+{
+	*found = false;
+
+	if (linear_range_get_max_value(r) < val)
+		return -EINVAL;
+
+	if (r->min <= val) {
+		*found = true;
+	} else {
+		*selector = r->min_sel;
+		return 0;
+	}
+
+	if (!r->step)
+		*selector = r->max_sel;
+	else
+		*selector = DIV_ROUND_UP(val - r->min, r->step) + r->min_sel;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(linear_range_get_selector_high);