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: <d5918dd3f7c39306dce17d0ec82823186e9ea0ed.camel@fi.rohmeurope.com>
Date:   Fri, 15 Jan 2021 09:10:19 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "lee.jones@...aro.org" <lee.jones@...aro.org>
CC:     "lgirdwood@...il.com" <lgirdwood@...il.com>,
        linux-power <linux-power@...rohmeurope.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 11/15] regulator: rohm-regulator: SNVS dvs and linear
 voltage support

Hello Lee,

Thanks for carefull review! I do appreciate this!

On Fri, 2021-01-15 at 08:26 +0000, Lee Jones wrote:
> 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>
> > ---
> > @@ -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.

Good catch. I think you are correct. I will revise this for next
version (and add also fixes tag as it seems the current code is broken
for LPSR).

> 
> >  /**
> > @@ -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)