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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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;
> +}

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek