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