[PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

Some drivers require sleeping, for example if the pwm device is connected
over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
with the generated IR signal when sleeping occurs.

This patch makes it possible to use pwm when the driver does not sleep,
by introducing the pwm_can_sleep() function.

Signed-off-by: Sean Young <sean@mess.org>
Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 drivers/pwm/core.c               |  2 +-
 drivers/pwm/pwm-fsl-ftm.c        |  1 +
 drivers/pwm/pwm-imx-tpm.c        |  1 +
 drivers/pwm/pwm-iqs620a.c        |  1 +
 drivers/pwm/pwm-lpc18xx-sct.c    |  1 +
 drivers/pwm/pwm-microchip-core.c |  1 +
 drivers/pwm/pwm-omap-dmtimer.c   |  1 +
 drivers/pwm/pwm-pca9685.c        |  1 +
 drivers/pwm/pwm-renesas-tpu.c    |  1 -
 drivers/pwm/pwm-rz-mtu3.c        |  1 +
 drivers/pwm/pwm-sifive.c         |  1 +
 drivers/pwm/pwm-sti.c            |  1 +
 drivers/pwm/pwm-stm32.c          |  1 +
 drivers/pwm/pwm-twl-led.c        |  1 +
 drivers/pwm/pwm-twl.c            |  1 +
 include/linux/pwm.h              | 27 +++++++++++++++++++++++----
 16 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index dc66e3405bf5..d9679ae5b2be 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	 * is a bad idea. So make it explicit that calling this function might
 	 * sleep.
 	 */
-	might_sleep();
+	might_sleep_if(pwm_can_sleep(pwm));
 
 	if (!pwm || !state || !state->period ||
 	    state->duty_cycle > state->period)
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index b7c6045c5d08..b8b9392844e9 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 
 	fpc->soc = of_device_get_match_data(&pdev->dev);
 	fpc->chip.dev = &pdev->dev;
+	fpc->chip.can_sleep = true;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
index 98ab65c89685..6fd579089240 100644
--- a/drivers/pwm/pwm-imx-tpm.c
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -365,6 +365,7 @@ static int pwm_imx_tpm_probe(struct platform_device *pdev)
 
 	tpm->chip.dev = &pdev->dev;
 	tpm->chip.ops = &imx_tpm_pwm_ops;
+	tpm->chip.can_sleep = true;
 
 	/* get number of channels */
 	val = readl(tpm->base + PWM_IMX_TPM_PARAM);
diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
index 47b3141135f3..ebce9e06b32e 100644
--- a/drivers/pwm/pwm-iqs620a.c
+++ b/drivers/pwm/pwm-iqs620a.c
@@ -209,6 +209,7 @@ static int iqs620_pwm_probe(struct platform_device *pdev)
 	iqs620_pwm->chip.dev = &pdev->dev;
 	iqs620_pwm->chip.ops = &iqs620_pwm_ops;
 	iqs620_pwm->chip.npwm = 1;
+	iqs620_pwm->chip.can_sleep = true;
 
 	mutex_init(&iqs620_pwm->lock);
 
diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index 7a19a840bca5..e26fc18b5304 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -395,6 +395,7 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
 	lpc18xx_pwm->chip.dev = &pdev->dev;
 	lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
 	lpc18xx_pwm->chip.npwm = LPC18XX_NUM_PWMS;
+	lpc18xx_pwm->chip.can_sleep = true;
 
 	/* SCT counter must be in unify (32 bit) mode */
 	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c
index e7525c98105e..503b5b427d69 100644
--- a/drivers/pwm/pwm-microchip-core.c
+++ b/drivers/pwm/pwm-microchip-core.c
@@ -474,6 +474,7 @@ static int mchp_core_pwm_probe(struct platform_device *pdev)
 	mchp_core_pwm->chip.dev = &pdev->dev;
 	mchp_core_pwm->chip.ops = &mchp_core_pwm_ops;
 	mchp_core_pwm->chip.npwm = 16;
+	mchp_core_pwm->chip.can_sleep = true;
 
 	mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0));
 	mchp_core_pwm->channel_enabled |=
diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 4889fbd8a431..438abbb80daf 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -404,6 +404,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	omap->chip.dev = &pdev->dev;
 	omap->chip.ops = &pwm_omap_dmtimer_ops;
 	omap->chip.npwm = 1;
+	omap->chip.can_sleep = true;
 
 	mutex_init(&omap->mutex);
 
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 3038a68412a7..a47e21977e49 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -571,6 +571,7 @@ static int pca9685_pwm_probe(struct i2c_client *client)
 	pca->chip.npwm = PCA9685_MAXCHAN + 1;
 
 	pca->chip.dev = &client->dev;
+	pca->chip.can_sleep = true;
 
 	ret = pwmchip_add(&pca->chip);
 	if (ret < 0)
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index d7311614c846..96797a33d8c6 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -11,7 +11,6 @@
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
index a56cecb0e46e..6d874a2a8785 100644
--- a/drivers/pwm/pwm-rz-mtu3.c
+++ b/drivers/pwm/pwm-rz-mtu3.c
@@ -516,6 +516,7 @@ static int rz_mtu3_pwm_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 	rz_mtu3_pwm->chip.dev = &pdev->dev;
+	rz_mtu3_pwm->chip.can_sleep = true;
 	ret = devm_add_action_or_reset(&pdev->dev, rz_mtu3_pwm_pm_disable,
 				       rz_mtu3_pwm);
 	if (ret < 0)
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index eabddb7c7820..5677ed6eb4d5 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -240,6 +240,7 @@ static int pwm_sifive_probe(struct platform_device *pdev)
 	chip->dev = dev;
 	chip->ops = &pwm_sifive_ops;
 	chip->npwm = 4;
+	chip->can_sleep = true;
 
 	ddata->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(ddata->regs))
diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index b1d1373648a3..de9fbd570104 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -643,6 +643,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	pc->chip.dev = dev;
 	pc->chip.ops = &sti_pwm_ops;
 	pc->chip.npwm = pc->cdata->pwm_num_devs;
+	pc->chip.can_sleep = true;
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 3d6be7749e23..cd408158e55b 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -636,6 +636,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
 	priv->chip.dev = dev;
 	priv->chip.ops = &stm32pwm_ops;
 	priv->chip.npwm = stm32_pwm_detect_channels(priv);
+	priv->chip.can_sleep = true;
 
 	ret = devm_pwmchip_add(dev, &priv->chip);
 	if (ret < 0)
diff --git a/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
index 8fb84b441853..9e71429ecaed 100644
--- a/drivers/pwm/pwm-twl-led.c
+++ b/drivers/pwm/pwm-twl-led.c
@@ -362,6 +362,7 @@ static int twl_pwmled_probe(struct platform_device *pdev)
 	}
 
 	twl->chip.dev = &pdev->dev;
+	twl->chip.can_sleep = true;
 
 	mutex_init(&twl->mutex);
 
diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
index 86567add79db..bd08014fec44 100644
--- a/drivers/pwm/pwm-twl.c
+++ b/drivers/pwm/pwm-twl.c
@@ -356,6 +356,7 @@ static int twl_pwm_probe(struct platform_device *pdev)
 
 	twl->chip.dev = &pdev->dev;
 	twl->chip.npwm = 2;
+	twl->chip.can_sleep = true;
 
 	mutex_init(&twl->mutex);
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d2f9f690a9c1..c94894ffa4c4 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -287,6 +287,7 @@ struct pwm_ops {
  * @ops: callbacks for this PWM controller
  * @base: number of first PWM controlled by this chip
  * @npwm: number of PWMs controlled by this chip
+ * @can_sleep: can the driver sleep in pwm_apply_state
  * @of_xlate: request a PWM device given a device tree PWM specifier
  * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
  * @list: list node for internal use
@@ -297,6 +298,7 @@ struct pwm_chip {
 	const struct pwm_ops *ops;
 	int base;
 	unsigned int npwm;
+	bool can_sleep;
 
 	struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
 					const struct of_phandle_args *args);
@@ -380,6 +382,18 @@ static inline void pwm_disable(struct pwm_device *pwm)
 	pwm_apply_state(pwm, &state);
 }
 
+/**
+ * pwm_can_sleep() - can a pwm driver sleep in pwm_apply_state()
+ * @pwm: PWM device
+ *
+ * Returns: true if the driver may sleep, false if pwm_apply_state()
+ * can be called from atomic context.
+ */
+static inline bool pwm_can_sleep(struct pwm_device *pwm)
+{
+	return pwm->chip->can_sleep;
+}
+
 /* PWM provider APIs */
 int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 		unsigned long timeout);
@@ -411,7 +425,7 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 static inline int pwm_apply_state(struct pwm_device *pwm,
 				  const struct pwm_state *state)
 {
-	might_sleep();
+	might_sleep_if(pwm_can_sleep(pwm));
 	return -ENOTSUPP;
 }
 
@@ -423,19 +437,24 @@ static inline int pwm_adjust_config(struct pwm_device *pwm)
 static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
 			     int period_ns)
 {
-	might_sleep();
+	might_sleep_if(pwm_can_sleep(pwm));
 	return -EINVAL;
 }
 
 static inline int pwm_enable(struct pwm_device *pwm)
 {
-	might_sleep();
+	might_sleep_if(pwm_can_sleep(pwm));
 	return -EINVAL;
 }
 
 static inline void pwm_disable(struct pwm_device *pwm)
 {
-	might_sleep();
+	might_sleep_if(pwm_can_sleep(pwm));
+}
+
+static inline bool pwm_can_sleep(struct pwm_device *pwm)
+{
+	return true;
 }
 
 static inline int pwm_capture(struct pwm_device *pwm,
-- 
2.42.0

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[kernel test robot]

Hi Sean,

kernel test robot noticed the following build errors:

[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on shawnguo/for-next atorgue-stm32/stm32-next media-tree/master linus/master v6.6-rc3 next-20230929]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Young/media-pwm-ir-tx-trigger-edges-from-hrtimer-interrupt-context/20231001-194056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
patch link:    https://lore.kernel.org/r/1bd5241d584ceb4d6b731c4dc3203fb9686ee1d1.1696156485.git.sean%40mess.org
patch subject: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context
config: arm-randconfig-002-20231001 (https://download.01.org/0day-ci/archive/20231001/202310012229.ldJwkjOY-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231001/202310012229.ldJwkjOY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310012229.ldJwkjOY-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/cpumask.h:10,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/spinlock.h:63,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from include/linux/resource_ext.h:11,
                    from include/linux/acpi.h:13,
                    from include/linux/i2c.h:13,
                    from drivers/input/misc/da7280.c:12:
   include/linux/pwm.h: In function 'pwm_apply_state':
>> include/linux/pwm.h:428:24: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'cant_sleep'? [-Werror=implicit-function-declaration]
     428 |         might_sleep_if(pwm_can_sleep(pwm));
         |                        ^~~~~~~~~~~~~
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
     194 | #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
         |                                       ^~~~
   In file included from drivers/input/misc/da7280.c:16:
   include/linux/pwm.h: At top level:
>> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'; have 'bool(struct pwm_device *)' {aka '_Bool(struct pwm_device *)'}
     455 | static inline bool pwm_can_sleep(struct pwm_device *pwm)
         |                    ^~~~~~~~~~~~~
   include/linux/pwm.h:428:24: note: previous implicit declaration of 'pwm_can_sleep' with type 'int()'
     428 |         might_sleep_if(pwm_can_sleep(pwm));
         |                        ^~~~~~~~~~~~~
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
     194 | #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
         |                                       ^~~~
   cc1: some warnings being treated as errors


vim +428 include/linux/pwm.h

   419	
   420	struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
   421	struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
   422					       struct fwnode_handle *fwnode,
   423					       const char *con_id);
   424	#else
   425	static inline int pwm_apply_state(struct pwm_device *pwm,
   426					  const struct pwm_state *state)
   427	{
 > 428		might_sleep_if(pwm_can_sleep(pwm));
   429		return -ENOTSUPP;
   430	}
   431	
   432	static inline int pwm_adjust_config(struct pwm_device *pwm)
   433	{
   434		return -ENOTSUPP;
   435	}
   436	
   437	static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
   438				     int period_ns)
   439	{
   440		might_sleep_if(pwm_can_sleep(pwm));
   441		return -EINVAL;
   442	}
   443	
   444	static inline int pwm_enable(struct pwm_device *pwm)
   445	{
   446		might_sleep_if(pwm_can_sleep(pwm));
   447		return -EINVAL;
   448	}
   449	
   450	static inline void pwm_disable(struct pwm_device *pwm)
   451	{
   452		might_sleep_if(pwm_can_sleep(pwm));
   453	}
   454	
 > 455	static inline bool pwm_can_sleep(struct pwm_device *pwm)
   456	{
   457		return true;
   458	}
   459	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[kernel test robot]

