| Message ID | cover.1701860672.git.u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | pwm: Fix lifetime issues for pwm_chips | expand |
On Wed, Dec 06, 2023 at 12:43:14PM +0100, Uwe Kleine-König wrote: > Hello, > > This series is based on Thierry's for-next. > > It starts with some cleanups that were all sent out separately already: > > - "pwm: Reduce number of pointer dereferences in pwm_device_request()" > https://lore.kernel.org/linux-pwm/20231130072105.966848-1-u.kleine-koenig@pengutronix.de > - "pwm: crc: Use consistent variable naming for driver data" > https://lore.kernel.org/linux-pwm/20231130074133.969806-1-u.kleine-koenig@pengutronix.de > - Two leds/qcom-lpg patches > https://lore.kernel.org/linux-leds/20231126095230.683204-1-u.kleine-koenig@pengutronix.de > Lee already claimed to have taken the series already, but it's not yet in > next. > - "leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm chip" > https://lore.kernel.org/linux-leds/20231124215208.616551-3-u.kleine-koenig@pengutronix.de > > In the following patches I changed: > > - "pwm: cros-ec: Change prototype of helper to prepare further changes" + > This was simplified in response to feedback by Tzung-Bi Shih > - Make pwmchip_priv() static (and don't export it), let drivers use a new > pwmchip_get_drvdata() instead. > - For drm/ti-sn65dsi86.c and leds/qcom-lpg make use of > pwmchip_set_drvdata() which makes the conversion to > devm_pwmchip_alloc() much prettier. > - Some cleanups here and there > - Add review tags received in v3 > I kept all tags even though the pwmchip_alloc() patches changed > slightly. Most of the time that's only > s/pwmchip_priv/pwmchip_get_drvdata/ though. Still, if you object, > just tell me. (This affects Paul Cercueil on patch #68, Conor Dooley > on patch #76 and Greg for patch #109.) > > I kept the pwmchip_alloc() + pwmchip_register() approach despite Bart > not liking it. To balance that out I don't like Bart's alternative > approach. There are no technically relevant differences between the two > approaches and no benchmarks that show either of the two to be better > than the other. Conceptually the design ideas around pwmchip_alloc() + > pwmchip_register() are used in several other subsystems, so it's a > proven way to do things. [Trimming the recipients, keeping only Bart and the mailing lists.] I do think there are technically relevant differences. For one, the better we isolate the internal data structure, the easier this becomes to manage. I'm attaching a patch that I've prototyped which should basically get us to somewhere around patch 110 of this series but with something like 1/8th of the changes. It doesn't need every driver to change and (mostly) decouples driver code from the core code. Now, I know that you think this is all bad because it's not a single allocation, but I much prefer the end result because it's got the driver and internals much more cleanly separated. Going forward I think it would be easier to apply all the ref-counting on top of that because we only need to keep the PWM framework-internal data structure alive after a PWM chip has gone away. Thierry From 72ea79887d96850f9ccc832ce52b907ca276c940 Mon Sep 17 00:00:00 2001 From: Thierry Reding <thierry.reding@gmail.com> Date: Tue, 28 Nov 2023 15:42:39 +0100 Subject: [PATCH] pwm: Isolate internal data into a separate structure In order to prepare for proper reference counting of PWM chips and PWM devices, move the internal data from the public PWM chip to a private PWM chip structure. This ensures that the data that the subsystem core may need to reference later on can stick around beyond the lifetime of the driver-private data. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/core.c | 185 +++++++++++++++++++++------------- drivers/pwm/internal.h | 38 +++++++ drivers/pwm/pwm-atmel-hlcdc.c | 8 +- drivers/pwm/pwm-fsl-ftm.c | 6 +- drivers/pwm/pwm-lpc18xx-sct.c | 4 +- drivers/pwm/pwm-lpss.c | 14 +-- drivers/pwm/pwm-pca9685.c | 6 +- drivers/pwm/pwm-samsung.c | 6 +- drivers/pwm/pwm-sifive.c | 4 +- drivers/pwm/pwm-stm32-lp.c | 6 +- drivers/pwm/pwm-stm32.c | 6 +- drivers/pwm/pwm-tiecap.c | 6 +- drivers/pwm/pwm-tiehrpwm.c | 6 +- drivers/pwm/sysfs.c | 48 ++++----- include/linux/pwm.h | 26 +---- 15 files changed, 228 insertions(+), 141 deletions(-) create mode 100644 drivers/pwm/internal.h diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index f60b715abe62..54d57dec6dce 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -24,17 +24,19 @@ #define CREATE_TRACE_POINTS #include <trace/events/pwm.h> +#include "internal.h" + static DEFINE_MUTEX(pwm_lookup_lock); static LIST_HEAD(pwm_lookup_list); -/* protects access to pwmchip_idr */ +/* protects access to pwm_chips */ static DEFINE_MUTEX(pwm_lock); -static DEFINE_IDR(pwmchip_idr); +static DEFINE_IDR(pwm_chips); static struct pwm_chip *pwmchip_find_by_name(const char *name) { - struct pwm_chip *chip; + struct pwm_chip_private *priv; unsigned long id, tmp; if (!name) @@ -42,12 +44,12 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name) mutex_lock(&pwm_lock); - idr_for_each_entry_ul(&pwmchip_idr, chip, tmp, id) { - const char *chip_name = dev_name(chip->dev); + idr_for_each_entry_ul(&pwm_chips, priv, tmp, id) { + const char *chip_name = dev_name(priv->chip->dev); if (chip_name && strcmp(chip_name, name) == 0) { mutex_unlock(&pwm_lock); - return chip; + return priv->chip; } } @@ -58,23 +60,24 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name) static int pwm_device_request(struct pwm_device *pwm, const char *label) { + struct pwm_chip *chip = pwm->priv->chip; int err; if (test_bit(PWMF_REQUESTED, &pwm->flags)) return -EBUSY; - if (!try_module_get(pwm->chip->owner)) + if (!try_module_get(pwm->priv->owner)) return -ENODEV; - if (pwm->chip->ops->request) { - err = pwm->chip->ops->request(pwm->chip, pwm); + if (chip->ops->request) { + err = chip->ops->request(chip, pwm); if (err) { - module_put(pwm->chip->owner); + module_put(pwm->priv->owner); return err; } } - if (pwm->chip->ops->get_state) { + if (chip->ops->get_state) { /* * Zero-initialize state because most drivers are unaware of * .usage_power. The other members of state are supposed to be @@ -84,7 +87,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label) */ struct pwm_state state = { 0, }; - err = pwm->chip->ops->get_state(pwm->chip, pwm, &state); + err = chip->ops->get_state(chip, pwm, &state); trace_pwm_get(pwm, &state, err); if (!err) @@ -196,6 +199,64 @@ static bool pwm_ops_check(const struct pwm_chip *chip) return true; } +static struct pwm_chip_private *pwmchip_alloc(struct pwm_chip *chip, + struct module *owner) +{ + struct pwm_chip_private *priv; + struct pwm_device *pwm; + unsigned int i; + int err; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return ERR_PTR(-ENOMEM); + + priv->pwms = kcalloc(chip->npwm, sizeof(*pwm), GFP_KERNEL); + if (!priv->pwms) { + err = -ENOMEM; + goto free; + } + + priv->owner = owner; + priv->chip = chip; + + for (i = 0; i < chip->npwm; i++) { + struct pwm_device *pwm = &priv->pwms[i]; + + pwm->priv = priv; + pwm->hwpwm = i; + } + + mutex_lock(&pwm_lock); + + err = idr_alloc(&pwm_chips, priv, 0, 0, GFP_KERNEL); + if (err < 0) { + mutex_unlock(&pwm_lock); + goto free; + } + + mutex_unlock(&pwm_lock); + + priv->id = err; + + return priv; + +free: + kfree(priv->pwms); + kfree(priv); + return ERR_PTR(err); +} + +static void pwmchip_free(struct pwm_chip_private *priv) +{ + mutex_lock(&pwm_lock); + idr_remove(&pwm_chips, priv->id); + mutex_unlock(&pwm_lock); + + kfree(priv->pwms); + kfree(priv); +} + /** * __pwmchip_add() - register a new PWM chip * @chip: the PWM chip to add @@ -208,8 +269,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip) */ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) { - unsigned int i; - int ret; + struct pwm_chip_private *priv; if (!chip || !chip->dev || !chip->ops || !chip->npwm) return -EINVAL; @@ -217,36 +277,16 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) if (!pwm_ops_check(chip)) return -EINVAL; - chip->owner = owner; - - chip->pwms = kcalloc(chip->npwm, sizeof(*chip->pwms), GFP_KERNEL); - if (!chip->pwms) + priv = pwmchip_alloc(chip, owner); + if (!priv) return -ENOMEM; - mutex_lock(&pwm_lock); - - ret = idr_alloc(&pwmchip_idr, chip, 0, 0, GFP_KERNEL); - if (ret < 0) { - mutex_unlock(&pwm_lock); - kfree(chip->pwms); - return ret; - } - - chip->id = ret; - - for (i = 0; i < chip->npwm; i++) { - struct pwm_device *pwm = &chip->pwms[i]; - - pwm->chip = chip; - pwm->hwpwm = i; - } - - mutex_unlock(&pwm_lock); + chip->priv = priv; if (IS_ENABLED(CONFIG_OF)) of_pwmchip_add(chip); - pwmchip_sysfs_export(chip); + pwmchip_sysfs_export(priv); return 0; } @@ -260,18 +300,14 @@ EXPORT_SYMBOL_GPL(__pwmchip_add); */ void pwmchip_remove(struct pwm_chip *chip) { - pwmchip_sysfs_unexport(chip); - - if (IS_ENABLED(CONFIG_OF)) - of_pwmchip_remove(chip); - mutex_lock(&pwm_lock); - idr_remove(&pwmchip_idr, chip->id); + pwmchip_sysfs_unexport(chip->priv); - mutex_unlock(&pwm_lock); + if (IS_ENABLED(CONFIG_OF)) + of_pwmchip_remove(chip); - kfree(chip->pwms); + pwmchip_free(chip->priv); } EXPORT_SYMBOL_GPL(pwmchip_remove); @@ -315,7 +351,7 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, return ERR_PTR(-EINVAL); mutex_lock(&pwm_lock); - pwm = &chip->pwms[index]; + pwm = &chip->priv->pwms[index]; err = pwm_device_request(pwm, label); if (err < 0) @@ -329,9 +365,9 @@ EXPORT_SYMBOL_GPL(pwm_request_from_chip); static void pwm_apply_state_debug(struct pwm_device *pwm, const struct pwm_state *state) { - struct pwm_state *last = &pwm->last; - struct pwm_chip *chip = pwm->chip; + struct pwm_chip *chip = pwm->priv->chip; struct pwm_state s1 = { 0 }, s2 = { 0 }; + struct pwm_state *last = &pwm->last; int err; if (!IS_ENABLED(CONFIG_PWM_DEBUG)) @@ -439,7 +475,6 @@ static void pwm_apply_state_debug(struct pwm_device *pwm, */ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) { - struct pwm_chip *chip; int err; /* @@ -455,8 +490,6 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) state->duty_cycle > state->period) return -EINVAL; - chip = pwm->chip; - if (state->period == pwm->state.period && state->duty_cycle == pwm->state.duty_cycle && state->polarity == pwm->state.polarity && @@ -464,7 +497,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) state->usage_power == pwm->state.usage_power) return 0; - err = chip->ops->apply(chip, pwm, state); + err = pwm->priv->chip->ops->apply(pwm->priv->chip, pwm, state); trace_pwm_apply(pwm, state, err); if (err) return err; @@ -492,16 +525,19 @@ EXPORT_SYMBOL_GPL(pwm_apply_state); int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout) { + struct pwm_chip *chip; int err; - if (!pwm || !pwm->chip->ops) + if (!pwm) return -EINVAL; - if (!pwm->chip->ops->capture) + chip = pwm->priv->chip; + + if (!chip || !chip->ops || !chip->ops->capture) return -ENOSYS; mutex_lock(&pwm_lock); - err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout); + err = chip->ops->capture(chip, pwm, result, timeout); mutex_unlock(&pwm_lock); return err; @@ -566,16 +602,19 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config); static struct pwm_chip *fwnode_to_pwmchip(struct fwnode_handle *fwnode) { - struct pwm_chip *chip; + struct pwm_chip_private *priv; unsigned long id, tmp; mutex_lock(&pwm_lock); - idr_for_each_entry_ul(&pwmchip_idr, chip, tmp, id) + idr_for_each_entry_ul(&pwm_chips, priv, tmp, id) { + struct pwm_chip *chip = priv->chip; + if (chip->dev && device_match_fwnode(chip->dev, fwnode)) { mutex_unlock(&pwm_lock); return chip; } + } mutex_unlock(&pwm_lock); @@ -585,6 +624,7 @@ static struct pwm_chip *fwnode_to_pwmchip(struct fwnode_handle *fwnode) static struct device_link *pwm_device_link_add(struct device *dev, struct pwm_device *pwm) { + struct pwm_chip *chip = pwm->priv->chip; struct device_link *dl; if (!dev) { @@ -593,15 +633,15 @@ static struct device_link *pwm_device_link_add(struct device *dev, * impact the PM sequence ordering: the PWM supplier may get * suspended before the consumer. */ - dev_warn(pwm->chip->dev, + dev_warn(chip->dev, "No consumer device specified to create a link to\n"); return NULL; } - dl = device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER); + dl = device_link_add(dev, chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER); if (!dl) { dev_err(dev, "failed to create device link to %s\n", - dev_name(pwm->chip->dev)); + dev_name(chip->dev)); return ERR_PTR(-EINVAL); } @@ -918,12 +958,12 @@ void pwm_put(struct pwm_device *pwm) goto out; } - if (pwm->chip->ops->free) - pwm->chip->ops->free(pwm->chip, pwm); + if (pwm->priv->chip->ops->free) + pwm->priv->chip->ops->free(pwm->priv->chip, pwm); pwm->label = NULL; - module_put(pwm->chip->owner); + module_put(pwm->priv->owner); out: mutex_unlock(&pwm_lock); } @@ -997,12 +1037,12 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev, EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get); #ifdef CONFIG_DEBUG_FS -static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) +static void pwm_dbg_show(struct pwm_chip_private *priv, struct seq_file *s) { unsigned int i; - for (i = 0; i < chip->npwm; i++) { - struct pwm_device *pwm = &chip->pwms[i]; + for (i = 0; i < priv->chip->npwm; i++) { + struct pwm_device *pwm = &priv->pwms[i]; struct pwm_state state; pwm_get_state(pwm, &state); @@ -1035,7 +1075,7 @@ static void *pwm_seq_start(struct seq_file *s, loff_t *pos) mutex_lock(&pwm_lock); s->private = ""; - ret = idr_get_next_ul(&pwmchip_idr, &id); + ret = idr_get_next_ul(&pwm_chips, &id); *pos = id; return ret; } @@ -1047,7 +1087,7 @@ static void *pwm_seq_next(struct seq_file *s, void *v, loff_t *pos) s->private = "\n"; - ret = idr_get_next_ul(&pwmchip_idr, &id); + ret = idr_get_next_ul(&pwm_chips, &id); *pos = id; return ret; } @@ -1059,15 +1099,16 @@ static void pwm_seq_stop(struct seq_file *s, void *v) static int pwm_seq_show(struct seq_file *s, void *v) { - struct pwm_chip *chip = v; + struct pwm_chip_private *priv = v; + struct pwm_chip *chip = priv->chip; seq_printf(s, "%s%d: %s/%s, %d PWM device%s\n", - (char *)s->private, chip->id, + (char *)s->private, priv->id, chip->dev->bus ? chip->dev->bus->name : "no-bus", dev_name(chip->dev), chip->npwm, (chip->npwm != 1) ? "s" : ""); - pwm_dbg_show(chip, s); + pwm_dbg_show(priv, s); return 0; } diff --git a/drivers/pwm/internal.h b/drivers/pwm/internal.h new file mode 100644 index 000000000000..44fffd3b6744 --- /dev/null +++ b/drivers/pwm/internal.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef PWM_INTERNAL_H +#define PWM_INTERNAL_H + +#include <linux/list.h> + +struct pwm_chip; +struct pwm_device; + +/** + * struct pwm_chip_private - subsystem-private PWM chip data + * @chip: driver-private PWM chip data + * @owner: module providing the chip + * @pwms: array of PWM devices allocated by the framework + * @base: number of first PWM controlled by this chip + */ +struct pwm_chip_private { + struct pwm_chip *chip; + struct module *owner; + struct pwm_device *pwms; + unsigned int id; +}; + +#ifdef CONFIG_PWM_SYSFS +void pwmchip_sysfs_export(struct pwm_chip_private *priv); +void pwmchip_sysfs_unexport(struct pwm_chip_private *priv); +#else +static inline void pwmchip_sysfs_export(struct pwm_chip_private *priv) +{ +} + +static inline void pwmchip_sysfs_unexport(struct pwm_chip_private *priv) +{ +} +#endif /* CONFIG_PWM_SYSFS */ + +#endif /* PWM_INTERNAL_H */ diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c index 3f2c5031a3ba..6bbbfaa582c1 100644 --- a/drivers/pwm/pwm-atmel-hlcdc.c +++ b/drivers/pwm/pwm-atmel-hlcdc.c @@ -15,6 +15,8 @@ #include <linux/pwm.h> #include <linux/regmap.h> +#include "internal.h" + #define ATMEL_HLCDC_PWMCVAL_MASK GENMASK(15, 8) #define ATMEL_HLCDC_PWMCVAL(x) (((x) << 8) & ATMEL_HLCDC_PWMCVAL_MASK) #define ATMEL_HLCDC_PWMPOL BIT(4) @@ -185,7 +187,7 @@ static int atmel_hlcdc_pwm_suspend(struct device *dev) struct atmel_hlcdc_pwm *atmel = dev_get_drvdata(dev); /* Keep the periph clock enabled if the PWM is still running. */ - if (pwm_is_enabled(&atmel->chip.pwms[0])) + if (pwm_is_enabled(&atmel->chip.priv->pwms[0])) clk_disable_unprepare(atmel->hlcdc->periph_clk); return 0; @@ -197,7 +199,7 @@ static int atmel_hlcdc_pwm_resume(struct device *dev) struct pwm_state state; int ret; - pwm_get_state(&atmel->chip.pwms[0], &state); + pwm_get_state(&atmel->chip.priv->pwms[0], &state); /* Re-enable the periph clock it was stopped during suspend. */ if (!state.enabled) { @@ -206,7 +208,7 @@ static int atmel_hlcdc_pwm_resume(struct device *dev) return ret; } - return atmel_hlcdc_pwm_apply(&atmel->chip, &atmel->chip.pwms[0], + return atmel_hlcdc_pwm_apply(&atmel->chip, &atmel->chip.priv->pwms[0], &state); } diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c index d1b6d1aa4773..72467d620a90 100644 --- a/drivers/pwm/pwm-fsl-ftm.c +++ b/drivers/pwm/pwm-fsl-ftm.c @@ -19,6 +19,8 @@ #include <linux/slab.h> #include <linux/fsl/ftm.h> +#include "internal.h" + #define FTM_SC_CLK(c) (((c) + 1) << FTM_SC_CLK_MASK_SHIFT) enum fsl_pwm_clk { @@ -468,7 +470,7 @@ static int fsl_pwm_suspend(struct device *dev) regcache_mark_dirty(fpc->regmap); for (i = 0; i < fpc->chip.npwm; i++) { - struct pwm_device *pwm = &fpc->chip.pwms[i]; + struct pwm_device *pwm = &fpc->chip.priv->pwms[i]; if (!test_bit(PWMF_REQUESTED, &pwm->flags)) continue; @@ -491,7 +493,7 @@ static int fsl_pwm_resume(struct device *dev) int i; for (i = 0; i < fpc->chip.npwm; i++) { - struct pwm_device *pwm = &fpc->chip.pwms[i]; + struct pwm_device *pwm = &fpc->chip.priv->pwms[i]; if (!test_bit(PWMF_REQUESTED, &pwm->flags)) continue; diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c index b3d4a955aa31..80f89ad2bc8f 100644 --- a/drivers/pwm/pwm-lpc18xx-sct.c +++ b/drivers/pwm/pwm-lpc18xx-sct.c @@ -27,6 +27,8 @@ #include <linux/platform_device.h> #include <linux/pwm.h> +#include "internal.h" + /* LPC18xx SCT registers */ #define LPC18XX_PWM_CONFIG 0x000 #define LPC18XX_PWM_CONFIG_UNIFY BIT(0) @@ -224,7 +226,7 @@ static int lpc18xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, !lpc18xx_pwm->period_ns) { lpc18xx_pwm->period_ns = period_ns; for (i = 0; i < chip->npwm; i++) - pwm_set_period(&chip->pwms[i], period_ns); + pwm_set_period(&chip->priv->pwms[i], period_ns); lpc18xx_pwm_config_period(chip, period_ns); } diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index a6ea3ce7e019..085d3c17f96b 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -19,6 +19,8 @@ #include <linux/pm_runtime.h> #include <linux/time.h> +#include "internal.h" + #define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS #include "pwm-lpss.h" @@ -73,21 +75,21 @@ static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) static inline u32 pwm_lpss_read(const struct pwm_device *pwm) { - struct pwm_lpss_chip *lpwm = to_lpwm(pwm->chip); + struct pwm_lpss_chip *lpwm = to_lpwm(pwm->priv->chip); return readl(lpwm->regs + pwm->hwpwm * PWM_SIZE + PWM); } static inline void pwm_lpss_write(const struct pwm_device *pwm, u32 value) { - struct pwm_lpss_chip *lpwm = to_lpwm(pwm->chip); + struct pwm_lpss_chip *lpwm = to_lpwm(pwm->priv->chip); writel(value, lpwm->regs + pwm->hwpwm * PWM_SIZE + PWM); } static int pwm_lpss_wait_for_update(struct pwm_device *pwm) { - struct pwm_lpss_chip *lpwm = to_lpwm(pwm->chip); + struct pwm_lpss_chip *lpwm = to_lpwm(pwm->priv->chip); const void __iomem *addr = lpwm->regs + pwm->hwpwm * PWM_SIZE + PWM; const unsigned int ms = 500 * USEC_PER_MSEC; u32 val; @@ -106,7 +108,7 @@ 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(pwm->priv->chip->dev, "PWM_SW_UPDATE was not cleared\n"); return err; } @@ -114,7 +116,7 @@ static int pwm_lpss_wait_for_update(struct pwm_device *pwm) static inline int pwm_lpss_is_updating(struct pwm_device *pwm) { if (pwm_lpss_read(pwm) & PWM_SW_UPDATE) { - dev_err(pwm->chip->dev, "PWM_SW_UPDATE is still set, skipping update\n"); + dev_err(pwm->priv->chip->dev, "PWM_SW_UPDATE is still set, skipping update\n"); return -EBUSY; } @@ -278,7 +280,7 @@ struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base } for (i = 0; i < lpwm->info->npwm; i++) { - ctrl = pwm_lpss_read(&lpwm->chip.pwms[i]); + ctrl = pwm_lpss_read(&lpwm->chip.priv->pwms[i]); if (ctrl & PWM_ENABLE) pm_runtime_get(dev); } diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index e79b1de8c4d8..d82cca90dba9 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -22,6 +22,8 @@ #include <linux/pm_runtime.h> #include <linux/bitmap.h> +#include "internal.h" + /* * Because the PCA9685 has only one prescaler per chip, only the first channel * that is enabled is allowed to change the prescale register. @@ -134,7 +136,7 @@ static int pca9685_write_reg(struct pca9685 *pca, unsigned int reg, unsigned int /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty) { - struct pwm_device *pwm = &pca->chip.pwms[channel]; + struct pwm_device *pwm = &pca->chip.priv->pwms[channel]; unsigned int on, off; if (duty == 0) { @@ -173,7 +175,7 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel) { - struct pwm_device *pwm = &pca->chip.pwms[channel]; + struct pwm_device *pwm = &pca->chip.priv->pwms[channel]; unsigned int off = 0, on = 0, val = 0; if (WARN_ON(channel >= PCA9685_MAXCHAN)) { diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c index 6e77302f7368..e1baede405bc 100644 --- a/drivers/pwm/pwm-samsung.c +++ b/drivers/pwm/pwm-samsung.c @@ -26,6 +26,8 @@ /* For struct samsung_timer_variant and samsung_pwm_lock. */ #include <clocksource/samsung_pwm.h> +#include "internal.h" + #define REG_TCFG0 0x00 #define REG_TCFG1 0x04 #define REG_TCON 0x08 @@ -627,10 +629,10 @@ static int pwm_samsung_resume(struct device *dev) unsigned int i; for (i = 0; i < SAMSUNG_PWM_NUM; i++) { - struct pwm_device *pwm = &chip->pwms[i]; + struct pwm_device *pwm = &chip->priv->pwms[i]; struct samsung_pwm_channel *chan = &our_chip->channel[i]; - if (!test_bit(PWMF_REQUESTED, &pwm->flags)) + if (WARN_ON(!pwm) || !test_bit(PWMF_REQUESTED, &pwm->flags)) continue; if (our_chip->variant.output_mask & BIT(i)) diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c index 089e50bdbbf0..e1dccaf33398 100644 --- a/drivers/pwm/pwm-sifive.c +++ b/drivers/pwm/pwm-sifive.c @@ -20,6 +20,8 @@ #include <linux/slab.h> #include <linux/bitfield.h> +#include "internal.h" + /* Register offsets */ #define PWM_SIFIVE_PWMCFG 0x0 #define PWM_SIFIVE_PWMCOUNT 0x8 @@ -322,7 +324,7 @@ static void pwm_sifive_remove(struct platform_device *dev) clk_notifier_unregister(ddata->clk, &ddata->notifier); for (ch = 0; ch < ddata->chip.npwm; ch++) { - pwm = &ddata->chip.pwms[ch]; + pwm = &ddata->chip.priv->pwms[ch]; if (pwm->state.enabled) clk_disable(ddata->clk); } diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c index 439068f3eca1..cb32caf5368a 100644 --- a/drivers/pwm/pwm-stm32-lp.c +++ b/drivers/pwm/pwm-stm32-lp.c @@ -17,6 +17,8 @@ #include <linux/platform_device.h> #include <linux/pwm.h> +#include "internal.h" + struct stm32_pwm_lp { struct pwm_chip chip; struct clk *clk; @@ -223,10 +225,10 @@ static int stm32_pwm_lp_suspend(struct device *dev) struct stm32_pwm_lp *priv = dev_get_drvdata(dev); struct pwm_state state; - pwm_get_state(&priv->chip.pwms[0], &state); + pwm_get_state(&priv->chip.priv->pwms[0], &state); if (state.enabled) { dev_err(dev, "The consumer didn't stop us (%s)\n", - priv->chip.pwms[0].label); + priv->chip.priv->pwms[0].label); return -EBUSY; } diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c index 5f10cba492ec..81933df80768 100644 --- a/drivers/pwm/pwm-stm32.c +++ b/drivers/pwm/pwm-stm32.c @@ -16,6 +16,8 @@ #include <linux/platform_device.h> #include <linux/pwm.h> +#include "internal.h" + #define CCMR_CHANNEL_SHIFT 8 #define CCMR_CHANNEL_MASK 0xFF #define MAX_BREAKINPUT 2 @@ -127,7 +129,7 @@ static int stm32_pwm_raw_capture(struct stm32_pwm *priv, struct pwm_device *pwm, *raw_prd = priv->max_arr - priv->capture[0] + priv->capture[2]; /* Duty cycle capture requires at least two capture units */ - if (pwm->chip->npwm < 2) + if (priv->chip.npwm < 2) *raw_dty = 0; else if (priv->capture[0] <= priv->capture[3]) *raw_dty = priv->capture[3] - priv->capture[0]; @@ -682,7 +684,7 @@ static int stm32_pwm_suspend(struct device *dev) mask = TIM_CCER_CC1E << (i * 4); if (ccer & mask) { dev_err(dev, "PWM %u still in use by consumer %s\n", - i, priv->chip.pwms[i].label); + i, priv->chip.priv->pwms[i].label); return -EBUSY; } } diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c index d974f4414ac9..f089257051d1 100644 --- a/drivers/pwm/pwm-tiecap.c +++ b/drivers/pwm/pwm-tiecap.c @@ -14,6 +14,8 @@ #include <linux/pwm.h> #include <linux/of.h> +#include "internal.h" + /* ECAP registers and bits definitions */ #define CAP1 0x08 #define CAP2 0x0C @@ -288,7 +290,7 @@ static void ecap_pwm_restore_context(struct ecap_pwm_chip *pc) static int ecap_pwm_suspend(struct device *dev) { struct ecap_pwm_chip *pc = dev_get_drvdata(dev); - struct pwm_device *pwm = pc->chip.pwms; + struct pwm_device *pwm = pc->chip.priv->pwms; ecap_pwm_save_context(pc); @@ -302,7 +304,7 @@ static int ecap_pwm_suspend(struct device *dev) static int ecap_pwm_resume(struct device *dev) { struct ecap_pwm_chip *pc = dev_get_drvdata(dev); - struct pwm_device *pwm = pc->chip.pwms; + struct pwm_device *pwm = pc->chip.priv->pwms; /* Enable explicitly if PWM was running */ if (pwm_is_enabled(pwm)) diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c index af231fa74fa9..0d137100b040 100644 --- a/drivers/pwm/pwm-tiehrpwm.c +++ b/drivers/pwm/pwm-tiehrpwm.c @@ -14,6 +14,8 @@ #include <linux/pm_runtime.h> #include <linux/of.h> +#include "internal.h" + /* EHRPWM registers and bits definitions */ /* Time base module registers */ @@ -557,7 +559,7 @@ static int ehrpwm_pwm_suspend(struct device *dev) ehrpwm_pwm_save_context(pc); for (i = 0; i < pc->chip.npwm; i++) { - struct pwm_device *pwm = &pc->chip.pwms[i]; + struct pwm_device *pwm = &pc->chip.priv->pwms[i]; if (!pwm_is_enabled(pwm)) continue; @@ -575,7 +577,7 @@ static int ehrpwm_pwm_resume(struct device *dev) unsigned int i; for (i = 0; i < pc->chip.npwm; i++) { - struct pwm_device *pwm = &pc->chip.pwms[i]; + struct pwm_device *pwm = &pc->chip.priv->pwms[i]; if (!pwm_is_enabled(pwm)) continue; diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 4edb994fa2e1..653df20afe1c 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -14,6 +14,8 @@ #include <linux/kdev_t.h> #include <linux/pwm.h> +#include "internal.h" + struct pwm_export { struct device child; struct pwm_device *pwm; @@ -311,7 +313,7 @@ static ssize_t export_store(struct device *parent, struct device_attribute *attr, const char *buf, size_t len) { - struct pwm_chip *chip = dev_get_drvdata(parent); + struct pwm_chip_private *priv = dev_get_drvdata(parent); struct pwm_device *pwm; unsigned int hwpwm; int ret; @@ -320,10 +322,10 @@ static ssize_t export_store(struct device *parent, if (ret < 0) return ret; - if (hwpwm >= chip->npwm) + if (hwpwm >= priv->chip->npwm) return -ENODEV; - pwm = pwm_request_from_chip(chip, hwpwm, "sysfs"); + pwm = pwm_request_from_chip(priv->chip, hwpwm, "sysfs"); if (IS_ERR(pwm)) return PTR_ERR(pwm); @@ -339,7 +341,7 @@ static ssize_t unexport_store(struct device *parent, struct device_attribute *attr, const char *buf, size_t len) { - struct pwm_chip *chip = dev_get_drvdata(parent); + struct pwm_chip_private *priv = dev_get_drvdata(parent); unsigned int hwpwm; int ret; @@ -347,10 +349,10 @@ static ssize_t unexport_store(struct device *parent, if (ret < 0) return ret; - if (hwpwm >= chip->npwm) + if (hwpwm >= priv->chip->npwm) return -ENODEV; - ret = pwm_unexport_child(parent, &chip->pwms[hwpwm]); + ret = pwm_unexport_child(parent, &priv->pwms[hwpwm]); return ret ? : len; } @@ -359,9 +361,9 @@ static DEVICE_ATTR_WO(unexport); static ssize_t npwm_show(struct device *parent, struct device_attribute *attr, char *buf) { - const struct pwm_chip *chip = dev_get_drvdata(parent); + const struct pwm_chip_private *priv = dev_get_drvdata(parent); - return sysfs_emit(buf, "%u\n", chip->npwm); + return sysfs_emit(buf, "%u\n", priv->chip->npwm); } static DEVICE_ATTR_RO(npwm); @@ -411,12 +413,12 @@ static int pwm_class_apply_state(struct pwm_export *export, static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) { - struct pwm_chip *chip = dev_get_drvdata(parent); + struct pwm_chip_private *priv = dev_get_drvdata(parent); unsigned int i; int ret = 0; for (i = 0; i < npwm; i++) { - struct pwm_device *pwm = &chip->pwms[i]; + struct pwm_device *pwm = &priv->pwms[i]; struct pwm_state state; struct pwm_export *export; @@ -442,12 +444,12 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) static int pwm_class_suspend(struct device *parent) { - struct pwm_chip *chip = dev_get_drvdata(parent); + struct pwm_chip_private *priv = dev_get_drvdata(parent); unsigned int i; int ret = 0; - for (i = 0; i < chip->npwm; i++) { - struct pwm_device *pwm = &chip->pwms[i]; + for (i = 0; i < priv->chip->npwm; i++) { + struct pwm_device *pwm = &priv->pwms[i]; struct pwm_state state; struct pwm_export *export; @@ -483,9 +485,9 @@ static int pwm_class_suspend(struct device *parent) static int pwm_class_resume(struct device *parent) { - struct pwm_chip *chip = dev_get_drvdata(parent); + struct pwm_chip_private *priv = dev_get_drvdata(parent); - return pwm_class_resume_npwm(parent, chip->npwm); + return pwm_class_resume_npwm(parent, priv->chip->npwm); } static DEFINE_SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume); @@ -501,7 +503,7 @@ static int pwmchip_sysfs_match(struct device *parent, const void *data) return dev_get_drvdata(parent) == data; } -void pwmchip_sysfs_export(struct pwm_chip *chip) +void pwmchip_sysfs_export(struct pwm_chip_private *priv) { struct device *parent; @@ -509,26 +511,26 @@ void pwmchip_sysfs_export(struct pwm_chip *chip) * If device_create() fails the pwm_chip is still usable by * the kernel it's just not exported. */ - parent = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip, - "pwmchip%d", chip->id); + parent = device_create(&pwm_class, priv->chip->dev, MKDEV(0, 0), priv, + "pwmchip%d", priv->id); if (IS_ERR(parent)) { - dev_warn(chip->dev, + dev_warn(priv->chip->dev, "device_create failed for pwm_chip sysfs export\n"); } } -void pwmchip_sysfs_unexport(struct pwm_chip *chip) +void pwmchip_sysfs_unexport(struct pwm_chip_private *priv) { struct device *parent; unsigned int i; - parent = class_find_device(&pwm_class, NULL, chip, + parent = class_find_device(&pwm_class, NULL, priv, pwmchip_sysfs_match); if (!parent) return; - for (i = 0; i < chip->npwm; i++) { - struct pwm_device *pwm = &chip->pwms[i]; + for (i = 0; i < priv->chip->npwm; i++) { + struct pwm_device *pwm = &priv->pwms[i]; if (test_bit(PWMF_EXPORTED, &pwm->flags)) pwm_unexport_child(parent, pwm); diff --git a/include/linux/pwm.h b/include/linux/pwm.h index f87655c06c82..5c478cd592d7 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -6,6 +6,7 @@ #include <linux/mutex.h> #include <linux/of.h> +struct pwm_chip_private; struct pwm_chip; /** @@ -79,11 +80,12 @@ struct pwm_device { const char *label; unsigned long flags; unsigned int hwpwm; - struct pwm_chip *chip; struct pwm_args args; struct pwm_state state; struct pwm_state last; + + struct pwm_chip_private *priv; }; /** @@ -280,26 +282,21 @@ struct pwm_ops { * struct pwm_chip - abstract a PWM controller * @dev: device providing the PWMs * @ops: callbacks for this PWM controller - * @owner: module providing this chip - * @id: unique number of this PWM chip * @npwm: number of PWMs controlled by this chip * @of_xlate: request a PWM device given a device tree PWM specifier * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier - * @pwms: array of PWM devices allocated by the framework + * @priv: subsystem internal chip data */ struct pwm_chip { struct device *dev; const struct pwm_ops *ops; - struct module *owner; - unsigned int id; unsigned int npwm; struct pwm_device * (*of_xlate)(struct pwm_chip *chip, const struct of_phandle_args *args); unsigned int of_pwm_n_cells; - /* only used internally by the PWM framework */ - struct pwm_device *pwms; + struct pwm_chip_private *priv; }; #if IS_ENABLED(CONFIG_PWM) @@ -564,17 +561,4 @@ static inline void pwm_remove_table(struct pwm_lookup *table, size_t num) } #endif -#ifdef CONFIG_PWM_SYSFS -void pwmchip_sysfs_export(struct pwm_chip *chip); -void pwmchip_sysfs_unexport(struct pwm_chip *chip); -#else -static inline void pwmchip_sysfs_export(struct pwm_chip *chip) -{ -} - -static inline void pwmchip_sysfs_unexport(struct pwm_chip *chip) -{ -} -#endif /* CONFIG_PWM_SYSFS */ - #endif /* __LINUX_PWM_H */
Hello Thierry, On Fri, Dec 08, 2023 at 04:41:39PM +0100, Thierry Reding wrote: > On Wed, Dec 06, 2023 at 12:43:14PM +0100, Uwe Kleine-König wrote: > > This series is based on Thierry's for-next. > > > > It starts with some cleanups that were all sent out separately already: > > > > - "pwm: Reduce number of pointer dereferences in pwm_device_request()" > > https://lore.kernel.org/linux-pwm/20231130072105.966848-1-u.kleine-koenig@pengutronix.de > > - "pwm: crc: Use consistent variable naming for driver data" > > https://lore.kernel.org/linux-pwm/20231130074133.969806-1-u.kleine-koenig@pengutronix.de > > - Two leds/qcom-lpg patches > > https://lore.kernel.org/linux-leds/20231126095230.683204-1-u.kleine-koenig@pengutronix.de > > Lee already claimed to have taken the series already, but it's not yet in > > next. > > - "leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm chip" > > https://lore.kernel.org/linux-leds/20231124215208.616551-3-u.kleine-koenig@pengutronix.de > > > > In the following patches I changed: > > > > - "pwm: cros-ec: Change prototype of helper to prepare further changes" + > > This was simplified in response to feedback by Tzung-Bi Shih > > - Make pwmchip_priv() static (and don't export it), let drivers use a new > > pwmchip_get_drvdata() instead. > > - For drm/ti-sn65dsi86.c and leds/qcom-lpg make use of > > pwmchip_set_drvdata() which makes the conversion to > > devm_pwmchip_alloc() much prettier. > > - Some cleanups here and there > > - Add review tags received in v3 > > I kept all tags even though the pwmchip_alloc() patches changed > > slightly. Most of the time that's only > > s/pwmchip_priv/pwmchip_get_drvdata/ though. Still, if you object, > > just tell me. (This affects Paul Cercueil on patch #68, Conor Dooley > > on patch #76 and Greg for patch #109.) > > > > I kept the pwmchip_alloc() + pwmchip_register() approach despite Bart > > not liking it. To balance that out I don't like Bart's alternative > > approach. There are no technically relevant differences between the two > > approaches and no benchmarks that show either of the two to be better > > than the other. Conceptually the design ideas around pwmchip_alloc() + > > pwmchip_register() are used in several other subsystems, so it's a > > proven way to do things. > > [Trimming the recipients, keeping only Bart and the mailing lists.] > > I do think there are technically relevant differences. For one, the > better we isolate the internal data structure, the easier this becomes > to manage. I'm attaching a patch that I've prototyped which should > basically get us to somewhere around patch 110 of this series but with > something like 1/8th of the changes. It doesn't need every driver to > change and (mostly) decouples driver code from the core code. You don't need to touch all drivers because you didn't change struct pwm_chip::dev yet. (If you really want, you don't need to change that, but then you have some duplication as chip->dev holds the same value as priv->dev.parent in the end.) > Now, I know that you think this is all bad because it's not a single > allocation, but I much prefer the end result because it's got the driver > and internals much more cleanly separated. Going forward I think it > would be easier to apply all the ref-counting on top of that because we > only need to keep the PWM framework-internal data structure alive after > a PWM chip has gone away. Nearly all drivers need driver private data. Isn't it a nice service by the pwm core to make handling this private data easier for the lowlevel drivers? > From 72ea79887d96850f9ccc832ce52b907ca276c940 Mon Sep 17 00:00:00 2001 > From: Thierry Reding <thierry.reding@gmail.com> > Date: Tue, 28 Nov 2023 15:42:39 +0100 > Subject: [PATCH] pwm: Isolate internal data into a separate structure > > In order to prepare for proper reference counting of PWM chips and PWM > devices, move the internal data from the public PWM chip to a private > PWM chip structure. This ensures that the data that the subsystem core > may need to reference later on can stick around beyond the lifetime of > the driver-private data. > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/core.c | 185 +++++++++++++++++++++------------- > drivers/pwm/internal.h | 38 +++++++ > drivers/pwm/pwm-atmel-hlcdc.c | 8 +- > drivers/pwm/pwm-fsl-ftm.c | 6 +- > drivers/pwm/pwm-lpc18xx-sct.c | 4 +- > drivers/pwm/pwm-lpss.c | 14 +-- > drivers/pwm/pwm-pca9685.c | 6 +- > drivers/pwm/pwm-samsung.c | 6 +- > drivers/pwm/pwm-sifive.c | 4 +- > drivers/pwm/pwm-stm32-lp.c | 6 +- > drivers/pwm/pwm-stm32.c | 6 +- > drivers/pwm/pwm-tiecap.c | 6 +- > drivers/pwm/pwm-tiehrpwm.c | 6 +- > drivers/pwm/sysfs.c | 48 ++++----- > include/linux/pwm.h | 26 +---- > 15 files changed, 228 insertions(+), 141 deletions(-) > create mode 100644 drivers/pwm/internal.h > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index f60b715abe62..54d57dec6dce 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -24,17 +24,19 @@ > #define CREATE_TRACE_POINTS > #include <trace/events/pwm.h> > > +#include "internal.h" > + > static DEFINE_MUTEX(pwm_lookup_lock); > static LIST_HEAD(pwm_lookup_list); > > -/* protects access to pwmchip_idr */ > +/* protects access to pwm_chips */ > static DEFINE_MUTEX(pwm_lock); > > -static DEFINE_IDR(pwmchip_idr); > +static DEFINE_IDR(pwm_chips); That's an unrelated change. > static struct pwm_chip *pwmchip_find_by_name(const char *name) > { > - struct pwm_chip *chip; > + struct pwm_chip_private *priv; > unsigned long id, tmp; > > if (!name) > @@ -42,12 +44,12 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name) > > mutex_lock(&pwm_lock); > > - idr_for_each_entry_ul(&pwmchip_idr, chip, tmp, id) { > - const char *chip_name = dev_name(chip->dev); > + idr_for_each_entry_ul(&pwm_chips, priv, tmp, id) { > + const char *chip_name = dev_name(priv->chip->dev); two pointer dereferences instead of one before. > if (chip_name && strcmp(chip_name, name) == 0) { > mutex_unlock(&pwm_lock); > - return chip; > + return priv->chip; one pointer reference instead of none before. > } > } > > @@ -58,23 +60,24 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name) > > static int pwm_device_request(struct pwm_device *pwm, const char *label) > { > + struct pwm_chip *chip = pwm->priv->chip; With my approach getting the chip of a struct pwm_device is only one pointer dereference away. You need two. > int err; > > if (test_bit(PWMF_REQUESTED, &pwm->flags)) > return -EBUSY; > > - if (!try_module_get(pwm->chip->owner)) > + if (!try_module_get(pwm->priv->owner)) > return -ENODEV; > > - if (pwm->chip->ops->request) { > - err = pwm->chip->ops->request(pwm->chip, pwm); > + if (chip->ops->request) { > + err = chip->ops->request(chip, pwm); You seem to have reimplemented half of https://lore.kernel.org/linux-pwm/20231130072105.966848-1-u.kleine-koenig@pengutronix.de here. > if (err) { > - module_put(pwm->chip->owner); > + module_put(pwm->priv->owner); > return err; > } > } > > - if (pwm->chip->ops->get_state) { > + if (chip->ops->get_state) { > /* > * Zero-initialize state because most drivers are unaware of > * .usage_power. The other members of state are supposed to be > @@ -84,7 +87,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label) > */ > struct pwm_state state = { 0, }; > > - err = pwm->chip->ops->get_state(pwm->chip, pwm, &state); > + err = chip->ops->get_state(chip, pwm, &state); > trace_pwm_get(pwm, &state, err); > > if (!err) > @@ -196,6 +199,64 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > return true; > } > > +static struct pwm_chip_private *pwmchip_alloc(struct pwm_chip *chip, > + struct module *owner) > +{ > + struct pwm_chip_private *priv; > + struct pwm_device *pwm; > + unsigned int i; > + int err; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return ERR_PTR(-ENOMEM); > + > + priv->pwms = kcalloc(chip->npwm, sizeof(*pwm), GFP_KERNEL); > + if (!priv->pwms) { > + err = -ENOMEM; > + goto free; > + } Oh, so you don't use one additional allocation but two. > + priv->owner = owner; > + priv->chip = chip; > + > + for (i = 0; i < chip->npwm; i++) { > + struct pwm_device *pwm = &priv->pwms[i]; > + > + pwm->priv = priv; > + pwm->hwpwm = i; > + } > + > + mutex_lock(&pwm_lock); > + > + err = idr_alloc(&pwm_chips, priv, 0, 0, GFP_KERNEL); > + if (err < 0) { > + mutex_unlock(&pwm_lock); > + goto free; > + } > + > + mutex_unlock(&pwm_lock); > + > + priv->id = err; > + > + return priv; > + > +free: > + kfree(priv->pwms); > + kfree(priv); > + return ERR_PTR(err); > +} > + > +static void pwmchip_free(struct pwm_chip_private *priv) > +{ > + mutex_lock(&pwm_lock); > + idr_remove(&pwm_chips, priv->id); > + mutex_unlock(&pwm_lock); > + > + kfree(priv->pwms); > + kfree(priv); > +} > + > /** > * __pwmchip_add() - register a new PWM chip > * @chip: the PWM chip to add > @@ -208,8 +269,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > */ > int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > { > - unsigned int i; > - int ret; > + struct pwm_chip_private *priv; > > if (!chip || !chip->dev || !chip->ops || !chip->npwm) > return -EINVAL; > @@ -217,36 +277,16 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > if (!pwm_ops_check(chip)) > return -EINVAL; > > - chip->owner = owner; > - > - chip->pwms = kcalloc(chip->npwm, sizeof(*chip->pwms), GFP_KERNEL); > - if (!chip->pwms) > + priv = pwmchip_alloc(chip, owner); > + if (!priv) > return -ENOMEM; > > - mutex_lock(&pwm_lock); > - > - ret = idr_alloc(&pwmchip_idr, chip, 0, 0, GFP_KERNEL); > - if (ret < 0) { > - mutex_unlock(&pwm_lock); > - kfree(chip->pwms); > - return ret; > - } > - > - chip->id = ret; > - > - for (i = 0; i < chip->npwm; i++) { > - struct pwm_device *pwm = &chip->pwms[i]; > - > - pwm->chip = chip; > - pwm->hwpwm = i; > - } > - > - mutex_unlock(&pwm_lock); > + chip->priv = priv; > > if (IS_ENABLED(CONFIG_OF)) > of_pwmchip_add(chip); > > - pwmchip_sysfs_export(chip); > + pwmchip_sysfs_export(priv); > > return 0; > } > @@ -260,18 +300,14 @@ EXPORT_SYMBOL_GPL(__pwmchip_add); > */ > void pwmchip_remove(struct pwm_chip *chip) > { > - pwmchip_sysfs_unexport(chip); > - > - if (IS_ENABLED(CONFIG_OF)) > - of_pwmchip_remove(chip); > - > mutex_lock(&pwm_lock); > > - idr_remove(&pwmchip_idr, chip->id); > + pwmchip_sysfs_unexport(chip->priv); > > - mutex_unlock(&pwm_lock); > + if (IS_ENABLED(CONFIG_OF)) > + of_pwmchip_remove(chip); > > - kfree(chip->pwms); > + pwmchip_free(chip->priv); > } > EXPORT_SYMBOL_GPL(pwmchip_remove); > > @@ -315,7 +351,7 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, > return ERR_PTR(-EINVAL); > > mutex_lock(&pwm_lock); > - pwm = &chip->pwms[index]; > + pwm = &chip->priv->pwms[index]; two pointer dereferences instead of one before. > err = pwm_device_request(pwm, label); > if (err < 0) > @@ -329,9 +365,9 @@ EXPORT_SYMBOL_GPL(pwm_request_from_chip); > static void pwm_apply_state_debug(struct pwm_device *pwm, > const struct pwm_state *state) > { > - struct pwm_state *last = &pwm->last; > - struct pwm_chip *chip = pwm->chip; > + struct pwm_chip *chip = pwm->priv->chip; > struct pwm_state s1 = { 0 }, s2 = { 0 }; > + struct pwm_state *last = &pwm->last; > int err; > > if (!IS_ENABLED(CONFIG_PWM_DEBUG)) > @@ -439,7 +475,6 @@ static void pwm_apply_state_debug(struct pwm_device *pwm, > */ > int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > { > - struct pwm_chip *chip; > int err; > > /* > @@ -455,8 +490,6 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > state->duty_cycle > state->period) > return -EINVAL; > > - chip = pwm->chip; > - > if (state->period == pwm->state.period && > state->duty_cycle == pwm->state.duty_cycle && > state->polarity == pwm->state.polarity && > @@ -464,7 +497,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > state->usage_power == pwm->state.usage_power) > return 0; > > - err = chip->ops->apply(chip, pwm, state); > + err = pwm->priv->chip->ops->apply(pwm->priv->chip, pwm, state); four pointer dereferences instead of two before. > trace_pwm_apply(pwm, state, err); > if (err) > return err; > @@ -492,16 +525,19 @@ EXPORT_SYMBOL_GPL(pwm_apply_state); > int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, > unsigned long timeout) > { > + struct pwm_chip *chip; > int err; > > - if (!pwm || !pwm->chip->ops) > + if (!pwm) > return -EINVAL; > > - if (!pwm->chip->ops->capture) > + chip = pwm->priv->chip; One pointer deference more than before (assuming sensible compilation). > + if (!chip || !chip->ops || !chip->ops->capture) > return -ENOSYS; Changing the return code for !chip->ops isn't intended, is it? > mutex_lock(&pwm_lock); > - err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout); > + err = chip->ops->capture(chip, pwm, result, timeout); > mutex_unlock(&pwm_lock); > > return err; > @@ -566,16 +602,19 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config); > > static struct pwm_chip *fwnode_to_pwmchip(struct fwnode_handle *fwnode) > { > - struct pwm_chip *chip; > + struct pwm_chip_private *priv; > unsigned long id, tmp; > > mutex_lock(&pwm_lock); > > - idr_for_each_entry_ul(&pwmchip_idr, chip, tmp, id) > + idr_for_each_entry_ul(&pwm_chips, priv, tmp, id) { > + struct pwm_chip *chip = priv->chip; > + One pointer dereference more per iteration than before. > if (chip->dev && device_match_fwnode(chip->dev, fwnode)) { > mutex_unlock(&pwm_lock); > return chip; > } > + } > > mutex_unlock(&pwm_lock); > > @@ -585,6 +624,7 @@ static struct pwm_chip *fwnode_to_pwmchip(struct fwnode_handle *fwnode) > static struct device_link *pwm_device_link_add(struct device *dev, > struct pwm_device *pwm) > { > + struct pwm_chip *chip = pwm->priv->chip; > struct device_link *dl; > > if (!dev) { > @@ -593,15 +633,15 @@ static struct device_link *pwm_device_link_add(struct device *dev, > * impact the PM sequence ordering: the PWM supplier may get > * suspended before the consumer. > */ > - dev_warn(pwm->chip->dev, > + dev_warn(chip->dev, One pointer deference more than before (assuming sensible compilation). > "No consumer device specified to create a link to\n"); > return NULL; > } > > - dl = device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER); > + dl = device_link_add(dev, chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER); > if (!dl) { > dev_err(dev, "failed to create device link to %s\n", > - dev_name(pwm->chip->dev)); > + dev_name(chip->dev)); > return ERR_PTR(-EINVAL); > } > > @@ -918,12 +958,12 @@ void pwm_put(struct pwm_device *pwm) > goto out; > } > > - if (pwm->chip->ops->free) > - pwm->chip->ops->free(pwm->chip, pwm); > + if (pwm->priv->chip->ops->free) > + pwm->priv->chip->ops->free(pwm->priv->chip, pwm); One pointer deference more than before (assuming sensible compilation). > pwm->label = NULL; > > - module_put(pwm->chip->owner); > + module_put(pwm->priv->owner); > out: > mutex_unlock(&pwm_lock); > } > @@ -997,12 +1037,12 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev, > EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get); > > #ifdef CONFIG_DEBUG_FS > -static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) > +static void pwm_dbg_show(struct pwm_chip_private *priv, struct seq_file *s) > { > unsigned int i; > > - for (i = 0; i < chip->npwm; i++) { > - struct pwm_device *pwm = &chip->pwms[i]; > + for (i = 0; i < priv->chip->npwm; i++) { > + struct pwm_device *pwm = &priv->pwms[i]; > struct pwm_state state; > > pwm_get_state(pwm, &state); > @@ -1035,7 +1075,7 @@ static void *pwm_seq_start(struct seq_file *s, loff_t *pos) > mutex_lock(&pwm_lock); > s->private = ""; > > - ret = idr_get_next_ul(&pwmchip_idr, &id); > + ret = idr_get_next_ul(&pwm_chips, &id); > *pos = id; > return ret; > } > @@ -1047,7 +1087,7 @@ static void *pwm_seq_next(struct seq_file *s, void *v, loff_t *pos) > > s->private = "\n"; > > - ret = idr_get_next_ul(&pwmchip_idr, &id); > + ret = idr_get_next_ul(&pwm_chips, &id); > *pos = id; > return ret; > } > @@ -1059,15 +1099,16 @@ static void pwm_seq_stop(struct seq_file *s, void *v) > > static int pwm_seq_show(struct seq_file *s, void *v) > { > - struct pwm_chip *chip = v; > + struct pwm_chip_private *priv = v; > + struct pwm_chip *chip = priv->chip; And another additional pointer dereference. > seq_printf(s, "%s%d: %s/%s, %d PWM device%s\n", > - (char *)s->private, chip->id, > + (char *)s->private, priv->id, > chip->dev->bus ? chip->dev->bus->name : "no-bus", > dev_name(chip->dev), chip->npwm, > (chip->npwm != 1) ? "s" : ""); > > - pwm_dbg_show(chip, s); > + pwm_dbg_show(priv, s); > > return 0; > } > diff --git a/drivers/pwm/internal.h b/drivers/pwm/internal.h > new file mode 100644 > index 000000000000..44fffd3b6744 > --- /dev/null > +++ b/drivers/pwm/internal.h > @@ -0,0 +1,38 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef PWM_INTERNAL_H > +#define PWM_INTERNAL_H > + > +#include <linux/list.h> This is unused? > +struct pwm_chip; > +struct pwm_device; > + > +/** > + * struct pwm_chip_private - subsystem-private PWM chip data > + * @chip: driver-private PWM chip data > + * @owner: module providing the chip > + * @pwms: array of PWM devices allocated by the framework > + * @base: number of first PWM controlled by this chip > + */ > +struct pwm_chip_private { > + struct pwm_chip *chip; > + struct module *owner; > + struct pwm_device *pwms; > + unsigned int id; > +}; With my approach the struct pwm_chip * pointer isn't needed because it's all in one structure. So you have two structures where I only have one and your two are bigger in sum than my single one (not counting memory management overhead and alignment). > +#ifdef CONFIG_PWM_SYSFS > +void pwmchip_sysfs_export(struct pwm_chip_private *priv); > +void pwmchip_sysfs_unexport(struct pwm_chip_private *priv); > +#else > +static inline void pwmchip_sysfs_export(struct pwm_chip_private *priv) > +{ > +} > + > +static inline void pwmchip_sysfs_unexport(struct pwm_chip_private *priv) > +{ > +} > +#endif /* CONFIG_PWM_SYSFS */ > + > +#endif /* PWM_INTERNAL_H */ > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c > index 3f2c5031a3ba..6bbbfaa582c1 100644 > --- a/drivers/pwm/pwm-atmel-hlcdc.c > +++ b/drivers/pwm/pwm-atmel-hlcdc.c > @@ -15,6 +15,8 @@ > #include <linux/pwm.h> > #include <linux/regmap.h> > > +#include "internal.h" > + > #define ATMEL_HLCDC_PWMCVAL_MASK GENMASK(15, 8) > #define ATMEL_HLCDC_PWMCVAL(x) (((x) << 8) & ATMEL_HLCDC_PWMCVAL_MASK) > #define ATMEL_HLCDC_PWMPOL BIT(4) > @@ -185,7 +187,7 @@ static int atmel_hlcdc_pwm_suspend(struct device *dev) > struct atmel_hlcdc_pwm *atmel = dev_get_drvdata(dev); > > /* Keep the periph clock enabled if the PWM is still running. */ > - if (pwm_is_enabled(&atmel->chip.pwms[0])) > + if (pwm_is_enabled(&atmel->chip.priv->pwms[0])) Didn't work so well to hide the internals of pwm_chip_private from the drivers? > clk_disable_unprepare(atmel->hlcdc->periph_clk); > > return 0; > [... a few more drivers where hiding the internals failed ...] > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c > index 4edb994fa2e1..653df20afe1c 100644 > --- a/drivers/pwm/sysfs.c > +++ b/drivers/pwm/sysfs.c > @@ -14,6 +14,8 @@ > #include <linux/kdev_t.h> > #include <linux/pwm.h> > > +#include "internal.h" > + > struct pwm_export { > struct device child; > struct pwm_device *pwm; > @@ -311,7 +313,7 @@ static ssize_t export_store(struct device *parent, > struct device_attribute *attr, > const char *buf, size_t len) > { > - struct pwm_chip *chip = dev_get_drvdata(parent); > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > struct pwm_device *pwm; > unsigned int hwpwm; > int ret; > @@ -320,10 +322,10 @@ static ssize_t export_store(struct device *parent, > if (ret < 0) > return ret; > > - if (hwpwm >= chip->npwm) > + if (hwpwm >= priv->chip->npwm) One more pointer dereference than before. You could trade that for some data duplication by copying npwm to struct pwmchip_priv for the case when you need .npwm but the chip is already gone. > return -ENODEV; > > - pwm = pwm_request_from_chip(chip, hwpwm, "sysfs"); > + pwm = pwm_request_from_chip(priv->chip, hwpwm, "sysfs"); > if (IS_ERR(pwm)) > return PTR_ERR(pwm); > > @@ -339,7 +341,7 @@ static ssize_t unexport_store(struct device *parent, > struct device_attribute *attr, > const char *buf, size_t len) > { > - struct pwm_chip *chip = dev_get_drvdata(parent); > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > unsigned int hwpwm; > int ret; > > @@ -347,10 +349,10 @@ static ssize_t unexport_store(struct device *parent, > if (ret < 0) > return ret; > > - if (hwpwm >= chip->npwm) > + if (hwpwm >= priv->chip->npwm) One more pointer dereference than before. > return -ENODEV; > > - ret = pwm_unexport_child(parent, &chip->pwms[hwpwm]); > + ret = pwm_unexport_child(parent, &priv->pwms[hwpwm]); > > return ret ? : len; > } > @@ -359,9 +361,9 @@ static DEVICE_ATTR_WO(unexport); > static ssize_t npwm_show(struct device *parent, struct device_attribute *attr, > char *buf) > { > - const struct pwm_chip *chip = dev_get_drvdata(parent); > + const struct pwm_chip_private *priv = dev_get_drvdata(parent); > > - return sysfs_emit(buf, "%u\n", chip->npwm); > + return sysfs_emit(buf, "%u\n", priv->chip->npwm); > } > static DEVICE_ATTR_RO(npwm); > > @@ -411,12 +413,12 @@ static int pwm_class_apply_state(struct pwm_export *export, > > static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) > { > - struct pwm_chip *chip = dev_get_drvdata(parent); > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > unsigned int i; > int ret = 0; > > for (i = 0; i < npwm; i++) { > - struct pwm_device *pwm = &chip->pwms[i]; > + struct pwm_device *pwm = &priv->pwms[i]; > struct pwm_state state; > struct pwm_export *export; > > @@ -442,12 +444,12 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) > > static int pwm_class_suspend(struct device *parent) > { > - struct pwm_chip *chip = dev_get_drvdata(parent); > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > unsigned int i; > int ret = 0; > > - for (i = 0; i < chip->npwm; i++) { > - struct pwm_device *pwm = &chip->pwms[i]; > + for (i = 0; i < priv->chip->npwm; i++) { > + struct pwm_device *pwm = &priv->pwms[i]; > struct pwm_state state; > struct pwm_export *export; > > @@ -483,9 +485,9 @@ static int pwm_class_suspend(struct device *parent) > > static int pwm_class_resume(struct device *parent) > { > - struct pwm_chip *chip = dev_get_drvdata(parent); > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > > - return pwm_class_resume_npwm(parent, chip->npwm); > + return pwm_class_resume_npwm(parent, priv->chip->npwm); One more pointer dereference than before. > } > > static DEFINE_SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume); > @@ -501,7 +503,7 @@ static int pwmchip_sysfs_match(struct device *parent, const void *data) > return dev_get_drvdata(parent) == data; > } > > -void pwmchip_sysfs_export(struct pwm_chip *chip) > +void pwmchip_sysfs_export(struct pwm_chip_private *priv) > { > struct device *parent; > > @@ -509,26 +511,26 @@ void pwmchip_sysfs_export(struct pwm_chip *chip) > * If device_create() fails the pwm_chip is still usable by > * the kernel it's just not exported. > */ > - parent = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip, > - "pwmchip%d", chip->id); > + parent = device_create(&pwm_class, priv->chip->dev, MKDEV(0, 0), priv, > + "pwmchip%d", priv->id); > if (IS_ERR(parent)) { > - dev_warn(chip->dev, > + dev_warn(priv->chip->dev, One more pointer dereference than before. > "device_create failed for pwm_chip sysfs export\n"); > } > } The TL;DR; is essentially what I already wrote in my last reply to Bart in the v3 thread[1]: - My approach needs more changes to the individual drivers (which I don't consider a relevant disadvantage given that the resulting code is better); - My approach works with less pointer dereferences which IMHO also simplifies understanding the code as all relevant data is in a single place. - My approach has a weaker separation between the core and the lowlevel drivers. That's ok in my book given that this doesn't complicate the lowlevel drivers and that hiding details considerably better doesn't work anyhow (see the drivers that need internal.h in your patch). For me the single allocation issue is only an added bonus. The relevant advantage of my approach is that the code is easier and (probably) more efficient. Best regards Uwe [1] https://lore.kernel.org/linux-pwm/20231205093145.l7p52xtw2ak756ra@pengutronix.de
On Fri, Dec 08, 2023 at 07:50:33PM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Fri, Dec 08, 2023 at 04:41:39PM +0100, Thierry Reding wrote: > > On Wed, Dec 06, 2023 at 12:43:14PM +0100, Uwe Kleine-König wrote: > > > This series is based on Thierry's for-next. > > > > > > It starts with some cleanups that were all sent out separately already: > > > > > > - "pwm: Reduce number of pointer dereferences in pwm_device_request()" > > > https://lore.kernel.org/linux-pwm/20231130072105.966848-1-u.kleine-koenig@pengutronix.de > > > - "pwm: crc: Use consistent variable naming for driver data" > > > https://lore.kernel.org/linux-pwm/20231130074133.969806-1-u.kleine-koenig@pengutronix.de > > > - Two leds/qcom-lpg patches > > > https://lore.kernel.org/linux-leds/20231126095230.683204-1-u.kleine-koenig@pengutronix.de > > > Lee already claimed to have taken the series already, but it's not yet in > > > next. > > > - "leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm chip" > > > https://lore.kernel.org/linux-leds/20231124215208.616551-3-u.kleine-koenig@pengutronix.de > > > > > > In the following patches I changed: > > > > > > - "pwm: cros-ec: Change prototype of helper to prepare further changes" + > > > This was simplified in response to feedback by Tzung-Bi Shih > > > - Make pwmchip_priv() static (and don't export it), let drivers use a new > > > pwmchip_get_drvdata() instead. > > > - For drm/ti-sn65dsi86.c and leds/qcom-lpg make use of > > > pwmchip_set_drvdata() which makes the conversion to > > > devm_pwmchip_alloc() much prettier. > > > - Some cleanups here and there > > > - Add review tags received in v3 > > > I kept all tags even though the pwmchip_alloc() patches changed > > > slightly. Most of the time that's only > > > s/pwmchip_priv/pwmchip_get_drvdata/ though. Still, if you object, > > > just tell me. (This affects Paul Cercueil on patch #68, Conor Dooley > > > on patch #76 and Greg for patch #109.) > > > > > > I kept the pwmchip_alloc() + pwmchip_register() approach despite Bart > > > not liking it. To balance that out I don't like Bart's alternative > > > approach. There are no technically relevant differences between the two > > > approaches and no benchmarks that show either of the two to be better > > > than the other. Conceptually the design ideas around pwmchip_alloc() + > > > pwmchip_register() are used in several other subsystems, so it's a > > > proven way to do things. > > > > [Trimming the recipients, keeping only Bart and the mailing lists.] > > > > I do think there are technically relevant differences. For one, the > > better we isolate the internal data structure, the easier this becomes > > to manage. I'm attaching a patch that I've prototyped which should > > basically get us to somewhere around patch 110 of this series but with > > something like 1/8th of the changes. It doesn't need every driver to > > change and (mostly) decouples driver code from the core code. > > You don't need to touch all drivers because you didn't change struct > pwm_chip::dev yet. (If you really want, you don't need to change that, > but then you have some duplication as chip->dev holds the same value as > priv->dev.parent in the end.) I don't think that's a problem. These are for two logically separate things, after all. Duplication can also sometimes be useful to simplify things. There are plently of cases where we use local variables for the same reason. > > Now, I know that you think this is all bad because it's not a single > > allocation, but I much prefer the end result because it's got the driver > > and internals much more cleanly separated. Going forward I think it > > would be easier to apply all the ref-counting on top of that because we > > only need to keep the PWM framework-internal data structure alive after > > a PWM chip has gone away. > > Nearly all drivers need driver private data. Isn't it a nice service by > the pwm core to make handling this private data easier for the lowlevel > drivers? That's only true if you think pwmchip_alloc() makes it easier. I happen to think that it doesn't. On the contrary, I think having drivers pass in a PWM chip descriptor that can be embedded in driver-private data is much easier. > > From 72ea79887d96850f9ccc832ce52b907ca276c940 Mon Sep 17 00:00:00 2001 > > From: Thierry Reding <thierry.reding@gmail.com> > > Date: Tue, 28 Nov 2023 15:42:39 +0100 > > Subject: [PATCH] pwm: Isolate internal data into a separate structure > > > > In order to prepare for proper reference counting of PWM chips and PWM > > devices, move the internal data from the public PWM chip to a private > > PWM chip structure. This ensures that the data that the subsystem core > > may need to reference later on can stick around beyond the lifetime of > > the driver-private data. > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/core.c | 185 +++++++++++++++++++++------------- > > drivers/pwm/internal.h | 38 +++++++ > > drivers/pwm/pwm-atmel-hlcdc.c | 8 +- > > drivers/pwm/pwm-fsl-ftm.c | 6 +- > > drivers/pwm/pwm-lpc18xx-sct.c | 4 +- > > drivers/pwm/pwm-lpss.c | 14 +-- > > drivers/pwm/pwm-pca9685.c | 6 +- > > drivers/pwm/pwm-samsung.c | 6 +- > > drivers/pwm/pwm-sifive.c | 4 +- > > drivers/pwm/pwm-stm32-lp.c | 6 +- > > drivers/pwm/pwm-stm32.c | 6 +- > > drivers/pwm/pwm-tiecap.c | 6 +- > > drivers/pwm/pwm-tiehrpwm.c | 6 +- > > drivers/pwm/sysfs.c | 48 ++++----- > > include/linux/pwm.h | 26 +---- > > 15 files changed, 228 insertions(+), 141 deletions(-) > > create mode 100644 drivers/pwm/internal.h > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > index f60b715abe62..54d57dec6dce 100644 > > --- a/drivers/pwm/core.c > > +++ b/drivers/pwm/core.c > > @@ -24,17 +24,19 @@ > > #define CREATE_TRACE_POINTS > > #include <trace/events/pwm.h> > > > > +#include "internal.h" > > + > > static DEFINE_MUTEX(pwm_lookup_lock); > > static LIST_HEAD(pwm_lookup_list); > > > > -/* protects access to pwmchip_idr */ > > +/* protects access to pwm_chips */ > > static DEFINE_MUTEX(pwm_lock); > > > > -static DEFINE_IDR(pwmchip_idr); > > +static DEFINE_IDR(pwm_chips); > > That's an unrelated change. Yeah, it should probably go into the patch that changes the list to an IDR. There's really no reason why this needed to change in the first place. > > > static struct pwm_chip *pwmchip_find_by_name(const char *name) > > { > > - struct pwm_chip *chip; > > + struct pwm_chip_private *priv; > > unsigned long id, tmp; > > > > if (!name) > > @@ -42,12 +44,12 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name) > > > > mutex_lock(&pwm_lock); > > > > - idr_for_each_entry_ul(&pwmchip_idr, chip, tmp, id) { > > - const char *chip_name = dev_name(chip->dev); > > + idr_for_each_entry_ul(&pwm_chips, priv, tmp, id) { > > + const char *chip_name = dev_name(priv->chip->dev); > > two pointer dereferences instead of one before. > > > if (chip_name && strcmp(chip_name, name) == 0) { > > mutex_unlock(&pwm_lock); > > - return chip; > > + return priv->chip; > > one pointer reference instead of none before. > > > } > > } > > > > @@ -58,23 +60,24 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name) > > > > static int pwm_device_request(struct pwm_device *pwm, const char *label) > > { > > + struct pwm_chip *chip = pwm->priv->chip; > > With my approach getting the chip of a struct pwm_device is only one > pointer dereference away. You need two. None of the functions here are called very often, so even if this isn't optimized away it would hardly matter. > > int err; > > > > if (test_bit(PWMF_REQUESTED, &pwm->flags)) > > return -EBUSY; > > > > - if (!try_module_get(pwm->chip->owner)) > > + if (!try_module_get(pwm->priv->owner)) > > return -ENODEV; > > > > - if (pwm->chip->ops->request) { > > - err = pwm->chip->ops->request(pwm->chip, pwm); > > + if (chip->ops->request) { > > + err = chip->ops->request(chip, pwm); > > You seem to have reimplemented half of > https://lore.kernel.org/linux-pwm/20231130072105.966848-1-u.kleine-koenig@pengutronix.de > here. I started prototyping this before you sent out that patch. > > if (err) { > > - module_put(pwm->chip->owner); > > + module_put(pwm->priv->owner); > > return err; > > } > > } > > > > - if (pwm->chip->ops->get_state) { > > + if (chip->ops->get_state) { > > /* > > * Zero-initialize state because most drivers are unaware of > > * .usage_power. The other members of state are supposed to be > > @@ -84,7 +87,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label) > > */ > > struct pwm_state state = { 0, }; > > > > - err = pwm->chip->ops->get_state(pwm->chip, pwm, &state); > > + err = chip->ops->get_state(chip, pwm, &state); > > trace_pwm_get(pwm, &state, err); > > > > if (!err) > > @@ -196,6 +199,64 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > > return true; > > } > > > > +static struct pwm_chip_private *pwmchip_alloc(struct pwm_chip *chip, > > + struct module *owner) > > +{ > > + struct pwm_chip_private *priv; > > + struct pwm_device *pwm; > > + unsigned int i; > > + int err; > > + > > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return ERR_PTR(-ENOMEM); > > + > > + priv->pwms = kcalloc(chip->npwm, sizeof(*pwm), GFP_KERNEL); > > + if (!priv->pwms) { > > + err = -ENOMEM; > > + goto free; > > + } > > Oh, so you don't use one additional allocation but two. This could easily be folded into one, but we already determined that we both have different opinions about whether this matters a lot or not. > > > + priv->owner = owner; > > + priv->chip = chip; > > + > > + for (i = 0; i < chip->npwm; i++) { > > + struct pwm_device *pwm = &priv->pwms[i]; > > + > > + pwm->priv = priv; > > + pwm->hwpwm = i; > > + } > > + > > + mutex_lock(&pwm_lock); > > + > > + err = idr_alloc(&pwm_chips, priv, 0, 0, GFP_KERNEL); > > + if (err < 0) { > > + mutex_unlock(&pwm_lock); > > + goto free; > > + } > > + > > + mutex_unlock(&pwm_lock); > > + > > + priv->id = err; > > + > > + return priv; > > + > > +free: > > + kfree(priv->pwms); > > + kfree(priv); > > + return ERR_PTR(err); > > +} > > + > > +static void pwmchip_free(struct pwm_chip_private *priv) > > +{ > > + mutex_lock(&pwm_lock); > > + idr_remove(&pwm_chips, priv->id); > > + mutex_unlock(&pwm_lock); > > + > > + kfree(priv->pwms); > > + kfree(priv); > > +} > > + > > /** > > * __pwmchip_add() - register a new PWM chip > > * @chip: the PWM chip to add > > @@ -208,8 +269,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > > */ > > int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > > { > > - unsigned int i; > > - int ret; > > + struct pwm_chip_private *priv; > > > > if (!chip || !chip->dev || !chip->ops || !chip->npwm) > > return -EINVAL; > > @@ -217,36 +277,16 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > > if (!pwm_ops_check(chip)) > > return -EINVAL; > > > > - chip->owner = owner; > > - > > - chip->pwms = kcalloc(chip->npwm, sizeof(*chip->pwms), GFP_KERNEL); > > - if (!chip->pwms) > > + priv = pwmchip_alloc(chip, owner); > > + if (!priv) > > return -ENOMEM; > > > > - mutex_lock(&pwm_lock); > > - > > - ret = idr_alloc(&pwmchip_idr, chip, 0, 0, GFP_KERNEL); > > - if (ret < 0) { > > - mutex_unlock(&pwm_lock); > > - kfree(chip->pwms); > > - return ret; > > - } > > - > > - chip->id = ret; > > - > > - for (i = 0; i < chip->npwm; i++) { > > - struct pwm_device *pwm = &chip->pwms[i]; > > - > > - pwm->chip = chip; > > - pwm->hwpwm = i; > > - } > > - > > - mutex_unlock(&pwm_lock); > > + chip->priv = priv; > > > > if (IS_ENABLED(CONFIG_OF)) > > of_pwmchip_add(chip); > > > > - pwmchip_sysfs_export(chip); > > + pwmchip_sysfs_export(priv); > > > > return 0; > > } > > @@ -260,18 +300,14 @@ EXPORT_SYMBOL_GPL(__pwmchip_add); > > */ > > void pwmchip_remove(struct pwm_chip *chip) > > { > > - pwmchip_sysfs_unexport(chip); > > - > > - if (IS_ENABLED(CONFIG_OF)) > > - of_pwmchip_remove(chip); > > - > > mutex_lock(&pwm_lock); > > > > - idr_remove(&pwmchip_idr, chip->id); > > + pwmchip_sysfs_unexport(chip->priv); > > > > - mutex_unlock(&pwm_lock); > > + if (IS_ENABLED(CONFIG_OF)) > > + of_pwmchip_remove(chip); > > > > - kfree(chip->pwms); > > + pwmchip_free(chip->priv); > > } > > EXPORT_SYMBOL_GPL(pwmchip_remove); > > > > @@ -315,7 +351,7 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, > > return ERR_PTR(-EINVAL); > > > > mutex_lock(&pwm_lock); > > - pwm = &chip->pwms[index]; > > + pwm = &chip->priv->pwms[index]; > > two pointer dereferences instead of one before. > > > err = pwm_device_request(pwm, label); > > if (err < 0) > > @@ -329,9 +365,9 @@ EXPORT_SYMBOL_GPL(pwm_request_from_chip); > > static void pwm_apply_state_debug(struct pwm_device *pwm, > > const struct pwm_state *state) > > { > > - struct pwm_state *last = &pwm->last; > > - struct pwm_chip *chip = pwm->chip; > > + struct pwm_chip *chip = pwm->priv->chip; > > struct pwm_state s1 = { 0 }, s2 = { 0 }; > > + struct pwm_state *last = &pwm->last; > > int err; > > > > if (!IS_ENABLED(CONFIG_PWM_DEBUG)) > > @@ -439,7 +475,6 @@ static void pwm_apply_state_debug(struct pwm_device *pwm, > > */ > > int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > > { > > - struct pwm_chip *chip; > > int err; > > > > /* > > @@ -455,8 +490,6 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > > state->duty_cycle > state->period) > > return -EINVAL; > > > > - chip = pwm->chip; > > - > > if (state->period == pwm->state.period && > > state->duty_cycle == pwm->state.duty_cycle && > > state->polarity == pwm->state.polarity && > > @@ -464,7 +497,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > > state->usage_power == pwm->state.usage_power) > > return 0; > > > > - err = chip->ops->apply(chip, pwm, state); > > + err = pwm->priv->chip->ops->apply(pwm->priv->chip, pwm, state); > > four pointer dereferences instead of two before. > > > trace_pwm_apply(pwm, state, err); > > if (err) > > return err; > > @@ -492,16 +525,19 @@ EXPORT_SYMBOL_GPL(pwm_apply_state); > > int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, > > unsigned long timeout) > > { > > + struct pwm_chip *chip; > > int err; > > > > - if (!pwm || !pwm->chip->ops) > > + if (!pwm) > > return -EINVAL; > > > > - if (!pwm->chip->ops->capture) > > + chip = pwm->priv->chip; > > One pointer deference more than before (assuming sensible compilation). > > > + if (!chip || !chip->ops || !chip->ops->capture) > > return -ENOSYS; > > Changing the return code for !chip->ops isn't intended, is it? It's a quick prototype, I didn't pay particular attention to keeping all error codes exactly as they were before. > > diff --git a/drivers/pwm/internal.h b/drivers/pwm/internal.h > > new file mode 100644 > > index 000000000000..44fffd3b6744 > > --- /dev/null > > +++ b/drivers/pwm/internal.h > > @@ -0,0 +1,38 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > +#ifndef PWM_INTERNAL_H > > +#define PWM_INTERNAL_H > > + > > +#include <linux/list.h> > > This is unused? Yes, can be dropped now. This was prototyped prior to the IDR rework and I forgot to drop this after rebasing it on top. > > +struct pwm_chip; > > +struct pwm_device; > > + > > +/** > > + * struct pwm_chip_private - subsystem-private PWM chip data > > + * @chip: driver-private PWM chip data > > + * @owner: module providing the chip > > + * @pwms: array of PWM devices allocated by the framework > > + * @base: number of first PWM controlled by this chip > > + */ > > +struct pwm_chip_private { > > + struct pwm_chip *chip; > > + struct module *owner; > > + struct pwm_device *pwms; > > + unsigned int id; > > +}; > > With my approach the struct pwm_chip * pointer isn't needed because it's > all in one structure. So you have two structures where I only have one > and your two are bigger in sum than my single one (not counting memory > management overhead and alignment). The split is on purpose and has the benefit of clearly separating the PWM internal data structures from the driver accessible ones. So in my book this is an advantage. > > +#ifdef CONFIG_PWM_SYSFS > > +void pwmchip_sysfs_export(struct pwm_chip_private *priv); > > +void pwmchip_sysfs_unexport(struct pwm_chip_private *priv); > > +#else > > +static inline void pwmchip_sysfs_export(struct pwm_chip_private *priv) > > +{ > > +} > > + > > +static inline void pwmchip_sysfs_unexport(struct pwm_chip_private *priv) > > +{ > > +} > > +#endif /* CONFIG_PWM_SYSFS */ > > + > > +#endif /* PWM_INTERNAL_H */ > > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c > > index 3f2c5031a3ba..6bbbfaa582c1 100644 > > --- a/drivers/pwm/pwm-atmel-hlcdc.c > > +++ b/drivers/pwm/pwm-atmel-hlcdc.c > > @@ -15,6 +15,8 @@ > > #include <linux/pwm.h> > > #include <linux/regmap.h> > > > > +#include "internal.h" > > + > > #define ATMEL_HLCDC_PWMCVAL_MASK GENMASK(15, 8) > > #define ATMEL_HLCDC_PWMCVAL(x) (((x) << 8) & ATMEL_HLCDC_PWMCVAL_MASK) > > #define ATMEL_HLCDC_PWMPOL BIT(4) > > @@ -185,7 +187,7 @@ static int atmel_hlcdc_pwm_suspend(struct device *dev) > > struct atmel_hlcdc_pwm *atmel = dev_get_drvdata(dev); > > > > /* Keep the periph clock enabled if the PWM is still running. */ > > - if (pwm_is_enabled(&atmel->chip.pwms[0])) > > + if (pwm_is_enabled(&atmel->chip.priv->pwms[0])) > > Didn't work so well to hide the internals of pwm_chip_private from the > drivers? > > > clk_disable_unprepare(atmel->hlcdc->periph_clk); > > > > return 0; > > > > [... a few more drivers where hiding the internals failed ...] Yes, and you'll recall that these are for exactly the cases where in the past you have argued that the core should take a more active role in making sure that PWMs get properly disabled. These should be temporary and once this logic is all moved into the core these should disappear. > > > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c > > index 4edb994fa2e1..653df20afe1c 100644 > > --- a/drivers/pwm/sysfs.c > > +++ b/drivers/pwm/sysfs.c > > @@ -14,6 +14,8 @@ > > #include <linux/kdev_t.h> > > #include <linux/pwm.h> > > > > +#include "internal.h" > > + > > struct pwm_export { > > struct device child; > > struct pwm_device *pwm; > > @@ -311,7 +313,7 @@ static ssize_t export_store(struct device *parent, > > struct device_attribute *attr, > > const char *buf, size_t len) > > { > > - struct pwm_chip *chip = dev_get_drvdata(parent); > > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > > struct pwm_device *pwm; > > unsigned int hwpwm; > > int ret; > > @@ -320,10 +322,10 @@ static ssize_t export_store(struct device *parent, > > if (ret < 0) > > return ret; > > > > - if (hwpwm >= chip->npwm) > > + if (hwpwm >= priv->chip->npwm) > > One more pointer dereference than before. You could trade that for some > data duplication by copying npwm to struct pwmchip_priv for the case > when you need .npwm but the chip is already gone. When the chip is gone, none of these functions should be doing anything other than returning an error. None of that is implemented in this yet. This is merely a preparatory patch. > > > return -ENODEV; > > > > - pwm = pwm_request_from_chip(chip, hwpwm, "sysfs"); > > + pwm = pwm_request_from_chip(priv->chip, hwpwm, "sysfs"); > > if (IS_ERR(pwm)) > > return PTR_ERR(pwm); > > > > @@ -339,7 +341,7 @@ static ssize_t unexport_store(struct device *parent, > > struct device_attribute *attr, > > const char *buf, size_t len) > > { > > - struct pwm_chip *chip = dev_get_drvdata(parent); > > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > > unsigned int hwpwm; > > int ret; > > > > @@ -347,10 +349,10 @@ static ssize_t unexport_store(struct device *parent, > > if (ret < 0) > > return ret; > > > > - if (hwpwm >= chip->npwm) > > + if (hwpwm >= priv->chip->npwm) > > One more pointer dereference than before. > > > return -ENODEV; > > > > - ret = pwm_unexport_child(parent, &chip->pwms[hwpwm]); > > + ret = pwm_unexport_child(parent, &priv->pwms[hwpwm]); > > > > return ret ? : len; > > } > > @@ -359,9 +361,9 @@ static DEVICE_ATTR_WO(unexport); > > static ssize_t npwm_show(struct device *parent, struct device_attribute *attr, > > char *buf) > > { > > - const struct pwm_chip *chip = dev_get_drvdata(parent); > > + const struct pwm_chip_private *priv = dev_get_drvdata(parent); > > > > - return sysfs_emit(buf, "%u\n", chip->npwm); > > + return sysfs_emit(buf, "%u\n", priv->chip->npwm); > > } > > static DEVICE_ATTR_RO(npwm); > > > > @@ -411,12 +413,12 @@ static int pwm_class_apply_state(struct pwm_export *export, > > > > static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) > > { > > - struct pwm_chip *chip = dev_get_drvdata(parent); > > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > > unsigned int i; > > int ret = 0; > > > > for (i = 0; i < npwm; i++) { > > - struct pwm_device *pwm = &chip->pwms[i]; > > + struct pwm_device *pwm = &priv->pwms[i]; > > struct pwm_state state; > > struct pwm_export *export; > > > > @@ -442,12 +444,12 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) > > > > static int pwm_class_suspend(struct device *parent) > > { > > - struct pwm_chip *chip = dev_get_drvdata(parent); > > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > > unsigned int i; > > int ret = 0; > > > > - for (i = 0; i < chip->npwm; i++) { > > - struct pwm_device *pwm = &chip->pwms[i]; > > + for (i = 0; i < priv->chip->npwm; i++) { > > + struct pwm_device *pwm = &priv->pwms[i]; > > struct pwm_state state; > > struct pwm_export *export; > > > > @@ -483,9 +485,9 @@ static int pwm_class_suspend(struct device *parent) > > > > static int pwm_class_resume(struct device *parent) > > { > > - struct pwm_chip *chip = dev_get_drvdata(parent); > > + struct pwm_chip_private *priv = dev_get_drvdata(parent); > > > > - return pwm_class_resume_npwm(parent, chip->npwm); > > + return pwm_class_resume_npwm(parent, priv->chip->npwm); > > One more pointer dereference than before. > > > } > > > > static DEFINE_SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume); > > @@ -501,7 +503,7 @@ static int pwmchip_sysfs_match(struct device *parent, const void *data) > > return dev_get_drvdata(parent) == data; > > } > > > > -void pwmchip_sysfs_export(struct pwm_chip *chip) > > +void pwmchip_sysfs_export(struct pwm_chip_private *priv) > > { > > struct device *parent; > > > > @@ -509,26 +511,26 @@ void pwmchip_sysfs_export(struct pwm_chip *chip) > > * If device_create() fails the pwm_chip is still usable by > > * the kernel it's just not exported. > > */ > > - parent = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip, > > - "pwmchip%d", chip->id); > > + parent = device_create(&pwm_class, priv->chip->dev, MKDEV(0, 0), priv, > > + "pwmchip%d", priv->id); > > if (IS_ERR(parent)) { > > - dev_warn(chip->dev, > > + dev_warn(priv->chip->dev, > > One more pointer dereference than before. > > > "device_create failed for pwm_chip sysfs export\n"); > > } > > } > > The TL;DR; is essentially what I already wrote in my last reply to Bart > in the v3 thread[1]: > > - My approach needs more changes to the individual drivers (which I > don't consider a relevant disadvantage given that the resulting code > is better); > - My approach works with less pointer dereferences which IMHO also > simplifies understanding the code as all relevant data is in a single > place. > - My approach has a weaker separation between the core and the lowlevel > drivers. That's ok in my book given that this doesn't complicate the > lowlevel drivers and that hiding details considerably better doesn't > work anyhow (see the drivers that need internal.h in your patch). > > For me the single allocation issue is only an added bonus. The relevant > advantage of my approach is that the code is easier and (probably) more > efficient. I happen to disagree. I think adding pwmchip_alloc() makes things much more complicated for low level drivers. Thierry
Hello Thierry, On Mon, Dec 11, 2023 at 12:33:04PM +0100, Thierry Reding wrote: > On Fri, Dec 08, 2023 at 07:50:33PM +0100, Uwe Kleine-König wrote: > > The TL;DR; is essentially what I already wrote in my last reply to Bart > > in the v3 thread[1]: > > > > - My approach needs more changes to the individual drivers (which I > > don't consider a relevant disadvantage given that the resulting code > > is better); > > - My approach works with less pointer dereferences which IMHO also > > simplifies understanding the code as all relevant data is in a single > > place. > > - My approach has a weaker separation between the core and the lowlevel > > drivers. That's ok in my book given that this doesn't complicate the > > lowlevel drivers and that hiding details considerably better doesn't > > work anyhow (see the drivers that need internal.h in your patch). > > > > For me the single allocation issue is only an added bonus. The relevant > > advantage of my approach is that the code is easier and (probably) more > > efficient. > > I happen to disagree. I think adding pwmchip_alloc() makes things much > more complicated for low level drivers. Looking at e.g. https://lore.kernel.org/linux-pwm/2dda818b8bbbe8ba4b9df5ab54f960ff4a4f1ab5.1701860672.git.u.kleine-koenig@pengutronix.de/ I wonder where you see "much more complication". OK, there are two pointers now for chip and private data, but I'd call that at most a "mild" complication[1] which is more than balanced out by the simplifications in the remaining parts of that patch. Best regards Uwe [1] I'm not sure I'd refuse someone suggesting the following patch on top of today's next: diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c index c0c53968f3e9..d32e65914599 100644 --- a/drivers/pwm/pwm-microchip-core.c +++ b/drivers/pwm/pwm-microchip-core.c @@ -448,12 +448,14 @@ MODULE_DEVICE_TABLE(of, mchp_core_of_match); static int mchp_core_pwm_probe(struct platform_device *pdev) { struct mchp_core_pwm_chip *mchp_core_pwm; + struct pwm_chip *chip; struct resource *regs; int ret; mchp_core_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_core_pwm), GFP_KERNEL); if (!mchp_core_pwm) return -ENOMEM; + chip = &mchp_core_pwm->chip; mchp_core_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, ®s); if (IS_ERR(mchp_core_pwm->base)) @@ -470,9 +472,9 @@ static int mchp_core_pwm_probe(struct platform_device *pdev) mutex_init(&mchp_core_pwm->lock); - mchp_core_pwm->chip.dev = &pdev->dev; - mchp_core_pwm->chip.ops = &mchp_core_pwm_ops; - mchp_core_pwm->chip.npwm = 16; + chip->dev = &pdev->dev; + chip->ops = &mchp_core_pwm_ops; + chip->npwm = 16; mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0)); mchp_core_pwm->channel_enabled |= @@ -485,7 +487,7 @@ static int mchp_core_pwm_probe(struct platform_device *pdev) writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD); mchp_core_pwm->update_timestamp = ktime_get(); - ret = devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip); + ret = devm_pwmchip_add(&pdev->dev, chip); if (ret) return dev_err_probe(&pdev->dev, ret, "Failed to add pwmchip\n"); With that applied before the above mentioned patch there is no complication at all in my eyes.
Hello Thierry, On Mon, Dec 11, 2023 at 12:33:04PM +0100, Thierry Reding wrote: > On Fri, Dec 08, 2023 at 07:50:33PM +0100, Uwe Kleine-König wrote: > > You don't need to touch all drivers because you didn't change struct > > pwm_chip::dev yet. (If you really want, you don't need to change that, > > but then you have some duplication as chip->dev holds the same value as > > priv->dev.parent in the end.) > > I don't think that's a problem. These are for two logically separate > things, after all. How are they different? I'd say one is the initializer for the other and (ideally) unused after that. With that interpretation they are indeed different, but then it's ugly that the initializer keeps staying around. > Duplication can also sometimes be useful to simplify > things. There are plently of cases where we use local variables for the > same reason. local variables go away though after the respective function is left. chip->dev and its copy priv->dev.parent stay around for the full lifetime of the chip. > > > @@ -58,23 +60,24 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name) > > > > > > static int pwm_device_request(struct pwm_device *pwm, const char *label) > > > { > > > + struct pwm_chip *chip = pwm->priv->chip; > > > > With my approach getting the chip of a struct pwm_device is only one > > pointer dereference away. You need two. > > None of the functions here are called very often, so even if this isn't > optimized away it would hardly matter. I'd say pwm_apply_state() at least matters. Also I think that making a slow path quicker is a good thing. I wonder how we'll converge to an approach that can go into the mainline given that we both have our strong opinions. Best regards Uwe
Hello, This series is based on Thierry's for-next. It starts with some cleanups that were all sent out separately already: - "pwm: Reduce number of pointer dereferences in pwm_device_request()" https://lore.kernel.org/linux-pwm/20231130072105.966848-1-u.kleine-koenig@pengutronix.de - "pwm: crc: Use consistent variable naming for driver data" https://lore.kernel.org/linux-pwm/20231130074133.969806-1-u.kleine-koenig@pengutronix.de - Two leds/qcom-lpg patches https://lore.kernel.org/linux-leds/20231126095230.683204-1-u.kleine-koenig@pengutronix.de Lee already claimed to have taken the series already, but it's not yet in next. - "leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm chip" https://lore.kernel.org/linux-leds/20231124215208.616551-3-u.kleine-koenig@pengutronix.de In the following patches I changed: - "pwm: cros-ec: Change prototype of helper to prepare further changes" + This was simplified in response to feedback by Tzung-Bi Shih - Make pwmchip_priv() static (and don't export it), let drivers use a new pwmchip_get_drvdata() instead. - For drm/ti-sn65dsi86.c and leds/qcom-lpg make use of pwmchip_set_drvdata() which makes the conversion to devm_pwmchip_alloc() much prettier. - Some cleanups here and there - Add review tags received in v3 I kept all tags even though the pwmchip_alloc() patches changed slightly. Most of the time that's only s/pwmchip_priv/pwmchip_get_drvdata/ though. Still, if you object, just tell me. (This affects Paul Cercueil on patch #68, Conor Dooley on patch #76 and Greg for patch #109.) I kept the pwmchip_alloc() + pwmchip_register() approach despite Bart not liking it. To balance that out I don't like Bart's alternative approach. There are no technically relevant differences between the two approaches and no benchmarks that show either of the two to be better than the other. Conceptually the design ideas around pwmchip_alloc() + pwmchip_register() are used in several other subsystems, so it's a proven way to do things. In the discussion about v3 Bart pointed out that there is an imbalance in the function naming, as pwmchip_alloc() must be paired with pwmchip_put() (and not something like pwmchip_free()). (This is hidden from the drivers though, as they all use a devm_pwmchip_alloc() which does the right thing for them.) I could be talked into renaming pwmchip_alloc() to pwmchip_get_new(). I hesitated to do that though as other subsystems also use foo_alloc() and I'm ambivalent about which is the better name. Thierry objected to patch "pwm: Ensure that pwm_chips are allocated using pwmchip_alloc()". I still think it's a nice idea to do that. It only adds very little overhead in a slow path and might save some time for people working on an out-of-tree driver. So it also helps the folks who currently work on new drivers for mainline who rebase on a tree containing this series. I won't insist about that one though, it can just be dropped. Otherwise I should have addressed all feedback and hope the series is now pretty enough to not cause further concerns. The last patch shows a prototype of the motivation for improving the lifetime tracking implemented in this series. It contains a character device userspace API to use PWMs that is considerably faster and easier to use than the sysfs API; apply() is faster by a factor of 4 on my test machine (arm/stm32mp1). Note that currently there is no known issue with lifetimes in the pwm subsystem as device links ensure that consumers go away if a pwm_chip gets unbound. There was an issue before, but that wasn't PWM specific and is fixed in v6.7-rc1~77^2~3 ("driver core: Release all resources during unbind before updating device links")[1] In the presence of character devices making use of PWMs, such an improved lifetime tracking becomes necessary, otherwise unloading drivers (or unbinding/hotplugging devices) might crash the kernel. As calls to .apply() are serialized now, the locking in several drivers could just be dropped in a future cleanup. I didn't do that now to not enlarge the series still more and maybe the locking in the core can be changed to be more fine-grained (abusing(?) a reader-writer-lock) which would allow concurrent callers for different lines of a single pwm_chip. In that case the locking must not be dropped. Also the explicit device links could be dropped now as they are not strictly needed any more (though requested PWMs might just stop working when the lowlevel driver is unbound which is a bit ugly, but can happen at any time for other reasons). Also on systems probed from device tree there are device links between consumers and providers anyhow. (Don't know about ACPI systems, I think there are device links then.) Best regards Uwe [1] This fix is also backported to the various stable releases (v6.6.3, v6.5.13, v6.1.64, v5.15.140; about to go into v5.10.x, v5.4.x, v4.19.x and v4.14.x) Uwe Kleine-König (115): pwm: Reduce number of pointer dereferences in pwm_device_request() pwm: crc: Use consistent variable naming for driver data leds: qcom-lpg: Use devm_pwmchip_add() simplifying driver removal leds: qcom-lpg: Consistenly use dev_err_probe() in .probe()'s error path leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm chip pwm: cros-ec: Change prototype of helpers to prepare further changes pwm: Provide a macro to get the parent device of a given chip pwm: ab8500: Make use of pwmchip_parent() macro pwm: atmel: Make use of pwmchip_parent() macro pwm: atmel-tcb: Make use of pwmchip_parent() macro pwm: bcm-kona: Make use of pwmchip_parent() macro pwm: crc: Make use of pwmchip_parent() macro pwm: cros-ec: Make use of pwmchip_parent() macro pwm: dwc: Make use of pwmchip_parent() macro pwm: ep93xx: Make use of pwmchip_parent() macro pwm: fsl-ftm: Make use of pwmchip_parent() macro pwm: img: Make use of parent device pointer in driver data pwm: imx27: Make use of pwmchip_parent() macro pwm: jz4740: Make use of pwmchip_parent() macro pwm: lpc18xx-sct: Make use of parent device pointer in driver data pwm: lpss: Make use of pwmchip_parent() macro pwm: mediatek: Make use of pwmchip_parent() macro pwm: meson: Make use of pwmchip_parent() macro pwm: mtk-disp: Make use of pwmchip_parent() macro pwm: omap: Make use of pwmchip_parent() macro pwm: pca9685: Store parent device in driver data pwm: raspberrypi-poe: Make use of pwmchip_parent() macro pwm: rcar: Make use of pwmchip_parent() macro pwm: rz-mtu3: Make use of pwmchip_parent() macro pwm: samsung: Make use of pwmchip_parent() macro pwm: sifive: Make use of pwmchip_parent() macro pwm: stm32-lp: Make use of pwmchip_parent() macro pwm: stm32: Make use of pwmchip_parent() macro pwm: stmpe: Make use of pwmchip_parent() macro pwm: sun4i: Make use of pwmchip_parent() macro pwm: tiecap: Make use of pwmchip_parent() macro pwm: tiehrpwm: Make use of pwmchip_parent() macro pwm: twl-led: Make use of pwmchip_parent() macro pwm: twl: Make use of pwmchip_parent() macro pwm: vt8500: Make use of pwmchip_parent() macro staging: greybus: pwm: Make use of pwmchip_parent() macro pwm: Provide wrappers for storing and getting driver private data pwm: Provide devm_pwmchip_alloc() function pwm: ab8500: Make use of devm_pwmchip_alloc() function pwm: apple: Make use of devm_pwmchip_alloc() function pwm: atmel-hlcdc: Make use of devm_pwmchip_alloc() function pwm: atmel: Make use of devm_pwmchip_alloc() function pwm: atmel-tcb: Make use of devm_pwmchip_alloc() function pwm: bcm2835: Make use of devm_pwmchip_alloc() function pwm: bcm-iproc: Make use of devm_pwmchip_alloc() function pwm: bcm-kona: Make use of devm_pwmchip_alloc() function pwm: berlin: Make use of devm_pwmchip_alloc() function pwm: brcmstb: Make use of devm_pwmchip_alloc() function pwm: clk: Make use of devm_pwmchip_alloc() function pwm: clps711x: Make use of devm_pwmchip_alloc() function pwm: crc: Make use of devm_pwmchip_alloc() function pwm: cros-ec: Make use of devm_pwmchip_alloc() function pwm: dwc: Make use of devm_pwmchip_alloc() function pwm: ep93xx: Make use of devm_pwmchip_alloc() function pwm: fsl-ftm: Make use of devm_pwmchip_alloc() function pwm: hibvt: Make use of devm_pwmchip_alloc() function pwm: img: Make use of devm_pwmchip_alloc() function pwm: imx1: Make use of devm_pwmchip_alloc() function pwm: imx27: Make use of devm_pwmchip_alloc() function pwm: imx-tpm: Make use of devm_pwmchip_alloc() function pwm: intel-lgm: Make use of devm_pwmchip_alloc() function pwm: iqs620a: Make use of devm_pwmchip_alloc() function pwm: jz4740: Make use of devm_pwmchip_alloc() function pwm: keembay: Make use of devm_pwmchip_alloc() function pwm: lp3943: Make use of devm_pwmchip_alloc() function pwm: lpc18xx-sct: Make use of devm_pwmchip_alloc() function pwm: lpc32xx: Make use of devm_pwmchip_alloc() function pwm: lpss-*: Make use of devm_pwmchip_alloc() function pwm: mediatek: Make use of devm_pwmchip_alloc() function pwm: meson: Make use of devm_pwmchip_alloc() function pwm: microchip-core: Make use of devm_pwmchip_alloc() function pwm: mtk-disp: Make use of devm_pwmchip_alloc() function pwm: mxs: Make use of devm_pwmchip_alloc() function pwm: ntxec: Make use of devm_pwmchip_alloc() function pwm: omap-dmtimer: Make use of devm_pwmchip_alloc() function pwm: pca9685: Make use of devm_pwmchip_alloc() function pwm: pxa: Make use of devm_pwmchip_alloc() function pwm: raspberrypi-poe: Make use of devm_pwmchip_alloc() function pwm: rcar: Make use of devm_pwmchip_alloc() function pwm: renesas-tpu: Make use of devm_pwmchip_alloc() function pwm: rockchip: Make use of devm_pwmchip_alloc() function pwm: rz-mtu3: Make use of devm_pwmchip_alloc() function pwm: samsung: Make use of devm_pwmchip_alloc() function pwm: sifive: Make use of devm_pwmchip_alloc() function pwm: sl28cpld: Make use of devm_pwmchip_alloc() function pwm: spear: Make use of devm_pwmchip_alloc() function pwm: sprd: Make use of devm_pwmchip_alloc() function pwm: sti: Make use of devm_pwmchip_alloc() function pwm: stm32-lp: Make use of devm_pwmchip_alloc() function pwm: stm32: Make use of devm_pwmchip_alloc() function pwm: stmpe: Make use of devm_pwmchip_alloc() function pwm: sun4i: Make use of devm_pwmchip_alloc() function pwm: sunplus: Make use of devm_pwmchip_alloc() function pwm: tegra: Make use of devm_pwmchip_alloc() function pwm: tiecap: Make use of devm_pwmchip_alloc() function pwm: twl-led: Make use of devm_pwmchip_alloc() function pwm: twl: Make use of devm_pwmchip_alloc() function pwm: visconti: Make use of devm_pwmchip_alloc() function pwm: vt8500: Make use of devm_pwmchip_alloc() function pwm: xilinx: Make use of devm_pwmchip_alloc() function gpio: mvebu: Make use of devm_pwmchip_alloc() function drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function leds: qcom-lpg: Make use of devm_pwmchip_alloc() function staging: greybus: pwm: Make use of devm_pwmchip_alloc() function pwm: Ensure that pwm_chips are allocated using pwmchip_alloc() pwm: Ensure a struct pwm has the same lifetime as its pwm_chip pwm: Ensure the memory backing a PWM chip isn't freed while used pwm: Add more locking pwm: Make pwmchip_[sg]et_drvdata a wrapper around dev_set_drvdata WIP: pwm: Add support for pwmchip devices for faster and easier userspace access .../driver-api/driver-model/devres.rst | 1 + Documentation/driver-api/pwm.rst | 10 +- drivers/gpio/gpio-mvebu.c | 18 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 27 +- drivers/leds/rgb/leds-qcom-lpg.c | 75 ++- drivers/pwm/Kconfig | 4 - drivers/pwm/Makefile | 3 +- drivers/pwm/core.c | 490 +++++++++++++++--- drivers/pwm/pwm-ab8500.c | 36 +- drivers/pwm/pwm-apple.c | 18 +- drivers/pwm/pwm-atmel-hlcdc.c | 35 +- drivers/pwm/pwm-atmel-tcb.c | 26 +- drivers/pwm/pwm-atmel.c | 37 +- drivers/pwm/pwm-bcm-iproc.c | 19 +- drivers/pwm/pwm-bcm-kona.c | 21 +- drivers/pwm/pwm-bcm2835.c | 17 +- drivers/pwm/pwm-berlin.c | 29 +- drivers/pwm/pwm-brcmstb.c | 17 +- drivers/pwm/pwm-clk.c | 27 +- drivers/pwm/pwm-clps711x.c | 21 +- drivers/pwm/pwm-crc.c | 26 +- drivers/pwm/pwm-cros-ec.c | 55 +- drivers/pwm/pwm-dwc-core.c | 25 +- drivers/pwm/pwm-dwc.c | 18 +- drivers/pwm/pwm-dwc.h | 9 +- drivers/pwm/pwm-ep93xx.c | 21 +- drivers/pwm/pwm-fsl-ftm.c | 48 +- drivers/pwm/pwm-hibvt.c | 25 +- drivers/pwm/pwm-img.c | 51 +- drivers/pwm/pwm-imx-tpm.c | 34 +- drivers/pwm/pwm-imx1.c | 20 +- drivers/pwm/pwm-imx27.c | 26 +- drivers/pwm/pwm-intel-lgm.c | 17 +- drivers/pwm/pwm-iqs620a.c | 37 +- drivers/pwm/pwm-jz4740.c | 35 +- drivers/pwm/pwm-keembay.c | 17 +- drivers/pwm/pwm-lp3943.c | 17 +- drivers/pwm/pwm-lpc18xx-sct.c | 35 +- drivers/pwm/pwm-lpc32xx.c | 21 +- drivers/pwm/pwm-lpss-pci.c | 10 +- drivers/pwm/pwm-lpss-platform.c | 10 +- drivers/pwm/pwm-lpss.c | 34 +- drivers/pwm/pwm-lpss.h | 1 - drivers/pwm/pwm-mediatek.c | 28 +- drivers/pwm/pwm-meson.c | 57 +- drivers/pwm/pwm-microchip-core.c | 17 +- drivers/pwm/pwm-mtk-disp.c | 25 +- drivers/pwm/pwm-mxs.c | 32 +- drivers/pwm/pwm-ntxec.c | 30 +- drivers/pwm/pwm-omap-dmtimer.c | 46 +- drivers/pwm/pwm-pca9685.c | 98 ++-- drivers/pwm/pwm-pxa.c | 21 +- drivers/pwm/pwm-raspberrypi-poe.c | 20 +- drivers/pwm/pwm-rcar.c | 25 +- drivers/pwm/pwm-renesas-tpu.c | 18 +- drivers/pwm/pwm-rockchip.c | 24 +- drivers/pwm/pwm-rz-mtu3.c | 38 +- drivers/pwm/pwm-samsung.c | 56 +- drivers/pwm/pwm-sifive.c | 30 +- drivers/pwm/pwm-sl28cpld.c | 13 +- drivers/pwm/pwm-spear.c | 17 +- drivers/pwm/pwm-sprd.c | 50 +- drivers/pwm/pwm-sti.c | 34 +- drivers/pwm/pwm-stm32-lp.c | 29 +- drivers/pwm/pwm-stm32.c | 53 +- drivers/pwm/pwm-stmpe.c | 58 ++- drivers/pwm/pwm-sun4i.c | 38 +- drivers/pwm/pwm-sunplus.c | 17 +- drivers/pwm/pwm-tegra.c | 27 +- drivers/pwm/pwm-tiecap.c | 55 +- drivers/pwm/pwm-tiehrpwm.c | 72 +-- drivers/pwm/pwm-twl-led.c | 58 ++- drivers/pwm/pwm-twl.c | 50 +- drivers/pwm/pwm-visconti.c | 17 +- drivers/pwm/pwm-vt8500.c | 41 +- drivers/pwm/pwm-xilinx.c | 34 +- drivers/pwm/sysfs.c | 64 +-- drivers/staging/greybus/pwm.c | 130 ++--- include/linux/platform_data/x86/pwm-lpss.h | 4 +- include/linux/pwm.h | 43 +- include/uapi/linux/pwm.h | 23 + 81 files changed, 1690 insertions(+), 1325 deletions(-) create mode 100644 include/uapi/linux/pwm.h base-commit: 6d0589a70587116c7ff09e027659c6254f883b2d