diff mbox series

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

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

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(-)

-- 
2.21.0.5.gaeb582a983

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) {


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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.
Mark Brown May 30, 2019, 3:26 p.m. UTC | #5 
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 | #6 
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
diff mbox series

Patch

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) {