Re: [PATCH v4 4/7] regulator: qcom_spmi: Add support for PM8005

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

On Mon, Jun 17, 2019 at 09:17:21AM -0600, Jeffrey Hugo wrote:
> On Mon, Jun 17, 2019 at 9:05 AM Mark Brown <broonie@kernel.org> wrote:

> > > +static int spmi_regulator_ftsmps426_set_voltage(struct regulator_dev *rdev,
> > > +                                           unsigned selector)
> > > +{

> > > +     mV = spmi_regulator_common_list_voltage(rdev, selector) / 1000;

> > This could just be a set_voltage_sel(), no need for it to be a
> > set_voltage() operation....

> This is a set_voltage_sel() in spmi_ftsmps426_ops.  Is the issue because this
> function is "spmi_regulator_ftsmps426_set_voltage" and not
> "spmi_regulator_ftsmps426_set_voltage_sel"?

Well, that's certainly confusing naming and there's some confusion in
the code about what a selector is - it's supposed to be a raw register
value so if you're having to convert it into a voltage something is
going wrong.  Just implement a set_voltage() operation?

> We already have code in the driver to convert a selector to the
> voltage.  Why duplicate
> that inline in spmi_regulator_ftsmps426_set_voltage?

Either work with selectors or work with voltages, don't mix and match
the two.

> > > +     switch (mode) {
> > > +     case REGULATOR_MODE_NORMAL:
> > > +             val = SPMI_FTSMPS426_MODE_HPM_MASK;
> > > +             break;
> > > +     case REGULATOR_MODE_FAST:
> > > +             val = SPMI_FTSMPS426_MODE_AUTO_MASK;
> > > +             break;
> > > +     default:
> > > +             val = SPMI_FTSMPS426_MODE_LPM_MASK;
> > > +             break;
> > > +     }

> > This should validate, it shouldn't just translate invalid values into
> > valid ones.

> Validate what?  The other defines are REGULATOR_MODE_IDLE
> and REGULATOR_MODE_STANDBY which correspond to the LPM
> mode.  Or are you suggesting that regulator framework is going to pass
> REGULATOR_MODE_INVALID to this operation?

You should be validating that the argument passed in is one that the
driver understands, your assumption will break if we add any new modes
and in any case there should be a 1:1 mapping between hardware and API
modes so you shouldn't be translating two different API modes into the
same hardware mode.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]