Re: [SPF Softfail] Re: [PATCH v7 03/10] lib: add linear ranges helpers

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek