[PATCH v3 000/108] pwm: Fix lifetime issues for pwm_chips

[PATCH v3 000/108] pwm: Fix lifetime issues for pwm_chips

[Uwe Kleine-König]

Hello,

this is v3 of the series improving life-time tracking for PWM chips. The
urgency is gone as device links now work as expected and so all
in-kernel users are fine since commit 2e84dc379200 ("driver core:
Release all resources during unbind before updating device links").

However proper lifetime tracking is a precondition to have robust
character device support, as we cannot kill a userspace process if the
used pwm driver goes away.

Changes since v2:

 - Cc: the relevant maintainers for wider testing/review audience
 - Rebase to v6.7-rc1 + https://lore.kernel.org/linux-pwm/20231121112029.gyv3gqirlycysyr4@pengutronix.de
 - Improvements for things pointed out during review and my own
   findings here and there.
 - Implementation for a few more ioctls in the WIP commit that adds
   character support

To go forward I'd like to get in patches up to #103 (i.e. adding
pwmchip_parent() (#2), devm_pwmchip_alloc() (#37) and the conversions of
the drivers to make use of these additions).

The few commits that touch drivers not living in drivers/pwm (i.e. #36,
#100-#103) can go in either via the pwm tree with the rest, or later
---when the used functions are in---via their trees.

After all in-tree drivers are prepared with the patches up to #103, we
can think about when and how we go on with the remaining bits.

Note that patch #104 breaks all drivers that don't use
devm_pwmchip_alloc(), so this is the commit that needs coordination with the
maintainers of

 drivers/gpio/gpio-mvebu.c
 drivers/gpu/drm/bridge/ti-sn65dsi86.c
 drivers/leds/rgb/leds-qcom-lpg.c
 drivers/staging/greybus/pwm.c

The motivation for this series is the last patch. It allows to control a
pwm device via ioctl. Compared to the sysfs API this already now has
some advantages:

- It changes all parameters in a single call.
  This simplifies things similar to the introduction of
  pwm_apply_state(). With sysfs it can happen that you want to
  switch polarity but that's refused because 

	pwm_get_state(mypwm, &state);
	state.polarity = new_value;

  sometimes yield an invalid state, e.g. because state.period is
  in some cases 0 after bootup. Theoretically it can even happen that you have
  to change two parameters before reaching an applicable state, then you're
  stuck with sysfs.

- It's faster than sysfs. In my measurements with stm32 about a factor
  4.

A userspace lib to make use of this can be found at
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git/ .
It makes use of the character devices if available and falls back to
sysfs. So it's somewhat useful already now.

Best regards
Uwe

Uwe Kleine-König (108):
  pwm: cros-ec: Change prototype of helper 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
  staging: greybus: pwm: Make use of pwmchip_parent() macro
  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: Add more locking
  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         |  25 +-
 drivers/leds/rgb/leds-qcom-lpg.c              |  30 +-
 drivers/pwm/Kconfig                           |   4 -
 drivers/pwm/Makefile                          |   3 +-
 drivers/pwm/core.c                            | 480 +++++++++++++++---
 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                     |  17 +-
 drivers/pwm/pwm-berlin.c                      |  29 +-
 drivers/pwm/pwm-brcmstb.c                     |  17 +-
 drivers/pwm/pwm-clk.c                         |  27 +-
 drivers/pwm/pwm-clps711x.c                    |  21 +-
 drivers/pwm/pwm-crc.c                         |  26 +-
 drivers/pwm/pwm-cros-ec.c                     |  51 +-
 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                        |  17 +-
 drivers/pwm/pwm-imx27.c                       |  26 +-
 drivers/pwm/pwm-intel-lgm.c                   |  17 +-
 drivers/pwm/pwm-iqs620a.c                     |  37 +-
 drivers/pwm/pwm-jz4740.c                      |  35 +-
 drivers/pwm/pwm-keembay.c                     |  17 +-
 drivers/pwm/pwm-lp3943.c                      |  17 +-
 drivers/pwm/pwm-lpc18xx-sct.c                 |  35 +-
 drivers/pwm/pwm-lpc32xx.c                     |  19 +-
 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                    |  28 +-
 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                |  46 +-
 drivers/pwm/pwm-pca9685.c                     |  98 ++--
 drivers/pwm/pwm-pxa.c                         |  21 +-
 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                     |  38 +-
 drivers/pwm/pwm-samsung.c                     |  56 +-
 drivers/pwm/pwm-sifive.c                      |  30 +-
 drivers/pwm/pwm-sl28cpld.c                    |  13 +-
 drivers/pwm/pwm-spear.c                       |  17 +-
 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                           |  36 +-
 include/uapi/linux/pwm.h                      |  23 +
 81 files changed, 1651 insertions(+), 1291 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

base-commit: 869de350ff3834145273a6d39faedea878c6715a
-- 
2.42.0

[PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm sub-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.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/rgb/leds-qcom-lpg.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 68d82a682bf6..283227e02df6 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -77,7 +77,7 @@ struct lpg {
 
 	struct mutex lock;
 
-	struct pwm_chip pwm;
+	struct pwm_chip *pwm;
 
 	const struct lpg_data *data;
 
@@ -977,9 +977,15 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
 	return lpg_pattern_clear(led);
 }
 
+static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
+{
+	struct lpg **lpg = pwmchip_priv(chip);
+	return *lpg;
+}
+
 static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg *lpg = lpg_pwm_from_chip(chip);
 	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
 
 	return chan->in_use ? -EBUSY : 0;
@@ -995,7 +1001,7 @@ static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			 const struct pwm_state *state)
 {
-	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg *lpg = lpg_pwm_from_chip(chip);
 	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
 	int ret = 0;
 
@@ -1026,7 +1032,7 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 static int lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 			     struct pwm_state *state)
 {
-	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg *lpg = lpg_pwm_from_chip(chip);
 	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
 	unsigned int resolution;
 	unsigned int pre_div;
@@ -1089,13 +1095,19 @@ static const struct pwm_ops lpg_pwm_ops = {
 
 static int lpg_add_pwm(struct lpg *lpg)
 {
+	struct pwm_chip *chip;
 	int ret;
 
-	lpg->pwm.dev = lpg->dev;
-	lpg->pwm.npwm = lpg->num_channels;
-	lpg->pwm.ops = &lpg_pwm_ops;
+	lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels,
+					     sizeof(&lpg));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
 
-	ret = pwmchip_add(&lpg->pwm);
+	*(struct lpg **)pwmchip_priv(chip) = lpg;
+
+	chip->ops = &lpg_pwm_ops;
+
+	ret = pwmchip_add(chip);
 	if (ret)
 		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
 
@@ -1367,7 +1379,7 @@ static void lpg_remove(struct platform_device *pdev)
 {
 	struct lpg *lpg = platform_get_drvdata(pdev);
 
-	pwmchip_remove(&lpg->pwm);
+	pwmchip_remove(lpg->pwm);
 }
 
 static const struct lpg_data pm8916_pwm_data = {
-- 
2.42.0

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Lee Jones]

On Tue, 21 Nov 2023, Uwe Kleine-König wrote:

> This prepares the pwm sub-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.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/leds/rgb/leds-qcom-lpg.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)

Does this need to be taken in with the other 107 patches?

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

On Tue, Nov 21, 2023 at 03:16:40PM +0000, Lee Jones wrote:
> On Tue, 21 Nov 2023, Uwe Kleine-König wrote:
> 
> > This prepares the pwm sub-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.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/leds/rgb/leds-qcom-lpg.c | 30 +++++++++++++++++++++---------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> Does this need to be taken in with the other 107 patches?

Not necessarily. The dependencies are:

	- This patch depends on #37 which provides devm_pwmchip_alloc
	- Patches #104 and later depend on this one to be applied.

I didn't talk with Thierry yet about how this should be merged. If all
affected maintainers agree to let this all go via PWM this would be
good, but I can also live with going a bit slower and getting the pwm
bits in during the next merge window and the changes to the PWM drivers
not living in drivers/pwm adapted after that.

Best regards
Uwe

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

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

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Lee Jones]

On Tue, 21 Nov 2023, Uwe Kleine-König wrote:

> This prepares the pwm sub-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.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/leds/rgb/leds-qcom-lpg.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 68d82a682bf6..283227e02df6 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -77,7 +77,7 @@ struct lpg {
>  
>  	struct mutex lock;
>  
> -	struct pwm_chip pwm;
> +	struct pwm_chip *pwm;
>  
>  	const struct lpg_data *data;
>  
> @@ -977,9 +977,15 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
>  	return lpg_pattern_clear(led);
>  }
>  
> +static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
> +{
> +	struct lpg **lpg = pwmchip_priv(chip);
> +	return *lpg;
> +}

I don't have easy-vis into the other patches, but if this is a common
pattern, perhaps add a generic helper in <linux/pwm.h>?

>  static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> -	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg *lpg = lpg_pwm_from_chip(chip);
>  	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
>  
>  	return chan->in_use ? -EBUSY : 0;
> @@ -995,7 +1001,7 @@ static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			 const struct pwm_state *state)
>  {
> -	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg *lpg = lpg_pwm_from_chip(chip);
>  	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
>  	int ret = 0;
>  
> @@ -1026,7 +1032,7 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  static int lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  			     struct pwm_state *state)
>  {
> -	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg *lpg = lpg_pwm_from_chip(chip);
>  	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
>  	unsigned int resolution;
>  	unsigned int pre_div;
> @@ -1089,13 +1095,19 @@ static const struct pwm_ops lpg_pwm_ops = {
>  
>  static int lpg_add_pwm(struct lpg *lpg)
>  {
> +	struct pwm_chip *chip;
>  	int ret;
>  
> -	lpg->pwm.dev = lpg->dev;
> -	lpg->pwm.npwm = lpg->num_channels;
> -	lpg->pwm.ops = &lpg_pwm_ops;
> +	lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels,
> +					     sizeof(&lpg));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
>  
> -	ret = pwmchip_add(&lpg->pwm);
> +	*(struct lpg **)pwmchip_priv(chip) = lpg;

This is vile!

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Thierry Reding]

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

On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote:
> On Tue, 21 Nov 2023, Uwe Kleine-König wrote:
> 
> > This prepares the pwm sub-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.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/leds/rgb/leds-qcom-lpg.c | 30 +++++++++++++++++++++---------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > index 68d82a682bf6..283227e02df6 100644
> > --- a/drivers/leds/rgb/leds-qcom-lpg.c
> > +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > @@ -77,7 +77,7 @@ struct lpg {
> >  
> >  	struct mutex lock;
> >  
> > -	struct pwm_chip pwm;
> > +	struct pwm_chip *pwm;
> >  
> >  	const struct lpg_data *data;
> >  
> > @@ -977,9 +977,15 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
> >  	return lpg_pattern_clear(led);
> >  }
> >  
> > +static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
> > +{
> > +	struct lpg **lpg = pwmchip_priv(chip);
> > +	return *lpg;
> > +}
> 
> I don't have easy-vis into the other patches, but if this is a common
> pattern, perhaps add a generic helper in <linux/pwm.h>?
> 
> >  static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> > -	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg *lpg = lpg_pwm_from_chip(chip);
> >  	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> >  
> >  	return chan->in_use ? -EBUSY : 0;
> > @@ -995,7 +1001,7 @@ static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >  static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			 const struct pwm_state *state)
> >  {
> > -	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg *lpg = lpg_pwm_from_chip(chip);
> >  	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> >  	int ret = 0;
> >  
> > @@ -1026,7 +1032,7 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  static int lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			     struct pwm_state *state)
> >  {
> > -	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg *lpg = lpg_pwm_from_chip(chip);
> >  	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> >  	unsigned int resolution;
> >  	unsigned int pre_div;
> > @@ -1089,13 +1095,19 @@ static const struct pwm_ops lpg_pwm_ops = {
> >  
> >  static int lpg_add_pwm(struct lpg *lpg)
> >  {
> > +	struct pwm_chip *chip;
> >  	int ret;
> >  
> > -	lpg->pwm.dev = lpg->dev;
> > -	lpg->pwm.npwm = lpg->num_channels;
> > -	lpg->pwm.ops = &lpg_pwm_ops;
> > +	lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels,
> > +					     sizeof(&lpg));
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> >  
> > -	ret = pwmchip_add(&lpg->pwm);
> > +	*(struct lpg **)pwmchip_priv(chip) = lpg;
> 
> This is vile!

Indeed. This highlights one of the weaker parts of this whole design and
I really don't like it. The whole chip_alloc() construct works fine if
you have everything isolated nicely in a single driver and subsystem
(like you usually have in network land), but for cases like this where
things are spread throughout and a device is actually more than just a
PWM controller, it looks like we now have to work around this design
because it doesn't fit.

In fact, this reminds me about the "midlayer mistake" in many ways and
combined with what Bartosz said, I'm not sure this is going to hold up
very well the more special cases we get.

Thierry

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

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Thierry,

[adding Bartosz to Cc]

On Wed, Nov 22, 2023 at 06:15:32PM +0100, Thierry Reding wrote:
> On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote:
> > On Tue, 21 Nov 2023, Uwe Kleine-König wrote:
> > > +	*(struct lpg **)pwmchip_priv(chip) = lpg;
> > 
> > This is vile!
> 
> Indeed. This highlights one of the weaker parts of this whole design and
> I really don't like it. The whole chip_alloc() construct works fine if
> you have everything isolated nicely in a single driver and subsystem
> (like you usually have in network land), but for cases like this where
> things are spread throughout and a device is actually more than just a
> PWM controller, it looks like we now have to work around this design
> because it doesn't fit.

With the patch I suggested in reply to Lee's mail this is IMHO much
nicer and with that squashed into the patch under discussion I'd not
call this a work around.

Note that the thing you consider ugly here (I think) is that for
handling a combined "PWM + something else" device a separate allocation
is needed for stuff that embedded a struct pwm_chip before. With
Bartosz's approach you have that second allocation for all PWM devices
---and so the downsides hurt all PWM implementations and not only those
combined devices.

Also note that among the four external PWM drivers (i.e.

	drivers/staging/greybus/pwm.c
	drivers/leds/rgb/leds-qcom-lpg.c
	drivers/gpu/drm/bridge/ti-sn65dsi86.c
	drivers/gpio/gpio-mvebu.c

) only two suffer from this complication, because the other two use a
pwm specific private data structure already which seems natural to me.

> In fact, this reminds me about the "midlayer mistake" in many ways and
> combined with what Bartosz said, I'm not sure this is going to hold up
> very well the more special cases we get.

Where do you see a midlayer and how would that be better with what
Bartosz suggests?

The relevant difference between my approach and Bartosz's is that I put
the driver specific private data in the same allocation as the struct
pwm_chip and thus reducing the number of allocations and pointer
traversals. This difference IMHO doesn't qualify my approach as a
midlayer without Bartosz's qualifying, too.

Best regards
Uwe

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

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

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Thierry Reding]

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

On Thu, Nov 23, 2023 at 11:44:58AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> [adding Bartosz to Cc]
> 
> On Wed, Nov 22, 2023 at 06:15:32PM +0100, Thierry Reding wrote:
> > On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote:
> > > On Tue, 21 Nov 2023, Uwe Kleine-König wrote:
> > > > +	*(struct lpg **)pwmchip_priv(chip) = lpg;
> > > 
> > > This is vile!
> > 
> > Indeed. This highlights one of the weaker parts of this whole design and
> > I really don't like it. The whole chip_alloc() construct works fine if
> > you have everything isolated nicely in a single driver and subsystem
> > (like you usually have in network land), but for cases like this where
> > things are spread throughout and a device is actually more than just a
> > PWM controller, it looks like we now have to work around this design
> > because it doesn't fit.
> 
> With the patch I suggested in reply to Lee's mail this is IMHO much
> nicer and with that squashed into the patch under discussion I'd not
> call this a work around.
> 
> Note that the thing you consider ugly here (I think) is that for
> handling a combined "PWM + something else" device a separate allocation
> is needed for stuff that embedded a struct pwm_chip before. With
> Bartosz's approach you have that second allocation for all PWM devices
> ---and so the downsides hurt all PWM implementations and not only those
> combined devices.
> 
> Also note that among the four external PWM drivers (i.e.
> 
> 	drivers/staging/greybus/pwm.c
> 	drivers/leds/rgb/leds-qcom-lpg.c
> 	drivers/gpu/drm/bridge/ti-sn65dsi86.c
> 	drivers/gpio/gpio-mvebu.c
> 
> ) only two suffer from this complication, because the other two use a
> pwm specific private data structure already which seems natural to me.

That's true for now, but new drivers get added all the time, so anything
we do here should be as future proof as we can make it.

> > In fact, this reminds me about the "midlayer mistake" in many ways and
> > combined with what Bartosz said, I'm not sure this is going to hold up
> > very well the more special cases we get.
> 
> Where do you see a midlayer and how would that be better with what
> Bartosz suggests?

I wasn't saying that this was a midlayer but rather that it reminds me
of one and the restrictions that it comes with.

Right now all of these drivers work just fine and we don't need any of
these weird assignments due to the single allocation. They all neatly
plug into whatever other drivers or subsystems do.

> The relevant difference between my approach and Bartosz's is that I put
> the driver specific private data in the same allocation as the struct
> pwm_chip and thus reducing the number of allocations and pointer
> traversals. This difference IMHO doesn't qualify my approach as a
> midlayer without Bartosz's qualifying, too.

The solution that Bartosz proposed in his talk has two big advantages:
it can potentially be generalized to a number of subsystems, which means
that eventually we may get an actual library that would allow this stuff
to be unified across subsystems without everyone having to invent their
own and fix the same bugs. Secondly it also puts the lifetime management
where it belongs: in the subsystem. Drivers don't really have to care
about lifetime management of whatever they expose. When they are
unloaded, they should only need to let the subsystem know that they're
gone and then the subsystem can take appropriate action.

There are other advantages as well, mostly derived from the above: the
patch series to implement this can probably be something like 5 patches,
so we don't actually need to touch every driver, because the drivers
themselves are not the issue. It's how the subsystem will expose them
via chardev (or already exposes them via sysfs) that's really the
problem. The only place where it makes sense to fix this is in the
subsystem. Drivers don't need to be concerned about this.

Thierry

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

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Thierry,

On Fri, Nov 24, 2023 at 01:27:21PM +0100, Thierry Reding wrote:
> On Thu, Nov 23, 2023 at 11:44:58AM +0100, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > [adding Bartosz to Cc]
> > 
> > On Wed, Nov 22, 2023 at 06:15:32PM +0100, Thierry Reding wrote:
> > > On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote:
> > > > On Tue, 21 Nov 2023, Uwe Kleine-König wrote:
> > > > > +	*(struct lpg **)pwmchip_priv(chip) = lpg;
> > > > 
> > > > This is vile!
> > > 
> > > Indeed. This highlights one of the weaker parts of this whole design and
> > > I really don't like it. The whole chip_alloc() construct works fine if
> > > you have everything isolated nicely in a single driver and subsystem
> > > (like you usually have in network land), but for cases like this where
> > > things are spread throughout and a device is actually more than just a
> > > PWM controller, it looks like we now have to work around this design
> > > because it doesn't fit.
> > 
> > With the patch I suggested in reply to Lee's mail this is IMHO much
> > nicer and with that squashed into the patch under discussion I'd not
> > call this a work around.
> > 
> > Note that the thing you consider ugly here (I think) is that for
> > handling a combined "PWM + something else" device a separate allocation
> > is needed for stuff that embedded a struct pwm_chip before. With
> > Bartosz's approach you have that second allocation for all PWM devices
> > ---and so the downsides hurt all PWM implementations and not only those
> > combined devices.
> > 
> > Also note that among the four external PWM drivers (i.e.
> > 
> > 	drivers/staging/greybus/pwm.c
> > 	drivers/leds/rgb/leds-qcom-lpg.c
> > 	drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > 	drivers/gpio/gpio-mvebu.c
> > 
> > ) only two suffer from this complication, because the other two use a
> > pwm specific private data structure already which seems natural to me.
> 
> That's true for now, but new drivers get added all the time, so anything
> we do here should be as future proof as we can make it.

If drivers are added with a sane separation between their functionality,
my approach doesn't result in these complications. See
https://lore.kernel.org/dri-devel/20231123175425.496956-1-u.kleine-koenig@pengutronix.de
for how this could look. With that applied, the ti-sn65dsi86 driver can
be nicely adapted.

So yes, if you have an ugly driver, the pwm support cannot be prettier.
I can live with that. You could even sell this as an advantage.

> > > In fact, this reminds me about the "midlayer mistake" in many ways and
> > > combined with what Bartosz said, I'm not sure this is going to hold up
> > > very well the more special cases we get.
> > 
> > Where do you see a midlayer and how would that be better with what
> > Bartosz suggests?
> 
> I wasn't saying that this was a midlayer but rather that it reminds me
> of one and the restrictions that it comes with.
> 
> Right now all of these drivers work just fine and we don't need any of
> these weird assignments due to the single allocation. They all neatly
> plug into whatever other drivers or subsystems do.

Do you see the advantages of my approach, too?

> > The relevant difference between my approach and Bartosz's is that I put
> > the driver specific private data in the same allocation as the struct
> > pwm_chip and thus reducing the number of allocations and pointer
> > traversals. This difference IMHO doesn't qualify my approach as a
> > midlayer without Bartosz's qualifying, too.
> 
> The solution that Bartosz proposed in his talk has two big advantages:
> it can potentially be generalized to a number of subsystems, which means
> that eventually we may get an actual library that would allow this stuff
> to be unified across subsystems without everyone having to invent their
> own and fix the same bugs.

Can you please point out the relevant difference between Bartosz's and
my approach that makes his generalizable but not mine? Also I don't see
much in the pwm core that could still benefit from such a
generalisation.

> Secondly it also puts the lifetime management
> where it belongs: in the subsystem. Drivers don't really have to care
> about lifetime management of whatever they expose. When they are
> unloaded, they should only need to let the subsystem know that they're
> gone and then the subsystem can take appropriate action.

I understand your words, but I don't see that critic to apply to my
patch set. Handling consumers of unloaded drivers is completely in the
core. If you don't agree, can you please point your finger on any of the
drivers adapted here that I might understand what you mean?
(OK, we need a one-time conversion of all drivers to an abstraction that
allows the core to handle the lifetime management. That's something that
my approach has in common with Bartosz's.)

> There are other advantages as well, mostly derived from the above: the
> patch series to implement this can probably be something like 5 patches,
> so we don't actually need to touch every driver, because the drivers
> themselves are not the issue.

While I don't think that the number of patches to reach a goal is a good
objective to judge the result of a patch set: We won't go down to 5. We
would still need to adapt every driver as they all assign struct
pwm_chip::dev.

> It's how the subsystem will expose them
> via chardev (or already exposes them via sysfs) that's really the
> problem. The only place where it makes sense to fix this is in the
> subsystem. Drivers don't need to be concerned about this.

This is another critic I don't understand. I agree it would be a
relevant issue if it applied. But the chardev stuff is completely in the
core.

I invested much thought, time and effort into this series. I'm convinced
it is a good one improving the pwm framework. I aligned the
implementation ideas to what several other frameworks do---I'm aware of
counter, iio, net, rtc, siox and spi that all use this idiom. I grepped
a bit around and found some more using the _alloc pattern: amba, drm,
hid, infiniband, input, libata. I also found some that don't: 

  - rpmsg: but that seems to rely on the lowlevel drivers to get the
    lifetime stuff right (look at mtk_rpmsg.c). 
  - i2c/i3c: has lifetime issues (though I think they are all "properly"
    worked around)
  - gpio: See how both gpio_chip and gpio_device have base, ngpio, a
    parent pointer (gpio_device has it in .dev), an owner and a label.
    Do they all have the same semantic? Yes? -> that's bad. No? ->
    that's IMHO even worse.

And now I'm supposed to rework my patch set to a different abstraction
because of some vapour ware resource lib that probably involves some
data duplication (see gpio above) and more overhead in the source *and*
the binaries because we need more pointer dereferences?

Honestly? That's really frustrating.

Can I please invest some of the karma I earned by caring for the pwm
subsystem to align it to how other major subsystems work?

Best regards
Uwe

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

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

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Fri, Nov 24, 2023 at 7:22 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>

[snip]

>   - gpio: See how both gpio_chip and gpio_device have base, ngpio, a
>     parent pointer (gpio_device has it in .dev), an owner and a label.
>     Do they all have the same semantic? Yes? -> that's bad. No? ->
>     that's IMHO even worse.

For the record: I am very well aware there are a lot of things wrong
with GPIO at the moment. It's years of technical debt biting back.
I've racked up ~100 patches last release cycle alone fixing various
cases of abuse of GPIOLIB in the wildest places (OMAP1?!). I'm slowly
doing cleanups all around the place but since GPIO is so ubiquitous
across the kernel tree, it's going quite slow. I'm going to get to the
duplication between gpio_chip and gpio_device but we still have users
in the kernel who shamelessly access gpio_chip directly without being
GPIO providers.

This is not an argument against the general approach I presented -
it's an argument against stacking up years' worth of cruft.

Bart

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Lee,

On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote:
> On Tue, 21 Nov 2023, Uwe Kleine-König wrote:
> 
> > This prepares the pwm sub-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.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/leds/rgb/leds-qcom-lpg.c | 30 +++++++++++++++++++++---------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > index 68d82a682bf6..283227e02df6 100644
> > --- a/drivers/leds/rgb/leds-qcom-lpg.c
> > +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > @@ -77,7 +77,7 @@ struct lpg {
> >  
> >  	struct mutex lock;
> >  
> > -	struct pwm_chip pwm;
> > +	struct pwm_chip *pwm;
> >  
> >  	const struct lpg_data *data;
> >  
> > @@ -977,9 +977,15 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
> >  	return lpg_pattern_clear(led);
> >  }
> >  
> > +static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
> > +{
> > +	struct lpg **lpg = pwmchip_priv(chip);
> > +	return *lpg;
> > +}
> 
> I don't have easy-vis into the other patches, but if this is a common
> pattern, perhaps add a generic helper in <linux/pwm.h>?
> 
> >  static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> > -	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg *lpg = lpg_pwm_from_chip(chip);
> >  	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> >  
> >  	return chan->in_use ? -EBUSY : 0;
> > [...]
> > @@ -1089,13 +1095,19 @@ static const struct pwm_ops lpg_pwm_ops = {
> >  
> >  static int lpg_add_pwm(struct lpg *lpg)
> >  {
> > +	struct pwm_chip *chip;
> >  	int ret;
> >  
> > -	lpg->pwm.dev = lpg->dev;
> > -	lpg->pwm.npwm = lpg->num_channels;
> > -	lpg->pwm.ops = &lpg_pwm_ops;
> > +	lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels,
> > +					     sizeof(&lpg));
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> >  
> > -	ret = pwmchip_add(&lpg->pwm);
> > +	*(struct lpg **)pwmchip_priv(chip) = lpg;
> 
> This is vile!

This is indeed one of the uglier conversions. It gets a bit prettier
with the following addon patch:

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 283227e02df6..e09eba823057 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -206,6 +206,10 @@ struct lpg_data {
 	const struct lpg_channel_data *channels;
 };
 
+struct lpg_pwm_data {
+	struct lpg *lpg;
+};
+
 static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
 {
 	/* Skip if we don't have a triled block */
@@ -979,8 +983,9 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
 
 static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
 {
-	struct lpg **lpg = pwmchip_priv(chip);
-	return *lpg;
+	struct lpg_pwm_data *lpg_pwm_data = pwmchip_priv(chip);
+
+	return lpg_pwm_data->lpg;
 }
 
 static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -1096,14 +1101,16 @@ static const struct pwm_ops lpg_pwm_ops = {
 static int lpg_add_pwm(struct lpg *lpg)
 {
 	struct pwm_chip *chip;
+	struct lpg_pwm_data *lpg_pwm_data;
 	int ret;
 
 	lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels,
-					     sizeof(&lpg));
+					     sizeof(*lpg_pwm_data));
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
-	*(struct lpg **)pwmchip_priv(chip) = lpg;
+	lpg_pwm_data = pwmchip_priv(chip);
+	lpg_pwm_data->lpg = lpg;
 
 	chip->ops = &lpg_pwm_ops;
 
Would you like it better then?

Best regards
Uwe

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

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

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Lee Jones]

On Wed, 22 Nov 2023, Uwe Kleine-König wrote:

> Hello Lee,
> 
> On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote:
> > On Tue, 21 Nov 2023, Uwe Kleine-König wrote:
> > 
> > > This prepares the pwm sub-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.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/leds/rgb/leds-qcom-lpg.c | 30 +++++++++++++++++++++---------
> > >  1 file changed, 21 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > > index 68d82a682bf6..283227e02df6 100644
> > > --- a/drivers/leds/rgb/leds-qcom-lpg.c
> > > +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > > @@ -77,7 +77,7 @@ struct lpg {
> > >  
> > >  	struct mutex lock;
> > >  
> > > -	struct pwm_chip pwm;
> > > +	struct pwm_chip *pwm;
> > >  
> > >  	const struct lpg_data *data;
> > >  
> > > @@ -977,9 +977,15 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
> > >  	return lpg_pattern_clear(led);
> > >  }
> > >  
> > > +static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
> > > +{
> > > +	struct lpg **lpg = pwmchip_priv(chip);
> > > +	return *lpg;
> > > +}
> > 
> > I don't have easy-vis into the other patches, but if this is a common
> > pattern, perhaps add a generic helper in <linux/pwm.h>?
> > 
> > >  static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  {
> > > -	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > > +	struct lpg *lpg = lpg_pwm_from_chip(chip);
> > >  	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > >  
> > >  	return chan->in_use ? -EBUSY : 0;
> > > [...]
> > > @@ -1089,13 +1095,19 @@ static const struct pwm_ops lpg_pwm_ops = {
> > >  
> > >  static int lpg_add_pwm(struct lpg *lpg)
> > >  {
> > > +	struct pwm_chip *chip;
> > >  	int ret;
> > >  
> > > -	lpg->pwm.dev = lpg->dev;
> > > -	lpg->pwm.npwm = lpg->num_channels;
> > > -	lpg->pwm.ops = &lpg_pwm_ops;
> > > +	lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels,
> > > +					     sizeof(&lpg));
> > > +	if (IS_ERR(chip))
> > > +		return PTR_ERR(chip);
> > >  
> > > -	ret = pwmchip_add(&lpg->pwm);
> > > +	*(struct lpg **)pwmchip_priv(chip) = lpg;
> > 
> > This is vile!
> 
> This is indeed one of the uglier conversions. It gets a bit prettier
> with the following addon patch:
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 283227e02df6..e09eba823057 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -206,6 +206,10 @@ struct lpg_data {
>  	const struct lpg_channel_data *channels;
>  };
>  
> +struct lpg_pwm_data {
> +	struct lpg *lpg;
> +};
> +
>  static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
>  {
>  	/* Skip if we don't have a triled block */
> @@ -979,8 +983,9 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
>  
>  static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
>  {
> -	struct lpg **lpg = pwmchip_priv(chip);
> -	return *lpg;
> +	struct lpg_pwm_data *lpg_pwm_data = pwmchip_priv(chip);
> +
> +	return lpg_pwm_data->lpg;
>  }
>  
>  static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -1096,14 +1101,16 @@ static const struct pwm_ops lpg_pwm_ops = {
>  static int lpg_add_pwm(struct lpg *lpg)
>  {
>  	struct pwm_chip *chip;
> +	struct lpg_pwm_data *lpg_pwm_data;
>  	int ret;
>  
>  	lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels,
> -					     sizeof(&lpg));
> +					     sizeof(*lpg_pwm_data));
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  
> -	*(struct lpg **)pwmchip_priv(chip) = lpg;
> +	lpg_pwm_data = pwmchip_priv(chip);
> +	lpg_pwm_data->lpg = lpg;
>  
>  	chip->ops = &lpg_pwm_ops;
>  
> Would you like it better then?

It's definitely nicer to read and more in-line with the style I expect,
but the additional wrapper/abstraction layer is still bothersome.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Lee,

On Thu, Nov 23, 2023 at 10:21:11AM +0000, Lee Jones wrote:
> On Wed, 22 Nov 2023, Uwe Kleine-König wrote:
> > Would you like it better then?
> 
> It's definitely nicer to read and more in-line with the style I expect,
> but the additional wrapper/abstraction layer is still bothersome.

I guess that's subjective because I think having a separate pwm private
data struct is nice. I don't see an immediate advantage for the
leds-qcom-lpg driver, but the ti-sn65dsi86 driver could move some
members of the parent driver to the pwm specific struct.

Best regards
Uwe

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

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