Re: [PATCH v2 7/7] regulator: pf8x00: fix nxp,phase-shift

[Adrien Grassein <adrien.grassein@gmail.com>]

Hello,

Thanks for your review.

Le jeu. 17 déc. 2020 à 18:42, Jagan Teki <jagan@amarulasolutions.com> a écrit :
>
> On Wed, Dec 16, 2020 at 5:27 AM Adrien Grassein
> <adrien.grassein@gmail.com> wrote:
> >
> > Fix the ternary condition which is a bad coding style
> > in the kernel
> >
> > I also remove the defering configuration of the nxp,phase-shift.
> > The configuration is now done at parsing time. It save some memory
> > and it's better for comprehension.
> >
> > I also use the OTP default configuration when the paramater is wrong
> > or not specified.
> > I think that it's better to use the default configuration from the chip
> > than an arbitrary value.
> >
> > Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> > ---
> >  drivers/regulator/pf8x00-regulator.c | 45 +++++++++++++---------------
> >  1 file changed, 20 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/regulator/pf8x00-regulator.c b/drivers/regulator/pf8x00-regulator.c
> > index 5ad940b3db0a..b8b3ac393ee8 100644
> > --- a/drivers/regulator/pf8x00-regulator.c
> > +++ b/drivers/regulator/pf8x00-regulator.c
> > @@ -107,7 +107,6 @@ enum pf8x00_ldo_states {
> >
> >  #define PF8X00_SWXILIM_MASK            GENMASK(4, 3)
> >  #define PF8X00_SWXPHASE_MASK           GENMASK(2, 0)
> > -#define PF8X00_SWXPHASE_DEFAULT                0
>
> Keep this as it is and assign it 1 as the below code assigns 1.

I don't understand, PF8X00_SWXPHASE_DEFAULT is not used anymore in the code.
Why should I keep it ?
>
> >  #define PF8X00_SWXPHASE_SHIFT          7
> >
> >  enum pf8x00_devid {
> > @@ -121,7 +120,6 @@ enum pf8x00_devid {
> >
> >  struct pf8x00_regulator {
> >         struct regulator_desc desc;
> > -       u8 phase_shift;
> >  };
> >
> >  struct pf8x00_chip {
> > @@ -167,17 +165,13 @@ static const int pf8x00_vsnvs_voltages[] = {
> >         0, 1800000, 3000000, 3300000,
> >  };
> >
> > -static struct pf8x00_regulator *desc_to_regulator(const struct regulator_desc *desc)
> > -{
> > -       return container_of(desc, struct pf8x00_regulator, desc);
> > -}
> > -
> >  static int pf8x00_of_parse_cb(struct device_node *np,
> >                               const struct regulator_desc *desc,
> >                               struct regulator_config *config)
> >  {
> > -       struct pf8x00_regulator *data = desc_to_regulator(desc);
> >         struct pf8x00_chip *chip = config->driver_data;
> > +       unsigned char id = desc->id - PF8X00_LDO4;
> > +       unsigned char reg = PF8X00_SW_BASE(id) + SW_CONFIG2;
> >         int phase;
> >         int val;
> >         int ret;
> > @@ -185,21 +179,30 @@ static int pf8x00_of_parse_cb(struct device_node *np,
> >         ret = of_property_read_u32(np, "nxp,phase-shift", &val);
> >         if (ret) {
> >                 dev_dbg(chip->dev,
> > -                       "unspecified phase-shift for BUCK%d, use 0 degrees\n",
> > -                       desc->id - PF8X00_LDO4);
> > -               val = PF8X00_SWXPHASE_DEFAULT;
> > +                       "unspecified phase-shift for BUCK%d, using OTP configuration\n",
> > +                       id);
> > +               goto end;
> >         }
> >
> > -       phase = val / 45;
> > -       if ((phase * 45) != val) {
> > +       if (val < 0 || val > 315 || val % 45 != 0) {
> >                 dev_warn(config->dev,
> > -                        "invalid phase_shift %d for BUCK%d, use 0 degrees\n",
> > -                        (phase * 45), desc->id - PF8X00_LDO4);
> > -               phase = PF8X00_SWXPHASE_SHIFT;
> > +                        "invalid phase_shift %d for BUCK%d, using OTP configuration\n",
> > +                        val, id);
> > +               goto end;
> >         }
> >
> > -       data->phase_shift = (phase >= 1) ? phase - 1 : PF8X00_SWXPHASE_SHIFT;
> > +       phase = val / 45;
> > +
> > +       if (phase >= 1)
> > +               phase -= 1;
> > +       else
> > +               phase = PF8X00_SWXPHASE_SHIFT;
> > +
> > +       regmap_update_bits(chip->regmap, reg,
> > +                       PF8X00_SWXPHASE_MASK,
> > +                       phase);
>
> Add all these arguments in the same line.
OK.


Thanks again,

Adrien