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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
