Re: [PATCH 3/3] pwm: Make capture support optional

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4728 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]