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

[Uwe Kleine-König]

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 at 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 at 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/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231127/5b0b77ce/attachment.sig>