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

[Vaittinen, Matti]

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@xxxxxxxxxxxxxxxxx>
> > ---
> > @@ -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)