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