Hello,
Thanks for your review.
Le jeu. 17 déc. 2020 à 18:42, Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> a écrit :
>
> On Wed, Dec 16, 2020 at 5:27 AM Adrien Grassein
> <adrien.grassein@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
