diff mbox series

[3/3] pwm: Make capture support optional

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

Commit Message

Uwe Kleine-König May 23, 2022, 5:45 p.m. UTC
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(-)

Comments

Thierry Reding June 22, 2022, 1:46 p.m. UTC | #1
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
Uwe Kleine-König June 22, 2022, 5:09 p.m. UTC | #2
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
Uwe Kleine-König Sept. 30, 2022, 3:43 p.m. UTC | #3
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
diff mbox series

Patch

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);