[PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"

[PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

while working on an extension for the pwm framework, I noticed that some
drivers and even the core only nearly consistently named all variables
and struct members holding a pointer to a struct pwm_chip "chip":

$ git grep -Pho 'struct pwm_chip \**[a-z0-9_]+(*nla:[\(a-z0-9_])' v6.5-rc1 | sort | uniq -c | sort -n
      1 struct pwm_chip *pwm
      1 struct pwm_chip pwm
      1 struct pwm_chip pwm_chip
      2 struct pwm_chip *_chip
      4 struct pwm_chip *c
      8 struct pwm_chip *pc
     57 struct pwm_chip chip
    358 struct pwm_chip *chip

With this series applied these are all called "chip" with one exception:
The led driver drivers/leds/rgb/leds-qcom-lpg.c uses "pwm". Maybe
"pwmchip" would be a better name, but I'm not sure that using "chip" was
an improvement there as this isn't a pure pwm driver. I'm not touching
that one.

The first offenders I found were the core and the atmel-hlcdc driver.
After I found these I optimistically assumed these were the only ones
with the unusual names and send patches for these out individually
before checking systematically.

The atmel-hlcdc patch is included here unchanged, the core patch now
also adapted the declaration of the changed functions in <linux/pwm.h>.
I marked these two as "superseded" in patchwork already.

All patches in this series are pairwise independent of each other. I
don't know if the staging patch should better go in via the greybus tree
or via pwm. Both is possible without needing coordination.

Best regards
Uwe


Uwe Kleine-König (10):
  pwm: Use a consistent name for pwm_chip pointers in the core
  pwm: atmel-hlcdc: Use consistent variable naming
  pwm: bcm-kona: Consistenly name pwm_chip variables "chip"
  pwm: crc: Consistenly name pwm_chip variables "chip"
  pwm: cros-ec: Consistenly name pwm_chip variables "chip"
  pwm: lp3943: Consistenly name pwm_chip variables "chip"
  pwm: rockchip: Consistenly name pwm_chip variables "chip"
  pwm: sifive: Consistenly name pwm_chip variables "chip"
  pwm: sl28cpld: Consistenly name pwm_chip variables "chip"
  staging: greybus: pwm: Consistenly name pwm_chip variables "chip"

 drivers/pwm/core.c            | 28 +++++++--------
 drivers/pwm/pwm-atmel-hlcdc.c | 64 +++++++++++++++++------------------
 drivers/pwm/pwm-bcm-kona.c    |  4 +--
 drivers/pwm/pwm-crc.c         |  4 +--
 drivers/pwm/pwm-cros-ec.c     | 10 +++---
 drivers/pwm/pwm-lp3943.c      |  4 +--
 drivers/pwm/pwm-rockchip.c    |  4 +--
 drivers/pwm/pwm-sifive.c      |  4 +--
 drivers/pwm/pwm-sl28cpld.c    | 10 +++---
 drivers/staging/greybus/pwm.c | 12 +++----
 include/linux/pwm.h           |  6 ++--
 11 files changed, 75 insertions(+), 75 deletions(-)

base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
-- 
2.39.2

[PATCH 01/10] pwm: Use a consistent name for pwm_chip pointers in the core

[Uwe Kleine-König]

Most variables of type struct pwm_chip * are named "chip", there are
only three outliers called "pc". Change these three to "chip", too, for
consistency.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c  | 28 ++++++++++++++--------------
 include/linux/pwm.h |  6 +++---
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3dacceaef4a9..8c798753c016 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -127,28 +127,28 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 }
 
 struct pwm_device *
-of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
+of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (pc->of_pwm_n_cells < 2)
+	if (chip->of_pwm_n_cells < 2)
 		return ERR_PTR(-EINVAL);
 
 	/* flags in the third cell are optional */
 	if (args->args_count < 2)
 		return ERR_PTR(-EINVAL);
 
-	if (args->args[0] >= pc->npwm)
+	if (args->args[0] >= chip->npwm)
 		return ERR_PTR(-EINVAL);
 
-	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
+	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
 	if (IS_ERR(pwm))
 		return pwm;
 
 	pwm->args.period = args->args[1];
 	pwm->args.polarity = PWM_POLARITY_NORMAL;
 
-	if (pc->of_pwm_n_cells >= 3) {
+	if (chip->of_pwm_n_cells >= 3) {
 		if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
 			pwm->args.polarity = PWM_POLARITY_INVERSED;
 	}
@@ -158,18 +158,18 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
 
 struct pwm_device *
-of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (pc->of_pwm_n_cells < 1)
+	if (chip->of_pwm_n_cells < 1)
 		return ERR_PTR(-EINVAL);
 
 	/* validate that one cell is specified, optionally with flags */
 	if (args->args_count != 1 && args->args_count != 2)
 		return ERR_PTR(-EINVAL);
 
-	pwm = pwm_request_from_chip(pc, 0, NULL);
+	pwm = pwm_request_from_chip(chip, 0, NULL);
 	if (IS_ERR(pwm))
 		return pwm;
 
@@ -692,7 +692,7 @@ static struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
 	struct pwm_device *pwm = NULL;
 	struct of_phandle_args args;
 	struct device_link *dl;
-	struct pwm_chip *pc;
+	struct pwm_chip *chip;
 	int index = 0;
 	int err;
 
@@ -709,16 +709,16 @@ static struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
 		return ERR_PTR(err);
 	}
 
-	pc = fwnode_to_pwmchip(of_fwnode_handle(args.np));
-	if (IS_ERR(pc)) {
-		if (PTR_ERR(pc) != -EPROBE_DEFER)
+	chip = fwnode_to_pwmchip(of_fwnode_handle(args.np));
+	if (IS_ERR(chip)) {
+		if (PTR_ERR(chip) != -EPROBE_DEFER)
 			pr_err("%s(): PWM chip not found\n", __func__);
 
-		pwm = ERR_CAST(pc);
+		pwm = ERR_CAST(chip);
 		goto put;
 	}
 
-	pwm = pc->of_xlate(pc, &args);
+	pwm = chip->of_xlate(chip, &args);
 	if (IS_ERR(pwm))
 		goto put;
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 04ae1d9073a7..d2f9f690a9c1 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -298,7 +298,7 @@ struct pwm_chip {
 	int base;
 	unsigned int npwm;
 
-	struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
+	struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
 					const struct of_phandle_args *args);
 	unsigned int of_pwm_n_cells;
 
@@ -395,9 +395,9 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
 					 const char *label);
 
-struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
+struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *chip,
 		const struct of_phandle_args *args);
