2021-11-03 19:49:01

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote:

> + for_each_property_of_node(dn, pp) {
> + for (i = 0; i < ns; i++) {
> + char prop_name[64]; /* 64 is max size of property name */
> +
> + snprintf(prop_name, 64, "%s-supply", supplies[i]);
> + if (strcmp(prop_name, pp->name) == 0)
> + break;
> + }
> + if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies))
> + continue;
> +
> + pcie->supplies[pcie->num_supplies++].supply = supplies[i];
> + }

Why are we doing this? If the DT omits the supplies the framework will
provide dummy supplies so there is no need to open code handling for
supplies not being present at all in client drivers. Just
unconditionally ask for all the supplies.


Attachments:
(No filename) (741.00 B)
signature.asc (499.00 B)
Download all attachments

2021-11-03 20:26:06

by Jim Quinlan

[permalink] [raw]
Subject: Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

On Wed, Nov 3, 2021 at 3:45 PM Mark Brown <[email protected]> wrote:
>
> On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote:
>
> > + for_each_property_of_node(dn, pp) {
> > + for (i = 0; i < ns; i++) {
> > + char prop_name[64]; /* 64 is max size of property name */
> > +
> > + snprintf(prop_name, 64, "%s-supply", supplies[i]);
> > + if (strcmp(prop_name, pp->name) == 0)
> > + break;
> > + }
> > + if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies))
> > + continue;
> > +
> > + pcie->supplies[pcie->num_supplies++].supply = supplies[i];
> > + }
>
> Why are we doing this? If the DT omits the supplies the framework will
> provide dummy supplies so there is no need to open code handling for
> supplies not being present at all in client drivers. Just
> unconditionally ask for all the supplies.

I did it to squelch the "supply xxxxx not found, using dummy
regulator" output. I'll change it.
Regards, Jim

2021-11-03 20:34:50

by Jim Quinlan

[permalink] [raw]
Subject: Re: [PATCH v7 5/7] PCI: brcmstb: Add control of subdevice voltage regulators

On Wed, Nov 3, 2021 at 4:25 PM Jim Quinlan <[email protected]> wrote:
>
> On Wed, Nov 3, 2021 at 3:45 PM Mark Brown <[email protected]> wrote:
> >
> > On Wed, Nov 03, 2021 at 02:49:35PM -0400, Jim Quinlan wrote:
> >
> > > + for_each_property_of_node(dn, pp) {
> > > + for (i = 0; i < ns; i++) {
> > > + char prop_name[64]; /* 64 is max size of property name */
> > > +
> > > + snprintf(prop_name, 64, "%s-supply", supplies[i]);
> > > + if (strcmp(prop_name, pp->name) == 0)
> > > + break;
> > > + }
> > > + if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies))
> > > + continue;
> > > +
> > > + pcie->supplies[pcie->num_supplies++].supply = supplies[i];
> > > + }
> >
> > Why are we doing this? If the DT omits the supplies the framework will
> > provide dummy supplies so there is no need to open code handling for
> > supplies not being present at all in client drivers. Just
> > unconditionally ask for all the supplies.
>
> I did it to squelch the "supply xxxxx not found, using dummy
> regulator" output. I'll change it.
Now I remember: if I know there are no vpciexxx-supplly props in the
DT, I can skip executing all of the buik regulator calls entirely, as
well as walking the PCI bus as in brcm_regulators_off().

Do you consider this a valid reason?

Jim

Jim

Jim

> Regards, Jim