Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/11/2013 02:52 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Sep 10, 2013 at 12:37:47PM -0600, Stephen Warren wrote:
>
>> OK, so for the DT binding we should make vcc-supply a required
>> property, yet the driver will still work OK if that property just
>> happens to be missing (or e.g. when instantiated from a board file,
>> and there's no regulator).
>
> Yup. That way we've got both the binding and code trying to make things
> work, hopefully that'll maximise robustness.
Ok, it looks like regulator_get will handle all things, looking forward
to your patches :)
Then I think my changes will be simple, just something like:
+ reg = devm_regulator_get(dev, "vcc");
+ if (!IS_ERR(reg)) {
+ err = regulator_enable(reg);
+ if (err < 0)
+ return err;
+ } else {
+ return PTR_ERR(reg);
+ }
Wei.
>
> * Unknown Key
> * 0x7EA229BD
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 08:11 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Tue, Sep 10, 2013 at 07:29:40PM +0800, Wei Ni wrote: > >> On my platform, it use palmas-regulator.c, ldo6 for this lm90 power >> rail. I checked this driver, it will handle ramp_delay except LDOx. >> Since I'm not familiar with this palmas device and driver, so do you >> mean I can set regulator-ramp-delay property in the DT for ldo6, but I >> really don't know what value should I set :( > > Laxman just submitted a patch adding the ramp delay information which I > applied. It seems like this is just a hack for your board and can > therefore be removed? Oh, yes, it seems the patches is Laxman's [PATCH 1/2] regulator: palmas: configure enable time for LDOs. Thanks, I will check it, and remove the delay in my next patch. Wei. > > * Unknown Key > * 0x7EA229BD > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 08:11 PM, Mark Brown wrote: * PGP Signed by an unknown key On Tue, Sep 10, 2013 at 07:29:40PM +0800, Wei Ni wrote: On my platform, it use palmas-regulator.c, ldo6 for this lm90 power rail. I checked this driver, it will handle ramp_delay except LDOx. Since I'm not familiar with this palmas device and driver, so do you mean I can set regulator-ramp-delay property in the DT for ldo6, but I really don't know what value should I set :( Laxman just submitted a patch adding the ramp delay information which I applied. It seems like this is just a hack for your board and can therefore be removed? Oh, yes, it seems the patches is Laxman's [PATCH 1/2] regulator: palmas: configure enable time for LDOs. Thanks, I will check it, and remove the delay in my next patch. Wei. * Unknown Key * 0x7EA229BD -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/11/2013 02:52 AM, Mark Brown wrote:
* PGP Signed by an unknown key
On Tue, Sep 10, 2013 at 12:37:47PM -0600, Stephen Warren wrote:
OK, so for the DT binding we should make vcc-supply a required
property, yet the driver will still work OK if that property just
happens to be missing (or e.g. when instantiated from a board file,
and there's no regulator).
Yup. That way we've got both the binding and code trying to make things
work, hopefully that'll maximise robustness.
Ok, it looks like regulator_get will handle all things, looking forward
to your patches :)
Then I think my changes will be simple, just something like:
+ reg = devm_regulator_get(dev, vcc);
+ if (!IS_ERR(reg)) {
+ err = regulator_enable(reg);
+ if (err 0)
+ return err;
+ } else {
+ return PTR_ERR(reg);
+ }
Wei.
* Unknown Key
* 0x7EA229BD
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 12:37:47PM -0600, Stephen Warren wrote: > OK, so for the DT binding we should make vcc-supply a required > property, yet the driver will still work OK if that property just > happens to be missing (or e.g. when instantiated from a board file, > and there's no regulator). Yup. That way we've got both the binding and code trying to make things work, hopefully that'll maximise robustness. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 12:18 PM, Mark Brown wrote: > On Tue, Sep 10, 2013 at 11:44:05AM -0600, Stephen Warren wrote: > >> OK, so I believe you're saying that the case of a chip with just >> a single power source, which absolutely must be present in HW for >> the chip to be powered, isn't appropriate for >> regulator_get_optional(). Something must always define a >> regulator for that power source, even if there is no external SW >> control over that power source. > > Well, it really should be mandatory - personally I don't think > it's sensible to add off-SoC chips without defining their > regulators, it's more trouble than it's worth to have to add them > later for all the time it takes to define the bindings. In IETF > terms it's a should. > >> We either allow the regulator to be optional (since SW control >> over the regulator is optional), or go back to every board file >> and DT and add a dummy regulator in (which then breaks DT ABI, >> and even ignoring that is a pain). > > The whole point of the way I'm changing the dummy support is to > allow us to gracefully cope with errors here so there's no > mandatory update even though strictly there should be one. OK, so for the DT binding we should make vcc-supply a required property, yet the driver will still work OK if that property just happens to be missing (or e.g. when instantiated from a board file, and there's no regulator). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 11:44:05AM -0600, Stephen Warren wrote: > OK, so I believe you're saying that the case of a chip with just a > single power source, which absolutely must be present in HW for the chip > to be powered, isn't appropriate for regulator_get_optional(). Something > must always define a regulator for that power source, even if there is > no external SW control over that power source. Well, it really should be mandatory - personally I don't think it's sensible to add off-SoC chips without defining their regulators, it's more trouble than it's worth to have to add them later for all the time it takes to define the bindings. In IETF terms it's a should. > We either allow the regulator to be optional (since SW control over the > regulator is optional), or go back to every board file and DT and add a > dummy regulator in (which then breaks DT ABI, and even ignoring that is > a pain). The whole point of the way I'm changing the dummy support is to allow us to gracefully cope with errors here so there's no mandatory update even though strictly there should be one. > And note that when I say "optional" at the start of the previous > paragraph, I'm talking about probe-time regulator_get() operations and > DT content. Clearly as far as the rest of the driver is concerned, > something can always provide a dummy regulator so that e.g. > regulator_enable/disable() elsewhere always have something to operate > on. However, probe() either needs to call an API that automatically > provides such a dummy regulator, or open-code that itself. I'm still not > clear which option you think should be used. Clearly open coding this in every single driver is just not sane, you're immediately in the situation where every single driver has to implement the same thing at which point no driver ought to be doing it. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 11:44:05AM -0600, Stephen Warren wrote: > On 09/10/2013 11:04 AM, Mark Brown wrote: > > On Tue, Sep 10, 2013 at 09:07:43AM -0600, Stephen Warren wrote: > >> On 09/10/2013 04:09 AM, Mark Brown wrote: > > > >>> No. There are a couple of issues here. One is that we don't want > >>> to litter all drivers with conditional code to check if they > >>> actually got the regulator and so on, that's just pointless make > >>> work on the part of consumers. > > > >> So that's exactly the difference between (a) and (b) above. > > > > Right, but the idea is that we just only ignore a failure to get a > > supply if we can usefully run without that supply being present and > > there's more code there than simply ignoring the error - if the driver > > can genuinely just ignore all errors and otherwise not do anything > > different then requesting the regulator in the first place is clearly a > > waste of time and enabling it would be a waste of power. > > > > A driver should only be carrying code for a missing regulator if it can > > usefully work without it, like the cases where devices can use an > > internal reference if one is not available. > > OK, so I believe you're saying that the case of a chip with just a > single power source, which absolutely must be present in HW for the chip > to be powered, isn't appropriate for regulator_get_optional(). Something > must always define a regulator for that power source, even if there is > no external SW control over that power source. > I think you are supposed to use a dummy regulator in that case. Guenter > If so, how does a driver (or binding) that's been written without any > support for a regulator (since so far all boards have had no SW control > over that power source; it's always on) get enhanced to support boards > where there is SW control over the power source? > > We either allow the regulator to be optional (since SW control over the > regulator is optional), or go back to every board file and DT and add a > dummy regulator in (which then breaks DT ABI, and even ignoring that is > a pain). > > And note that when I say "optional" at the start of the previous > paragraph, I'm talking about probe-time regulator_get() operations and > DT content. Clearly as far as the rest of the driver is concerned, > something can always provide a dummy regulator so that e.g. > regulator_enable/disable() elsewhere always have something to operate > on. However, probe() either needs to call an API that automatically > provides such a dummy regulator, or open-code that itself. I'm still not > clear which option you think should be used. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 11:04 AM, Mark Brown wrote: > On Tue, Sep 10, 2013 at 09:07:43AM -0600, Stephen Warren wrote: >> On 09/10/2013 04:09 AM, Mark Brown wrote: > >>> No. There are a couple of issues here. One is that we don't want >>> to litter all drivers with conditional code to check if they >>> actually got the regulator and so on, that's just pointless make >>> work on the part of consumers. > >> So that's exactly the difference between (a) and (b) above. > > Right, but the idea is that we just only ignore a failure to get a > supply if we can usefully run without that supply being present and > there's more code there than simply ignoring the error - if the driver > can genuinely just ignore all errors and otherwise not do anything > different then requesting the regulator in the first place is clearly a > waste of time and enabling it would be a waste of power. > > A driver should only be carrying code for a missing regulator if it can > usefully work without it, like the cases where devices can use an > internal reference if one is not available. OK, so I believe you're saying that the case of a chip with just a single power source, which absolutely must be present in HW for the chip to be powered, isn't appropriate for regulator_get_optional(). Something must always define a regulator for that power source, even if there is no external SW control over that power source. If so, how does a driver (or binding) that's been written without any support for a regulator (since so far all boards have had no SW control over that power source; it's always on) get enhanced to support boards where there is SW control over the power source? We either allow the regulator to be optional (since SW control over the regulator is optional), or go back to every board file and DT and add a dummy regulator in (which then breaks DT ABI, and even ignoring that is a pain). And note that when I say "optional" at the start of the previous paragraph, I'm talking about probe-time regulator_get() operations and DT content. Clearly as far as the rest of the driver is concerned, something can always provide a dummy regulator so that e.g. regulator_enable/disable() elsewhere always have something to operate on. However, probe() either needs to call an API that automatically provides such a dummy regulator, or open-code that itself. I'm still not clear which option you think should be used. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 11:22:01AM +0800, Wei Ni wrote: > Mark, do you mean you have patches for regulator_get_optional() and > regulator_get()? Not yet but they'll be there by the time the next merge window comes. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 09:07:43AM -0600, Stephen Warren wrote: > On 09/10/2013 04:09 AM, Mark Brown wrote: > > No. There are a couple of issues here. One is that we don't want > > to litter all drivers with conditional code to check if they > > actually got the regulator and so on, that's just pointless make > > work on the part of consumers. > So that's exactly the difference between (a) and (b) above. Right, but the idea is that we just only ignore a failure to get a supply if we can usefully run without that supply being present and there's more code there than simply ignoring the error - if the driver can genuinely just ignore all errors and otherwise not do anything different then requesting the regulator in the first place is clearly a waste of time and enabling it would be a waste of power. A driver should only be carrying code for a missing regulator if it can usefully work without it, like the cases where devices can use an internal reference if one is not available. > > The other is that just ignoring errors is generally terrible > > practice which we don't want to encourage - ignoring the specific > > case where nothing is provided and the system has control of that > > is one thing but just ignoring any error is another. > Yes, obviously the code somewhere needs to distinguish between > missing-so-use-a-dummy, and specified-but-in-a-broken-way. Doesn't > regulator_get_optional() already distinguish those two cases? Perhaps > that's the enhancement to regulator_get_optional() that you were > requesting. Both calls are identical in terms of handling genuine errors. If the driver is just trying to ignore errors it will be more work to use _optional() since it has to cope with the regulator not being present in a system. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 04:09 AM, Mark Brown wrote: > On Mon, Sep 09, 2013 at 10:13:56PM -0600, Stephen Warren wrote: >> On 09/09/2013 09:53 PM, Guenter Roeck wrote: > >>> Earlier comments suggest that this is not the intended use case >>> for regulator_get_optional(). > > That's right. > >> Isn't the issue only whether the optional aspect of the regulator >> is implemented by: > >> a) regulator_get_optional() returning failure, then the driver >> having to check for that and either using or not-using the >> regulator. > >> b) regulator_get_optional() returning a dummy regulator >> automatically when none is specified in DT or the regulator >> lookup table, and hence the driver can always call >> regulator_enable/disable on the returned value. > > No. There are a couple of issues here. One is that we don't want > to litter all drivers with conditional code to check if they > actually got the regulator and so on, that's just pointless make > work on the part of consumers. So that's exactly the difference between (a) and (b) above. > The other is that just ignoring errors is generally terrible > practice which we don't want to encourage - ignoring the specific > case where nothing is provided and the system has control of that > is one thing but just ignoring any error is another. Yes, obviously the code somewhere needs to distinguish between missing-so-use-a-dummy, and specified-but-in-a-broken-way. Doesn't regulator_get_optional() already distinguish those two cases? Perhaps that's the enhancement to regulator_get_optional() that you were requesting. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 07:29:40PM +0800, Wei Ni wrote: > On my platform, it use palmas-regulator.c, ldo6 for this lm90 power > rail. I checked this driver, it will handle ramp_delay except LDOx. > Since I'm not familiar with this palmas device and driver, so do you > mean I can set regulator-ramp-delay property in the DT for ldo6, but I > really don't know what value should I set :( Laxman just submitted a patch adding the ramp delay information which I applied. It seems like this is just a hack for your board and can therefore be removed? signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 06:13 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Tue, Sep 10, 2013 at 01:39:59PM +0800, Wei Ni wrote: > >> I think the device need time to wait stable after power on, but it's >> difficult to get an exact delay value, and this delay may also relate >> with platform design, so how about to add a optional property in the DT >> node, such as "power-on-delay-ms" ? > > This is something you should *really* be able to get from the datasheet > for the part - this sort of stuff has to be documented for hardware to > be used robustly. It seems entirely possible that you are working > around an issue with the regulator driver you are using not correctly > providing its ramp time here. On my platform, it use palmas-regulator.c, ldo6 for this lm90 power rail. I checked this driver, it will handle ramp_delay except LDOx. Since I'm not familiar with this palmas device and driver, so do you mean I can set regulator-ramp-delay property in the DT for ldo6, but I really don't know what value should I set :( Thanks. Wei. > > * Unknown Key > * 0x7EA229BD > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 01:39:59PM +0800, Wei Ni wrote: > I think the device need time to wait stable after power on, but it's > difficult to get an exact delay value, and this delay may also relate > with platform design, so how about to add a optional property in the DT > node, such as "power-on-delay-ms" ? This is something you should *really* be able to get from the datasheet for the part - this sort of stuff has to be documented for hardware to be used robustly. It seems entirely possible that you are working around an issue with the regulator driver you are using not correctly providing its ramp time here. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 10:13:56PM -0600, Stephen Warren wrote: > On 09/09/2013 09:53 PM, Guenter Roeck wrote: > > Earlier comments suggest that this is not the intended use case for > > regulator_get_optional(). That's right. > Isn't the issue only whether the optional aspect of the regulator is > implemented by: > a) regulator_get_optional() returning failure, then the driver having to > check for that and either using or not-using the regulator. > b) regulator_get_optional() returning a dummy regulator automatically > when none is specified in DT or the regulator lookup table, and hence > the driver can always call regulator_enable/disable on the returned value. No. There are a couple of issues here. One is that we don't want to litter all drivers with conditional code to check if they actually got the regulator and so on, that's just pointless make work on the part of consumers. The other is that just ignoring errors is generally terrible practice which we don't want to encourage - ignoring the specific case where nothing is provided and the system has control of that is one thing but just ignoring any error is another. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 01:54 PM, Guenter Roeck wrote: > On 09/09/2013 10:39 PM, Wei Ni wrote: >> On 09/10/2013 12:50 PM, Guenter Roeck wrote: >>> On 09/09/2013 09:05 PM, Wei Ni wrote: On 09/10/2013 04:39 AM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: >> On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: > >>> It does, though it gets complicated trying to use it for a case like >>> this since you can't really tell if the regulator was powered on >>> immediately before the device got probed by another device on the bus. > >> Why not ? Just keep a timestamp. > > The support is a callback on state changes; we could keep a timestamp > but there's still going to be race conditions around bootloaders. It's > doable though. > On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? > >>> I'm not sure what the subsystem would do for such delays? It's fairly >>> common for things that need this to also want to do things like >>> manipulate GPIOs as part of the power on sequence so the applicability >>> is relatively limited, plus it's not even I2C specific, the same applies >>> to other buses so it ought to be a driver core thing. > >> Possibly. I just thought about i2c since it also takes care of basic >> devicetree bindings. Something along the line of >> if devicetree bindings for this device declare one or more >> regulators, enable those regulators before calling the driver >> probe function. > > That's definitely a driver core thing, not I2C - there's nothing > specific to I2C in there at all, needing power is pretty generic. I > have considered this before, something along the lines of what we have > for pinctrl, but unfortunately the generic case isn't quite generic > enough to make it easy. It'd need to be an explicit list of regulators > (partly just to make it opt in and avoid breaking things) and you'd want > to have a way of handling the different suspend/resume behaviour that > devices want. There's a few patterns there. > > It's definitely something I think about from time to time and it would > be useful to factor things out, the issue is getting a good enough model > of what's going on. > >>> There was some work on a generic helper for power on sequences but it >>> stalled since it wasn't accepted for the original purpose (LCD panel >>> power ons IIRC). > >> Too bad. I think it could be kept quite simple, though, by handling it >> through the regulator subsystem as suggested above. A generic binding >> for a per-regulator and per-device poweron delay should solve that >> and possibly even make it transparent to the actual driver code. > > Lots of things have a GPIO for reset too, and some want clocks too. For > maximum usefulness this should be cross subsystem. I suspect the reset > controller API may be able to handle some of it. > > The regulator power on delays are already handled transparently, by the > time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of "SMBus Clock Low Time" is 25ms, so I supposed that it may need about 25ms to stable after power on. >>> >>> Problem is that you are always waiting, even if the same regulator was >>> turned on already, and even if it is a dummy regulator. >>> >>> Imagine every driver doing that. Booting would take forever, just because of >>> unnecessary delays all over the place. There has to be a better solution >>> which does not include a mandatory and potentially unnecessary wait time >>> in the driver. At a previous company we had a design with literally dozens >>> of those chip. You really want to force such a boot delay on every user ? >>> >>> But essentially you don't even know if it is needed; you are just guessing. >>> That is not an acceptable reason to add such a delay, mandatory or not. >> I think the device need time to wait stable after power on, but it's >> difficult to get an exact delay value, and this delay may also relate >> with platform design, so how about to add a optional property in the DT >> node, such as "power-on-delay-ms" ? >> > > Possibly, but
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 01:54 PM, Guenter Roeck wrote: On 09/09/2013 10:39 PM, Wei Ni wrote: On 09/10/2013 12:50 PM, Guenter Roeck wrote: On 09/09/2013 09:05 PM, Wei Ni wrote: On 09/10/2013 04:39 AM, Mark Brown wrote: * PGP Signed by an unknown key On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of SMBus Clock Low Time is 25ms, so I supposed that it may need about 25ms to stable after power on. Problem is that you are always waiting, even if the same regulator was turned on already, and even if it is a dummy regulator. Imagine every driver doing that. Booting would take forever, just because of unnecessary delays all over the place. There has to be a better solution which does not include a mandatory and potentially unnecessary wait time in the driver. At a previous company we had a design with literally dozens of those chip. You really want to force such a boot delay on every user ? But essentially you don't even know if it is needed; you are just guessing. That is not an acceptable reason to add such a delay, mandatory or not. I think the device need time to wait stable after power on, but it's difficult to get an exact delay value, and this delay may also relate with platform design, so how about to add a optional property in the DT node, such as power-on-delay-ms ? Possibly, but that still doesn't solve the problem that you are going to wait even if the regulator was already turned on. Simple example: A system with two sensors, both of which share the same regulator. Each of them will require a delay after turning on power, but only if it was just turned on and not if it was already active. Yes, but as Mark said, the regulator subsystem still can't know if the regulator was powered on. Do you have any suggestions?
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 10:13:56PM -0600, Stephen Warren wrote: On 09/09/2013 09:53 PM, Guenter Roeck wrote: Earlier comments suggest that this is not the intended use case for regulator_get_optional(). That's right. Isn't the issue only whether the optional aspect of the regulator is implemented by: a) regulator_get_optional() returning failure, then the driver having to check for that and either using or not-using the regulator. b) regulator_get_optional() returning a dummy regulator automatically when none is specified in DT or the regulator lookup table, and hence the driver can always call regulator_enable/disable on the returned value. No. There are a couple of issues here. One is that we don't want to litter all drivers with conditional code to check if they actually got the regulator and so on, that's just pointless make work on the part of consumers. The other is that just ignoring errors is generally terrible practice which we don't want to encourage - ignoring the specific case where nothing is provided and the system has control of that is one thing but just ignoring any error is another. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 01:39:59PM +0800, Wei Ni wrote: I think the device need time to wait stable after power on, but it's difficult to get an exact delay value, and this delay may also relate with platform design, so how about to add a optional property in the DT node, such as power-on-delay-ms ? This is something you should *really* be able to get from the datasheet for the part - this sort of stuff has to be documented for hardware to be used robustly. It seems entirely possible that you are working around an issue with the regulator driver you are using not correctly providing its ramp time here. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 06:13 PM, Mark Brown wrote: * PGP Signed by an unknown key On Tue, Sep 10, 2013 at 01:39:59PM +0800, Wei Ni wrote: I think the device need time to wait stable after power on, but it's difficult to get an exact delay value, and this delay may also relate with platform design, so how about to add a optional property in the DT node, such as power-on-delay-ms ? This is something you should *really* be able to get from the datasheet for the part - this sort of stuff has to be documented for hardware to be used robustly. It seems entirely possible that you are working around an issue with the regulator driver you are using not correctly providing its ramp time here. On my platform, it use palmas-regulator.c, ldo6 for this lm90 power rail. I checked this driver, it will handle ramp_delay except LDOx. Since I'm not familiar with this palmas device and driver, so do you mean I can set regulator-ramp-delay property in the DT for ldo6, but I really don't know what value should I set :( Thanks. Wei. * Unknown Key * 0x7EA229BD -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 07:29:40PM +0800, Wei Ni wrote: On my platform, it use palmas-regulator.c, ldo6 for this lm90 power rail. I checked this driver, it will handle ramp_delay except LDOx. Since I'm not familiar with this palmas device and driver, so do you mean I can set regulator-ramp-delay property in the DT for ldo6, but I really don't know what value should I set :( Laxman just submitted a patch adding the ramp delay information which I applied. It seems like this is just a hack for your board and can therefore be removed? signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 04:09 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 10:13:56PM -0600, Stephen Warren wrote: On 09/09/2013 09:53 PM, Guenter Roeck wrote: Earlier comments suggest that this is not the intended use case for regulator_get_optional(). That's right. Isn't the issue only whether the optional aspect of the regulator is implemented by: a) regulator_get_optional() returning failure, then the driver having to check for that and either using or not-using the regulator. b) regulator_get_optional() returning a dummy regulator automatically when none is specified in DT or the regulator lookup table, and hence the driver can always call regulator_enable/disable on the returned value. No. There are a couple of issues here. One is that we don't want to litter all drivers with conditional code to check if they actually got the regulator and so on, that's just pointless make work on the part of consumers. So that's exactly the difference between (a) and (b) above. The other is that just ignoring errors is generally terrible practice which we don't want to encourage - ignoring the specific case where nothing is provided and the system has control of that is one thing but just ignoring any error is another. Yes, obviously the code somewhere needs to distinguish between missing-so-use-a-dummy, and specified-but-in-a-broken-way. Doesn't regulator_get_optional() already distinguish those two cases? Perhaps that's the enhancement to regulator_get_optional() that you were requesting. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 09:07:43AM -0600, Stephen Warren wrote: On 09/10/2013 04:09 AM, Mark Brown wrote: No. There are a couple of issues here. One is that we don't want to litter all drivers with conditional code to check if they actually got the regulator and so on, that's just pointless make work on the part of consumers. So that's exactly the difference between (a) and (b) above. Right, but the idea is that we just only ignore a failure to get a supply if we can usefully run without that supply being present and there's more code there than simply ignoring the error - if the driver can genuinely just ignore all errors and otherwise not do anything different then requesting the regulator in the first place is clearly a waste of time and enabling it would be a waste of power. A driver should only be carrying code for a missing regulator if it can usefully work without it, like the cases where devices can use an internal reference if one is not available. The other is that just ignoring errors is generally terrible practice which we don't want to encourage - ignoring the specific case where nothing is provided and the system has control of that is one thing but just ignoring any error is another. Yes, obviously the code somewhere needs to distinguish between missing-so-use-a-dummy, and specified-but-in-a-broken-way. Doesn't regulator_get_optional() already distinguish those two cases? Perhaps that's the enhancement to regulator_get_optional() that you were requesting. Both calls are identical in terms of handling genuine errors. If the driver is just trying to ignore errors it will be more work to use _optional() since it has to cope with the regulator not being present in a system. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 11:22:01AM +0800, Wei Ni wrote: Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? Not yet but they'll be there by the time the next merge window comes. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 11:04 AM, Mark Brown wrote: On Tue, Sep 10, 2013 at 09:07:43AM -0600, Stephen Warren wrote: On 09/10/2013 04:09 AM, Mark Brown wrote: No. There are a couple of issues here. One is that we don't want to litter all drivers with conditional code to check if they actually got the regulator and so on, that's just pointless make work on the part of consumers. So that's exactly the difference between (a) and (b) above. Right, but the idea is that we just only ignore a failure to get a supply if we can usefully run without that supply being present and there's more code there than simply ignoring the error - if the driver can genuinely just ignore all errors and otherwise not do anything different then requesting the regulator in the first place is clearly a waste of time and enabling it would be a waste of power. A driver should only be carrying code for a missing regulator if it can usefully work without it, like the cases where devices can use an internal reference if one is not available. OK, so I believe you're saying that the case of a chip with just a single power source, which absolutely must be present in HW for the chip to be powered, isn't appropriate for regulator_get_optional(). Something must always define a regulator for that power source, even if there is no external SW control over that power source. If so, how does a driver (or binding) that's been written without any support for a regulator (since so far all boards have had no SW control over that power source; it's always on) get enhanced to support boards where there is SW control over the power source? We either allow the regulator to be optional (since SW control over the regulator is optional), or go back to every board file and DT and add a dummy regulator in (which then breaks DT ABI, and even ignoring that is a pain). And note that when I say optional at the start of the previous paragraph, I'm talking about probe-time regulator_get() operations and DT content. Clearly as far as the rest of the driver is concerned, something can always provide a dummy regulator so that e.g. regulator_enable/disable() elsewhere always have something to operate on. However, probe() either needs to call an API that automatically provides such a dummy regulator, or open-code that itself. I'm still not clear which option you think should be used. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 11:44:05AM -0600, Stephen Warren wrote: On 09/10/2013 11:04 AM, Mark Brown wrote: On Tue, Sep 10, 2013 at 09:07:43AM -0600, Stephen Warren wrote: On 09/10/2013 04:09 AM, Mark Brown wrote: No. There are a couple of issues here. One is that we don't want to litter all drivers with conditional code to check if they actually got the regulator and so on, that's just pointless make work on the part of consumers. So that's exactly the difference between (a) and (b) above. Right, but the idea is that we just only ignore a failure to get a supply if we can usefully run without that supply being present and there's more code there than simply ignoring the error - if the driver can genuinely just ignore all errors and otherwise not do anything different then requesting the regulator in the first place is clearly a waste of time and enabling it would be a waste of power. A driver should only be carrying code for a missing regulator if it can usefully work without it, like the cases where devices can use an internal reference if one is not available. OK, so I believe you're saying that the case of a chip with just a single power source, which absolutely must be present in HW for the chip to be powered, isn't appropriate for regulator_get_optional(). Something must always define a regulator for that power source, even if there is no external SW control over that power source. I think you are supposed to use a dummy regulator in that case. Guenter If so, how does a driver (or binding) that's been written without any support for a regulator (since so far all boards have had no SW control over that power source; it's always on) get enhanced to support boards where there is SW control over the power source? We either allow the regulator to be optional (since SW control over the regulator is optional), or go back to every board file and DT and add a dummy regulator in (which then breaks DT ABI, and even ignoring that is a pain). And note that when I say optional at the start of the previous paragraph, I'm talking about probe-time regulator_get() operations and DT content. Clearly as far as the rest of the driver is concerned, something can always provide a dummy regulator so that e.g. regulator_enable/disable() elsewhere always have something to operate on. However, probe() either needs to call an API that automatically provides such a dummy regulator, or open-code that itself. I'm still not clear which option you think should be used. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 11:44:05AM -0600, Stephen Warren wrote: OK, so I believe you're saying that the case of a chip with just a single power source, which absolutely must be present in HW for the chip to be powered, isn't appropriate for regulator_get_optional(). Something must always define a regulator for that power source, even if there is no external SW control over that power source. Well, it really should be mandatory - personally I don't think it's sensible to add off-SoC chips without defining their regulators, it's more trouble than it's worth to have to add them later for all the time it takes to define the bindings. In IETF terms it's a should. We either allow the regulator to be optional (since SW control over the regulator is optional), or go back to every board file and DT and add a dummy regulator in (which then breaks DT ABI, and even ignoring that is a pain). The whole point of the way I'm changing the dummy support is to allow us to gracefully cope with errors here so there's no mandatory update even though strictly there should be one. And note that when I say optional at the start of the previous paragraph, I'm talking about probe-time regulator_get() operations and DT content. Clearly as far as the rest of the driver is concerned, something can always provide a dummy regulator so that e.g. regulator_enable/disable() elsewhere always have something to operate on. However, probe() either needs to call an API that automatically provides such a dummy regulator, or open-code that itself. I'm still not clear which option you think should be used. Clearly open coding this in every single driver is just not sane, you're immediately in the situation where every single driver has to implement the same thing at which point no driver ought to be doing it. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 12:18 PM, Mark Brown wrote: On Tue, Sep 10, 2013 at 11:44:05AM -0600, Stephen Warren wrote: OK, so I believe you're saying that the case of a chip with just a single power source, which absolutely must be present in HW for the chip to be powered, isn't appropriate for regulator_get_optional(). Something must always define a regulator for that power source, even if there is no external SW control over that power source. Well, it really should be mandatory - personally I don't think it's sensible to add off-SoC chips without defining their regulators, it's more trouble than it's worth to have to add them later for all the time it takes to define the bindings. In IETF terms it's a should. We either allow the regulator to be optional (since SW control over the regulator is optional), or go back to every board file and DT and add a dummy regulator in (which then breaks DT ABI, and even ignoring that is a pain). The whole point of the way I'm changing the dummy support is to allow us to gracefully cope with errors here so there's no mandatory update even though strictly there should be one. OK, so for the DT binding we should make vcc-supply a required property, yet the driver will still work OK if that property just happens to be missing (or e.g. when instantiated from a board file, and there's no regulator). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Tue, Sep 10, 2013 at 12:37:47PM -0600, Stephen Warren wrote: OK, so for the DT binding we should make vcc-supply a required property, yet the driver will still work OK if that property just happens to be missing (or e.g. when instantiated from a board file, and there's no regulator). Yup. That way we've got both the binding and code trying to make things work, hopefully that'll maximise robustness. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 10:39 PM, Wei Ni wrote: On 09/10/2013 12:50 PM, Guenter Roeck wrote: On 09/09/2013 09:05 PM, Wei Ni wrote: On 09/10/2013 04:39 AM, Mark Brown wrote: * PGP Signed by an unknown key On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of "SMBus Clock Low Time" is 25ms, so I supposed that it may need about 25ms to stable after power on. Problem is that you are always waiting, even if the same regulator was turned on already, and even if it is a dummy regulator. Imagine every driver doing that. Booting would take forever, just because of unnecessary delays all over the place. There has to be a better solution which does not include a mandatory and potentially unnecessary wait time in the driver. At a previous company we had a design with literally dozens of those chip. You really want to force such a boot delay on every user ? But essentially you don't even know if it is needed; you are just guessing. That is not an acceptable reason to add such a delay, mandatory or not. I think the device need time to wait stable after power on, but it's difficult to get an exact delay value, and this delay may also relate with platform design, so how about to add a optional property in the DT node, such as "power-on-delay-ms" ? Possibly, but that still doesn't solve the problem that you are going to wait even if the regulator was already turned on. Simple example: A system with two sensors, both of which share the same regulator. Each of them will require a delay after turning on power, but only if it was just turned on and not if it was already active. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 12:50 PM, Guenter Roeck wrote: > On 09/09/2013 09:05 PM, Wei Ni wrote: >> On 09/10/2013 04:39 AM, Mark Brown wrote: >>> * PGP Signed by an unknown key >>> >>> On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: >>> > It does, though it gets complicated trying to use it for a case like > this since you can't really tell if the regulator was powered on > immediately before the device got probed by another device on the bus. >>> Why not ? Just keep a timestamp. >>> >>> The support is a callback on state changes; we could keep a timestamp >>> but there's still going to be race conditions around bootloaders. It's >>> doable though. >>> >> On a higher level, I wonder if such functionality should be added in the >> i2c >> subsystem and not in i2c client drivers. Has anyone thought about this ? >>> > I'm not sure what the subsystem would do for such delays? It's fairly > common for things that need this to also want to do things like > manipulate GPIOs as part of the power on sequence so the applicability > is relatively limited, plus it's not even I2C specific, the same applies > to other buses so it ought to be a driver core thing. >>> Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. >>> >>> That's definitely a driver core thing, not I2C - there's nothing >>> specific to I2C in there at all, needing power is pretty generic. I >>> have considered this before, something along the lines of what we have >>> for pinctrl, but unfortunately the generic case isn't quite generic >>> enough to make it easy. It'd need to be an explicit list of regulators >>> (partly just to make it opt in and avoid breaking things) and you'd want >>> to have a way of handling the different suspend/resume behaviour that >>> devices want. There's a few patterns there. >>> >>> It's definitely something I think about from time to time and it would >>> be useful to factor things out, the issue is getting a good enough model >>> of what's going on. >>> > There was some work on a generic helper for power on sequences but it > stalled since it wasn't accepted for the original purpose (LCD panel > power ons IIRC). >>> Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. >>> >>> Lots of things have a GPIO for reset too, and some want clocks too. For >>> maximum usefulness this should be cross subsystem. I suspect the reset >>> controller API may be able to handle some of it. >>> >>> The regulator power on delays are already handled transparently, by the >>> time regulator_enable() returns the ramp should be finished. >> >> I think the regulator should encoded its own startup delay. Each >> individual device should handle its own requirements for delay after >> power is stable. >> The regulator_enable() will handle the delays for the regulator device. >> And adding the msleep(25) is for lm90 device. If without delay, >> sometimes the device can't work properly. If read lm90 register >> immediately after enabling regulator, the reading may be failed. >> I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max >> of "SMBus Clock Low Time" is 25ms, so I supposed that it may need about >> 25ms to stable after power on. >> > > Problem is that you are always waiting, even if the same regulator was > turned on already, and even if it is a dummy regulator. > > Imagine every driver doing that. Booting would take forever, just because of > unnecessary delays all over the place. There has to be a better solution > which does not include a mandatory and potentially unnecessary wait time > in the driver. At a previous company we had a design with literally dozens > of those chip. You really want to force such a boot delay on every user ? > > But essentially you don't even know if it is needed; you are just guessing. > That is not an acceptable reason to add such a delay, mandatory or not. I think the device need time to wait stable after power on, but it's difficult to get an exact delay value, and this delay may also relate with platform design, so how about to add a optional property in the DT node, such as "power-on-delay-ms" ? > > Guenter > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 09:05 PM, Wei Ni wrote: On 09/10/2013 04:39 AM, Mark Brown wrote: * PGP Signed by an unknown key On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of "SMBus Clock Low Time" is 25ms, so I supposed that it may need about 25ms to stable after power on. Problem is that you are always waiting, even if the same regulator was turned on already, and even if it is a dummy regulator. Imagine every driver doing that. Booting would take forever, just because of unnecessary delays all over the place. There has to be a better solution which does not include a mandatory and potentially unnecessary wait time in the driver. At a previous company we had a design with literally dozens of those chip. You really want to force such a boot delay on every user ? But essentially you don't even know if it is needed; you are just guessing. That is not an acceptable reason to add such a delay, mandatory or not. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 09:13 PM, Stephen Warren wrote: On 09/09/2013 09:53 PM, Guenter Roeck wrote: On 09/09/2013 08:40 PM, Stephen Warren wrote: On 09/09/2013 09:36 PM, Guenter Roeck wrote: ... My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. By using regulator_get_optional(), the regulator should be optional, hence you only have to add it to platforms that need it. Earlier comments suggest that this is not the intended use case for regulator_get_optional(). Isn't the issue only whether the optional aspect of the regulator is implemented by: a) regulator_get_optional() returning failure, then the driver having to check for that and either using or not-using the regulator. b) regulator_get_optional() returning a dummy regulator automatically when none is specified in DT or the regulator lookup table, and hence the driver can always call regulator_enable/disable on the returned value. I don't know. The regulator folks would have to answer that. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 09:53 PM, Guenter Roeck wrote: > On 09/09/2013 08:40 PM, Stephen Warren wrote: >> On 09/09/2013 09:36 PM, Guenter Roeck wrote: ... >>> My understanding is that by adding regulator support you essentially >>> committed to adding regulators (if necessary dummy ones) for this driver >>> to all those platforms. This is quite similar to other drivers in the >>> same situation. Once you start along that route, you'll have to go it >>> all the way. >> >> By using regulator_get_optional(), the regulator should be optional, >> hence you only have to add it to platforms that need it. >> > > Earlier comments suggest that this is not the intended use case for > regulator_get_optional(). Isn't the issue only whether the optional aspect of the regulator is implemented by: a) regulator_get_optional() returning failure, then the driver having to check for that and either using or not-using the regulator. b) regulator_get_optional() returning a dummy regulator automatically when none is specified in DT or the regulator lookup table, and hence the driver can always call regulator_enable/disable on the returned value. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 11:53 AM, Guenter Roeck wrote: > On 09/09/2013 08:40 PM, Stephen Warren wrote: >> On 09/09/2013 09:36 PM, Guenter Roeck wrote: >>> On 09/09/2013 08:22 PM, Wei Ni wrote: On 09/09/2013 11:50 PM, Guenter Roeck wrote: > On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: >> On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: >>> On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: >> This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). >> >>> Then he'll have to make sure that all devicetree files in the system >>> contain references to this regulator. >> >> Or get the patches applied on top of the code that'll be going in this >> cycle implementing get_optional() properly - when that's done the >> default will be to provide a dummy supply for regulator_get(). If you >> ack the patch I'd be happy to carry it. >> > Jean will have to ack it. > I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? >>> >>> My understanding is that by adding regulator support you essentially >>> committed to adding regulators (if necessary dummy ones) for this driver >>> to all those platforms. This is quite similar to other drivers in the >>> same situation. Once you start along that route, you'll have to go it >>> all the way. >> >> By using regulator_get_optional(), the regulator should be optional, >> hence you only have to add it to platforms that need it. >> > > Earlier comments suggest that this is not the intended use case for > regulator_get_optional(). So I just need to use the regulator_get() instead, is it right? > > Guenter > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 04:39 AM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: >> On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: > >>> It does, though it gets complicated trying to use it for a case like >>> this since you can't really tell if the regulator was powered on >>> immediately before the device got probed by another device on the bus. > >> Why not ? Just keep a timestamp. > > The support is a callback on state changes; we could keep a timestamp > but there's still going to be race conditions around bootloaders. It's > doable though. > On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? > >>> I'm not sure what the subsystem would do for such delays? It's fairly >>> common for things that need this to also want to do things like >>> manipulate GPIOs as part of the power on sequence so the applicability >>> is relatively limited, plus it's not even I2C specific, the same applies >>> to other buses so it ought to be a driver core thing. > >> Possibly. I just thought about i2c since it also takes care of basic >> devicetree bindings. Something along the line of >> if devicetree bindings for this device declare one or more >> regulators, enable those regulators before calling the driver >> probe function. > > That's definitely a driver core thing, not I2C - there's nothing > specific to I2C in there at all, needing power is pretty generic. I > have considered this before, something along the lines of what we have > for pinctrl, but unfortunately the generic case isn't quite generic > enough to make it easy. It'd need to be an explicit list of regulators > (partly just to make it opt in and avoid breaking things) and you'd want > to have a way of handling the different suspend/resume behaviour that > devices want. There's a few patterns there. > > It's definitely something I think about from time to time and it would > be useful to factor things out, the issue is getting a good enough model > of what's going on. > >>> There was some work on a generic helper for power on sequences but it >>> stalled since it wasn't accepted for the original purpose (LCD panel >>> power ons IIRC). > >> Too bad. I think it could be kept quite simple, though, by handling it >> through the regulator subsystem as suggested above. A generic binding >> for a per-regulator and per-device poweron delay should solve that >> and possibly even make it transparent to the actual driver code. > > Lots of things have a GPIO for reset too, and some want clocks too. For > maximum usefulness this should be cross subsystem. I suspect the reset > controller API may be able to handle some of it. > > The regulator power on delays are already handled transparently, by the > time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of "SMBus Clock Low Time" is 25ms, so I supposed that it may need about 25ms to stable after power on. Thanks. Wei. > > * Unknown Key > * 0x7EA229BD > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 08:40 PM, Stephen Warren wrote: On 09/09/2013 09:36 PM, Guenter Roeck wrote: On 09/09/2013 08:22 PM, Wei Ni wrote: On 09/09/2013 11:50 PM, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Jean will have to ack it. I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. By using regulator_get_optional(), the regulator should be optional, hence you only have to add it to platforms that need it. Earlier comments suggest that this is not the intended use case for regulator_get_optional(). Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 09:36 PM, Guenter Roeck wrote: > On 09/09/2013 08:22 PM, Wei Ni wrote: >> On 09/09/2013 11:50 PM, Guenter Roeck wrote: >>> On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: > On 09/09/2013 04:12 AM, Mark Brown wrote: >> On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: >> This doesn't look good, it is going to ignore actual errors - I >> *really* >> doubt that vcc is optional, it looks like it's the main power >> supply for >> the device. You should use normal regulator_get(), _optional() is >> for >> supplies which could physically not be provided in a system (eg, >> if the >> device can generate them internally if required). > Then he'll have to make sure that all devicetree files in the system > contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. >>> Jean will have to ack it. >>> >> I think it's better to use get_optional(), and ignore the errors except >> -EPROBE_DEFER. Because many platform may always power on this device, >> and will not provide regulator for it, so if we get errors from >> regulator subsystem and return it directly, then the probe() can't be >> implemented, this driver can't work properly, even though it can work >> without regulator support. >> Mark, do you mean you have patches for regulator_get_optional() and >> regulator_get()? >> > > My understanding is that by adding regulator support you essentially > committed to adding regulators (if necessary dummy ones) for this driver > to all those platforms. This is quite similar to other drivers in the > same situation. Once you start along that route, you'll have to go it > all the way. By using regulator_get_optional(), the regulator should be optional, hence you only have to add it to platforms that need it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 08:22 PM, Wei Ni wrote: On 09/09/2013 11:50 PM, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Jean will have to ack it. I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 11:50 PM, Guenter Roeck wrote: > On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: >> On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: >>> On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: >> This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). >> >>> Then he'll have to make sure that all devicetree files in the system >>> contain references to this regulator. >> >> Or get the patches applied on top of the code that'll be going in this >> cycle implementing get_optional() properly - when that's done the >> default will be to provide a dummy supply for regulator_get(). If you >> ack the patch I'd be happy to carry it. >> > Jean will have to ack it. > I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? Wei. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: > On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: > > It does, though it gets complicated trying to use it for a case like > > this since you can't really tell if the regulator was powered on > > immediately before the device got probed by another device on the bus. > Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. > > > On a higher level, I wonder if such functionality should be added in the > > > i2c > > > subsystem and not in i2c client drivers. Has anyone thought about this ? > > I'm not sure what the subsystem would do for such delays? It's fairly > > common for things that need this to also want to do things like > > manipulate GPIOs as part of the power on sequence so the applicability > > is relatively limited, plus it's not even I2C specific, the same applies > > to other buses so it ought to be a driver core thing. > Possibly. I just thought about i2c since it also takes care of basic > devicetree bindings. Something along the line of > if devicetree bindings for this device declare one or more > regulators, enable those regulators before calling the driver > probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. > > There was some work on a generic helper for power on sequences but it > > stalled since it wasn't accepted for the original purpose (LCD panel > > power ons IIRC). > Too bad. I think it could be kept quite simple, though, by handling it > through the regulator subsystem as suggested above. A generic binding > for a per-regulator and per-device poweron delay should solve that > and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: > On Mon, Sep 09, 2013 at 08:50:43AM -0700, Guenter Roeck wrote: > > On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: > > > > And indeed it does this (well, it does whatever the driver says in terms > > > of delay). However it is possible that the lm90 needs this time for > > > itself - if it's doing some sort of initialisation or callibration > > > sequence then that'll happen after the supplies come up. 25ms did seem > > > rather long, especially for such simple devices, but it's not beyond the > > > bounds of possibility. > > > Even then it would be unreasonable to enforce such a delay for every > > instance > > of this driver, even if the regulator is a dummy one and/or has been enabled > > already. Many if not almost all users of the driver work just fine as-is, > > without additional enforced delay (and, for that matter, without regulator > > ;). > > > If there is a delay, it would have to be optional: Only wait if the > > regulator > > was really turned on by the call and not already active. I don't know if the > > regulator subsystem has this capability; if not, maybe it should. > > It does, though it gets complicated trying to use it for a case like > this since you can't really tell if the regulator was powered on > immediately before the device got probed by another device on the bus. > Why not ? Just keep a timestamp. > > On a higher level, I wonder if such functionality should be added in the i2c > > subsystem and not in i2c client drivers. Has anyone thought about this ? > > I'm not sure what the subsystem would do for such delays? It's fairly > common for things that need this to also want to do things like > manipulate GPIOs as part of the power on sequence so the applicability > is relatively limited, plus it's not even I2C specific, the same applies > to other buses so it ought to be a driver core thing. > Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That was not intended to solve the delay problem, though. The delay problem seems to be more of a generic regulator problem and, as I suggested, should be solved in the regulator subsystem. if the regulator is enabled and the device specifies a poweron delay, wait for the specified amount of time after (and only after) enabling the regulator. > There was some work on a generic helper for power on sequences but it > stalled since it wasn't accepted for the original purpose (LCD panel > power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 08:50:43AM -0700, Guenter Roeck wrote: > On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: > > And indeed it does this (well, it does whatever the driver says in terms > > of delay). However it is possible that the lm90 needs this time for > > itself - if it's doing some sort of initialisation or callibration > > sequence then that'll happen after the supplies come up. 25ms did seem > > rather long, especially for such simple devices, but it's not beyond the > > bounds of possibility. > Even then it would be unreasonable to enforce such a delay for every instance > of this driver, even if the regulator is a dummy one and/or has been enabled > already. Many if not almost all users of the driver work just fine as-is, > without additional enforced delay (and, for that matter, without regulator ;). > If there is a delay, it would have to be optional: Only wait if the regulator > was really turned on by the call and not already active. I don't know if the > regulator subsystem has this capability; if not, maybe it should. It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. > On a higher level, I wonder if such functionality should be added in the i2c > subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: > On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: > > On 09/09/2013 04:12 AM, Mark Brown wrote: > > >On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: > > > >This doesn't look good, it is going to ignore actual errors - I *really* > > >doubt that vcc is optional, it looks like it's the main power supply for > > >the device. You should use normal regulator_get(), _optional() is for > > >supplies which could physically not be provided in a system (eg, if the > > >device can generate them internally if required). > > > Then he'll have to make sure that all devicetree files in the system > > contain references to this regulator. > > Or get the patches applied on top of the code that'll be going in this > cycle implementing get_optional() properly - when that's done the > default will be to provide a dummy supply for regulator_get(). If you > ack the patch I'd be happy to carry it. > Jean will have to ack it. > > >Also do you really need 25ms after power on? > > > I had not noticed, but I recommend to reject the patch because of it. > > If we add 25ms delay to each driver, booting the system will take as > > long as booting windows. If enabling the regulator needs time, the > > regulator subsystem should do it. > > And indeed it does this (well, it does whatever the driver says in terms > of delay). However it is possible that the lm90 needs this time for > itself - if it's doing some sort of initialisation or callibration > sequence then that'll happen after the supplies come up. 25ms did seem > rather long, especially for such simple devices, but it's not beyond the > bounds of possibility. Even then it would be unreasonable to enforce such a delay for every instance of this driver, even if the regulator is a dummy one and/or has been enabled already. Many if not almost all users of the driver work just fine as-is, without additional enforced delay (and, for that matter, without regulator ;). If there is a delay, it would have to be optional: Only wait if the regulator was really turned on by the call and not already active. I don't know if the regulator subsystem has this capability; if not, maybe it should. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: > On 09/09/2013 04:12 AM, Mark Brown wrote: > >On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: > >This doesn't look good, it is going to ignore actual errors - I *really* > >doubt that vcc is optional, it looks like it's the main power supply for > >the device. You should use normal regulator_get(), _optional() is for > >supplies which could physically not be provided in a system (eg, if the > >device can generate them internally if required). > Then he'll have to make sure that all devicetree files in the system > contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. > >Also do you really need 25ms after power on? > I had not noticed, but I recommend to reject the patch because of it. > If we add 25ms delay to each driver, booting the system will take as > long as booting windows. If enabling the regulator needs time, the > regulator subsystem should do it. And indeed it does this (well, it does whatever the driver says in terms of delay). However it is possible that the lm90 needs this time for itself - if it's doing some sort of initialisation or callibration sequence then that'll happen after the supplies come up. 25ms did seem rather long, especially for such simple devices, but it's not beyond the bounds of possibility. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 04:12 AM, Mark Brown wrote:
On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote:
+ reg = devm_regulator_get_optional(dev, "vcc");
+ if (!IS_ERR(reg)) {
+ err = regulator_enable(reg);
+ if (err < 0) {
+ dev_err(>dev,
+ "Failed to enable regulator: %d\n", err);
+ return err;
+ }
+ msleep(25);
+ } else {
+ if (PTR_ERR(reg) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ }
This doesn't look good, it is going to ignore actual errors - I *really*
doubt that vcc is optional, it looks like it's the main power supply for
the device. You should use normal regulator_get(), _optional() is for
supplies which could physically not be provided in a system (eg, if the
device can generate them internally if required).
Then he'll have to make sure that all devicetree files in the system contain
references
to this regulator.
Also do you really need 25ms after power on?
I had not noticed, but I recommend to reject the patch because of it. If we add
25ms delay to each driver,
booting the system will take as long as booting windows. If enabling the
regulator needs time,
the regulator subsystem should do it.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote:
> + reg = devm_regulator_get_optional(dev, "vcc");
> + if (!IS_ERR(reg)) {
> + err = regulator_enable(reg);
> + if (err < 0) {
> + dev_err(>dev,
> + "Failed to enable regulator: %d\n", err);
> + return err;
> + }
> + msleep(25);
> + } else {
> + if (PTR_ERR(reg) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + }
This doesn't look good, it is going to ignore actual errors - I *really*
doubt that vcc is optional, it looks like it's the main power supply for
the device. You should use normal regulator_get(), _optional() is for
supplies which could physically not be provided in a system (eg, if the
device can generate them internally if required).
Also do you really need 25ms after power on?
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote:
+ reg = devm_regulator_get_optional(dev, vcc);
+ if (!IS_ERR(reg)) {
+ err = regulator_enable(reg);
+ if (err 0) {
+ dev_err(client-dev,
+ Failed to enable regulator: %d\n, err);
+ return err;
+ }
+ msleep(25);
+ } else {
+ if (PTR_ERR(reg) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ }
This doesn't look good, it is going to ignore actual errors - I *really*
doubt that vcc is optional, it looks like it's the main power supply for
the device. You should use normal regulator_get(), _optional() is for
supplies which could physically not be provided in a system (eg, if the
device can generate them internally if required).
Also do you really need 25ms after power on?
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 04:12 AM, Mark Brown wrote:
On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote:
+ reg = devm_regulator_get_optional(dev, vcc);
+ if (!IS_ERR(reg)) {
+ err = regulator_enable(reg);
+ if (err 0) {
+ dev_err(client-dev,
+ Failed to enable regulator: %d\n, err);
+ return err;
+ }
+ msleep(25);
+ } else {
+ if (PTR_ERR(reg) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ }
This doesn't look good, it is going to ignore actual errors - I *really*
doubt that vcc is optional, it looks like it's the main power supply for
the device. You should use normal regulator_get(), _optional() is for
supplies which could physically not be provided in a system (eg, if the
device can generate them internally if required).
Then he'll have to make sure that all devicetree files in the system contain
references
to this regulator.
Also do you really need 25ms after power on?
I had not noticed, but I recommend to reject the patch because of it. If we add
25ms delay to each driver,
booting the system will take as long as booting windows. If enabling the
regulator needs time,
the regulator subsystem should do it.
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Also do you really need 25ms after power on? I had not noticed, but I recommend to reject the patch because of it. If we add 25ms delay to each driver, booting the system will take as long as booting windows. If enabling the regulator needs time, the regulator subsystem should do it. And indeed it does this (well, it does whatever the driver says in terms of delay). However it is possible that the lm90 needs this time for itself - if it's doing some sort of initialisation or callibration sequence then that'll happen after the supplies come up. 25ms did seem rather long, especially for such simple devices, but it's not beyond the bounds of possibility. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 08:50:43AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: And indeed it does this (well, it does whatever the driver says in terms of delay). However it is possible that the lm90 needs this time for itself - if it's doing some sort of initialisation or callibration sequence then that'll happen after the supplies come up. 25ms did seem rather long, especially for such simple devices, but it's not beyond the bounds of possibility. Even then it would be unreasonable to enforce such a delay for every instance of this driver, even if the regulator is a dummy one and/or has been enabled already. Many if not almost all users of the driver work just fine as-is, without additional enforced delay (and, for that matter, without regulator ;). If there is a delay, it would have to be optional: Only wait if the regulator was really turned on by the call and not already active. I don't know if the regulator subsystem has this capability; if not, maybe it should. It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Jean will have to ack it. Also do you really need 25ms after power on? I had not noticed, but I recommend to reject the patch because of it. If we add 25ms delay to each driver, booting the system will take as long as booting windows. If enabling the regulator needs time, the regulator subsystem should do it. And indeed it does this (well, it does whatever the driver says in terms of delay). However it is possible that the lm90 needs this time for itself - if it's doing some sort of initialisation or callibration sequence then that'll happen after the supplies come up. 25ms did seem rather long, especially for such simple devices, but it's not beyond the bounds of possibility. Even then it would be unreasonable to enforce such a delay for every instance of this driver, even if the regulator is a dummy one and/or has been enabled already. Many if not almost all users of the driver work just fine as-is, without additional enforced delay (and, for that matter, without regulator ;). If there is a delay, it would have to be optional: Only wait if the regulator was really turned on by the call and not already active. I don't know if the regulator subsystem has this capability; if not, maybe it should. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 08:50:43AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: And indeed it does this (well, it does whatever the driver says in terms of delay). However it is possible that the lm90 needs this time for itself - if it's doing some sort of initialisation or callibration sequence then that'll happen after the supplies come up. 25ms did seem rather long, especially for such simple devices, but it's not beyond the bounds of possibility. Even then it would be unreasonable to enforce such a delay for every instance of this driver, even if the regulator is a dummy one and/or has been enabled already. Many if not almost all users of the driver work just fine as-is, without additional enforced delay (and, for that matter, without regulator ;). If there is a delay, it would have to be optional: Only wait if the regulator was really turned on by the call and not already active. I don't know if the regulator subsystem has this capability; if not, maybe it should. It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That was not intended to solve the delay problem, though. The delay problem seems to be more of a generic regulator problem and, as I suggested, should be solved in the regulator subsystem. if the regulator is enabled and the device specifies a poweron delay, wait for the specified amount of time after (and only after) enabling the regulator. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. signature.asc Description: Digital signature
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 11:50 PM, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Jean will have to ack it. I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? Wei. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 08:22 PM, Wei Ni wrote: On 09/09/2013 11:50 PM, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Jean will have to ack it. I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 09:36 PM, Guenter Roeck wrote: On 09/09/2013 08:22 PM, Wei Ni wrote: On 09/09/2013 11:50 PM, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Jean will have to ack it. I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. By using regulator_get_optional(), the regulator should be optional, hence you only have to add it to platforms that need it. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 08:40 PM, Stephen Warren wrote: On 09/09/2013 09:36 PM, Guenter Roeck wrote: On 09/09/2013 08:22 PM, Wei Ni wrote: On 09/09/2013 11:50 PM, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Jean will have to ack it. I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. By using regulator_get_optional(), the regulator should be optional, hence you only have to add it to platforms that need it. Earlier comments suggest that this is not the intended use case for regulator_get_optional(). Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 04:39 AM, Mark Brown wrote: * PGP Signed by an unknown key On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of SMBus Clock Low Time is 25ms, so I supposed that it may need about 25ms to stable after power on. Thanks. Wei. * Unknown Key * 0x7EA229BD -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 11:53 AM, Guenter Roeck wrote: On 09/09/2013 08:40 PM, Stephen Warren wrote: On 09/09/2013 09:36 PM, Guenter Roeck wrote: On 09/09/2013 08:22 PM, Wei Ni wrote: On 09/09/2013 11:50 PM, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 02:50:22PM +0100, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:34:43AM -0700, Guenter Roeck wrote: On 09/09/2013 04:12 AM, Mark Brown wrote: On Mon, Sep 09, 2013 at 06:29:11PM +0800, Wei Ni wrote: This doesn't look good, it is going to ignore actual errors - I *really* doubt that vcc is optional, it looks like it's the main power supply for the device. You should use normal regulator_get(), _optional() is for supplies which could physically not be provided in a system (eg, if the device can generate them internally if required). Then he'll have to make sure that all devicetree files in the system contain references to this regulator. Or get the patches applied on top of the code that'll be going in this cycle implementing get_optional() properly - when that's done the default will be to provide a dummy supply for regulator_get(). If you ack the patch I'd be happy to carry it. Jean will have to ack it. I think it's better to use get_optional(), and ignore the errors except -EPROBE_DEFER. Because many platform may always power on this device, and will not provide regulator for it, so if we get errors from regulator subsystem and return it directly, then the probe() can't be implemented, this driver can't work properly, even though it can work without regulator support. Mark, do you mean you have patches for regulator_get_optional() and regulator_get()? My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. By using regulator_get_optional(), the regulator should be optional, hence you only have to add it to platforms that need it. Earlier comments suggest that this is not the intended use case for regulator_get_optional(). So I just need to use the regulator_get() instead, is it right? Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 09:53 PM, Guenter Roeck wrote: On 09/09/2013 08:40 PM, Stephen Warren wrote: On 09/09/2013 09:36 PM, Guenter Roeck wrote: ... My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. By using regulator_get_optional(), the regulator should be optional, hence you only have to add it to platforms that need it. Earlier comments suggest that this is not the intended use case for regulator_get_optional(). Isn't the issue only whether the optional aspect of the regulator is implemented by: a) regulator_get_optional() returning failure, then the driver having to check for that and either using or not-using the regulator. b) regulator_get_optional() returning a dummy regulator automatically when none is specified in DT or the regulator lookup table, and hence the driver can always call regulator_enable/disable on the returned value. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 09:13 PM, Stephen Warren wrote: On 09/09/2013 09:53 PM, Guenter Roeck wrote: On 09/09/2013 08:40 PM, Stephen Warren wrote: On 09/09/2013 09:36 PM, Guenter Roeck wrote: ... My understanding is that by adding regulator support you essentially committed to adding regulators (if necessary dummy ones) for this driver to all those platforms. This is quite similar to other drivers in the same situation. Once you start along that route, you'll have to go it all the way. By using regulator_get_optional(), the regulator should be optional, hence you only have to add it to platforms that need it. Earlier comments suggest that this is not the intended use case for regulator_get_optional(). Isn't the issue only whether the optional aspect of the regulator is implemented by: a) regulator_get_optional() returning failure, then the driver having to check for that and either using or not-using the regulator. b) regulator_get_optional() returning a dummy regulator automatically when none is specified in DT or the regulator lookup table, and hence the driver can always call regulator_enable/disable on the returned value. I don't know. The regulator folks would have to answer that. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 09:05 PM, Wei Ni wrote: On 09/10/2013 04:39 AM, Mark Brown wrote: * PGP Signed by an unknown key On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of SMBus Clock Low Time is 25ms, so I supposed that it may need about 25ms to stable after power on. Problem is that you are always waiting, even if the same regulator was turned on already, and even if it is a dummy regulator. Imagine every driver doing that. Booting would take forever, just because of unnecessary delays all over the place. There has to be a better solution which does not include a mandatory and potentially unnecessary wait time in the driver. At a previous company we had a design with literally dozens of those chip. You really want to force such a boot delay on every user ? But essentially you don't even know if it is needed; you are just guessing. That is not an acceptable reason to add such a delay, mandatory or not. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/10/2013 12:50 PM, Guenter Roeck wrote: On 09/09/2013 09:05 PM, Wei Ni wrote: On 09/10/2013 04:39 AM, Mark Brown wrote: * PGP Signed by an unknown key On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of SMBus Clock Low Time is 25ms, so I supposed that it may need about 25ms to stable after power on. Problem is that you are always waiting, even if the same regulator was turned on already, and even if it is a dummy regulator. Imagine every driver doing that. Booting would take forever, just because of unnecessary delays all over the place. There has to be a better solution which does not include a mandatory and potentially unnecessary wait time in the driver. At a previous company we had a design with literally dozens of those chip. You really want to force such a boot delay on every user ? But essentially you don't even know if it is needed; you are just guessing. That is not an acceptable reason to add such a delay, mandatory or not. I think the device need time to wait stable after power on, but it's difficult to get an exact delay value, and this delay may also relate with platform design, so how about to add a optional property in the DT node, such as power-on-delay-ms ? Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] hwmon: (lm90) Add power control
On 09/09/2013 10:39 PM, Wei Ni wrote: On 09/10/2013 12:50 PM, Guenter Roeck wrote: On 09/09/2013 09:05 PM, Wei Ni wrote: On 09/10/2013 04:39 AM, Mark Brown wrote: * PGP Signed by an unknown key On Mon, Sep 09, 2013 at 09:17:35AM -0700, Guenter Roeck wrote: On Mon, Sep 09, 2013 at 05:02:37PM +0100, Mark Brown wrote: It does, though it gets complicated trying to use it for a case like this since you can't really tell if the regulator was powered on immediately before the device got probed by another device on the bus. Why not ? Just keep a timestamp. The support is a callback on state changes; we could keep a timestamp but there's still going to be race conditions around bootloaders. It's doable though. On a higher level, I wonder if such functionality should be added in the i2c subsystem and not in i2c client drivers. Has anyone thought about this ? I'm not sure what the subsystem would do for such delays? It's fairly common for things that need this to also want to do things like manipulate GPIOs as part of the power on sequence so the applicability is relatively limited, plus it's not even I2C specific, the same applies to other buses so it ought to be a driver core thing. Possibly. I just thought about i2c since it also takes care of basic devicetree bindings. Something along the line of if devicetree bindings for this device declare one or more regulators, enable those regulators before calling the driver probe function. That's definitely a driver core thing, not I2C - there's nothing specific to I2C in there at all, needing power is pretty generic. I have considered this before, something along the lines of what we have for pinctrl, but unfortunately the generic case isn't quite generic enough to make it easy. It'd need to be an explicit list of regulators (partly just to make it opt in and avoid breaking things) and you'd want to have a way of handling the different suspend/resume behaviour that devices want. There's a few patterns there. It's definitely something I think about from time to time and it would be useful to factor things out, the issue is getting a good enough model of what's going on. There was some work on a generic helper for power on sequences but it stalled since it wasn't accepted for the original purpose (LCD panel power ons IIRC). Too bad. I think it could be kept quite simple, though, by handling it through the regulator subsystem as suggested above. A generic binding for a per-regulator and per-device poweron delay should solve that and possibly even make it transparent to the actual driver code. Lots of things have a GPIO for reset too, and some want clocks too. For maximum usefulness this should be cross subsystem. I suspect the reset controller API may be able to handle some of it. The regulator power on delays are already handled transparently, by the time regulator_enable() returns the ramp should be finished. I think the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. The regulator_enable() will handle the delays for the regulator device. And adding the msleep(25) is for lm90 device. If without delay, sometimes the device can't work properly. If read lm90 register immediately after enabling regulator, the reading may be failed. I'm not sure if 25ms is the right value, I read the LM90 SPEC, the max of SMBus Clock Low Time is 25ms, so I supposed that it may need about 25ms to stable after power on. Problem is that you are always waiting, even if the same regulator was turned on already, and even if it is a dummy regulator. Imagine every driver doing that. Booting would take forever, just because of unnecessary delays all over the place. There has to be a better solution which does not include a mandatory and potentially unnecessary wait time in the driver. At a previous company we had a design with literally dozens of those chip. You really want to force such a boot delay on every user ? But essentially you don't even know if it is needed; you are just guessing. That is not an acceptable reason to add such a delay, mandatory or not. I think the device need time to wait stable after power on, but it's difficult to get an exact delay value, and this delay may also relate with platform design, so how about to add a optional property in the DT node, such as power-on-delay-ms ? Possibly, but that still doesn't solve the problem that you are going to wait even if the regulator was already turned on. Simple example: A system with two sensors, both of which share the same regulator. Each of them will require a delay after turning on power, but only if it was just turned on and not if it was already active. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please