-struct pwm_device *of_pwm_single_xlate(struct pwm_chip *pc,
+struct pwm_device *of_pwm_single_xlate(struct pwm_chip *chip,
 				       const struct of_phandle_args *args);
 
 struct pwm_device *pwm_get(struct device *dev, const char *con_id);
-- 
2.39.2

[PATCH 02/10] pwm: atmel-hlcdc: Use consistent variable naming

[Uwe Kleine-König]

In pwm drivers the variable name "chip" is usually only used for struct
pwm_chip pointers. This driver however used "chip" for its driver data
and pwm_chip pointers are named "chip", too, when there is no driver
data around and "c" otherwise. Instead use "ddata" for driver data and
always "chip" for pwm_chips.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel-hlcdc.c | 64 +++++++++++++++++------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index 96a709a9d49a..9b0165d61c49 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -38,11 +38,11 @@ static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
 	return container_of(chip, struct atmel_hlcdc_pwm, chip);
 }
 
-static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
+static int atmel_hlcdc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 				 const struct pwm_state *state)
 {
-	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
-	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	struct atmel_hlcdc_pwm *ddata = to_atmel_hlcdc_pwm(chip);
+	struct atmel_hlcdc *hlcdc = ddata->hlcdc;
 	unsigned int status;
 	int ret;
 
@@ -54,7 +54,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
 		u32 pwmcfg;
 		int pres;
 
-		if (!chip->errata || !chip->errata->slow_clk_erratum) {
+		if (!ddata->errata || !ddata->errata->slow_clk_erratum) {
 			clk_freq = clk_get_rate(new_clk);
 			if (!clk_freq)
 				return -EINVAL;
@@ -64,7 +64,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
 		}
 
 		/* Errata: cannot use slow clk on some IP revisions */
-		if ((chip->errata && chip->errata->slow_clk_erratum) ||
+		if ((ddata->errata && ddata->errata->slow_clk_erratum) ||
 		    clk_period_ns > state->period) {
 			new_clk = hlcdc->sys_clk;
 			clk_freq = clk_get_rate(new_clk);
@@ -77,8 +77,8 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
 
 		for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
 		/* Errata: cannot divide by 1 on some IP revisions */
-			if (!pres && chip->errata &&
-			    chip->errata->div1_clk_erratum)
+			if (!pres && ddata->errata &&
+			    ddata->errata->div1_clk_erratum)
 				continue;
 
 			if ((clk_period_ns << pres) >= state->period)
@@ -90,7 +90,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
 
 		pwmcfg = ATMEL_HLCDC_PWMPS(pres);
 
-		if (new_clk != chip->cur_clk) {
+		if (new_clk != ddata->cur_clk) {
 			u32 gencfg = 0;
 			int ret;
 
@@ -98,8 +98,8 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
 			if (ret)
 				return ret;
 
-			clk_disable_unprepare(chip->cur_clk);
-			chip->cur_clk = new_clk;
+			clk_disable_unprepare(ddata->cur_clk);
+			ddata->cur_clk = new_clk;
 
 			if (new_clk == hlcdc->sys_clk)
 				gencfg = ATMEL_HLCDC_CLKPWMSEL;
@@ -160,8 +160,8 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
 		if (ret)
 			return ret;
 
-		clk_disable_unprepare(chip->cur_clk);
-		chip->cur_clk = NULL;
+		clk_disable_unprepare(ddata->cur_clk);
+		ddata->cur_clk = NULL;
 	}
 
 	return 0;
@@ -183,31 +183,31 @@ static const struct atmel_hlcdc_pwm_errata atmel_hlcdc_pwm_sama5d3_errata = {
 #ifdef CONFIG_PM_SLEEP
 static int atmel_hlcdc_pwm_suspend(struct device *dev)
 {
-	struct atmel_hlcdc_pwm *chip = dev_get_drvdata(dev);
+	struct atmel_hlcdc_pwm *ddata = dev_get_drvdata(dev);
 
 	/* Keep the periph clock enabled if the PWM is still running. */
-	if (pwm_is_enabled(&chip->chip.pwms[0]))
-		clk_disable_unprepare(chip->hlcdc->periph_clk);
+	if (pwm_is_enabled(&ddata->chip.pwms[0]))
+		clk_disable_unprepare(ddata->hlcdc->periph_clk);
 
 	return 0;
 }
 
 static int atmel_hlcdc_pwm_resume(struct device *dev)
 {
-	struct atmel_hlcdc_pwm *chip = dev_get_drvdata(dev);
+	struct atmel_hlcdc_pwm *ddata = dev_get_drvdata(dev);
 	struct pwm_state state;
 	int ret;
 
-	pwm_get_state(&chip->chip.pwms[0], &state);
+	pwm_get_state(&ddata->chip.pwms[0], &state);
 
 	/* Re-enable the periph clock it was stopped during suspend. */
 	if (!state.enabled) {
-		ret = clk_prepare_enable(chip->hlcdc->periph_clk);
+		ret = clk_prepare_enable(ddata->hlcdc->periph_clk);
 		if (ret)
 			return ret;
 	}
 
-	return atmel_hlcdc_pwm_apply(&chip->chip, &chip->chip.pwms[0], &state);
+	return atmel_hlcdc_pwm_apply(&ddata->chip, &ddata->chip.pwms[0], &state);
 }
 #endif
 
@@ -244,14 +244,14 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 	struct device *dev = &pdev->dev;
-	struct atmel_hlcdc_pwm *chip;
+	struct atmel_hlcdc_pwm *ddata;
 	struct atmel_hlcdc *hlcdc;
 	int ret;
 
 	hlcdc = dev_get_drvdata(dev->parent);
 
-	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
-	if (!chip)
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
 		return -ENOMEM;
 
 	ret = clk_prepare_enable(hlcdc->periph_clk);
@@ -260,31 +260,31 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 
 	match = of_match_node(atmel_hlcdc_dt_ids, dev->parent->of_node);
 	if (match)
-		chip->errata = match->data;
+		ddata->errata = match->data;
 
-	chip->hlcdc = hlcdc;
-	chip->chip.ops = &atmel_hlcdc_pwm_ops;
-	chip->chip.dev = dev;
-	chip->chip.npwm = 1;
+	ddata->hlcdc = hlcdc;
+	ddata->chip.ops = &atmel_hlcdc_pwm_ops;
+	ddata->chip.dev = dev;
+	ddata->chip.npwm = 1;
 
-	ret = pwmchip_add(&chip->chip);
+	ret = pwmchip_add(&ddata->chip);
 	if (ret) {
 		clk_disable_unprepare(hlcdc->periph_clk);
 		return ret;
 	}
 
-	platform_set_drvdata(pdev, chip);
+	platform_set_drvdata(pdev, ddata);
 
 	return 0;
 }
 
 static void atmel_hlcdc_pwm_remove(struct platform_device *pdev)
 {
-	struct atmel_hlcdc_pwm *chip = platform_get_drvdata(pdev);
+	struct atmel_hlcdc_pwm *ddata = platform_get_drvdata(pdev);
 
-	pwmchip_remove(&chip->chip);
+	pwmchip_remove(&ddata->chip);
 
-	clk_disable_unprepare(chip->hlcdc->periph_clk);
+	clk_disable_unprepare(ddata->hlcdc->periph_clk);
 }
 
 static const struct of_device_id atmel_hlcdc_pwm_dt_ids[] = {
-- 
2.39.2

Re: [PATCH 02/10] pwm: atmel-hlcdc: Use consistent variable naming

[claudiu beznea]

On 14.07.2023 23:56, Uwe Kleine-König wrote:
> In pwm drivers the variable name "chip" is usually only used for struct
> pwm_chip pointers. This driver however used "chip" for its driver data
> and pwm_chip pointers are named "chip", too, when there is no driver
> data around and "c" otherwise. Instead use "ddata" for driver data and
> always "chip" for pwm_chips.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>   drivers/pwm/pwm-atmel-hlcdc.c | 64 +++++++++++++++++------------------
>   1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> index 96a709a9d49a..9b0165d61c49 100644
> --- a/drivers/pwm/pwm-atmel-hlcdc.c
> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> @@ -38,11 +38,11 @@ static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
>   	return container_of(chip, struct atmel_hlcdc_pwm, chip);
>   }
>   
> -static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
> +static int atmel_hlcdc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   				 const struct pwm_state *state)
>   {
> -	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
> -	struct atmel_hlcdc *hlcdc = chip->hlcdc;
> +	struct atmel_hlcdc_pwm *ddata = to_atmel_hlcdc_pwm(chip);
> +	struct atmel_hlcdc *hlcdc = ddata->hlcdc;
>   	unsigned int status;
>   	int ret;
>   
> @@ -54,7 +54,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   		u32 pwmcfg;
>   		int pres;
>   
> -		if (!chip->errata || !chip->errata->slow_clk_erratum) {
> +		if (!ddata->errata || !ddata->errata->slow_clk_erratum) {
>   			clk_freq = clk_get_rate(new_clk);
>   			if (!clk_freq)
>   				return -EINVAL;
> @@ -64,7 +64,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   		}
>   
>   		/* Errata: cannot use slow clk on some IP revisions */
> -		if ((chip->errata && chip->errata->slow_clk_erratum) ||
> +		if ((ddata->errata && ddata->errata->slow_clk_erratum) ||
>   		    clk_period_ns > state->period) {
>   			new_clk = hlcdc->sys_clk;
>   			clk_freq = clk_get_rate(new_clk);
> @@ -77,8 +77,8 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   
>   		for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
>   		/* Errata: cannot divide by 1 on some IP revisions */
> -			if (!pres && chip->errata &&
> -			    chip->errata->div1_clk_erratum)
> +			if (!pres && ddata->errata &&
> +			    ddata->errata->div1_clk_erratum)
>   				continue;
>   
>   			if ((clk_period_ns << pres) >= state->period)
> @@ -90,7 +90,7 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   
>   		pwmcfg = ATMEL_HLCDC_PWMPS(pres);
>   
> -		if (new_clk != chip->cur_clk) {
> +		if (new_clk != ddata->cur_clk) {
>   			u32 gencfg = 0;
>   			int ret;
>   
> @@ -98,8 +98,8 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   			if (ret)
>   				return ret;
>   
> -			clk_disable_unprepare(chip->cur_clk);
> -			chip->cur_clk = new_clk;
> +			clk_disable_unprepare(ddata->cur_clk);
> +			ddata->cur_clk = new_clk;
>   
>   			if (new_clk == hlcdc->sys_clk)
>   				gencfg = ATMEL_HLCDC_CLKPWMSEL;
> @@ -160,8 +160,8 @@ static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
>   		if (ret)
>   			return ret;
>   
> -		clk_disable_unprepare(chip->cur_clk);
> -		chip->cur_clk = NULL;
> +		clk_disable_unprepare(ddata->cur_clk);
> +		ddata->cur_clk = NULL;
>   	}
>   
>   	return 0;
> @@ -183,31 +183,31 @@ static const struct atmel_hlcdc_pwm_errata atmel_hlcdc_pwm_sama5d3_errata = {
>   #ifdef CONFIG_PM_SLEEP
>   static int atmel_hlcdc_pwm_suspend(struct device *dev)
>   {
> -	struct atmel_hlcdc_pwm *chip = dev_get_drvdata(dev);
> +	struct atmel_hlcdc_pwm *ddata = dev_get_drvdata(dev);
>   
>   	/* Keep the periph clock enabled if the PWM is still running. */
> -	if (pwm_is_enabled(&chip->chip.pwms[0]))
> -		clk_disable_unprepare(chip->hlcdc->periph_clk);
> +	if (pwm_is_enabled(&ddata->chip.pwms[0]))
> +		clk_disable_unprepare(ddata->hlcdc->periph_clk);
>   
>   	return 0;
>   }
>   
>   static int atmel_hlcdc_pwm_resume(struct device *dev)
>   {
> -	struct atmel_hlcdc_pwm *chip = dev_get_drvdata(dev);
> +	struct atmel_hlcdc_pwm *ddata = dev_get_drvdata(dev);
>   	struct pwm_state state;
>   	int ret;
>   
> -	pwm_get_state(&chip->chip.pwms[0], &state);
> +	pwm_get_state(&ddata->chip.pwms[0], &state);
>   
>   	/* Re-enable the periph clock it was stopped during suspend. */
>   	if (!state.enabled) {
> -		ret = clk_prepare_enable(chip->hlcdc->periph_clk);
> +		ret = clk_prepare_enable(ddata->hlcdc->periph_clk);
>   		if (ret)
>   			return ret;
>   	}
>   
> -	return atmel_hlcdc_pwm_apply(&chip->chip, &chip->chip.pwms[0], &state);
> +	return atmel_hlcdc_pwm_apply(&ddata->chip, &ddata->chip.pwms[0], &state);
>   }
>   #endif
>   
> @@ -244,14 +244,14 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
>   {
>   	const struct of_device_id *match;
>   	struct device *dev = &pdev->dev;
> -	struct atmel_hlcdc_pwm *chip;
> +	struct atmel_hlcdc_pwm *ddata;
>   	struct atmel_hlcdc *hlcdc;
>   	int ret;
>   
>   	hlcdc = dev_get_drvdata(dev->parent);
>   
> -	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> -	if (!chip)
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
>   		return -ENOMEM;
>   
>   	ret = clk_prepare_enable(hlcdc->periph_clk);
> @@ -260,31 +260,31 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
>   
>   	match = of_match_node(atmel_hlcdc_dt_ids, dev->parent->of_node);
>   	if (match)
> -		chip->errata = match->data;
> +		ddata->errata = match->data;
>   
> -	chip->hlcdc = hlcdc;
> -	chip->chip.ops = &atmel_hlcdc_pwm_ops;
> -	chip->chip.dev = dev;
> -	chip->chip.npwm = 1;
> +	ddata->hlcdc = hlcdc;
> +	ddata->chip.ops = &atmel_hlcdc_pwm_ops;
> +	ddata->chip.dev = dev;
> +	ddata->chip.npwm = 1;
>   
> -	ret = pwmchip_add(&chip->chip);
> +	ret = pwmchip_add(&ddata->chip);
>   	if (ret) {
>   		clk_disable_unprepare(hlcdc->periph_clk);
>   		return ret;
>   	}
>   
> -	platform_set_drvdata(pdev, chip);
> +	platform_set_drvdata(pdev, ddata);
>   
>   	return 0;
>   }
>   
>   static void atmel_hlcdc_pwm_remove(struct platform_device *pdev)
>   {
> -	struct atmel_hlcdc_pwm *chip = platform_get_drvdata(pdev);
> +	struct atmel_hlcdc_pwm *ddata = platform_get_drvdata(pdev);
>   
> -	pwmchip_remove(&chip->chip);
> +	pwmchip_remove(&ddata->chip);
>   
> -	clk_disable_unprepare(chip->hlcdc->periph_clk);
> +	clk_disable_unprepare(ddata->hlcdc->periph_clk);
>   }
>   
>   static const struct of_device_id atmel_hlcdc_pwm_dt_ids[] = {

Re: [PATCH 02/10] pwm: atmel-hlcdc: Use consistent variable naming

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]

On Fri, Jul 14, 2023 at 10:56:15PM +0200, Uwe Kleine-König wrote:
> In pwm drivers the variable name "chip" is usually only used for struct
> pwm_chip pointers. This driver however used "chip" for its driver data
> and pwm_chip pointers are named "chip", too, when there is no driver
> data around and "c" otherwise. Instead use "ddata" for driver data and
> always "chip" for pwm_chips.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-atmel-hlcdc.c | 64 +++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> index 96a709a9d49a..9b0165d61c49 100644
> --- a/drivers/pwm/pwm-atmel-hlcdc.c
> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> @@ -38,11 +38,11 @@ static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
>  	return container_of(chip, struct atmel_hlcdc_pwm, chip);
>  }
>  
> -static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
> +static int atmel_hlcdc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  				 const struct pwm_state *state)
>  {
> -	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
> -	struct atmel_hlcdc *hlcdc = chip->hlcdc;
> +	struct atmel_hlcdc_pwm *ddata = to_atmel_hlcdc_pwm(chip);

Can we not just use something like "data", "priv" or "atmel"? "ddata" is
horrific and looks like a typo.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 02/10] pwm: atmel-hlcdc: Use consistent variable naming

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On Thu, Jul 20, 2023 at 08:53:50AM +0200, Thierry Reding wrote:
> On Fri, Jul 14, 2023 at 10:56:15PM +0200, Uwe Kleine-König wrote:
> > In pwm drivers the variable name "chip" is usually only used for struct
> > pwm_chip pointers. This driver however used "chip" for its driver data
> > and pwm_chip pointers are named "chip", too, when there is no driver
> > data around and "c" otherwise. Instead use "ddata" for driver data and
> > always "chip" for pwm_chips.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-atmel-hlcdc.c | 64 +++++++++++++++++------------------
> >  1 file changed, 32 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> > index 96a709a9d49a..9b0165d61c49 100644
> > --- a/drivers/pwm/pwm-atmel-hlcdc.c
> > +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> > @@ -38,11 +38,11 @@ static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
> >  	return container_of(chip, struct atmel_hlcdc_pwm, chip);
> >  }
> >  
> > -static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
> > +static int atmel_hlcdc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  				 const struct pwm_state *state)
> >  {
> > -	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
> > -	struct atmel_hlcdc *hlcdc = chip->hlcdc;
> > +	struct atmel_hlcdc_pwm *ddata = to_atmel_hlcdc_pwm(chip);
> 
> Can we not just use something like "data", "priv" or "atmel"? "ddata" is
> horrific and looks like a typo.

I like "ddata" which isn't so uncommon (pwm-atmel-hlcdc and pwm-sifive
use it and git grep '\<ddata\>' suggests it's common in other parts of
the kernel, too.)
It's the same naming scheme as "pdata". If you feel strong here, do you
like "drvdata" better? Among your suggestions my favourite is "priv".

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 02/10] pwm: atmel-hlcdc: Use consistent variable naming

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]

Hello Thierry,

On Thu, Jul 20, 2023 at 09:29:58AM +0200, Uwe Kleine-König wrote:
> On Thu, Jul 20, 2023 at 08:53:50AM +0200, Thierry Reding wrote:
> > On Fri, Jul 14, 2023 at 10:56:15PM +0200, Uwe Kleine-König wrote:
> > > [...]
> > > -	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
> > > -	struct atmel_hlcdc *hlcdc = chip->hlcdc;
> > > +	struct atmel_hlcdc_pwm *ddata = to_atmel_hlcdc_pwm(chip);
> > 
> > Can we not just use something like "data", "priv" or "atmel"? "ddata" is
> > horrific and looks like a typo.
> 
> I like "ddata" which isn't so uncommon (pwm-atmel-hlcdc and pwm-sifive
> use it

I have to correct this. pwm-atmel-hlcdc only used it after this patch of
course. But there is pwm-sti which uses it and the two stm32 drivers use
if for driver data of the pwm's parent device.

> and git grep '\<ddata\>' suggests it's common in other parts of
> the kernel, too.)
> It's the same naming scheme as "pdata". If you feel strong here, do you
> like "drvdata" better? Among your suggestions my favourite is "priv".

I noticed you applied my patch and replaced "ddata" by "atmel" without
mentioning that in the commit log or this thread.

After my reply above, that's pretty steep.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 02/10] pwm: atmel-hlcdc: Use consistent variable naming

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

On Fri, Jul 21, 2023 at 07:58:04PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Jul 20, 2023 at 09:29:58AM +0200, Uwe Kleine-König wrote:
> > On Thu, Jul 20, 2023 at 08:53:50AM +0200, Thierry Reding wrote:
> > > On Fri, Jul 14, 2023 at 10:56:15PM +0200, Uwe Kleine-König wrote:
> > > > [...]
> > > > -	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
> > > > -	struct atmel_hlcdc *hlcdc = chip->hlcdc;
> > > > +	struct atmel_hlcdc_pwm *ddata = to_atmel_hlcdc_pwm(chip);
> > > 
> > > Can we not just use something like "data", "priv" or "atmel"? "ddata" is
> > > horrific and looks like a typo.
> > 
> > I like "ddata" which isn't so uncommon (pwm-atmel-hlcdc and pwm-sifive
> > use it
> 
> I have to correct this. pwm-atmel-hlcdc only used it after this patch of
> course. But there is pwm-sti which uses it and the two stm32 drivers use
> if for driver data of the pwm's parent device.
> 
> > and git grep '\<ddata\>' suggests it's common in other parts of
> > the kernel, too.)
> > It's the same naming scheme as "pdata". If you feel strong here, do you
> > like "drvdata" better? Among your suggestions my favourite is "priv".
> 
> I noticed you applied my patch and replaced "ddata" by "atmel" without
> mentioning that in the commit log or this thread.
> 
> After my reply above, that's pretty steep.

I didn't think you'd care. I've added a note to the commit message now.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 03/10] pwm: bcm-kona: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

Most variables holding a pointer to a pwm_chip are called "chip" which
is also the usual name in most other pwm drivers. Rename the single
variable that have a different name to be called "chip", too, for
consistency.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-bcm-kona.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 4fa6e249e4cf..e5b00cc9f7a7 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -61,9 +61,9 @@ struct kona_pwmc {
 	struct clk *clk;
 };
 
-static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
+static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *chip)
 {
-	return container_of(_chip, struct kona_pwmc, chip);
+	return container_of(chip, struct kona_pwmc, chip);
 }
 
 /*
-- 
2.39.2

[PATCH 04/10] pwm: crc: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

Most variables holding a pointer to a pwm_chip are called "chip" which
is also the usual name in most other pwm drivers. Rename the single
variable that have a different name to be called "chip", too, for
consistency.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-crc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
index 4703b4a0b6e4..b9f063dc6b5f 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -34,9 +34,9 @@ struct crystalcove_pwm {
 	struct regmap *regmap;
 };
 
-static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *pc)
+static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *chip)
 {
-	return container_of(pc, struct crystalcove_pwm, chip);
+	return container_of(chip, struct crystalcove_pwm, chip);
 }
 
 static int crc_pwm_calc_clk_div(int period_ns)
-- 
2.39.2

[PATCH 05/10] pwm: cros-ec: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

Most variables holding a pointer to a pwm_chip are called "chip" which
is also the usual name in most other pwm drivers. Rename the two
variables that have a different name to be called "chip", too, for
consistency.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-cros-ec.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 74e863aa1d8d..154ca0f90847 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -37,9 +37,9 @@ struct cros_ec_pwm {
 	u16 duty_cycle;
 };
 
-static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
+static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *chip)
 {
-	return container_of(c, struct cros_ec_pwm_device, chip);
+	return container_of(chip, struct cros_ec_pwm_device, chip);
 }
 
 static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -218,14 +218,14 @@ static int cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 }
 
 static struct pwm_device *
-cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+cros_ec_pwm_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (args->args[0] >= pc->npwm)
+	if (args->args[0] >= chip->npwm)
 		return ERR_PTR(-EINVAL);
 
-	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
+	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
 	if (IS_ERR(pwm))
 		return pwm;
 
-- 
2.39.2

Re: [PATCH 05/10] pwm: cros-ec: Consistenly name pwm_chip variables "chip"

[Tzung-Bi Shih]

On Fri, Jul 14, 2023 at 10:56:18PM +0200, Uwe Kleine-König wrote:
> Most variables holding a pointer to a pwm_chip are called "chip" which
> is also the usual name in most other pwm drivers. Rename the two
> variables that have a different name to be called "chip", too, for
> consistency.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

[PATCH 06/10] pwm: lp3943: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

Most variables holding a pointer to a pwm_chip are called "chip" which
is also the usual name in most other pwm drivers. Rename the single
variable that have a different name to be called "chip", too, for
consistency.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lp3943.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-lp3943.c b/drivers/pwm/pwm-lp3943.c
index 35675e4058c6..a39411b87e19 100644
--- a/drivers/pwm/pwm-lp3943.c
+++ b/drivers/pwm/pwm-lp3943.c
@@ -24,9 +24,9 @@ struct lp3943_pwm {
 	struct lp3943_platform_data *pdata;
 };
 
-static inline struct lp3943_pwm *to_lp3943_pwm(struct pwm_chip *_chip)
+static inline struct lp3943_pwm *to_lp3943_pwm(struct pwm_chip *chip)
 {
-	return container_of(_chip, struct lp3943_pwm, chip);
+	return container_of(chip, struct lp3943_pwm, chip);
 }
 
 static struct lp3943_pwm_map *
-- 
2.39.2

[PATCH 07/10] pwm: rockchip: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

Most variables holding a pointer to a pwm_chip are called "chip" which
is also the usual name in most other pwm drivers. Rename the single
variable that have a different name to be called "chip", too, for
consistency.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-rockchip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index c1a1f2d864b5..03ee18fb82d5 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -52,9 +52,9 @@ struct rockchip_pwm_data {
 	u32 enable_conf;
 };
 
-static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
+static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *chip)
 {
-	return container_of(c, struct rockchip_pwm_chip, chip);
+	return container_of(chip, struct rockchip_pwm_chip, chip);
 }
 
 static int rockchip_pwm_get_state(struct pwm_chip *chip,
-- 
2.39.2

[PATCH 08/10] pwm: sifive: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

Most variables holding a pointer to a pwm_chip are called "chip" which
is also the usual name in most other pwm drivers. Rename the single
variable that have a different name to be called "chip", too, for
consistency.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-sifive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index ae49d67ab2b1..25b9b7d9476a 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -51,9 +51,9 @@ struct pwm_sifive_ddata {
 };
 
 static inline
-struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c)
+struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *chip)
 {
-	return container_of(c, struct pwm_sifive_ddata, chip);
+	return container_of(chip, struct pwm_sifive_ddata, chip);
 }
 
 static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *pwm)
-- 
2.39.2

[PATCH 09/10] pwm: sl28cpld: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

Nearly all pwm drivers use the name "chip" for the member in the driver
struct pointing to the pwm_chip. Also all local variables and function
parameters with this type use this name. Rename the struct pwm_chip
member accordingly for consistency.

Also rename the parameter of the macro sl28cpld_pwm_from_chip to "chip".

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-sl28cpld.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
index e64900ad4ba1..98b0024f9658 100644
--- a/drivers/pwm/pwm-sl28cpld.c
+++ b/drivers/pwm/pwm-sl28cpld.c
@@ -80,12 +80,12 @@
 	regmap_write((priv)->regmap, (priv)->offset + (reg), (val))
 
 struct sl28cpld_pwm {
-	struct pwm_chip pwm_chip;
+	struct pwm_chip chip;
 	struct regmap *regmap;
 	u32 offset;
 };
-#define sl28cpld_pwm_from_chip(_chip) \
-	container_of(_chip, struct sl28cpld_pwm, pwm_chip)
+#define sl28cpld_pwm_from_chip(chip) \
+	container_of(chip, struct sl28cpld_pwm, chip)
 
 static int sl28cpld_pwm_get_state(struct pwm_chip *chip,
 				  struct pwm_device *pwm,
@@ -228,12 +228,12 @@ static int sl28cpld_pwm_probe(struct platform_device *pdev)
 	}
 
 	/* Initialize the pwm_chip structure */
-	chip = &priv->pwm_chip;
+	chip = &priv->chip;
 	chip->dev = &pdev->dev;
 	chip->ops = &sl28cpld_pwm_ops;
 	chip->npwm = 1;
 
-	ret = devm_pwmchip_add(&pdev->dev, &priv->pwm_chip);
+	ret = devm_pwmchip_add(&pdev->dev, chip);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to add PWM chip (%pe)",
 			ERR_PTR(ret));
