Hello Doug,
On Thu, Jan 25, 2024 at 09:48:04AM -0800, Doug Anderson wrote:
> On Thu, Jan 25, 2024 at 4:11 AM Uwe Kleine-König
> <u.kleine-koe...@pengutronix.de> wrote:
> > @@ -1374,7 +1374,7 @@ static void ti_sn_pwm_pin_release(struct ti_sn65dsi86
> > *pdata)
> >
> > static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> > {
> > - return container_of(chip, struct ti_sn65dsi86, pchip);
> > + return pwmchip_get_drvdata(chip);
> > }
>
> nit: given Linux conventions that I'm aware of, a reader of the code
> would see the name "pwm_chip_to_ti_sn_bridge" and assume it's doing a
> container_of operation. It no longer is, so the name doesn't make as
> much sense. ...and, in fact, the function itself doesn't make as much
> sense. Maybe just have all callers call pwmchip_get_drvdata()
> directly?The upside of keeping the thin wrapper is that it returns the right type, so I tend to keep it. Probably subjective, but even if it the function should be dropped, I'd suggest to do that in a separate change to keep the changes easier to review. > In any case, this seems fine to me. I haven't done lots to analyze > your full plans to fix lifetime issues, but this patch itself looks > benign and I wouldn't object to it landing. Thus I'm OK with: > > Acked-by: Douglas Anderson <diand...@chromium.org> Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
