[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 at 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