| Message ID | 500a4f39531bd5fc61eb8ac0092e34d4b1ae2cc0.1701860672.git.u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | pwm: Fix lifetime issues for pwm_chips | expand |
Uwe Kleine-König wrote: > This prepares the pwm-sifive driver 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/pwm/pwm-sifive.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > index c1b412905d9e..cf3fcffe7b3a 100644 > --- a/drivers/pwm/pwm-sifive.c > +++ b/drivers/pwm/pwm-sifive.c > @@ -41,7 +41,7 @@ > #define PWM_SIFIVE_DEFAULT_PERIOD 10000000 > > struct pwm_sifive_ddata { > - struct pwm_chip chip; > + struct pwm_chip *chip; In patch 43 you do ddata = (void *)chip + sizeof(*chip). Shouldn't we be able to get the chip pointer from ddata using chip = (void *)ddata - sizeof(*chip)? > struct mutex lock; /* lock to protect user_count and approx_period */ > struct notifier_block notifier; > struct clk *clk; > @@ -54,7 +54,7 @@ struct pwm_sifive_ddata { > static inline > struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *chip) > { > - return container_of(chip, struct pwm_sifive_ddata, chip); > + return pwmchip_get_drvdata(chip); > } > > static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *pwm) > @@ -102,7 +102,7 @@ static void pwm_sifive_update_clock(struct pwm_sifive_ddata *ddata, > /* As scale <= 15 the shift operation cannot overflow. */ > num = (unsigned long long)NSEC_PER_SEC << (PWM_SIFIVE_CMPWIDTH + scale); > ddata->real_period = div64_ul(num, rate); > - dev_dbg(pwmchip_parent(&ddata->chip), > + dev_dbg(pwmchip_parent(ddata->chip), > "New real_period = %u ns\n", ddata->real_period); > } > > @@ -230,15 +230,14 @@ static int pwm_sifive_probe(struct platform_device *pdev) > u32 val; > unsigned int enabled_pwms = 0, enabled_clks = 1; > > - ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); > - if (!ddata) > - return -ENOMEM; > + chip = devm_pwmchip_alloc(dev, 4, sizeof(*ddata)); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + ddata = pwm_sifive_chip_to_ddata(chip); > + ddata->chip = chip; > > mutex_init(&ddata->lock); > - chip = &ddata->chip; > - chip->dev = dev; > chip->ops = &pwm_sifive_ops; > - chip->npwm = 4; > > ddata->regs = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(ddata->regs)) > @@ -296,7 +295,7 @@ static int pwm_sifive_probe(struct platform_device *pdev) > goto unregister_clk; > } > > - platform_set_drvdata(pdev, ddata); > + platform_set_drvdata(pdev, chip); > dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm); > > return 0; > @@ -314,15 +313,16 @@ static int pwm_sifive_probe(struct platform_device *pdev) > > static void pwm_sifive_remove(struct platform_device *dev) > { > - struct pwm_sifive_ddata *ddata = platform_get_drvdata(dev); > + struct pwm_chip *chip = platform_get_drvdata(dev); > + struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip); > struct pwm_device *pwm; > int ch; > > - pwmchip_remove(&ddata->chip); > + pwmchip_remove(chip); > clk_notifier_unregister(ddata->clk, &ddata->notifier); > > - for (ch = 0; ch < ddata->chip.npwm; ch++) { > - pwm = &ddata->chip.pwms[ch]; > + for (ch = 0; ch < chip->npwm; ch++) { > + pwm = &chip->pwms[ch]; > if (pwm->state.enabled) > clk_disable(ddata->clk); > } > -- > 2.42.0 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hello Emil, On Fri, Dec 08, 2023 at 04:30:41AM -0500, Emil Renner Berthing wrote: > Uwe Kleine-König wrote: > > This prepares the pwm-sifive driver 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/pwm/pwm-sifive.c | 28 ++++++++++++++-------------- > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > index c1b412905d9e..cf3fcffe7b3a 100644 > > --- a/drivers/pwm/pwm-sifive.c > > +++ b/drivers/pwm/pwm-sifive.c > > @@ -41,7 +41,7 @@ > > #define PWM_SIFIVE_DEFAULT_PERIOD 10000000 > > > > struct pwm_sifive_ddata { > > - struct pwm_chip chip; > > + struct pwm_chip *chip; > > In patch 43 you do ddata = (void *)chip + sizeof(*chip). Shouldn't we > be able to get > the chip pointer from ddata using chip = (void *)ddata - sizeof(*chip)? That would work, but I don't want to use that implementation detail in lowlevel drivers. Also it's a bit obscure because not all drivers use the driver data located after the pwmchip. Another difficulty is that starting with patch #111 the memory layout changes and you can only determine the chip from the driver data if you know the value of npwm. (So you need information from the chip to get access to the chip. huch) > > struct mutex lock; /* lock to protect user_count and approx_period */ > > struct notifier_block notifier; > > struct clk *clk; Best regards Uwe
Uwe Kleine-König wrote: > Hello Emil, > > On Fri, Dec 08, 2023 at 04:30:41AM -0500, Emil Renner Berthing wrote: > > Uwe Kleine-König wrote: > > > This prepares the pwm-sifive driver 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/pwm/pwm-sifive.c | 28 ++++++++++++++-------------- > > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > > > index c1b412905d9e..cf3fcffe7b3a 100644 > > > --- a/drivers/pwm/pwm-sifive.c > > > +++ b/drivers/pwm/pwm-sifive.c > > > @@ -41,7 +41,7 @@ > > > #define PWM_SIFIVE_DEFAULT_PERIOD 10000000 > > > > > > struct pwm_sifive_ddata { > > > - struct pwm_chip chip; > > > + struct pwm_chip *chip; > > > > In patch 43 you do ddata = (void *)chip + sizeof(*chip). Shouldn't we > > be able to get > > the chip pointer from ddata using chip = (void *)ddata - sizeof(*chip)? > > That would work, but I don't want to use that implementation detail in > lowlevel drivers. Also it's a bit obscure because not all drivers use > the driver data located after the pwmchip. Another difficulty is that > starting with patch #111 the memory layout changes and you can only > determine the chip from the driver data if you know the value of npwm. > (So you need information from the chip to get access to the chip. huch) Ah, yeah it would of course need wrappers so drivers won't need to do the calculation, but the last past definitely makes sense. > > > > struct mutex lock; /* lock to protect user_count and approx_period */ > > > struct notifier_block notifier; > > > struct clk *clk; > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c index c1b412905d9e..cf3fcffe7b3a 100644 --- a/drivers/pwm/pwm-sifive.c +++ b/drivers/pwm/pwm-sifive.c @@ -41,7 +41,7 @@ #define PWM_SIFIVE_DEFAULT_PERIOD 10000000 struct pwm_sifive_ddata { - struct pwm_chip chip; + struct pwm_chip *chip; struct mutex lock; /* lock to protect user_count and approx_period */ struct notifier_block notifier; struct clk *clk; @@ -54,7 +54,7 @@ struct pwm_sifive_ddata { static inline struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *chip) { - return container_of(chip, struct pwm_sifive_ddata, chip); + return pwmchip_get_drvdata(chip); } static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *pwm) @@ -102,7 +102,7 @@ static void pwm_sifive_update_clock(struct pwm_sifive_ddata *ddata, /* As scale <= 15 the shift operation cannot overflow. */ num = (unsigned long long)NSEC_PER_SEC << (PWM_SIFIVE_CMPWIDTH + scale); ddata->real_period = div64_ul(num, rate); - dev_dbg(pwmchip_parent(&ddata->chip), + dev_dbg(pwmchip_parent(ddata->chip), "New real_period = %u ns\n", ddata->real_period); } @@ -230,15 +230,14 @@ static int pwm_sifive_probe(struct platform_device *pdev) u32 val; unsigned int enabled_pwms = 0, enabled_clks = 1; - ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); - if (!ddata) - return -ENOMEM; + chip = devm_pwmchip_alloc(dev, 4, sizeof(*ddata)); + if (IS_ERR(chip)) + return PTR_ERR(chip); + ddata = pwm_sifive_chip_to_ddata(chip); + ddata->chip = chip; mutex_init(&ddata->lock); - chip = &ddata->chip; - chip->dev = dev; chip->ops = &pwm_sifive_ops; - chip->npwm = 4; ddata->regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(ddata->regs)) @@ -296,7 +295,7 @@ static int pwm_sifive_probe(struct platform_device *pdev) goto unregister_clk; } - platform_set_drvdata(pdev, ddata); + platform_set_drvdata(pdev, chip); dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm); return 0; @@ -314,15 +313,16 @@ static int pwm_sifive_probe(struct platform_device *pdev) static void pwm_sifive_remove(struct platform_device *dev) { - struct pwm_sifive_ddata *ddata = platform_get_drvdata(dev); + struct pwm_chip *chip = platform_get_drvdata(dev); + struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip); struct pwm_device *pwm; int ch; - pwmchip_remove(&ddata->chip); + pwmchip_remove(chip); clk_notifier_unregister(ddata->clk, &ddata->notifier); - for (ch = 0; ch < ddata->chip.npwm; ch++) { - pwm = &ddata->chip.pwms[ch]; + for (ch = 0; ch < chip->npwm; ch++) { + pwm = &chip->pwms[ch]; if (pwm->state.enabled) clk_disable(ddata->clk); }
This prepares the pwm-sifive driver 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/pwm/pwm-sifive.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)