Hi Sean,

kernel test robot noticed the following build errors:

[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on shawnguo/for-next atorgue-stm32/stm32-next media-tree/master linus/master v6.6-rc3 next-20230929]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Young/media-pwm-ir-tx-trigger-edges-from-hrtimer-interrupt-context/20231001-194056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
patch link:    https://lore.kernel.org/r/1bd5241d584ceb4d6b731c4dc3203fb9686ee1d1.1696156485.git.sean%40mess.org
patch subject: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context
config: i386-buildonly-randconfig-004-20231001 (https://download.01.org/0day-ci/archive/20231001/202310012348.puyNjoMk-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231001/202310012348.puyNjoMk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310012348.puyNjoMk-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/x86/include/asm/percpu.h:27:0,
                    from arch/x86/include/asm/current.h:10,
                    from include/linux/sched.h:12,
                    from include/linux/ratelimit.h:6,
                    from include/linux/dev_printk.h:16,
                    from include/linux/device.h:15,
                    from include/linux/backlight.h:12,
                    from drivers/video/fbdev/ssd1307fb.c:8:
   include/linux/pwm.h: In function 'pwm_apply_state':
   include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'gpiod_cansleep'? [-Werror=implicit-function-declaration]
     might_sleep_if(pwm_can_sleep(pwm));
                    ^
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
    #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
                                          ^~~~
   In file included from drivers/video/fbdev/ssd1307fb.c:16:0:
   include/linux/pwm.h: At top level:
>> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'
    static inline bool pwm_can_sleep(struct pwm_device *pwm)
                       ^~~~~~~~~~~~~
   In file included from arch/x86/include/asm/percpu.h:27:0,
                    from arch/x86/include/asm/current.h:10,
                    from include/linux/sched.h:12,
                    from include/linux/ratelimit.h:6,
                    from include/linux/dev_printk.h:16,
                    from include/linux/device.h:15,
                    from include/linux/backlight.h:12,
                    from drivers/video/fbdev/ssd1307fb.c:8:
   include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
     might_sleep_if(pwm_can_sleep(pwm));
                    ^
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
    #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
                                          ^~~~
   cc1: some warnings being treated as errors
--
   In file included from arch/x86/include/asm/percpu.h:27:0,
                    from arch/x86/include/asm/preempt.h:6,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from include/linux/resource_ext.h:11,
                    from include/linux/acpi.h:13,
                    from include/linux/i2c.h:13,
                    from drivers/mfd/intel_soc_pmic_crc.c:11:
   include/linux/pwm.h: In function 'pwm_apply_state':
   include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'cant_sleep'? [-Werror=implicit-function-declaration]
     might_sleep_if(pwm_can_sleep(pwm));
                    ^
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
    #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
                                          ^~~~
   In file included from drivers/mfd/intel_soc_pmic_crc.c:18:0:
   include/linux/pwm.h: At top level:
>> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'
    static inline bool pwm_can_sleep(struct pwm_device *pwm)
                       ^~~~~~~~~~~~~
   In file included from arch/x86/include/asm/percpu.h:27:0,
                    from arch/x86/include/asm/preempt.h:6,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from include/linux/resource_ext.h:11,
                    from include/linux/acpi.h:13,
                    from include/linux/i2c.h:13,
                    from drivers/mfd/intel_soc_pmic_crc.c:11:
   include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
     might_sleep_if(pwm_can_sleep(pwm));
                    ^
   include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
    #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
                                          ^~~~
   cc1: some warnings being treated as errors


vim +/pwm_can_sleep +455 include/linux/pwm.h

   454	
 > 455	static inline bool pwm_can_sleep(struct pwm_device *pwm)
   456	{
   457		return true;
   458	}
   459	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

On Mon, Oct 02, 2023 at 12:07:58AM +0800, kernel test robot wrote:
> Hi Sean,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on thierry-reding-pwm/for-next]
> [also build test ERROR on shawnguo/for-next atorgue-stm32/stm32-next media-tree/master linus/master v6.6-rc3 next-20230929]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]

There are indeed some configs in which there are build errors if
CONFIG_PWM is disabled, I'll fix it in the next version.

In the mean time it would be great to have some feedback on this patch,
and see if there is any other rework needed.

Thanks
Sean

> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Young/media-pwm-ir-tx-trigger-edges-from-hrtimer-interrupt-context/20231001-194056
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
> patch link:    https://lore.kernel.org/r/1bd5241d584ceb4d6b731c4dc3203fb9686ee1d1.1696156485.git.sean%40mess.org
> patch subject: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context
> config: i386-buildonly-randconfig-004-20231001 (https://download.01.org/0day-ci/archive/20231001/202310012348.puyNjoMk-lkp@intel.com/config)
> compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231001/202310012348.puyNjoMk-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202310012348.puyNjoMk-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from arch/x86/include/asm/percpu.h:27:0,
>                     from arch/x86/include/asm/current.h:10,
>                     from include/linux/sched.h:12,
>                     from include/linux/ratelimit.h:6,
>                     from include/linux/dev_printk.h:16,
>                     from include/linux/device.h:15,
>                     from include/linux/backlight.h:12,
>                     from drivers/video/fbdev/ssd1307fb.c:8:
>    include/linux/pwm.h: In function 'pwm_apply_state':
>    include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'gpiod_cansleep'? [-Werror=implicit-function-declaration]
>      might_sleep_if(pwm_can_sleep(pwm));
>                     ^
>    include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
>     #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
>                                           ^~~~
>    In file included from drivers/video/fbdev/ssd1307fb.c:16:0:
>    include/linux/pwm.h: At top level:
> >> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'
>     static inline bool pwm_can_sleep(struct pwm_device *pwm)
>                        ^~~~~~~~~~~~~
>    In file included from arch/x86/include/asm/percpu.h:27:0,
>                     from arch/x86/include/asm/current.h:10,
>                     from include/linux/sched.h:12,
>                     from include/linux/ratelimit.h:6,
>                     from include/linux/dev_printk.h:16,
>                     from include/linux/device.h:15,
>                     from include/linux/backlight.h:12,
>                     from drivers/video/fbdev/ssd1307fb.c:8:
>    include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
>      might_sleep_if(pwm_can_sleep(pwm));
>                     ^
>    include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
>     #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
>                                           ^~~~
>    cc1: some warnings being treated as errors
> --
>    In file included from arch/x86/include/asm/percpu.h:27:0,
>                     from arch/x86/include/asm/preempt.h:6,
>                     from include/linux/preempt.h:79,
>                     from include/linux/spinlock.h:56,
>                     from include/linux/mmzone.h:8,
>                     from include/linux/gfp.h:7,
>                     from include/linux/slab.h:16,
>                     from include/linux/resource_ext.h:11,
>                     from include/linux/acpi.h:13,
>                     from include/linux/i2c.h:13,
>                     from drivers/mfd/intel_soc_pmic_crc.c:11:
>    include/linux/pwm.h: In function 'pwm_apply_state':
>    include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'cant_sleep'? [-Werror=implicit-function-declaration]
>      might_sleep_if(pwm_can_sleep(pwm));
>                     ^
>    include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
>     #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
>                                           ^~~~
>    In file included from drivers/mfd/intel_soc_pmic_crc.c:18:0:
>    include/linux/pwm.h: At top level:
> >> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'
>     static inline bool pwm_can_sleep(struct pwm_device *pwm)
>                        ^~~~~~~~~~~~~
>    In file included from arch/x86/include/asm/percpu.h:27:0,
>                     from arch/x86/include/asm/preempt.h:6,
>                     from include/linux/preempt.h:79,
>                     from include/linux/spinlock.h:56,
>                     from include/linux/mmzone.h:8,
>                     from include/linux/gfp.h:7,
>                     from include/linux/slab.h:16,
>                     from include/linux/resource_ext.h:11,
>                     from include/linux/acpi.h:13,
>                     from include/linux/i2c.h:13,
>                     from drivers/mfd/intel_soc_pmic_crc.c:11:
>    include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
>      might_sleep_if(pwm_can_sleep(pwm));
>                     ^
>    include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
>     #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
>                                           ^~~~
>    cc1: some warnings being treated as errors
> 
> 
> vim +/pwm_can_sleep +455 include/linux/pwm.h
> 
>    454	
>  > 455	static inline bool pwm_can_sleep(struct pwm_device *pwm)
>    456	{
>    457		return true;
>    458	}
>    459	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[Uwe Kleine-König]

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

Hello Sean,

On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index dc66e3405bf5..d9679ae5b2be 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>  	 * is a bad idea. So make it explicit that calling this function might
>  	 * sleep.
>  	 */
> -	might_sleep();
> +	might_sleep_if(pwm_can_sleep(pwm));
>  
>  	if (!pwm || !state || !state->period ||
>  	    state->duty_cycle > state->period)

I'd like to have a mechanism to catch drivers that missed to set
.can_sleep. The best idea I currently have for that is to disable
preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while
.apply() is running.

> diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> index b7c6045c5d08..b8b9392844e9 100644
> --- a/drivers/pwm/pwm-fsl-ftm.c
> +++ b/drivers/pwm/pwm-fsl-ftm.c
> @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
>  
>  	fpc->soc = of_device_get_match_data(&pdev->dev);
>  	fpc->chip.dev = &pdev->dev;
> +	fpc->chip.can_sleep = true;

As .apply() being callable in non-sleepable context only depends on
.apply() I think a better place for this property is in struct pwm_ops.

Also I wonder if the distinction between atomic and sleeping
pwm_state_apply() should be more explicit. For GPIOs you have a sleeping
variant gpiod_set_value_cansleep() that allows to immediately determine
the intended context in the caller. This would allow that programming
a PWM stays a preemption point (if possible/desired) even if the
underlying hardware/driver is atomic. To not have to touch all consumer
drivers, maybe the pair for pwm should better be

	pwm_apply_state()
	pwm_apply_state_atomic()

instead of a "cansleep" suffix for the sleeping variant? Or maybe it's
better to accept touching all consumer drivers to get semantics similar
to gpio? I couldn't decide quickly what I really like better here, so
that's your chance to comment and influence the outcome :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

Hello Uwe,

On Wed, Oct 04, 2023 at 11:59:20AM +0200, Uwe Kleine-König wrote:
> On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index dc66e3405bf5..d9679ae5b2be 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> >  	 * is a bad idea. So make it explicit that calling this function might
> >  	 * sleep.
> >  	 */
> > -	might_sleep();
> > +	might_sleep_if(pwm_can_sleep(pwm));
> >  
> >  	if (!pwm || !state || !state->period ||
> >  	    state->duty_cycle > state->period)
> 
> I'd like to have a mechanism to catch drivers that missed to set
> .can_sleep. The best idea I currently have for that is to disable
> preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while
> .apply() is running.

If we have pwm_apply_state_atomic(), then CONFIG_DEBUG_ATOMIC_SLEEP will
catch them, but only in that code path of course.

How about using non_block_start() and non_block_end() if can_sleep is
not set?

> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> > index b7c6045c5d08..b8b9392844e9 100644
> > --- a/drivers/pwm/pwm-fsl-ftm.c
> > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> >  
> >  	fpc->soc = of_device_get_match_data(&pdev->dev);
> >  	fpc->chip.dev = &pdev->dev;
> > +	fpc->chip.can_sleep = true;
> 
> As .apply() being callable in non-sleepable context only depends on
> .apply() I think a better place for this property is in struct pwm_ops.

That makes sense.

> Also I wonder if the distinction between atomic and sleeping
> pwm_state_apply() should be more explicit. For GPIOs you have a sleeping
> variant gpiod_set_value_cansleep() that allows to immediately determine
> the intended context in the caller. This would allow that programming
> a PWM stays a preemption point (if possible/desired) even if the
> underlying hardware/driver is atomic. To not have to touch all consumer
> drivers, maybe the pair for pwm should better be
> 
> 	pwm_apply_state()
> 	pwm_apply_state_atomic()

Do we need pwm_config_atomic(), pwm_enable_atomic(), and pwm_disable_atomic()
too? These are just convenience functions, so we can probably do without them.

> instead of a "cansleep" suffix for the sleeping variant? Or maybe it's
> better to accept touching all consumer drivers to get semantics similar
> to gpio? I couldn't decide quickly what I really like better here, so
> that's your chance to comment and influence the outcome :-)

If you expect to have more parameters for pwm_apply_state() then a flags
argument makes sense.

TBH I like the pwm_apply_state_atomic() option.


Sean

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[Uwe Kleine-König]

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

Hello Sean,

On Thu, Oct 05, 2023 at 09:30:32AM +0100, Sean Young wrote:
> On Wed, Oct 04, 2023 at 11:59:20AM +0200, Uwe Kleine-König wrote:
> > On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index dc66e3405bf5..d9679ae5b2be 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > >  	 * is a bad idea. So make it explicit that calling this function might
> > >  	 * sleep.
> > >  	 */
> > > -	might_sleep();
> > > +	might_sleep_if(pwm_can_sleep(pwm));
> > >  
> > >  	if (!pwm || !state || !state->period ||
> > >  	    state->duty_cycle > state->period)
> > 
> > I'd like to have a mechanism to catch drivers that missed to set
> > .can_sleep. The best idea I currently have for that is to disable
> > preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while
> > .apply() is running.
> 
> If we have pwm_apply_state_atomic(), then CONFIG_DEBUG_ATOMIC_SLEEP will
> catch them, but only in that code path of course.
> 
> How about using non_block_start() and non_block_end() if can_sleep is
> not set?

TIL, looks like it was created for that task.

> > > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> > > index b7c6045c5d08..b8b9392844e9 100644
> > > --- a/drivers/pwm/pwm-fsl-ftm.c
> > > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > > @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> > >  
> > >  	fpc->soc = of_device_get_match_data(&pdev->dev);
> > >  	fpc->chip.dev = &pdev->dev;
> > > +	fpc->chip.can_sleep = true;
> > 
> > Also I wonder if the distinction between atomic and sleeping
> > pwm_state_apply() should be more explicit. For GPIOs you have a sleeping
> > variant gpiod_set_value_cansleep() that allows to immediately determine
> > the intended context in the caller. This would allow that programming
> > a PWM stays a preemption point (if possible/desired) even if the
> > underlying hardware/driver is atomic. To not have to touch all consumer
> > drivers, maybe the pair for pwm should better be
> > 
> > 	pwm_apply_state()
> > 	pwm_apply_state_atomic()
> 
> Do we need pwm_config_atomic(), pwm_enable_atomic(), and pwm_disable_atomic()
> too? These are just convenience functions, so we can probably do without them.

I'd like to get rid of these, so for now I'd keep them as is.

> > instead of a "cansleep" suffix for the sleeping variant? Or maybe it's
> > better to accept touching all consumer drivers to get semantics similar
> > to gpio? I couldn't decide quickly what I really like better here, so
> > that's your chance to comment and influence the outcome :-)
> 
> If you expect to have more parameters for pwm_apply_state() then a flags
> argument makes sense.

Actually I don't want more parameters -- at least for this use case. I
could imagine another parameter for something like apply-immediately vs.
complete-current-period, but that's another topic.

> TBH I like the pwm_apply_state_atomic() option.

ok.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[Thierry Reding]

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

On Wed, Oct 04, 2023 at 11:59:20AM +0200, Uwe Kleine-König wrote:
> Hello Sean,
> 
> On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index dc66e3405bf5..d9679ae5b2be 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> >  	 * is a bad idea. So make it explicit that calling this function might
> >  	 * sleep.
> >  	 */
> > -	might_sleep();
> > +	might_sleep_if(pwm_can_sleep(pwm));
> >  
> >  	if (!pwm || !state || !state->period ||
> >  	    state->duty_cycle > state->period)
> 
> I'd like to have a mechanism to catch drivers that missed to set
> .can_sleep. The best idea I currently have for that is to disable
> preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while
> .apply() is running.
> 
> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> > index b7c6045c5d08..b8b9392844e9 100644
> > --- a/drivers/pwm/pwm-fsl-ftm.c
> > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> >  
> >  	fpc->soc = of_device_get_match_data(&pdev->dev);
> >  	fpc->chip.dev = &pdev->dev;
> > +	fpc->chip.can_sleep = true;
> 
> As .apply() being callable in non-sleepable context only depends on
> .apply() I think a better place for this property is in struct pwm_ops.

What about drivers for devices that can be either sleeping or not? There
are things like regmap which can abstract those differences away, so you
could have a driver that works on both types of devices, so setting this
in ops wouldn't be correct all the time. I think can_sleep needs to be
per-chip rather than per-driver.

Thierry

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

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[Uwe Kleine-König]

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

Hello Thierry,

On Fri, Oct 06, 2023 at 12:27:51PM +0200, Thierry Reding wrote:
> On Wed, Oct 04, 2023 at 11:59:20AM +0200, Uwe Kleine-König wrote:
> > On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> > > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> > > index b7c6045c5d08..b8b9392844e9 100644
> > > --- a/drivers/pwm/pwm-fsl-ftm.c
> > > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > > @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> > >  
> > >  	fpc->soc = of_device_get_match_data(&pdev->dev);
> > >  	fpc->chip.dev = &pdev->dev;
> > > +	fpc->chip.can_sleep = true;
> > 
> > As .apply() being callable in non-sleepable context only depends on
> > .apply() I think a better place for this property is in struct pwm_ops.
> 
> What about drivers for devices that can be either sleeping or not? There
> are things like regmap which can abstract those differences away, so you
> could have a driver that works on both types of devices, so setting this
> in ops wouldn't be correct all the time. I think can_sleep needs to be
> per-chip rather than per-driver.

I would consider that a theoretic possibility. If there is a hardware
that has a (say) i2c and a memory-mapped register variant, I never
encountered such a thing. Hmm, the dwc driver seems to have a pci and a
memory-mapped variant, both would be "fast" though. (Wouldn't they?)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context

[Thierry Reding]

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

On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
[...]
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index d2f9f690a9c1..c94894ffa4c4 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -287,6 +287,7 @@ struct pwm_ops {
>   * @ops: callbacks for this PWM controller
>   * @base: number of first PWM controlled by this chip
>   * @npwm: number of PWMs controlled by this chip
> + * @can_sleep: can the driver sleep in pwm_apply_state
>   * @of_xlate: request a PWM device given a device tree PWM specifier
>   * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
>   * @list: list node for internal use
> @@ -297,6 +298,7 @@ struct pwm_chip {
>  	const struct pwm_ops *ops;
>  	int base;
>  	unsigned int npwm;
> +	bool can_sleep;

Can we please call this "might_sleep"?

>  
>  	struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
>  					const struct of_phandle_args *args);
> @@ -380,6 +382,18 @@ static inline void pwm_disable(struct pwm_device *pwm)
>  	pwm_apply_state(pwm, &state);
>  }
>  
> +/**
> + * pwm_can_sleep() - can a pwm driver sleep in pwm_apply_state()
> + * @pwm: PWM device
> + *
> + * Returns: true if the driver may sleep, false if pwm_apply_state()
> + * can be called from atomic context.
> + */
> +static inline bool pwm_can_sleep(struct pwm_device *pwm)

And this one pwm_might_sleep()? I don't see why we need to deviate from
the nomenclature that the core introduced.

Thierry

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

[PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Sean Young]

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
from delays as this is done in process context. Make this work in atomic
context.

This makes the driver much more precise.

Signed-off-by: Sean Young <sean@mess.org>
Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
 1 file changed, 63 insertions(+), 16 deletions(-)

diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index c5f37c03af9c..557725a07a67 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -10,6 +10,8 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/hrtimer.h>
+#include <linux/completion.h>
 #include <media/rc-core.h>
 
 #define DRIVER_NAME	"pwm-ir-tx"
@@ -17,8 +19,13 @@
 
 struct pwm_ir {
 	struct pwm_device *pwm;
-	unsigned int carrier;
-	unsigned int duty_cycle;
+	struct hrtimer timer;
+	struct completion completion;
+	uint carrier;
+	uint duty_cycle;
+	uint *txbuf;
+	uint txbuf_len;
+	uint txbuf_index;
 };
 
 static const struct of_device_id pwm_ir_of_match[] = {
@@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
 	struct pwm_ir *pwm_ir = dev->priv;
 	struct pwm_device *pwm = pwm_ir->pwm;
 	struct pwm_state state;
-	int i;
-	ktime_t edge;
-	long delta;
+
+	reinit_completion(&pwm_ir->completion);
 
 	pwm_init_state(pwm, &state);
 
 	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
 	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
+	state.enabled = false;
 
-	edge = ktime_get();
+	pwm_ir->txbuf = txbuf;
+	pwm_ir->txbuf_len = count;
+	pwm_ir->txbuf_index = 0;
 
-	for (i = 0; i < count; i++) {
-		state.enabled = !(i % 2);
-		pwm_apply_state(pwm, &state);
+	pwm_apply_state(pwm, &state);
 
-		edge = ktime_add_us(edge, txbuf[i]);
-		delta = ktime_us_delta(edge, ktime_get());
-		if (delta > 0)
-			usleep_range(delta, delta + 10);
-	}
+	hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);
 
-	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	wait_for_completion(&pwm_ir->completion);
 
 	return count;
 }
 
+static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */
+	do {
+		u64 ns;
+
+		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
+			/* Stop TX here */
+			pwm_disable(pwm_ir->pwm);
+
+			complete(&pwm_ir->completion);
+
+			return HRTIMER_NORESTART;
+		}
+
+		if (pwm_ir->txbuf_index % 2)
+			pwm_disable(pwm_ir->pwm);
+		else
+			pwm_enable(pwm_ir->pwm);
+
+		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
+		hrtimer_add_expires_ns(timer, ns);
+
+		pwm_ir->txbuf_index++;
+
+		now = timer->base->get_time();
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+}
+
 static int pwm_ir_probe(struct platform_device *pdev)
 {
 	struct pwm_ir *pwm_ir;
@@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
 	if (IS_ERR(pwm_ir->pwm))
 		return PTR_ERR(pwm_ir->pwm);
 
+	if (pwm_can_sleep(pwm_ir->pwm)) {
+		dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");
+		return -ENODEV;
+	}
+
 	pwm_ir->carrier = 38000;
 	pwm_ir->duty_cycle = 50;
+	init_completion(&pwm_ir->completion);
+	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	pwm_ir->timer.function = pwm_ir_timer;
 
 	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
 	if (!rcdev)
-- 
2.42.0

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Ivaylo Dimitrov]

Hi,

