[PATCH] Input: ads7846: add regulator support

[PATCH] Input: ads7846: add regulator support

[Grazvydas Ignotas]

The ADS7846/TSC2046 touchscreen controllers can (and usually are)
connected to various regulators for power, so add regulator support.
Make it optional for now to avoid breaking all current users of this
driver.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 drivers/input/touchscreen/ads7846.c |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 52d2ca1..5da902a 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -27,6 +27,7 @@
 #include <linux/gpio.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/ads7846.h>
+#include <linux/regulator/consumer.h>
 #include <asm/irq.h>
 
 /*
@@ -85,6 +86,7 @@ struct ads7846 {
 	char			name[32];
 
 	struct spi_device	*spi;
+	struct regulator	*reg;
 
 #if defined(CONFIG_HWMON) || defined(CONFIG_HWMON_MODULE)
 	struct attribute_group	*attr_group;
@@ -788,6 +790,9 @@ static void ads7846_disable(struct ads7846 *ts)
 		}
 	}
 
+	if (!IS_ERR(ts->reg))
+		regulator_disable(ts->reg);
+
 	/* we know the chip's in lowpower mode since we always
 	 * leave it that way after every request
 	 */
@@ -799,6 +804,9 @@ static void ads7846_enable(struct ads7846 *ts)
 	if (!ts->disabled)
 		return;
 
+	if (!IS_ERR(ts->reg))
+		regulator_enable(ts->reg);
+
 	ts->disabled = 0;
 	ts->irq_disabled = 0;
 	enable_irq(ts->spi->irq);
@@ -1139,6 +1147,13 @@ 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 = regulator_enable(ts->reg);
+		if (err)
+			goto err_put_regulator;
+	}
+
 	if (request_irq(spi->irq, ads7846_irq, IRQF_TRIGGER_FALLING,
 			spi->dev.driver->name, ts)) {
 		dev_info(&spi->dev,
@@ -1148,7 +1163,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 				  spi->dev.driver->name, ts);
 		if (err) {
 			dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq);
-			goto err_free_gpio;
+			goto err_disable_regulator;
 		}
 	}
 
@@ -1180,7 +1195,12 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 	ads784x_hwmon_unregister(spi, ts);
  err_free_irq:
 	free_irq(spi->irq, ts);
- err_free_gpio:
+ err_disable_regulator:
+	if (!IS_ERR(ts->reg))
+		regulator_disable(ts->reg);
+ err_put_regulator:
+	if (!IS_ERR(ts->reg))
+		regulator_put(ts->reg);
 	if (ts->gpio_pendown != -1)
 		gpio_free(ts->gpio_pendown);
  err_cleanup_filter:
@@ -1208,6 +1228,11 @@ static int __devexit ads7846_remove(struct spi_device *spi)
 	/* suspend left the IRQ disabled */
 	enable_irq(ts->spi->irq);
 
+	if (!IS_ERR(ts->reg)) {
+		regulator_disable(ts->reg);
+		regulator_put(ts->reg);
+	}
+
 	if (ts->gpio_pendown != -1)
 		gpio_free(ts->gpio_pendown);
 
-- 
1.6.3.3

Re: [PATCH] Input: ads7846: add regulator support

[Mark Brown]

On Thu, Feb 04, 2010 at 03:39:18PM +0200, Grazvydas Ignotas wrote:

> The ADS7846/TSC2046 touchscreen controllers can (and usually are)
> connected to various regulators for power, so add regulator support.
> Make it optional for now to avoid breaking all current users of this
> driver.

This should not be required.  The regulator API stubs itself out when it
is not built so all API calls report as successful.

> +	ts->reg = regulator_get(&spi->dev, "vcc");
> +	if (!IS_ERR(ts->reg)) {
> +		err = regulator_enable(ts->reg);
> +		if (err)
> +			goto err_put_regulator;
> +	}

If the regulator API is not compiled in then the regulator_get() will
return succesfully.  If the regulator API is in use then failure to
acquire the regulator is a serious problem which really should be at a
minimium be being communicated to the user.  For example, the regulator
API may end up powering down regulators which it believes are unused or
the regulator may not be powered by default and needs to be enabled.