-- 
2.39.2

Re: [PATCH 09/10] pwm: sl28cpld: Consistenly name pwm_chip variables "chip"

[Michael Walle]

Hi,

Am 2023-07-14 22:56, schrieb Uwe Kleine-König:
> Nearly all pwm drivers use the name "chip" for the member in the driver
> struct pointing to the pwm_chip. Also all local variables and function
> parameters with this type use this name. Rename the struct pwm_chip
> member accordingly for consistency.
> 
> Also rename the parameter of the macro sl28cpld_pwm_from_chip to 
> "chip".
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> ---

> -#define sl28cpld_pwm_from_chip(_chip) \
> -	container_of(_chip, struct sl28cpld_pwm, pwm_chip)
> +#define sl28cpld_pwm_from_chip(chip) \
> +	container_of(chip, struct sl28cpld_pwm, chip)

This looks strange now. Before only the the first parameter of
container_of() is a parameter of the macro. After the change
it's the first and the last. I.e. the name of the member
comes from the argument of the macro.

Was that intended? Looks like it happens to work because
everything is named "chip", but nevertheless it doesn't look
correct to me.

I'd expect it to be:
#define sl28cpld_pwm_from_chip(_chip) \
	container_of(_chip, struct sl28cpld_pwm, chip)

-michael