On 1.10.23 г. 13:40 ч., Sean Young wrote:
> The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
> from delays as this is done in process context. Make this work in atomic
> context.
> 
> This makes the driver much more precise.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
>   1 file changed, 63 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index c5f37c03af9c..557725a07a67 100644
> --- a/drivers/media/rc/pwm-ir-tx.c
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -10,6 +10,8 @@
>   #include <linux/slab.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
> +#include <linux/completion.h>
>   #include <media/rc-core.h>
>   
>   #define DRIVER_NAME	"pwm-ir-tx"
> @@ -17,8 +19,13 @@
>   
>   struct pwm_ir {
>   	struct pwm_device *pwm;
> -	unsigned int carrier;
> -	unsigned int duty_cycle;
> +	struct hrtimer timer;
> +	struct completion completion;
> +	uint carrier;
> +	uint duty_cycle;
> +	uint *txbuf;
> +	uint txbuf_len;
> +	uint txbuf_index;
>   };
>   
>   static const struct of_device_id pwm_ir_of_match[] = {
> @@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>   	struct pwm_ir *pwm_ir = dev->priv;
>   	struct pwm_device *pwm = pwm_ir->pwm;
>   	struct pwm_state state;
> -	int i;
> -	ktime_t edge;
> -	long delta;
> +
> +	reinit_completion(&pwm_ir->completion);

You should not need that.

>   
>   	pwm_init_state(pwm, &state);
>   
>   	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
>   	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
> +	state.enabled = false;
>   
> -	edge = ktime_get();
> +	pwm_ir->txbuf = txbuf;
> +	pwm_ir->txbuf_len = count;
> +	pwm_ir->txbuf_index = 0;
>   
> -	for (i = 0; i < count; i++) {
> -		state.enabled = !(i % 2);
> -		pwm_apply_state(pwm, &state);
> +	pwm_apply_state(pwm, &state);
>   

ditto, first pwm control should be in the timer function

> -		edge = ktime_add_us(edge, txbuf[i]);
> -		delta = ktime_us_delta(edge, ktime_get());
> -		if (delta > 0)
> -			usleep_range(delta, delta + 10);
> -	}
> +	hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);
>   

why not just call it with 0 time?

> -	state.enabled = false;
> -	pwm_apply_state(pwm, &state);
> +	wait_for_completion(&pwm_ir->completion);
>   
>   	return count;
>   }
>   
> +static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
> +{
> +	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
> +	ktime_t now;
> +
> +	/*
> +	 * If we happen to hit an odd latency spike, loop through the
> +	 * pulses until we catch up.
> +	 */
> +	do {
> +		u64 ns;
> +
> +		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
> +			/* Stop TX here */
> +			pwm_disable(pwm_ir->pwm);
> +
> +			complete(&pwm_ir->completion);
> +
> +			return HRTIMER_NORESTART;
> +		}
> +
> +		if (pwm_ir->txbuf_index % 2)
> +			pwm_disable(pwm_ir->pwm);
> +		else
> +			pwm_enable(pwm_ir->pwm);
> +

pwm_ir->pwm->state.enabled = !(pwm_ir->txbuf_index % 2);
pwm_apply_state(pwm_ir->pwm, pwm_ir->state);

> +		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
> +		hrtimer_add_expires_ns(timer, ns);
> +
> +		pwm_ir->txbuf_index++;
> +
> +		now = timer->base->get_time();
> +	} while (hrtimer_get_expires_tv64(timer) < now);
> +
> +	return HRTIMER_RESTART;
> +}
> +
>   static int pwm_ir_probe(struct platform_device *pdev)
>   {
>   	struct pwm_ir *pwm_ir;
> @@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
>   	if (IS_ERR(pwm_ir->pwm))
>   		return PTR_ERR(pwm_ir->pwm);
>   
> +	if (pwm_can_sleep(pwm_ir->pwm)) {
> +		dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");
> +		return -ENODEV;
> +	}
> +

I think we shall not limit, but use high priority thread to support 
those drivers. I have that working on n900 with current (sleeping) pwm, 
see my reply on the other mail. Maybe we can combine both patches in a 
way to support both atomic and sleeping pwm drivers.

>   	pwm_ir->carrier = 38000;
>   	pwm_ir->duty_cycle = 50;
> +	init_completion(&pwm_ir->completion);
> +	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	pwm_ir->timer.function = pwm_ir_timer;
>   
>   	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
>   	if (!rcdev)
> 

Regards,
Ivo

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Sean Young]

Hi,

On Mon, Oct 02, 2023 at 08:49:47AM +0300, Ivaylo Dimitrov wrote:
> On 1.10.23 г. 13:40 ч., Sean Young wrote:
> > The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
> > from delays as this is done in process context. Make this work in atomic
> > context.
> > 
> > This makes the driver much more precise.
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > ---
> >   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
> >   1 file changed, 63 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> > index c5f37c03af9c..557725a07a67 100644
> > --- a/drivers/media/rc/pwm-ir-tx.c
> > +++ b/drivers/media/rc/pwm-ir-tx.c
> > @@ -10,6 +10,8 @@
> >   #include <linux/slab.h>
> >   #include <linux/of.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/completion.h>
> >   #include <media/rc-core.h>
> >   #define DRIVER_NAME	"pwm-ir-tx"
> > @@ -17,8 +19,13 @@
> >   struct pwm_ir {
> >   	struct pwm_device *pwm;
> > -	unsigned int carrier;
> > -	unsigned int duty_cycle;
> > +	struct hrtimer timer;
> > +	struct completion completion;
> > +	uint carrier;
> > +	uint duty_cycle;
> > +	uint *txbuf;
> > +	uint txbuf_len;
> > +	uint txbuf_index;
> >   };
> >   static const struct of_device_id pwm_ir_of_match[] = {
> > @@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> >   	struct pwm_ir *pwm_ir = dev->priv;
> >   	struct pwm_device *pwm = pwm_ir->pwm;
> >   	struct pwm_state state;
> > -	int i;
> > -	ktime_t edge;
> > -	long delta;
> > +
> > +	reinit_completion(&pwm_ir->completion);
> 
> You should not need that.

It does not work without it - the process doing the 2nd tx hangs indefinitely.

> >   	pwm_init_state(pwm, &state);
> >   	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
> >   	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
> > +	state.enabled = false;
> > -	edge = ktime_get();
> > +	pwm_ir->txbuf = txbuf;
> > +	pwm_ir->txbuf_len = count;
> > +	pwm_ir->txbuf_index = 0;
> > -	for (i = 0; i < count; i++) {
> > -		state.enabled = !(i % 2);
> > -		pwm_apply_state(pwm, &state);
> > +	pwm_apply_state(pwm, &state);
> 
> ditto, first pwm control should be in the timer function

This requires keeping a copy of pwm_state in pwm_ir but does avoid the extra
call to pwm_apply_state() here.

Having said that, the extra call to pwm_apply_state() may have benefits,
see this comment in the pwm-sifive driver:

 * - When changing both duty cycle and period, we cannot prevent in
 *   software that the output might produce a period with mixed
 *   settings (new period length and old duty cycle).

So setting the duty cycle and period once with enabled = false prevents a
first period with mixed settings (i.e. bogus).

> > -		edge = ktime_add_us(edge, txbuf[i]);
> > -		delta = ktime_us_delta(edge, ktime_get());
> > -		if (delta > 0)
> > -			usleep_range(delta, delta + 10);
> > -	}
> > +	hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);
> 
> why not just call it with 0 time?

Otherwise the timings are a little off for the first edge - hrtimer setup
time, I think. I can experiment again.

> > -	state.enabled = false;
> > -	pwm_apply_state(pwm, &state);
> > +	wait_for_completion(&pwm_ir->completion);
> >   	return count;
> >   }
> > +static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
> > +{
> > +	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
> > +	ktime_t now;
> > +
> > +	/*
> > +	 * If we happen to hit an odd latency spike, loop through the
> > +	 * pulses until we catch up.
> > +	 */
> > +	do {
> > +		u64 ns;
> > +
> > +		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
> > +			/* Stop TX here */
> > +			pwm_disable(pwm_ir->pwm);
> > +
> > +			complete(&pwm_ir->completion);
> > +
> > +			return HRTIMER_NORESTART;
> > +		}
> > +
> > +		if (pwm_ir->txbuf_index % 2)
> > +			pwm_disable(pwm_ir->pwm);
> > +		else
> > +			pwm_enable(pwm_ir->pwm);
> > +
> 
> pwm_ir->pwm->state.enabled = !(pwm_ir->txbuf_index % 2);
> pwm_apply_state(pwm_ir->pwm, pwm_ir->state);

Requires a copy of pwm_state in pwm_ir, not a huge difference (copy of 28
bytes vs keeping it around).

> > +		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
> > +		hrtimer_add_expires_ns(timer, ns);
> > +
> > +		pwm_ir->txbuf_index++;
> > +
> > +		now = timer->base->get_time();
> > +	} while (hrtimer_get_expires_tv64(timer) < now);
> > +
> > +	return HRTIMER_RESTART;
> > +}
> > +
> >   static int pwm_ir_probe(struct platform_device *pdev)
> >   {
> >   	struct pwm_ir *pwm_ir;
> > @@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
> >   	if (IS_ERR(pwm_ir->pwm))
> >   		return PTR_ERR(pwm_ir->pwm);
> > +	if (pwm_can_sleep(pwm_ir->pwm)) {
> > +		dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");
> > +		return -ENODEV;
> > +	}
> > +
> 
> I think we shall not limit, but use high priority thread to support those
> drivers. I have that working on n900 with current (sleeping) pwm, see my
> reply on the other mail. Maybe we can combine both patches in a way to
> support both atomic and sleeping pwm drivers.

If the ir-rx51 driver uses a sleeping pwm then that's broken and only works
by accident - the current driver is broken then.

Spinning for longer periods (e.g. 100us) does not play well with RT. Would
make more sense to fix the pwm driver to non-sleeping when a pwm driver
is used for pwm-ir-tx?

Thanks

Sean

> 
> >   	pwm_ir->carrier = 38000;
> >   	pwm_ir->duty_cycle = 50;
> > +	init_completion(&pwm_ir->completion);
> > +	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +	pwm_ir->timer.function = pwm_ir_timer;
> >   	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
> >   	if (!rcdev)
> > 
> 
> Regards,
> Ivo

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Uwe Kleine-König]

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

Hello Sean,

On Mon, Oct 02, 2023 at 09:20:20AM +0100, Sean Young wrote:
> Having said that, the extra call to pwm_apply_state() may have benefits,
> see this comment in the pwm-sifive driver:
> 
>  * - When changing both duty cycle and period, we cannot prevent in
>  *   software that the output might produce a period with mixed
>  *   settings (new period length and old duty cycle).

You don't gain anything here I think. Disabling a PWM might also result
in a constant active level. If you want to prevent this, your best bet
is to never disable the PWM.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Ivaylo Dimitrov]

