| Message ID | b567ab5dd992e361eb884fa6c2cac11be9c7dde3.1707900770.git.u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | pwm: Improve lifetime tracking for pwm_chips | expand |
On Wed, Feb 14, 2024 at 10:31:54AM +0100, Uwe Kleine-König wrote: > This prepares the pwm-lpc drivers to further changes of the pwm core lpc --> lpss pwm --> PWM > outlined in the commit introducing devm_pwmchip_alloc(). There is no > intended semantical change and the driver should behave as before. ... > -struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, > +struct pwm_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, > const struct pwm_lpss_boardinfo *info) Missing indentation amendment for the second line. ... > + struct pwm_chip *chip; > struct pwm_lpss_chip *lpwm; > unsigned long c; > int i, ret; Please, keep reversed xmas tree order in place. struct pwm_lpss_chip *lpwm; struct pwm_chip *chip; unsigned long c; int i, ret; ... With the above being addressed, Reviewed-by: Andy Shevchenko <andy@kernel.org>
Hello, On Wed, Feb 14, 2024 at 10:31:54AM +0100, Uwe Kleine-König wrote: > @@ -256,9 +257,10 @@ struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base > if (WARN_ON(info->npwm > LPSS_MAX_PWMS)) > return ERR_PTR(-ENODEV); > > - lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL); > - if (!lpwm) > + chip = devm_pwmchip_alloc(dev, info->npwm, sizeof(*lpwm)); > + if (!chip) > return ERR_PTR(-ENOMEM); while adapting the patch for Andy's feedback I noticed this being wrong, devm_pwmchip_alloc() returns an error pointer and not NULL on failure. I'll squash the following change into the commit: diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 3247b420b67a..867e2bc8c601 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -258,8 +258,8 @@ struct pwm_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, return ERR_PTR(-ENODEV); chip = devm_pwmchip_alloc(dev, info->npwm, sizeof(*lpwm)); - if (!chip) - return ERR_PTR(-ENOMEM); + if (IS_ERR(chip)) + return chip; lpwm = to_lpwm(chip); lpwm->regs = base; Best regards Uwe
Hello, On Wed, Feb 14, 2024 at 02:46:17PM +0200, Andy Shevchenko wrote: > On Wed, Feb 14, 2024 at 10:31:54AM +0100, Uwe Kleine-König wrote: > > This prepares the pwm-lpc drivers to further changes of the pwm core > > lpc --> lpss > pwm --> PWM I'd keep pwm in lower case. While I agree that the thing the pwm core is about is written PWM, I use "pwm" to describe the framework. So the directory is drivers/pwm (and not drivers/PWM), the subject prefix is "pwm", the function prefix for pwm functions is pwm_. Agreed for the other changes. The current state of the patch is available at: https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git/commit/?h=pwm-lifetime-tracking&id=5b8f3aa33b3def3c8441ce28c93062766f278b09 (i.e. I didn't add your Reviewed-by tag because I didn't capitalize pwm). Best regards Uwe
On Wed, Feb 14, 2024 at 6:01 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Wed, Feb 14, 2024 at 02:46:17PM +0200, Andy Shevchenko wrote: > (i.e. I didn't add your Reviewed-by tag because I didn't capitalize > pwm). Are you expecting me to bikeshed?! :-) Please, add it there.
On Wed, Feb 14, 2024 at 06:09:47PM +0200, Andy Shevchenko wrote: > On Wed, Feb 14, 2024 at 6:01 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, Feb 14, 2024 at 02:46:17PM +0200, Andy Shevchenko wrote: > > > (i.e. I didn't add your Reviewed-by tag because I didn't capitalize > > pwm). > > Are you expecting me to bikeshed?! :-) > Please, add it there. No, not expecting it, but taking the possibility into account :-) And I prefer being told that I'm over-cautious and should add it over being told to have made a wrong assumption and should drop it. Best regards Uwe
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index d6f29e6faab7..89bd7ce6711a 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1492,7 +1492,7 @@ static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl, .base_unit_bits = 22, .bypass = true, }; - struct pwm_lpss_chip *pwm; + struct pwm_chip *chip; if (!(community->features & PINCTRL_FEATURE_PWM)) return 0; @@ -1500,8 +1500,8 @@ static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl, if (!IS_REACHABLE(CONFIG_PWM_LPSS)) return 0; - pwm = devm_pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); - return PTR_ERR_OR_ZERO(pwm); + chip = devm_pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); + return PTR_ERR_OR_ZERO(chip); } int intel_pinctrl_probe(struct platform_device *pdev, diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c index 34acfe99b74f..25045c229520 100644 --- a/drivers/pwm/pwm-lpss-pci.c +++ b/drivers/pwm/pwm-lpss-pci.c @@ -18,7 +18,7 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev, const struct pci_device_id *id) { const struct pwm_lpss_boardinfo *info; - struct pwm_lpss_chip *lpwm; + struct pwm_chip *chip; int err; err = pcim_enable_device(pdev); @@ -30,9 +30,9 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev, return err; info = (struct pwm_lpss_boardinfo *)id->driver_data; - lpwm = devm_pwm_lpss_probe(&pdev->dev, pcim_iomap_table(pdev)[0], info); - if (IS_ERR(lpwm)) - return PTR_ERR(lpwm); + chip = devm_pwm_lpss_probe(&pdev->dev, pcim_iomap_table(pdev)[0], info); + if (IS_ERR(chip)) + return PTR_ERR(chip); pm_runtime_put(&pdev->dev); pm_runtime_allow(&pdev->dev); diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c index 5f6ee300e342..dbc9f5b17bdc 100644 --- a/drivers/pwm/pwm-lpss-platform.c +++ b/drivers/pwm/pwm-lpss-platform.c @@ -20,7 +20,7 @@ static int pwm_lpss_probe_platform(struct platform_device *pdev) { const struct pwm_lpss_boardinfo *info; - struct pwm_lpss_chip *lpwm; + struct pwm_chip *chip; void __iomem *base; info = device_get_match_data(&pdev->dev); @@ -31,9 +31,9 @@ static int pwm_lpss_probe_platform(struct platform_device *pdev) if (IS_ERR(base)) return PTR_ERR(base); - lpwm = devm_pwm_lpss_probe(&pdev->dev, base, info); - if (IS_ERR(lpwm)) - return PTR_ERR(lpwm); + chip = devm_pwm_lpss_probe(&pdev->dev, base, info); + if (IS_ERR(chip)) + return PTR_ERR(chip); /* * On Cherry Trail devices the GFX0._PS0 AML checks if the controller diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 394c768f5a5f..b79fd3405e15 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -68,7 +68,7 @@ EXPORT_SYMBOL_GPL(pwm_lpss_tng_info); static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) { - return container_of(chip, struct pwm_lpss_chip, chip); + return pwmchip_get_drvdata(chip); } static inline u32 pwm_lpss_read(const struct pwm_device *pwm) @@ -245,9 +245,10 @@ static const struct pwm_ops pwm_lpss_ops = { .get_state = pwm_lpss_get_state, }; -struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, +struct pwm_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, const struct pwm_lpss_boardinfo *info) { + struct pwm_chip *chip; struct pwm_lpss_chip *lpwm; unsigned long c; int i, ret; @@ -256,9 +257,10 @@ struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base if (WARN_ON(info->npwm > LPSS_MAX_PWMS)) return ERR_PTR(-ENODEV); - lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL); - if (!lpwm) + chip = devm_pwmchip_alloc(dev, info->npwm, sizeof(*lpwm)); + if (!chip) return ERR_PTR(-ENOMEM); + lpwm = to_lpwm(chip); lpwm->regs = base; lpwm->info = info; @@ -267,23 +269,21 @@ struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base if (!c) return ERR_PTR(-EINVAL); - lpwm->chip.dev = dev; - lpwm->chip.ops = &pwm_lpss_ops; - lpwm->chip.npwm = info->npwm; + chip->ops = &pwm_lpss_ops; - ret = devm_pwmchip_add(dev, &lpwm->chip); + ret = devm_pwmchip_add(dev, chip); if (ret) { dev_err(dev, "failed to add PWM chip: %d\n", ret); return ERR_PTR(ret); } for (i = 0; i < lpwm->info->npwm; i++) { - ctrl = pwm_lpss_read(&lpwm->chip.pwms[i]); + ctrl = pwm_lpss_read(&chip->pwms[i]); if (ctrl & PWM_ENABLE) pm_runtime_get(dev); } - return lpwm; + return chip; } EXPORT_SYMBOL_GPL(devm_pwm_lpss_probe); diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h index bf841250385f..b5267ab5193b 100644 --- a/drivers/pwm/pwm-lpss.h +++ b/drivers/pwm/pwm-lpss.h @@ -18,7 +18,6 @@ #define LPSS_MAX_PWMS 4 struct pwm_lpss_chip { - struct pwm_chip chip; void __iomem *regs; const struct pwm_lpss_boardinfo *info; }; diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h index c852fe24fe2a..752c06b47cc8 100644 --- a/include/linux/platform_data/x86/pwm-lpss.h +++ b/include/linux/platform_data/x86/pwm-lpss.h @@ -27,7 +27,7 @@ struct pwm_lpss_boardinfo { bool other_devices_aml_touches_pwm_regs; }; -struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, - const struct pwm_lpss_boardinfo *info); +struct pwm_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, + const struct pwm_lpss_boardinfo *info); #endif /* __PLATFORM_DATA_X86_PWM_LPSS_H */
This prepares the pwm-lpc drivers 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/pinctrl/intel/pinctrl-intel.c | 6 +++--- drivers/pwm/pwm-lpss-pci.c | 8 ++++---- drivers/pwm/pwm-lpss-platform.c | 8 ++++---- drivers/pwm/pwm-lpss.c | 20 ++++++++++---------- drivers/pwm/pwm-lpss.h | 1 - include/linux/platform_data/x86/pwm-lpss.h | 4 ++-- 6 files changed, 23 insertions(+), 24 deletions(-)