Re: [PATCH 2/3] PCI: brcmstb: Add regulator support

[Linus Walleij <linus.walleij@linaro.org>]

Hi Jaedon,

thanks for your patch!

On Thu, Feb 13, 2020 at 3:59 AM Jaedon Shin <jaedon.shin@gmail.com> wrote:

> +#ifdef CONFIG_REGULATOR
> +       int                     num_regs;
> +       struct regulator        **regs;
> +#endif

Is this #ifdef:in necessary? Since the regulator framework provides
stubs if compiled out, I think you can just include all code
unconditionally and it will work fine anyway.

> +static void brcm_pcie_regulator_enable(struct brcm_pcie *pcie)
> +static void brcm_pcie_regulator_disable(struct brcm_pcie *pcie)
> +static void brcm_pcie_regulator_init(struct brcm_pcie *pcie)

I would replace the word "regulator" with "power" here to indicate
what it is about (easier to read).

> +       struct device_node *np = pcie->dev->of_node;
> +       struct device *dev = pcie->dev;
> +       const char *name;
> +       struct regulator *reg;
> +       int i;
> +
> +       pcie->num_regs = of_property_count_strings(np, "supply-names");
> +       if (pcie->num_regs <= 0) {
> +               pcie->num_regs = 0;
> +               return;
> +       }
> +
> +       pcie->regs = devm_kcalloc(dev, pcie->num_regs, sizeof(pcie->regs[0]),
> +                                 GFP_KERNEL);
> +       if (!pcie->regs) {
> +               pcie->num_regs = 0;
> +               return;
> +       }
> +
> +       for (i = 0; i < pcie->num_regs; i++) {
> +               if (of_property_read_string_index(np, "supply-names", i, &name))
> +                       continue;
> +
> +               reg = devm_regulator_get_optional(dev, name);
> +               if (IS_ERR(reg))
> +                       continue;
> +
> +               pcie->regs[i] = reg;
> +       }
> +}

So what this does is just grab any regulators, no matter what they are
named, and enable them? The swiss army knife used is the raw
of_* parsing functions.

I don't think that is very good practice.

First define very cleanly what regulators exist in the device tree bindings.
If the set of regulators differ between variants, then key that with the
compatible value.

Then explicitly grab the resources by name, using the
regulator_bulk_get() API, which will transparently grab the
regulators for you from the device tree.

Then use regulator_bulk_[enable|disable]
 to enable/disable the regulators.

git grep in the kernel tree for good examples!

Also involve the regulator maintainer in the review. (I added
him on the To: line.)

Yours,
Linus Walleij