On 2.10.23 г. 11:20 ч., Sean Young wrote:
> Hi,
> 
> On Mon, Oct 02, 2023 at 08:49:47AM +0300, Ivaylo Dimitrov wrote:
>> On 1.10.23 г. 13:40 ч., Sean Young wrote:
>>> The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
>>> from delays as this is done in process context. Make this work in atomic
>>> context.
>>>
>>> This makes the driver much more precise.
>>>
>>> Signed-off-by: Sean Young <sean@mess.org>
>>> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>>> ---
>>>    drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
>>>    1 file changed, 63 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
>>> index c5f37c03af9c..557725a07a67 100644
>>> --- a/drivers/media/rc/pwm-ir-tx.c
>>> +++ b/drivers/media/rc/pwm-ir-tx.c
>>> @@ -10,6 +10,8 @@
>>>    #include <linux/slab.h>
>>>    #include <linux/of.h>
>>>    #include <linux/platform_device.h>
>>> +#include <linux/hrtimer.h>
>>> +#include <linux/completion.h>
>>>    #include <media/rc-core.h>
>>>    #define DRIVER_NAME	"pwm-ir-tx"
>>> @@ -17,8 +19,13 @@
>>>    struct pwm_ir {
>>>    	struct pwm_device *pwm;
>>> -	unsigned int carrier;
>>> -	unsigned int duty_cycle;
>>> +	struct hrtimer timer;
>>> +	struct completion completion;
>>> +	uint carrier;
>>> +	uint duty_cycle;
>>> +	uint *txbuf;
>>> +	uint txbuf_len;
>>> +	uint txbuf_index;
>>>    };
>>>    static const struct of_device_id pwm_ir_of_match[] = {
>>> @@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>>>    	struct pwm_ir *pwm_ir = dev->priv;
>>>    	struct pwm_device *pwm = pwm_ir->pwm;
>>>    	struct pwm_state state;
>>> -	int i;
>>> -	ktime_t edge;
>>> -	long delta;
>>> +
>>> +	reinit_completion(&pwm_ir->completion);
>>
>> You should not need that.
> 
> It does not work without it - the process doing the 2nd tx hangs indefinitely.
> 

that means your calls to wait_for_completion() / complete() do not 
match. I think you should check why.

>>>    	pwm_init_state(pwm, &state);
>>>    	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
>>>    	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
>>> +	state.enabled = false;
>>> -	edge = ktime_get();
>>> +	pwm_ir->txbuf = txbuf;
>>> +	pwm_ir->txbuf_len = count;
>>> +	pwm_ir->txbuf_index = 0;
>>> -	for (i = 0; i < count; i++) {
>>> -		state.enabled = !(i % 2);
>>> -		pwm_apply_state(pwm, &state);
>>> +	pwm_apply_state(pwm, &state);
>>
>> ditto, first pwm control should be in the timer function
> 
> This requires keeping a copy of pwm_state in pwm_ir but does avoid the extra
> call to pwm_apply_state() here.
> 

not really, you can have pwm_state * pwm_ir member and pass pointer to 
the stack variable.

> Having said that, the extra call to pwm_apply_state() may have benefits,
> see this comment in the pwm-sifive driver:
> 
>   * - When changing both duty cycle and period, we cannot prevent in
>   *   software that the output might produce a period with mixed
>   *   settings (new period length and old duty cycle).
> 
> So setting the duty cycle and period once with enabled = false prevents a
> first period with mixed settings (i.e. bogus).
> 

Who will enable pwm if not in tx? Like, doesn't the driver have 
exclusive ownership of the pwm? Also, every transmission ends up wit pwm 
disabled, so disabling it once again does not make sense to me.


>>> -		edge = ktime_add_us(edge, txbuf[i]);
>>> -		delta = ktime_us_delta(edge, ktime_get());
>>> -		if (delta > 0)
>>> -			usleep_range(delta, delta + 10);
>>> -	}
>>> +	hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);
>>
>> why not just call it with 0 time?
> 
> Otherwise the timings are a little off for the first edge - hrtimer setup
> time, I think. I can experiment again.
> 

Why is that? Edge start is controlled by the calls in timer function, it 
should not matter when it is called for the first time.

>>> -	state.enabled = false;
>>> -	pwm_apply_state(pwm, &state);
>>> +	wait_for_completion(&pwm_ir->completion);
>>>    	return count;
>>>    }
>>> +static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
>>> +{
>>> +	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
>>> +	ktime_t now;
>>> +
>>> +	/*
>>> +	 * If we happen to hit an odd latency spike, loop through the
>>> +	 * pulses until we catch up.
>>> +	 */
>>> +	do {
>>> +		u64 ns;
>>> +
>>> +		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
>>> +			/* Stop TX here */
>>> +			pwm_disable(pwm_ir->pwm);
>>> +
>>> +			complete(&pwm_ir->completion);
>>> +
>>> +			return HRTIMER_NORESTART;
>>> +		}
>>> +
>>> +		if (pwm_ir->txbuf_index % 2)
>>> +			pwm_disable(pwm_ir->pwm);
>>> +		else
>>> +			pwm_enable(pwm_ir->pwm);
>>> +
>>
>> pwm_ir->pwm->state.enabled = !(pwm_ir->txbuf_index % 2);
>> pwm_apply_state(pwm_ir->pwm, pwm_ir->state);
> 
> Requires a copy of pwm_state in pwm_ir, not a huge difference (copy of 28
> bytes vs keeping it around).

see my previous comment re struct var. Also, look at the overhead: 
https://elixir.bootlin.com/linux/v6.6-rc3/source/include/linux/pwm.h#L349 
- you call pwm_get_state() for every edge.

> 
>>> +		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
>>> +		hrtimer_add_expires_ns(timer, ns);
>>> +
>>> +		pwm_ir->txbuf_index++;
>>> +
>>> +		now = timer->base->get_time();
>>> +	} while (hrtimer_get_expires_tv64(timer) < now);
>>> +
>>> +	return HRTIMER_RESTART;
>>> +}
>>> +
>>>    static int pwm_ir_probe(struct platform_device *pdev)
>>>    {
>>>    	struct pwm_ir *pwm_ir;
>>> @@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
>>>    	if (IS_ERR(pwm_ir->pwm))
>>>    		return PTR_ERR(pwm_ir->pwm);
>>> +	if (pwm_can_sleep(pwm_ir->pwm)) {
>>> +		dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>
>> I think we shall not limit, but use high priority thread to support those
>> drivers. I have that working on n900 with current (sleeping) pwm, see my
>> reply on the other mail. Maybe we can combine both patches in a way to
>> support both atomic and sleeping pwm drivers.
> 
> If the ir-rx51 driver uses a sleeping pwm then that's broken and only works
> by accident - the current driver is broken then.
> 

Yes, and I stated that couple of times in my previous emails :)

> Spinning for longer periods (e.g. 100us) does not play well with RT. Would
> make more sense to fix the pwm driver to non-sleeping when a pwm driver
> is used for pwm-ir-tx?
> 

Sure, and I have a patch for n900 that does this, however, for your i2c 
case there is no solution. Also, we may play smart and dynamically 
decrease sleep time (by adjusting edge by lets say 5-10 us every pulse 
until we have some sane value) if we see it is too long. No strong 
preferences here, it is just that I have code that works.

Thanks,
Ivo

> Thanks
> 
> Sean
> 
>>
>>>    	pwm_ir->carrier = 38000;
>>>    	pwm_ir->duty_cycle = 50;
>>> +	init_completion(&pwm_ir->completion);
>>> +	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>>> +	pwm_ir->timer.function = pwm_ir_timer;
>>>    	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
>>>    	if (!rcdev)
>>>
>>
>> Regards,
>> Ivo

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Sean Young]

Hi,

On Mon, Oct 02, 2023 at 12:52:00PM +0300, Ivaylo Dimitrov wrote:
> On 2.10.23 г. 11:20 ч., Sean Young wrote:
> > On Mon, Oct 02, 2023 at 08:49:47AM +0300, Ivaylo Dimitrov wrote:
> > > On 1.10.23 г. 13:40 ч., Sean Young wrote:
> > > > The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
> > > > from delays as this is done in process context. Make this work in atomic
> > > > context.
> > > > 
> > > > This makes the driver much more precise.
> > > > 
> > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > > ---
> > > >    drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
> > > >    1 file changed, 63 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> > > > index c5f37c03af9c..557725a07a67 100644
> > > > --- a/drivers/media/rc/pwm-ir-tx.c
> > > > +++ b/drivers/media/rc/pwm-ir-tx.c
> > > > @@ -10,6 +10,8 @@
> > > >    #include <linux/slab.h>
> > > >    #include <linux/of.h>
> > > >    #include <linux/platform_device.h>
> > > > +#include <linux/hrtimer.h>
> > > > +#include <linux/completion.h>
> > > >    #include <media/rc-core.h>
> > > >    #define DRIVER_NAME	"pwm-ir-tx"
> > > > @@ -17,8 +19,13 @@
> > > >    struct pwm_ir {
> > > >    	struct pwm_device *pwm;
> > > > -	unsigned int carrier;
> > > > -	unsigned int duty_cycle;
> > > > +	struct hrtimer timer;
> > > > +	struct completion completion;
> > > > +	uint carrier;
> > > > +	uint duty_cycle;
> > > > +	uint *txbuf;
> > > > +	uint txbuf_len;
> > > > +	uint txbuf_index;
> > > >    };
> > > >    static const struct of_device_id pwm_ir_of_match[] = {
> > > > @@ -55,33 +62,65 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> > > >    	struct pwm_ir *pwm_ir = dev->priv;
> > > >    	struct pwm_device *pwm = pwm_ir->pwm;
> > > >    	struct pwm_state state;
> > > > -	int i;
> > > > -	ktime_t edge;
> > > > -	long delta;
> > > > +
> > > > +	reinit_completion(&pwm_ir->completion);
> > > 
> > > You should not need that.
> > 
> > It does not work without it - the process doing the 2nd tx hangs indefinitely.
> > 
> 
> that means your calls to wait_for_completion() / complete() do not match. I
> think you should check why.

I've had a deeper look and you're right. Thanks!

> > > >    	pwm_init_state(pwm, &state);
> > > >    	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
> > > >    	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
> > > > +	state.enabled = false;
> > > > -	edge = ktime_get();
> > > > +	pwm_ir->txbuf = txbuf;
> > > > +	pwm_ir->txbuf_len = count;
> > > > +	pwm_ir->txbuf_index = 0;
> > > > -	for (i = 0; i < count; i++) {
> > > > -		state.enabled = !(i % 2);
> > > > -		pwm_apply_state(pwm, &state);
> > > > +	pwm_apply_state(pwm, &state);
> > > 
> > > ditto, first pwm control should be in the timer function
> > 
> > This requires keeping a copy of pwm_state in pwm_ir but does avoid the extra
> > call to pwm_apply_state() here.
> > 
> 
> not really, you can have pwm_state * pwm_ir member and pass pointer to the
> stack variable.

Ah, good point, I had not thought of that :)

> > Having said that, the extra call to pwm_apply_state() may have benefits,
> > see this comment in the pwm-sifive driver:
> > 
> >   * - When changing both duty cycle and period, we cannot prevent in
> >   *   software that the output might produce a period with mixed
> >   *   settings (new period length and old duty cycle).
> > 
> > So setting the duty cycle and period once with enabled = false prevents a
> > first period with mixed settings (i.e. bogus).
> > 
> 
> Who will enable pwm if not in tx? Like, doesn't the driver have exclusive
> ownership of the pwm? Also, every transmission ends up wit pwm disabled, so
> disabling it once again does not make sense to me.

My only point was that if the period/duty cycle have changed, then the extra
disable may set up the parameters in the pwm hardware correctly (according
to this comment). Uwe has already responded saying this is not going to work,
so this can be ignored.

> > > > -		edge = ktime_add_us(edge, txbuf[i]);
> > > > -		delta = ktime_us_delta(edge, ktime_get());
> > > > -		if (delta > 0)
> > > > -			usleep_range(delta, delta + 10);
> > > > -	}
> > > > +	hrtimer_start(&pwm_ir->timer, 1000, HRTIMER_MODE_REL);
> > > 
> > > why not just call it with 0 time?
> > 
> > Otherwise the timings are a little off for the first edge - hrtimer setup
> > time, I think. I can experiment again.
> > 
> 
> Why is that? Edge start is controlled by the calls in timer function, it
> should not matter when it is called for the first time.

Again, you're right.

