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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210115082636.GF3975472@dell>
Date:   Fri, 15 Jan 2021 08:26:36 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     mazziesaccount@...il.com, Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
        linux-power@...rohmeurope.com
Subject: Re: [PATCH 11/15] regulator: rohm-regulator: SNVS dvs and linear
 voltage support

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@...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