Re: [PATCH v5 16/18] power: pwrseq: add a driver for the QCA6390 PMU module

[Bartosz Golaszewski]

On Sat, Feb 17, 2024 at 12:17 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Fri, 16 Feb 2024 at 22:33, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > This adds the power sequencing driver for the QCA6390's PMU module. It
> > uses the pwrseq subsystem and knows how to match the sequencer to the
> > consumer device by verifying the relevant properties and DT layout.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > ---

[snip]

> > +
> > +static const struct pwrseq_qca6390_vreg pwrseq_qca6390_vregs_common[] = {
> > +       {
> > +               .name = "vddio",
> > +               .load_uA = 20000,
> > +       },
> > +       {
> > +               .name = "vddaon",
> > +               .load_uA = 100000,
> > +       },
> > +       {
> > +               .name = "vddpmu",
> > +               .load_uA = 1250000,
> > +       },
> > +       {
> > +               .name = "vddrfa0p95",
> > +               .load_uA = 200000,
> > +       },
> > +       {
> > +               .name = "vddrfa1p3",
> > +               .load_uA = 400000,
> > +       },
> > +       {
> > +               .name = "vddrfa1p9",
> > +               .load_uA = 400000,
> > +       },
> > +};
> > +
> > +static const struct pwrseq_qca6390_vreg pwrseq_qca6390_vregs_wlan[] = {
> > +       {
> > +               .name = "vddpcie1p3",
> > +               .load_uA = 35000,
> > +       },
> > +       {
> > +               .name = "vddpcie1p9",
> > +               .load_uA = 15000,
> > +       },
> > +};
>
> I thought that we had discussed this already. According to the docs,
> all PMU supplies should be powered on when the chip is being switched
> on, no matter whether it is for the WiFi or for the BT.
>

I know, I mostly did it to make Bjorn happy because he was adamant we
don't need the PCIe regulators for BT and when I checked, it does work
in practice so I thought: whatever. But indeed, the docs say
otherwise. Noted for v6.

[snip]

> > +
> > +static const struct pwrseq_unit_data pwrseq_qca6390_bt_unit_data = {
> > +       .name = "bluetooth-enable",
> > +       .deps = pwrseq_qca6390_unit_deps,
>
> Can we call corresponding regulator_bulk functions from bt and wlan
> enable/disable? This will save us from building the tree-like
> structures (and possible loops inside that tree).
>

Can we? Sure, but the dependency graph (yeah, we should enforce it to
be acyclic) is what makes this code future-proof and allows it to
avoid repeating calls in different targets.

[snip]

Bart