* [PATCH 0/2] Improve pwm-ir-tx precision @ 2023-10-01 10:40 Sean Young 2023-10-01 10:40 ` [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context Sean Young 0 siblings, 1 reply; 11+ messages in thread From: Sean Young @ 2023-10-01 10:40 UTC (permalink / raw) To: Palmer Dabbelt, Paul Walmsley; +Cc: Sean Young, linux-riscv 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. Sean Young (2): pwm: make it possible to apply pwm changes in atomic context media: pwm-ir-tx: trigger edges from hrtimer interrupt context drivers/media/rc/pwm-ir-tx.c | 79 +++++++++++++++++++++++++------- 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 +++++++++-- 17 files changed, 100 insertions(+), 22 deletions(-) -- 2.42.0 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-01 10:40 [PATCH 0/2] Improve pwm-ir-tx precision Sean Young @ 2023-10-01 10:40 ` Sean Young 2023-10-01 14:43 ` kernel test robot ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Sean Young @ 2023-10-01 10:40 UTC (permalink / raw) To: Thierry Reding, Uwe Kleine-König, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Vladimir Zapolskiy, Conor Dooley, Daire McNamara, Palmer Dabbelt, Paul Walmsley, Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue Cc: Sean Young, Ivaylo Dimitrov, linux-pwm, linux-kernel, linux-arm-kernel, linux-riscv, linux-stm32 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-01 10:40 ` [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context Sean Young @ 2023-10-01 14:43 ` kernel test robot 2023-10-01 16:07 ` kernel test robot ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: kernel test robot @ 2023-10-01 14:43 UTC (permalink / raw) To: Sean Young, Thierry Reding, Uwe Kleine-König, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Vladimir Zapolskiy, Conor Dooley, Daire McNamara, Palmer Dabbelt, Paul Walmsley, Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue Cc: oe-kbuild-all, Sean Young, Ivaylo Dimitrov, linux-pwm, linux-kernel, linux-arm-kernel, linux-riscv, linux-stm32 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-01 10:40 ` [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context Sean Young 2023-10-01 14:43 ` kernel test robot @ 2023-10-01 16:07 ` kernel test robot 2023-10-01 17:21 ` Sean Young 2023-10-04 9:59 ` Uwe Kleine-König 2023-10-06 10:29 ` Thierry Reding 3 siblings, 1 reply; 11+ messages in thread From: kernel test robot @ 2023-10-01 16:07 UTC (permalink / raw) To: Sean Young, Thierry Reding, Uwe Kleine-König, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Vladimir Zapolskiy, Conor Dooley, Daire McNamara, Palmer Dabbelt, Paul Walmsley, Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue Cc: oe-kbuild-all, Sean Young, Ivaylo Dimitrov, linux-pwm, linux-kernel, linux-arm-kernel, linux-riscv, linux-stm32 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-01 16:07 ` kernel test robot @ 2023-10-01 17:21 ` Sean Young 0 siblings, 0 replies; 11+ messages in thread From: Sean Young @ 2023-10-01 17:21 UTC (permalink / raw) To: kernel test robot Cc: Thierry Reding, Uwe Kleine-König, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Vladimir Zapolskiy, Conor Dooley, Daire McNamara, Palmer Dabbelt, Paul Walmsley, Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue, oe-kbuild-all, Ivaylo Dimitrov, linux-pwm, linux-kernel, linux-arm-kernel, linux-riscv, linux-stm32 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-01 10:40 ` [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context Sean Young 2023-10-01 14:43 ` kernel test robot 2023-10-01 16:07 ` kernel test robot @ 2023-10-04 9:59 ` Uwe Kleine-König 2023-10-05 8:30 ` Sean Young 2023-10-06 10:27 ` Thierry Reding 2023-10-06 10:29 ` Thierry Reding 3 siblings, 2 replies; 11+ messages in thread From: Uwe Kleine-König @ 2023-10-04 9:59 UTC (permalink / raw) To: Sean Young Cc: Thierry Reding, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Vladimir Zapolskiy, Conor Dooley, Daire McNamara, Palmer Dabbelt, Paul Walmsley, Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue, linux-pwm, Ivaylo Dimitrov, linux-kernel, linux-riscv, linux-stm32, linux-arm-kernel [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-04 9:59 ` Uwe Kleine-König @ 2023-10-05 8:30 ` Sean Young 2023-10-05 9:17 ` Uwe Kleine-König 2023-10-06 10:27 ` Thierry Reding 1 sibling, 1 reply; 11+ messages in thread From: Sean Young @ 2023-10-05 8:30 UTC (permalink / raw) To: Uwe Kleine-König Cc: Thierry Reding, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Vladimir Zapolskiy, Conor Dooley, Daire McNamara, Palmer Dabbelt, Paul Walmsley, Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue, linux-pwm, Ivaylo Dimitrov, linux-kernel, linux-riscv, linux-stm32, linux-arm-kernel 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-05 8:30 ` Sean Young @ 2023-10-05 9:17 ` Uwe Kleine-König 0 siblings, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2023-10-05 9:17 UTC (permalink / raw) To: Sean Young Cc: linux-arm-kernel, linux-pwm, Ivaylo Dimitrov, Maxime Coquelin, Daire McNamara, Shawn Guo, Sascha Hauer, Alexandre Torgue, Vladimir Zapolskiy, Conor Dooley, Thierry Reding, Palmer Dabbelt, NXP Linux Team, Pengutronix Kernel Team, Paul Walmsley, linux-riscv, Fabio Estevam, linux-stm32, linux-kernel, Fabrice Gasnier [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-04 9:59 ` Uwe Kleine-König 2023-10-05 8:30 ` Sean Young @ 2023-10-06 10:27 ` Thierry Reding 2023-10-06 14:44 ` Uwe Kleine-König 1 sibling, 1 reply; 11+ messages in thread From: Thierry Reding @ 2023-10-06 10:27 UTC (permalink / raw) To: Uwe Kleine-König Cc: Sean Young, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Vladimir Zapolskiy, Conor Dooley, Daire McNamara, Palmer Dabbelt, Paul Walmsley, Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue, linux-pwm, Ivaylo Dimitrov, linux-kernel, linux-riscv, linux-stm32, linux-arm-kernel [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-06 10:27 ` Thierry Reding @ 2023-10-06 14:44 ` Uwe Kleine-König 0 siblings, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2023-10-06 14:44 UTC (permalink / raw) To: Thierry Reding Cc: linux-arm-kernel, linux-pwm, Ivaylo Dimitrov, Maxime Coquelin, Sean Young, Daire McNamara, Shawn Guo, Sascha Hauer, Alexandre Torgue, Vladimir Zapolskiy, Conor Dooley, Fabrice Gasnier, Palmer Dabbelt, NXP Linux Team, Pengutronix Kernel Team, Paul Walmsley, linux-riscv, Fabio Estevam, linux-stm32, linux-kernel [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context 2023-10-01 10:40 ` [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context Sean Young ` (2 preceding siblings ...) 2023-10-04 9:59 ` Uwe Kleine-König @ 2023-10-06 10:29 ` Thierry Reding 3 siblings, 0 replies; 11+ messages in thread From: Thierry Reding @ 2023-10-06 10:29 UTC (permalink / raw) To: Sean Young Cc: Uwe Kleine-König, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Vladimir Zapolskiy, Conor Dooley, Daire McNamara, Palmer Dabbelt, Paul Walmsley, Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue, Ivaylo Dimitrov, linux-pwm, linux-kernel, linux-arm-kernel, linux-riscv, linux-stm32 [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-10-06 14:45 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-01 10:40 [PATCH 0/2] Improve pwm-ir-tx precision Sean Young 2023-10-01 10:40 ` [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context Sean Young 2023-10-01 14:43 ` kernel test robot 2023-10-01 16:07 ` kernel test robot 2023-10-01 17:21 ` Sean Young 2023-10-04 9:59 ` Uwe Kleine-König 2023-10-05 8:30 ` Sean Young 2023-10-05 9:17 ` Uwe Kleine-König 2023-10-06 10:27 ` Thierry Reding 2023-10-06 14:44 ` Uwe Kleine-König 2023-10-06 10:29 ` Thierry Reding
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).