The updates to fix up the boards that need this are fairly
straightforward and given that it's fairly easy to identify systems
which are using the driver in mainline so I'd really prefer not to go
down the route of trying to carry on in the face of error, it papers
over stuff now but is not robust in the face of future changes.

Re: [PATCH] Input: ads7846: add regulator support

[Grazvydas Ignotas]

On Thu, Feb 4, 2010 at 4:24 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Feb 04, 2010 at 03:39:18PM +0200, Grazvydas Ignotas wrote:
>
>> The ADS7846/TSC2046 touchscreen controllers can (and usually are)
>> connected to various regulators for power, so add regulator support.
>> Make it optional for now to avoid breaking all current users of this
>> driver.
>
> This should not be required.  The regulator API stubs itself out when it
> is not built so all API calls report as successful.

ok

>
>> +     ts->reg = regulator_get(&spi->dev, "vcc");
>> +     if (!IS_ERR(ts->reg)) {
>> +             err = regulator_enable(ts->reg);
>> +             if (err)
>> +                     goto err_put_regulator;
>> +     }
>
> If the regulator API is not compiled in then the regulator_get() will
> return succesfully.  If the regulator API is in use then failure to
> acquire the regulator is a serious problem which really should be at a
> minimium be being communicated to the user.  For example, the regulator
> API may end up powering down regulators which it believes are unused or
> the regulator may not be powered by default and needs to be enabled.
>
> The updates to fix up the boards that need this are fairly
> straightforward and given that it's fairly easy to identify systems
> which are using the driver in mainline so I'd really prefer not to go
> down the route of trying to carry on in the face of error, it papers
> over stuff now but is not robust in the face of future changes.

What about warning and continuing only on ENODEV then? I really don't
want to be responsible for potentially breaking touchscreen on ~30
boards, where I have no idea about how things are wired up and how the
regulators should be set up.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: ads7846: add regulator support

[Mike Rapoport]

Mark Brown wrote:
> On Thu, Feb 04, 2010 at 03:39:18PM +0200, Grazvydas Ignotas wrote:
> 
>> The ADS7846/TSC2046 touchscreen controllers can (and usually are)
>> connected to various regulators for power, so add regulator support.
>> Make it optional for now to avoid breaking all current users of this
>> driver.
> 
> This should not be required.  The regulator API stubs itself out when it
> is not built so all API calls report as successful.

And what about boards that have the ads7846 tied to power rail and still
want to use regulator API for other staff?

>> +	ts->reg = regulator_get(&spi->dev, "vcc");
>> +	if (!IS_ERR(ts->reg)) {
>> +		err = regulator_enable(ts->reg);
>> +		if (err)
>> +			goto err_put_regulator;
>> +	}
> 
> If the regulator API is not compiled in then the regulator_get() will
> return succesfully.  If the regulator API is in use then failure to
> acquire the regulator is a serious problem which really should be at a
> minimium be being communicated to the user.  For example, the regulator
> API may end up powering down regulators which it believes are unused or
> the regulator may not be powered by default and needs to be enabled.
> 
> The updates to fix up the boards that need this are fairly
> straightforward and given that it's fairly easy to identify systems
> which are using the driver in mainline so I'd really prefer not to go
> down the route of trying to carry on in the face of error, it papers
> over stuff now but is not robust in the face of future changes.

The updates are straightforward for boards that actually _have_
regulator powering the touchscreen controller. What about the boards
that do not have such a regulator?

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


-- 
Sincerely yours,
Mike.

Re: [PATCH] Input: ads7846: add regulator support

[Mark Brown]

On Thu, Feb 04, 2010 at 05:08:11PM +0200, Mike Rapoport wrote:
> Mark Brown wrote:

> > This should not be required.  The regulator API stubs itself out when it
> > is not built so all API calls report as successful.

> And what about boards that have the ads7846 tied to power rail and still
> want to use regulator API for other staff?

