[PATCH v2] Input: Make ADS7846 independent on regulator

[PATCH v2] Input: Make ADS7846 independent on regulator

[Marek Vasut]

In case regulator was not found, ADS7846 failed to probe.

This fixes a problem on some Sharp Zaurus devices, where there is no regulator
present and the touchscreen is used.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/input/touchscreen/ads7846.c |   38 +++++++++++++++++++---------------
 1 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index a9fdf55..53b318b 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -791,7 +791,8 @@ static void ads7846_disable(struct ads7846 *ts)
 		}
 	}
 
-	regulator_disable(ts->reg);
+	if (ts->reg)
+		regulator_disable(ts->reg);
 
 	/* we know the chip's in lowpower mode since we always
 	 * leave it that way after every request
@@ -804,7 +805,8 @@ static void ads7846_enable(struct ads7846 *ts)
 	if (!ts->disabled)
 		return;
 
-	regulator_enable(ts->reg);
+	if (ts->reg)
+		regulator_enable(ts->reg);
 
 	ts->disabled = 0;
 	ts->irq_disabled = 0;
@@ -1162,16 +1164,15 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 	ts->last_msg = m;
 
 	ts->reg = regulator_get(&spi->dev, "vcc");
-	if (IS_ERR(ts->reg)) {
-		err = PTR_ERR(ts->reg);
-		dev_err(&spi->dev, "unable to get regulator: %d\n", err);
-		goto err_free_gpio;
-	}
-
-	err = regulator_enable(ts->reg);
-	if (err) {
-		dev_err(&spi->dev, "unable to enable regulator: %d\n", err);
-		goto err_put_regulator;
+	if (IS_ERR(ts->reg))
+		ts->reg = NULL;
+	else {
+		err = regulator_enable(ts->reg);
+		if (err) {
+			dev_err(&spi->dev, "unable to enable regulator: %d\n",
+				err);
+			goto err_put_regulator;
+		}
 	}
 
 	if (request_irq(spi->irq, ads7846_irq, IRQF_TRIGGER_FALLING,
@@ -1218,10 +1219,11 @@ static int __devinit ads7846_probe(struct spi_device *spi)
  err_free_irq:
 	free_irq(spi->irq, ts);
  err_disable_regulator:
-	regulator_disable(ts->reg);
+	if (ts->reg)
+		regulator_disable(ts->reg);
  err_put_regulator:
-	regulator_put(ts->reg);
- err_free_gpio:
+	if (ts->reg)
+		regulator_put(ts->reg);
 	if (ts->gpio_pendown != -1)
 		gpio_free(ts->gpio_pendown);
  err_cleanup_filter:
@@ -1251,8 +1253,10 @@ static int __devexit ads7846_remove(struct spi_device *spi)
 	/* suspend left the IRQ disabled */
 	enable_irq(ts->spi->irq);
 
-	regulator_disable(ts->reg);
-	regulator_put(ts->reg);
+	if (ts->reg) {
+		regulator_disable(ts->reg);
+		regulator_put(ts->reg);
+	}
 
 	if (ts->gpio_pendown != -1)
 		gpio_free(ts->gpio_pendown);
-- 
1.7.1

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Igor Grinberg]

 Seems that there are some more platforms then Sharp Zaurus
are interested in this patch.

Dmitry, Mike, Pavel,

What's the status of it?

