Re: [PATCH 1/3] regulator: s2mps11: Refactor setting ramp delay

[Krzysztof Kozlowski]

On wto, 2014-05-27 at 11:56 +0530, Yadwinder Singh Brar wrote:
> Hi Krzysztof,
> 
> 
> On Mon, May 26, 2014 at 6:50 PM, Krzysztof Kozlowski
> <k.kozlowski@xxxxxxxxxxx> wrote:
> > Prepare for merging the s2mpa01 regulator driver into s2mps11 by:
> > 1. Adding common id for buck regulators.
> > 2. Splitting shared ramp delay settings to match S2MPA01.
> > 3. Adding a configuration of registers for setting ramp delay for each
> >    buck regulator.
> >
> > The functionality of the driver should not change as this patch only
> > prepares for supporting S2MPA01 device.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> > ---
> >  drivers/regulator/s2mps11.c | 210 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 144 insertions(+), 66 deletions(-)
> >
> 
> [snip]
> 
> >
> > -               if (ramp_delay > s2mps11->ramp_delay34)
> > -                       s2mps11->ramp_delay34 = ramp_delay;
> > +               if (ramp_delay > s2mps11->ramp_delay3)
> > +                       s2mps11->ramp_delay3 = ramp_delay;
> >                 else
> > -                       ramp_delay = s2mps11->ramp_delay34;
> > -
> > -               ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
> > -               ramp_reg = S2MPS11_REG_RAMP;
> > +                       ramp_delay = s2mps11->ramp_delay3;
> >                 break;
> >         case S2MPS11_BUCK4:
> > -               enable_shift = S2MPS11_BUCK4_RAMP_EN_SHIFT;
> >                 if (!ramp_delay) {
> >                         ramp_enable = 0;
> >                         break;
> >                 }
> >
> > -               if (ramp_delay > s2mps11->ramp_delay34)
> > -                       s2mps11->ramp_delay34 = ramp_delay;
> > +               if (ramp_delay > s2mps11->ramp_delay4)
> > +                       s2mps11->ramp_delay4 = ramp_delay;
> >                 else
> > -                       ramp_delay = s2mps11->ramp_delay34;
> > -
> > -               ramp_shift = S2MPS11_BUCK34_RAMP_SHIFT;
> > -               ramp_reg = S2MPS11_REG_RAMP;
> > +                       ramp_delay = s2mps11->ramp_delay4;
> 
> Main rationale behind shared value is completely omitted here, in
> other cases also,
> after just giving a NOTE in documentation asking user to make sure to
> pass same value.
> It doesn't seem safe, simply leaving a scope of stability issue (in
> case ramp_delay3 > ramp_delay4).

The documentation already states that these bucks (e.g. 3 and 4) share
the ramp delay setting and 'regulator-ramp-delay' should be the same.
However you're right that patch is not safe against changing shared ramp
delays to different values. Previously the code was safe so this is not
entirely "non-functional" change. I'll fix it to retain the same
behavior.

Best regards,
Krzysztof



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/