| Message ID | 20231121134901.208535-102-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: Fix lifetime issues for pwm_chips | expand |
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
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
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
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
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);
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
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
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.
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
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);
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(-)