> > > > -	state.enabled = false;
> > > > -	pwm_apply_state(pwm, &state);
> > > > +	wait_for_completion(&pwm_ir->completion);
> > > >    	return count;
> > > >    }
> > > > +static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
> > > > +{
> > > > +	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
> > > > +	ktime_t now;
> > > > +
> > > > +	/*
> > > > +	 * If we happen to hit an odd latency spike, loop through the
> > > > +	 * pulses until we catch up.
> > > > +	 */
> > > > +	do {
> > > > +		u64 ns;
> > > > +
> > > > +		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
> > > > +			/* Stop TX here */
> > > > +			pwm_disable(pwm_ir->pwm);
> > > > +
> > > > +			complete(&pwm_ir->completion);
> > > > +
> > > > +			return HRTIMER_NORESTART;
> > > > +		}
> > > > +
> > > > +		if (pwm_ir->txbuf_index % 2)
> > > > +			pwm_disable(pwm_ir->pwm);
> > > > +		else
> > > > +			pwm_enable(pwm_ir->pwm);
> > > > +
> > > 
> > > pwm_ir->pwm->state.enabled = !(pwm_ir->txbuf_index % 2);
> > > pwm_apply_state(pwm_ir->pwm, pwm_ir->state);
> > 
> > Requires a copy of pwm_state in pwm_ir, not a huge difference (copy of 28
> > bytes vs keeping it around).
> 
> see my previous comment re struct var. Also, look at the overhead:
> https://elixir.bootlin.com/linux/v6.6-rc3/source/include/linux/pwm.h#L349 -
> you call pwm_get_state() for every edge.

That's the 28 bytes copy I was talking about.

However keeping a pointer in struct pwm_ir is a good compromise and makes
the rest of the code cleaner.

> > > > +		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
> > > > +		hrtimer_add_expires_ns(timer, ns);
> > > > +
> > > > +		pwm_ir->txbuf_index++;
> > > > +
> > > > +		now = timer->base->get_time();
> > > > +	} while (hrtimer_get_expires_tv64(timer) < now);
> > > > +
> > > > +	return HRTIMER_RESTART;
> > > > +}
> > > > +
> > > >    static int pwm_ir_probe(struct platform_device *pdev)
> > > >    {
> > > >    	struct pwm_ir *pwm_ir;
> > > > @@ -96,8 +135,16 @@ static int pwm_ir_probe(struct platform_device *pdev)
> > > >    	if (IS_ERR(pwm_ir->pwm))
> > > >    		return PTR_ERR(pwm_ir->pwm);
> > > > +	if (pwm_can_sleep(pwm_ir->pwm)) {
> > > > +		dev_err(&pdev->dev, "unsupported pwm device: driver can sleep\n");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > 
> > > I think we shall not limit, but use high priority thread to support those
> > > drivers. I have that working on n900 with current (sleeping) pwm, see my
> > > reply on the other mail. Maybe we can combine both patches in a way to
> > > support both atomic and sleeping pwm drivers.
> > 
> > If the ir-rx51 driver uses a sleeping pwm then that's broken and only works
> > by accident - the current driver is broken then.
> > 
> 
> Yes, and I stated that couple of times in my previous emails :)
> 
> > Spinning for longer periods (e.g. 100us) does not play well with RT. Would
> > make more sense to fix the pwm driver to non-sleeping when a pwm driver
> > is used for pwm-ir-tx?
> > 
> 
> Sure, and I have a patch for n900 that does this, however, for your i2c case
> there is no solution. Also, we may play smart and dynamically decrease sleep
> time (by adjusting edge by lets say 5-10 us every pulse until we have some
> sane value) if we see it is too long. No strong preferences here, it is just
> that I have code that works.

So in order to get everything in order for atomic pwm, we need a few patches
to land. Let's try to get your patch merged for the next release cycle, so
that at least tx on the n900 works and there is improved tx for other devices
(although not the most efficient).

Thanks,
Sean

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Uwe Kleine-König]

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

Hello,

On Wed, Oct 04, 2023 at 08:43:53AM +0100, Sean Young wrote:
> On Mon, Oct 02, 2023 at 12:52:00PM +0300, Ivaylo Dimitrov wrote:
> > On 2.10.23 г. 11:20 ч., Sean Young wrote:
> > > Requires a copy of pwm_state in pwm_ir, not a huge difference (copy of 28
> > > bytes vs keeping it around).
> > 
> > see my previous comment re struct var. Also, look at the overhead:
> > https://elixir.bootlin.com/linux/v6.6-rc3/source/include/linux/pwm.h#L349 -
> > you call pwm_get_state() for every edge.
> 
> That's the 28 bytes copy I was talking about.

Note that pwm_get_state() also has (IMHO) confusing semantics. It gives
you (most of the time) the state that was last pwm_state_apply()d and
not the state the hardware is currently in. In my book keeping the
pwm_state around is the nicer approach that often is also simpler ...

> However keeping a pointer in struct pwm_ir is a good compromise and makes
> the rest of the code cleaner.

... which seems to apply here, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Ivaylo Dimitrov]

Hi,

On 1.10.23 г. 13:40 ч., Sean Young wrote:
> The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
> from delays as this is done in process context. Make this work in atomic
> context.
> 
> This makes the driver much more precise.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
>   1 file changed, 63 insertions(+), 16 deletions(-)
> 

what about the following patch(not a proper one, just RFC)? It achieves 
the same (if not better) precision (on n900 at least) without using 
atomic pwm. What it does is: create a fifo thread in which we swicth pwm 
on/off, start hrtimer that is used to signal thread when to switch pwm.
As signal comes earlier than needed(because hrtimer runs async to the 
thread), we do a busy loop wait for the precise time to switch the pwm. 
At least on n900, this busy loop is less than 200 us per switch(worst 
case, usually it is less than 100 us). That way, even if we have some 
latency spike, it is covered by not doing busy loop for that particular 
pwm switch and we keep the precise timing.

Maybe we shall merge both patches so fifo thread to be used for sleeping 
pwms and hrtimer for atomic. I can do that and test it here if you think 
that approach makes sense.

Regards,
Ivo

PS: I have a patch that converts timer-ti-dm to non-sleeping one, will 
send it when it comes to it.

diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index 105a9c24f1e3..e8b620f53056 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -4,6 +4,7 @@
   */

  #include <linux/kernel.h>
+#include <linux/kthread.h>
  #include <linux/module.h>
  #include <linux/pwm.h>
  #include <linux/delay.h>
@@ -17,8 +18,16 @@

  struct pwm_ir {
  	struct pwm_device *pwm;
+	struct hrtimer timer;
+	struct task_struct *tx_thread;
+	wait_queue_head_t tx_wq;
+	struct completion tx_done;
+	struct completion edge;
  	unsigned int carrier;
  	unsigned int duty_cycle;
+	unsigned int *txbuf;
+	unsigned int count;
+	unsigned int index;
  };

  static const struct of_device_id pwm_ir_of_match[] = {
@@ -48,35 +57,103 @@ static int pwm_ir_set_carrier(struct rc_dev *dev, 
u32 carrier)
  	return 0;
  }

-static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
-		     unsigned int count)
+static enum hrtimer_restart pwm_ir_timer_cb(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */
+	do {
+		u64 edge;
+
+		if (pwm_ir->index >= pwm_ir->count)
+			goto out;
+
+		complete(&pwm_ir->edge);
+
+		edge = US_TO_NS(pwm_ir->txbuf[pwm_ir->index]);
+		hrtimer_add_expires_ns(timer, edge);
+
+		pwm_ir->index++;
+
+		now = timer->base->get_time();
+
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+out:
+	complete(&pwm_ir->edge);
+
+	return HRTIMER_NORESTART;
+}
+
+static void _pwm_ir_tx(struct pwm_ir *pwm_ir)
  {
-	struct pwm_ir *pwm_ir = dev->priv;
-	struct pwm_device *pwm = pwm_ir->pwm;
  	struct pwm_state state;
  	int i;
  	ktime_t edge;
  	long delta;

-	pwm_init_state(pwm, &state);
+	pwm_init_state(pwm_ir->pwm, &state);

  	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
  	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);

+	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
+	wait_for_completion(&pwm_ir->edge);
  	edge = ktime_get();

-	for (i = 0; i < count; i++) {
+	for (i = 0; i < pwm_ir->count; i++) {
  		state.enabled = !(i % 2);
-		pwm_apply_state(pwm, &state);
+		pwm_apply_state(pwm_ir->pwm, &state);
+
+		edge = ktime_add_us(edge, pwm_ir->txbuf[i]);
+		wait_for_completion(&pwm_ir->edge);

-		edge = ktime_add_us(edge, txbuf[i]);
  		delta = ktime_us_delta(edge, ktime_get());
+
  		if (delta > 0)
-			usleep_range(delta, delta + 10);
+			udelay(delta);
  	}

  	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	pwm_apply_state(pwm_ir->pwm, &state);
+
+	pwm_ir->count = 0;
+}
+
+static int pwm_ir_thread(void *data)
+{
+	struct pwm_ir *pwm_ir = data;
+
+	for (;;) {
+		wait_event_idle(pwm_ir->tx_wq,
+				kthread_should_stop() || pwm_ir->count);
+
+		if (kthread_should_stop())
+			break;
+
+		_pwm_ir_tx(pwm_ir);
+		complete(&pwm_ir->tx_done);
+	}
+
+	return 0;
+}
+
+static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
+		     unsigned int count)
+{
+	struct pwm_ir *pwm_ir = dev->priv;
+
+	pwm_ir->txbuf = txbuf;
+	pwm_ir->count = count;
+	pwm_ir->index = 0;
+
+	wake_up(&pwm_ir->tx_wq);
+	wait_for_completion(&pwm_ir->tx_done);

  	return count;
  }
@@ -91,12 +168,24 @@ static int pwm_ir_probe(struct platform_device *pdev)
  	if (!pwm_ir)
  		return -ENOMEM;

+	platform_set_drvdata(pdev, pwm_ir);
+
+	pwm_ir->count = 0;
+
  	pwm_ir->pwm = devm_pwm_get(&pdev->dev, NULL);
  	if (IS_ERR(pwm_ir->pwm))
  		return PTR_ERR(pwm_ir->pwm);

-	pwm_ir->carrier = 38000;
+	init_waitqueue_head(&pwm_ir->tx_wq);
+	init_completion(&pwm_ir->edge);
+	init_completion(&pwm_ir->tx_done);
+
+	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	pwm_ir->timer.function = pwm_ir_timer_cb;
+
  	pwm_ir->duty_cycle = 50;
+	pwm_ir->carrier = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm_ir->pwm),
+						NSEC_PER_SEC);

  	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
  	if (!rcdev)
@@ -109,15 +198,38 @@ static int pwm_ir_probe(struct platform_device *pdev)
  	rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle;
  	rcdev->s_tx_carrier = pwm_ir_set_carrier;

+	pwm_ir->tx_thread = kthread_create(pwm_ir_thread, pwm_ir, "%s/tx",
+					   dev_name(&pdev->dev));
+	if (IS_ERR(pwm_ir->tx_thread))
+		return PTR_ERR(pwm_ir->tx_thread);
+
+	sched_set_fifo(pwm_ir->tx_thread);
+	wake_up_process(pwm_ir->tx_thread);
+
  	rc = devm_rc_register_device(&pdev->dev, rcdev);
-	if (rc < 0)
+	if (rc < 0) {
+		kthread_stop(pwm_ir->tx_thread);
  		dev_err(&pdev->dev, "failed to register rc device\n");
+	}

  	return rc;
  }