Re: [PATCH 09/10] pwm: sl28cpld: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2209 bytes --]

Hello,

On Wed, Jul 19, 2023 at 04:17:41PM +0200, Michael Walle wrote:
> Am 2023-07-14 22:56, schrieb Uwe Kleine-König:
> > Nearly all pwm drivers use the name "chip" for the member in the driver
> > struct pointing to the pwm_chip. Also all local variables and function
> > parameters with this type use this name. Rename the struct pwm_chip
> > member accordingly for consistency.
> > 
> > Also rename the parameter of the macro sl28cpld_pwm_from_chip to "chip".
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > ---
> 
> > -#define sl28cpld_pwm_from_chip(_chip) \
> > -	container_of(_chip, struct sl28cpld_pwm, pwm_chip)
> > +#define sl28cpld_pwm_from_chip(chip) \
> > +	container_of(chip, struct sl28cpld_pwm, chip)
> 
> This looks strange now. Before only the the first parameter of
> container_of() is a parameter of the macro. After the change
> it's the first and the last. I.e. the name of the member
> comes from the argument of the macro.
> 
> Was that intended? Looks like it happens to work because
> everything is named "chip", but nevertheless it doesn't look
> correct to me.
> 
> I'd expect it to be:
> #define sl28cpld_pwm_from_chip(_chip) \
> 	container_of(_chip, struct sl28cpld_pwm, chip)

