Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Mark Brown]

On Wed, May 29, 2019 at 03:47:08PM -0500, Dan Murphy wrote:
> On 5/29/19 10:10 AM, Mark Brown wrote:
> > On Wed, May 29, 2019 at 06:51:32AM -0500, Dan Murphy wrote:

> > > Although I don't disagree with you I don't see how the interface is fragile
> > > with only these 3 regulators defined.
> > > Would it not be prudent to amend this driver if/when a new regulator is
> > > needed that has a different enable bit/register combination?   And if that

> > The fragility I'm worried about is someone forgetting to make suitable
> > updates, especially if they don't use the feature in their own system.

> If they don't define the enable GPIO in the device tree then the gpio
> descriptor pointer is NULL and the register write does not occur.

> The documentation indicates that this is only applicable for 3632 I need to
> add the LM36274.

This isn't so much about people's DTs (though that's a definite concern
as well) as it is about support for any future devices in the driver, a
user might see that the driver supports GPIO enables, correctly set up
their device tree and have things fall over because the driver silently
tries to configure the registers incorrectly.

> Currently I don't have a device in this family that requires this level of
> flexibility for this pin or configuration.

> So if a need should arise should we not implement that flexibility at that
> point?

This isn't about implementing support for some theoretical thing, this
is about making the implementation of the current support more robust
and making the driver more maintainable going forwards.

> Not only this but the gpio descriptor is protected in
> lm363x_regulator_of_get_enable_gpio due to checking of the regulator ID.  As
> in patch #4 of this series if LM3632 or LM36274 check for enable definition
> in the DT and create the descriptor if found.  If it is any other regulator
> then don't do anything.

> If the HW designer changes these bits we end up with a new part and then at
> that point we could add code to redefine the bit mask as well.

That code is rather far away from the code you're changing and it's
really not clear that this will do the right thing for new devices, it
already appears that we're handling two devices without obvious code for
that (the regulator IDs get reused AFAICT).  If there were a switch
statement right there it'd be clearer.

Part of this is a reviewability thing - I initially stopped on the patch
because it looked like it was using the enable field for something other
than the intended purpose and now there's all this chasing around to see
if the code is OK.

Attachment: signature.asc
Description: PGP signature