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
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);
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);
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
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
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
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,
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!
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!
-
Alex Elder -
patchwork-bot+chrome-platform＠kernel.org -
Thierry Reding -
Uwe Kleine-König