On 07/31/10 10:09, Marek Vasut wrote:
> In case regulator was not found, ADS7846 failed to probe.
>
> This fixes a problem on some Sharp Zaurus devices, where there is no regulator
> present and the touchscreen is used.
>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> ---
>  drivers/input/touchscreen/ads7846.c |   38 +++++++++++++++++++---------------
>  1 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index a9fdf55..53b318b 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -791,7 +791,8 @@ static void ads7846_disable(struct ads7846 *ts)
>  		}
>  	}
>  
> -	regulator_disable(ts->reg);
> +	if (ts->reg)
> +		regulator_disable(ts->reg);
>  
>  	/* we know the chip's in lowpower mode since we always
>  	 * leave it that way after every request
> @@ -804,7 +805,8 @@ static void ads7846_enable(struct ads7846 *ts)
>  	if (!ts->disabled)
>  		return;
>  
> -	regulator_enable(ts->reg);
> +	if (ts->reg)
> +		regulator_enable(ts->reg);
>  
>  	ts->disabled = 0;
>  	ts->irq_disabled = 0;
> @@ -1162,16 +1164,15 @@ static int __devinit ads7846_probe(struct spi_device *spi)
>  	ts->last_msg = m;
>  
>  	ts->reg = regulator_get(&spi->dev, "vcc");
> -	if (IS_ERR(ts->reg)) {
> -		err = PTR_ERR(ts->reg);
> -		dev_err(&spi->dev, "unable to get regulator: %d\n", err);
> -		goto err_free_gpio;
> -	}
> -
> -	err = regulator_enable(ts->reg);
> -	if (err) {
> -		dev_err(&spi->dev, "unable to enable regulator: %d\n", err);
> -		goto err_put_regulator;
> +	if (IS_ERR(ts->reg))
> +		ts->reg = NULL;
> +	else {
> +		err = regulator_enable(ts->reg);
> +		if (err) {
> +			dev_err(&spi->dev, "unable to enable regulator: %d\n",
> +				err);
> +			goto err_put_regulator;
> +		}
>  	}
>  
>  	if (request_irq(spi->irq, ads7846_irq, IRQF_TRIGGER_FALLING,
> @@ -1218,10 +1219,11 @@ static int __devinit ads7846_probe(struct spi_device *spi)
>   err_free_irq:
>  	free_irq(spi->irq, ts);
>   err_disable_regulator:
> -	regulator_disable(ts->reg);
> +	if (ts->reg)
> +		regulator_disable(ts->reg);
>   err_put_regulator:
> -	regulator_put(ts->reg);
> - err_free_gpio:
> +	if (ts->reg)
> +		regulator_put(ts->reg);
>  	if (ts->gpio_pendown != -1)
>  		gpio_free(ts->gpio_pendown);
>   err_cleanup_filter:
> @@ -1251,8 +1253,10 @@ static int __devexit ads7846_remove(struct spi_device *spi)
>  	/* suspend left the IRQ disabled */
>  	enable_irq(ts->spi->irq);
>  
> -	regulator_disable(ts->reg);
> -	regulator_put(ts->reg);
> +	if (ts->reg) {
> +		regulator_disable(ts->reg);
> +		regulator_put(ts->reg);
> +	}
>  
>  	if (ts->gpio_pendown != -1)
>  		gpio_free(ts->gpio_pendown);

-- 
Regards,
Igor.

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Mark Brown]

On Tue, Sep 07, 2010 at 03:23:15PM +0300, Igor Grinberg wrote:
>  Seems that there are some more platforms then Sharp Zaurus
> are interested in this patch.
> 
> Dmitry, Mike, Pavel,
> 
> What's the status of it?

>From a regulator API usage point of view a separate implementation of
the same thing was nacked - there are regulator API facilties for hiding
missing regulators from drivers when needed to get systems going, unless
the device genuinely can cope without supplies it should be relying on
those.

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Marek Vasut]

Dne Út 7. září 2010 14:53:35 Mark Brown napsal(a):
> On Tue, Sep 07, 2010 at 03:23:15PM +0300, Igor Grinberg wrote:
> >  Seems that there are some more platforms then Sharp Zaurus
> > 
> > are interested in this patch.
> > 
> > Dmitry, Mike, Pavel,
> > 
> > What's the status of it?
> 
> From a regulator API usage point of view a separate implementation of
> the same thing was nacked - there are regulator API facilties for hiding
> missing regulators from drivers when needed to get systems going, unless
> the device genuinely can cope without supplies it should be relying on
> those.

Maybe these platforms should have been fixed prior to applying the patch adding 
regulator goo into ads7846 driver then. What's the way to go now then ?

Thanks, Cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Mark Brown]

On Thu, Sep 09, 2010 at 10:27:17AM +0200, Marek Vasut wrote:
> Dne Út 7. září 2010 14:53:35 Mark Brown napsal(a):