Indeed, that's bogus. My preference would be to make this a static
inline, i.e. 

diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
index 98b0024f9658..c789e934671e 100644
--- a/drivers/pwm/pwm-sl28cpld.c
+++ b/drivers/pwm/pwm-sl28cpld.c
@@ -84,8 +84,11 @@ struct sl28cpld_pwm {
 	struct regmap *regmap;
 	u32 offset;
 };
-#define sl28cpld_pwm_from_chip(chip) \
-	container_of(chip, struct sl28cpld_pwm, chip)
+
+static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct sl28cpld_pwm, chip);
+}
 
 static int sl28cpld_pwm_get_state(struct pwm_chip *chip,
 				  struct pwm_device *pwm,

but I can live with _chip, too.

Best regards and thanks for catching that,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 09/10] pwm: sl28cpld: Consistenly name pwm_chip variables "chip"

[Michael Walle]

Hi,

>> I'd expect it to be:
>> #define sl28cpld_pwm_from_chip(_chip) \
>> 	container_of(_chip, struct sl28cpld_pwm, chip)
> 
> Indeed, that's bogus. My preference would be to make this a static
> inline, i.e.
> 
> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> index 98b0024f9658..c789e934671e 100644
> --- a/drivers/pwm/pwm-sl28cpld.c
> +++ b/drivers/pwm/pwm-sl28cpld.c
> @@ -84,8 +84,11 @@ struct sl28cpld_pwm {
>  	struct regmap *regmap;
>  	u32 offset;
>  };
> -#define sl28cpld_pwm_from_chip(chip) \
> -	container_of(chip, struct sl28cpld_pwm, chip)
> +
> +static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct 
> pwm_chip *chip)
> +{
> +	return container_of(chip, struct sl28cpld_pwm, chip);
> +}
> 
>  static int sl28cpld_pwm_get_state(struct pwm_chip *chip,
>  				  struct pwm_device *pwm,
> 
> but I can live with _chip, too.

I don't have a strong preference. Looks like most drivers use
the inline. Go with that :)

