Re: [PATCH] Input: ads7846: add regulator support
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 majord...@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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
On Thu, Feb 4, 2010 at 6:21 PM, 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 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 majord...@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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
On 4 Feb 2010, at 18:08, Dmitry Torokhov wrote: On Thu, Feb 04, 2010 at 04:21:55PM +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
On Thu, Feb 04, 2010 at 04:21:55PM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
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 majord...@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-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
On Thu, Feb 4, 2010 at 4:24 PM, 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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: ads7846: add regulator support
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