> > From a regulator API usage point of view a separate implementation of
> > the same thing was nacked - there are regulator API facilties for hiding
> > missing regulators from drivers when needed to get systems going, unless
> > the device genuinely can cope without supplies it should be relying on
> > those.

> Maybe these platforms should have been fixed prior to applying the patch adding 
> regulator goo into ads7846 driver then. What's the way to go now then ?

Fix the platforms and use the dummy regulators to keep them going until
that happens.  It's trivial to do the hookup in the platforms.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Marek Vasut]

Dne Čt 9. září 2010 11:41:20 Mark Brown napsal(a):
> On Thu, Sep 09, 2010 at 10:27:17AM +0200, Marek Vasut wrote:
> > Dne Út 7. září 2010 14:53:35 Mark Brown napsal(a):
> > > From a regulator API usage point of view a separate implementation of
> > > the same thing was nacked - there are regulator API facilties for
> > > hiding missing regulators from drivers when needed to get systems
> > > going, unless the device genuinely can cope without supplies it should
> > > be relying on those.
> > 
> > Maybe these platforms should have been fixed prior to applying the patch
> > adding regulator goo into ads7846 driver then. What's the way to go now
> > then ?
> 
> Fix the platforms and use the dummy regulators to keep them going until
> that happens.  It's trivial to do the hookup in the platforms.

Igor, any update on this?

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Igor Grinberg]

 On 09/09/10 12:41, Mark Brown wrote:
> On Thu, Sep 09, 2010 at 10:27:17AM +0200, Marek Vasut wrote:
>> Dne Út 7. září 2010 14:53:35 Mark Brown napsal(a):
>>> From a regulator API usage point of view a separate implementation of
>>> the same thing was nacked - there are regulator API facilties for hiding
>>> missing regulators from drivers when needed to get systems going, unless
>>> the device genuinely can cope without supplies it should be relying on
>>> those.

I actually, don't see why ads7846 is strictly relying on the regulator
and I don't understand, why ads7846 driver has to bail out if the regulator
is not found? Why shouldn't the driver try to continue?
I think it should bail out only in case communicating with the device failed.

>> Maybe these platforms should have been fixed prior to applying the patch adding 
>> regulator goo into ads7846 driver then. What's the way to go now then ?
> Fix the platforms and use the dummy regulators to keep them going until
> that happens.  It's trivial to do the hookup in the platforms.

You want each platform, that does not have a special regulated power supply
for the ads7846, to define a dummy regulator just to cope with that artificial
dependency of the device driver?
I think it is a waste and big code duplication in each platform
that does not have that special regulator.

-- 
Regards,
Igor.

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Marek Vasut]

Dne Út 5. října 2010 08:49:07 Igor Grinberg napsal(a):
>  On 09/09/10 12:41, Mark Brown wrote:
> > On Thu, Sep 09, 2010 at 10:27:17AM +0200, Marek Vasut wrote:
> >> Dne Út 7. září 2010 14:53:35 Mark Brown napsal(a):
> >>> From a regulator API usage point of view a separate implementation of
> >>> the same thing was nacked - there are regulator API facilties for
> >>> hiding missing regulators from drivers when needed to get systems
> >>> going, unless the device genuinely can cope without supplies it should
> >>> be relying on those.
> 
> I actually, don't see why ads7846 is strictly relying on the regulator
> and I don't understand, why ads7846 driver has to bail out if the regulator
> is not found? Why shouldn't the driver try to continue?
> I think it should bail out only in case communicating with the device
> failed.

Well, I can't but agree ... it's unnecessary crud indeed.
> 
> >> Maybe these platforms should have been fixed prior to applying the patch
> >> adding regulator goo into ads7846 driver then. What's the way to go now
> >> then ?
> > 
> > Fix the platforms and use the dummy regulators to keep them going until
> > that happens.  It's trivial to do the hookup in the platforms.
> 
> You want each platform, that does not have a special regulated power supply
> for the ads7846, to define a dummy regulator just to cope with that
> artificial dependency of the device driver?

You can enable some option in the regulator stuff that automagically shoves in a 
dummy regulator, but it's still additional crud.

> I think it is a waste and big code duplication in each platform
> that does not have that special regulator.

