Re: [PATCH v5 1/3] lib: add linear range get selector within

[Vaittinen, Matti]

On Fri, 2021-05-28 at 16:12 +0800, Gene Chen wrote:
> From: Gene Chen <gene_chen@xxxxxxxxxxx>
> 
> Add linear range get selector within for choose closest selector
> between minimum and maximum selector.
> 
> Signed-off-by: Gene Chen <gene_chen@xxxxxxxxxxx>
> ---
>  include/linux/linear_range.h |  2 ++
>  lib/linear_ranges.c          | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/include/linux/linear_range.h
> b/include/linux/linear_range.h
> index 17b5943727d5..fd3d0b358f22 100644
> --- a/include/linux/linear_range.h
> +++ b/include/linux/linear_range.h
> @@ -41,6 +41,8 @@ int linear_range_get_selector_low(const struct
> linear_range *r,
>  int linear_range_get_selector_high(const struct linear_range *r,
>  				   unsigned int val, unsigned int
> *selector,
>  				   bool *found);
> +void linear_range_get_selector_within(const struct linear_range *r,
> +				      unsigned int val, unsigned int
> *selector);
>  int linear_range_get_selector_low_array(const struct linear_range
> *r,
>  					int ranges, unsigned int val,
>  					unsigned int *selector, bool
> *found);
> diff --git a/lib/linear_ranges.c b/lib/linear_ranges.c
> index ced5c15d3f04..a1a7dfa881de 100644
> --- a/lib/linear_ranges.c
> +++ b/lib/linear_ranges.c
> @@ -241,5 +241,36 @@ int linear_range_get_selector_high(const struct
> linear_range *r,
>  }
>  EXPORT_SYMBOL_GPL(linear_range_get_selector_high);
>  
> +/**
> + * linear_range_get_selector_within - 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
> + *
> + * Return selector for which range value is closest match for given
> + * input value. Value is matching if it is equal or lower than given
> + * value. But return maximum selector if given value is higher than
> + * maximum value.
> + */
> +void linear_range_get_selector_within(const struct linear_range *r,
> +				      unsigned int val, unsigned int
> *selector)

I like the naming! The "within" sounds good to me :)
It slightly bothers my "style eye" to see this not returning a value
(void) - but still returning the value via parameter (selector). It may
be slightly more complicated to read when used.

Please consider for first time reading code like:
unsigned int do_something(const struct linear_range *r, unsigned int
val, int *myvalue)
{
	int ret;

	...

	linear_range_get_selector_within(r, val, myvalue);

	...

	return ret;
}

to reading code like:
unsigned int do_something(const struct linear_range *r, unsigned int
val, int *myvalue)
{
	int ret;

	...

	*myvalue = linear_range_get_selector_within(r, val);

	...

	return ret;
}

For me reading the latter shows better where the myvalue is really set.

OTOH, Your suggestion is consistent with the other functions so I am
not asking for a change if this is Ok with others :) Thanks a lot!

Reviewed-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>

> +{
> +	if (r->min > val) {
> +		*selector = r->min_sel;
> +		return;
> +	}
> +
> +	if (linear_range_get_max_value(r) < val) {
> +		*selector = r->max_sel;
> +		return;
> +	}
> +
> +	if (r->step == 0)
> +		*selector = r->min_sel;
> +	else
> +		*selector = (val - r->min) / r->step + r->min_sel;
> +}
> +EXPORT_SYMBOL_GPL(linear_range_get_selector_within);
> +
>  MODULE_DESCRIPTION("linear-ranges helper");
>  MODULE_LICENSE("GPL");