| Message ID | 0316aaec9dbfc0c73788bcd3ee532ae7ecadb180.1706182805.git.u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: Improve lifetime tracking for pwm_chips | expand |
Hi, On Thu, Jan 25, 2024 at 4:11 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > This prepares the pwm driver of the ti-sn65dsi86 to further changes of > the pwm core outlined in the commit introducing devm_pwmchip_alloc(). > There is no intended semantical change and the driver should behave as > before. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index f1fffbef3324..7fbc307cc025 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -197,7 +197,7 @@ struct ti_sn65dsi86 { > DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); > #endif > #if defined(CONFIG_PWM) > - struct pwm_chip pchip; > + struct pwm_chip *pchip; > bool pwm_enabled; > atomic_t pwm_pin_busy; > #endif > @@ -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? 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@chromium.org> Similar to the other ti-sn65dsi86 patch in this series, unless someone more senior in the drm-misc community contradicts me I think it's safe to assume you could land this through your tree. -Doug
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@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 <dianders@chromium.org> Thanks Uwe
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index f1fffbef3324..7fbc307cc025 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -197,7 +197,7 @@ struct ti_sn65dsi86 { DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); #endif #if defined(CONFIG_PWM) - struct pwm_chip pchip; + struct pwm_chip *pchip; bool pwm_enabled; atomic_t pwm_pin_busy; #endif @@ -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); } static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) @@ -1585,23 +1585,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 = &adev->dev; - pdata->pchip.ops = &ti_sn_pwm_ops; - pdata->pchip.npwm = 1; - pdata->pchip.of_xlate = of_pwm_single_xlate; + pdata->pchip = chip = devm_pwmchip_alloc(&adev->dev, 1, 0); + if (IS_ERR(chip)) + return PTR_ERR(chip); + + pwmchip_set_drvdata(chip, pdata); + + chip->ops = &ti_sn_pwm_ops; + chip->of_xlate = of_pwm_single_xlate; devm_pm_runtime_enable(&adev->dev); - return pwmchip_add(&pdata->pchip); + return pwmchip_add(chip); } static void ti_sn_pwm_remove(struct auxiliary_device *adev) { struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent); - pwmchip_remove(&pdata->pchip); + pwmchip_remove(pdata->pchip); if (pdata->pwm_enabled) pm_runtime_put_sync(&adev->dev);
This prepares the pwm driver of the ti-sn65dsi86 to further changes of the pwm core outlined in the commit introducing devm_pwmchip_alloc(). There is no intended semantical change and the driver should behave as before. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)