Indeed
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Dmitry Torokhov]

On Tue, Oct 05, 2010 at 08:49:07AM +0200, Igor Grinberg wrote:
>  On 09/09/10 12:41, Mark Brown wrote:
> > On Thu, Sep 09, 2010 at 10:27:17AM +0200, Marek Vasut wrote:
> >> Dne Út 7. září 2010 14:53:35 Mark Brown napsal(a):
> >>> From a regulator API usage point of view a separate implementation of
> >>> the same thing was nacked - there are regulator API facilties for hiding
> >>> missing regulators from drivers when needed to get systems going, unless
> >>> the device genuinely can cope without supplies it should be relying on
> >>> those.
> 
> I actually, don't see why ads7846 is strictly relying on the regulator
> and I don't understand, why ads7846 driver has to bail out if the regulator
> is not found? Why shouldn't the driver try to continue?
> I think it should bail out only in case communicating with the device failed.
> 
> >> Maybe these platforms should have been fixed prior to applying the patch adding 
> >> regulator goo into ads7846 driver then. What's the way to go now then ?
> > Fix the platforms and use the dummy regulators to keep them going until
> > that happens.  It's trivial to do the hookup in the platforms.
> 
> You want each platform, that does not have a special regulated power supply
> for the ads7846, to define a dummy regulator just to cope with that artificial
> dependency of the device driver?
> I think it is a waste and big code duplication in each platform
> that does not have that special regulator.
>

I tend to agree, however I think that original patch that simply ignored
failures from regulator_get() is not the best option either. Can we have
a flag in platform data indicating that the board does not employ a
regulator? Then we could retain the hard failure in cases when we expect
regulator to be present while allowing to continue on boards that do not
have it.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Mark Brown]

On Tue, Oct 05, 2010 at 09:16:08AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 05, 2010 at 08:49:07AM +0200, Igor Grinberg wrote:

> > You want each platform, that does not have a special regulated power supply
> > for the ads7846, to define a dummy regulator just to cope with that artificial
> > dependency of the device driver?
> > I think it is a waste and big code duplication in each platform
> > that does not have that special regulator.

It's a pretty good fit for most current systems - with current hardware
you will normally have some software control for the vast majority of
the regulators on the board if you have regulator control at all since
that's the way PMICs have gone.  Having a complete map of the regulator
usage in the system is useful since it allows us to do optimisations
like powering down idle regulators much more readily.

> I tend to agree, however I think that original patch that simply ignored
> failures from regulator_get() is not the best option either. Can we have
> a flag in platform data indicating that the board does not employ a
> regulator? Then we could retain the hard failure in cases when we expect
> regulator to be present while allowing to continue on boards that do not
> have it.

I really don't think it's a good idea to add this code to every single
regulator using driver - this seems like an enormous waste of time and
code complexity cost.  I have suggested several times that we should
extend the dummy regulator mode so that boards can enable it from code
as well as users enable it from Kconfig, I'm not sure why everyone is so
keen on bodging this in drivers.

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Dmitry Torokhov]

On Tue, Oct 05, 2010 at 09:40:38AM -0700, Mark Brown wrote:
> On Tue, Oct 05, 2010 at 09:16:08AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 05, 2010 at 08:49:07AM +0200, Igor Grinberg wrote:
> 
> > > You want each platform, that does not have a special regulated power supply
> > > for the ads7846, to define a dummy regulator just to cope with that artificial
> > > dependency of the device driver?
> > > I think it is a waste and big code duplication in each platform
> > > that does not have that special regulator.
> 
> It's a pretty good fit for most current systems - with current hardware
> you will normally have some software control for the vast majority of
> the regulators on the board if you have regulator control at all since
> that's the way PMICs have gone.  Having a complete map of the regulator
> usage in the system is useful since it allows us to do optimisations
> like powering down idle regulators much more readily.
> 
> > I tend to agree, however I think that original patch that simply ignored
> > failures from regulator_get() is not the best option either. Can we have
> > a flag in platform data indicating that the board does not employ a
> > regulator? Then we could retain the hard failure in cases when we expect
> > regulator to be present while allowing to continue on boards that do not
> > have it.
> 
> I really don't think it's a good idea to add this code to every single
> regulator using driver - this seems like an enormous waste of time and
> code complexity cost.  I have suggested several times that we should
> extend the dummy regulator mode so that boards can enable it from code
> as well as users enable it from Kconfig, I'm not sure why everyone is so
> keen on bodging this in drivers.

