[RESEND,v4,1/6] regulator: lm363x: Make the gpio register enable flexible
diff mbox series

Message ID 20190522192733.13422-2-dmurphy@ti.com
State Superseded
Headers show
Series
  • LM36274 Introduction
Related show

Commit Message

Dan Murphy May 22, 2019, 7:27 p.m. UTC
The use of and enablement of the GPIO can be used across devices.
Use the enable_reg in the regulator descriptor for the register to
write.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/regulator/lm363x-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Pavel Machek May 22, 2019, 9:22 p.m. UTC | #1
On Wed 2019-05-22 14:27:28, Dan Murphy wrote:
> The use of and enablement of the GPIO can be used across devices.
> Use the enable_reg in the regulator descriptor for the register to
> write.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  drivers/regulator/lm363x-regulator.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
> index c876e161052a..382b1cecdd93 100644
> --- a/drivers/regulator/lm363x-regulator.c
> +++ b/drivers/regulator/lm363x-regulator.c
> @@ -263,8 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
>  
>  	if (gpiod) {
>  		cfg.ena_gpiod = gpiod;
> -
> -		ret = regmap_update_bits(regmap, LM3632_REG_BIAS_CONFIG,
> +		ret = regmap_update_bits(regmap,
> +					 lm363x_regulator_desc[id].enable_reg,
>  					 LM3632_EXT_EN_MASK,
>  					 LM3632_EXT_EN_MASK);
>  		if (ret) {
Mark Brown May 23, 2019, 1:03 p.m. UTC | #2
On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:
> The use of and enablement of the GPIO can be used across devices.
> Use the enable_reg in the regulator descriptor for the register to
> write.

> @@ -263,8 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
>  
>  	if (gpiod) {
>  		cfg.ena_gpiod = gpiod;
> -
> -		ret = regmap_update_bits(regmap, LM3632_REG_BIAS_CONFIG,
> +		ret = regmap_update_bits(regmap,
> +					 lm363x_regulator_desc[id].enable_reg,
>  					 LM3632_EXT_EN_MASK,
>  					 LM3632_EXT_EN_MASK);
>  		if (ret) {

Is it guaranteed that the bitmask for enabling the use of the GPIO is
going to be the same for all regulators?  The bitmasks for the regulator
enable look to be different, and it also looks like this setting might
affect multiple regulators since it seems there are multiple enable bits
in the same register.  If this affects multiple regulators then how's
that working at the minute?
Dan Murphy May 23, 2019, 1:50 p.m. UTC | #3
Mark

On 5/23/19 8:03 AM, Mark Brown wrote:
> On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:
>> The use of and enablement of the GPIO can be used across devices.
>> Use the enable_reg in the regulator descriptor for the register to
>> write.
> 
>> @@ -263,8 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
>>  
>>  	if (gpiod) {
>>  		cfg.ena_gpiod = gpiod;
>> -
>> -		ret = regmap_update_bits(regmap, LM3632_REG_BIAS_CONFIG,
>> +		ret = regmap_update_bits(regmap,
>> +					 lm363x_regulator_desc[id].enable_reg,
>>  					 LM3632_EXT_EN_MASK,
>>  					 LM3632_EXT_EN_MASK);
>>  		if (ret) {
> 
> Is it guaranteed that the bitmask for enabling the use of the GPIO is
> going to be the same for all regulators?  The bitmasks for the regulator
> enable look to be different, and it also looks like this setting might
> affect multiple regulators since it seems there are multiple enable bits
> in the same register.  If this affects multiple regulators then how's
> that working at the minute?
> 

Yes for the 3632 and 36274 bit0 is the EXT_EN for LCM on these chips.
LM3631 does not have LCM GPIO control so there is no setting and this should not be called.
If it is then the developer implemented the DT wrong.

LM3631 - No LCM GPIO control

LM36274 reg 0x09 bit 0 7.6.9 of the data sheet
LM3632 reg 0x0c bit 0 7.6.12 of the data sheet

Dan
Mark Brown May 26, 2019, 12:48 p.m. UTC | #4
On Thu, May 23, 2019 at 08:50:20AM -0500, Dan Murphy wrote:
> On 5/23/19 8:03 AM, Mark Brown wrote:
> > On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:

> > Is it guaranteed that the bitmask for enabling the use of the GPIO is
> > going to be the same for all regulators?  The bitmasks for the regulator
> > enable look to be different, and it also looks like this setting might
> > affect multiple regulators since it seems there are multiple enable bits
> > in the same register.  If this affects multiple regulators then how's
> > that working at the minute?

> Yes for the 3632 and 36274 bit0 is the EXT_EN for LCM on these chips.
> LM3631 does not have LCM GPIO control so there is no setting and this should not be called.
> If it is then the developer implemented the DT wrong.

This feels fragile - it works for the current users but it's just
assuming that the placement of this bit will always be in the same
position in the same register as the enable and will silently fail if a
new chip variant does things differently.  Either storing the data
separately somewhere driver specific or just having explicit switch
statements would be more robust.
Dan Murphy May 29, 2019, 11:51 a.m. UTC | #5
Mark

On 5/26/19 7:48 AM, Mark Brown wrote:
> On Thu, May 23, 2019 at 08:50:20AM -0500, Dan Murphy wrote:
>> On 5/23/19 8:03 AM, Mark Brown wrote:
>>> On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:
>>> Is it guaranteed that the bitmask for enabling the use of the GPIO is
>>> going to be the same for all regulators?  The bitmasks for the regulator
>>> enable look to be different, and it also looks like this setting might
>>> affect multiple regulators since it seems there are multiple enable bits
>>> in the same register.  If this affects multiple regulators then how's
>>> that working at the minute?
>> Yes for the 3632 and 36274 bit0 is the EXT_EN for LCM on these chips.
>> LM3631 does not have LCM GPIO control so there is no setting and this should not be called.
>> If it is then the developer implemented the DT wrong.
> This feels fragile - it works for the current users but it's just
> assuming that the placement of this bit will always be in the same
> position in the same register as the enable and will silently fail if a
> new chip variant does things differently.  Either storing the data
> separately somewhere driver specific or just having explicit switch
> statements would be more robust.


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 was the case I would almost expect a different driver completely if 
the regmap did not line up correctly.  I only reused this driver because 
the registers and bits lined up and did not think it was necessary to 
create a whole new driver.

Dan
Mark Brown May 29, 2019, 3:10 p.m. UTC | #6
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.

> was the case I would almost expect a different driver completely if the
> regmap did not line up correctly.  I only reused this driver because the
> registers and bits lined up and did not think it was necessary to create a
> whole new driver.

This is a single register bit which is set once on startup isn't it?  It
seems like exactly the sort of thing that a hardware designer might
change incompatibly, perhaps even for good reasons like adding more
flexibility over which pins can be used to control the enable and far
from something that would require a totally new driver if it was handled
differently.
Dan Murphy May 29, 2019, 8:47 p.m. UTC | #7
Mark

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.

>> was the case I would almost expect a different driver completely if the
>> regmap did not line up correctly.  I only reused this driver because the
>> registers and bits lined up and did not think it was necessary to create a
>> whole new driver.
> This is a single register bit which is set once on startup isn't it?  It
> seems like exactly the sort of thing that a hardware designer might
> change incompatibly, perhaps even for good reasons like adding more
> flexibility over which pins can be used to control the enable and far
> from something that would require a totally new driver if it was handled
> differently.

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?

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.

Dan
Mark Brown May 30, 2019, 3:26 p.m. UTC | #8
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.
Dan Murphy June 4, 2019, 3:14 p.m. UTC | #9
Mark

On 5/30/19 10:26 AM, Mark Brown wrote:
>
> 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.

I am going to introduce this update in patch 4 of this series when the 
LM36274 is introduced to the regulator driver.

It makes more sense to add it there as in patch 1 there is not really a 
need to extend the value or mask as it only supports the LM3632 device.  
It makes sense to add the condition when adding another device to support

Thoughts?

Dan
Mark Brown June 4, 2019, 3:33 p.m. UTC | #10
On Tue, Jun 04, 2019 at 10:14:42AM -0500, Dan Murphy wrote:

> I am going to introduce this update in patch 4 of this series when the
> LM36274 is introduced to the regulator driver.

> It makes more sense to add it there as in patch 1 there is not really a need
> to extend the value or mask as it only supports the LM3632 device.  It makes
> sense to add the condition when adding another device to support

Sure, add it along with the rest of the support for the new device.

Patch
diff mbox series

diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
index c876e161052a..382b1cecdd93 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -263,8 +263,8 @@  static int lm363x_regulator_probe(struct platform_device *pdev)
 
 	if (gpiod) {
 		cfg.ena_gpiod = gpiod;
-
-		ret = regmap_update_bits(regmap, LM3632_REG_BIAS_CONFIG,
+		ret = regmap_update_bits(regmap,
+					 lm363x_regulator_desc[id].enable_reg,
 					 LM3632_EXT_EN_MASK,
 					 LM3632_EXT_EN_MASK);
 		if (ret) {