+static int pwm_ir_remove(struct platform_device *pdev)
+{
+	struct pwm_ir *pwm_ir = platform_get_drvdata(pdev);
+
+	if (pwm_ir->tx_thread) {
+		kthread_stop(pwm_ir->tx_thread);
+		pwm_ir->tx_thread = NULL;
+	}
+
+	return 0;
+}
+
  static struct platform_driver pwm_ir_driver = {
  	.probe = pwm_ir_probe,
+	.remove = pwm_ir_remove,
  	.driver = {
  		.name	= DRIVER_NAME,
  		.of_match_table = of_match_ptr(pwm_ir_of_match),

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Sean Young]

Hi,

On Mon, Oct 02, 2023 at 09:16:53AM +0300, Ivaylo Dimitrov wrote:
> On 1.10.23 г. 13:40 ч., Sean Young wrote:
> > The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
> > from delays as this is done in process context. Make this work in atomic
> > context.
> > 
> > This makes the driver much more precise.
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > ---
> >   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
> >   1 file changed, 63 insertions(+), 16 deletions(-)
> > 
> 
> what about the following patch(not a proper one, just RFC)? It achieves the
> same (if not better) precision (on n900 at least) without using atomic pwm.
> What it does is: create a fifo thread in which we swicth pwm on/off, start
> hrtimer that is used to signal thread when to switch pwm.
> As signal comes earlier than needed(because hrtimer runs async to the
> thread), we do a busy loop wait for the precise time to switch the pwm. At
> least on n900, this busy loop is less than 200 us per switch(worst case,
> usually it is less than 100 us). That way, even if we have some latency
> spike, it is covered by not doing busy loop for that particular pwm switch
> and we keep the precise timing.

I think this is a good idea.

> Maybe we shall merge both patches so fifo thread to be used for sleeping
> pwms and hrtimer for atomic. I can do that and test it here if you think
> that approach makes sense.

Let's try and merge this patch for the next merge window, and worry about
the atomic version after that. We've already queued the ir-rx51 removal
patches to media_stage so it would be nice to have to revert these patches,
and improve pwm-ir-tx for the next kernel release.

I'll test your patch, in the mean time would you mind posting this patch
as a proper patch (with review comments below addressed)?

Thanks,

Sean


> 
> Regards,
> Ivo
> 
> PS: I have a patch that converts timer-ti-dm to non-sleeping one, will send
> it when it comes to it.
> 
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index 105a9c24f1e3..e8b620f53056 100644
> --- a/drivers/media/rc/pwm-ir-tx.c
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -4,6 +4,7 @@
>   */
> 
>  #include <linux/kernel.h>
> +#include <linux/kthread.h>
>  #include <linux/module.h>
>  #include <linux/pwm.h>
>  #include <linux/delay.h>
> @@ -17,8 +18,16 @@
> 
>  struct pwm_ir {
>  	struct pwm_device *pwm;
> +	struct hrtimer timer;
> +	struct task_struct *tx_thread;
> +	wait_queue_head_t tx_wq;
> +	struct completion tx_done;
> +	struct completion edge;
>  	unsigned int carrier;
>  	unsigned int duty_cycle;
> +	unsigned int *txbuf;
> +	unsigned int count;
> +	unsigned int index;
>  };
> 
>  static const struct of_device_id pwm_ir_of_match[] = {
> @@ -48,35 +57,103 @@ static int pwm_ir_set_carrier(struct rc_dev *dev, u32
> carrier)
>  	return 0;
>  }
> 
> -static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> -		     unsigned int count)
> +static enum hrtimer_restart pwm_ir_timer_cb(struct hrtimer *timer)
> +{
> +	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
> +	ktime_t now;
> +
> +	/*
> +	 * If we happen to hit an odd latency spike, loop through the
> +	 * pulses until we catch up.
> +	 */
> +	do {
> +		u64 edge;
> +
> +		if (pwm_ir->index >= pwm_ir->count)
> +			goto out;

Might as well avoid the goto and put the complete and return in the body of
the if.

> +
> +		complete(&pwm_ir->edge);
> +
> +		edge = US_TO_NS(pwm_ir->txbuf[pwm_ir->index]);
> +		hrtimer_add_expires_ns(timer, edge);
> +
> +		pwm_ir->index++;
> +
> +		now = timer->base->get_time();
> +
> +	} while (hrtimer_get_expires_tv64(timer) < now);
> +
> +	return HRTIMER_RESTART;
> +out:
> +	complete(&pwm_ir->edge);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
> +static void _pwm_ir_tx(struct pwm_ir *pwm_ir)
>  {
> -	struct pwm_ir *pwm_ir = dev->priv;
> -	struct pwm_device *pwm = pwm_ir->pwm;
>  	struct pwm_state state;
>  	int i;
>  	ktime_t edge;
>  	long delta;
> 
> -	pwm_init_state(pwm, &state);
> +	pwm_init_state(pwm_ir->pwm, &state);
> 
>  	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
>  	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
> 
> +	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
> +	wait_for_completion(&pwm_ir->edge);
>  	edge = ktime_get();
> 
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < pwm_ir->count; i++) {
>  		state.enabled = !(i % 2);
> -		pwm_apply_state(pwm, &state);
> +		pwm_apply_state(pwm_ir->pwm, &state);
> +
> +		edge = ktime_add_us(edge, pwm_ir->txbuf[i]);
> +		wait_for_completion(&pwm_ir->edge);
> 
> -		edge = ktime_add_us(edge, txbuf[i]);
>  		delta = ktime_us_delta(edge, ktime_get());
> +
>  		if (delta > 0)
> -			usleep_range(delta, delta + 10);
> +			udelay(delta);
>  	}
> 
>  	state.enabled = false;
> -	pwm_apply_state(pwm, &state);
> +	pwm_apply_state(pwm_ir->pwm, &state);
> +
> +	pwm_ir->count = 0;
> +}
> +
> +static int pwm_ir_thread(void *data)
> +{
> +	struct pwm_ir *pwm_ir = data;
> +
> +	for (;;) {
> +		wait_event_idle(pwm_ir->tx_wq,
> +				kthread_should_stop() || pwm_ir->count);
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		_pwm_ir_tx(pwm_ir);
> +		complete(&pwm_ir->tx_done);
> +	}
> +
> +	return 0;
> +}
> +
> +static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> +		     unsigned int count)
> +{
> +	struct pwm_ir *pwm_ir = dev->priv;
> +
> +	pwm_ir->txbuf = txbuf;
> +	pwm_ir->count = count;
> +	pwm_ir->index = 0;
> +
> +	wake_up(&pwm_ir->tx_wq);
> +	wait_for_completion(&pwm_ir->tx_done);
> 
>  	return count;
>  }
> @@ -91,12 +168,24 @@ static int pwm_ir_probe(struct platform_device *pdev)
>  	if (!pwm_ir)
>  		return -ENOMEM;
> 
> +	platform_set_drvdata(pdev, pwm_ir);
> +
> +	pwm_ir->count = 0;
> +
>  	pwm_ir->pwm = devm_pwm_get(&pdev->dev, NULL);
>  	if (IS_ERR(pwm_ir->pwm))
>  		return PTR_ERR(pwm_ir->pwm);
> 
> -	pwm_ir->carrier = 38000;
> +	init_waitqueue_head(&pwm_ir->tx_wq);
> +	init_completion(&pwm_ir->edge);
> +	init_completion(&pwm_ir->tx_done);
> +
> +	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	pwm_ir->timer.function = pwm_ir_timer_cb;
> +
>  	pwm_ir->duty_cycle = 50;
> +	pwm_ir->carrier = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm_ir->pwm),
> +						NSEC_PER_SEC);
> 
>  	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
>  	if (!rcdev)
> @@ -109,15 +198,38 @@ static int pwm_ir_probe(struct platform_device *pdev)
>  	rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle;
>  	rcdev->s_tx_carrier = pwm_ir_set_carrier;
> 
> +	pwm_ir->tx_thread = kthread_create(pwm_ir_thread, pwm_ir, "%s/tx",
> +					   dev_name(&pdev->dev));
> +	if (IS_ERR(pwm_ir->tx_thread))
> +		return PTR_ERR(pwm_ir->tx_thread);
> +
> +	sched_set_fifo(pwm_ir->tx_thread);
> +	wake_up_process(pwm_ir->tx_thread);

This means the thread is always around. How about creating the thread 
per-tx?

