| Message ID | 20250717151117.1828585-2-u.kleine-koenig@baylibre.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] pwm: Provide a gpio device for waveform drivers | expand |
On Thu, Jul 17, 2025 at 5:11 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > A PWM is a more general concept than an output-only GPIO. When using > duty_length = period_length the PWM looks like an active GPIO, with > duty_length = 0 like an inactive GPIO. With the waveform abstraction > there is enough control over the configuration to ensure that PWMs that > cannot generate a constant signal at both levels error out. > > The pwm-pca9685 driver already provides a gpio chip. When this driver is > converted to the waveform callbacks, the gpio part can just be dropped. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > --- > Hello, > > I found some uncommitted changes in my tree that belong in this patch. > This v2 actually compiles ... > > Best regards > Uwe > [...] > + > /** > * __pwmchip_add() - register a new PWM chip > * @chip: the PWM chip to add > @@ -2449,9 +2494,33 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > if (ret) > goto err_device_add; > > + if (IS_ENABLED(CONFIG_PWM_PROVIDE_GPIO) && chip->ops->write_waveform) { > + struct device *parent = pwmchip_parent(chip); > + > + chip->gpio = (typeof(chip->gpio)){ > + .label = dev_name(parent), > + .parent = parent, > + .request = pwm_gpio_request, > + .free = pwm_gpio_free, > + .get_direction = pwm_gpio_get_direction, > + .set_rv = pwm_gpio_set, > + .base = -1, > + .ngpio = chip->npwm, > + .can_sleep = true, > + }; I would have probably just assigned each field separately and avoid the cast but it's your code so I don't have a strong opinion. Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> [...]
Hello Bartosz, On Fri, Jul 18, 2025 at 10:24:35AM +0200, Bartosz Golaszewski wrote: > On Thu, Jul 17, 2025 at 5:11 PM Uwe Kleine-König > <u.kleine-koenig@baylibre.com> wrote: > > @@ -2449,9 +2494,33 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > > if (ret) > > goto err_device_add; > > > > + if (IS_ENABLED(CONFIG_PWM_PROVIDE_GPIO) && chip->ops->write_waveform) { > > + struct device *parent = pwmchip_parent(chip); > > + > > + chip->gpio = (typeof(chip->gpio)){ > > + .label = dev_name(parent), > > + .parent = parent, > > + .request = pwm_gpio_request, > > + .free = pwm_gpio_free, > > + .get_direction = pwm_gpio_get_direction, > > + .set_rv = pwm_gpio_set, > > + .base = -1, > > + .ngpio = chip->npwm, > > + .can_sleep = true, > > + }; > > I would have probably just assigned each field separately and avoid > the cast but it's your code so I don't have a strong opinion. This is not a cast but an initializer. The nice side effect is that all unmentioned struct members are default initialized, so there is no need for an explicit memset(..., 0, ...);. > Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Thanks Uwe
Hello, On Thu, Jul 17, 2025 at 05:11:16PM +0200, Uwe Kleine-König wrote: > A PWM is a more general concept than an output-only GPIO. When using > duty_length = period_length the PWM looks like an active GPIO, with > duty_length = 0 like an inactive GPIO. With the waveform abstraction > there is enough control over the configuration to ensure that PWMs that > cannot generate a constant signal at both levels error out. > > The pwm-pca9685 driver already provides a gpio chip. When this driver is > converted to the waveform callbacks, the gpio part can just be dropped. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-nexxt as 6.18-rc1 material. (I just claimed the same for the v1 patch, that's of course bogus and this v2 is on for-nexxt.) Best regards Uwe
Hello again, On Fri, Aug 01, 2025 at 12:03:20PM +0200, Uwe Kleine-König wrote: > On Thu, Jul 17, 2025 at 05:11:16PM +0200, Uwe Kleine-König wrote: > > A PWM is a more general concept than an output-only GPIO. When using > > duty_length = period_length the PWM looks like an active GPIO, with > > duty_length = 0 like an inactive GPIO. With the waveform abstraction > > there is enough control over the configuration to ensure that PWMs that > > cannot generate a constant signal at both levels error out. > > > > The pwm-pca9685 driver already provides a gpio chip. When this driver is > > converted to the waveform callbacks, the gpio part can just be dropped. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-nexxt > > as 6.18-rc1 material. (I just claimed the same for the v1 patch, that's > of course bogus and this v2 is on for-nexxt.) Now that 6.17-rc1 is tagged I rebase my tree and squash the following change into the commit for this patch: diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index b05186b9569e..ec4112e6209a 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -2511,7 +2511,7 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) .request = pwm_gpio_request, .free = pwm_gpio_free, .get_direction = pwm_gpio_get_direction, - .set_rv = pwm_gpio_set, + .set = pwm_gpio_set, .base = -1, .ngpio = chip->npwm, .can_sleep = true, diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 6e7d02c24991..549ac4aaad59 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -322,6 +322,7 @@ struct pwm_ops { * @npwm: number of PWMs controlled by this chip * @of_xlate: request a PWM device given a device tree PWM specifier * @atomic: can the driver's ->apply() be called in atomic context + * @gpio: &struct gpio_chip to operate this PWM chip's lines as GPO * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip * @operational: signals if the chip can be used (or is already deregistered) * @nonatomic_lock: mutex for nonatomic chips The first hunk is to make the code compile after commit d9d87d90cc0b ("treewide: rename GPIO set callbacks back to their original names"). The second to please `make htmldocs`. Both issues were highlighted by Stephen Rothwell, a big ❤️ for the tedious work to build the next integration tree each day and pointing out these issues. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 3ef1757502eb..778168e71055 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -38,6 +38,15 @@ config PWM_DEBUG It is expected to introduce some runtime overhead and diagnostic output to the kernel log, so only enable while working on a driver. +config PWM_PROVIDE_GPIO + bool "Provide a GPIO chip for each PWM chip" + depends on GPIOLIB + help + Most PWMs can emit both a constant active high and a constant active + low signal and so they can be used as GPIO. Say Y here to let each + PWM chip provide a GPIO chip and so be easily plugged into consumers + that know how to handle GPIOs but not PWMs. + config PWM_AB8500 tristate "AB8500 PWM support" depends on AB8500_CORE && ARCH_U8500 diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 0d66376a83ec..7f048e09b3ce 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -2383,6 +2383,51 @@ static const struct file_operations pwm_cdev_fileops = { static dev_t pwm_devt; +static int pwm_gpio_request(struct gpio_chip *gc, unsigned int offset) +{ + struct pwm_chip *chip = gpiochip_get_data(gc); + struct pwm_device *pwm; + + pwm = pwm_request_from_chip(chip, offset, "pwm-gpio"); + if (IS_ERR(pwm)) + return PTR_ERR(pwm); + + return 0; +} + +static void pwm_gpio_free(struct gpio_chip *gc, unsigned int offset) +{ + struct pwm_chip *chip = gpiochip_get_data(gc); + + pwm_put(&chip->pwms[offset]); +} + +static int pwm_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + return GPIO_LINE_DIRECTION_OUT; +} + +static int pwm_gpio_set(struct gpio_chip *gc, unsigned int offset, int value) +{ + struct pwm_chip *chip = gpiochip_get_data(gc); + struct pwm_device *pwm = &chip->pwms[offset]; + int ret; + struct pwm_waveform wf = { + .period_length_ns = 1, + }; + + ret = pwm_round_waveform_might_sleep(pwm, &wf); + if (ret < 0) + return ret; + + if (value) + wf.duty_length_ns = wf.period_length_ns; + else + wf.duty_length_ns = 0; + + return pwm_set_waveform_might_sleep(pwm, &wf, true); +} + /** * __pwmchip_add() - register a new PWM chip * @chip: the PWM chip to add @@ -2449,9 +2494,33 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) if (ret) goto err_device_add; + if (IS_ENABLED(CONFIG_PWM_PROVIDE_GPIO) && chip->ops->write_waveform) { + struct device *parent = pwmchip_parent(chip); + + chip->gpio = (typeof(chip->gpio)){ + .label = dev_name(parent), + .parent = parent, + .request = pwm_gpio_request, + .free = pwm_gpio_free, + .get_direction = pwm_gpio_get_direction, + .set_rv = pwm_gpio_set, + .base = -1, + .ngpio = chip->npwm, + .can_sleep = true, + }; + + ret = gpiochip_add_data(&chip->gpio, chip); + if (ret) + goto err_gpiochip_add; + } + return 0; +err_gpiochip_add: + + cdev_device_del(&chip->cdev, &chip->dev); err_device_add: + scoped_guard(pwmchip, chip) chip->operational = false; @@ -2472,6 +2541,9 @@ EXPORT_SYMBOL_GPL(__pwmchip_add); */ void pwmchip_remove(struct pwm_chip *chip) { + if (IS_ENABLED(CONFIG_PWM_PROVIDE_GPIO) && chip->ops->write_waveform) + gpiochip_remove(&chip->gpio); + pwmchip_sysfs_unexport(chip); scoped_guard(mutex, &pwm_lock) { diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 8cafc483db53..6e7d02c24991 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -5,6 +5,7 @@ #include <linux/cdev.h> #include <linux/device.h> #include <linux/err.h> +#include <linux/gpio/driver.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/of.h> @@ -340,6 +341,7 @@ struct pwm_chip { bool atomic; /* only used internally by the PWM framework */ + struct gpio_chip gpio; bool uses_pwmchip_alloc; bool operational; union {
A PWM is a more general concept than an output-only GPIO. When using duty_length = period_length the PWM looks like an active GPIO, with duty_length = 0 like an inactive GPIO. With the waveform abstraction there is enough control over the configuration to ensure that PWMs that cannot generate a constant signal at both levels error out. The pwm-pca9685 driver already provides a gpio chip. When this driver is converted to the waveform callbacks, the gpio part can just be dropped. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- Hello, I found some uncommitted changes in my tree that belong in this patch. This v2 actually compiles ... Best regards Uwe drivers/pwm/Kconfig | 9 ++++++ drivers/pwm/core.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/pwm.h | 2 ++ 3 files changed, 83 insertions(+) base-commit: a582469541a3f39bed452c50c5d2744620b6db02