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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
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:
signature.asc
Description: PGP signature