That's one of the use cases the fixed voltage regulator is there for
(it's the one that originally caused me to write the driver) - just
define one or more fixed voltage regulators representing the fixed power
rails and hook up the device to that/those.

Re: [PATCH] Input: ads7846: add regulator support

[Mark Brown]

On Thu, Feb 04, 2010 at 04:52:26PM +0200, Grazvydas Ignotas wrote:
> On Thu, Feb 4, 2010 at 4:24 PM, Mark Brown

> > The updates to fix up the boards that need this are fairly
> > straightforward and given that it's fairly easy to identify systems
> > which are using the driver in mainline so I'd really prefer not to go
> > down the route of trying to carry on in the face of error, it papers
> > over stuff now but is not robust in the face of future changes.

> What about warning and continuing only on ENODEV then? I really don't
> want to be responsible for potentially breaking touchscreen on ~30
> boards, where I have no idea about how things are wired up and how the
> regulators should be set up.

I'm really not terribly enthusiastic about that approach - like I say,
it just moves the problem about a bit and postpones the breakage in a
potentially more obscure fashion.

I'm also thinking that if we were going to go down that route then
handling it in the regulator core rather than in each individual driver
is going to be a much more sensible approach.  Having to conditionalise
each and every regulator API call in every single driver to handle this
feels like the wrong approach, it'd spread noise through the kernel as a
whole and obscure what's going on in cases where there are problems.

The bodge I'm thinking of would do something like log an error and
substitute in a dummy regulator when regulator_get() would have failed
so that the driver sees behaviour equivalent to the stubbed regulator
API if the bodge is active.  A central thing seems much more sensible
here - there's nothing specific to this driver going on here and having
the API behave in a consistent manner seems good.

Re: [PATCH] Input: ads7846: add regulator support

[Dmitry Torokhov]

On Thu, Feb 04, 2010 at 04:21:55PM +0000, Mark Brown wrote:
> On Thu, Feb 04, 2010 at 04:52:26PM +0200, Grazvydas Ignotas wrote:
> > On Thu, Feb 4, 2010 at 4:24 PM, Mark Brown
> 
> > > The updates to fix up the boards that need this are fairly
> > > straightforward and given that it's fairly easy to identify systems
> > > which are using the driver in mainline so I'd really prefer not to go
> > > down the route of trying to carry on in the face of error, it papers
> > > over stuff now but is not robust in the face of future changes.
> 
> > What about warning and continuing only on ENODEV then? I really don't
> > want to be responsible for potentially breaking touchscreen on ~30
> > boards, where I have no idea about how things are wired up and how the
> > regulators should be set up.
> 
> I'm really not terribly enthusiastic about that approach - like I say,
> it just moves the problem about a bit and postpones the breakage in a
> potentially more obscure fashion.
> 

Maybe we should rely on platform data to tell us whether we need to plug
into regulator framework and in this case fail hard on any error?

-- 
Dmitry

Re: [PATCH] Input: ads7846: add regulator support

[Mark Brown]

On 4 Feb 2010, at 18:08, Dmitry Torokhov <dmitry.torokhov@gmail.com>  
wrote:

> On Thu, Feb 04, 2010 at 04:21:55PM +0000, Mark Brown wrote:
>> On Thu, Feb 04, 2010 at 04:52:26PM +0200, Grazvydas Ignotas wrote:
>>> On Thu, Feb 4, 2010 at 4:24 PM, Mark Brown
>>
>>>> The updates to fix up the boards that need this are fairly
>>>> straightforward and given that it's fairly easy to identify systems
>>>> which are using the driver in mainline so I'd really prefer not  
>>>> to go
>>>> down the route of trying to carry on in the face of error, it  
>>>> papers
>>>> over stuff now but is not robust in the face of future changes.
>>
>>> What about warning and continuing only on ENODEV then? I really  
>>> don't
>>> want to be responsible for potentially breaking touchscreen on ~30
>>> boards, where I have no idea about how things are wired up and how  
>>> the
>>> regulators should be set up.
>>
>> I'm really not terribly enthusiastic about that approach - like I  
>> say,
>> it just moves the problem about a bit and postpones the breakage in a
>> potentially more obscure fashion.
>>
>
> Maybe we should rely on platform data to tell us whether we need to  
> plug
> into regulator framework and in this case fail hard on any error?

That doesn't really help - it requres per driver code for the platform  
data and for conditionalising all the regulator API calls, results in  
inconsistent behaviour between drivers (does this driver have a no  
regulator switch?) and is looking a bit like the situation where  
you're passing around the regulator name to match device and supply.  
It seems like more work overall for users. 
  

Re: [PATCH] Input: ads7846: add regulator support

[Mike Rapoport]

On Thu, Feb 4, 2010 at 6:21 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Feb 04, 2010 at 04:52:26PM +0200, Grazvydas Ignotas wrote:
>> On Thu, Feb 4, 2010 at 4:24 PM, Mark Brown
>
> The bodge I'm thinking of would do something like log an error and
> substitute in a dummy regulator when regulator_get() would have failed
> so that the driver sees behaviour equivalent to the stubbed regulator
> API if the bodge is active.  A central thing seems much more sensible
> here - there's nothing specific to this driver going on here and having
> the API behave in a consistent manner seems good.

I agree that such approach have more sense than checking for -ENODEV
in each and every driver that uses the regulator framework. I just
wonder, if there should be some mechanism that  can switch the
substitution of the dummy regulators on and off. And if yes, how
should the platform code communicate with the regulator core the need
for such dummy regulators...

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



-- 
	Sincerely Yours,
		Mike.
--
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] Input: ads7846: add regulator support