-michael

[PATCH 10/10] staging: greybus: pwm: Consistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

All function parameters of type pointer to struct pwm_chip in this
driver are called chip which is also the usual name of function
parameters and local variables in most other pwm drivers. For consistency
use the same name for the local variable of that type.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/staging/greybus/pwm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
index 88da1d796f13..c483e1f0738e 100644
--- a/drivers/staging/greybus/pwm.c
+++ b/drivers/staging/greybus/pwm.c
@@ -267,7 +267,7 @@ static int gb_pwm_probe(struct gbphy_device *gbphy_dev,
 {
 	struct gb_connection *connection;
 	struct gb_pwm_chip *pwmc;
-	struct pwm_chip *pwm;
+	struct pwm_chip *chip;
 	int ret;
 
 	pwmc = kzalloc(sizeof(*pwmc), GFP_KERNEL);
@@ -295,13 +295,13 @@ static int gb_pwm_probe(struct gbphy_device *gbphy_dev,
 	if (ret)
 		goto exit_connection_disable;
 
-	pwm = &pwmc->chip;
+	chip = &pwmc->chip;
 
-	pwm->dev = &gbphy_dev->dev;
-	pwm->ops = &gb_pwm_ops;
-	pwm->npwm = pwmc->pwm_max + 1;
+	chip->dev = &gbphy_dev->dev;
+	chip->ops = &gb_pwm_ops;
+	chip->npwm = pwmc->pwm_max + 1;
 
-	ret = pwmchip_add(pwm);
+	ret = pwmchip_add(chip);
 	if (ret) {
 		dev_err(&gbphy_dev->dev,
 			"failed to register PWM: %d\n", ret);
-- 
2.39.2

Re: [PATCH 10/10] staging: greybus: pwm: Consistenly name pwm_chip variables "chip"

[Alex Elder]

On 7/14/23 3:56 PM, Uwe Kleine-König wrote:
> All function parameters of type pointer to struct pwm_chip in this
> driver are called chip which is also the usual name of function
> parameters and local variables in most other pwm drivers. For consistency
> use the same name for the local variable of that type.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Looks good to me.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>   drivers/staging/greybus/pwm.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
> index 88da1d796f13..c483e1f0738e 100644
> --- a/drivers/staging/greybus/pwm.c
> +++ b/drivers/staging/greybus/pwm.c
> @@ -267,7 +267,7 @@ static int gb_pwm_probe(struct gbphy_device *gbphy_dev,
>   {
>   	struct gb_connection *connection;
>   	struct gb_pwm_chip *pwmc;
> -	struct pwm_chip *pwm;
> +	struct pwm_chip *chip;
>   	int ret;
>   
>   	pwmc = kzalloc(sizeof(*pwmc), GFP_KERNEL);
> @@ -295,13 +295,13 @@ static int gb_pwm_probe(struct gbphy_device *gbphy_dev,
>   	if (ret)
>   		goto exit_connection_disable;
>   
> -	pwm = &pwmc->chip;
> +	chip = &pwmc->chip;
>   
> -	pwm->dev = &gbphy_dev->dev;
> -	pwm->ops = &gb_pwm_ops;
> -	pwm->npwm = pwmc->pwm_max + 1;
> +	chip->dev = &gbphy_dev->dev;
> +	chip->ops = &gb_pwm_ops;
> +	chip->npwm = pwmc->pwm_max + 1;
>   
> -	ret = pwmchip_add(pwm);
> +	ret = pwmchip_add(chip);
>   	if (ret) {
>   		dev_err(&gbphy_dev->dev,
>   			"failed to register PWM: %d\n", ret);

Re: [PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

On Fri, Jul 14, 2023 at 10:56:13PM +0200, Uwe Kleine-König wrote:
> while working on an extension for the pwm framework, I noticed that some
> drivers and even the core only nearly consistently named all variables
> and struct members holding a pointer to a struct pwm_chip "chip":
> 
> $ git grep -Pho 'struct pwm_chip \**[a-z0-9_]+(*nla:[\(a-z0-9_])' v6.5-rc1 | sort | uniq -c | sort -n
>       1 struct pwm_chip *pwm
>       1 struct pwm_chip pwm
>       1 struct pwm_chip pwm_chip
>       2 struct pwm_chip *_chip
>       4 struct pwm_chip *c
>       8 struct pwm_chip *pc
>      57 struct pwm_chip chip
>     358 struct pwm_chip *chip
> 
> With this series applied these are all called "chip" with one exception:
> The led driver drivers/leds/rgb/leds-qcom-lpg.c uses "pwm". Maybe
> "pwmchip" would be a better name, but I'm not sure that using "chip" was
> an improvement there as this isn't a pure pwm driver. I'm not touching
> that one.
> 
> The first offenders I found were the core and the atmel-hlcdc driver.
> After I found these I optimistically assumed these were the only ones
> with the unusual names and send patches for these out individually
> before checking systematically.
> 
> The atmel-hlcdc patch is included here unchanged, the core patch now
> also adapted the declaration of the changed functions in <linux/pwm.h>.
> I marked these two as "superseded" in patchwork already.
> 
> All patches in this series are pairwise independent of each other. I
> don't know if the staging patch should better go in via the greybus tree
> or via pwm. Both is possible without needing coordination.
> 
> Best regards
> Uwe
> 
> 
> Uwe Kleine-König (10):
>   pwm: Use a consistent name for pwm_chip pointers in the core
>   pwm: atmel-hlcdc: Use consistent variable naming
>   pwm: bcm-kona: Consistenly name pwm_chip variables "chip"
>   pwm: crc: Consistenly name pwm_chip variables "chip"
>   pwm: cros-ec: Consistenly name pwm_chip variables "chip"
>   pwm: lp3943: Consistenly name pwm_chip variables "chip"
>   pwm: rockchip: Consistenly name pwm_chip variables "chip"
>   pwm: sifive: Consistenly name pwm_chip variables "chip"
>   pwm: sl28cpld: Consistenly name pwm_chip variables "chip"
>   staging: greybus: pwm: Consistenly name pwm_chip variables "chip"

This would've been much easier if it had been a single patch. Now I have
to either make you redo the whole series because you've misspelled PWM
or I have to go and update it myself in most of the above patches. Hint:
I'll do the latter.

There is really no reason to split this up into this many patches for
such a trivial change.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

Hello Thierry,

On Thu, Jul 20, 2023 at 08:48:11AM +0200, Thierry Reding wrote:
> On Fri, Jul 14, 2023 at 10:56:13PM +0200, Uwe Kleine-König wrote:
> > Uwe Kleine-König (10):
> >   pwm: Use a consistent name for pwm_chip pointers in the core
> >   pwm: atmel-hlcdc: Use consistent variable naming
> >   pwm: bcm-kona: Consistenly name pwm_chip variables "chip"
> >   pwm: crc: Consistenly name pwm_chip variables "chip"
> >   pwm: cros-ec: Consistenly name pwm_chip variables "chip"
> >   pwm: lp3943: Consistenly name pwm_chip variables "chip"
> >   pwm: rockchip: Consistenly name pwm_chip variables "chip"
> >   pwm: sifive: Consistenly name pwm_chip variables "chip"
> >   pwm: sl28cpld: Consistenly name pwm_chip variables "chip"
> >   staging: greybus: pwm: Consistenly name pwm_chip variables "chip"
> 
> This would've been much easier if it had been a single patch. Now I have
> to either make you redo the whole series because you've misspelled PWM
> or I have to go and update it myself in most of the above patches. Hint:
> I'll do the latter.

I guess you want to do s/pwm driver/PWM driver/? Fine for me, thanks.

> There is really no reason to split this up into this many patches for
> such a trivial change.

Well, that's a subjective view. There are reasons to prefer several
small patches over one big one, too. A small patch can be indiviually
reviewed, so the "Reviewed-by: Alex Elder ..." tag only goes to the one
change that he actually looked at and if later a fix to the sifive
driver is to be backported to stable, the stable maintainers just pick
the sifive one instead of one big patch.

Did you skip the sl28cpld patch, or squash the fixup I sent in the reply
to Michael Walle?

Best regards and thanks,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2074 bytes --]

On Thu, Jul 20, 2023 at 09:10:33AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Jul 20, 2023 at 08:48:11AM +0200, Thierry Reding wrote:
> > On Fri, Jul 14, 2023 at 10:56:13PM +0200, Uwe Kleine-König wrote:
> > > Uwe Kleine-König (10):
> > >   pwm: Use a consistent name for pwm_chip pointers in the core
> > >   pwm: atmel-hlcdc: Use consistent variable naming
> > >   pwm: bcm-kona: Consistenly name pwm_chip variables "chip"
> > >   pwm: crc: Consistenly name pwm_chip variables "chip"
> > >   pwm: cros-ec: Consistenly name pwm_chip variables "chip"
> > >   pwm: lp3943: Consistenly name pwm_chip variables "chip"
> > >   pwm: rockchip: Consistenly name pwm_chip variables "chip"
> > >   pwm: sifive: Consistenly name pwm_chip variables "chip"
> > >   pwm: sl28cpld: Consistenly name pwm_chip variables "chip"
> > >   staging: greybus: pwm: Consistenly name pwm_chip variables "chip"
> > 
> > This would've been much easier if it had been a single patch. Now I have
> > to either make you redo the whole series because you've misspelled PWM
> > or I have to go and update it myself in most of the above patches. Hint:
> > I'll do the latter.
> 
> I guess you want to do s/pwm driver/PWM driver/? Fine for me, thanks.
> 
> > There is really no reason to split this up into this many patches for
> > such a trivial change.
> 
> Well, that's a subjective view. There are reasons to prefer several
> small patches over one big one, too. A small patch can be indiviually
> reviewed, so the "Reviewed-by: Alex Elder ..." tag only goes to the one
> change that he actually looked at and if later a fix to the sifive
> driver is to be backported to stable, the stable maintainers just pick
> the sifive one instead of one big patch.

Backports becoming more complicated would actually be a good reason not
to do this in the first place, but we've already discussed that enough
elsewhere.

> Did you skip the sl28cpld patch, or squash the fixup I sent in the reply
> to Michael Walle?

I squashed the fixup.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"

[Thierry Reding]

On Fri, 14 Jul 2023 22:56:13 +0200, Uwe Kleine-König wrote:
> while working on an extension for the pwm framework, I noticed that some
> drivers and even the core only nearly consistently named all variables
> and struct members holding a pointer to a struct pwm_chip "chip":
> 
> $ git grep -Pho 'struct pwm_chip \**[a-z0-9_]+(*nla:[\(a-z0-9_])' v6.5-rc1 | sort | uniq -c | sort -n
>       1 struct pwm_chip *pwm
>       1 struct pwm_chip pwm
>       1 struct pwm_chip pwm_chip
>       2 struct pwm_chip *_chip
>       4 struct pwm_chip *c
>       8 struct pwm_chip *pc
>      57 struct pwm_chip chip
>     358 struct pwm_chip *chip
> 
> [...]

Applied, thanks!

[01/10] pwm: Use a consistent name for pwm_chip pointers in the core
        commit: b4f78ff746ec5274fffa92fa2a4dc531360b5016
[02/10] pwm: atmel-hlcdc: Use consistent variable naming
        commit: 509143926e184762cdaffb6b67d3809fddd7f4d9
[03/10] pwm: bcm-kona: Consistenly name pwm_chip variables "chip"
        commit: af87385c7ad278207d34ff3681fa325a240ae87c
[04/10] pwm: crc: Consistenly name pwm_chip variables "chip"
        commit: fc30826d50d10d67628addfabb9367b5067efa42
[05/10] pwm: cros-ec: Consistenly name pwm_chip variables "chip"
        commit: 6b5fdb2b655ac9abe6fbd2cbcb25c8837e3e8553
[06/10] pwm: lp3943: Consistenly name pwm_chip variables "chip"
        commit: dd499b63618e523b47f30d99bf20f417de1187ff
[07/10] pwm: rockchip: Consistenly name pwm_chip variables "chip"
        commit: 8c297d1fdb5d2b81d39ada6b435fb92a41be9f17
[08/10] pwm: sifive: Consistenly name pwm_chip variables "chip"
        commit: cb69f40ea7cb139223901fcfc81e4e0a0a03673c
[09/10] pwm: sl28cpld: Consistenly name pwm_chip variables "chip"
        commit: e79974c5c3ddc3e8181f582117c4368557524f20
[10/10] staging: greybus: pwm: Consistenly name pwm_chip variables "chip"
        commit: efd1d1ad7f525809fcdf7538638a08274b75c99f

Best regards,
-- 
Thierry Reding <thierry.reding@gmail.com>

Re: [PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-kernelci)
by Thierry Reding <thierry.reding@gmail.com>:

On Fri, 14 Jul 2023 22:56:13 +0200 you wrote:
> while working on an extension for the pwm framework, I noticed that some
> drivers and even the core only nearly consistently named all variables
> and struct members holding a pointer to a struct pwm_chip "chip":
> 
> $ git grep -Pho 'struct pwm_chip \**[a-z0-9_]+(*nla:[\(a-z0-9_])' v6.5-rc1 | sort | uniq -c | sort -n
>       1 struct pwm_chip *pwm
>       1 struct pwm_chip pwm
>       1 struct pwm_chip pwm_chip
>       2 struct pwm_chip *_chip
>       4 struct pwm_chip *c
>       8 struct pwm_chip *pc
>      57 struct pwm_chip chip
>     358 struct pwm_chip *chip
> 
> [...]

Here is the summary with links:
  - [05/10] pwm: cros-ec: Consistenly name pwm_chip variables "chip"
    https://git.kernel.org/chrome-platform/c/5996cdf132da

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-next)
by Thierry Reding <thierry.reding@gmail.com>:

On Fri, 14 Jul 2023 22:56:13 +0200 you wrote:
> while working on an extension for the pwm framework, I noticed that some
> drivers and even the core only nearly consistently named all variables
> and struct members holding a pointer to a struct pwm_chip "chip":
> 
> $ git grep -Pho 'struct pwm_chip \**[a-z0-9_]+(*nla:[\(a-z0-9_])' v6.5-rc1 | sort | uniq -c | sort -n
>       1 struct pwm_chip *pwm
>       1 struct pwm_chip pwm
>       1 struct pwm_chip pwm_chip
>       2 struct pwm_chip *_chip
>       4 struct pwm_chip *c
>       8 struct pwm_chip *pc
>      57 struct pwm_chip chip
>     358 struct pwm_chip *chip
> 
> [...]

Here is the summary with links:
  - [05/10] pwm: cros-ec: Consistenly name pwm_chip variables "chip"
    https://git.kernel.org/chrome-platform/c/5996cdf132da

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html