It all depends on what instances you expect to encounted more often -
drivers or boards without regulators...

-- 
Dmitry

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Mark Brown]

On Tue, Oct 05, 2010 at 11:07:03AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 05, 2010 at 09:40:38AM -0700, Mark Brown wrote:

> > I really don't think it's a good idea to add this code to every single
> > regulator using driver - this seems like an enormous waste of time and
> > code complexity cost.  I have suggested several times that we should
> > extend the dummy regulator mode so that boards can enable it from code
> > as well as users enable it from Kconfig, I'm not sure why everyone is so
> > keen on bodging this in drivers.

> It all depends on what instances you expect to encounted more often -
> drivers or boards without regulators...

If the board doesn't use regulators you can just disable the regulator
API at which point it compiles out into stubs which report success -
this has been the case from day one.  There's only an issue if the board
has a regulator configuration which is partially visible to software.

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Alan Cox]

> If the board doesn't use regulators you can just disable the regulator
> API at which point it compiles out into stubs which report success -
> this has been the case from day one.  There's only an issue if the board
> has a regulator configuration which is partially visible to software.

Which is quite likely on anything complex and means that hardcoding
regulator assumptions is bad.

I actually see two ways of attacking that, one is that the dummy
regulator *and* the compiled in regulator system have a standard
regulator value that can be passed which means "report success, move
along nothing to see" that could be passed into drivers, the other is to
not stick the stuff in drivers.

I suspect the former may be cleaner ?

Alan

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Mark Brown]

On Tue, Oct 05, 2010 at 08:35:08PM +0100, Alan Cox wrote:

> > If the board doesn't use regulators you can just disable the regulator
> > API at which point it compiles out into stubs which report success -
> > this has been the case from day one.  There's only an issue if the board
> > has a regulator configuration which is partially visible to software.

> Which is quite likely on anything complex and means that hardcoding
> regulator assumptions is bad.

When I say "partially visible" I mean where we can't say what's there -
we can cope fine with stuff that isn't software controllable.  The issue
is that we can't automatically discover the system configuration.

> I actually see two ways of attacking that, one is that the dummy
> regulator *and* the compiled in regulator system have a standard
> regulator value that can be passed which means "report success, move
> along nothing to see" that could be passed into drivers, the other is to
> not stick the stuff in drivers.

> I suspect the former may be cleaner ?

I'm afraid I'm having a hard time parsing what you're saying here (what
would a "regulator value" be?), but I suspect that you're trying to
suggest something fairly close to what is already implemented.

The model the regulator API has is that neither the driver for the
regulator nor the driver for the consumer should know anything about the
particular board, with the consumers just requesting the supplies the
device has.  A separate piece of board-specific configuration code
provides a table mapping the physical regulators in the system to the
supplies on the individual chips.  There is an option, currently Kconfig
only, which causes the regulator API to provide a do-nothing dummy
regulator if an attempt is made to request a supply that doesn't exist.
I *think* this dummy regulator corresponds to what you're suggesting
above but like I say I'm having a hard time parsing it.

If the regulator API is compiled out then any attempt to get, enable or
disable a regulator just unconditionally reports success.

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Linus Walleij]

2010/10/5 Alan Cox <alan@lxorguk.ukuu.org.uk>:

> I actually see two ways of attacking that, one is that the dummy
> regulator *and* the compiled in regulator system have a standard
> regulator value that can be passed which means "report success, move
> along nothing to see" that could be passed into drivers,

Something like this patch series? If people like it I'll test it a bit
more and submit this 2-patch series.


From: Linus Walleij <linus.walleij@stericsson.com>
Date: Tue, 5 Oct 2010 23:23:47 +0200
Subject: [PATCH 1/2] regulator: add a relaxed regulor_try_get()