[Mark Brown]

On Fri, Feb 05, 2010 at 10:45:09PM +0200, Mike Rapoport wrote:
> On Thu, Feb 4, 2010 at 6:21 PM, Mark Brown

> > The bodge I'm thinking of would do something like log an error and
> > substitute in a dummy regulator when regulator_get() would have failed
> > so that the driver sees behaviour equivalent to the stubbed regulator
> > API if the bodge is active.  A central thing seems much more sensible
> > here - there's nothing specific to this driver going on here and having
> > the API behave in a consistent manner seems good.

> I agree that such approach have more sense than checking for -ENODEV
> in each and every driver that uses the regulator framework. I just
> wonder, if there should be some mechanism that  can switch the
> substitution of the dummy regulators on and off. And if yes, how
> should the platform code communicate with the regulator core the need
> for such dummy regulators...

So, having thought about this a bit more we actually have two different
use cases here.  One is where you've got a system which has software
controllable regulators for everything but may not have plumbed in all
the supplies, the other is for systems where only a very few supplies
are on software controlled regulators which are just trying to save the
hassle of hooking up the bulk of the supplies to fixed voltage
regulators.  These two use cases should probably be handled differently
- the first one is really expected to have all the supplies hooked up
and so should warn when using the bodge regulator but the warning isn't
helpful in the second case.

We already have some support for boards to set up the API in the form of
regulator_set_full_constraints() so we could do something similar for
dummy regulators, or create a new single API to set a bunch of options
via a struct which is probably less hassle going forward.
--
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] Input: ads7846: add regulator support

[Mike Rapoport]

Mark Brown wrote:
> On Fri, Feb 05, 2010 at 10:45:09PM +0200, Mike Rapoport wrote:
>> On Thu, Feb 4, 2010 at 6:21 PM, Mark Brown
> 
>>> The bodge I'm thinking of would do something like log an error and
>>> substitute in a dummy regulator when regulator_get() would have failed
>>> so that the driver sees behaviour equivalent to the stubbed regulator
>>> API if the bodge is active.  A central thing seems much more sensible
>>> here - there's nothing specific to this driver going on here and having
>>> the API behave in a consistent manner seems good.
> 
>> I agree that such approach have more sense than checking for -ENODEV
>> in each and every driver that uses the regulator framework. I just
>> wonder, if there should be some mechanism that  can switch the
>> substitution of the dummy regulators on and off. And if yes, how
>> should the platform code communicate with the regulator core the need
>> for such dummy regulators...
> 
> So, having thought about this a bit more we actually have two different
> use cases here.  One is where you've got a system which has software
> controllable regulators for everything but may not have plumbed in all
> the supplies, the other is for systems where only a very few supplies
> are on software controlled regulators which are just trying to save the
> hassle of hooking up the bulk of the supplies to fixed voltage
> regulators.  These two use cases should probably be handled differently
> - the first one is really expected to have all the supplies hooked up
> and so should warn when using the bodge regulator but the warning isn't
> helpful in the second case.

Sounds right to me.

> We already have some support for boards to set up the API in the form of
> regulator_set_full_constraints() so we could do something similar for
> dummy regulators, or create a new single API to set a bunch of options
> via a struct which is probably less hassle going forward.

Struct sounds more reasonable that just a call to e.g.
regulator_warn_dummy_fixed_regulator :)

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


-- 
Sincerely yours,
Mike.