Re: [PATCH] regulator: bd718x7, bd71828, Fix dvs voltage levels

[Lee Jones]

On Fri, 15 Jan 2021, Matti Vaittinen wrote:

> The ROHM BD718x7 and BD71828 drivers support setting HW state
> specific voltages from device-tree. This is used also by various
> in-tree DTS files.
> 
> These drivers do incorrectly try to compose bit-map using enum
> values. By a chance this works for first two valid levels having
> values 1 and 2 - but setting values for the rest of the levels
> do indicate capbility of setting values for first levels as
> well. Luckily the regulators which support settin values for
> SUSPEND/LPSR do usually also support setting values for RUN
> and IDLE too - thus this has not been such a fatal issue.
> 
> Fix this by defining the old enum values as bits and using
> new enum in parsing code. This allows keeping existing IC
> specific drivers intact and only adding the defines and
> slightly changing the rohm-regulator.c
> 
> Fixes: 21b72156ede8b ("regulator: bd718x7: Split driver to common and bd718x7 specific parts")
> Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/regulator/rohm-regulator.c |  8 ++++----
>  include/linux/mfd/rohm-generic.h   | 22 ++++++++++++++++------
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/regulator/rohm-regulator.c b/drivers/regulator/rohm-regulator.c
> index 399002383b28..96caae7dbef4 100644
> --- a/drivers/regulator/rohm-regulator.c
> +++ b/drivers/regulator/rohm-regulator.c
> @@ -55,25 +55,25 @@ int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
>  	for (i = 0; i < ROHM_DVS_LEVEL_MAX && !ret; i++) {
>  		if (dvs->level_map & (1 << i)) {
>  			switch (i + 1) {
> -			case ROHM_DVS_LEVEL_RUN:
> +			case _ROHM_DVS_LEVEL_RUN:
>  				prop = "rohm,dvs-run-voltage";
>  				reg = dvs->run_reg;
>  				mask = dvs->run_mask;
>  				omask = dvs->run_on_mask;
>  				break;
> -			case ROHM_DVS_LEVEL_IDLE:
> +			case _ROHM_DVS_LEVEL_IDLE:
>  				prop = "rohm,dvs-idle-voltage";
>  				reg = dvs->idle_reg;
>  				mask = dvs->idle_mask;
>  				omask = dvs->idle_on_mask;
>  				break;
> -			case ROHM_DVS_LEVEL_SUSPEND:
> +			case _ROHM_DVS_LEVEL_SUSPEND:
>  				prop = "rohm,dvs-suspend-voltage";
>  				reg = dvs->suspend_reg;
>  				mask = dvs->suspend_mask;
>  				omask = dvs->suspend_on_mask;
>  				break;
> -			case ROHM_DVS_LEVEL_LPSR:
> +			case _ROHM_DVS_LEVEL_LPSR:
>  				prop = "rohm,dvs-lpsr-voltage";
>  				reg = dvs->lpsr_reg;
>  				mask = dvs->lpsr_mask;
> diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
> index 4283b5b33e04..a557988831d7 100644
> --- a/include/linux/mfd/rohm-generic.h
> +++ b/include/linux/mfd/rohm-generic.h
> @@ -20,15 +20,25 @@ struct rohm_regmap_dev {
>  	struct regmap *regmap;
>  };
>  
> +/*
> + * Do not use these in IC specific driver - the bit map should be created using
> + * defines without the underscore. These should be used only in rohm-regulator.c
> + */
>  enum {
> -	ROHM_DVS_LEVEL_UNKNOWN,
> -	ROHM_DVS_LEVEL_RUN,
> -	ROHM_DVS_LEVEL_IDLE,
> -	ROHM_DVS_LEVEL_SUSPEND,
> -	ROHM_DVS_LEVEL_LPSR,
> -	ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR,
> +	_ROHM_DVS_LEVEL_UNKNOWN,
> +	_ROHM_DVS_LEVEL_RUN,
> +	_ROHM_DVS_LEVEL_IDLE,
> +	_ROHM_DVS_LEVEL_SUSPEND,
> +	_ROHM_DVS_LEVEL_LPSR,
> +	ROHM_DVS_LEVEL_MAX = _ROHM_DVS_LEVEL_LPSR,

I don't understand how this is still not broken.

I think you either need to change the for() loop that consumes this to
use "<=" or push the MAX enum to the last line (on its own).

The latter would be my personal preference.

>  };
>  
> +#define ROHM_DVS_LEVEL_UNKNOWN	(1 << _ROHM_DVS_LEVEL_UNKNOWN)
> +#define ROHM_DVS_LEVEL_RUN	(1 << _ROHM_DVS_LEVEL_RUN)
> +#define ROHM_DVS_LEVEL_IDLE	(1 << _ROHM_DVS_LEVEL_IDLE)
> +#define ROHM_DVS_LEVEL_SUSPEND	(1 << _ROHM_DVS_LEVEL_SUSPEND)
> +#define ROHM_DVS_LEVEL_LPSR	(1 << _ROHM_DVS_LEVEL_LPSR)
> +
>  /**
>   * struct rohm_dvs_config - dynamic voltage scaling register descriptions
>   *
> 
> base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837

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