[PATCH 1/2] regulator: max8660: Remove boot_on handling

[PATCH 1/2] regulator: max8660: Remove boot_on handling

[Markus Pargmann]

boot_on is handled by the regulator core. It will call enable() on the
regulators that are specified to be boot_on. So we don't require any
additional handling in this driver

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/regulator/max8660.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/regulator/max8660.c b/drivers/regulator/max8660.c
index 7eee2ca18541..32cf277a7662 100644
--- a/drivers/regulator/max8660.c
+++ b/drivers/regulator/max8660.c
@@ -385,7 +385,7 @@ static int max8660_probe(struct i2c_client *client,
 	struct max8660_platform_data *pdata = dev_get_platdata(dev);
 	struct regulator_config config = { };
 	struct max8660 *max8660;
-	int boot_on, i, id, ret = -EINVAL;
+	int i, id, ret = -EINVAL;
 	struct device_node *of_node[MAX8660_V_END];
 	unsigned long type;
 
@@ -441,39 +441,21 @@ static int max8660_probe(struct i2c_client *client,
 		max8660->shadow_regs[MAX8660_MDTV2] = 0x04;
 
 	for (i = 0; i < pdata->num_subdevs; i++) {
-
 		if (!pdata->subdevs[i].platform_data)
-			return ret;
-
-		boot_on = pdata->subdevs[i].platform_data->constraints.boot_on;
+			return -EINVAL;
 
 		switch (pdata->subdevs[i].id) {
 		case MAX8660_V3:
-			if (boot_on)
-				max8660->shadow_regs[MAX8660_OVER1] |= 1;
-			break;
-
 		case MAX8660_V4:
-			if (boot_on)
-				max8660->shadow_regs[MAX8660_OVER1] |= 4;
-			break;
-
 		case MAX8660_V5:
-			break;
-
 		case MAX8660_V6:
-			if (boot_on)
-				max8660->shadow_regs[MAX8660_OVER2] |= 2;
 			break;
-
 		case MAX8660_V7:
 			if (type == MAX8661) {
 				dev_err(dev, "Regulator not on this chip!\n");
 				return -EINVAL;
 			}
 
-			if (boot_on)
-				max8660->shadow_regs[MAX8660_OVER2] |= 4;
 			break;
 
 		default:
-- 
2.1.4

[PATCH 2/2] regulator: max8660: Add error message for missing regulator data

[Markus Pargmann]

The driver probe fails when there is a subdevice without platform_data.
Add a error message so it is clear what failed.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/regulator/max8660.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/max8660.c b/drivers/regulator/max8660.c
index 32cf277a7662..b1ba1d09e064 100644
--- a/drivers/regulator/max8660.c
+++ b/drivers/regulator/max8660.c
@@ -441,8 +441,10 @@ static int max8660_probe(struct i2c_client *client,
 		max8660->shadow_regs[MAX8660_MDTV2] = 0x04;
 
 	for (i = 0; i < pdata->num_subdevs; i++) {
-		if (!pdata->subdevs[i].platform_data)
+		if (!pdata->subdevs[i].platform_data) {
+			dev_err(dev, "No data for %d regulator\n", i);
 			return -EINVAL;
+		}
 
 		switch (pdata->subdevs[i].id) {
 		case MAX8660_V3:
-- 
2.1.4

Re: [PATCH 2/2] regulator: max8660: Add error message for missing regulator data

[Wolfram Sang]

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


>  	for (i = 0; i < pdata->num_subdevs; i++) {
> -		if (!pdata->subdevs[i].platform_data)
> +		if (!pdata->subdevs[i].platform_data) {
> +			dev_err(dev, "No data for %d regulator\n", i);
>  			return -EINVAL;
> +		}

I'd rather save the string and go for -ENOENT instead. But I don't mind
much...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] regulator: max8660: Add error message for missing regulator data

[Markus Pargmann]

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

Hi Wolfram,

On Tue, Mar 24, 2015 at 01:38:09PM +0100, Wolfram Sang wrote:
> 
> >  	for (i = 0; i < pdata->num_subdevs; i++) {
> > -		if (!pdata->subdevs[i].platform_data)
> > +		if (!pdata->subdevs[i].platform_data) {
> > +			dev_err(dev, "No data for %d regulator\n", i);
> >  			return -EINVAL;
> > +		}
> 
> I'd rather save the string and go for -ENOENT instead. But I don't mind
> much...

I think -ENOENT doesn't describe it enough. For example if pdata is
parsed from DT, this may as well be a missing regulator node in the
devicetree.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] regulator: max8660: Add error message for missing regulator data

[Mark Brown]

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

On Tue, Mar 24, 2015 at 01:08:03PM +0100, Markus Pargmann wrote:
> The driver probe fails when there is a subdevice without platform_data.
> Add a error message so it is clear what failed.

...

>  	for (i = 0; i < pdata->num_subdevs; i++) {
> -		if (!pdata->subdevs[i].platform_data)
> +		if (!pdata->subdevs[i].platform_data) {
> +			dev_err(dev, "No data for %d regulator\n", i);
>  			return -EINVAL;
> +		}

Why is the platform data mandatory?  In general the goal is that a
regulator driver should be able to probe with no platform data.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regulator: max8660: Add error message for missing regulator data

[Markus Pargmann]

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

On Tue, Mar 24, 2015 at 09:08:57AM -0700, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 01:08:03PM +0100, Markus Pargmann wrote:
> > The driver probe fails when there is a subdevice without platform_data.
> > Add a error message so it is clear what failed.
> 
> ...
> 
> >  	for (i = 0; i < pdata->num_subdevs; i++) {
> > -		if (!pdata->subdevs[i].platform_data)
> > +		if (!pdata->subdevs[i].platform_data) {
> > +			dev_err(dev, "No data for %d regulator\n", i);
> >  			return -EINVAL;
> > +		}
> 
> Why is the platform data mandatory?  In general the goal is that a
> regulator driver should be able to probe with no platform data.

subdevs[]->platform_data is a struct regulator_init_data which has to
exist so we can register the regulator later.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] regulator: max8660: Add error message for missing regulator data

[Mark Brown]

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

On Tue, Mar 24, 2015 at 05:40:11PM +0100, Markus Pargmann wrote:
> On Tue, Mar 24, 2015 at 09:08:57AM -0700, Mark Brown wrote:

> > Why is the platform data mandatory?  In general the goal is that a
> > regulator driver should be able to probe with no platform data.

> subdevs[]->platform_data is a struct regulator_init_data which has to
> exist so we can register the regulator later.

No, that's not the case.  The regulator API does not require that one be
provided in order to support the above use case.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: max8660: Remove boot_on handling

[Mark Brown]

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

On Tue, Mar 24, 2015 at 01:08:02PM +0100, Markus Pargmann wrote:

> boot_on is handled by the regulator core. It will call enable() on the
> regulators that are specified to be boot_on. So we don't require any
> additional handling in this driver

>  		switch (pdata->subdevs[i].id) {
>  		case MAX8660_V3:
> -			if (boot_on)
> -				max8660->shadow_regs[MAX8660_OVER1] |= 1;
> -			break;
> -

So, this is updating the register default value rather than writing to
the device.  That suggests that the purpose is to either avoid needless
write outs to the device or something, or at the very least it's not a
completely direct reimplementation of the core functionality.  It is
certainly underdocumented at the minute (and the driver probably wants
to be converted to regmap) but this makes me a bit cautious about
obvious cleanups missing something.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: max8660: Remove boot_on handling

[Markus Pargmann]

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

On Tue, Mar 24, 2015 at 09:22:27AM -0700, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 01:08:02PM +0100, Markus Pargmann wrote:
> 
> > boot_on is handled by the regulator core. It will call enable() on the
> > regulators that are specified to be boot_on. So we don't require any
> > additional handling in this driver
> 
> >  		switch (pdata->subdevs[i].id) {
> >  		case MAX8660_V3:
> > -			if (boot_on)
> > -				max8660->shadow_regs[MAX8660_OVER1] |= 1;
> > -			break;
> > -
> 
> So, this is updating the register default value rather than writing to
> the device.  That suggests that the purpose is to either avoid needless
> write outs to the device or something, or at the very least it's not a
> completely direct reimplementation of the core functionality.  It is
> certainly underdocumented at the minute (and the driver probably wants
> to be converted to regmap) but this makes me a bit cautious about
> obvious cleanups missing something.

Sorry, this patch is wrong. I just realized that this chip is write-only
and has different regulators that share the same registers. So it is
impossible to selectively enable some regulators without accidentaly
disabling others that may be enabled. So the code is necessary.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]