Hello,
this is v5 of this series. The relevant changes since v4 (https://lore.kernel.org/linux-pwm/cover.1701860672.git.u.kleine-koenig@pengu...):
- New first patch to reshuffle functions in core.c. This is a preparation for the later changes which brings functions in a better order to not need declarations. - Fix kernel docs in several drivers - Added a few ack and review tags received for v4 - non-trivially rebased to current pwm/for-next (the changes to drivers/gpu/drm/bridge/ti-sn65dsi86.c were intrusive enough to not add the ack tag by Robert Foss I got).
Handling got a bit more complicated with the recent addition of pwm_apply_atomic/pwm_apply_might_sleep the locking got more complicated. I didn't work out all the necessary details. So this series won't work as is. However as there is probably some more coordination needed to get the patches in that touch files outside of drivers/pwm and I'm confident they can stay as is, I want to get the biggest part of this series in (up to patch #106) during the next merge window and get them into next soon. After that I can spend the time necessary to fix the locking maybe to get the remaining bits in during the following merge window.
There are patches touching drivers/gpu/drm/bridge/ti-sn65dsi86.c (#37 and #104), drivers/staging/greybus/pwm.c (#38 and #106), drivers/gpio/gpio-mvebu.c (#103) and drivers/leds/rgb/leds-qcom-lpg.c (#105). These depend on earlier patches in this series (#3, #39 and #40) The patches touching staging/greybus and leds-qcom-lpg already have a maintainer ack, so I'd merge them via my tree. For the other two it would be nice to get an ack to merge via my tree, too. But if you want to merge via your own tree, please tell, so we can coordinate accordingly.
Best regards Uwe
Uwe Kleine-König (111): pwm: Reorder symbols in core.c pwm: cros-ec: Change prototype of helpers to prepare further changes pwm: Provide a macro to get the parent device of a given chip pwm: ab8500: Make use of pwmchip_parent() macro pwm: atmel: Make use of pwmchip_parent() macro pwm: atmel-tcb: Make use of pwmchip_parent() macro pwm: bcm-kona: Make use of pwmchip_parent() macro pwm: crc: Make use of pwmchip_parent() macro pwm: cros-ec: Make use of pwmchip_parent() macro pwm: dwc: Make use of pwmchip_parent() macro pwm: ep93xx: Make use of pwmchip_parent() macro pwm: fsl-ftm: Make use of pwmchip_parent() macro pwm: img: Make use of parent device pointer in driver data pwm: imx27: Make use of pwmchip_parent() macro pwm: jz4740: Make use of pwmchip_parent() macro pwm: lpc18xx-sct: Make use of parent device pointer in driver data pwm: lpss: Make use of pwmchip_parent() macro pwm: mediatek: Make use of pwmchip_parent() macro pwm: meson: Make use of pwmchip_parent() macro pwm: mtk-disp: Make use of pwmchip_parent() macro pwm: omap: Make use of pwmchip_parent() macro pwm: pca9685: Store parent device in driver data pwm: raspberrypi-poe: Make use of pwmchip_parent() macro pwm: rcar: Make use of pwmchip_parent() macro pwm: rz-mtu3: Make use of pwmchip_parent() macro pwm: samsung: Make use of pwmchip_parent() macro pwm: sifive: Make use of pwmchip_parent() macro pwm: stm32-lp: Make use of pwmchip_parent() macro pwm: stm32: Make use of pwmchip_parent() macro pwm: stmpe: Make use of pwmchip_parent() macro pwm: sun4i: Make use of pwmchip_parent() macro pwm: tiecap: Make use of pwmchip_parent() macro pwm: tiehrpwm: Make use of pwmchip_parent() macro pwm: twl-led: Make use of pwmchip_parent() macro pwm: twl: Make use of pwmchip_parent() macro pwm: vt8500: Make use of pwmchip_parent() macro drm/bridge: ti-sn65dsi86: Make use of pwmchip_parent() macro staging: greybus: pwm: Make use of pwmchip_parent() macro pwm: Provide wrappers for storing and getting driver private data pwm: Provide devm_pwmchip_alloc() function pwm: ab8500: Make use of devm_pwmchip_alloc() function pwm: apple: Make use of devm_pwmchip_alloc() function pwm: atmel-hlcdc: Make use of devm_pwmchip_alloc() function pwm: atmel: Make use of devm_pwmchip_alloc() function pwm: atmel-tcb: Make use of devm_pwmchip_alloc() function pwm: bcm2835: Make use of devm_pwmchip_alloc() function pwm: bcm-iproc: Make use of devm_pwmchip_alloc() function pwm: bcm-kona: Make use of devm_pwmchip_alloc() function pwm: berlin: Make use of devm_pwmchip_alloc() function pwm: brcmstb: Make use of devm_pwmchip_alloc() function pwm: clk: Make use of devm_pwmchip_alloc() function pwm: clps711x: Make use of devm_pwmchip_alloc() function pwm: crc: Make use of devm_pwmchip_alloc() function pwm: cros-ec: Make use of devm_pwmchip_alloc() function pwm: dwc: Make use of devm_pwmchip_alloc() function pwm: ep93xx: Make use of devm_pwmchip_alloc() function pwm: fsl-ftm: Make use of devm_pwmchip_alloc() function pwm: hibvt: Make use of devm_pwmchip_alloc() function pwm: img: Make use of devm_pwmchip_alloc() function pwm: imx1: Make use of devm_pwmchip_alloc() function pwm: imx27: Make use of devm_pwmchip_alloc() function pwm: imx-tpm: Make use of devm_pwmchip_alloc() function pwm: intel-lgm: Make use of devm_pwmchip_alloc() function pwm: iqs620a: Make use of devm_pwmchip_alloc() function pwm: jz4740: Make use of devm_pwmchip_alloc() function pwm: keembay: Make use of devm_pwmchip_alloc() function pwm: lp3943: Make use of devm_pwmchip_alloc() function pwm: lpc18xx-sct: Make use of devm_pwmchip_alloc() function pwm: lpc32xx: Make use of devm_pwmchip_alloc() function pwm: lpss-*: Make use of devm_pwmchip_alloc() function pwm: mediatek: Make use of devm_pwmchip_alloc() function pwm: meson: Make use of devm_pwmchip_alloc() function pwm: microchip-core: Make use of devm_pwmchip_alloc() function pwm: mtk-disp: Make use of devm_pwmchip_alloc() function pwm: mxs: Make use of devm_pwmchip_alloc() function pwm: ntxec: Make use of devm_pwmchip_alloc() function pwm: omap-dmtimer: Make use of devm_pwmchip_alloc() function pwm: pca9685: Make use of devm_pwmchip_alloc() function pwm: pxa: Make use of devm_pwmchip_alloc() function pwm: raspberrypi-poe: Make use of devm_pwmchip_alloc() function pwm: rcar: Make use of devm_pwmchip_alloc() function pwm: renesas-tpu: Make use of devm_pwmchip_alloc() function pwm: rockchip: Make use of devm_pwmchip_alloc() function pwm: rz-mtu3: Make use of devm_pwmchip_alloc() function pwm: samsung: Make use of devm_pwmchip_alloc() function pwm: sifive: Make use of devm_pwmchip_alloc() function pwm: sl28cpld: Make use of devm_pwmchip_alloc() function pwm: spear: Make use of devm_pwmchip_alloc() function pwm: sprd: Make use of devm_pwmchip_alloc() function pwm: sti: Make use of devm_pwmchip_alloc() function pwm: stm32-lp: Make use of devm_pwmchip_alloc() function pwm: stm32: Make use of devm_pwmchip_alloc() function pwm: stmpe: Make use of devm_pwmchip_alloc() function pwm: sun4i: Make use of devm_pwmchip_alloc() function pwm: sunplus: Make use of devm_pwmchip_alloc() function pwm: tegra: Make use of devm_pwmchip_alloc() function pwm: tiecap: Make use of devm_pwmchip_alloc() function pwm: twl-led: Make use of devm_pwmchip_alloc() function pwm: twl: Make use of devm_pwmchip_alloc() function pwm: visconti: Make use of devm_pwmchip_alloc() function pwm: vt8500: Make use of devm_pwmchip_alloc() function pwm: xilinx: Make use of devm_pwmchip_alloc() function gpio: mvebu: Make use of devm_pwmchip_alloc() function drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function leds: qcom-lpg: Make use of devm_pwmchip_alloc() function staging: greybus: pwm: Make use of devm_pwmchip_alloc() function pwm: Ensure that pwm_chips are allocated using pwmchip_alloc() pwm: Ensure a struct pwm has the same lifetime as its pwm_chip pwm: Ensure the memory backing a PWM chip isn't freed while used pwm: Make pwmchip_[sg]et_drvdata() a wrapper around dev_set_drvdata() WIP: pwm: Add support for pwmchip devices for faster and easier userspace access
.../driver-api/driver-model/devres.rst | 1 + Documentation/driver-api/pwm.rst | 10 +- drivers/gpio/gpio-mvebu.c | 18 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 31 +- drivers/leds/rgb/leds-qcom-lpg.c | 16 +- drivers/pwm/Kconfig | 4 - drivers/pwm/Makefile | 3 +- drivers/pwm/core.c | 958 ++++++++++++------ drivers/pwm/pwm-ab8500.c | 36 +- drivers/pwm/pwm-apple.c | 18 +- drivers/pwm/pwm-atmel-hlcdc.c | 35 +- drivers/pwm/pwm-atmel-tcb.c | 26 +- drivers/pwm/pwm-atmel.c | 37 +- drivers/pwm/pwm-bcm-iproc.c | 19 +- drivers/pwm/pwm-bcm-kona.c | 21 +- drivers/pwm/pwm-bcm2835.c | 19 +- drivers/pwm/pwm-berlin.c | 29 +- drivers/pwm/pwm-brcmstb.c | 17 +- drivers/pwm/pwm-clk.c | 27 +- drivers/pwm/pwm-clps711x.c | 17 +- drivers/pwm/pwm-crc.c | 22 +- drivers/pwm/pwm-cros-ec.c | 58 +- drivers/pwm/pwm-dwc-core.c | 25 +- drivers/pwm/pwm-dwc.c | 18 +- drivers/pwm/pwm-dwc.h | 9 +- drivers/pwm/pwm-ep93xx.c | 21 +- drivers/pwm/pwm-fsl-ftm.c | 48 +- drivers/pwm/pwm-hibvt.c | 25 +- drivers/pwm/pwm-img.c | 51 +- drivers/pwm/pwm-imx-tpm.c | 34 +- drivers/pwm/pwm-imx1.c | 20 +- drivers/pwm/pwm-imx27.c | 26 +- drivers/pwm/pwm-intel-lgm.c | 17 +- drivers/pwm/pwm-iqs620a.c | 37 +- drivers/pwm/pwm-jz4740.c | 36 +- drivers/pwm/pwm-keembay.c | 17 +- drivers/pwm/pwm-lp3943.c | 17 +- drivers/pwm/pwm-lpc18xx-sct.c | 35 +- drivers/pwm/pwm-lpc32xx.c | 21 +- drivers/pwm/pwm-lpss-pci.c | 10 +- drivers/pwm/pwm-lpss-platform.c | 10 +- drivers/pwm/pwm-lpss.c | 34 +- drivers/pwm/pwm-lpss.h | 1 - drivers/pwm/pwm-mediatek.c | 29 +- drivers/pwm/pwm-meson.c | 57 +- drivers/pwm/pwm-microchip-core.c | 17 +- drivers/pwm/pwm-mtk-disp.c | 25 +- drivers/pwm/pwm-mxs.c | 32 +- drivers/pwm/pwm-ntxec.c | 30 +- drivers/pwm/pwm-omap-dmtimer.c | 47 +- drivers/pwm/pwm-pca9685.c | 98 +- drivers/pwm/pwm-pxa.c | 19 +- drivers/pwm/pwm-raspberrypi-poe.c | 20 +- drivers/pwm/pwm-rcar.c | 25 +- drivers/pwm/pwm-renesas-tpu.c | 18 +- drivers/pwm/pwm-rockchip.c | 24 +- drivers/pwm/pwm-rz-mtu3.c | 39 +- drivers/pwm/pwm-samsung.c | 57 +- drivers/pwm/pwm-sifive.c | 30 +- drivers/pwm/pwm-sl28cpld.c | 13 +- drivers/pwm/pwm-spear.c | 18 +- drivers/pwm/pwm-sprd.c | 50 +- drivers/pwm/pwm-sti.c | 34 +- drivers/pwm/pwm-stm32-lp.c | 29 +- drivers/pwm/pwm-stm32.c | 53 +- drivers/pwm/pwm-stmpe.c | 58 +- drivers/pwm/pwm-sun4i.c | 38 +- drivers/pwm/pwm-sunplus.c | 17 +- drivers/pwm/pwm-tegra.c | 27 +- drivers/pwm/pwm-tiecap.c | 55 +- drivers/pwm/pwm-tiehrpwm.c | 72 +- drivers/pwm/pwm-twl-led.c | 58 +- drivers/pwm/pwm-twl.c | 50 +- drivers/pwm/pwm-visconti.c | 17 +- drivers/pwm/pwm-vt8500.c | 41 +- drivers/pwm/pwm-xilinx.c | 34 +- drivers/pwm/sysfs.c | 64 +- drivers/staging/greybus/pwm.c | 130 +-- include/linux/platform_data/x86/pwm-lpss.h | 4 +- include/linux/pwm.h | 39 +- include/uapi/linux/pwm.h | 23 + 81 files changed, 1870 insertions(+), 1555 deletions(-) create mode 100644 include/uapi/linux/pwm.h
base-commit: 6530623212338fc0902e211ea624e790aacb00ef
Currently a pwm_chip stores in its struct device *dev member a pointer to the parent device. Preparing a change that embeds a full struct device in struct pwm_chip, this accessor macro should be used in all drivers directly accessing chip->dev now. This way struct pwm_chip and this macro can be changed without having to touch all drivers in the same change set.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de --- include/linux/pwm.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 8ffe9ae7a23a..d7966918f301 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -289,6 +289,11 @@ struct pwm_chip { struct pwm_device *pwms; };
+static inline struct device *pwmchip_parent(struct pwm_chip *chip) +{ + return chip->dev; +} + #if IS_ENABLED(CONFIG_PWM) /* PWM user APIs */ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
Il 25/01/24 13:08, Uwe Kleine-König ha scritto:
Currently a pwm_chip stores in its struct device *dev member a pointer to the parent device. Preparing a change that embeds a full struct device in struct pwm_chip, this accessor macro should be used in all drivers directly accessing chip->dev now. This way struct pwm_chip and this macro can be changed without having to touch all drivers in the same change set.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
Reviewed-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com
include/linux/pwm.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 8ffe9ae7a23a..d7966918f301 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -289,6 +289,11 @@ struct pwm_chip { struct pwm_device *pwms; }; +static inline struct device *pwmchip_parent(struct pwm_chip *chip) +{
- return chip->dev;
+}
- #if IS_ENABLED(CONFIG_PWM) /* PWM user APIs */ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
On 1/25/24 04:08, Uwe Kleine-König wrote:
Currently a pwm_chip stores in its struct device *dev member a pointer to the parent device. Preparing a change that embeds a full struct device in struct pwm_chip, this accessor macro should be used in all drivers directly accessing chip->dev now. This way struct pwm_chip and this macro can be changed without having to touch all drivers in the same change set.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
Nit: this is not a macro but an inline function.
On Thu, Jan 25, 2024 at 11:32:47AM -0800, Florian Fainelli wrote:
On 1/25/24 04:08, Uwe Kleine-König wrote:
Currently a pwm_chip stores in its struct device *dev member a pointer to the parent device. Preparing a change that embeds a full struct device in struct pwm_chip, this accessor macro should be used in all drivers directly accessing chip->dev now. This way struct pwm_chip and this macro can be changed without having to touch all drivers in the same change set.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
Nit: this is not a macro but an inline function.
Oh right, it used to be a macro, but I changed that. I made the commit log read:
pwm: Provide an inline function to get the parent device of a given chip
Currently a pwm_chip stores in its struct device *dev member a pointer to the parent device. Preparing a change that embeds a full struct device in struct pwm_chip, this accessor function should be used in all drivers directly accessing chip->dev now. This way struct pwm_chip and this new function can be changed without having to touch all drivers in the same change set.
Thanks, Uwe
On Thu, Jan 25, 2024 at 09:29:37PM +0100, Uwe Kleine-König wrote:
On Thu, Jan 25, 2024 at 11:32:47AM -0800, Florian Fainelli wrote:
On 1/25/24 04:08, Uwe Kleine-König wrote:
Currently a pwm_chip stores in its struct device *dev member a pointer to the parent device. Preparing a change that embeds a full struct device in struct pwm_chip, this accessor macro should be used in all drivers directly accessing chip->dev now. This way struct pwm_chip and this macro can be changed without having to touch all drivers in the same change set.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
Nit: this is not a macro but an inline function.
Oh right, it used to be a macro, but I changed that. I made the commit log read:
pwm: Provide an inline function to get the parent device of a given chip Currently a pwm_chip stores in its struct device *dev member a pointer to the parent device. Preparing a change that embeds a full struct device in struct pwm_chip, this accessor function should be used in all drivers directly accessing chip->dev now. This way struct pwm_chip and this new function can be changed without having to touch all drivers in the same change set.
While looking into further feedback, I noticed I did the same mistake in all the patches that convert the drivers to use this function. I did
git filter-branch --msg-filter 'sed "s/Make use of pwmchip_parent() macro/Make use of pwmchip_parent() accessor/; s/commit as struct pwm_chip::dev, use the macro/commit as struct pwm_chip::dev, use the accessor/; s/provided for exactly this purpose./function provided for exactly this purpose./"' linus/master..
on my branch to make the typical commit log read:
pwm: atmel: Make use of pwmchip_parent() accessor struct pwm_chip::dev is about to change. To not have to touch this driver in the same commit as struct pwm_chip::dev, use the accessor function provided for exactly this purpose.
I wont resend the whole series if this is the only change to it.
Best regards Uwe
struct pwm_chip::dev is about to change. To not have to touch this driver in the same commit as struct pwm_chip::dev, use the macro provided for exactly this purpose.
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de --- drivers/staging/greybus/pwm.c | 55 +++++++++++++++++------------------ 1 file changed, 26 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c index a3cb68cfa0f9..75e0518791d8 100644 --- a/drivers/staging/greybus/pwm.c +++ b/drivers/staging/greybus/pwm.c @@ -17,7 +17,6 @@ struct gb_pwm_chip { struct gb_connection *connection; u8 pwm_max; /* max pwm number */ - struct pwm_chip chip; };
@@ -39,9 +38,9 @@ static int gb_pwm_count_operation(struct gb_pwm_chip *pwmc) return 0; }
-static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc, - u8 which) +static int gb_pwm_activate_operation(struct pwm_chip *chip, u8 which) { + struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_activate_request request; struct gbphy_device *gbphy_dev; int ret; @@ -51,7 +50,7 @@ static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc,
request.which = which;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev); + gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -64,9 +63,10 @@ static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc, return ret; }
-static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc, +static int gb_pwm_deactivate_operation(struct pwm_chip *chip, u8 which) { + struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_deactivate_request request; struct gbphy_device *gbphy_dev; int ret; @@ -76,7 +76,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
request.which = which;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev); + gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -89,9 +89,10 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc, return ret; }
-static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc, +static int gb_pwm_config_operation(struct pwm_chip *chip, u8 which, u32 duty, u32 period) { + struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_config_request request; struct gbphy_device *gbphy_dev; int ret; @@ -103,7 +104,7 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc, request.duty = cpu_to_le32(duty); request.period = cpu_to_le32(period);
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev); + gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -116,9 +117,10 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc, return ret; }
-static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc, +static int gb_pwm_set_polarity_operation(struct pwm_chip *chip, u8 which, u8 polarity) { + struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_polarity_request request; struct gbphy_device *gbphy_dev; int ret; @@ -129,7 +131,7 @@ static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc, request.which = which; request.polarity = polarity;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev); + gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -142,9 +144,9 @@ static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc, return ret; }
-static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc, - u8 which) +static int gb_pwm_enable_operation(struct pwm_chip *chip, u8 which) { + struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_enable_request request; struct gbphy_device *gbphy_dev; int ret; @@ -154,7 +156,7 @@ static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc,
request.which = which;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev); + gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -167,9 +169,9 @@ static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc, return ret; }
-static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc, - u8 which) +static int gb_pwm_disable_operation(struct pwm_chip *chip, u8 which) { + struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_disable_request request; struct gbphy_device *gbphy_dev; int ret; @@ -182,7 +184,7 @@ static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc, ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_DISABLE, &request, sizeof(request), NULL, 0);
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev); + gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); gbphy_runtime_put_autosuspend(gbphy_dev);
return ret; @@ -190,19 +192,15 @@ static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc,
static int gb_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) { - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); - - return gb_pwm_activate_operation(pwmc, pwm->hwpwm); + return gb_pwm_activate_operation(chip, pwm->hwpwm); };
static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) { - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); - if (pwm_is_enabled(pwm)) - dev_warn(chip->dev, "freeing PWM device without disabling\n"); + dev_warn(pwmchip_parent(chip), "freeing PWM device without disabling\n");
- gb_pwm_deactivate_operation(pwmc, pwm->hwpwm); + gb_pwm_deactivate_operation(chip, pwm->hwpwm); }
static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, @@ -212,22 +210,21 @@ static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, bool enabled = pwm->state.enabled; u64 period = state->period; u64 duty_cycle = state->duty_cycle; - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
/* Set polarity */ if (state->polarity != pwm->state.polarity) { if (enabled) { - gb_pwm_disable_operation(pwmc, pwm->hwpwm); + gb_pwm_disable_operation(chip, pwm->hwpwm); enabled = false; } - err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity); + err = gb_pwm_set_polarity_operation(chip, pwm->hwpwm, state->polarity); if (err) return err; }
if (!state->enabled) { if (enabled) - gb_pwm_disable_operation(pwmc, pwm->hwpwm); + gb_pwm_disable_operation(chip, pwm->hwpwm); return 0; }
@@ -243,13 +240,13 @@ static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (duty_cycle > period) duty_cycle = period;
- err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period); + err = gb_pwm_config_operation(chip, pwm->hwpwm, duty_cycle, period); if (err) return err;
/* enable/disable */ if (!enabled) - return gb_pwm_enable_operation(pwmc, pwm->hwpwm); + return gb_pwm_enable_operation(chip, pwm->hwpwm);
return 0; }
On 1/25/24 6:09 AM, Uwe Kleine-König wrote:
struct pwm_chip::dev is about to change. To not have to touch this driver in the same commit as struct pwm_chip::dev, use the macro provided for exactly this purpose.
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
I think I'd rather see the footprint of your change be much smaller than it is. Please see below.
-Alex
drivers/staging/greybus/pwm.c | 55 +++++++++++++++++------------------ 1 file changed, 26 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c index a3cb68cfa0f9..75e0518791d8 100644 --- a/drivers/staging/greybus/pwm.c +++ b/drivers/staging/greybus/pwm.c @@ -17,7 +17,6 @@ struct gb_pwm_chip { struct gb_connection *connection; u8 pwm_max; /* max pwm number */
- struct pwm_chip chip; };
@@ -39,9 +38,9 @@ static int gb_pwm_count_operation(struct gb_pwm_chip *pwmc) return 0; } -static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc,
u8 which)+static int gb_pwm_activate_operation(struct pwm_chip *chip, u8 which)
Why change the type of the argument here?
{
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_activate_request request; struct gbphy_device *gbphy_dev; int ret;
@@ -51,7 +50,7 @@ static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc, request.which = which;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
- gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));
Just make this line look like this:
gbphy_dev = to_gbphy_dev(pwmchip_parent(&pwmc->chip));
ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -64,9 +63,10 @@ static int gb_pwm_activate_operation(struct gb_pwm_chip *pwmc, return ret; } -static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc, +static int gb_pwm_deactivate_operation(struct pwm_chip *chip, u8 which)
Same question here.
{
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_deactivate_request request; struct gbphy_device *gbphy_dev; int ret;
@@ -76,7 +76,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc, request.which = which;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
- gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));
gbphy_dev = to_gbphy_dev(pwmchip_parent(&pwmc->chip));
ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -89,9 +89,10 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc, return ret; } -static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc, +static int gb_pwm_config_operation(struct pwm_chip *chip, u8 which, u32 duty, u32 period)
And here.
{
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_config_request request; struct gbphy_device *gbphy_dev; int ret;
@@ -103,7 +104,7 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc, request.duty = cpu_to_le32(duty); request.period = cpu_to_le32(period);
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
- gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));
gbphy_dev = to_gbphy_dev(pwmchip_parent(&pwmc->chip));
ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -116,9 +117,10 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc, return ret; } -static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc, +static int gb_pwm_set_polarity_operation(struct pwm_chip *chip, u8 which, u8 polarity)
And here.
{
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_polarity_request request; struct gbphy_device *gbphy_dev; int ret;
@@ -129,7 +131,7 @@ static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc, request.which = which; request.polarity = polarity;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
- gbphy_dev = to_gbphy_dev(pwmchip_parent(chip));
gbphy_dev = to_gbphy_dev(pwmchip_parent(&pwmc->chip));
ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret; @@ -142,9 +144,9 @@ static int gb_pwm_set_polarity_operation(struct gb_pwm_chip *pwmc, return ret; } -static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc,
u8 which)+static int gb_pwm_enable_operation(struct pwm_chip *chip, u8 which)
And on and on.
{
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_enable_request request; struct gbphy_device *gbphy_dev; int ret;
@@ -154,7 +156,7 @@ static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc, request.which = which;
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
- gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); ret = gbphy_runtime_get_sync(gbphy_dev); if (ret) return ret;
@@ -167,9 +169,9 @@ static int gb_pwm_enable_operation(struct gb_pwm_chip *pwmc, return ret; } -static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc,
u8 which)+static int gb_pwm_disable_operation(struct pwm_chip *chip, u8 which) {
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_pwm_disable_request request; struct gbphy_device *gbphy_dev; int ret;
@@ -182,7 +184,7 @@ static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc, ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_DISABLE, &request, sizeof(request), NULL, 0);
- gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
- gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); gbphy_runtime_put_autosuspend(gbphy_dev);
return ret; @@ -190,19 +192,15 @@ static int gb_pwm_disable_operation(struct gb_pwm_chip *pwmc, static int gb_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) {
I guess my question now is, why don't this function and the next one take a gb_pwm_chip pointer as argument like the rest... (Not your problem--don't "fix" this in this series.)
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
- return gb_pwm_activate_operation(pwmc, pwm->hwpwm);
- return gb_pwm_activate_operation(chip, pwm->hwpwm); };
static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) {
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
- if (pwm_is_enabled(pwm))
dev_warn(chip->dev, "freeing PWM device without disabling\n");
dev_warn(pwmchip_parent(chip), "freeing PWM device without disabling\n");
- gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
- gb_pwm_deactivate_operation(chip, pwm->hwpwm); }
static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, @@ -212,22 +210,21 @@ static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, bool enabled = pwm->state.enabled; u64 period = state->period; u64 duty_cycle = state->duty_cycle;
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
/* Set polarity */ if (state->polarity != pwm->state.polarity) { if (enabled) {
gb_pwm_disable_operation(pwmc, pwm->hwpwm);
gb_pwm_disable_operation(chip, pwm->hwpwm); enabled = false;
err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
err = gb_pwm_set_polarity_operation(chip, pwm->hwpwm, state->polarity);if (!state->enabled) { if (enabled)
gb_pwm_disable_operation(pwmc, pwm->hwpwm);
gb_pwm_disable_operation(chip, pwm->hwpwm);@@ -243,13 +240,13 @@ static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (duty_cycle > period) duty_cycle = period;
- err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
- err = gb_pwm_config_operation(chip, pwm->hwpwm, duty_cycle, period); if (err) return err;
/* enable/disable */ if (!enabled)
return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
return gb_pwm_enable_operation(chip, pwm->hwpwm);return 0; }
On Fri, Jan 26, 2024 at 08:46:15AM -0600, Alex Elder wrote:
On 1/25/24 6:09 AM, Uwe Kleine-König wrote:
struct pwm_chip::dev is about to change. To not have to touch this driver in the same commit as struct pwm_chip::dev, use the macro provided for exactly this purpose.
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
I think I'd rather see the footprint of your change be much smaller than it is. Please see below.
You have noticed already yourself while reviewing a later patch touching this driver. But to be explicit here: Later it's not trivial any more to get the pwm_chip from a gb_pwm_chip. So I changed some functions to take a pwm_chip. This should be mentioned in the commit log though. I will improve that in the next iteration, and maybe split the introduction of pwmchip_parent() and the parameter changes in two changes.
Best regards Uwe
These functions are useful to store and query driver private data a After struct pwm_chip got its own struct device, this can make use of dev_get_drvdata() and dev_set_drvdata() on that device. These functions are required already now to convert drivers to pwmchip_alloc() which must happen before changing pwm_chip::dev.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de --- include/linux/pwm.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/include/linux/pwm.h b/include/linux/pwm.h index d7966918f301..2c49d2fe2fe7 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -272,6 +272,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 + * @driver_data: Private pointer for driver specific info * @pwms: array of PWM devices allocated by the framework */ struct pwm_chip { @@ -286,6 +287,7 @@ struct pwm_chip { bool atomic;
/* only used internally by the PWM framework */ + void *driver_data; struct pwm_device *pwms; };
@@ -294,6 +296,24 @@ static inline struct device *pwmchip_parent(struct pwm_chip *chip) return chip->dev; }
+static inline void *pwmchip_get_drvdata(struct pwm_chip *chip) +{ + /* + * After pwm_chip got a dedicated struct device, this can be replaced by + * dev_get_drvdata(&chip->dev); + */ + return chip->driver_data; +} + +static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data) +{ + /* + * After pwm_chip got a dedicated struct device, this can be replaced by + * dev_set_drvdata(&chip->dev, data); + */ + chip->driver_data = data; +} + #if IS_ENABLED(CONFIG_PWM) /* PWM user APIs */ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
Il 25/01/24 13:09, Uwe Kleine-König ha scritto:
These functions are useful to store and query driver private data a After struct pwm_chip got its own struct device, this can make use of dev_get_drvdata() and dev_set_drvdata() on that device. These functions are required already now to convert drivers to pwmchip_alloc() which must happen before changing pwm_chip::dev.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
Reviewed-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com
This prepares the greybus pwm driver to further changes of the pwm core outlined in the commit introducing devm_pwmchip_alloc(). There is no intended semantical change and the driver should behave as before.
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de --- drivers/staging/greybus/pwm.c | 75 ++++++++++------------------------- 1 file changed, 20 insertions(+), 55 deletions(-)
diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c index 75e0518791d8..a90f41c374dc 100644 --- a/drivers/staging/greybus/pwm.c +++ b/drivers/staging/greybus/pwm.c @@ -16,26 +16,11 @@
struct gb_pwm_chip { struct gb_connection *connection; - u8 pwm_max; /* max pwm number */ - struct pwm_chip chip; };
static inline struct gb_pwm_chip *pwm_chip_to_gb_pwm_chip(struct pwm_chip *chip) { - return container_of(chip, struct gb_pwm_chip, chip); -} - -static int gb_pwm_count_operation(struct gb_pwm_chip *pwmc) -{ - struct gb_pwm_count_response response; - int ret; - - ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_PWM_COUNT, - NULL, 0, &response, sizeof(response)); - if (ret) - return ret; - pwmc->pwm_max = response.count; - return 0; + return pwmchip_get_drvdata(chip); }
static int gb_pwm_activate_operation(struct pwm_chip *chip, u8 which) @@ -45,9 +30,6 @@ static int gb_pwm_activate_operation(struct pwm_chip *chip, u8 which) struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max) - return -EINVAL; - request.which = which;
gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); @@ -71,9 +53,6 @@ static int gb_pwm_deactivate_operation(struct pwm_chip *chip, struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max) - return -EINVAL; - request.which = which;
gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); @@ -97,9 +76,6 @@ static int gb_pwm_config_operation(struct pwm_chip *chip, struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max) - return -EINVAL; - request.which = which; request.duty = cpu_to_le32(duty); request.period = cpu_to_le32(period); @@ -125,9 +101,6 @@ static int gb_pwm_set_polarity_operation(struct pwm_chip *chip, struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max) - return -EINVAL; - request.which = which; request.polarity = polarity;
@@ -151,9 +124,6 @@ static int gb_pwm_enable_operation(struct pwm_chip *chip, u8 which) struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max) - return -EINVAL; - request.which = which;
gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); @@ -176,9 +146,6 @@ static int gb_pwm_disable_operation(struct pwm_chip *chip, u8 which) struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max) - return -EINVAL; - request.which = which;
ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_DISABLE, @@ -263,38 +230,37 @@ static int gb_pwm_probe(struct gbphy_device *gbphy_dev, struct gb_connection *connection; struct gb_pwm_chip *pwmc; struct pwm_chip *chip; + struct gb_pwm_count_response response; int ret;
- pwmc = kzalloc(sizeof(*pwmc), GFP_KERNEL); - if (!pwmc) - return -ENOMEM; - connection = gb_connection_create(gbphy_dev->bundle, le16_to_cpu(gbphy_dev->cport_desc->id), NULL); - if (IS_ERR(connection)) { - ret = PTR_ERR(connection); - goto exit_pwmc_free; + if (IS_ERR(connection)) + return PTR_ERR(connection); + + ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_PWM_COUNT, + NULL, 0, &response, sizeof(response)); + if (ret) + goto exit_connection_destroy; + + chip = devm_pwmchip_alloc(&gbphy_dev->dev, response.count, sizeof(*pwmc)); + if (IS_ERR(chip)) { + ret = PTR_ERR(chip); + goto exit_connection_destroy; }
+ pwmc = pwm_chip_to_gb_pwm_chip(chip); pwmc->connection = connection; + gb_connection_set_data(connection, pwmc); - gb_gbphy_set_data(gbphy_dev, pwmc); + gb_gbphy_set_data(gbphy_dev, chip);
ret = gb_connection_enable(connection); if (ret) goto exit_connection_destroy;
- /* Query number of pwms present */ - ret = gb_pwm_count_operation(pwmc); - if (ret) - goto exit_connection_disable; - - chip = &pwmc->chip; - - chip->dev = &gbphy_dev->dev; chip->ops = &gb_pwm_ops; - chip->npwm = pwmc->pwm_max + 1;
ret = pwmchip_add(chip); if (ret) { @@ -310,14 +276,13 @@ static int gb_pwm_probe(struct gbphy_device *gbphy_dev, gb_connection_disable(connection); exit_connection_destroy: gb_connection_destroy(connection); -exit_pwmc_free: - kfree(pwmc); return ret; }
static void gb_pwm_remove(struct gbphy_device *gbphy_dev) { - struct gb_pwm_chip *pwmc = gb_gbphy_get_data(gbphy_dev); + struct pwm_chip *chip = gb_gbphy_get_data(gbphy_dev); + struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_connection *connection = pwmc->connection; int ret;
@@ -325,7 +290,7 @@ static void gb_pwm_remove(struct gbphy_device *gbphy_dev) if (ret) gbphy_runtime_get_noresume(gbphy_dev);
- pwmchip_remove(&pwmc->chip); + pwmchip_remove(chip); gb_connection_disable(connection); gb_connection_destroy(connection); kfree(pwmc);
On 1/25/24 6:10 AM, Uwe Kleine-König wrote:
This prepares the greybus pwm driver to further changes of the pwm core outlined in the commit introducing devm_pwmchip_alloc(). There is no intended semantical change and the driver should behave as before.
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
I think you are changing more than you need to in this code.
drivers/staging/greybus/pwm.c | 75 ++++++++++------------------------- 1 file changed, 20 insertions(+), 55 deletions(-)
diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c index 75e0518791d8..a90f41c374dc 100644 --- a/drivers/staging/greybus/pwm.c +++ b/drivers/staging/greybus/pwm.c @@ -16,26 +16,11 @@ struct gb_pwm_chip { struct gb_connection *connection;
- u8 pwm_max; /* max pwm number */
- struct pwm_chip chip; };
static inline struct gb_pwm_chip *pwm_chip_to_gb_pwm_chip(struct pwm_chip *chip) {
- return container_of(chip, struct gb_pwm_chip, chip);
Now I understand why you were changing the type of the argument passed to all those functions. I still don't think you need to do that though.
-}
-static int gb_pwm_count_operation(struct gb_pwm_chip *pwmc)
This function logically isolates doing the operation from the rest of the code. I'd rather you not get rid of it.
-{
- struct gb_pwm_count_response response;
- int ret;
- ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_PWM_COUNT,
NULL, 0, &response, sizeof(response));- if (ret)
return ret;- pwmc->pwm_max = response.count;
- return 0;
- return pwmchip_get_drvdata(chip); }
static int gb_pwm_activate_operation(struct pwm_chip *chip, u8 which) @@ -45,9 +30,6 @@ static int gb_pwm_activate_operation(struct pwm_chip *chip, u8 which) struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max)
return -EINVAL;- request.which = which;
gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); @@ -71,9 +53,6 @@ static int gb_pwm_deactivate_operation(struct pwm_chip *chip, struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max)
return -EINVAL;- request.which = which;
gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); @@ -97,9 +76,6 @@ static int gb_pwm_config_operation(struct pwm_chip *chip, struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max)
return -EINVAL;- request.which = which; request.duty = cpu_to_le32(duty); request.period = cpu_to_le32(period);
@@ -125,9 +101,6 @@ static int gb_pwm_set_polarity_operation(struct pwm_chip *chip, struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max)
return -EINVAL;- request.which = which; request.polarity = polarity;
@@ -151,9 +124,6 @@ static int gb_pwm_enable_operation(struct pwm_chip *chip, u8 which) struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max)
return -EINVAL;- request.which = which;
gbphy_dev = to_gbphy_dev(pwmchip_parent(chip)); @@ -176,9 +146,6 @@ static int gb_pwm_disable_operation(struct pwm_chip *chip, u8 which) struct gbphy_device *gbphy_dev; int ret;
- if (which > pwmc->pwm_max)
return -EINVAL;- request.which = which;
ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_DISABLE, @@ -263,38 +230,37 @@ static int gb_pwm_probe(struct gbphy_device *gbphy_dev, struct gb_connection *connection; struct gb_pwm_chip *pwmc; struct pwm_chip *chip;
- struct gb_pwm_count_response response; int ret;
- pwmc = kzalloc(sizeof(*pwmc), GFP_KERNEL);
- if (!pwmc)
return -ENOMEM;- connection = gb_connection_create(gbphy_dev->bundle, le16_to_cpu(gbphy_dev->cport_desc->id), NULL);
- if (IS_ERR(connection)) {
ret = PTR_ERR(connection);goto exit_pwmc_free;
- if (IS_ERR(connection))
return PTR_ERR(connection);
This is actually a bug fix that should probably be done separately, prior to this series.
- ret = gb_operation_sync(pwmc->connection, GB_PWM_TYPE_PWM_COUNT,
NULL, 0, &response, sizeof(response));- if (ret)
goto exit_connection_destroy;
You didn't mention in your header that you were getting rid of gb_pwm_count_operation(), and as I said above, I don't think you should. Just keep the existing code as it was and focus only on your conversion to attaching driver data to the PWM chip structure.
- chip = devm_pwmchip_alloc(&gbphy_dev->dev, response.count, sizeof(*pwmc));
- if (IS_ERR(chip)) {
ret = PTR_ERR(chip);goto exit_connection_destroy;
- pwmc = pwm_chip_to_gb_pwm_chip(chip); pwmc->connection = connection;
- gb_connection_set_data(connection, pwmc);
- gb_gbphy_set_data(gbphy_dev, pwmc);
- gb_gbphy_set_data(gbphy_dev, chip);
I'm pretty sure driver data should still be the Greybus structure, otherwise you're really changing the way this works (most likely in a way that's different from other Greybus drivers.
ret = gb_connection_enable(connection); if (ret) goto exit_connection_destroy;
- /* Query number of pwms present */
- ret = gb_pwm_count_operation(pwmc);
- if (ret)
goto exit_connection_disable;- chip = &pwmc->chip;
- chip->dev = &gbphy_dev->dev; chip->ops = &gb_pwm_ops;
- chip->npwm = pwmc->pwm_max + 1;
ret = pwmchip_add(chip); if (ret) { @@ -310,14 +276,13 @@ static int gb_pwm_probe(struct gbphy_device *gbphy_dev, gb_connection_disable(connection); exit_connection_destroy: gb_connection_destroy(connection); -exit_pwmc_free:
- kfree(pwmc); return ret; }
static void gb_pwm_remove(struct gbphy_device *gbphy_dev) {
- struct gb_pwm_chip *pwmc = gb_gbphy_get_data(gbphy_dev);
- struct pwm_chip *chip = gb_gbphy_get_data(gbphy_dev);
- struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip); struct gb_connection *connection = pwmc->connection; int ret;
@@ -325,7 +290,7 @@ static void gb_pwm_remove(struct gbphy_device *gbphy_dev) if (ret) gbphy_runtime_get_noresume(gbphy_dev);
- pwmchip_remove(&pwmc->chip);
- pwmchip_remove(chip); gb_connection_disable(connection); gb_connection_destroy(connection); kfree(pwmc);
Hello Alex,
On Fri, Jan 26, 2024 at 09:08:15AM -0600, Alex Elder wrote:
On 1/25/24 6:10 AM, Uwe Kleine-König wrote:
- chip = devm_pwmchip_alloc(&gbphy_dev->dev, response.count, sizeof(*pwmc));
- if (IS_ERR(chip)) {
ret = PTR_ERR(chip);goto exit_connection_destroy;- pwmc = pwm_chip_to_gb_pwm_chip(chip); pwmc->connection = connection;
- gb_connection_set_data(connection, pwmc);
- gb_gbphy_set_data(gbphy_dev, pwmc);
- gb_gbphy_set_data(gbphy_dev, chip);
I'm pretty sure driver data should still be the Greybus structure, otherwise you're really changing the way this works (most likely in a way that's different from other Greybus drivers.
The problem is that you cannot easily get the chip from pwmc unless you add a pointer to struct gb_pwm_chip.
Regarding your other feedback: You're right, this patch is more intrusive than it should be. I wonder what went wrong. I'll do some research and rework accordingly. Probably I squashed some things together that should be separate patches.
thanks for your feedback Uwe
This function allocates a struct pwm_chip and driver data. Compared to the status quo the split into pwm_chip and driver data is new, otherwise it doesn't change anything relevant (yet).
The intention is that after all drivers are switched to use this allocation function, its possible to add a struct device to struct pwm_chip to properly track the latter's lifetime without touching all drivers again. Proper lifetime tracking is a necessary precondition to introduce character device support for PWMs (that implements atomic setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm userspace support).
The new function pwmchip_priv() (obviously?) only works for chips allocated with devm_pwmchip_alloc().
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de --- .../driver-api/driver-model/devres.rst | 1 + Documentation/driver-api/pwm.rst | 10 ++++---- drivers/pwm/core.c | 25 +++++++++++++++++++ include/linux/pwm.h | 2 ++ 4 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst index c5f99d834ec5..e4df72c408d2 100644 --- a/Documentation/driver-api/driver-model/devres.rst +++ b/Documentation/driver-api/driver-model/devres.rst @@ -420,6 +420,7 @@ POWER devm_reboot_mode_unregister()
PWM + devm_pwmchip_alloc() devm_pwmchip_add() devm_pwm_get() devm_fwnode_pwm_get() diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst index 3c28ccc4b611..cee66c7f0335 100644 --- a/Documentation/driver-api/pwm.rst +++ b/Documentation/driver-api/pwm.rst @@ -143,11 +143,11 @@ to implement the pwm_*() functions itself. This means that it's impossible to have multiple PWM drivers in the system. For this reason it's mandatory for new drivers to use the generic PWM framework.
-A new PWM controller/chip can be added using pwmchip_add() and removed -again with pwmchip_remove(). pwmchip_add() takes a filled in struct -pwm_chip as argument which provides a description of the PWM chip, the -number of PWM devices provided by the chip and the chip-specific -implementation of the supported PWM operations to the framework. +A new PWM controller/chip can be allocated using devm_pwmchip_alloc, then added +using pwmchip_add() and removed again with pwmchip_remove(). pwmchip_add() +takes a filled in struct pwm_chip as argument which provides a description of +the PWM chip, the number of PWM devices provided by the chip and the +chip-specific implementation of the supported PWM operations to the framework.
When implementing polarity support in a PWM driver, make sure to respect the signal conventions in the PWM framework. By definition, normal polarity diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 1b4c3d0caa82..b821a2b0b172 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -454,6 +454,31 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args) } EXPORT_SYMBOL_GPL(of_pwm_single_xlate);
+static void *pwmchip_priv(struct pwm_chip *chip) +{ + return (void *)chip + sizeof(*chip); +} + +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) +{ + struct pwm_chip *chip; + size_t alloc_size; + + alloc_size = size_add(sizeof(*chip), sizeof_priv); + + chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); + if (!chip) + return ERR_PTR(-ENOMEM); + + chip->dev = parent; + chip->npwm = npwm; + + pwmchip_set_drvdata(chip, pwmchip_priv(chip)); + + return chip; +} +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); + static void of_pwmchip_add(struct pwm_chip *chip) { if (!chip->dev || !chip->dev->of_node) diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 2c49d2fe2fe7..8bc7504aa7d4 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -403,6 +403,8 @@ static inline bool pwm_might_sleep(struct pwm_device *pwm) int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout);
+struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv); + int __pwmchip_add(struct pwm_chip *chip, struct module *owner); #define pwmchip_add(chip) __pwmchip_add(chip, THIS_MODULE) void pwmchip_remove(struct pwm_chip *chip);
On 1/25/24 6:09 AM, Uwe Kleine-König wrote:
This function allocates a struct pwm_chip and driver data. Compared to the status quo the split into pwm_chip and driver data is new, otherwise it doesn't change anything relevant (yet).
The intention is that after all drivers are switched to use this allocation function, its possible to add a struct device to struct pwm_chip to properly track the latter's lifetime without touching all drivers again. Proper lifetime tracking is a necessary precondition to introduce character device support for PWMs (that implements atomic setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm userspace support).
The new function pwmchip_priv() (obviously?) only works for chips allocated with devm_pwmchip_alloc().
I think this looks good. Two questions: - Should you explicitly align the private data? Or do you believe the default alignment (currently pointer size aligned) is adequate? - Is there a non-devres version of the allocation function?
-Alex
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
.../driver-api/driver-model/devres.rst | 1 + Documentation/driver-api/pwm.rst | 10 ++++---- drivers/pwm/core.c | 25 +++++++++++++++++++ include/linux/pwm.h | 2 ++ 4 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst index c5f99d834ec5..e4df72c408d2 100644 --- a/Documentation/driver-api/driver-model/devres.rst +++ b/Documentation/driver-api/driver-model/devres.rst @@ -420,6 +420,7 @@ POWER devm_reboot_mode_unregister() PWM
- devm_pwmchip_alloc() devm_pwmchip_add() devm_pwm_get() devm_fwnode_pwm_get()
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst index 3c28ccc4b611..cee66c7f0335 100644 --- a/Documentation/driver-api/pwm.rst +++ b/Documentation/driver-api/pwm.rst @@ -143,11 +143,11 @@ to implement the pwm_*() functions itself. This means that it's impossible to have multiple PWM drivers in the system. For this reason it's mandatory for new drivers to use the generic PWM framework. -A new PWM controller/chip can be added using pwmchip_add() and removed -again with pwmchip_remove(). pwmchip_add() takes a filled in struct -pwm_chip as argument which provides a description of the PWM chip, the -number of PWM devices provided by the chip and the chip-specific -implementation of the supported PWM operations to the framework. +A new PWM controller/chip can be allocated using devm_pwmchip_alloc, then added +using pwmchip_add() and removed again with pwmchip_remove(). pwmchip_add() +takes a filled in struct pwm_chip as argument which provides a description of +the PWM chip, the number of PWM devices provided by the chip and the +chip-specific implementation of the supported PWM operations to the framework. When implementing polarity support in a PWM driver, make sure to respect the signal conventions in the PWM framework. By definition, normal polarity diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 1b4c3d0caa82..b821a2b0b172 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -454,6 +454,31 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args) } EXPORT_SYMBOL_GPL(of_pwm_single_xlate); +static void *pwmchip_priv(struct pwm_chip *chip) +{
- return (void *)chip + sizeof(*chip);
+}
+struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) +{
- struct pwm_chip *chip;
- size_t alloc_size;
- alloc_size = size_add(sizeof(*chip), sizeof_priv);
- chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL);
- if (!chip)
return ERR_PTR(-ENOMEM);- chip->dev = parent;
- chip->npwm = npwm;
- pwmchip_set_drvdata(chip, pwmchip_priv(chip));
- return chip;
+} +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc);
- static void of_pwmchip_add(struct pwm_chip *chip) { if (!chip->dev || !chip->dev->of_node)
diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 2c49d2fe2fe7..8bc7504aa7d4 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -403,6 +403,8 @@ static inline bool pwm_might_sleep(struct pwm_device *pwm) int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout); +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv);
- int __pwmchip_add(struct pwm_chip *chip, struct module *owner); #define pwmchip_add(chip) __pwmchip_add(chip, THIS_MODULE) void pwmchip_remove(struct pwm_chip *chip);
Hello Alex,
On Fri, Jan 26, 2024 at 08:56:33AM -0600, Alex Elder wrote:
On 1/25/24 6:09 AM, Uwe Kleine-König wrote:
This function allocates a struct pwm_chip and driver data. Compared to the status quo the split into pwm_chip and driver data is new, otherwise it doesn't change anything relevant (yet).
The intention is that after all drivers are switched to use this allocation function, its possible to add a struct device to struct pwm_chip to properly track the latter's lifetime without touching all drivers again. Proper lifetime tracking is a necessary precondition to introduce character device support for PWMs (that implements atomic setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm userspace support).
The new function pwmchip_priv() (obviously?) only works for chips allocated with devm_pwmchip_alloc().
I think this looks good. Two questions:
- Should you explicitly align the private data? Or do you believe the default alignment (currently pointer size aligned) is adequate?
I'm not aware of a requirement for a higher order alignment (but I might well miss something). I did my tests on arm, nothing exploded there. Maybe the conservative approach of asserting the same alignment as kmalloc would be a good idea. I'll think and research about that.
iio uses ARCH_DMA_MINALIGN, net uses 32 (NETDEV_ALIGN).
- Is there a non-devres version of the allocation function?
Patch #109 introduces a non-devres variant. As it's not used it's a static function though. Can easily be changed is a use case pops up.
Best regards Uwe
-
Alex Elder -
AngeloGioacchino Del Regno -
Florian Fainelli -
Uwe Kleine-König