> +
>  	rc = devm_rc_register_device(&pdev->dev, rcdev);
> -	if (rc < 0)
> +	if (rc < 0) {
> +		kthread_stop(pwm_ir->tx_thread);
>  		dev_err(&pdev->dev, "failed to register rc device\n");
> +	}
> 
>  	return rc;
>  }
> 
> +static int pwm_ir_remove(struct platform_device *pdev)
> +{
> +	struct pwm_ir *pwm_ir = platform_get_drvdata(pdev);
> +
> +	if (pwm_ir->tx_thread) {
> +		kthread_stop(pwm_ir->tx_thread);
> +		pwm_ir->tx_thread = NULL;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct platform_driver pwm_ir_driver = {
>  	.probe = pwm_ir_probe,
> +	.remove = pwm_ir_remove,
>  	.driver = {
>  		.name	= DRIVER_NAME,
>  		.of_match_table = of_match_ptr(pwm_ir_of_match),

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Ivaylo Dimitrov]

Hi,

On 4.10.23 г. 11:00 ч., Sean Young wrote:
> Hi,
> 
> On Mon, Oct 02, 2023 at 09:16:53AM +0300, Ivaylo Dimitrov wrote:
>> On 1.10.23 г. 13:40 ч., Sean Young wrote:
>>> The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
>>> from delays as this is done in process context. Make this work in atomic
>>> context.
>>>
>>> This makes the driver much more precise.
>>>
>>> Signed-off-by: Sean Young <sean@mess.org>
>>> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>>> ---
>>>    drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
>>>    1 file changed, 63 insertions(+), 16 deletions(-)
>>>
>>
>> what about the following patch(not a proper one, just RFC)? It achieves the
>> same (if not better) precision (on n900 at least) without using atomic pwm.
>> What it does is: create a fifo thread in which we swicth pwm on/off, start
>> hrtimer that is used to signal thread when to switch pwm.
>> As signal comes earlier than needed(because hrtimer runs async to the
>> thread), we do a busy loop wait for the precise time to switch the pwm. At
>> least on n900, this busy loop is less than 200 us per switch(worst case,
>> usually it is less than 100 us). That way, even if we have some latency
>> spike, it is covered by not doing busy loop for that particular pwm switch
>> and we keep the precise timing.
> 
> I think this is a good idea.
> 
>> Maybe we shall merge both patches so fifo thread to be used for sleeping
>> pwms and hrtimer for atomic. I can do that and test it here if you think
>> that approach makes sense.
> 
> Let's try and merge this patch for the next merge window, and worry about
> the atomic version after that. We've already queued the ir-rx51 removal
> patches to media_stage so it would be nice to have to revert these patches,
> and improve pwm-ir-tx for the next kernel release.
> 

ir-rx51 is broken without 
https://www.spinics.net/lists/kernel/msg4953300.html, it is also missing 
a call to init_waitqueue_head() in the probe() function. So I have no 
strong opinion on what shall be done with it.

> I'll test your patch, in the mean time would you mind posting this patch
> as a proper patch (with review comments below addressed)?
> 

ok

> Thanks,
> 
> Sean
> 
> 
>>
>> Regards,
>> Ivo
>>
>> PS: I have a patch that converts timer-ti-dm to non-sleeping one, will send
>> it when it comes to it.
>>
>> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
>> index 105a9c24f1e3..e8b620f53056 100644
>> --- a/drivers/media/rc/pwm-ir-tx.c
>> +++ b/drivers/media/rc/pwm-ir-tx.c
>> @@ -4,6 +4,7 @@
>>    */
>>
>>   #include <linux/kernel.h>
>> +#include <linux/kthread.h>
>>   #include <linux/module.h>
>>   #include <linux/pwm.h>
>>   #include <linux/delay.h>
>> @@ -17,8 +18,16 @@
>>
>>   struct pwm_ir {
>>   	struct pwm_device *pwm;
>> +	struct hrtimer timer;
>> +	struct task_struct *tx_thread;
>> +	wait_queue_head_t tx_wq;
>> +	struct completion tx_done;
>> +	struct completion edge;
>>   	unsigned int carrier;
>>   	unsigned int duty_cycle;
>> +	unsigned int *txbuf;
>> +	unsigned int count;
>> +	unsigned int index;
>>   };
>>
>>   static const struct of_device_id pwm_ir_of_match[] = {
>> @@ -48,35 +57,103 @@ static int pwm_ir_set_carrier(struct rc_dev *dev, u32
>> carrier)
>>   	return 0;
>>   }
>>
>> -static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>> -		     unsigned int count)
>> +static enum hrtimer_restart pwm_ir_timer_cb(struct hrtimer *timer)
>> +{
>> +	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
>> +	ktime_t now;
>> +
>> +	/*
>> +	 * If we happen to hit an odd latency spike, loop through the
>> +	 * pulses until we catch up.
>> +	 */
>> +	do {
>> +		u64 edge;
>> +
>> +		if (pwm_ir->index >= pwm_ir->count)
>> +			goto out;
> 
> Might as well avoid the goto and put the complete and return in the body of
> the if.
> 

right, will fix

>> +
>> +		complete(&pwm_ir->edge);
>> +
>> +		edge = US_TO_NS(pwm_ir->txbuf[pwm_ir->index]);
>> +		hrtimer_add_expires_ns(timer, edge);
>> +
>> +		pwm_ir->index++;
>> +
>> +		now = timer->base->get_time();
>> +
>> +	} while (hrtimer_get_expires_tv64(timer) < now);
>> +
>> +	return HRTIMER_RESTART;
>> +out:
>> +	complete(&pwm_ir->edge);
>> +
>> +	return HRTIMER_NORESTART;
>> +}
>> +
>> +static void _pwm_ir_tx(struct pwm_ir *pwm_ir)
>>   {
>> -	struct pwm_ir *pwm_ir = dev->priv;
>> -	struct pwm_device *pwm = pwm_ir->pwm;
>>   	struct pwm_state state;
>>   	int i;
>>   	ktime_t edge;
>>   	long delta;
>>
>> -	pwm_init_state(pwm, &state);
>> +	pwm_init_state(pwm_ir->pwm, &state);
>>
>>   	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
>>   	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
>>
>> +	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
>> +	wait_for_completion(&pwm_ir->edge);
>>   	edge = ktime_get();
>>
>> -	for (i = 0; i < count; i++) {
>> +	for (i = 0; i < pwm_ir->count; i++) {
>>   		state.enabled = !(i % 2);
>> -		pwm_apply_state(pwm, &state);
>> +		pwm_apply_state(pwm_ir->pwm, &state);
>> +
>> +		edge = ktime_add_us(edge, pwm_ir->txbuf[i]);
>> +		wait_for_completion(&pwm_ir->edge);
>>
>> -		edge = ktime_add_us(edge, txbuf[i]);
>>   		delta = ktime_us_delta(edge, ktime_get());
>> +
>>   		if (delta > 0)
>> -			usleep_range(delta, delta + 10);
>> +			udelay(delta);
>>   	}
>>
>>   	state.enabled = false;
>> -	pwm_apply_state(pwm, &state);
>> +	pwm_apply_state(pwm_ir->pwm, &state);
>> +
>> +	pwm_ir->count = 0;
>> +}
>> +
>> +static int pwm_ir_thread(void *data)
>> +{
>> +	struct pwm_ir *pwm_ir = data;
>> +
>> +	for (;;) {
>> +		wait_event_idle(pwm_ir->tx_wq,
>> +				kthread_should_stop() || pwm_ir->count);
>> +
>> +		if (kthread_should_stop())
>> +			break;
>> +
>> +		_pwm_ir_tx(pwm_ir);
>> +		complete(&pwm_ir->tx_done);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>> +		     unsigned int count)
>> +{
>> +	struct pwm_ir *pwm_ir = dev->priv;
>> +
>> +	pwm_ir->txbuf = txbuf;
>> +	pwm_ir->count = count;
>> +	pwm_ir->index = 0;
>> +
>> +	wake_up(&pwm_ir->tx_wq);
>> +	wait_for_completion(&pwm_ir->tx_done);
>>
>>   	return count;
>>   }
>> @@ -91,12 +168,24 @@ static int pwm_ir_probe(struct platform_device *pdev)
>>   	if (!pwm_ir)
>>   		return -ENOMEM;
>>
>> +	platform_set_drvdata(pdev, pwm_ir);
>> +
>> +	pwm_ir->count = 0;
>> +
>>   	pwm_ir->pwm = devm_pwm_get(&pdev->dev, NULL);
>>   	if (IS_ERR(pwm_ir->pwm))
>>   		return PTR_ERR(pwm_ir->pwm);
>>
>> -	pwm_ir->carrier = 38000;
>> +	init_waitqueue_head(&pwm_ir->tx_wq);
>> +	init_completion(&pwm_ir->edge);
>> +	init_completion(&pwm_ir->tx_done);
>> +
>> +	hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>> +	pwm_ir->timer.function = pwm_ir_timer_cb;
>> +
>>   	pwm_ir->duty_cycle = 50;
>> +	pwm_ir->carrier = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm_ir->pwm),
>> +						NSEC_PER_SEC);
>>
>>   	rcdev = devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX);
>>   	if (!rcdev)
>> @@ -109,15 +198,38 @@ static int pwm_ir_probe(struct platform_device *pdev)
>>   	rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle;
>>   	rcdev->s_tx_carrier = pwm_ir_set_carrier;
>>
>> +	pwm_ir->tx_thread = kthread_create(pwm_ir_thread, pwm_ir, "%s/tx",
>> +					   dev_name(&pdev->dev));
>> +	if (IS_ERR(pwm_ir->tx_thread))
>> +		return PTR_ERR(pwm_ir->tx_thread);
>> +
>> +	sched_set_fifo(pwm_ir->tx_thread);
>> +	wake_up_process(pwm_ir->tx_thread);
> 
> This means the thread is always around. How about creating the thread
> per-tx?
> 

Yes, that can be done, just not sure what the overhead would be.

Also, I think we shall reconsider the way the driver works:

Imagine we have to pretend we are TV remote that supports NEC protocol 
(for example), especially the "REPEAT CODES" part. Currently, no matter 
what we do, there is no way to get the timings even remotely right, as 
we have no idea what the "warmup" and "complete" delays are. Like, 
starting thread (if needed), hrtimer setup time, completions waiting, 
contexts switching, etc.

So, I think the correct thing to do is to copy txbuf (as a list of 
txbufs) into pwm_ir in tx function, start pulses generation and return 
from pwm_ir_tx() *immediately*, without waiting for tx to finish. If 
userspace requests submission of another set of pulses while we are 
still sending the current one, well, we accept it, add it to the list 
and delay the sending until the current one is finished. When there is 
nothing more to send (the list is empty), stop the hrtimer (and perhaps 
the thread)

I think that way userspace will be able to append as many "repeat" 
pulses with proper timings as it wants (with some sane limits ofc).

Unless we somehow have API restriction that we shall not return until tx 
is finished.

Does that make any sense to you?

Thanks,
Ivo

>> +
>>   	rc = devm_rc_register_device(&pdev->dev, rcdev);
>> -	if (rc < 0)
>> +	if (rc < 0) {
>> +		kthread_stop(pwm_ir->tx_thread);
>>   		dev_err(&pdev->dev, "failed to register rc device\n");
>> +	}
>>
>>   	return rc;
>>   }
>>
>> +static int pwm_ir_remove(struct platform_device *pdev)
>> +{
>> +	struct pwm_ir *pwm_ir = platform_get_drvdata(pdev);
>> +
>> +	if (pwm_ir->tx_thread) {
>> +		kthread_stop(pwm_ir->tx_thread);
>> +		pwm_ir->tx_thread = NULL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static struct platform_driver pwm_ir_driver = {
>>   	.probe = pwm_ir_probe,
>> +	.remove = pwm_ir_remove,
>>   	.driver = {
>>   		.name	= DRIVER_NAME,
>>   		.of_match_table = of_match_ptr(pwm_ir_of_match),

Re: [PATCH 2/2] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Sean Young]

On Wed, Oct 04, 2023 at 03:54:32PM +0300, Ivaylo Dimitrov wrote:
> On 4.10.23 г. 11:00 ч., Sean Young wrote:
> > On Mon, Oct 02, 2023 at 09:16:53AM +0300, Ivaylo Dimitrov wrote:
> > > On 1.10.23 г. 13:40 ч., Sean Young wrote:
> > > > The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
> > > > from delays as this is done in process context. Make this work in atomic
> > > > context.
> > > > 
> > > > This makes the driver much more precise.
> > > > 
> > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > > ---
> > > >    drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++--------
> > > >    1 file changed, 63 insertions(+), 16 deletions(-)
> > > > 
> > > 
> > > what about the following patch(not a proper one, just RFC)? It achieves the
> > > same (if not better) precision (on n900 at least) without using atomic pwm.
> > > What it does is: create a fifo thread in which we swicth pwm on/off, start
> > > hrtimer that is used to signal thread when to switch pwm.
> > > As signal comes earlier than needed(because hrtimer runs async to the
> > > thread), we do a busy loop wait for the precise time to switch the pwm. At
> > > least on n900, this busy loop is less than 200 us per switch(worst case,
> > > usually it is less than 100 us). That way, even if we have some latency
> > > spike, it is covered by not doing busy loop for that particular pwm switch
> > > and we keep the precise timing.
> > 
> > I think this is a good idea.
> > 
> > > Maybe we shall merge both patches so fifo thread to be used for sleeping
> > > pwms and hrtimer for atomic. I can do that and test it here if you think
> > > that approach makes sense.
> > 
> > Let's try and merge this patch for the next merge window, and worry about
> > the atomic version after that. We've already queued the ir-rx51 removal
> > patches to media_stage so it would be nice to have to revert these patches,
> > and improve pwm-ir-tx for the next kernel release.
> > 
> 
> ir-rx51 is broken without
> https://www.spinics.net/lists/kernel/msg4953300.html, it is also missing a
> call to init_waitqueue_head() in the probe() function. So I have no strong
> opinion on what shall be done with it.

Sure, ok. I guess the pwm-ir-tx driver is less broken in that regard.

In that case I propose we merge the ir-rx51 for the next merge window,
and further fixes to pwm-ir-tx go in when they're ready.

> > This means the thread is always around. How about creating the thread
> > per-tx?
> > 
> 
> Yes, that can be done, just not sure what the overhead would be.
> 
> Also, I think we shall reconsider the way the driver works:
> 
> Imagine we have to pretend we are TV remote that supports NEC protocol (for
> example), especially the "REPEAT CODES" part. Currently, no matter what we
> do, there is no way to get the timings even remotely right, as we have no
> idea what the "warmup" and "complete" delays are. Like, starting thread (if
> needed), hrtimer setup time, completions waiting, contexts switching, etc.

It's not perfect, but the assumption is that those times are going to be
the same or very similar for each tx. So, if the setup/warmup time is the same
and if there is no complete delay, then using usleep() between two txs 
works fine. I think in reality the setup/complete times are extremely
short (time to send usb packet or so), and compared to IR timings this is
insignificant.

Having said that, maybe a different scheme would be nice, which could offer
better precision.
 
> So, I think the correct thing to do is to copy txbuf (as a list of txbufs)
> into pwm_ir in tx function, start pulses generation and return from
> pwm_ir_tx() *immediately*, without waiting for tx to finish. If userspace
> requests submission of another set of pulses while we are still sending the
> current one, well, we accept it, add it to the list and delay the sending
> until the current one is finished. When there is nothing more to send (the
> list is empty), stop the hrtimer (and perhaps the thread)
> 
> I think that way userspace will be able to append as many "repeat" pulses
> with proper timings as it wants (with some sane limits ofc).
> 
> Unless we somehow have API restriction that we shall not return until tx is
> finished.
> 
> Does that make any sense to you?

Two problems:

It's a breaking uapi change: for example lircd and ir-ctl use this for 
calculating the gap between transmits. If we start returning early then
things break.

Secondly, not all drivers can support this, or they would need to support
it using a thread or so, which makes the driver code much more complicated
and we'd have to change nearly every driver.


Sean