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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMty3ZDgyAx-maPqEOR_cBizQDfRZB0EMGj6iddK1BhGvziFkA@mail.gmail.com>
Date:   Thu, 17 Dec 2020 23:11:52 +0530
From:   Jagan Teki <jagan@...rulasolutions.com>
To:     Adrien Grassein <adrien.grassein@...il.com>
Cc:     Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Troy Kisky <troy.kisky@...ndarydevices.com>
Subject: Re: [PATCH v2 7/7] regulator: pf8x00: fix nxp,phase-shift

On Wed, Dec 16, 2020 at 5:27 AM Adrien Grassein
<adrien.grassein@...il.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@...il.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.

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