| Message ID | 20230808171931.944154-81-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: Fix lifetime issues for pwm_chips | expand |
On Tue, Aug 08, 2023 at 07:19:10PM +0200, Uwe Kleine-König wrote: > struct pwm_chip::dev is about to change. To not have to touch this > driver in the same commit as struct pwm_chip::dev, store a pointer to > the parent device in driver data. I'm not against this change, so Acked-by: Andy Shevchenko <andy@kernel.org> bu see some comments below. I think ideally pwm_chip should be an opaque to the driver (or something near to it). OTOH it may be I understood that wrong. ... > if (state->enabled) { > if (!pwm_is_enabled(pwm)) { > - pm_runtime_get_sync(chip->dev); > + pm_runtime_get_sync(lpwm->parent); > ret = pwm_lpss_prepare_enable(lpwm, pwm, state); > if (ret) > - pm_runtime_put(chip->dev); > + pm_runtime_put(lpwm->parent); > } else { > ret = pwm_lpss_prepare_enable(lpwm, pwm, state); > } > } else if (pwm_is_enabled(pwm)) { > pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); > - pm_runtime_put(chip->dev); > + pm_runtime_put(lpwm->parent); > } I'm wondering why PM runtime calls can't be part of PWM core? We may cleanup a lot of code with it, no? ... > - pm_runtime_get_sync(chip->dev); > + pm_runtime_get_sync(lpwm->parent); > - pm_runtime_put(chip->dev); > + pm_runtime_put(lpwm->parent); Ditto. ... > struct pwm_lpss_chip { > + struct device *parent; Have you checked IIO approach with the public and private members (under private the opaque pointer is meant)? Maybe something of that can be applied to PWM code as well, dunno. > void __iomem *regs; > const struct pwm_lpss_boardinfo *info; > };
Hello Andy, On Tue, Aug 08, 2023 at 08:49:53PM +0300, Andy Shevchenko wrote: > On Tue, Aug 08, 2023 at 07:19:10PM +0200, Uwe Kleine-König wrote: > > struct pwm_chip::dev is about to change. To not have to touch this > > driver in the same commit as struct pwm_chip::dev, store a pointer to > > the parent device in driver data. > > I'm not against this change, so > Acked-by: Andy Shevchenko <andy@kernel.org> > bu see some comments below. > > I think ideally pwm_chip should be an opaque to the driver > (or something near to it). OTOH it may be I understood that > wrong. What would be the benefit of making it opaque? True, the drivers only use .ops which could be added to devm_pwmchip_alloc() as a parameter to drop the need to assign .ops, but the benefit of hiding the details isn't clear to me. > > if (state->enabled) { > > if (!pwm_is_enabled(pwm)) { > > - pm_runtime_get_sync(chip->dev); > > + pm_runtime_get_sync(lpwm->parent); > > ret = pwm_lpss_prepare_enable(lpwm, pwm, state); > > if (ret) > > - pm_runtime_put(chip->dev); > > + pm_runtime_put(lpwm->parent); > > } else { > > ret = pwm_lpss_prepare_enable(lpwm, pwm, state); > > } > > } else if (pwm_is_enabled(pwm)) { > > pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); > > - pm_runtime_put(chip->dev); > > + pm_runtime_put(lpwm->parent); > > } > > I'm wondering why PM runtime calls can't be part of PWM core? > We may cleanup a lot of code with it, no? Yes, I wondered about that one during the conversion, too. One thing at a time. (There are also a few drivers using SET_SYSTEM_SLEEP_PM_OPS / SET_RUNTIME_PM_OPS which could be converted to moderner variants.) > > - pm_runtime_get_sync(chip->dev); > > + pm_runtime_get_sync(lpwm->parent); > > > > - pm_runtime_put(chip->dev); > > + pm_runtime_put(lpwm->parent); > > Ditto. > > ... > > > struct pwm_lpss_chip { > > + struct device *parent; > > Have you checked IIO approach with the public and private members > (under private the opaque pointer is meant)? Maybe something of that > can be applied to PWM code as well, dunno. Not sure I see what you mean. I guess it's about iio_device_alloc() and how the size calculations are done there? I didn't do any measurements if aligning the priv data helps. ISTR that for net the aligning is done because some drivers do DMA to/from their priv data area. That's not the case for PWMs. So I kept that simple. I'm open to address that, but unless there is an obvious reason that doesn't involve extensive runtime testing, I'd postpone that for a later time and concentrate on getting the groundwork for my character device plans done. Thanks for your feedback Uwe
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 13087203d05a..105b06f1a6bc 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -106,15 +106,16 @@ static int pwm_lpss_wait_for_update(struct pwm_device *pwm) */ err = readl_poll_timeout(addr, val, !(val & PWM_SW_UPDATE), 40, ms); if (err) - dev_err(pwm->chip->dev, "PWM_SW_UPDATE was not cleared\n"); + dev_err(lpwm->parent, "PWM_SW_UPDATE was not cleared\n"); return err; } static inline int pwm_lpss_is_updating(struct pwm_device *pwm) { + struct pwm_lpss_chip *lpwm = to_lpwm(pwm->chip); if (pwm_lpss_read(pwm) & PWM_SW_UPDATE) { - dev_err(pwm->chip->dev, "PWM_SW_UPDATE is still set, skipping update\n"); + dev_err(lpwm->parent, "PWM_SW_UPDATE is still set, skipping update\n"); return -EBUSY; } @@ -190,16 +191,16 @@ static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (state->enabled) { if (!pwm_is_enabled(pwm)) { - pm_runtime_get_sync(chip->dev); + pm_runtime_get_sync(lpwm->parent); ret = pwm_lpss_prepare_enable(lpwm, pwm, state); if (ret) - pm_runtime_put(chip->dev); + pm_runtime_put(lpwm->parent); } else { ret = pwm_lpss_prepare_enable(lpwm, pwm, state); } } else if (pwm_is_enabled(pwm)) { pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); - pm_runtime_put(chip->dev); + pm_runtime_put(lpwm->parent); } return ret; @@ -213,7 +214,7 @@ static int pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm, unsigned long long base_unit, freq, on_time_div; u32 ctrl; - pm_runtime_get_sync(chip->dev); + pm_runtime_get_sync(lpwm->parent); base_unit_range = BIT(lpwm->info->base_unit_bits); @@ -235,7 +236,7 @@ static int pwm_lpss_get_state(struct pwm_chip *chip, struct pwm_device *pwm, state->polarity = PWM_POLARITY_NORMAL; state->enabled = !!(ctrl & PWM_ENABLE); - pm_runtime_put(chip->dev); + pm_runtime_put(lpwm->parent); return 0; } @@ -262,6 +263,7 @@ struct pwm_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, return ERR_PTR(-ENOMEM); lpwm = to_lpwm(chip); + lpwm->parent = dev; lpwm->regs = base; lpwm->info = info; diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h index b5267ab5193b..5ccc607c6811 100644 --- a/drivers/pwm/pwm-lpss.h +++ b/drivers/pwm/pwm-lpss.h @@ -18,6 +18,7 @@ #define LPSS_MAX_PWMS 4 struct pwm_lpss_chip { + struct device *parent; void __iomem *regs; const struct pwm_lpss_boardinfo *info; };
struct pwm_chip::dev is about to change. To not have to touch this driver in the same commit as struct pwm_chip::dev, store a pointer to the parent device in driver data. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-lpss.c | 16 +++++++++------- drivers/pwm/pwm-lpss.h | 1 + 2 files changed, 10 insertions(+), 7 deletions(-)