This wrapper function will accept non-existing regulators and
not treat them as errors. Consumers will have to NULL-check their
regulators.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 include/linux/regulator/consumer.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/linux/regulator/consumer.h
b/include/linux/regulator/consumer.h
index ebd7472..6ee7fdc 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -36,6 +36,7 @@
 #define __LINUX_REGULATOR_CONSUMER_H_

 #include <linux/device.h>
+#include <linux/err.h>

 /*
  * Regulator operating modes.
@@ -297,4 +298,23 @@ static inline void regulator_set_drvdata(struct
regulator *regulator,

 #endif

+/*
+ * Try to get a regulator, if it fails, no big deal.
+ * This wrapper function is intended to be used for code
+ * where regulator control is optional for the particular
+ * consumer, but still the regulator framework may be in
+ * use for other things in the platform.
+ */
+static inline struct regulator *regulator_try_get(struct device *dev,
+	const char *id)
+{
+	struct regulator *reg = regulator_get(dev, id);
+
+	/* It's just that this regulator is not (yet) defined */
+	if (IS_ERR(reg) && PTR_ERR(reg) == -ENODEV)
+		return NULL;
+
+	return reg;
+}
+
 #endif
-- 
1.7.2.3

From: Linus Walleij <linus.walleij@stericsson.com>
Date: Wed, 6 Oct 2010 00:04:25 +0200
Subject: [PATCH 2/2] regulator: handle NULL regulators in core

If the user has selected to use regulator_try_get() to fetch
regulators, we bail out of consumer operations if the regulator
happens to be NULL.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/regulator/core.c |   64 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cc8b337..989e4e4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1337,9 +1337,12 @@ static int _regulator_enable(struct regulator_dev *rdev)
  */
 int regulator_enable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret = 0;

+	if (regulator == NULL)
+		return 0;
+	rdev = regulator->rdev;
 	mutex_lock(&rdev->mutex);
 	ret = _regulator_enable(rdev);
 	mutex_unlock(&rdev->mutex);
@@ -1406,9 +1409,12 @@ static int _regulator_disable(struct regulator_dev *rdev)
  */
 int regulator_disable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret = 0;

+	if (regulator == NULL)
+		return 0;
+	rdev = regulator->rdev;
 	mutex_lock(&rdev->mutex);
 	ret = _regulator_disable(rdev);
 	mutex_unlock(&rdev->mutex);
