Re: [PATCH 11/15] regulator: rohm-regulator: SNVS dvs and linear voltage support

[Lee Jones <lee.jones@linaro.org>]

On Fri, 08 Jan 2021, Matti Vaittinen wrote:

> The helper for obtaining HW-state based DVS voltage levels currently only
> works for regulators using linear-ranges. Extend support to regulators with
> simple linear mappings and add also proper error path if pickable-ranges
> regulators call this.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  drivers/regulator/rohm-regulator.c | 23 +++++++++++++++++++++--
>  include/linux/mfd/rohm-generic.h   |  6 +++++-
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/rohm-regulator.c b/drivers/regulator/rohm-regulator.c
> index 399002383b28..9248bd63afa9 100644
> --- a/drivers/regulator/rohm-regulator.c
> +++ b/drivers/regulator/rohm-regulator.c
> @@ -22,13 +22,26 @@ static int set_dvs_level(const struct regulator_desc *desc,
>  			return ret;
>  		return 0;
>  	}
> -
> +	/* If voltage is set to 0 => disable */
>  	if (uv == 0) {
>  		if (omask)
>  			return regmap_update_bits(regmap, oreg, omask, 0);
>  	}
> +	/* Some setups don't allow setting own voltage but do allow enabling */
> +	if (!mask) {
> +		if (omask)
> +			return regmap_update_bits(regmap, oreg, omask, omask);
> +
> +		return -EINVAL;
> +	}
>  	for (i = 0; i < desc->n_voltages; i++) {
> -		ret = regulator_desc_list_voltage_linear_range(desc, i);
> +		/* NOTE to next hacker - Does not support pickable ranges */
> +		if (desc->linear_range_selectors)
> +			return -EINVAL;
> +		if (desc->n_linear_ranges)
> +			ret = regulator_desc_list_voltage_linear_range(desc, i);
> +		else
> +			ret = regulator_desc_list_voltage_linear(desc, i);
>  		if (ret < 0)
>  			continue;
>  		if (ret == uv) {
> @@ -79,6 +92,12 @@ int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
>  				mask = dvs->lpsr_mask;
>  				omask = dvs->lpsr_on_mask;
>  				break;
> +			case ROHM_DVS_LEVEL_SNVS:
> +				prop = "rohm,dvs-snvs-voltage";
> +				reg = dvs->snvs_reg;
> +				mask = dvs->snvs_mask;
> +				omask = dvs->snvs_on_mask;
> +				break;
>  			default:
>  				return -EINVAL;
>  			}
> diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
> index e99e569d3cc1..2f5fbfd0c6b3 100644
> --- a/include/linux/mfd/rohm-generic.h
> +++ b/include/linux/mfd/rohm-generic.h
> @@ -27,7 +27,8 @@ enum {
>  	ROHM_DVS_LEVEL_IDLE,
>  	ROHM_DVS_LEVEL_SUSPEND,
>  	ROHM_DVS_LEVEL_LPSR,
> -	ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR,
> +	ROHM_DVS_LEVEL_SNVS,
> +	ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_SNVS,
>  };

Does this actually work?

The code that consumes it looks like:

    for (i = 0; i < ROHM_DVS_LEVEL_MAX && !ret; i++)

So it will loop through like:

0 (ROHM_DVS_LEVEL_IDLE)
1 (ROHM_DVS_LEVEL_SUSPEND)
2 (ROHM_DVS_LEVEL_LPSR)
3 (ROHM_DVS_LEVEL_SNVS)

Then break, since 'i' will be (== 4) not (< 4).

So the following will never be used:

4 (ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_SNVS)

Unless I'm missing something, I think MAX should be the last entry.

>  /**
> @@ -66,6 +67,9 @@ struct rohm_dvs_config {
>  	unsigned int lpsr_reg;
>  	unsigned int lpsr_mask;
>  	unsigned int lpsr_on_mask;
> +	unsigned int snvs_reg;
> +	unsigned int snvs_mask;
> +	unsigned int snvs_on_mask;
>  };
>  
>  #if IS_ENABLED(CONFIG_REGULATOR_ROHM)

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog