Re: [PATCH v3 2/3] regulator: dt-bindings: Add mps,mpq2286 power-management IC

[Guenter Roeck]

On Wed, Oct 11, 2023 at 12:02:44PM +0530, Saravanan Sekar wrote:
> On 08/10/23 19:22, Guenter Roeck wrote:
> > On Sun, Oct 08, 2023 at 12:40:29PM +0200, Krzysztof Kozlowski wrote:
> > > On 08/10/2023 03:20, Guenter Roeck wrote:
> > > > On Sat, Oct 07, 2023 at 10:28:02PM +0530, Saravanan Sekar wrote:
> > > > > Document mpq2286 power-management IC. Instead of simple 'buck', 'buck0' is
> > > > > used to keep the driver common which handles multiple regulators.
> > > > 
> > > > Sorry for the maybe dumb question, but where can I find the driver
> > > > depencency on buck naming ?
> > > 
> > > I guess it is because:
> > > PMBUS_REGULATOR_STEP("buck", 0, MPQ7932_N_VOLTAGES,
> > > creates regulator name as buck+id (so buck0).
> > > 
> > 
> > Ah, good point. Problem here is that this is already kind of common,
> > even though the use of "buckX" isn't. Look for "vout0", or
> > 'PMBUS_REGULATOR("vout", 0)'. Apparently so far no one took offence
> > if a regulator was named "vout0" even if "vout1" didn't exist.
> > 
> > I don't really have a good solution right now, but I guess we'll need
> > a second set of macros for the single-regulator case, or maybe generate
> > struct regulator_desc arrays using a function. I'll have to explore
> > options.
> > 
> > Please let me know how you want the subsystem to handle existing
> > single-channel regulators with numbered regulator name.
> > 
> > Saravanan - for this driver please just declare a local driver-specific
> > variant of the PMBUS_REGULATOR_STEP() macro which doesn't use indexing,
> > use it to initialise a second regulators_desc array, and use that second
> > array for mpq2286. That is a bit messy, but acceptable for now until
> > there is a more generic solution (unless of course you have an idea for
> > one and want to implement it, but that is not a requirement).
> Hello Guenter,
> 
> Thanks for your proposal as intermediate fix local declaration of macro,
> could you please suggest whether below changes is acceptable as workaround?
> 

No, because that would overwrite a data structure which might be needed
by another mpq7932 in the system. mpq7932_regulators_desc should really
be declared const to clarify that it is not supposed to be changed.

Thinking more about it, the solution is actually quite simple. Please add
a second patch adding PMBUS_REGULATOR_STEP_ONE() and PMBUS_REGULATOR_ONE()
macros to drivers/hwmon/pmbus/pmbus.h and use the new macro in this patch.
That would result in code such as

static const struct regulator_desc mpq7932_regulators_desc_one[] = {
        PMBUS_REGULATOR_STEP_ONE("buck", MPQ7932_N_VOLTAGES,
				 MPQ7932_UV_STEP, MPQ7932_BUCK_UV_MIN),
};

...

        if (info->num_regulators == 1)
                info->reg_desc = mpq7932_regulators_desc_one;
        else
                info->reg_desc = mpq7932_regulators_desc;

We can then use the xxx_ONE macros when adding regulator support to existing
or new drivers, and either keep existing drivers as-is or update them based
on DT maintainer input.

Thanks,
Guenter