@@ -1456,6 +1462,8 @@ int regulator_force_disable(struct regulator *regulator)
 {
 	int ret;

+	if (regulator == NULL)
+		return 0;
 	mutex_lock(&regulator->rdev->mutex);
 	regulator->uA_load = 0;
 	ret = _regulator_force_disable(regulator->rdev);
@@ -1489,6 +1497,8 @@ int regulator_is_enabled(struct regulator *regulator)
 {
 	int ret;

+	if (regulator == NULL)
+		return 1;
 	mutex_lock(&regulator->rdev->mutex);
 	ret = _regulator_is_enabled(regulator->rdev);
 	mutex_unlock(&regulator->rdev->mutex);
@@ -1507,8 +1517,11 @@ EXPORT_SYMBOL_GPL(regulator_is_enabled);
  */
 int regulator_count_voltages(struct regulator *regulator)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
+	struct regulator_dev	*rdev;

+	if (regulator == NULL)
+		return 0;
+	rdev = regulator->rdev;
 	return rdev->desc->n_voltages ? : -EINVAL;
 }
 EXPORT_SYMBOL_GPL(regulator_count_voltages);
@@ -1525,10 +1538,15 @@ EXPORT_SYMBOL_GPL(regulator_count_voltages);
  */
 int regulator_list_voltage(struct regulator *regulator, unsigned selector)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
-	struct regulator_ops	*ops = rdev->desc->ops;
+	struct regulator_dev	*rdev;
+	struct regulator_ops	*ops;
 	int			ret;

+	if (regulator == NULL)
+		return 0;
+	rdev = regulator->rdev;
+	ops = rdev->desc->ops;
+
 	if (!ops->list_voltage || selector >= rdev->desc->n_voltages)
 		return -EINVAL;

@@ -1561,6 +1579,8 @@ int regulator_is_supported_voltage(struct
regulator *regulator,
 {
 	int i, voltages, ret;

+	if (regulator == NULL)
+		return 0;
 	ret = regulator_count_voltages(regulator);
 	if (ret < 0)
 		return ret;
@@ -1596,9 +1616,12 @@ int regulator_is_supported_voltage(struct
regulator *regulator,
  */
 int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret;

+	if (regulator == NULL)
+		return 0;
+	rdev = regulator->rdev;
 	mutex_lock(&rdev->mutex);

 	/* sanity check */
@@ -1644,6 +1667,9 @@ int regulator_get_voltage(struct regulator *regulator)
 {
 	int ret;

+	if (regulator == NULL)
+		return 0;
+
 	mutex_lock(&regulator->rdev->mutex);

 	ret = _regulator_get_voltage(regulator->rdev);
@@ -1673,9 +1699,13 @@ EXPORT_SYMBOL_GPL(regulator_get_voltage);
 int regulator_set_current_limit(struct regulator *regulator,
 			       int min_uA, int max_uA)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret;

+	if (regulator == NULL)
+		return 0;
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);

 	/* sanity check */
@@ -1725,6 +1755,8 @@ out:
  */
 int regulator_get_current_limit(struct regulator *regulator)
 {
+	if (regulator == NULL)
+		return 0;
 	return _regulator_get_current_limit(regulator->rdev);
 }
 EXPORT_SYMBOL_GPL(regulator_get_current_limit);
@@ -1742,10 +1774,14 @@ EXPORT_SYMBOL_GPL(regulator_get_current_limit);
  */
 int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret;
 	int regulator_curr_mode;

+	if (regulator == NULL)
+		return 0;
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);

 	/* sanity check */
@@ -1801,6 +1837,8 @@ out:
  */
 unsigned int regulator_get_mode(struct regulator *regulator)
 {
+	if (regulator == NULL)
+		return REGULATOR_MODE_NORMAL;
 	return _regulator_get_mode(regulator->rdev);
 }
 EXPORT_SYMBOL_GPL(regulator_get_mode);
@@ -1833,11 +1871,15 @@ EXPORT_SYMBOL_GPL(regulator_get_mode);
  */
 int regulator_set_optimum_mode(struct regulator *regulator, int uA_load)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	struct regulator *consumer;
 	int ret, output_uV, input_uV, total_uA_load = 0;
 	unsigned int mode;

+	if (regulator == NULL)
+		return REGULATOR_MODE_NORMAL;
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);

 	regulator->uA_load = uA_load;
@@ -1907,6 +1949,8 @@ EXPORT_SYMBOL_GPL(regulator_set_optimum_mode);
 int regulator_register_notifier(struct regulator *regulator,
 			      struct notifier_block *nb)
 {
+	if (regulator == NULL)
+		return 0;
 	return blocking_notifier_chain_register(&regulator->rdev->notifier,
 						nb);
 }
@@ -1922,6 +1966,8 @@ EXPORT_SYMBOL_GPL(regulator_register_notifier);
 int regulator_unregister_notifier(struct regulator *regulator,
 				struct notifier_block *nb)
 {
+	if (regulator == NULL)
+		return 0;
 	return blocking_notifier_chain_unregister(&regulator->rdev->notifier,
 						  nb);
 }
-- 
1.7.2.3

Yours,
Linus Walleij

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Mark Brown]

On Wed, Oct 06, 2010 at 12:09:39AM +0200, Linus Walleij wrote:

> This wrapper function will accept non-existing regulators and
> not treat them as errors. Consumers will have to NULL-check their
> regulators.

No, this is actively unhelpful.  If consumers can null check their
regulators then they can just as well do an IS_ERR() check and if we
just eat null pointers like this then the API doesn't degrade gracefully
when it can't implement things - for example, your regulator_set_voltage() 
will report success when it fails to set a voltage which is going to
upset any consumer that thinks it did actually change the current
voltage.

The existing dummy regulator support already does all this in a manner
which is much less invasive for the core.

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Linus Walleij]

2010/10/6 Mark Brown <broonie@opensource.wolfsonmicro.com>:

