[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 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm driver of the ti-sn65dsi86 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/gpu/drm/bridge/ti-sn65dsi86.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c45c07840f64..cd40530ffd71 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -197,7 +197,7 @@ struct ti_sn65dsi86 {
 	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
 #endif
 #if defined(CONFIG_PWM)
-	struct pwm_chip			pchip;
+	struct pwm_chip			*pchip;
 	bool				pwm_enabled;
 	atomic_t			pwm_pin_busy;
 #endif
@@ -1372,7 +1372,8 @@ static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
 
 static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
 {
-	return container_of(chip, struct ti_sn65dsi86, pchip);
+	struct ti_sn65dsi86 **pdata = pwmchip_priv(chip);
+	return *pdata;
 }
 
 static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -1585,22 +1586,28 @@ static const struct pwm_ops ti_sn_pwm_ops = {
 static int ti_sn_pwm_probe(struct auxiliary_device *adev,
 			   const struct auxiliary_device_id *id)
 {
+	struct pwm_chip *chip;
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 
-	pdata->pchip.dev = pdata->dev;
-	pdata->pchip.ops = &ti_sn_pwm_ops;
-	pdata->pchip.npwm = 1;
-	pdata->pchip.of_xlate = of_pwm_single_xlate;
-	pdata->pchip.of_pwm_n_cells = 1;
+	/* XXX: should this better use adev->dev instead of pdata->dev? */
+	pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(&pdata));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
 
-	return pwmchip_add(&pdata->pchip);
+	*(struct ti_sn65dsi86 **)pwmchip_priv(chip) = pdata;
+
+	chip->ops = &ti_sn_pwm_ops;
+	chip->of_xlate = of_pwm_single_xlate;
+	chip->of_pwm_n_cells = 1;
+
+	return pwmchip_add(chip);
 }
 
 static void ti_sn_pwm_remove(struct auxiliary_device *adev)
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 
-	pwmchip_remove(&pdata->pchip);
+	pwmchip_remove(pdata->pchip);
 
 	if (pdata->pwm_enabled)
 		pm_runtime_put_sync(pdata->dev);
-- 
2.42.0

Re: [PATCH v3 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Doug Anderson]

Hi,

On Tue, Nov 21, 2023 at 5:52 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> This prepares the pwm driver of the ti-sn65dsi86 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/gpu/drm/bridge/ti-sn65dsi86.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c45c07840f64..cd40530ffd71 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -197,7 +197,7 @@ struct ti_sn65dsi86 {
>         DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
>  #if defined(CONFIG_PWM)
> -       struct pwm_chip                 pchip;
> +       struct pwm_chip                 *pchip;
>         bool                            pwm_enabled;
>         atomic_t                        pwm_pin_busy;
>  #endif
> @@ -1372,7 +1372,8 @@ static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
>
>  static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
>  {
> -       return container_of(chip, struct ti_sn65dsi86, pchip);
> +       struct ti_sn65dsi86 **pdata = pwmchip_priv(chip);
> +       return *pdata;
>  }
>
>  static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -1585,22 +1586,28 @@ static const struct pwm_ops ti_sn_pwm_ops = {
>  static int ti_sn_pwm_probe(struct auxiliary_device *adev,
>                            const struct auxiliary_device_id *id)
>  {
> +       struct pwm_chip *chip;
>         struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
>
> -       pdata->pchip.dev = pdata->dev;
> -       pdata->pchip.ops = &ti_sn_pwm_ops;
> -       pdata->pchip.npwm = 1;
> -       pdata->pchip.of_xlate = of_pwm_single_xlate;
> -       pdata->pchip.of_pwm_n_cells = 1;
> +       /* XXX: should this better use adev->dev instead of pdata->dev? */
> +       pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(&pdata));

Yes, it should be "adev->dev". See recent commits like commit
7b821db95140 ("drm/bridge: ti-sn65dsi86: Associate DSI device lifetime
with auxiliary device").

I also think the size you're passing is technically wrong. The private
data you're storing is a pointer to a "struct ti_sn65dsi86". The size
of that is "sizeof(pdata)", not "sizeof(&pdata)".

Other than the above, this looks OK to me. Once the dependencies are
in I'd be happy to apply this to drm-misc. I could also "Ack" it for
landing in other trees and I _think_ that would be OK (the driver
isn't changing much and it's unlikely to cause conflicts), though
technically the Ack would be more legit from one of the drm-misc
maintainers [1].

[1] https://drm.pages.freedesktop.org/maintainer-tools/repositories.html?highlight=maxime#the-drm-misc-repository

Re: [PATCH v3 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Doug,

On Tue, Nov 21, 2023 at 07:15:51AM -0800, Doug Anderson wrote:
> > @@ -1585,22 +1586,28 @@ static const struct pwm_ops ti_sn_pwm_ops = {
> >  static int ti_sn_pwm_probe(struct auxiliary_device *adev,
> >                            const struct auxiliary_device_id *id)
> >  {
> > +       struct pwm_chip *chip;
> >         struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
> >
> > -       pdata->pchip.dev = pdata->dev;
> > -       pdata->pchip.ops = &ti_sn_pwm_ops;
> > -       pdata->pchip.npwm = 1;
> > -       pdata->pchip.of_xlate = of_pwm_single_xlate;
> > -       pdata->pchip.of_pwm_n_cells = 1;
> > +       /* XXX: should this better use adev->dev instead of pdata->dev? */
> > +       pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(&pdata));
> 
> Yes, it should be "adev->dev". See recent commits like commit
> 7b821db95140 ("drm/bridge: ti-sn65dsi86: Associate DSI device lifetime
> with auxiliary device").

I'd do that in a separate commit and not change that hidden in patch
like this one. Agree? Then I'd keep that as is and not address this in
this series. Maybe it will take another cycle until this patch goes in
anyhow ...

> I also think the size you're passing is technically wrong. The private
> data you're storing is a pointer to a "struct ti_sn65dsi86". The size
> of that is "sizeof(pdata)", not "sizeof(&pdata)".

sizeof(*pdata)?

> Other than the above, this looks OK to me. Once the dependencies are
> in I'd be happy to apply this to drm-misc. I could also "Ack" it for
> landing in other trees and I _think_ that would be OK (the driver
> isn't changing much and it's unlikely to cause conflicts), though
> technically the Ack would be more legit from one of the drm-misc
> maintainers [1].
> 
> [1] https://drm.pages.freedesktop.org/maintainer-tools/repositories.html?highlight=maxime#the-drm-misc-repository

*nod*

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 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Doug Anderson]

Hi,

On Tue, Nov 21, 2023 at 8:05 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Doug,
>
> On Tue, Nov 21, 2023 at 07:15:51AM -0800, Doug Anderson wrote:
> > > @@ -1585,22 +1586,28 @@ static const struct pwm_ops ti_sn_pwm_ops = {
> > >  static int ti_sn_pwm_probe(struct auxiliary_device *adev,
> > >                            const struct auxiliary_device_id *id)
> > >  {
> > > +       struct pwm_chip *chip;
> > >         struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
> > >
> > > -       pdata->pchip.dev = pdata->dev;
> > > -       pdata->pchip.ops = &ti_sn_pwm_ops;
> > > -       pdata->pchip.npwm = 1;
> > > -       pdata->pchip.of_xlate = of_pwm_single_xlate;
> > > -       pdata->pchip.of_pwm_n_cells = 1;
> > > +       /* XXX: should this better use adev->dev instead of pdata->dev? */
> > > +       pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(&pdata));
> >
> > Yes, it should be "adev->dev". See recent commits like commit
> > 7b821db95140 ("drm/bridge: ti-sn65dsi86: Associate DSI device lifetime
> > with auxiliary device").
>
> I'd do that in a separate commit and not change that hidden in patch
> like this one. Agree? Then I'd keep that as is and not address this in
> this series. Maybe it will take another cycle until this patch goes in
> anyhow ...

You could do it in a commit _before_ this one, but not a commit after
this one. Specifically before "${SUBJECT}" commit I think it was
benign to set pdata->pchip.dev to pdata->dev. Now you're starting to
use it for devm and the incorrect lifetime is worse, I think. Do you
agree?

NOTE: I don't actually have any hardware that uses the PWM here, so
you probably want to CC someone like Bjorn (who wrote the PWM code
here) so that he can test it and make sure it didn't break anything.


> > I also think the size you're passing is technically wrong. The private
> > data you're storing is a pointer to a "struct ti_sn65dsi86". The size
> > of that is "sizeof(pdata)", not "sizeof(&pdata)".
>
> sizeof(*pdata)?

No, that's also wrong. You're not storing a copy of the "struct
ti_sn65dsi86", you're storing a pointer to "struct ti_sn65dsi86".
That's "sizeof(pdata)".

Essentially I'm thinking of it like this. If you were storing 1 byte
of data then you'd pass 1 here. Then allocate and write you'd do:

u8 my_byte;
chip = devm_pwmchip_alloc(dev, 1, sizeof(my_byte));
*(u8 *)pwmchip_priv(chip) = my_byte;

Here you're storing a pointer instead of a byte, but the idea is the same.

void *my_ptr;
chip = devm_pwmchip_alloc(dev, 1, sizeof(my_ptr));
*(void **)pwmchip_priv(chip) = my_ptr;

-Doug

Re: [PATCH v3 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Doug, hello Bjorn,

On Tue, Nov 21, 2023 at 08:14:14AM -0800, Doug Anderson wrote:
> On Tue, Nov 21, 2023 at 8:05 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Nov 21, 2023 at 07:15:51AM -0800, Doug Anderson wrote:
> > > > @@ -1585,22 +1586,28 @@ static const struct pwm_ops ti_sn_pwm_ops = {
> > > >  static int ti_sn_pwm_probe(struct auxiliary_device *adev,
> > > >                            const struct auxiliary_device_id *id)
> > > >  {
> > > > +       struct pwm_chip *chip;
> > > >         struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
> > > >
> > > > -       pdata->pchip.dev = pdata->dev;
> > > > -       pdata->pchip.ops = &ti_sn_pwm_ops;
> > > > -       pdata->pchip.npwm = 1;
> > > > -       pdata->pchip.of_xlate = of_pwm_single_xlate;
> > > > -       pdata->pchip.of_pwm_n_cells = 1;
> > > > +       /* XXX: should this better use adev->dev instead of pdata->dev? */
> > > > +       pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(&pdata));
> > >
> > > Yes, it should be "adev->dev". See recent commits like commit
> > > 7b821db95140 ("drm/bridge: ti-sn65dsi86: Associate DSI device lifetime
> > > with auxiliary device").
> >
> > I'd do that in a separate commit and not change that hidden in patch
> > like this one. Agree? Then I'd keep that as is and not address this in
> > this series. Maybe it will take another cycle until this patch goes in
> > anyhow ...
> 
> You could do it in a commit _before_ this one, but not a commit after
> this one. Specifically before "${SUBJECT}" commit I think it was
> benign to set pdata->pchip.dev to pdata->dev. Now you're starting to
> use it for devm and the incorrect lifetime is worse, I think. Do you
> agree?

I considered suggesting:

------>8------
From 35e5050084737070686fc3e293e88e50276f0eeb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Date: Thu, 23 Nov 2023 09:55:13 +0100
Subject: [PATCH] drm/bridge: ti-sn65dsi86: Associate PWM device to auxiliary
 device

It's the ti_sn65dsi86.pwm auxiliary driver that creates the pwmchip, so
let the auxiliary device be the parent of the pwm device.
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c45c07840f64..b5d4c30c28b7 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1587,7 +1587,7 @@ static int ti_sn_pwm_probe(struct auxiliary_device *adev,
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 
-	pdata->pchip.dev = pdata->dev;
+	pdata->pchip.dev = &adev->dev;
 	pdata->pchip.ops = &ti_sn_pwm_ops;
 	pdata->pchip.npwm = 1;
 	pdata->pchip.of_xlate = of_pwm_single_xlate;

base-commit: 815d8b0425ad1164e45953ac3d56a9f6f63792cc
------>8------

But I wonder if pwm lookup (e.g. in
arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts for &backlight) still
works then?

> > > I also think the size you're passing is technically wrong. The private
> > > data you're storing is a pointer to a "struct ti_sn65dsi86". The size
> > > of that is "sizeof(pdata)", not "sizeof(&pdata)".
> >
> > sizeof(*pdata)?
> 
> No, that's also wrong. You're not storing a copy of the "struct
> ti_sn65dsi86", you're storing a pointer to "struct ti_sn65dsi86".
> That's "sizeof(pdata)".

You're right. I suggest making this simpler by adding 

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 9200c41d48b6..35cf038595c8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1370,10 +1370,14 @@ static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
 	atomic_set(&pdata->pwm_pin_busy, 0);
 }
 
+struct ti_sn_bridge_pwm_ddata {
+	struct ti_sn65dsi86 *pdata;
+};
+
 static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
 {
-	struct ti_sn65dsi86 **pdata = pwmchip_priv(chip);
-	return *pdata;
+	struct ti_sn_bridge_pwm_ddata *ddata = pwmchip_priv(chip);
+	return ddata->pdata;
 }
 
 static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -1587,14 +1591,16 @@ static int ti_sn_pwm_probe(struct auxiliary_device *adev,
 			   const struct auxiliary_device_id *id)
 {
 	struct pwm_chip *chip;
+	struct ti_sn_bridge_pwm_ddata *ddata;
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 
 	/* XXX: should this better use adev->dev instead of pdata->dev? */
-	pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(*pdata));
+	pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(*ddata));
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
-	*(struct ti_sn65dsi86 **)pwmchip_priv(chip) = pdata;
+	ddata = pwmchip_priv(chip);
+	ddata->pdata = pdata;
 
 	chip->ops = &ti_sn_pwm_ops;
 	chip->of_xlate = of_pwm_single_xlate;

to the patch.

Thanks for your feedback,
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 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Laurent Pinchart]

Hi Uwe,

(CC'ing Bartosz)

Thank you for the patch.

On Tue, Nov 21, 2023 at 02:50:43PM +0100, Uwe Kleine-König wrote:
> This prepares the pwm driver of the ti-sn65dsi86 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/gpu/drm/bridge/ti-sn65dsi86.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c45c07840f64..cd40530ffd71 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -197,7 +197,7 @@ struct ti_sn65dsi86 {
>  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
>  #if defined(CONFIG_PWM)
> -	struct pwm_chip			pchip;
> +	struct pwm_chip			*pchip;

Dynamic allocation with devm_*() isn't the right solution for lifetime
issues related to cdev. See my talk at LPC 2022
(https://www.youtube.com/watch?v=kW8LHWlJPTU, slides at
https://lpc.events/event/16/contributions/1227/attachments/1103/2115/20220914-lpc-devm_kzalloc.pdf),
and Bartosz's talk at LPC 2023
(https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf).

>  	bool				pwm_enabled;
>  	atomic_t			pwm_pin_busy;
>  #endif
> @@ -1372,7 +1372,8 @@ static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
>  
>  static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
>  {
> -	return container_of(chip, struct ti_sn65dsi86, pchip);
> +	struct ti_sn65dsi86 **pdata = pwmchip_priv(chip);
> +	return *pdata;
>  }
>  
>  static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -1585,22 +1586,28 @@ static const struct pwm_ops ti_sn_pwm_ops = {
>  static int ti_sn_pwm_probe(struct auxiliary_device *adev,
>  			   const struct auxiliary_device_id *id)
>  {
> +	struct pwm_chip *chip;
>  	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
>  
> -	pdata->pchip.dev = pdata->dev;
> -	pdata->pchip.ops = &ti_sn_pwm_ops;
> -	pdata->pchip.npwm = 1;
> -	pdata->pchip.of_xlate = of_pwm_single_xlate;
> -	pdata->pchip.of_pwm_n_cells = 1;
> +	/* XXX: should this better use adev->dev instead of pdata->dev? */
> +	pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(&pdata));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
>  
> -	return pwmchip_add(&pdata->pchip);
> +	*(struct ti_sn65dsi86 **)pwmchip_priv(chip) = pdata;
> +
> +	chip->ops = &ti_sn_pwm_ops;
> +	chip->of_xlate = of_pwm_single_xlate;
> +	chip->of_pwm_n_cells = 1;
> +
> +	return pwmchip_add(chip);
>  }
>  
>  static void ti_sn_pwm_remove(struct auxiliary_device *adev)
>  {
>  	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
>  
> -	pwmchip_remove(&pdata->pchip);
> +	pwmchip_remove(pdata->pchip);
>  
>  	if (pdata->pwm_enabled)
>  		pm_runtime_put_sync(pdata->dev);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Laurent,

On Thu, Nov 23, 2023 at 11:46:52AM +0200, Laurent Pinchart wrote:
> (CC'ing Bartosz)

I'm already in discussion with Bart :-)

> On Tue, Nov 21, 2023 at 02:50:43PM +0100, Uwe Kleine-König wrote:
> > This prepares the pwm driver of the ti-sn65dsi86 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/gpu/drm/bridge/ti-sn65dsi86.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index c45c07840f64..cd40530ffd71 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -197,7 +197,7 @@ struct ti_sn65dsi86 {
> >  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> >  #endif
> >  #if defined(CONFIG_PWM)
> > -	struct pwm_chip			pchip;
> > +	struct pwm_chip			*pchip;
> 
> Dynamic allocation with devm_*() isn't the right solution for lifetime
> issues related to cdev. See my talk at LPC 2022
> (https://www.youtube.com/watch?v=kW8LHWlJPTU, slides at
> https://lpc.events/event/16/contributions/1227/attachments/1103/2115/20220914-lpc-devm_kzalloc.pdf),
> and Bartosz's talk at LPC 2023
> (https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf).

Once the series is completely applied, the pwm_chip isn't allocated
using devm_kzalloc any more. You're only looking at an intermediate
state where I push the broken lifetime tracking from all drivers into a
single function in the core that is then fixed after all drivers are
converted to it.

If you find issues with the complete series applied, please tell me.

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 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello,

On Thu, Nov 23, 2023 at 10:17:15AM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 21, 2023 at 08:14:14AM -0800, Doug Anderson wrote:
> > On Tue, Nov 21, 2023 at 8:05 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Tue, Nov 21, 2023 at 07:15:51AM -0800, Doug Anderson wrote:
> > > > > @@ -1585,22 +1586,28 @@ static const struct pwm_ops ti_sn_pwm_ops = {
> > > > >  static int ti_sn_pwm_probe(struct auxiliary_device *adev,
> > > > >                            const struct auxiliary_device_id *id)
> > > > >  {
> > > > > +       struct pwm_chip *chip;
> > > > >         struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
> > > > >
> > > > > -       pdata->pchip.dev = pdata->dev;
> > > > > -       pdata->pchip.ops = &ti_sn_pwm_ops;
> > > > > -       pdata->pchip.npwm = 1;
> > > > > -       pdata->pchip.of_xlate = of_pwm_single_xlate;
> > > > > -       pdata->pchip.of_pwm_n_cells = 1;
> > > > > +       /* XXX: should this better use adev->dev instead of pdata->dev? */
> > > > > +       pdata->pchip = chip = devm_pwmchip_alloc(pdata->dev, 1, sizeof(&pdata));
> > > >
> > > > Yes, it should be "adev->dev". See recent commits like commit
> > > > 7b821db95140 ("drm/bridge: ti-sn65dsi86: Associate DSI device lifetime
> > > > with auxiliary device").
> > >
> > > I'd do that in a separate commit and not change that hidden in patch
> > > like this one. Agree? Then I'd keep that as is and not address this in
> > > this series. Maybe it will take another cycle until this patch goes in
> > > anyhow ...
> > 
> > You could do it in a commit _before_ this one, but not a commit after
> > this one. Specifically before "${SUBJECT}" commit I think it was
> > benign to set pdata->pchip.dev to pdata->dev. Now you're starting to
> > use it for devm and the incorrect lifetime is worse, I think. Do you
> > agree?
> 
> I considered suggesting:
> 
> ------>8------
> From 35e5050084737070686fc3e293e88e50276f0eeb Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
> Date: Thu, 23 Nov 2023 09:55:13 +0100
> Subject: [PATCH] drm/bridge: ti-sn65dsi86: Associate PWM device to auxiliary
>  device
> 
> It's the ti_sn65dsi86.pwm auxiliary driver that creates the pwmchip, so
> let the auxiliary device be the parent of the pwm device.
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c45c07840f64..b5d4c30c28b7 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1587,7 +1587,7 @@ static int ti_sn_pwm_probe(struct auxiliary_device *adev,
>  {
>  	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
>  
> -	pdata->pchip.dev = pdata->dev;
> +	pdata->pchip.dev = &adev->dev;
>  	pdata->pchip.ops = &ti_sn_pwm_ops;
>  	pdata->pchip.npwm = 1;
>  	pdata->pchip.of_xlate = of_pwm_single_xlate;
> 
> base-commit: 815d8b0425ad1164e45953ac3d56a9f6f63792cc
> ------>8------
> 
> But I wonder if pwm lookup (e.g. in
> arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts for &backlight) still
> works then?

I checked the source and I think it works fine because
ti_sn65dsi86_add_aux_device() calls
device_set_of_node_from_dev(&aux->dev, dev); and so the
auxiliary_device's of_node points to the node with the #pwm-cells
property. I'll send a proper patch.

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 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Laurent Pinchart]

On Thu, Nov 23, 2023 at 11:10:18AM +0100, Uwe Kleine-König wrote:
> Hello Laurent,
> 
> On Thu, Nov 23, 2023 at 11:46:52AM +0200, Laurent Pinchart wrote:
> > (CC'ing Bartosz)
> 
> I'm already in discussion with Bart :-)
> 
> > On Tue, Nov 21, 2023 at 02:50:43PM +0100, Uwe Kleine-König wrote:
> > > This prepares the pwm driver of the ti-sn65dsi86 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/gpu/drm/bridge/ti-sn65dsi86.c | 25 ++++++++++++++++---------
> > >  1 file changed, 16 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index c45c07840f64..cd40530ffd71 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -197,7 +197,7 @@ struct ti_sn65dsi86 {
> > >  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> > >  #endif
> > >  #if defined(CONFIG_PWM)
> > > -	struct pwm_chip			pchip;
> > > +	struct pwm_chip			*pchip;
> > 
> > Dynamic allocation with devm_*() isn't the right solution for lifetime
> > issues related to cdev. See my talk at LPC 2022
> > (https://www.youtube.com/watch?v=kW8LHWlJPTU, slides at
> > https://lpc.events/event/16/contributions/1227/attachments/1103/2115/20220914-lpc-devm_kzalloc.pdf),
> > and Bartosz's talk at LPC 2023
> > (https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf).
> 
> Once the series is completely applied, the pwm_chip isn't allocated
> using devm_kzalloc any more. You're only looking at an intermediate
> state where I push the broken lifetime tracking from all drivers into a
> single function in the core that is then fixed after all drivers are
> converted to it.

Indeed, I missed that devm_pwm_alloc() got changed later in the series
to not call devm_kzalloc(). The naming is quite unfortunate, a
devm_*_alloc() function really gives a strong hint that the
corresponding cleanup at device remove time will be a free(), not a
put() :-S That's pretty confusing for the readers.

> If you find issues with the complete series applied, please tell me.

One thing I still dislike is forcing drivers to dynamically allocate the
pwm_chip series.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

On Wed, Dec 06, 2023 at 02:06:11PM +0200, Laurent Pinchart wrote:
> On Thu, Nov 23, 2023 at 11:10:18AM +0100, Uwe Kleine-König wrote:
> > Once the series is completely applied, the pwm_chip isn't allocated
> > using devm_kzalloc any more. You're only looking at an intermediate
> > state where I push the broken lifetime tracking from all drivers into a
> > single function in the core that is then fixed after all drivers are
> > converted to it.
> 
> Indeed, I missed that devm_pwm_alloc() got changed later in the series
> to not call devm_kzalloc(). The naming is quite unfortunate, a
> devm_*_alloc() function really gives a strong hint that the
> corresponding cleanup at device remove time will be a free(), not a
> put() :-S That's pretty confusing for the readers.

Note there is a v4 in the meantime. My suggestion to rename
pwmchip_alloc() to pwmchip_get_new() could address this concern. Would
you like that? I didn't get any feedback about it when I suggested it
somewhere in the v3 thread. (I'm not sure I like it, given that
foo_alloc() is quite usual for other subsystems.)

> > If you find issues with the complete series applied, please tell me.
> 
> One thing I still dislike is forcing drivers to dynamically allocate the
> pwm_chip series.

A struct pwm_chip must be allocated dynamically as it's reference
counted by a struct device. Given that nearly all drivers allocate their
driver data dynamically, too, this isn't a big issue IMO.

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 --]