Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Fri, 2012-08-24 at 18:24 +0900, Alex Courbot wrote: > On Tuesday 21 August 2012 17:54:20 Tomi Valkeinen wrote: > > And as I said, I don't have any problems with some kind of generic power > > sequences. So the code in the board file could be moved and converted to > > use the power sequences, if that is better than just plain c code. > > My concern now is, provided that all drivers to their job and handle how > their > devices are switched on and off, when (if at all) are encoded power sequences > better than their equivalent C code? There is the matching database size > issue > that you mentionned, is it a sufficient concern to justify a new kernel > feature? Good question. I think obviously the worst solution is to have separate .c driver files for each panel, where the drivers do 99% the same thing. So the question is how to represent the 1% difference the panels have. I think it depends on the panels. If it looks like all the panel have, say, max 2 regulators and one reset/enable gpio, and they are always enabled in the same order (regulators first, then the gpio), it should be easy to handle it in the driver without any power sequence framework. If the panels require more complex setups, then the code in the panel driver would probably start to form into a power sequence framework, and it would make sense to have it as a separate framework. Then again, if the panel setup is complex, it makes me wonder if it'd be just easier to handle it with c code in a separate driver. Also, the database size issue is a bit separate issue. There's the db size problem with or without the framework, if we do not pass the data from DT. So as clarification, I see 4 different options: - Power sequences in DT (as proposed in this series) - Custom panel data in DT, that the driver uses to power up properly - Power sequences in a panel database in kernel - Custom panel data in a panel db in kernel > On the other hand some devices like panels are typically not used in many > different appliances, so maybe it is not worth to separate them from their Yep, this is the reason for my concern with the database size. The DB could contain 10k panels, of which a board uses one. The rest just waste memory. But then again, 10k panels is probably not a realistic amount. It's difficult to guess the amount of memory used by such a database, though. If it uses, say, 8kB, I'm not sure if it's a reason to panic. And, as I mentioned, it could be optimized so that the driver throws away the unneeded panels at __init. Of course here the problem is that the panel needs to know what panels are needed. > board definition. As Mark mentionned, having .dtsi files for the DT (and > their > equivalent .h for kernels that use platform data) might be a good middle > ground. I guess it's the perfectionist in me that leans toward handling it fully in the kernel, as I think that's architecturally correct thing to do. The DT data should contain parameters that can be configured per board, or nodes (like regulators) that are needed by other parts of the DT data. If the only reason why to do it via DT is to avoid the possible size problem with the panel database, perhaps what we should solve is the optimization of the database. If that seems unsolvable, then DT approach could be used as a workaround. And I have to say I'm not too familiar with DT, so if I'm wrong about what DT should contain, I'm all ears =). Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tuesday 21 August 2012 17:54:20 Tomi Valkeinen wrote: > > However this also means we'll essentially just be moving the board code. > > > What do you mean "just"? Wasn't the point of the whole "arm board file > mess" to get rid of the code from the board files? If the code in the > board file is device specific code, not board specific, then the driver > of the device is a logical place to place it. I think Tomi has a point here - these sequences were not belonging to the board code in the first place. They are definitely tied to the device, hence should have been handled by the driver all along, with the board code assigning the correct resources to the device (like the vast majority of device drivers do). > And as I said, I don't have any problems with some kind of generic power > sequences. So the code in the board file could be moved and converted to > use the power sequences, if that is better than just plain c code. My concern now is, provided that all drivers to their job and handle how their devices are switched on and off, when (if at all) are encoded power sequences better than their equivalent C code? There is the matching database size issue that you mentionned, is it a sufficient concern to justify a new kernel feature? On the other hand some devices like panels are typically not used in many different appliances, so maybe it is not worth to separate them from their board definition. As Mark mentionned, having .dtsi files for the DT (and their equivalent .h for kernels that use platform data) might be a good middle ground. But the line is really tight between what is code and what is data here. Alex. -- 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 v4 1/3] Runtime Interpreted Power Sequences
On Tuesday 21 August 2012 17:54:20 Tomi Valkeinen wrote: However this also means we'll essentially just be moving the board code. What do you mean just? Wasn't the point of the whole arm board file mess to get rid of the code from the board files? If the code in the board file is device specific code, not board specific, then the driver of the device is a logical place to place it. I think Tomi has a point here - these sequences were not belonging to the board code in the first place. They are definitely tied to the device, hence should have been handled by the driver all along, with the board code assigning the correct resources to the device (like the vast majority of device drivers do). And as I said, I don't have any problems with some kind of generic power sequences. So the code in the board file could be moved and converted to use the power sequences, if that is better than just plain c code. My concern now is, provided that all drivers to their job and handle how their devices are switched on and off, when (if at all) are encoded power sequences better than their equivalent C code? There is the matching database size issue that you mentionned, is it a sufficient concern to justify a new kernel feature? On the other hand some devices like panels are typically not used in many different appliances, so maybe it is not worth to separate them from their board definition. As Mark mentionned, having .dtsi files for the DT (and their equivalent .h for kernels that use platform data) might be a good middle ground. But the line is really tight between what is code and what is data here. Alex. -- 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 v4 1/3] Runtime Interpreted Power Sequences
On Fri, 2012-08-24 at 18:24 +0900, Alex Courbot wrote: On Tuesday 21 August 2012 17:54:20 Tomi Valkeinen wrote: And as I said, I don't have any problems with some kind of generic power sequences. So the code in the board file could be moved and converted to use the power sequences, if that is better than just plain c code. My concern now is, provided that all drivers to their job and handle how their devices are switched on and off, when (if at all) are encoded power sequences better than their equivalent C code? There is the matching database size issue that you mentionned, is it a sufficient concern to justify a new kernel feature? Good question. I think obviously the worst solution is to have separate .c driver files for each panel, where the drivers do 99% the same thing. So the question is how to represent the 1% difference the panels have. I think it depends on the panels. If it looks like all the panel have, say, max 2 regulators and one reset/enable gpio, and they are always enabled in the same order (regulators first, then the gpio), it should be easy to handle it in the driver without any power sequence framework. If the panels require more complex setups, then the code in the panel driver would probably start to form into a power sequence framework, and it would make sense to have it as a separate framework. Then again, if the panel setup is complex, it makes me wonder if it'd be just easier to handle it with c code in a separate driver. Also, the database size issue is a bit separate issue. There's the db size problem with or without the framework, if we do not pass the data from DT. So as clarification, I see 4 different options: - Power sequences in DT (as proposed in this series) - Custom panel data in DT, that the driver uses to power up properly - Power sequences in a panel database in kernel - Custom panel data in a panel db in kernel On the other hand some devices like panels are typically not used in many different appliances, so maybe it is not worth to separate them from their Yep, this is the reason for my concern with the database size. The DB could contain 10k panels, of which a board uses one. The rest just waste memory. But then again, 10k panels is probably not a realistic amount. It's difficult to guess the amount of memory used by such a database, though. If it uses, say, 8kB, I'm not sure if it's a reason to panic. And, as I mentioned, it could be optimized so that the driver throws away the unneeded panels at __init. Of course here the problem is that the panel needs to know what panels are needed. board definition. As Mark mentionned, having .dtsi files for the DT (and their equivalent .h for kernels that use platform data) might be a good middle ground. I guess it's the perfectionist in me that leans toward handling it fully in the kernel, as I think that's architecturally correct thing to do. The DT data should contain parameters that can be configured per board, or nodes (like regulators) that are needed by other parts of the DT data. If the only reason why to do it via DT is to avoid the possible size problem with the panel database, perhaps what we should solve is the optimization of the database. If that seems unsolvable, then DT approach could be used as a workaround. And I have to say I'm not too familiar with DT, so if I'm wrong about what DT should contain, I'm all ears =). Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, Aug 21, 2012 at 05:57:38PM +0100, Mark Brown wrote: > On Tue, Aug 21, 2012 at 12:54:20PM +0300, Tomi Valkeinen wrote: > > > However, if we already have a generic driver for that type of panel, > > (which we would need anyway for the DT based approach), the developer > > only needs to add the name of the panel and the data for the power > > sequence to the panel driver's database, which is about the same amount > > of work as with DT. > > > So it's really just a question of where to put the data in question, DT > > or driver. Both contain the same data, and the data structure may also > > be the same. In DT's case it just needs to be parsed first, whereas in > > database case you'll enter the data in structs. > > I think the device tree idiomatic way of doing this is to have a bunch > of .dtsi files for the panels which then get included by reference in > the board files. This isn't helpful to people working on non-DT > architectures though. One problem that's likely to crop up here again is that the panels won't be properly describable in the DT. Typically there is no device that can be addressed by the CPU. This is the same as for backlights and fixed regulators. Thierry pgpm455F21Z7E.pgp Description: PGP signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, Aug 21, 2012 at 12:54:20PM +0300, Tomi Valkeinen wrote: > However, if we already have a generic driver for that type of panel, > (which we would need anyway for the DT based approach), the developer > only needs to add the name of the panel and the data for the power > sequence to the panel driver's database, which is about the same amount > of work as with DT. > So it's really just a question of where to put the data in question, DT > or driver. Both contain the same data, and the data structure may also > be the same. In DT's case it just needs to be parsed first, whereas in > database case you'll enter the data in structs. I think the device tree idiomatic way of doing this is to have a bunch of .dtsi files for the panels which then get included by reference in the board files. This isn't helpful to people working on non-DT architectures though. signature.asc Description: Digital signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, 2012-08-21 at 11:13 +0200, Thierry Reding wrote: > On Tue, Aug 21, 2012 at 11:57:45AM +0300, Tomi Valkeinen wrote: > > This doesn't mean that we'd have a separate driver for each device. For > > example, we have a generic panel driver in OMAP, which contains a kind > > of small panel database. The panel database contains the name of the > > panel as a key, and panel specific configuration as a value. This > > configuration could also contain some kind of generic power sequence. > > I see. I do like the idea of it, because it is more straightforward than > representing the whole sequence in DT. Matching could be done using the > standard compatible property. Right. > However this also means we'll essentially just be moving the board code. What do you mean "just"? Wasn't the point of the whole "arm board file mess" to get rid of the code from the board files? If the code in the board file is device specific code, not board specific, then the driver of the device is a logical place to place it. And as I said, I don't have any problems with some kind of generic power sequences. So the code in the board file could be moved and converted to use the power sequences, if that is better than just plain c code. > Being in a central location it would be easier to refactor commonalities > though. > > > I'd like to require the board developer to only fill in to the DT data > > what panel he is using, and how it's connected on his board. Not panel's > > internal functionality. > > The amount of work required by the board developer would largely depend > on whether support for the panel is already present or not. For new > panels this would mean that a new driver needs to be written, while > representing the power sequence in DT might be easier to do, and readily > available from some datasheet. That's true. However, if we already have a generic driver for that type of panel, (which we would need anyway for the DT based approach), the developer only needs to add the name of the panel and the data for the power sequence to the panel driver's database, which is about the same amount of work as with DT. So it's really just a question of where to put the data in question, DT or driver. Both contain the same data, and the data structure may also be the same. In DT's case it just needs to be parsed first, whereas in database case you'll enter the data in structs. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, Aug 21, 2012 at 11:57:45AM +0300, Tomi Valkeinen wrote: > On Tue, 2012-08-21 at 10:33 +0200, Thierry Reding wrote: > > > I suppose power sequences aren't needed if you have a specific driver > > for every panel out there. However that also means that you'd have to > > write drivers for literally every panel that requires support. In the > > end this will just result in discussion down the road how the common > > functionality can be refactored and we may end up with power sequences > > again. > > > > Also as you mentioned, power sequences are useful for a number of other > > use-cases. Without power sequences you'll have to potentially create > > extra frameworks tha reimplement parts of the power sequence code for > > their specific hardware needs. > > Right. I think my main concern is the use of DT data, not power > sequences as such. I've been going back and forth in my mind with this > issue with OMAP also. > > The question is: what stuff belongs to DT data and what belongs to the > kernel? I've been trying to go to the direction where DT is used to > describe the HW connections of different IP blocks and to pass board > specific configuration. Everything else is in the driver. > > This doesn't mean that we'd have a separate driver for each device. For > example, we have a generic panel driver in OMAP, which contains a kind > of small panel database. The panel database contains the name of the > panel as a key, and panel specific configuration as a value. This > configuration could also contain some kind of generic power sequence. I see. I do like the idea of it, because it is more straightforward than representing the whole sequence in DT. Matching could be done using the standard compatible property. However this also means we'll essentially just be moving the board code. Being in a central location it would be easier to refactor commonalities though. > I'd like to require the board developer to only fill in to the DT data > what panel he is using, and how it's connected on his board. Not panel's > internal functionality. The amount of work required by the board developer would largely depend on whether support for the panel is already present or not. For new panels this would mean that a new driver needs to be written, while representing the power sequence in DT might be easier to do, and readily available from some datasheet. > The one benefit I see with DT based approach is that if we have, say, > 1 panels, we'd have quite a big database in kernel memory and a > board may only need one or two of those. But perhaps that could be > helped with the use of __initdata. I haven't worked with many different panels, so maybe I can't judge this too well, but if panel drivers were kept in a central location, then the number can be reduced by generalizing parts of existing drivers. Thierry pgpElSchhsMsd.pgp Description: PGP signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, 2012-08-21 at 10:33 +0200, Thierry Reding wrote: > I suppose power sequences aren't needed if you have a specific driver > for every panel out there. However that also means that you'd have to > write drivers for literally every panel that requires support. In the > end this will just result in discussion down the road how the common > functionality can be refactored and we may end up with power sequences > again. > > Also as you mentioned, power sequences are useful for a number of other > use-cases. Without power sequences you'll have to potentially create > extra frameworks tha reimplement parts of the power sequence code for > their specific hardware needs. Right. I think my main concern is the use of DT data, not power sequences as such. I've been going back and forth in my mind with this issue with OMAP also. The question is: what stuff belongs to DT data and what belongs to the kernel? I've been trying to go to the direction where DT is used to describe the HW connections of different IP blocks and to pass board specific configuration. Everything else is in the driver. This doesn't mean that we'd have a separate driver for each device. For example, we have a generic panel driver in OMAP, which contains a kind of small panel database. The panel database contains the name of the panel as a key, and panel specific configuration as a value. This configuration could also contain some kind of generic power sequence. I'd like to require the board developer to only fill in to the DT data what panel he is using, and how it's connected on his board. Not panel's internal functionality. The one benefit I see with DT based approach is that if we have, say, 1 panels, we'd have quite a big database in kernel memory and a board may only need one or two of those. But perhaps that could be helped with the use of __initdata. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tuesday 21 August 2012 16:33:30 Thierry Reding wrote: > I suppose power sequences aren't needed if you have a specific driver > for every panel out there. However that also means that you'd have to > write drivers for literally every panel that requires support. In the > end this will just result in discussion down the road how the common > functionality can be refactored and we may end up with power sequences > again. > > Also as you mentioned, power sequences are useful for a number of other > use-cases. Without power sequences you'll have to potentially create > extra frameworks tha reimplement parts of the power sequence code for > their specific hardware needs. Yes, I can imagine what a mess it would become it we had one driver for every panel out there which sole purpose would be to define power sequences over more generic drivers. That reassures me about the usefulness of this work. Another (small) benefit of power sequences over specific drivers is that being defined in the DT, they would allow an old kernel to operate a newer device if the base driver is the same. Alex. -- 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 v4 1/3] Runtime Interpreted Power Sequences
On Tue, Aug 21, 2012 at 05:22:12PM +0900, Alex Courbot wrote: > Hi Tomi, > > On Tuesday 21 August 2012 15:44:29 Tomi Valkeinen wrote: > > > +Problem > > > +--- > > > +One very common board-dependent code is the out-of-driver code that is > > > used to +turn a device on or off. For instance, SoC boards very commonly > > > use a GPIO +(abstracted to a regulator or not) to control the power > > > supply of a backlight, +disabling it when the backlight is not used in > > > order to save power. The GPIO +that should be used, however, as well as > > > the exact power sequence that may +also involve other resources, is > > > board-dependent and thus unknown of the driver. + > > > +This was previously addressed by having hooks in the device's platform > > > data that +are called whenever the state of the device might reflect a > > > power change. This +approach, however, introduces board-dependant code > > > into the kernel and is not +compatible with the device tree. > > > > I've been having the same problems on OMAP display related code, but the > > problem has always been fixable by just having the driver to use a > > common framework to do the job (a framework which may not have existed > > at the time). The problems have never been board specific in the end, > > but device specific. > > > > Can you describe your particular HW problem a bit more? In the backlight > > case, what exactly requires the delays and the sequence you show in the > > example to enable/disable the backlight? > > In the example, the sequence (including delays) is clearly stated by the > backlight specification, which is part of the panel specification. The > backlight > uses a PWM, which makes it suitable to use the generic PWM backlight driver, > but how to turn the backlight on and off is very backlight specific. The > power > sequences allow to replace the board-specific backlight callbacks by > sequences > in the DT. > > On the other hand, I saw your discussion with Laurent about the panel > framework, and I wonder how this would fit into it. Backlights are typically > embedded within panels. Power sequences are a good way to deal with the > absence of specific drivers for every panels, since they allow to tailor the > behavior of generic drivers to fit particular needs. But if every panel comes > with its own driver (which would define the backlight device using the most > appropriate driver), then it could just as well perform its backlight's > sequences via regular callbacks. In this particular case, there would be no > need for power sequences. > > Power sequences are supposed to go beyond backlight drivers and support all > sort of devices (I have heard that it could be useful for modems as well), > but > I wonder how relevant they are when there is a proper driver for a device. I > hate to question my own work but now I cannot help but think that a proper > driver could do the same job. So what are we trying to achieve with power > sequences? Are we trying to avoid a drivers' explosion by keeping some of the > specifics out of them? Which approach would be preferable? Are there cases > where a dedicated driver could not replace power sequences? I suppose power sequences aren't needed if you have a specific driver for every panel out there. However that also means that you'd have to write drivers for literally every panel that requires support. In the end this will just result in discussion down the road how the common functionality can be refactored and we may end up with power sequences again. Also as you mentioned, power sequences are useful for a number of other use-cases. Without power sequences you'll have to potentially create extra frameworks tha reimplement parts of the power sequence code for their specific hardware needs. Thierry pgphUH0I2TdYC.pgp Description: PGP signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
Hi Tomi, On Tuesday 21 August 2012 15:44:29 Tomi Valkeinen wrote: > > +Problem > > +--- > > +One very common board-dependent code is the out-of-driver code that is > > used to +turn a device on or off. For instance, SoC boards very commonly > > use a GPIO +(abstracted to a regulator or not) to control the power > > supply of a backlight, +disabling it when the backlight is not used in > > order to save power. The GPIO +that should be used, however, as well as > > the exact power sequence that may +also involve other resources, is > > board-dependent and thus unknown of the driver. + > > +This was previously addressed by having hooks in the device's platform > > data that +are called whenever the state of the device might reflect a > > power change. This +approach, however, introduces board-dependant code > > into the kernel and is not +compatible with the device tree. > > I've been having the same problems on OMAP display related code, but the > problem has always been fixable by just having the driver to use a > common framework to do the job (a framework which may not have existed > at the time). The problems have never been board specific in the end, > but device specific. > > Can you describe your particular HW problem a bit more? In the backlight > case, what exactly requires the delays and the sequence you show in the > example to enable/disable the backlight? In the example, the sequence (including delays) is clearly stated by the backlight specification, which is part of the panel specification. The backlight uses a PWM, which makes it suitable to use the generic PWM backlight driver, but how to turn the backlight on and off is very backlight specific. The power sequences allow to replace the board-specific backlight callbacks by sequences in the DT. On the other hand, I saw your discussion with Laurent about the panel framework, and I wonder how this would fit into it. Backlights are typically embedded within panels. Power sequences are a good way to deal with the absence of specific drivers for every panels, since they allow to tailor the behavior of generic drivers to fit particular needs. But if every panel comes with its own driver (which would define the backlight device using the most appropriate driver), then it could just as well perform its backlight's sequences via regular callbacks. In this particular case, there would be no need for power sequences. Power sequences are supposed to go beyond backlight drivers and support all sort of devices (I have heard that it could be useful for modems as well), but I wonder how relevant they are when there is a proper driver for a device. I hate to question my own work but now I cannot help but think that a proper driver could do the same job. So what are we trying to achieve with power sequences? Are we trying to avoid a drivers' explosion by keeping some of the specifics out of them? Which approach would be preferable? Are there cases where a dedicated driver could not replace power sequences? Alex. -- 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 v4 1/3] Runtime Interpreted Power Sequences
Hi, On Thu, 2012-08-16 at 15:08 +0900, Alexandre Courbot wrote: > +Problem > +--- > +One very common board-dependent code is the out-of-driver code that is used > to > +turn a device on or off. For instance, SoC boards very commonly use a GPIO > +(abstracted to a regulator or not) to control the power supply of a > backlight, > +disabling it when the backlight is not used in order to save power. The GPIO > +that should be used, however, as well as the exact power sequence that may > +also involve other resources, is board-dependent and thus unknown of the > driver. > + > +This was previously addressed by having hooks in the device's platform data > that > +are called whenever the state of the device might reflect a power change. > This > +approach, however, introduces board-dependant code into the kernel and is not > +compatible with the device tree. I've been having the same problems on OMAP display related code, but the problem has always been fixable by just having the driver to use a common framework to do the job (a framework which may not have existed at the time). The problems have never been board specific in the end, but device specific. Can you describe your particular HW problem a bit more? In the backlight case, what exactly requires the delays and the sequence you show in the example to enable/disable the backlight? Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, Aug 21, 2012 at 05:57:38PM +0100, Mark Brown wrote: On Tue, Aug 21, 2012 at 12:54:20PM +0300, Tomi Valkeinen wrote: However, if we already have a generic driver for that type of panel, (which we would need anyway for the DT based approach), the developer only needs to add the name of the panel and the data for the power sequence to the panel driver's database, which is about the same amount of work as with DT. So it's really just a question of where to put the data in question, DT or driver. Both contain the same data, and the data structure may also be the same. In DT's case it just needs to be parsed first, whereas in database case you'll enter the data in structs. I think the device tree idiomatic way of doing this is to have a bunch of .dtsi files for the panels which then get included by reference in the board files. This isn't helpful to people working on non-DT architectures though. One problem that's likely to crop up here again is that the panels won't be properly describable in the DT. Typically there is no device that can be addressed by the CPU. This is the same as for backlights and fixed regulators. Thierry pgpm455F21Z7E.pgp Description: PGP signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
Hi, On Thu, 2012-08-16 at 15:08 +0900, Alexandre Courbot wrote: +Problem +--- +One very common board-dependent code is the out-of-driver code that is used to +turn a device on or off. For instance, SoC boards very commonly use a GPIO +(abstracted to a regulator or not) to control the power supply of a backlight, +disabling it when the backlight is not used in order to save power. The GPIO +that should be used, however, as well as the exact power sequence that may +also involve other resources, is board-dependent and thus unknown of the driver. + +This was previously addressed by having hooks in the device's platform data that +are called whenever the state of the device might reflect a power change. This +approach, however, introduces board-dependant code into the kernel and is not +compatible with the device tree. I've been having the same problems on OMAP display related code, but the problem has always been fixable by just having the driver to use a common framework to do the job (a framework which may not have existed at the time). The problems have never been board specific in the end, but device specific. Can you describe your particular HW problem a bit more? In the backlight case, what exactly requires the delays and the sequence you show in the example to enable/disable the backlight? Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
Hi Tomi, On Tuesday 21 August 2012 15:44:29 Tomi Valkeinen wrote: +Problem +--- +One very common board-dependent code is the out-of-driver code that is used to +turn a device on or off. For instance, SoC boards very commonly use a GPIO +(abstracted to a regulator or not) to control the power supply of a backlight, +disabling it when the backlight is not used in order to save power. The GPIO +that should be used, however, as well as the exact power sequence that may +also involve other resources, is board-dependent and thus unknown of the driver. + +This was previously addressed by having hooks in the device's platform data that +are called whenever the state of the device might reflect a power change. This +approach, however, introduces board-dependant code into the kernel and is not +compatible with the device tree. I've been having the same problems on OMAP display related code, but the problem has always been fixable by just having the driver to use a common framework to do the job (a framework which may not have existed at the time). The problems have never been board specific in the end, but device specific. Can you describe your particular HW problem a bit more? In the backlight case, what exactly requires the delays and the sequence you show in the example to enable/disable the backlight? In the example, the sequence (including delays) is clearly stated by the backlight specification, which is part of the panel specification. The backlight uses a PWM, which makes it suitable to use the generic PWM backlight driver, but how to turn the backlight on and off is very backlight specific. The power sequences allow to replace the board-specific backlight callbacks by sequences in the DT. On the other hand, I saw your discussion with Laurent about the panel framework, and I wonder how this would fit into it. Backlights are typically embedded within panels. Power sequences are a good way to deal with the absence of specific drivers for every panels, since they allow to tailor the behavior of generic drivers to fit particular needs. But if every panel comes with its own driver (which would define the backlight device using the most appropriate driver), then it could just as well perform its backlight's sequences via regular callbacks. In this particular case, there would be no need for power sequences. Power sequences are supposed to go beyond backlight drivers and support all sort of devices (I have heard that it could be useful for modems as well), but I wonder how relevant they are when there is a proper driver for a device. I hate to question my own work but now I cannot help but think that a proper driver could do the same job. So what are we trying to achieve with power sequences? Are we trying to avoid a drivers' explosion by keeping some of the specifics out of them? Which approach would be preferable? Are there cases where a dedicated driver could not replace power sequences? Alex. -- 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 v4 1/3] Runtime Interpreted Power Sequences
On Tue, Aug 21, 2012 at 05:22:12PM +0900, Alex Courbot wrote: Hi Tomi, On Tuesday 21 August 2012 15:44:29 Tomi Valkeinen wrote: +Problem +--- +One very common board-dependent code is the out-of-driver code that is used to +turn a device on or off. For instance, SoC boards very commonly use a GPIO +(abstracted to a regulator or not) to control the power supply of a backlight, +disabling it when the backlight is not used in order to save power. The GPIO +that should be used, however, as well as the exact power sequence that may +also involve other resources, is board-dependent and thus unknown of the driver. + +This was previously addressed by having hooks in the device's platform data that +are called whenever the state of the device might reflect a power change. This +approach, however, introduces board-dependant code into the kernel and is not +compatible with the device tree. I've been having the same problems on OMAP display related code, but the problem has always been fixable by just having the driver to use a common framework to do the job (a framework which may not have existed at the time). The problems have never been board specific in the end, but device specific. Can you describe your particular HW problem a bit more? In the backlight case, what exactly requires the delays and the sequence you show in the example to enable/disable the backlight? In the example, the sequence (including delays) is clearly stated by the backlight specification, which is part of the panel specification. The backlight uses a PWM, which makes it suitable to use the generic PWM backlight driver, but how to turn the backlight on and off is very backlight specific. The power sequences allow to replace the board-specific backlight callbacks by sequences in the DT. On the other hand, I saw your discussion with Laurent about the panel framework, and I wonder how this would fit into it. Backlights are typically embedded within panels. Power sequences are a good way to deal with the absence of specific drivers for every panels, since they allow to tailor the behavior of generic drivers to fit particular needs. But if every panel comes with its own driver (which would define the backlight device using the most appropriate driver), then it could just as well perform its backlight's sequences via regular callbacks. In this particular case, there would be no need for power sequences. Power sequences are supposed to go beyond backlight drivers and support all sort of devices (I have heard that it could be useful for modems as well), but I wonder how relevant they are when there is a proper driver for a device. I hate to question my own work but now I cannot help but think that a proper driver could do the same job. So what are we trying to achieve with power sequences? Are we trying to avoid a drivers' explosion by keeping some of the specifics out of them? Which approach would be preferable? Are there cases where a dedicated driver could not replace power sequences? I suppose power sequences aren't needed if you have a specific driver for every panel out there. However that also means that you'd have to write drivers for literally every panel that requires support. In the end this will just result in discussion down the road how the common functionality can be refactored and we may end up with power sequences again. Also as you mentioned, power sequences are useful for a number of other use-cases. Without power sequences you'll have to potentially create extra frameworks tha reimplement parts of the power sequence code for their specific hardware needs. Thierry pgphUH0I2TdYC.pgp Description: PGP signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tuesday 21 August 2012 16:33:30 Thierry Reding wrote: I suppose power sequences aren't needed if you have a specific driver for every panel out there. However that also means that you'd have to write drivers for literally every panel that requires support. In the end this will just result in discussion down the road how the common functionality can be refactored and we may end up with power sequences again. Also as you mentioned, power sequences are useful for a number of other use-cases. Without power sequences you'll have to potentially create extra frameworks tha reimplement parts of the power sequence code for their specific hardware needs. Yes, I can imagine what a mess it would become it we had one driver for every panel out there which sole purpose would be to define power sequences over more generic drivers. That reassures me about the usefulness of this work. Another (small) benefit of power sequences over specific drivers is that being defined in the DT, they would allow an old kernel to operate a newer device if the base driver is the same. Alex. -- 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 v4 1/3] Runtime Interpreted Power Sequences
On Tue, 2012-08-21 at 10:33 +0200, Thierry Reding wrote: I suppose power sequences aren't needed if you have a specific driver for every panel out there. However that also means that you'd have to write drivers for literally every panel that requires support. In the end this will just result in discussion down the road how the common functionality can be refactored and we may end up with power sequences again. Also as you mentioned, power sequences are useful for a number of other use-cases. Without power sequences you'll have to potentially create extra frameworks tha reimplement parts of the power sequence code for their specific hardware needs. Right. I think my main concern is the use of DT data, not power sequences as such. I've been going back and forth in my mind with this issue with OMAP also. The question is: what stuff belongs to DT data and what belongs to the kernel? I've been trying to go to the direction where DT is used to describe the HW connections of different IP blocks and to pass board specific configuration. Everything else is in the driver. This doesn't mean that we'd have a separate driver for each device. For example, we have a generic panel driver in OMAP, which contains a kind of small panel database. The panel database contains the name of the panel as a key, and panel specific configuration as a value. This configuration could also contain some kind of generic power sequence. I'd like to require the board developer to only fill in to the DT data what panel he is using, and how it's connected on his board. Not panel's internal functionality. The one benefit I see with DT based approach is that if we have, say, 1 panels, we'd have quite a big database in kernel memory and a board may only need one or two of those. But perhaps that could be helped with the use of __initdata. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, Aug 21, 2012 at 11:57:45AM +0300, Tomi Valkeinen wrote: On Tue, 2012-08-21 at 10:33 +0200, Thierry Reding wrote: I suppose power sequences aren't needed if you have a specific driver for every panel out there. However that also means that you'd have to write drivers for literally every panel that requires support. In the end this will just result in discussion down the road how the common functionality can be refactored and we may end up with power sequences again. Also as you mentioned, power sequences are useful for a number of other use-cases. Without power sequences you'll have to potentially create extra frameworks tha reimplement parts of the power sequence code for their specific hardware needs. Right. I think my main concern is the use of DT data, not power sequences as such. I've been going back and forth in my mind with this issue with OMAP also. The question is: what stuff belongs to DT data and what belongs to the kernel? I've been trying to go to the direction where DT is used to describe the HW connections of different IP blocks and to pass board specific configuration. Everything else is in the driver. This doesn't mean that we'd have a separate driver for each device. For example, we have a generic panel driver in OMAP, which contains a kind of small panel database. The panel database contains the name of the panel as a key, and panel specific configuration as a value. This configuration could also contain some kind of generic power sequence. I see. I do like the idea of it, because it is more straightforward than representing the whole sequence in DT. Matching could be done using the standard compatible property. However this also means we'll essentially just be moving the board code. Being in a central location it would be easier to refactor commonalities though. I'd like to require the board developer to only fill in to the DT data what panel he is using, and how it's connected on his board. Not panel's internal functionality. The amount of work required by the board developer would largely depend on whether support for the panel is already present or not. For new panels this would mean that a new driver needs to be written, while representing the power sequence in DT might be easier to do, and readily available from some datasheet. The one benefit I see with DT based approach is that if we have, say, 1 panels, we'd have quite a big database in kernel memory and a board may only need one or two of those. But perhaps that could be helped with the use of __initdata. I haven't worked with many different panels, so maybe I can't judge this too well, but if panel drivers were kept in a central location, then the number can be reduced by generalizing parts of existing drivers. Thierry pgpElSchhsMsd.pgp Description: PGP signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, 2012-08-21 at 11:13 +0200, Thierry Reding wrote: On Tue, Aug 21, 2012 at 11:57:45AM +0300, Tomi Valkeinen wrote: This doesn't mean that we'd have a separate driver for each device. For example, we have a generic panel driver in OMAP, which contains a kind of small panel database. The panel database contains the name of the panel as a key, and panel specific configuration as a value. This configuration could also contain some kind of generic power sequence. I see. I do like the idea of it, because it is more straightforward than representing the whole sequence in DT. Matching could be done using the standard compatible property. Right. However this also means we'll essentially just be moving the board code. What do you mean just? Wasn't the point of the whole arm board file mess to get rid of the code from the board files? If the code in the board file is device specific code, not board specific, then the driver of the device is a logical place to place it. And as I said, I don't have any problems with some kind of generic power sequences. So the code in the board file could be moved and converted to use the power sequences, if that is better than just plain c code. Being in a central location it would be easier to refactor commonalities though. I'd like to require the board developer to only fill in to the DT data what panel he is using, and how it's connected on his board. Not panel's internal functionality. The amount of work required by the board developer would largely depend on whether support for the panel is already present or not. For new panels this would mean that a new driver needs to be written, while representing the power sequence in DT might be easier to do, and readily available from some datasheet. That's true. However, if we already have a generic driver for that type of panel, (which we would need anyway for the DT based approach), the developer only needs to add the name of the panel and the data for the power sequence to the panel driver's database, which is about the same amount of work as with DT. So it's really just a question of where to put the data in question, DT or driver. Both contain the same data, and the data structure may also be the same. In DT's case it just needs to be parsed first, whereas in database case you'll enter the data in structs. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Tue, Aug 21, 2012 at 12:54:20PM +0300, Tomi Valkeinen wrote: However, if we already have a generic driver for that type of panel, (which we would need anyway for the DT based approach), the developer only needs to add the name of the panel and the data for the power sequence to the panel driver's database, which is about the same amount of work as with DT. So it's really just a question of where to put the data in question, DT or driver. Both contain the same data, and the data structure may also be the same. In DT's case it just needs to be parsed first, whereas in database case you'll enter the data in structs. I think the device tree idiomatic way of doing this is to have a bunch of .dtsi files for the panels which then get included by reference in the board files. This isn't helpful to people working on non-DT architectures though. signature.asc Description: Digital signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 11:10:30AM -1000, Mitch Bradley wrote: > On 8/16/2012 8:38 AM, Stephen Warren wrote: > > Device tree bindings shouldn't reference Linux documentation; the > > bindings are supposed to be OS-agnostic. > While it is true that bindings should try to be OS-agnostic, there is > the practical matter of where to put documentation so that it is widely > accessible. The Linux source tree is one of the most accessible things > there is, considering how widely it is replicated. > As the original instigator of the policy that the device tree should > describe the hardware "OS-neutrally", I personally don't have a problem > with bindings referring to Linux documentation. I wouldn't like > references to proprietary and inaccessible documentation. OS agnosticness isn't the only issue here - the other problem with using Linux documentation is that except for things that are specifically userspace interfaces and the DT bindings nothing is intended to be stable so bindings defined in terms of Linux documentation may randomly change. We're not doing an awesome job of that with DT right now but we should try and so we ought to avoid including non-ABI things in ABIs like this. signature.asc Description: Digital signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 08/17/2012 03:38 AM, Stephen Warren wrote:
On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
Device tree bindings shouldn't reference Linux documentation; the
bindings are supposed to be OS-agnostic.
Ok, I should be able to do without this reference anyway.
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can find
+them. The driver's documentation should list the sequence names it recognizes.
That's a little roundabout. That might be better as simply:
Valid power sequence names are defined by each device's binding. For a
power sequence named "foo", a node named "foo-power-sequence" defines
that sequence.
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result in
a
+parsing error.
Node names shouldn't be interpreted by the code; nodes should all be
named after the type of object the represent. Hence, every step should
be named just "step" for example.
The node's unit address (@0) should be used to distinguish the nodes.
This requires reg properties within each node to match the unit address,
and hence #address-cells and #size-cells properties in the power
sequence itself.
While this logic is perfectly suitable and adapted for devices, I think
we should keep in mind that power sequences and their steps are not
devices, but just arbitrary bits of information. Having adress cells and
reg properties is useful when one needs to reference a node through a
phandle, but this is *never* going to happen with sequence steps (unless
we go insane and decide to allow goto-like statements :P). So having
#address-cells, #size-cells, and reg serve absolutely no purpose here
but cluttering the DT. If there is a hard rule that needs to be
enforced, then let that be, but the other discussion you started (thanks
for that by the way) does not seem to suggest so.
Note that this somewhat conflicts with accessing the top-level power
sequence by name too; perhaps that should be re-thought too. I must
admit this DT rule kinda sucks.
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource. It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type, additional
+properties can be defined to control the action to be performed.
I think you need to add a property that indicates what type of resource
each step applies to. Sure, this is implicit in that if a "gpio"
property exists, the step is a GPIO step, but in order to make that
work, you'd have to search each node for each possible resource type's
property name. It'd be far better to read a single type="gpio" property,
then parse the step based on that.
Indeed right now all resource types must be checked. Having a type
property would make that easier. I like to keep the DT as compact and
expressive as possible, but I guess one more property per step would not
hurt and is perhaps easier to understand too.
+Example
+---
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+ backlight {
+ compatible = "pwm-backlight";
+ ...
+
+ /* resources used by the sequences */
+ pwms = < 2 500>;
+ pwm-names = "backlight";
+ power-supply = <_reg>;
+ enable-gpio = < 28 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 08/17/2012 03:38 AM, Stephen Warren wrote:
On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
Device tree bindings shouldn't reference Linux documentation; the
bindings are supposed to be OS-agnostic.
Ok, I should be able to do without this reference anyway.
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can find
+them. The driver's documentation should list the sequence names it recognizes.
That's a little roundabout. That might be better as simply:
Valid power sequence names are defined by each device's binding. For a
power sequence named foo, a node named foo-power-sequence defines
that sequence.
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result in
a
+parsing error.
Node names shouldn't be interpreted by the code; nodes should all be
named after the type of object the represent. Hence, every step should
be named just step for example.
The node's unit address (@0) should be used to distinguish the nodes.
This requires reg properties within each node to match the unit address,
and hence #address-cells and #size-cells properties in the power
sequence itself.
While this logic is perfectly suitable and adapted for devices, I think
we should keep in mind that power sequences and their steps are not
devices, but just arbitrary bits of information. Having adress cells and
reg properties is useful when one needs to reference a node through a
phandle, but this is *never* going to happen with sequence steps (unless
we go insane and decide to allow goto-like statements :P). So having
#address-cells, #size-cells, and reg serve absolutely no purpose here
but cluttering the DT. If there is a hard rule that needs to be
enforced, then let that be, but the other discussion you started (thanks
for that by the way) does not seem to suggest so.
Note that this somewhat conflicts with accessing the top-level power
sequence by name too; perhaps that should be re-thought too. I must
admit this DT rule kinda sucks.
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource. It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type, additional
+properties can be defined to control the action to be performed.
I think you need to add a property that indicates what type of resource
each step applies to. Sure, this is implicit in that if a gpio
property exists, the step is a GPIO step, but in order to make that
work, you'd have to search each node for each possible resource type's
property name. It'd be far better to read a single type=gpio property,
then parse the step based on that.
Indeed right now all resource types must be checked. Having a type
property would make that easier. I like to keep the DT as compact and
expressive as possible, but I guess one more property per step would not
hurt and is perhaps easier to understand too.
+Example
+---
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+ backlight {
+ compatible = pwm-backlight;
+ ...
+
+ /* resources used by the sequences */
+ pwms = pwm 2 500;
+ pwm-names = backlight;
+ power-supply = backlight_reg;
+ enable-gpio = gpio 28 0;
+
+ power-on-sequence {
+ step0 {
+ regulator = power;
+ enable;
+
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 11:10:30AM -1000, Mitch Bradley wrote: On 8/16/2012 8:38 AM, Stephen Warren wrote: Device tree bindings shouldn't reference Linux documentation; the bindings are supposed to be OS-agnostic. While it is true that bindings should try to be OS-agnostic, there is the practical matter of where to put documentation so that it is widely accessible. The Linux source tree is one of the most accessible things there is, considering how widely it is replicated. As the original instigator of the policy that the device tree should describe the hardware OS-neutrally, I personally don't have a problem with bindings referring to Linux documentation. I wouldn't like references to proprietary and inaccessible documentation. OS agnosticness isn't the only issue here - the other problem with using Linux documentation is that except for things that are specifically userspace interfaces and the DT bindings nothing is intended to be stable so bindings defined in terms of Linux documentation may randomly change. We're not doing an awesome job of that with DT right now but we should try and so we ought to avoid including non-ABI things in ABIs like this. signature.asc Description: Digital signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 8/16/2012 8:38 AM, Stephen Warren wrote:
> On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
>> Some device drivers (panel backlights especially) need to follow precise
>> sequences for powering on and off, involving gpios, regulators, PWMs
>> with a precise powering order and delays to respect between each steps.
>> These sequences are board-specific, and do not belong to a particular
>> driver - therefore they have been performed by board-specific hook
>> functions to far.
>>
>> With the advent of the device tree and of ARM kernels that are not
>> board-tied, we cannot rely on these board-specific hooks anymore but
>> need a way to implement these sequences in a portable manner. This patch
>> introduces a simple interpreter that can execute such power sequences
>> encoded either as platform data or within the device tree.
>
>> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
>> b/Documentation/devicetree/bindings/power_seq/power_seq.txt
>
>> +Specifying Power Sequences in the Device Tree
>> +=
>> +In the device tree, power sequences are specified as sub-nodes of the device
>> +node and reference resources declared by that device.
>> +
>> +For an introduction about runtime interpreted power sequences, see
>> +Documentation/power/power_seq.txt and include/linux/power_seq.h.
>
> Device tree bindings shouldn't reference Linux documentation; the
> bindings are supposed to be OS-agnostic.
While it is true that bindings should try to be OS-agnostic, there is
the practical matter of where to put documentation so that it is widely
accessible. The Linux source tree is one of the most accessible things
there is, considering how widely it is replicated.
As the original instigator of the policy that the device tree should
describe the hardware "OS-neutrally", I personally don't have a problem
with bindings referring to Linux documentation. I wouldn't like
references to proprietary and inaccessible documentation.
>
>> +Power Sequences Structure
>> +-
>> +Power sequences are sub-nodes that are named such as the device driver can
>> find
>> +them. The driver's documentation should list the sequence names it
>> recognizes.
>
> That's a little roundabout. That might be better as simply:
>
> Valid power sequence names are defined by each device's binding. For a
> power sequence named "foo", a node named "foo-power-sequence" defines
> that sequence.
>
>> +Inside a power sequence node are sub-nodes that describe the different steps
>> +of the sequence. Each step must be named sequentially, with the first step
>> +named step0, the second step1, etc. Failure to follow this rule will result
>> in a
>> +parsing error.
>
> Node names shouldn't be interpreted by the code; nodes should all be
> named after the type of object the represent. Hence, every step should
> be named just "step" for example.
>
> The node's unit address (@0) should be used to distinguish the nodes.
> This requires reg properties within each node to match the unit address,
> and hence #address-cells and #size-cells properties in the power
> sequence itself.
>
> Note that this somewhat conflicts with accessing the top-level power
> sequence by name too; perhaps that should be re-thought too. I must
> admit this DT rule kinda sucks.
>
>> +Power Sequences Steps
>> +-
>> +Every step of a sequence describes an action to be performed on a resource.
>> It
>> +generally includes a property named after the resource type, and which value
>> +references the resource to be used. Depending on the resource type,
>> additional
>> +properties can be defined to control the action to be performed.
>
> I think you need to add a property that indicates what type of resource
> each step applies to. Sure, this is implicit in that if a "gpio"
> property exists, the step is a GPIO step, but in order to make that
> work, you'd have to search each node for each possible resource type's
> property name. It'd be far better to read a single type="gpio" property,
> then parse the step based on that.
>
>> +Example
>> +---
>> +Here are example sequences declared within a backlight device that use all
>> the
>> +supported resources types:
>> +
>> +backlight {
>> +compatible = "pwm-backlight";
>> +...
>> +
>> +/* resources used by the sequences */
>> +pwms = < 2 500>;
>> +pwm-names = "backlight";
>> +power-supply = <_reg>;
>> +enable-gpio = < 28 0>;
>> +
>> +power-on-sequence {
>> +step0 {
>> +regulator = "power";
>> +enable;
>> +};
>> +step1 {
>> +delay = <1>;
>> +};
>> +step2 {
>> +pwm = "backlight";
>> +
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: > On 08/16/2012 12:08 AM, Alexandre Courbot wrote: > > diff --git a/Documentation/power/power_seq.txt > > b/Documentation/power/power_seq.txt > > > +Usage by Drivers and Resources Management > > +- > > +Power sequences make use of resources that must be properly allocated and > > +managed. The power_seq_build() function builds a power sequence from the > > +platform data. It also takes care of resolving and allocating the resources > > +referenced by the sequence if needed: > > + > > + struct power_seq *power_seq_build(struct device *dev, struct list_head > > *ress, > > +struct platform_power_seq *pseq); > > + > > +The 'dev' argument is the device in the name of which the resources are to > > be > > +allocated. > > + > > +The 'ress' argument is a list to which the resolved resources are > > appended. This > > +avoids allocating a resource referenced in several power sequences multiple > > +times. > > I see in other parts of the thread, there has been discussion re: > needing the separate ress parameter, and requiring the driver to pass > this in to multiple power_seq_build calls. > > The way the pinctrl subsystem solved this was to have a single function > that parsed all pinctrl setting (c.f. all power sequences) at once, and > return a single handle. Later, the driver passes this handle > pinctrl_lookup_state(), along with the requested state (c.f. sequence > name) to search for, and finally passes that handle to > pinctrl_select_state(). Doing something similar here would result in: > > struct power_seqs *power_seq_get(struct device *dev); > void power_seq_put(struct power_seqs *seqs); > struct power_seq *power_seq_lookup(struct power_seqs *seqs, > const char *name); > int power_seq_executestruct power_seq *seq); > > struct power_seqs *devm_power_seq_get(struct device *dev); > void devm_power_seq_put(struct power_seqs *seqs); Hehe, this looks very much like what I had in mind when I proposed this during the last version of this series. The nice thing about this is that the API is very much in line with how other subsystems work. Thierry pgpNOSsUBT5sx.pgp Description: PGP signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 12:57 PM, Stephen Warren wrote: > On 08/16/2012 12:47 PM, Mark Brown wrote: >> On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: >> >>> Note that this somewhat conflicts with accessing the top-level power >>> sequence by name too; perhaps that should be re-thought too. I must >>> admit this DT rule kinda sucks. >> >> Given that currently the information there is useless and ignored is >> there any reason we can't just change the rule? It'd be rather more >> constructive... > > I'll start a new thread on that topic and see. I would simplify matters > a lot... In case anyone wants to follow it, it's at: https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-August/018502.html -- 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 v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 12:47 PM, Mark Brown wrote: > On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: > >> Note that this somewhat conflicts with accessing the top-level power >> sequence by name too; perhaps that should be re-thought too. I must >> admit this DT rule kinda sucks. > > Given that currently the information there is useless and ignored is > there any reason we can't just change the rule? It'd be rather more > constructive... I'll start a new thread on that topic and see. I would simplify matters a lot... -- 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 v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: > Note that this somewhat conflicts with accessing the top-level power > sequence by name too; perhaps that should be re-thought too. I must > admit this DT rule kinda sucks. Given that currently the information there is useless and ignored is there any reason we can't just change the rule? It'd be rather more constructive... -- 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 v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
>
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
> b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> +Specifying Power Sequences in the Device Tree
> +=
> +In the device tree, power sequences are specified as sub-nodes of the device
> +node and reference resources declared by that device.
> +
> +For an introduction about runtime interpreted power sequences, see
> +Documentation/power/power_seq.txt and include/linux/power_seq.h.
Device tree bindings shouldn't reference Linux documentation; the
bindings are supposed to be OS-agnostic.
> +Power Sequences Structure
> +-
> +Power sequences are sub-nodes that are named such as the device driver can
> find
> +them. The driver's documentation should list the sequence names it
> recognizes.
That's a little roundabout. That might be better as simply:
Valid power sequence names are defined by each device's binding. For a
power sequence named "foo", a node named "foo-power-sequence" defines
that sequence.
> +Inside a power sequence node are sub-nodes that describe the different steps
> +of the sequence. Each step must be named sequentially, with the first step
> +named step0, the second step1, etc. Failure to follow this rule will result
> in a
> +parsing error.
Node names shouldn't be interpreted by the code; nodes should all be
named after the type of object the represent. Hence, every step should
be named just "step" for example.
The node's unit address (@0) should be used to distinguish the nodes.
This requires reg properties within each node to match the unit address,
and hence #address-cells and #size-cells properties in the power
sequence itself.
Note that this somewhat conflicts with accessing the top-level power
sequence by name too; perhaps that should be re-thought too. I must
admit this DT rule kinda sucks.
> +Power Sequences Steps
> +-
> +Every step of a sequence describes an action to be performed on a resource.
> It
> +generally includes a property named after the resource type, and which value
> +references the resource to be used. Depending on the resource type,
> additional
> +properties can be defined to control the action to be performed.
I think you need to add a property that indicates what type of resource
each step applies to. Sure, this is implicit in that if a "gpio"
property exists, the step is a GPIO step, but in order to make that
work, you'd have to search each node for each possible resource type's
property name. It'd be far better to read a single type="gpio" property,
then parse the step based on that.
> +Example
> +---
> +Here are example sequences declared within a backlight device that use all
> the
> +supported resources types:
> +
> + backlight {
> + compatible = "pwm-backlight";
> + ...
> +
> + /* resources used by the sequences */
> + pwms = < 2 500>;
> + pwm-names = "backlight";
> + power-supply = <_reg>;
> + enable-gpio = < 28 0>;
> +
> + power-on-sequence {
> + step0 {
> + regulator = "power";
> + enable;
> + };
> + step1 {
> + delay = <1>;
> + };
> + step2 {
> + pwm = "backlight";
> + enable;
> + };
> + step3 {
> + gpio = "enable";
> + enable;
> + };
> + };
> +
> + power-off-sequence {
> + step0 {
> + gpio = "enable";
> + disable;
> + };
> + step1 {
> + pwm = "backlight";
> + disable;
> + };
> + step2 {
> + delay = <1>;
> + };
> + step3 {
> +
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
> + power-off-sequence {
> + step0 {
> + gpio = "enable";
> + disable;
I'd change the name to "reset" or something in the example - avoids any
confusion with the action.
> +#ifdef CONFIG_REGULATOR
> + case POWER_SEQ_REGULATOR:
> + if (pdata->regulator.enable)
> + err = regulator_enable(step->resource->regulator);
> + else
> + err = regulator_disable(step->resource->regulator);
> + break;
The regulator and GPIO APIs should stub themselves out adequately to not
need the ifdefs at least for regulator I'd use the stubs since...
> + /*
> + * should never happen unless the sequence includes a step which
> + * type does not have support compiled in
> + */
> + default:
> + return -EINVAL;
...this probably isn't what's meant. It might also be nice to have
support for bulk_enable() but that's definitely something that could be
added later.
> + err = power_seq_step_run(>steps[cpt]);
> + if (err) {
> + dev_err(dev, "error %d while running power sequence!\n",
> + err);
> + return err;
I'd also log at least the step number, it'll make diagnostics easier.
--
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 v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 07:33:27PM +0900, Alex Courbot wrote: > On 08/16/2012 06:52 PM, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote: > >>On 08/16/2012 04:42 PM, Thierry Reding wrote: > Old Signed by an unknown key > >>> > >>>On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote: > >[...] > +Usage by Drivers and Resources Management > +- > +Power sequences make use of resources that must be properly allocated and > +managed. The power_seq_build() function builds a power sequence from the > +platform data. It also takes care of resolving and allocating the > resources > +referenced by the sequence if needed: > + > + struct power_seq *power_seq_build(struct device *dev, struct list_head > *ress, > +struct platform_power_seq *pseq); > + > +The 'dev' argument is the device in the name of which the resources are > to be > +allocated. > + > +The 'ress' argument is a list to which the resolved resources are > appended. This > +avoids allocating a resource referenced in several power sequences > multiple > +times. > + > +On success, the function returns a devm allocated resolved sequence that > is > +ready to be passed to power_seq_run(). In case of failure, and error > code is > +returned. > + > +A resolved power sequence returned by power_seq_build can be run by > +power_run_run(): > + > + int power_seq_run(power_seq *seq); > + > +It returns 0 if the sequence has successfully been run, or an error code > if a > +problem occured. > + > +There is no need to explicitly free the resources used by the sequence > as they > +are devm-allocated. > >>> > >>>I had some comments about this particular interface for creating > >>>sequences in the last series. My point was that explicitly requiring > >>>drivers to manage a list of already allocated resources may be too much > >>>added complexity. Power sequences should be easy to use, and I find the > >>>requirement for a separately managed list of resources cumbersome. > >>> > >>>What I proposed last time was to collect all power sequences under a > >>>common parent object, which in turn would take care of managing the > >>>resources. > >> > >>Yes, I remember that. While I see why you don't like this list, > >>having a common parent object to all sequences will not reduce the > >>number of arguments to pass to power_seq_build() (which is the only > >>function that has to handle this list now). Also having the list of > >>resources at hand is needed for some drivers: for instance, > >>pwm-backlight needs to check that exactly one PWM has been > >>allocated, and takes a reference to it from this list in order to > >>control the brightness. > > > >I'm not complaining about the additional argument to power_seq_build() > >but about the missing encapsulation. I just think that keeping a list > >external to the power sequencing code is error-prone. Drivers could do > >just about anything with it between calls to power_seq_build(). If you > >do all of this internally, then you don't depend on the driver at all > >and power sequencing code can just do the right thing. > > On the opposite side, I am concerned about over-encapsulation. :) > IIRC you proposed to have a top structure to hold the power > sequences, their resources and the associated device. Power > sequences would then have a name and be run through a 2 arguments > power_seq_run(): > > power_seq_run(sequences, "up"); > > There are two things that bother me with this solution. First is > that addressing power sequences by name looks a little bit overkill, > when a single pointer should be enough. It would also complicate the > design. Second thing is that this design would place the power > sequences structure on top of the device - in effect, you could > perfectly have several of these structures all using the same device > and failing to see each other's resources. While that would be a > error from the device driver's side, the design allows it. I see. Perhaps I'm just bugged by the interface being a simple list. If it was something just a little more sophisticated, like a very primitive resource manager attached to one device, I would be appeased. Maybe an opaque structure that carries the list and hides it for drivers would do as well. > >Obtaining a reference to the PWM, or any other resource for that matter, > >from the power sequence could be done via an explicit API. > > > >>Ideally we could embed the list into the device structure, but I > >>don't see how we can do that without modifying it (and we don't want > >>to modify it). Another solution would be to keep a static mapping > >>table that associates a device to its power_seq related resources > >>within power_seq.c.
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 06:52 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote: On 08/16/2012 04:42 PM, Thierry Reding wrote: Old Signed by an unknown key On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote: [...] +Usage by Drivers and Resources Management +- +Power sequences make use of resources that must be properly allocated and +managed. The power_seq_build() function builds a power sequence from the +platform data. It also takes care of resolving and allocating the resources +referenced by the sequence if needed: + + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress, +struct platform_power_seq *pseq); + +The 'dev' argument is the device in the name of which the resources are to be +allocated. + +The 'ress' argument is a list to which the resolved resources are appended. This +avoids allocating a resource referenced in several power sequences multiple +times. + +On success, the function returns a devm allocated resolved sequence that is +ready to be passed to power_seq_run(). In case of failure, and error code is +returned. + +A resolved power sequence returned by power_seq_build can be run by +power_run_run(): + + int power_seq_run(power_seq *seq); + +It returns 0 if the sequence has successfully been run, or an error code if a +problem occured. + +There is no need to explicitly free the resources used by the sequence as they +are devm-allocated. I had some comments about this particular interface for creating sequences in the last series. My point was that explicitly requiring drivers to manage a list of already allocated resources may be too much added complexity. Power sequences should be easy to use, and I find the requirement for a separately managed list of resources cumbersome. What I proposed last time was to collect all power sequences under a common parent object, which in turn would take care of managing the resources. Yes, I remember that. While I see why you don't like this list, having a common parent object to all sequences will not reduce the number of arguments to pass to power_seq_build() (which is the only function that has to handle this list now). Also having the list of resources at hand is needed for some drivers: for instance, pwm-backlight needs to check that exactly one PWM has been allocated, and takes a reference to it from this list in order to control the brightness. I'm not complaining about the additional argument to power_seq_build() but about the missing encapsulation. I just think that keeping a list external to the power sequencing code is error-prone. Drivers could do just about anything with it between calls to power_seq_build(). If you do all of this internally, then you don't depend on the driver at all and power sequencing code can just do the right thing. On the opposite side, I am concerned about over-encapsulation. :) IIRC you proposed to have a top structure to hold the power sequences, their resources and the associated device. Power sequences would then have a name and be run through a 2 arguments power_seq_run(): power_seq_run(sequences, "up"); There are two things that bother me with this solution. First is that addressing power sequences by name looks a little bit overkill, when a single pointer should be enough. It would also complicate the design. Second thing is that this design would place the power sequences structure on top of the device - in effect, you could perfectly have several of these structures all using the same device and failing to see each other's resources. While that would be a error from the device driver's side, the design allows it. Obtaining a reference to the PWM, or any other resource for that matter, from the power sequence could be done via an explicit API. Ideally we could embed the list into the device structure, but I don't see how we can do that without modifying it (and we don't want to modify it). Another solution would be to keep a static mapping table that associates a device to its power_seq related resources within power_seq.c. If we protect it for concurrent access this should make it possible to make resources management transparent. How does this sound? Only drawback I see is that we would need to explicitly clean it up through a dedicated function when the driver exits. I don't think that's much better. Since the power sequences will be very tightly coupled to a specific device, tying the sequences and their resources to the device makes a lot of sense. Keeping a global list of resources doesn't in my opinion. That is not what would happen actually - what I proposed is to have a mapping (hash map, or more likely binary tree) between a device and the list_head of the resources for that device. In C++ (forgive me, this makes the types more explicit) that would be: static std::map
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
> On 08/16/2012 04:42 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
[...]
> >>+Usage by Drivers and Resources Management
> >>+-
> >>+Power sequences make use of resources that must be properly allocated and
> >>+managed. The power_seq_build() function builds a power sequence from the
> >>+platform data. It also takes care of resolving and allocating the resources
> >>+referenced by the sequence if needed:
> >>+
> >>+ struct power_seq *power_seq_build(struct device *dev, struct list_head
> >>*ress,
> >>+struct platform_power_seq *pseq);
> >>+
> >>+The 'dev' argument is the device in the name of which the resources are to
> >>be
> >>+allocated.
> >>+
> >>+The 'ress' argument is a list to which the resolved resources are
> >>appended. This
> >>+avoids allocating a resource referenced in several power sequences multiple
> >>+times.
> >>+
> >>+On success, the function returns a devm allocated resolved sequence that is
> >>+ready to be passed to power_seq_run(). In case of failure, and error code
> >>is
> >>+returned.
> >>+
> >>+A resolved power sequence returned by power_seq_build can be run by
> >>+power_run_run():
> >>+
> >>+ int power_seq_run(power_seq *seq);
> >>+
> >>+It returns 0 if the sequence has successfully been run, or an error code
> >>if a
> >>+problem occured.
> >>+
> >>+There is no need to explicitly free the resources used by the sequence as
> >>they
> >>+are devm-allocated.
> >
> >I had some comments about this particular interface for creating
> >sequences in the last series. My point was that explicitly requiring
> >drivers to manage a list of already allocated resources may be too much
> >added complexity. Power sequences should be easy to use, and I find the
> >requirement for a separately managed list of resources cumbersome.
> >
> >What I proposed last time was to collect all power sequences under a
> >common parent object, which in turn would take care of managing the
> >resources.
>
> Yes, I remember that. While I see why you don't like this list,
> having a common parent object to all sequences will not reduce the
> number of arguments to pass to power_seq_build() (which is the only
> function that has to handle this list now). Also having the list of
> resources at hand is needed for some drivers: for instance,
> pwm-backlight needs to check that exactly one PWM has been
> allocated, and takes a reference to it from this list in order to
> control the brightness.
I'm not complaining about the additional argument to power_seq_build()
but about the missing encapsulation. I just think that keeping a list
external to the power sequencing code is error-prone. Drivers could do
just about anything with it between calls to power_seq_build(). If you
do all of this internally, then you don't depend on the driver at all
and power sequencing code can just do the right thing.
Obtaining a reference to the PWM, or any other resource for that matter,
from the power sequence could be done via an explicit API.
> Ideally we could embed the list into the device structure, but I
> don't see how we can do that without modifying it (and we don't want
> to modify it). Another solution would be to keep a static mapping
> table that associates a device to its power_seq related resources
> within power_seq.c. If we protect it for concurrent access this
> should make it possible to make resources management transparent.
> How does this sound? Only drawback I see is that we would need to
> explicitly clean it up through a dedicated function when the driver
> exits.
I don't think that's much better. Since the power sequences will be very
tightly coupled to a specific device, tying the sequences and their
resources to the device makes a lot of sense. Keeping a global list of
resources doesn't in my opinion.
> >>+static int power_seq_step_run(struct power_seq_step *step)
> >>+{
> >>+ struct platform_power_seq_step *pdata = >pdata;
> >>+ int err = 0;
> >>+
> >>+ switch (pdata->type) {
> >>+ case POWER_SEQ_DELAY:
> >>+ usleep_range(pdata->delay.delay_us,
> >>+ pdata->delay.delay_us + 1000);
> >>+ break;
> >>+#ifdef CONFIG_REGULATOR
> >>+ case POWER_SEQ_REGULATOR:
> >>+ if (pdata->regulator.enable)
> >>+ err = regulator_enable(step->resource->regulator);
> >>+ else
> >>+ err = regulator_disable(step->resource->regulator);
> >>+ break;
> >>+#endif
> >>+#ifdef CONFIG_PWM
> >>+ case POWER_SEQ_PWM:
> >>+ if (pdata->gpio.enable)
> >>+ err = pwm_enable(step->resource->pwm);
> >>+ else
> >>+ pwm_disable(step->resource->pwm);
> >>+ break;
> >>+#endif
> >>+#ifdef
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 04:42 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
Signed-off-by: Alexandre Courbot
---
.../devicetree/bindings/power_seq/power_seq.txt| 101 +
Documentation/power/power_seq.txt | 129 +++
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +
include/linux/power_seq.h | 142 +++
6 files changed, 796 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
new file mode 100644
index 000..749c6e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,101 @@
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
+
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can find
"named such that"?
+them. The driver's documentation should list the sequence names it recognizes.
+
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result in
a
+parsing error.
+
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource. It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type, additional
+properties can be defined to control the action to be performed.
+
+Currently supported resource types are:
I think the "currently" can be dropped.
+- "delay", which should simply contain a delay in microseconds to wait before
+ going on with the rest of the sequence. It takes no additional property.
+- "regulator" contains the name of a regulator to be acquired using
+ regulator_get(). An additional property, either "enable" or "disable", must
be
+ present to control whether the regulator should be enabled or disabled during
+ that step.
+- "pwm" is set to the name of a PWM acquired using pwm_get(). As with
regulator,
+ an additional "enable" or "disable" property is required.
+- "gpio" contains the name of a GPIO to enable or disable using the same
+ additional property as regulator or pwm. The gpio is resolved by appending
+ "-gpio" to the given name and looking for a device property with a GPIO
+ phandle.
I find this part slightly confusing. It doesn't seem quite obvious from
the text that "delay", "regulator", "pwm" and "gpio" are also actual
property names.
It might also be worth saying that the "enable" and "disable" properties
are boolean in nature and don't need a value.
Then again this becomes much clearer with the example below, so maybe
I'm just being too picky here.
This could clearly be improved. I have added a sentence before that says
that every step must define one property named after a supported
resource type - that should help. Then, as you said, the example should
clear all remaining doubts.
+
+Example
+---
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+ backlight {
+ compatible = "pwm-backlight";
+ ...
+
+ /* resources used by the sequences */
+ pwms = < 2 500>;
+ pwm-names = "backlight";
+ power-supply = <_reg>;
+ enable-gpio = < 28 0>;
+
+ power-on-sequence {
+
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
>
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
>
> Signed-off-by: Alexandre Courbot
> ---
> .../devicetree/bindings/power_seq/power_seq.txt| 101 +
> Documentation/power/power_seq.txt | 129 +++
> drivers/power/Kconfig | 3 +
> drivers/power/Makefile | 1 +
> drivers/power/power_seq.c | 420
> +
> include/linux/power_seq.h | 142 +++
> 6 files changed, 796 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
> create mode 100644 Documentation/power/power_seq.txt
> create mode 100644 drivers/power/power_seq.c
> create mode 100644 include/linux/power_seq.h
>
> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
> b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> new file mode 100644
> index 000..749c6e4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> @@ -0,0 +1,101 @@
> +Specifying Power Sequences in the Device Tree
> +=
> +In the device tree, power sequences are specified as sub-nodes of the device
> +node and reference resources declared by that device.
> +
> +For an introduction about runtime interpreted power sequences, see
> +Documentation/power/power_seq.txt and include/linux/power_seq.h.
> +
> +Power Sequences Structure
> +-
> +Power sequences are sub-nodes that are named such as the device driver can
> find
"named such that"?
> +them. The driver's documentation should list the sequence names it
> recognizes.
> +
> +Inside a power sequence node are sub-nodes that describe the different steps
> +of the sequence. Each step must be named sequentially, with the first step
> +named step0, the second step1, etc. Failure to follow this rule will result
> in a
> +parsing error.
> +
> +Power Sequences Steps
> +-
> +Every step of a sequence describes an action to be performed on a resource.
> It
> +generally includes a property named after the resource type, and which value
> +references the resource to be used. Depending on the resource type,
> additional
> +properties can be defined to control the action to be performed.
> +
> +Currently supported resource types are:
I think the "currently" can be dropped.
> +- "delay", which should simply contain a delay in microseconds to wait before
> + going on with the rest of the sequence. It takes no additional property.
> +- "regulator" contains the name of a regulator to be acquired using
> + regulator_get(). An additional property, either "enable" or "disable",
> must be
> + present to control whether the regulator should be enabled or disabled
> during
> + that step.
> +- "pwm" is set to the name of a PWM acquired using pwm_get(). As with
> regulator,
> + an additional "enable" or "disable" property is required.
> +- "gpio" contains the name of a GPIO to enable or disable using the same
> + additional property as regulator or pwm. The gpio is resolved by appending
> + "-gpio" to the given name and looking for a device property with a GPIO
> + phandle.
I find this part slightly confusing. It doesn't seem quite obvious from
the text that "delay", "regulator", "pwm" and "gpio" are also actual
property names.
It might also be worth saying that the "enable" and "disable" properties
are boolean in nature and don't need a value.
Then again this becomes much clearer with the example below, so maybe
I'm just being too picky here.
> +
> +Example
> +---
> +Here are example sequences declared within a backlight device that use all
> the
> +supported resources types:
> +
> + backlight {
> + compatible = "pwm-backlight";
> + ...
> +
> + /* resources used by the sequences */
> + pwms = < 2 500>;
> + pwm-names = "backlight";
> + power-supply = <_reg>;
> + enable-gpio = < 28 0>;
> +
> + power-on-sequence {
> + step0 {
> + regulator = "power";
> + enable;
> +
[PATCH v4 1/3] Runtime Interpreted Power Sequences
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
Signed-off-by: Alexandre Courbot
---
.../devicetree/bindings/power_seq/power_seq.txt| 101 +
Documentation/power/power_seq.txt | 129 +++
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +
include/linux/power_seq.h | 142 +++
6 files changed, 796 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
new file mode 100644
index 000..749c6e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,101 @@
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
+
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can find
+them. The driver's documentation should list the sequence names it recognizes.
+
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result in
a
+parsing error.
+
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource. It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type, additional
+properties can be defined to control the action to be performed.
+
+Currently supported resource types are:
+- "delay", which should simply contain a delay in microseconds to wait before
+ going on with the rest of the sequence. It takes no additional property.
+- "regulator" contains the name of a regulator to be acquired using
+ regulator_get(). An additional property, either "enable" or "disable", must
be
+ present to control whether the regulator should be enabled or disabled during
+ that step.
+- "pwm" is set to the name of a PWM acquired using pwm_get(). As with
regulator,
+ an additional "enable" or "disable" property is required.
+- "gpio" contains the name of a GPIO to enable or disable using the same
+ additional property as regulator or pwm. The gpio is resolved by appending
+ "-gpio" to the given name and looking for a device property with a GPIO
+ phandle.
+
+Example
+---
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+ backlight {
+ compatible = "pwm-backlight";
+ ...
+
+ /* resources used by the sequences */
+ pwms = < 2 500>;
+ pwm-names = "backlight";
+ power-supply = <_reg>;
+ enable-gpio = < 28 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <1>;
+ };
+ step2 {
+ pwm = "backlight";
+ enable;
+ };
+ step3 {
+ gpio = "enable";
+ enable;
+ };
+ };
+
+ power-off-sequence {
+ step0 {
+ gpio = "enable";
+ disable;
+ };
+ step1 {
+ pwm =
[PATCH v4 1/3] Runtime Interpreted Power Sequences
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
.../devicetree/bindings/power_seq/power_seq.txt| 101 +
Documentation/power/power_seq.txt | 129 +++
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +
include/linux/power_seq.h | 142 +++
6 files changed, 796 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
new file mode 100644
index 000..749c6e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,101 @@
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
+
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can find
+them. The driver's documentation should list the sequence names it recognizes.
+
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result in
a
+parsing error.
+
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource. It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type, additional
+properties can be defined to control the action to be performed.
+
+Currently supported resource types are:
+- delay, which should simply contain a delay in microseconds to wait before
+ going on with the rest of the sequence. It takes no additional property.
+- regulator contains the name of a regulator to be acquired using
+ regulator_get(). An additional property, either enable or disable, must
be
+ present to control whether the regulator should be enabled or disabled during
+ that step.
+- pwm is set to the name of a PWM acquired using pwm_get(). As with
regulator,
+ an additional enable or disable property is required.
+- gpio contains the name of a GPIO to enable or disable using the same
+ additional property as regulator or pwm. The gpio is resolved by appending
+ -gpio to the given name and looking for a device property with a GPIO
+ phandle.
+
+Example
+---
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+ backlight {
+ compatible = pwm-backlight;
+ ...
+
+ /* resources used by the sequences */
+ pwms = pwm 2 500;
+ pwm-names = backlight;
+ power-supply = backlight_reg;
+ enable-gpio = gpio 28 0;
+
+ power-on-sequence {
+ step0 {
+ regulator = power;
+ enable;
+ };
+ step1 {
+ delay = 1;
+ };
+ step2 {
+ pwm = backlight;
+ enable;
+ };
+ step3 {
+ gpio = enable;
+ enable;
+ };
+ };
+
+ power-off-sequence {
+ step0 {
+ gpio = enable;
+ disable;
+ };
+ step1 {
+ pwm =
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
.../devicetree/bindings/power_seq/power_seq.txt| 101 +
Documentation/power/power_seq.txt | 129 +++
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420
+
include/linux/power_seq.h | 142 +++
6 files changed, 796 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
new file mode 100644
index 000..749c6e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,101 @@
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
+
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can
find
named such that?
+them. The driver's documentation should list the sequence names it
recognizes.
+
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result
in a
+parsing error.
+
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource.
It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type,
additional
+properties can be defined to control the action to be performed.
+
+Currently supported resource types are:
I think the currently can be dropped.
+- delay, which should simply contain a delay in microseconds to wait before
+ going on with the rest of the sequence. It takes no additional property.
+- regulator contains the name of a regulator to be acquired using
+ regulator_get(). An additional property, either enable or disable,
must be
+ present to control whether the regulator should be enabled or disabled
during
+ that step.
+- pwm is set to the name of a PWM acquired using pwm_get(). As with
regulator,
+ an additional enable or disable property is required.
+- gpio contains the name of a GPIO to enable or disable using the same
+ additional property as regulator or pwm. The gpio is resolved by appending
+ -gpio to the given name and looking for a device property with a GPIO
+ phandle.
I find this part slightly confusing. It doesn't seem quite obvious from
the text that delay, regulator, pwm and gpio are also actual
property names.
It might also be worth saying that the enable and disable properties
are boolean in nature and don't need a value.
Then again this becomes much clearer with the example below, so maybe
I'm just being too picky here.
+
+Example
+---
+Here are example sequences declared within a backlight device that use all
the
+supported resources types:
+
+ backlight {
+ compatible = pwm-backlight;
+ ...
+
+ /* resources used by the sequences */
+ pwms = pwm 2 500;
+ pwm-names = backlight;
+ power-supply = backlight_reg;
+ enable-gpio = gpio 28 0;
+
+ power-on-sequence {
+ step0 {
+ regulator = power;
+ enable;
+ };
+ step1 {
+ delay = 1;
+ };
+
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 04:42 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
.../devicetree/bindings/power_seq/power_seq.txt| 101 +
Documentation/power/power_seq.txt | 129 +++
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +
include/linux/power_seq.h | 142 +++
6 files changed, 796 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
new file mode 100644
index 000..749c6e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,101 @@
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
+
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can find
named such that?
+them. The driver's documentation should list the sequence names it recognizes.
+
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result in
a
+parsing error.
+
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource. It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type, additional
+properties can be defined to control the action to be performed.
+
+Currently supported resource types are:
I think the currently can be dropped.
+- delay, which should simply contain a delay in microseconds to wait before
+ going on with the rest of the sequence. It takes no additional property.
+- regulator contains the name of a regulator to be acquired using
+ regulator_get(). An additional property, either enable or disable, must
be
+ present to control whether the regulator should be enabled or disabled during
+ that step.
+- pwm is set to the name of a PWM acquired using pwm_get(). As with
regulator,
+ an additional enable or disable property is required.
+- gpio contains the name of a GPIO to enable or disable using the same
+ additional property as regulator or pwm. The gpio is resolved by appending
+ -gpio to the given name and looking for a device property with a GPIO
+ phandle.
I find this part slightly confusing. It doesn't seem quite obvious from
the text that delay, regulator, pwm and gpio are also actual
property names.
It might also be worth saying that the enable and disable properties
are boolean in nature and don't need a value.
Then again this becomes much clearer with the example below, so maybe
I'm just being too picky here.
This could clearly be improved. I have added a sentence before that says
that every step must define one property named after a supported
resource type - that should help. Then, as you said, the example should
clear all remaining doubts.
+
+Example
+---
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+ backlight {
+ compatible = pwm-backlight;
+ ...
+
+ /* resources used by the sequences */
+ pwms = pwm 2 500;
+ pwm-names = backlight;
+ power-supply = backlight_reg;
+ enable-gpio = gpio 28 0;
+
+ power-on-sequence {
+
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
On 08/16/2012 04:42 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
[...]
+Usage by Drivers and Resources Management
+-
+Power sequences make use of resources that must be properly allocated and
+managed. The power_seq_build() function builds a power sequence from the
+platform data. It also takes care of resolving and allocating the resources
+referenced by the sequence if needed:
+
+ struct power_seq *power_seq_build(struct device *dev, struct list_head
*ress,
+struct platform_power_seq *pseq);
+
+The 'dev' argument is the device in the name of which the resources are to
be
+allocated.
+
+The 'ress' argument is a list to which the resolved resources are
appended. This
+avoids allocating a resource referenced in several power sequences multiple
+times.
+
+On success, the function returns a devm allocated resolved sequence that is
+ready to be passed to power_seq_run(). In case of failure, and error code
is
+returned.
+
+A resolved power sequence returned by power_seq_build can be run by
+power_run_run():
+
+ int power_seq_run(power_seq *seq);
+
+It returns 0 if the sequence has successfully been run, or an error code
if a
+problem occured.
+
+There is no need to explicitly free the resources used by the sequence as
they
+are devm-allocated.
I had some comments about this particular interface for creating
sequences in the last series. My point was that explicitly requiring
drivers to manage a list of already allocated resources may be too much
added complexity. Power sequences should be easy to use, and I find the
requirement for a separately managed list of resources cumbersome.
What I proposed last time was to collect all power sequences under a
common parent object, which in turn would take care of managing the
resources.
Yes, I remember that. While I see why you don't like this list,
having a common parent object to all sequences will not reduce the
number of arguments to pass to power_seq_build() (which is the only
function that has to handle this list now). Also having the list of
resources at hand is needed for some drivers: for instance,
pwm-backlight needs to check that exactly one PWM has been
allocated, and takes a reference to it from this list in order to
control the brightness.
I'm not complaining about the additional argument to power_seq_build()
but about the missing encapsulation. I just think that keeping a list
external to the power sequencing code is error-prone. Drivers could do
just about anything with it between calls to power_seq_build(). If you
do all of this internally, then you don't depend on the driver at all
and power sequencing code can just do the right thing.
Obtaining a reference to the PWM, or any other resource for that matter,
from the power sequence could be done via an explicit API.
Ideally we could embed the list into the device structure, but I
don't see how we can do that without modifying it (and we don't want
to modify it). Another solution would be to keep a static mapping
table that associates a device to its power_seq related resources
within power_seq.c. If we protect it for concurrent access this
should make it possible to make resources management transparent.
How does this sound? Only drawback I see is that we would need to
explicitly clean it up through a dedicated function when the driver
exits.
I don't think that's much better. Since the power sequences will be very
tightly coupled to a specific device, tying the sequences and their
resources to the device makes a lot of sense. Keeping a global list of
resources doesn't in my opinion.
+static int power_seq_step_run(struct power_seq_step *step)
+{
+ struct platform_power_seq_step *pdata = step-pdata;
+ int err = 0;
+
+ switch (pdata-type) {
+ case POWER_SEQ_DELAY:
+ usleep_range(pdata-delay.delay_us,
+ pdata-delay.delay_us + 1000);
+ break;
+#ifdef CONFIG_REGULATOR
+ case POWER_SEQ_REGULATOR:
+ if (pdata-regulator.enable)
+ err = regulator_enable(step-resource-regulator);
+ else
+ err = regulator_disable(step-resource-regulator);
+ break;
+#endif
+#ifdef CONFIG_PWM
+ case POWER_SEQ_PWM:
+ if (pdata-gpio.enable)
+ err = pwm_enable(step-resource-pwm);
+ else
+ pwm_disable(step-resource-pwm);
+ break;
+#endif
+#ifdef CONFIG_GPIOLIB
+ case POWER_SEQ_GPIO:
+ gpio_set_value_cansleep(pdata-gpio.gpio, pdata-gpio.enable);
+ break;
+#endif
+ /*
+ * should never happen unless the sequence includes a step which
+ * type does
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 06:52 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote: On 08/16/2012 04:42 PM, Thierry Reding wrote: Old Signed by an unknown key On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote: [...] +Usage by Drivers and Resources Management +- +Power sequences make use of resources that must be properly allocated and +managed. The power_seq_build() function builds a power sequence from the +platform data. It also takes care of resolving and allocating the resources +referenced by the sequence if needed: + + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress, +struct platform_power_seq *pseq); + +The 'dev' argument is the device in the name of which the resources are to be +allocated. + +The 'ress' argument is a list to which the resolved resources are appended. This +avoids allocating a resource referenced in several power sequences multiple +times. + +On success, the function returns a devm allocated resolved sequence that is +ready to be passed to power_seq_run(). In case of failure, and error code is +returned. + +A resolved power sequence returned by power_seq_build can be run by +power_run_run(): + + int power_seq_run(power_seq *seq); + +It returns 0 if the sequence has successfully been run, or an error code if a +problem occured. + +There is no need to explicitly free the resources used by the sequence as they +are devm-allocated. I had some comments about this particular interface for creating sequences in the last series. My point was that explicitly requiring drivers to manage a list of already allocated resources may be too much added complexity. Power sequences should be easy to use, and I find the requirement for a separately managed list of resources cumbersome. What I proposed last time was to collect all power sequences under a common parent object, which in turn would take care of managing the resources. Yes, I remember that. While I see why you don't like this list, having a common parent object to all sequences will not reduce the number of arguments to pass to power_seq_build() (which is the only function that has to handle this list now). Also having the list of resources at hand is needed for some drivers: for instance, pwm-backlight needs to check that exactly one PWM has been allocated, and takes a reference to it from this list in order to control the brightness. I'm not complaining about the additional argument to power_seq_build() but about the missing encapsulation. I just think that keeping a list external to the power sequencing code is error-prone. Drivers could do just about anything with it between calls to power_seq_build(). If you do all of this internally, then you don't depend on the driver at all and power sequencing code can just do the right thing. On the opposite side, I am concerned about over-encapsulation. :) IIRC you proposed to have a top structure to hold the power sequences, their resources and the associated device. Power sequences would then have a name and be run through a 2 arguments power_seq_run(): power_seq_run(sequences, up); There are two things that bother me with this solution. First is that addressing power sequences by name looks a little bit overkill, when a single pointer should be enough. It would also complicate the design. Second thing is that this design would place the power sequences structure on top of the device - in effect, you could perfectly have several of these structures all using the same device and failing to see each other's resources. While that would be a error from the device driver's side, the design allows it. Obtaining a reference to the PWM, or any other resource for that matter, from the power sequence could be done via an explicit API. Ideally we could embed the list into the device structure, but I don't see how we can do that without modifying it (and we don't want to modify it). Another solution would be to keep a static mapping table that associates a device to its power_seq related resources within power_seq.c. If we protect it for concurrent access this should make it possible to make resources management transparent. How does this sound? Only drawback I see is that we would need to explicitly clean it up through a dedicated function when the driver exits. I don't think that's much better. Since the power sequences will be very tightly coupled to a specific device, tying the sequences and their resources to the device makes a lot of sense. Keeping a global list of resources doesn't in my opinion. That is not what would happen actually - what I proposed is to have a mapping (hash map, or more likely binary tree) between a device and the list_head of the resources for that device. In C++ (forgive me, this makes the types more explicit) that would be: static std::mapstruct
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 07:33:27PM +0900, Alex Courbot wrote: On 08/16/2012 06:52 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote: On 08/16/2012 04:42 PM, Thierry Reding wrote: Old Signed by an unknown key On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote: [...] +Usage by Drivers and Resources Management +- +Power sequences make use of resources that must be properly allocated and +managed. The power_seq_build() function builds a power sequence from the +platform data. It also takes care of resolving and allocating the resources +referenced by the sequence if needed: + + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress, +struct platform_power_seq *pseq); + +The 'dev' argument is the device in the name of which the resources are to be +allocated. + +The 'ress' argument is a list to which the resolved resources are appended. This +avoids allocating a resource referenced in several power sequences multiple +times. + +On success, the function returns a devm allocated resolved sequence that is +ready to be passed to power_seq_run(). In case of failure, and error code is +returned. + +A resolved power sequence returned by power_seq_build can be run by +power_run_run(): + + int power_seq_run(power_seq *seq); + +It returns 0 if the sequence has successfully been run, or an error code if a +problem occured. + +There is no need to explicitly free the resources used by the sequence as they +are devm-allocated. I had some comments about this particular interface for creating sequences in the last series. My point was that explicitly requiring drivers to manage a list of already allocated resources may be too much added complexity. Power sequences should be easy to use, and I find the requirement for a separately managed list of resources cumbersome. What I proposed last time was to collect all power sequences under a common parent object, which in turn would take care of managing the resources. Yes, I remember that. While I see why you don't like this list, having a common parent object to all sequences will not reduce the number of arguments to pass to power_seq_build() (which is the only function that has to handle this list now). Also having the list of resources at hand is needed for some drivers: for instance, pwm-backlight needs to check that exactly one PWM has been allocated, and takes a reference to it from this list in order to control the brightness. I'm not complaining about the additional argument to power_seq_build() but about the missing encapsulation. I just think that keeping a list external to the power sequencing code is error-prone. Drivers could do just about anything with it between calls to power_seq_build(). If you do all of this internally, then you don't depend on the driver at all and power sequencing code can just do the right thing. On the opposite side, I am concerned about over-encapsulation. :) IIRC you proposed to have a top structure to hold the power sequences, their resources and the associated device. Power sequences would then have a name and be run through a 2 arguments power_seq_run(): power_seq_run(sequences, up); There are two things that bother me with this solution. First is that addressing power sequences by name looks a little bit overkill, when a single pointer should be enough. It would also complicate the design. Second thing is that this design would place the power sequences structure on top of the device - in effect, you could perfectly have several of these structures all using the same device and failing to see each other's resources. While that would be a error from the device driver's side, the design allows it. I see. Perhaps I'm just bugged by the interface being a simple list. If it was something just a little more sophisticated, like a very primitive resource manager attached to one device, I would be appeased. Maybe an opaque structure that carries the list and hides it for drivers would do as well. Obtaining a reference to the PWM, or any other resource for that matter, from the power sequence could be done via an explicit API. Ideally we could embed the list into the device structure, but I don't see how we can do that without modifying it (and we don't want to modify it). Another solution would be to keep a static mapping table that associates a device to its power_seq related resources within power_seq.c. If we protect it for concurrent access this should make it possible to make resources management transparent. How does this sound? Only drawback I see is that we would need to explicitly clean it up through a dedicated function when the driver exits. I don't think that's much better. Since the power sequences will be very tightly
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
+ power-off-sequence {
+ step0 {
+ gpio = enable;
+ disable;
I'd change the name to reset or something in the example - avoids any
confusion with the action.
+#ifdef CONFIG_REGULATOR
+ case POWER_SEQ_REGULATOR:
+ if (pdata-regulator.enable)
+ err = regulator_enable(step-resource-regulator);
+ else
+ err = regulator_disable(step-resource-regulator);
+ break;
The regulator and GPIO APIs should stub themselves out adequately to not
need the ifdefs at least for regulator I'd use the stubs since...
+ /*
+ * should never happen unless the sequence includes a step which
+ * type does not have support compiled in
+ */
+ default:
+ return -EINVAL;
...this probably isn't what's meant. It might also be nice to have
support for bulk_enable() but that's definitely something that could be
added later.
+ err = power_seq_step_run(seq-steps[cpt]);
+ if (err) {
+ dev_err(dev, error %d while running power sequence!\n,
+ err);
+ return err;
I'd also log at least the step number, it'll make diagnostics easier.
--
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 v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
Device tree bindings shouldn't reference Linux documentation; the
bindings are supposed to be OS-agnostic.
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can
find
+them. The driver's documentation should list the sequence names it
recognizes.
That's a little roundabout. That might be better as simply:
Valid power sequence names are defined by each device's binding. For a
power sequence named foo, a node named foo-power-sequence defines
that sequence.
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result
in a
+parsing error.
Node names shouldn't be interpreted by the code; nodes should all be
named after the type of object the represent. Hence, every step should
be named just step for example.
The node's unit address (@0) should be used to distinguish the nodes.
This requires reg properties within each node to match the unit address,
and hence #address-cells and #size-cells properties in the power
sequence itself.
Note that this somewhat conflicts with accessing the top-level power
sequence by name too; perhaps that should be re-thought too. I must
admit this DT rule kinda sucks.
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource.
It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type,
additional
+properties can be defined to control the action to be performed.
I think you need to add a property that indicates what type of resource
each step applies to. Sure, this is implicit in that if a gpio
property exists, the step is a GPIO step, but in order to make that
work, you'd have to search each node for each possible resource type's
property name. It'd be far better to read a single type=gpio property,
then parse the step based on that.
+Example
+---
+Here are example sequences declared within a backlight device that use all
the
+supported resources types:
+
+ backlight {
+ compatible = pwm-backlight;
+ ...
+
+ /* resources used by the sequences */
+ pwms = pwm 2 500;
+ pwm-names = backlight;
+ power-supply = backlight_reg;
+ enable-gpio = gpio 28 0;
+
+ power-on-sequence {
+ step0 {
+ regulator = power;
+ enable;
+ };
+ step1 {
+ delay = 1;
+ };
+ step2 {
+ pwm = backlight;
+ enable;
+ };
+ step3 {
+ gpio = enable;
+ enable;
+ };
+ };
+
+ power-off-sequence {
+ step0 {
+ gpio = enable;
+ disable;
+ };
+ step1 {
+ pwm = backlight;
+ disable;
+ };
+ step2 {
+ delay = 1;
+ };
+ step3 {
+ regulator = power;
+ disable;
+ };
+
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: Note that this somewhat conflicts with accessing the top-level power sequence by name too; perhaps that should be re-thought too. I must admit this DT rule kinda sucks. Given that currently the information there is useless and ignored is there any reason we can't just change the rule? It'd be rather more constructive... -- 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 v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 12:47 PM, Mark Brown wrote: On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: Note that this somewhat conflicts with accessing the top-level power sequence by name too; perhaps that should be re-thought too. I must admit this DT rule kinda sucks. Given that currently the information there is useless and ignored is there any reason we can't just change the rule? It'd be rather more constructive... I'll start a new thread on that topic and see. I would simplify matters a lot... -- 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 v4 1/3] Runtime Interpreted Power Sequences
On 08/16/2012 12:57 PM, Stephen Warren wrote: On 08/16/2012 12:47 PM, Mark Brown wrote: On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: Note that this somewhat conflicts with accessing the top-level power sequence by name too; perhaps that should be re-thought too. I must admit this DT rule kinda sucks. Given that currently the information there is useless and ignored is there any reason we can't just change the rule? It'd be rather more constructive... I'll start a new thread on that topic and see. I would simplify matters a lot... In case anyone wants to follow it, it's at: https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-August/018502.html -- 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 v4 1/3] Runtime Interpreted Power Sequences
On Thu, Aug 16, 2012 at 12:38:33PM -0600, Stephen Warren wrote: On 08/16/2012 12:08 AM, Alexandre Courbot wrote: diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt +Usage by Drivers and Resources Management +- +Power sequences make use of resources that must be properly allocated and +managed. The power_seq_build() function builds a power sequence from the +platform data. It also takes care of resolving and allocating the resources +referenced by the sequence if needed: + + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress, +struct platform_power_seq *pseq); + +The 'dev' argument is the device in the name of which the resources are to be +allocated. + +The 'ress' argument is a list to which the resolved resources are appended. This +avoids allocating a resource referenced in several power sequences multiple +times. I see in other parts of the thread, there has been discussion re: needing the separate ress parameter, and requiring the driver to pass this in to multiple power_seq_build calls. The way the pinctrl subsystem solved this was to have a single function that parsed all pinctrl setting (c.f. all power sequences) at once, and return a single handle. Later, the driver passes this handle pinctrl_lookup_state(), along with the requested state (c.f. sequence name) to search for, and finally passes that handle to pinctrl_select_state(). Doing something similar here would result in: struct power_seqs *power_seq_get(struct device *dev); void power_seq_put(struct power_seqs *seqs); struct power_seq *power_seq_lookup(struct power_seqs *seqs, const char *name); int power_seq_executestruct power_seq *seq); struct power_seqs *devm_power_seq_get(struct device *dev); void devm_power_seq_put(struct power_seqs *seqs); Hehe, this looks very much like what I had in mind when I proposed this during the last version of this series. The nice thing about this is that the API is very much in line with how other subsystems work. Thierry pgpNOSsUBT5sx.pgp Description: PGP signature
Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
On 8/16/2012 8:38 AM, Stephen Warren wrote:
On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
b/Documentation/devicetree/bindings/power_seq/power_seq.txt
+Specifying Power Sequences in the Device Tree
+=
+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
Device tree bindings shouldn't reference Linux documentation; the
bindings are supposed to be OS-agnostic.
While it is true that bindings should try to be OS-agnostic, there is
the practical matter of where to put documentation so that it is widely
accessible. The Linux source tree is one of the most accessible things
there is, considering how widely it is replicated.
As the original instigator of the policy that the device tree should
describe the hardware OS-neutrally, I personally don't have a problem
with bindings referring to Linux documentation. I wouldn't like
references to proprietary and inaccessible documentation.
+Power Sequences Structure
+-
+Power sequences are sub-nodes that are named such as the device driver can
find
+them. The driver's documentation should list the sequence names it
recognizes.
That's a little roundabout. That might be better as simply:
Valid power sequence names are defined by each device's binding. For a
power sequence named foo, a node named foo-power-sequence defines
that sequence.
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result
in a
+parsing error.
Node names shouldn't be interpreted by the code; nodes should all be
named after the type of object the represent. Hence, every step should
be named just step for example.
The node's unit address (@0) should be used to distinguish the nodes.
This requires reg properties within each node to match the unit address,
and hence #address-cells and #size-cells properties in the power
sequence itself.
Note that this somewhat conflicts with accessing the top-level power
sequence by name too; perhaps that should be re-thought too. I must
admit this DT rule kinda sucks.
+Power Sequences Steps
+-
+Every step of a sequence describes an action to be performed on a resource.
It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type,
additional
+properties can be defined to control the action to be performed.
I think you need to add a property that indicates what type of resource
each step applies to. Sure, this is implicit in that if a gpio
property exists, the step is a GPIO step, but in order to make that
work, you'd have to search each node for each possible resource type's
property name. It'd be far better to read a single type=gpio property,
then parse the step based on that.
+Example
+---
+Here are example sequences declared within a backlight device that use all
the
+supported resources types:
+
+backlight {
+compatible = pwm-backlight;
+...
+
+/* resources used by the sequences */
+pwms = pwm 2 500;
+pwm-names = backlight;
+power-supply = backlight_reg;
+enable-gpio = gpio 28 0;
+
+power-on-sequence {
+step0 {
+regulator = power;
+enable;
+};
+step1 {
+delay = 1;
+};
+step2 {
+pwm = backlight;
+enable;
+};
+step3 {
+gpio = enable;
+enable;
+};
+