> On Wed, Oct 06, 2010 at 12:09:39AM +0200, Linus Walleij wrote:
>
>> This wrapper function will accept non-existing regulators and
>> not treat them as errors. Consumers will have to NULL-check their
>> regulators.
>
> No, this is actively unhelpful.  If consumers can null check their
> regulators then they can just as well do an IS_ERR() check

.. and wrap all enable/disable/ etc into conditional statements
that depend on whether we could locate a regulator or not.

And yes, this is what I'm already doing in drivers like
drivers/mmc/host/mmci.c, where I check for the regulator
holder in the mmci state struct to be NULL (another way
would be to have a boolean .has_regulator but you get the
idea).

I think what Alan requested is that it be made more seamless.
The patches try to make regulator support less hairy in these
cases, a non-existing regulator will be a NULL pointer (no error
and the runtime functions will act as the compile-time stubs.

> The existing dummy regulator support already does all this in a manner
> which is much less invasive for the core.

Correct me if I'm wrong but the dummy regulators are something
different which appear when you choose to compile out regulator
support altogether.

This was not what Alan was asking about I believe, the usecase
to addressed is partial regulator support, where regulator framework
*is* compiled in, but a driver *may* not get a regulator from the
framework.

The patch makes the runtime functions with partial regulator
support for a NULL regulator behave exactly like the
compile-time stubs.

If I read your answer right, your stance is that the regulator
framework shall not help out in situations like this, instead
the driver shall recognice -ENODEV from regulator_get() and
avoid doing any regulator calls after that. Is this correct?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Input: Make ADS7846 independent on regulator

[Mark Brown]

On Wed, Oct 06, 2010 at 10:01:10AM +0200, Linus Walleij wrote:
> 2010/10/6 Mark Brown <broonie@opensource.wolfsonmicro.com>:

> > No, this is actively unhelpful.  If consumers can null check their
> > regulators then they can just as well do an IS_ERR() check

> .. and wrap all enable/disable/ etc into conditional statements
> that depend on whether we could locate a regulator or not.

This is a totally different use case to that being discussed in this
thread - the use case being discussed in this thread is for things like
core power supplies which are critical to the operation of the device.
The expected error handling if these supplies are missing would be to
abort.

For users that can reasonably do optional supplies I'd expect that there
would be other code which also needs to be conditional which relies on
some action having being taken on the regulator and would also need to
be bypassed so it'd be a bit surprising if things didn't get caught up
in a bigger conditional block.

> I think what Alan requested is that it be made more seamless.

I'm fairly sure he's talking about the use case above, though I'm not
100% sure how familiar with the API Alan is.  Note also that the way
you've done this with things returning success for nonexistant supplies
means that things are less seamless than they might be.

> > The existing dummy regulator support already does all this in a manner
> > which is much less invasive for the core.

> Correct me if I'm wrong but the dummy regulators are something
> different which appear when you choose to compile out regulator
> support altogether.

No, they're a feature of the compiled in regulator API (which happen to
behave in the same manner as the stubs you get if the API is disbled).

> This was not what Alan was asking about I believe, the usecase
> to addressed is partial regulator support, where regulator framework
> *is* compiled in, but a driver *may* not get a regulator from the
> framework.

He's looking for something that can be used for supplies that are
critical to device operation on random devices on the board, something
that says that all unconfigured supplies are actually provided by
non-specific fixed voltage regulators.  This is different from the case
where some supplies may be missing because they really are not present
(in that the supplies are there, we've just not told the system how
they're provided).  Alan and Marek need boards to be able to say that
every device on the board has a supply even if it's not explicitly
configured while you want to be able to more easily ignore failures with
specific supplies.

> If I read your answer right, your stance is that the regulator
> framework shall not help out in situations like this, instead
> the driver shall recognice -ENODEV from regulator_get() and
> avoid doing any regulator calls after that. Is this correct?

Pretty much for your use case, though if enough things like MMC have
cases where supplies truly can just be omitted with no effect on the
operation of the system at all then I guess a consumer initiated way to
request a dummy could be in order.  I'd worry that people will just end
up randomly using it for all supplies, though.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html