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

[Jagan Teki]

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.

>  #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.