| Message ID | 20220523174502.987113-3-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/3] pwm: Reorder header file to get rid of struct pwm_capture forward declaration | expand |
On Mon, May 23, 2022 at 07:45:02PM +0200, Uwe Kleine-König wrote: > The only code making use of the capture functionality is the sysfs code > in the PWM framework. I suspect there are no real users and would like to > deprecate it in favor of the counter framework. So introduce a kconfig > symbol to remove the capture support and make the sysfs file a stub. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/Kconfig | 12 ++++++++++++ > drivers/pwm/core.c | 3 ++- > drivers/pwm/pwm-sti.c | 4 ++++ > drivers/pwm/pwm-stm32.c | 4 ++++ > drivers/pwm/sysfs.c | 4 ++++ > include/linux/pwm.h | 5 +++++ > 6 files changed, 31 insertions(+), 1 deletion(-) I've applied patches 1-2 for now, but I'm not convinced about this yet. The PWM capture is something that's typically useful for applications served from userspace, which is why only the sysfs implementation exists. So anything that's based on another framework is likely not going to have in-kernel users either. Can you specify exactly how this alternative implementation would look like and how it would be an improvement over the current implementation? Thierry
Hello Thierry, On Wed, Jun 22, 2022 at 03:46:32PM +0200, Thierry Reding wrote: > On Mon, May 23, 2022 at 07:45:02PM +0200, Uwe Kleine-König wrote: > > The only code making use of the capture functionality is the sysfs code > > in the PWM framework. I suspect there are no real users and would like to > > deprecate it in favor of the counter framework. So introduce a kconfig > > symbol to remove the capture support and make the sysfs file a stub. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/Kconfig | 12 ++++++++++++ > > drivers/pwm/core.c | 3 ++- > > drivers/pwm/pwm-sti.c | 4 ++++ > > drivers/pwm/pwm-stm32.c | 4 ++++ > > drivers/pwm/sysfs.c | 4 ++++ > > include/linux/pwm.h | 5 +++++ > > 6 files changed, 31 insertions(+), 1 deletion(-) > > I've applied patches 1-2 for now, but I'm not convinced about this yet. > > The PWM capture is something that's typically useful for applications > served from userspace, which is why only the sysfs implementation > exists. So anything that's based on another framework is likely not > going to have in-kernel users either. Can you specify exactly how this > alternative implementation would look like and how it would be an > improvement over the current implementation? The counter framework would generate a continous stream of events while you measure and from the timestamps of the events you can determine period and duty cycle. So this is even more flexible because pwm-capture only supports one-shot mode while with the counter stuff you can stop to measure whenever you want to. Having said that, I didn't actually use the counter framework for something like that, but that's how I think it works and the framework has users. Other than that I have no better reasoning than the commit log. It's some time ago something happend in pwm that concerns the capture functionality[1] and the 13 new drivers since then all didn't implement capture support. Also the capture stuff was done by an ST employee for an ST driver, so that might not even be an active user but just a developer fulfilling a management roadmap such that the marketing department can advertise capture support. (Added Fabrice Gasnier to Cc:, maybe he will comment.) I don't know of any user of this, but of course I cannot rule out there are users I just don't know of. So the suggestion here looks reasonable to me: There is a Kconfig item now, people who don't use capture can disable it and the ones who rely on it set it =y. I expect that when this switch hits the distribution kernels it will initially be off. Then either people will wail to enable it. Or they don't and in a few years we can be even more convinced there are no active users. Best regards Uwe [1] ab3a89784783 ("pwm: stm32: Use input prescaler to improve period capture") from 2018
Hello Thierry, On Wed, Jun 22, 2022 at 07:09:45PM +0200, Uwe Kleine-König wrote: > On Wed, Jun 22, 2022 at 03:46:32PM +0200, Thierry Reding wrote: > > On Mon, May 23, 2022 at 07:45:02PM +0200, Uwe Kleine-König wrote: > > > The only code making use of the capture functionality is the sysfs code > > > in the PWM framework. I suspect there are no real users and would like to > > > deprecate it in favor of the counter framework. So introduce a kconfig > > > symbol to remove the capture support and make the sysfs file a stub. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/pwm/Kconfig | 12 ++++++++++++ > > > drivers/pwm/core.c | 3 ++- > > > drivers/pwm/pwm-sti.c | 4 ++++ > > > drivers/pwm/pwm-stm32.c | 4 ++++ > > > drivers/pwm/sysfs.c | 4 ++++ > > > include/linux/pwm.h | 5 +++++ > > > 6 files changed, 31 insertions(+), 1 deletion(-) > > > > I've applied patches 1-2 for now, but I'm not convinced about this yet. > > > > The PWM capture is something that's typically useful for applications > > served from userspace, which is why only the sysfs implementation > > exists. So anything that's based on another framework is likely not > > going to have in-kernel users either. Can you specify exactly how this > > alternative implementation would look like and how it would be an > > improvement over the current implementation? > > The counter framework would generate a continous stream of events while > you measure and from the timestamps of the events you can determine > period and duty cycle. So this is even more flexible because pwm-capture > only supports one-shot mode while with the counter stuff you can stop > to measure whenever you want to. Having said that, I didn't actually use > the counter framework for something like that, but that's how I think it > works and the framework has users. > > Other than that I have no better reasoning than the commit log. It's > some time ago something happend in pwm that concerns the capture > functionality[1] and the 13 new drivers since then all didn't implement > capture support. Also the capture stuff was done by an ST employee for > an ST driver, so that might not even be an active user but just a > developer fulfilling a management roadmap such that the marketing > department can advertise capture support. (Added Fabrice Gasnier to Cc:, > maybe he will comment.) > > I don't know of any user of this, but of course I cannot rule out there > are users I just don't know of. So the suggestion here looks reasonable > to me: There is a Kconfig item now, people who don't use capture can > disable it and the ones who rely on it set it =y. I expect that when > this switch hits the distribution kernels it will initially be off. Then > either people will wail to enable it. Or they don't and in a few years > we can be even more convinced there are no active users. You discarded this patch as "rejected" without any feedback to my explanation :-\ Do you think that capture support is such a vital part of the pwm framework that everyone who makes use of a PWM should also have the capture stuff even though only two drivers implement the needed callback and all drivers that were added in the last five years don't? Best regards Uwe
On Fri, Sep 30, 2022 at 05:43:55PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Wed, Jun 22, 2022 at 07:09:45PM +0200, Uwe Kleine-König wrote: > > On Wed, Jun 22, 2022 at 03:46:32PM +0200, Thierry Reding wrote: > > > On Mon, May 23, 2022 at 07:45:02PM +0200, Uwe Kleine-König wrote: > > > > The only code making use of the capture functionality is the sysfs code > > > > in the PWM framework. I suspect there are no real users and would like to > > > > deprecate it in favor of the counter framework. So introduce a kconfig > > > > symbol to remove the capture support and make the sysfs file a stub. > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > --- > > > > drivers/pwm/Kconfig | 12 ++++++++++++ > > > > drivers/pwm/core.c | 3 ++- > > > > drivers/pwm/pwm-sti.c | 4 ++++ > > > > drivers/pwm/pwm-stm32.c | 4 ++++ > > > > drivers/pwm/sysfs.c | 4 ++++ > > > > include/linux/pwm.h | 5 +++++ > > > > 6 files changed, 31 insertions(+), 1 deletion(-) > > > > > > I've applied patches 1-2 for now, but I'm not convinced about this yet. > > > > > > The PWM capture is something that's typically useful for applications > > > served from userspace, which is why only the sysfs implementation > > > exists. So anything that's based on another framework is likely not > > > going to have in-kernel users either. Can you specify exactly how this > > > alternative implementation would look like and how it would be an > > > improvement over the current implementation? > > > > The counter framework would generate a continous stream of events while > > you measure and from the timestamps of the events you can determine > > period and duty cycle. So this is even more flexible because pwm-capture > > only supports one-shot mode while with the counter stuff you can stop > > to measure whenever you want to. Having said that, I didn't actually use > > the counter framework for something like that, but that's how I think it > > works and the framework has users. > > > > Other than that I have no better reasoning than the commit log. It's > > some time ago something happend in pwm that concerns the capture > > functionality[1] and the 13 new drivers since then all didn't implement > > capture support. Also the capture stuff was done by an ST employee for > > an ST driver, so that might not even be an active user but just a > > developer fulfilling a management roadmap such that the marketing > > department can advertise capture support. (Added Fabrice Gasnier to Cc:, > > maybe he will comment.) > > > > I don't know of any user of this, but of course I cannot rule out there > > are users I just don't know of. So the suggestion here looks reasonable > > to me: There is a Kconfig item now, people who don't use capture can > > disable it and the ones who rely on it set it =y. I expect that when > > this switch hits the distribution kernels it will initially be off. Then > > either people will wail to enable it. Or they don't and in a few years > > we can be even more convinced there are no active users. > > You discarded this patch as "rejected" without any feedback to my > explanation :-\ Sorry, I hadn't realized that there was outstanding feedback on this. > Do you think that capture support is such a vital part of the pwm > framework that everyone who makes use of a PWM should also have the > capture stuff even though only two drivers implement the needed callback > and all drivers that were added in the last five years don't? Just because only two drivers support a feature doesn't automatically make it useless or non-vital. This feature was added because somebody needed it and I haven't received feedback that this is unused, so I see no reason why this should be removed. The overhead for drivers that don't use this is negligible. There's exactly one function pointer that would always be NULL for those. Then there's one sysfs attribute with associated code and one core function that's just a teeny tiny wrapper around the function pointer. On the other side of this we have an additional PWM_CAPTURE Kconfig symbol that introduces another combination that needs to be compile- tested for along with the potential to confuse users when they don't get capture support, etc. And that's just not worth it. If you really think that the counters framework is a better fit, the right way to deprecate this is to add support for equivalent capture functionality to that framework and get everyone to transition to that. Once that's done we can deprecate (and eventually perhaps even remove) the PWM capture stuff. Thierry
Hello Thierry, On Thu, Oct 06, 2022 at 03:10:13PM +0200, Thierry Reding wrote: > On Fri, Sep 30, 2022 at 05:43:55PM +0200, Uwe Kleine-König wrote: > > On Wed, Jun 22, 2022 at 07:09:45PM +0200, Uwe Kleine-König wrote: > > > On Wed, Jun 22, 2022 at 03:46:32PM +0200, Thierry Reding wrote: > > > > On Mon, May 23, 2022 at 07:45:02PM +0200, Uwe Kleine-König wrote: > > > > > The only code making use of the capture functionality is the sysfs code > > > > > in the PWM framework. I suspect there are no real users and would like to > > > > > deprecate it in favor of the counter framework. So introduce a kconfig > > > > > symbol to remove the capture support and make the sysfs file a stub. > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > --- > > > > > drivers/pwm/Kconfig | 12 ++++++++++++ > > > > > drivers/pwm/core.c | 3 ++- > > > > > drivers/pwm/pwm-sti.c | 4 ++++ > > > > > drivers/pwm/pwm-stm32.c | 4 ++++ > > > > > drivers/pwm/sysfs.c | 4 ++++ > > > > > include/linux/pwm.h | 5 +++++ > > > > > 6 files changed, 31 insertions(+), 1 deletion(-) > > > > > > > > I've applied patches 1-2 for now, but I'm not convinced about this yet. > > > > > > > > The PWM capture is something that's typically useful for applications > > > > served from userspace, which is why only the sysfs implementation > > > > exists. So anything that's based on another framework is likely not > > > > going to have in-kernel users either. Can you specify exactly how this > > > > alternative implementation would look like and how it would be an > > > > improvement over the current implementation? > > > > > > The counter framework would generate a continous stream of events while > > > you measure and from the timestamps of the events you can determine > > > period and duty cycle. So this is even more flexible because pwm-capture > > > only supports one-shot mode while with the counter stuff you can stop > > > to measure whenever you want to. Having said that, I didn't actually use > > > the counter framework for something like that, but that's how I think it > > > works and the framework has users. > > > > > > Other than that I have no better reasoning than the commit log. It's > > > some time ago something happend in pwm that concerns the capture > > > functionality[1] and the 13 new drivers since then all didn't implement > > > capture support. Also the capture stuff was done by an ST employee for > > > an ST driver, so that might not even be an active user but just a > > > developer fulfilling a management roadmap such that the marketing > > > department can advertise capture support. (Added Fabrice Gasnier to Cc:, > > > maybe he will comment.) > > > > > > I don't know of any user of this, but of course I cannot rule out there > > > are users I just don't know of. So the suggestion here looks reasonable > > > to me: There is a Kconfig item now, people who don't use capture can > > > disable it and the ones who rely on it set it =y. I expect that when > > > this switch hits the distribution kernels it will initially be off. Then > > > either people will wail to enable it. Or they don't and in a few years > > > we can be even more convinced there are no active users. > > > > You discarded this patch as "rejected" without any feedback to my > > explanation :-\ > > Sorry, I hadn't realized that there was outstanding feedback on this. > > > Do you think that capture support is such a vital part of the pwm > > framework that everyone who makes use of a PWM should also have the > > capture stuff even though only two drivers implement the needed callback > > and all drivers that were added in the last five years don't? > > Just because only two drivers support a feature doesn't automatically > make it useless or non-vital. Ack, but two drivers also don't automatically make it useful or vital. I never used it even tough I already used PWMs a lot. Did you ever use it? > This feature was added because somebody > needed it and I haven't received feedback that this is unused, so I So you expect people to tell you which part of the PWM framework they stopped using (or never used)? <sarcasm>Has anybody ever told you that they don't use CONFIG_PWM? Maybe we should compile it in unconditionally.</sarcasm> > see no reason why this should be removed. I don't want to remove it---at least for now. Just make it possible to disable this code for users who don't need it. (To pick up your line of reasoning: I haven't received feedback that it is used.) I'm happy about every unused feature I can disable, even if it only affects a single sysfs property. Code that is compiled out cannot trash cache lines or bring in runtime overhead or security problems without any gain. Yeah, this affects only very little code, but flipping a Kconfig switch is still easier than even to consider to review the capture stuff for unwanted effects. Given that more than 90% (99% ?) don't use capture, I do see some benefit. > The overhead for drivers that > don't use this is negligible. There's exactly one function pointer that > would always be NULL for those. Then there's one sysfs attribute with > associated code and one core function that's just a teeny tiny wrapper > around the function pointer. Agreed, the overhead is small. Still code that is hardly used, not even by the maintainers of the framework is a bad thing in my eyes. So being able to disable it (and ship kernels with that item disabled in distros) is a good way to find users who care, should they really exist. Either outcome is a win. Either we can get rid of an unused feature at some point in time, or we actually know there are users and we know their email address. > On the other side of this we have an additional PWM_CAPTURE Kconfig > symbol that introduces another combination that needs to be compile- > tested for along with the potential to confuse users when they don't > get capture support, etc. And that's just not worth it. My (subjective) judgement is obviously different. IMHO compile testing isn't a big issue. There are enough auto builders around to catch this and should really someone get it wrong, it's easy and trivial to fix. And if we had done this change 5 years ago, exactly zero patches since then would have had to care about this Kconfig symbol. > If you really think that the counters framework is a better fit, the > right way to deprecate this is to add support for equivalent capture > functionality to that framework and get everyone to transition to that. I claim everyone already transitioned. I cannot prove it as you cannot prove the opposite. Until we make it harder for people to use it. > Once that's done we can deprecate (and eventually perhaps even remove) > the PWM capture stuff. Best regards Uwe
On Thu, Oct 06, 2022 at 04:35:44PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Thu, Oct 06, 2022 at 03:10:13PM +0200, Thierry Reding wrote: > > On Fri, Sep 30, 2022 at 05:43:55PM +0200, Uwe Kleine-König wrote: > > > On Wed, Jun 22, 2022 at 07:09:45PM +0200, Uwe Kleine-König wrote: > > > > On Wed, Jun 22, 2022 at 03:46:32PM +0200, Thierry Reding wrote: > > > > > On Mon, May 23, 2022 at 07:45:02PM +0200, Uwe Kleine-König wrote: > > > > > > The only code making use of the capture functionality is the sysfs code > > > > > > in the PWM framework. I suspect there are no real users and would like to > > > > > > deprecate it in favor of the counter framework. So introduce a kconfig > > > > > > symbol to remove the capture support and make the sysfs file a stub. > > > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > --- > > > > > > drivers/pwm/Kconfig | 12 ++++++++++++ > > > > > > drivers/pwm/core.c | 3 ++- > > > > > > drivers/pwm/pwm-sti.c | 4 ++++ > > > > > > drivers/pwm/pwm-stm32.c | 4 ++++ > > > > > > drivers/pwm/sysfs.c | 4 ++++ > > > > > > include/linux/pwm.h | 5 +++++ > > > > > > 6 files changed, 31 insertions(+), 1 deletion(-) > > > > > > > > > > I've applied patches 1-2 for now, but I'm not convinced about this yet. > > > > > > > > > > The PWM capture is something that's typically useful for applications > > > > > served from userspace, which is why only the sysfs implementation > > > > > exists. So anything that's based on another framework is likely not > > > > > going to have in-kernel users either. Can you specify exactly how this > > > > > alternative implementation would look like and how it would be an > > > > > improvement over the current implementation? > > > > > > > > The counter framework would generate a continous stream of events while > > > > you measure and from the timestamps of the events you can determine > > > > period and duty cycle. So this is even more flexible because pwm-capture > > > > only supports one-shot mode while with the counter stuff you can stop > > > > to measure whenever you want to. Having said that, I didn't actually use > > > > the counter framework for something like that, but that's how I think it > > > > works and the framework has users. > > > > > > > > Other than that I have no better reasoning than the commit log. It's > > > > some time ago something happend in pwm that concerns the capture > > > > functionality[1] and the 13 new drivers since then all didn't implement > > > > capture support. Also the capture stuff was done by an ST employee for > > > > an ST driver, so that might not even be an active user but just a > > > > developer fulfilling a management roadmap such that the marketing > > > > department can advertise capture support. (Added Fabrice Gasnier to Cc:, > > > > maybe he will comment.) > > > > > > > > I don't know of any user of this, but of course I cannot rule out there > > > > are users I just don't know of. So the suggestion here looks reasonable > > > > to me: There is a Kconfig item now, people who don't use capture can > > > > disable it and the ones who rely on it set it =y. I expect that when > > > > this switch hits the distribution kernels it will initially be off. Then > > > > either people will wail to enable it. Or they don't and in a few years > > > > we can be even more convinced there are no active users. > > > > > > You discarded this patch as "rejected" without any feedback to my > > > explanation :-\ > > > > Sorry, I hadn't realized that there was outstanding feedback on this. > > > > > Do you think that capture support is such a vital part of the pwm > > > framework that everyone who makes use of a PWM should also have the > > > capture stuff even though only two drivers implement the needed callback > > > and all drivers that were added in the last five years don't? > > > > Just because only two drivers support a feature doesn't automatically > > make it useless or non-vital. > > Ack, but two drivers also don't automatically make it useful or vital. I > never used it even tough I already used PWMs a lot. Did you ever use it? No, I have never used it. But I don't see how that's relevant. > > This feature was added because somebody > > needed it and I haven't received feedback that this is unused, so I > > So you expect people to tell you which part of the PWM framework they > stopped using (or never used)? <sarcasm>Has anybody ever told you that > they don't use CONFIG_PWM? Maybe we should compile it in > unconditionally.</sarcasm> No, they don't need to tell me. This was merged because somebody needed it at some point, so I'm going to assume that it keeps being used. If we start removing existing features because we think they are no longer useful, we're going to end up annoying people and worst case they will be stuck on some old version of the kernel. > > see no reason why this should be removed. > > I don't want to remove it---at least for now. Just make it possible to > disable this code for users who don't need it. (To pick up your line of > reasoning: I haven't received feedback that it is used.) I'm happy about > every unused feature I can disable, even if it only affects a single > sysfs property. Code that is compiled out cannot trash cache lines or > bring in runtime overhead or security problems without any gain. Yeah, > this affects only very little code, but flipping a Kconfig switch is > still easier than even to consider to review the capture stuff for > unwanted effects. Given that more than 90% (99% ?) don't use capture, I > do see some benefit. There may be some benefit to disabling or removing this, but I don't think it outweighs the downsides. > > The overhead for drivers that > > don't use this is negligible. There's exactly one function pointer that > > would always be NULL for those. Then there's one sysfs attribute with > > associated code and one core function that's just a teeny tiny wrapper > > around the function pointer. > > Agreed, the overhead is small. Still code that is hardly used, not even > by the maintainers of the framework is a bad thing in my eyes. Heh... I don't think there's such a rule. Code doesn't have to be used by its maintainer for it to be part of the kernel. If you continue that reasoning, then anything that Linus doesn't use would have to be removed. > So being > able to disable it (and ship kernels with that item disabled in distros) > is a good way to find users who care, should they really exist. Either > outcome is a win. Either we can get rid of an unused feature at some > point in time, or we actually know there are users and we know their > email address. I would agree with you if this was somehow problematic to maintain or a burden in some way. It's really not, so adding the Kconfig knob, and in fact having this discussion, is way more time than I have spent on the capture feature in the last few years combined. > > On the other side of this we have an additional PWM_CAPTURE Kconfig > > symbol that introduces another combination that needs to be compile- > > tested for along with the potential to confuse users when they don't > > get capture support, etc. And that's just not worth it. > > My (subjective) judgement is obviously different. IMHO compile testing > isn't a big issue. There are enough auto builders around to catch this > and should really someone get it wrong, it's easy and trivial to fix. > And if we had done this change 5 years ago, exactly zero patches since > then would have had to care about this Kconfig symbol. Just as a reminder: the whole COMPILE_TEST business that people have sunk a significant amount of work into, has been to alleviate the pain that comes from too many Kconfig switches. Keeping things simple and unconditional in individual subsystems tremendously helps with keeping the combinatorial explosion in check. > > If you really think that the counters framework is a better fit, the > > right way to deprecate this is to add support for equivalent capture > > functionality to that framework and get everyone to transition to that. > > I claim everyone already transitioned. I cannot prove it as you cannot > prove the opposite. Until we make it harder for people to use it. I don't have to prove the opposite. Also, making it harder for people to use this isn't necessarily going to get us feedback. You assume that the people that would be inconvenienced by this getting disabled by default will be motivated enough to bring this up. I think it's at least equally likely that they'll get annoyed and stop updating their kernel or switch operating system to something with more sensible maintainers. Again, if this were somehow a significant impediment this might be a good idea. As it is, it is not. Sorry. Thierry
On Wed, Jun 22, 2022 at 07:09:45PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Wed, Jun 22, 2022 at 03:46:32PM +0200, Thierry Reding wrote: > > On Mon, May 23, 2022 at 07:45:02PM +0200, Uwe Kleine-König wrote: > > > The only code making use of the capture functionality is the sysfs code > > > in the PWM framework. I suspect there are no real users and would like to > > > deprecate it in favor of the counter framework. So introduce a kconfig > > > symbol to remove the capture support and make the sysfs file a stub. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/pwm/Kconfig | 12 ++++++++++++ > > > drivers/pwm/core.c | 3 ++- > > > drivers/pwm/pwm-sti.c | 4 ++++ > > > drivers/pwm/pwm-stm32.c | 4 ++++ > > > drivers/pwm/sysfs.c | 4 ++++ > > > include/linux/pwm.h | 5 +++++ > > > 6 files changed, 31 insertions(+), 1 deletion(-) > > > > I've applied patches 1-2 for now, but I'm not convinced about this yet. > > > > The PWM capture is something that's typically useful for applications > > served from userspace, which is why only the sysfs implementation > > exists. So anything that's based on another framework is likely not > > going to have in-kernel users either. Can you specify exactly how this > > alternative implementation would look like and how it would be an > > improvement over the current implementation? > > The counter framework would generate a continous stream of events while > you measure and from the timestamps of the events you can determine > period and duty cycle. So this is even more flexible because pwm-capture > only supports one-shot mode while with the counter stuff you can stop > to measure whenever you want to. Having said that, I didn't actually use > the counter framework for something like that, but that's how I think it > works and the framework has users. > > Other than that I have no better reasoning than the commit log. It's > some time ago something happend in pwm that concerns the capture > functionality[1] and the 13 new drivers since then all didn't implement > capture support. Also the capture stuff was done by an ST employee for > an ST driver, so that might not even be an active user but just a > developer fulfilling a management roadmap such that the marketing > department can advertise capture support. (Added Fabrice Gasnier to Cc:, > maybe he will comment.) > > I don't know of any user of this, but of course I cannot rule out there > are users I just don't know of. So the suggestion here looks reasonable > to me: There is a Kconfig item now, people who don't use capture can > disable it and the ones who rely on it set it =y. I expect that when > this switch hits the distribution kernels it will initially be off. Then > either people will wail to enable it. Or they don't and in a few years > we can be even more convinced there are no active users. > > Best regards > Uwe > > [1] ab3a89784783 ("pwm: stm32: Use input prescaler to improve period capture") from 2018 > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | Hi all, I want to give some context about the Counter subsystem as I think that will help in deciding whether to deprecate the existing PWM capture code. Support for PWM input capture was added in 2018 as part of the 4.18 merge window, but the Counter subsystem was added later in 2019. Although ST engineers involved with STM32 were at the center of discussions steering the requirements of the nascent Counter subsystem, the Counter events (capture) system just wasn't ready at the time the pwm-stm32 patch introducing capture support was accepted [0]. The Counter events system enables drivers to push device events on demand to a kfifo that userspace can pop via a respective character device node [1]. Each event is timestamped at the moment of capture, and userspace has the freedom to select what particular data should be captured (count value, signal value, supported extensions, etc.). The system really is quite flexible to what a particular user desires to capture from the device. As an example, suppose a Counter event is pushed to capture the current count value every time a change-of-state is detected for a signal. Each event will have a timestamp of when that change-of-state occurred. We get a time delta by comparing the timestamps between two events. Because events are in a kfifo, userspace can skip and combine events as desired. This is how you can determine period and duty cycle, among many other data computations. Of course there may be current users of the existing PWM capture system, so it shouldn't be removed outright. However, given that now the PWM capture system functionality is essentially provided by the Counter events system in a more flexible and extensible manner, I think it makes sense to deprecate support for PWM capture and encourage users to migrate to the Counter interface for such functionality. William Breathitt Gray [0] https://lore.kernel.org/r/1526456161-27865-4-git-send-email-fabrice.gasnier@st.com/ [1] https://docs.kernel.org/driver-api/generic-counter.html#counter-character-device
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 904de8d61828..0686dbb07fa5 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -42,6 +42,18 @@ config PWM_DEBUG It is expected to introduce some runtime overhead and diagnostic output to the kernel log, so only enable while working on a driver. +config PWM_CAPTURE + bool "PWM capture support" + help + A few drivers support capturing an input PWM signal. This is unused + by mainline drivers, and only exposed in sysfs. This doesn't match + the main purpose of the PWM framework and there is a superior + alternative (the counter framework) this might get removed at some + point. So if you need it, select Y here and speak up about your + use-case. + + If unsure, say no. + config PWM_AB8500 tristate "AB8500 PWM support" depends on AB8500_CORE && ARCH_U8500 diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index c7552df32082..7bc47f94685f 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -668,6 +668,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) } EXPORT_SYMBOL_GPL(pwm_apply_state); +#ifdef CONFIG_PWM_CAPTURE /** * pwm_capture() - capture and report a PWM signal * @pwm: PWM device @@ -693,7 +694,7 @@ int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, return err; } -EXPORT_SYMBOL_GPL(pwm_capture); +#endif /** * pwm_adjust_config() - adjust the current PWM config to the PWM arguments diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c index 44b1f93256b3..709dcf207291 100644 --- a/drivers/pwm/pwm-sti.c +++ b/drivers/pwm/pwm-sti.c @@ -309,6 +309,7 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) clear_bit(pwm->hwpwm, &pc->configured); } +#ifdef CONFIG_PWM_CAPTURE static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout) { @@ -390,6 +391,7 @@ static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm, mutex_unlock(&ddata->lock); return ret; } +#endif static int sti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) @@ -417,7 +419,9 @@ static int sti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, } static const struct pwm_ops sti_pwm_ops = { +#ifdef CONFIG_PWM_CAPTURE .capture = sti_pwm_capture, +#endif .apply = sti_pwm_apply, .free = sti_pwm_free, .owner = THIS_MODULE, diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c index 794ca5b02968..da192a32b570 100644 --- a/drivers/pwm/pwm-stm32.c +++ b/drivers/pwm/pwm-stm32.c @@ -67,6 +67,7 @@ static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value) return -EINVAL; } +#ifdef CONFIG_PWM_CAPTURE #define TIM_CCER_CC12P (TIM_CCER_CC1P | TIM_CCER_CC2P) #define TIM_CCER_CC12E (TIM_CCER_CC1E | TIM_CCER_CC2E) #define TIM_CCER_CC34P (TIM_CCER_CC3P | TIM_CCER_CC4P) @@ -318,6 +319,7 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm, return ret; } +#endif static int stm32_pwm_config(struct stm32_pwm *priv, int ch, int duty_ns, int period_ns) @@ -485,7 +487,9 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm, static const struct pwm_ops stm32pwm_ops = { .owner = THIS_MODULE, .apply = stm32_pwm_apply_locked, +#ifdef CONFIG_PWM_CAPTURE .capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL, +#endif }; static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 9903c3a7eced..b74b8140308f 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -204,6 +204,7 @@ static ssize_t capture_show(struct device *child, struct device_attribute *attr, char *buf) { +#ifdef CONFIG_PWM_CAPTURE struct pwm_device *pwm = child_to_pwm_device(child); struct pwm_capture result; int ret; @@ -213,6 +214,9 @@ static ssize_t capture_show(struct device *child, return ret; return sprintf(buf, "%u %u\n", result.period, result.duty_cycle); +#else + return -ENOSYS; +#endif } static DEVICE_ATTR_RW(period); diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 219db73956d1..d2d2be59beae 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -276,8 +276,10 @@ struct pwm_capture { struct pwm_ops { int (*request)(struct pwm_chip *chip, struct pwm_device *pwm); void (*free)(struct pwm_chip *chip, struct pwm_device *pwm); +#ifdef CONFIG_PWM_CAPTURE int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout); +#endif int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state); void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, @@ -395,8 +397,11 @@ static inline void pwm_disable(struct pwm_device *pwm) } /* PWM provider APIs */ +#ifdef CONFIG_PWM_CAPTURE int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout); +#endif + int pwm_set_chip_data(struct pwm_device *pwm, void *data); void *pwm_get_chip_data(struct pwm_device *pwm);
The only code making use of the capture functionality is the sysfs code in the PWM framework. I suspect there are no real users and would like to deprecate it in favor of the counter framework. So introduce a kconfig symbol to remove the capture support and make the sysfs file a stub. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/Kconfig | 12 ++++++++++++ drivers/pwm/core.c | 3 ++- drivers/pwm/pwm-sti.c | 4 ++++ drivers/pwm/pwm-stm32.c | 4 ++++ drivers/pwm/sysfs.c | 4 ++++ include/linux/pwm.h | 5 +++++ 6 files changed, 31 insertions(+), 1 deletion(-)