| Message ID | 20211209162020.105255-3-nikita@trvn.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clock based PWM output driver | expand |
Hello, On Thu, Dec 09, 2021 at 09:20:20PM +0500, Nikita Travkin wrote: > +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip) > + > +static int pwm_clk_apply(struct pwm_chip *pwm_chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct pwm_clk_chip *chip = to_pwm_clk_chip(pwm_chip); > + int ret; > + u32 rate; > + > + if (!state->enabled && !pwm->state.enabled) > + return 0; > + > + if (state->enabled && !pwm->state.enabled) { > + ret = clk_enable(chip->clk); > + if (ret) > + return ret; > + } > + > + if (!state->enabled && pwm->state.enabled) { > + clk_disable(chip->clk); > + return 0; > + } This can be written a bit more compact as: if (!state->enabled) { if (pwm->state.enabled) clk_disable(chip->clk); return 0; } else if (!pwm->state.enabled) { ret = clk_enable(chip->clk); if (ret) return ret; } personally I find my version also easier to read, but that might be subjective. Missing handling for polarity. Either refuse inverted polarity, or set the duty_cycle to state->period - state->duty_cycle in the inverted case. > + rate = div64_u64(NSEC_PER_SEC, state->period); Please round up here, as .apply() should never implement a period bigger than requested. This also automatically improves the behaviour if state->period > NSEC_PER_SEC. > + ret = clk_set_rate(chip->clk, rate); > + if (ret) > + return ret; > + > + return clk_set_duty_cycle(chip->clk, state->duty_cycle, state->period); Is it possible to enable only after the duty cycle is set? This way we could prevent in some cases that a wrong setting makes it to the output. As there is not a single function to set rate (i.e. period) and duty_cycle it's not possible to prevent all glitches. Can you please note that in a paragraph at the beginning of the driver as does e.g. drivers/pwm/pwm-sl28cpld.c. (Please stick to the format, i.e. "Limitations:" and then all items without an empty line, to make this greppable.) > +} > + > +static const struct pwm_ops pwm_clk_ops = { > + .apply = pwm_clk_apply, > + .owner = THIS_MODULE, > +}; > + > +static int pwm_clk_probe(struct platform_device *pdev) > +{ > + struct pwm_clk_chip *chip; > + int ret; > + > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(chip->clk)) { > + dev_err(&pdev->dev, "Failed to get clock: %ld\n", PTR_ERR(chip->clk)); > + return PTR_ERR(chip->clk); Please use dev_err_probe() here and in the other error paths below. > + } > + > + chip->chip.dev = &pdev->dev; > + chip->chip.ops = &pwm_clk_ops; > + chip->chip.of_xlate = of_pwm_xlate_with_flags; > + chip->chip.of_pwm_n_cells = 2; > + chip->chip.base = 0; Please drop this line (see commit f9a8ee8c8bcd) > + chip->chip.npwm = 1; > + > + ret = clk_prepare(chip->clk); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to prepare clock: %d\n", ret); > + return ret; > + } > + > + ret = pwmchip_add(&chip->chip); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to add pwm chip: %d\n", ret); > + return ret; > + } > + > + platform_set_drvdata(pdev, chip); > + return 0; > +} > + > +static int pwm_clk_remove(struct platform_device *pdev) > +{ > + struct pwm_clk_chip *chip = platform_get_drvdata(pdev); > + > + clk_unprepare(chip->clk); > + > + pwmchip_remove(&chip->chip); This is bad. clk_unprepare() stops the output which must not happen before pwmchip_remove() returns. What happens if the PWM (and so the clk) is still on and you call clk_unprepare? Is this allowed at all if the enable count might be > 0? > + return 0; > +} > + > +static const struct of_device_id pwm_clk_dt_ids[] = { > + { .compatible = "clk-pwm", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, pwm_clk_dt_ids); > + > +static struct platform_driver pwm_clk_driver = { > + .driver = { > + .name = "clk-pwm", Hmm, is this name sane? I would have expected that a driver called "clk-pwm" exposes a clk using a PWM. OTOH there is a "pwm-clock" driver that does exactly that. To complete the confusion the function prefix of said driver is clk_pwm_ and this one used pwm_clk_ ... > + .of_match_table = pwm_clk_dt_ids, > + }, > + .probe = pwm_clk_probe, > + .remove = pwm_clk_remove, > +}; > +module_platform_driver(pwm_clk_driver); > + > +MODULE_ALIAS("platform:clk-pwm"); > +MODULE_AUTHOR("Nikita Travkin <nikita@trvn.ru>"); > +MODULE_DESCRIPTION("Clock based PWM driver"); > +MODULE_LICENSE("GPL v2"); MODULE_LICENSE("GPL"); is the more usual today (and has the same meaning). Best regards Uwe
Hi, Thanks for the review! Uwe Kleine-König писал(а) 10.12.2021 03:05: > Hello, > > On Thu, Dec 09, 2021 at 09:20:20PM +0500, Nikita Travkin wrote: >> +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip) >> + >> +static int pwm_clk_apply(struct pwm_chip *pwm_chip, struct pwm_device *pwm, >> + const struct pwm_state *state) >> +{ >> + struct pwm_clk_chip *chip = to_pwm_clk_chip(pwm_chip); >> + int ret; >> + u32 rate; >> + >> + if (!state->enabled && !pwm->state.enabled) >> + return 0; >> + >> + if (state->enabled && !pwm->state.enabled) { >> + ret = clk_enable(chip->clk); >> + if (ret) >> + return ret; >> + } >> + >> + if (!state->enabled && pwm->state.enabled) { >> + clk_disable(chip->clk); >> + return 0; >> + } > > This can be written a bit more compact as: > > if (!state->enabled) { > if (pwm->state.enabled) > clk_disable(chip->clk); > return 0; > } else if (!pwm->state.enabled) { > ret = clk_enable(chip->clk); > if (ret) > return ret; > } > > personally I find my version also easier to read, but that might be > subjective. > Having three discrete checks for three possible outcomes is a bit easier for me to understand, but I have no preference and can change it to your version. > Missing handling for polarity. Either refuse inverted polarity, or set > the duty_cycle to state->period - state->duty_cycle in the inverted > case. Will add the latter. > >> + rate = div64_u64(NSEC_PER_SEC, state->period); > > Please round up here, as .apply() should never implement a period bigger > than requested. This also automatically improves the behaviour if > state->period > NSEC_PER_SEC. Will do. I'm not sure if the underlying clock drivers guarantee the chosen rate to be rounded in line with that however, e.g. qcom SoC clocks that I target use lookup tables to find the closest rate with known M/N config values and set that. (So unless one makes sure the table has all the required rates, period is not guaranteed.) This is not an issue for my use cases though: Don't think any of the led or haptic motor controllers I've seen in my devices need a perfect rate. I think this is another line into the "Limitations" description that was suggested later. > >> + ret = clk_set_rate(chip->clk, rate); >> + if (ret) >> + return ret; >> + >> + return clk_set_duty_cycle(chip->clk, state->duty_cycle, state->period); > > Is it possible to enable only after the duty cycle is set? This way we > could prevent in some cases that a wrong setting makes it to the output. > Will move clk_enable() as the last action. After that it makes more sense to "squash" two leftover checks for disabled state so will do that as well. ... Except moving it caused a nice big WARNING from my clock controller... [ 76.353557] gcc_camss_gp1_clk status stuck at 'off' [ 76.353593] WARNING: CPU: 2 PID: 97 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x144/0x160 (...) [ 76.571531] Call trace: [ 76.578644] clk_branch_wait+0x144/0x160 [ 76.580903] clk_branch2_enable+0x34/0x44 [ 76.585069] clk_core_enable+0x6c/0xc0 [ 76.588974] clk_enable+0x30/0x50 [ 76.592620] pwm_clk_apply+0xb0/0xe4 [ 76.596007] pwm_apply_state+0x6c/0x1ec [ 76.599651] sgm3140_brightness_set+0xb4/0x190 [leds_sgm3140] (Which doesn't stop it from working afaict, but very much undesirable for me.) Unsure if this is something common or just a quirk of this specific driver but I'd rather take a little glitch on the output than make clock driver unhappy knowing how picky this hardware sometimes is... > As there is not a single function to set rate (i.e. period) and > duty_cycle it's not possible to prevent all glitches. > > Can you please note that in a paragraph at the beginning of the driver > as does e.g. drivers/pwm/pwm-sl28cpld.c. (Please stick to the format, > i.e. "Limitations:" and then all items without an empty line, to make > this greppable.) > Will add the description with limitations. >> +} >> + >> +static const struct pwm_ops pwm_clk_ops = { >> + .apply = pwm_clk_apply, >> + .owner = THIS_MODULE, >> +}; >> + >> +static int pwm_clk_probe(struct platform_device *pdev) >> +{ >> + struct pwm_clk_chip *chip; >> + int ret; >> + >> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); >> + if (!chip) >> + return -ENOMEM; >> + >> + chip->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(chip->clk)) { >> + dev_err(&pdev->dev, "Failed to get clock: %ld\n", PTR_ERR(chip->clk)); >> + return PTR_ERR(chip->clk); > > Please use dev_err_probe() here and in the other error paths below. > Will do. >> + } >> + >> + chip->chip.dev = &pdev->dev; >> + chip->chip.ops = &pwm_clk_ops; >> + chip->chip.of_xlate = of_pwm_xlate_with_flags; >> + chip->chip.of_pwm_n_cells = 2; >> + chip->chip.base = 0; > > Please drop this line (see commit f9a8ee8c8bcd) Will drop. > >> + chip->chip.npwm = 1; >> + >> + ret = clk_prepare(chip->clk); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "Failed to prepare clock: %d\n", ret); >> + return ret; >> + } >> + >> + ret = pwmchip_add(&chip->chip); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "Failed to add pwm chip: %d\n", ret); >> + return ret; >> + } >> + >> + platform_set_drvdata(pdev, chip); >> + return 0; >> +} >> + >> +static int pwm_clk_remove(struct platform_device *pdev) >> +{ >> + struct pwm_clk_chip *chip = platform_get_drvdata(pdev); >> + >> + clk_unprepare(chip->clk); >> + >> + pwmchip_remove(&chip->chip); > > This is bad. clk_unprepare() stops the output which must not happen > before pwmchip_remove() returns. What happens if the PWM (and so the > clk) is still on and you call clk_unprepare? Is this allowed at all if > the enable count might be > 0? > Will change the order. Also adding an clk_disable() there for cases when it's still enabled. >> + return 0; >> +} >> + >> +static const struct of_device_id pwm_clk_dt_ids[] = { >> + { .compatible = "clk-pwm", }, >> + { /* sentinel */ } >> +}; >> +MODULE_DEVICE_TABLE(of, pwm_clk_dt_ids); >> + >> +static struct platform_driver pwm_clk_driver = { >> + .driver = { >> + .name = "clk-pwm", > > Hmm, is this name sane? I would have expected that a driver called > "clk-pwm" exposes a clk using a PWM. OTOH there is a "pwm-clock" driver > that does exactly that. To complete the confusion the function prefix of > said driver is clk_pwm_ and this one used pwm_clk_ ... Oops, existence of "pwm-clock" (clk-pwm.c) and my attempts to not collide with it's naming causes quite a bit of weird mix-up's like this. .name should be "pwm-clk" in line with the driver filename and all the method prefixes there but compatible should still be "clk-pwm" so it's not as similar with an opposite "pwm-clock" compatible. > >> + .of_match_table = pwm_clk_dt_ids, >> + }, >> + .probe = pwm_clk_probe, >> + .remove = pwm_clk_remove, >> +}; >> +module_platform_driver(pwm_clk_driver); >> + >> +MODULE_ALIAS("platform:clk-pwm"); >> +MODULE_AUTHOR("Nikita Travkin <nikita@trvn.ru>"); >> +MODULE_DESCRIPTION("Clock based PWM driver"); >> +MODULE_LICENSE("GPL v2"); > > MODULE_LICENSE("GPL"); > > is the more usual today (and has the same meaning). Will use the more common one then. I will send a v2 with those changes (as well as with a filename fix for the DT bindings) a bit later. Thanks, Nikita > > Best regards > Uwe
Hello, On Fri, Dec 10, 2021 at 06:13:34PM +0500, Nikita Travkin wrote: > Uwe Kleine-König писал(а) 10.12.2021 03:05: > > Hello, > > > > On Thu, Dec 09, 2021 at 09:20:20PM +0500, Nikita Travkin wrote: > >> +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip) > >> + > >> +static int pwm_clk_apply(struct pwm_chip *pwm_chip, struct pwm_device *pwm, > >> + const struct pwm_state *state) > >> +{ > >> + struct pwm_clk_chip *chip = to_pwm_clk_chip(pwm_chip); > >> + int ret; > >> + u32 rate; > >> + > >> + if (!state->enabled && !pwm->state.enabled) > >> + return 0; > >> + > >> + if (state->enabled && !pwm->state.enabled) { > >> + ret = clk_enable(chip->clk); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> + if (!state->enabled && pwm->state.enabled) { > >> + clk_disable(chip->clk); > >> + return 0; > >> + } > > > > This can be written a bit more compact as: > > > > if (!state->enabled) { > > if (pwm->state.enabled) > > clk_disable(chip->clk); > > return 0; > > } else if (!pwm->state.enabled) { > > ret = clk_enable(chip->clk); > > if (ret) > > return ret; > > } > > > > personally I find my version also easier to read, but that might be > > subjective. > > Having three discrete checks for three possible outcomes is a bit > easier for me to understand, but I have no preference and can change > it to your version. > > > Missing handling for polarity. Either refuse inverted polarity, or set > > the duty_cycle to state->period - state->duty_cycle in the inverted > > case. > > Will add the latter. > > > > >> + rate = div64_u64(NSEC_PER_SEC, state->period); > > > > Please round up here, as .apply() should never implement a period bigger > > than requested. This also automatically improves the behaviour if > > state->period > NSEC_PER_SEC. > > Will do. I'm not sure if the underlying clock drivers guarantee the > chosen rate to be rounded in line with that however, e.g. qcom SoC This is a problem that most drivers have. Very few use clk_set_rate, but also clk_get_rate is subject to (much smaller) rounding issues. There doesn't seem to be an agreement that this is important enough to address. > clocks that I target use lookup tables to find the closest rate > with known M/N config values and set that. (So unless one makes sure > the table has all the required rates, period is not guaranteed.) > > This is not an issue for my use cases though: Don't think any > of the led or haptic motor controllers I've seen in my devices > need a perfect rate. > > I think this is another line into the "Limitations" description > that was suggested later. > > > > >> + ret = clk_set_rate(chip->clk, rate); > >> + if (ret) > >> + return ret; > >> + > >> + return clk_set_duty_cycle(chip->clk, state->duty_cycle, state->period); > > > > Is it possible to enable only after the duty cycle is set? This way we > > could prevent in some cases that a wrong setting makes it to the output. > > > > Will move clk_enable() as the last action. After that it makes more > sense to "squash" two leftover checks for disabled state so will do > that as well. > > ... Except moving it caused a nice big WARNING from my clock controller... > > [ 76.353557] gcc_camss_gp1_clk status stuck at 'off' > [ 76.353593] WARNING: CPU: 2 PID: 97 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x144/0x160 > (...) > [ 76.571531] Call trace: > [ 76.578644] clk_branch_wait+0x144/0x160 > [ 76.580903] clk_branch2_enable+0x34/0x44 > [ 76.585069] clk_core_enable+0x6c/0xc0 > [ 76.588974] clk_enable+0x30/0x50 > [ 76.592620] pwm_clk_apply+0xb0/0xe4 > [ 76.596007] pwm_apply_state+0x6c/0x1ec > [ 76.599651] sgm3140_brightness_set+0xb4/0x190 [leds_sgm3140] > > (Which doesn't stop it from working afaict, but very much undesirable > for me.) > Unsure if this is something common or just a quirk of this specific > driver but I'd rather take a little glitch on the output than > make clock driver unhappy knowing how picky this hardware sometimes is... I think clk_set_rate for a disabled clk isn't well defined. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 21e3b05a5153..daa2491a4054 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -140,6 +140,16 @@ config PWM_BRCMSTB To compile this driver as a module, choose M Here: the module will be called pwm-brcmstb.c. +config PWM_CLK + tristate "Clock based PWM support" + depends on HAVE_CLK || COMPILE_TEST + help + Generic PWM framework driver for outputs that can be + muxed to clocks. + + To compile this driver as a module, choose M here: the module + will be called pwm-clk. + config PWM_CLPS711X tristate "CLPS711X PWM support" depends on ARCH_CLPS711X || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 708840b7fba8..4a860103c470 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o obj-$(CONFIG_PWM_BERLIN) += pwm-berlin.o obj-$(CONFIG_PWM_BRCMSTB) += pwm-brcmstb.o +obj-$(CONFIG_PWM_CLK) += pwm-clk.o obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o obj-$(CONFIG_PWM_CRC) += pwm-crc.o obj-$(CONFIG_PWM_CROS_EC) += pwm-cros-ec.o diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c new file mode 100644 index 000000000000..11c156529761 --- /dev/null +++ b/drivers/pwm/pwm-clk.c @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> + +struct pwm_clk_chip { + struct pwm_chip chip; + struct clk *clk; +}; + +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip) + +static int pwm_clk_apply(struct pwm_chip *pwm_chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct pwm_clk_chip *chip = to_pwm_clk_chip(pwm_chip); + int ret; + u32 rate; + + if (!state->enabled && !pwm->state.enabled) + return 0; + + if (state->enabled && !pwm->state.enabled) { + ret = clk_enable(chip->clk); + if (ret) + return ret; + } + + if (!state->enabled && pwm->state.enabled) { + clk_disable(chip->clk); + return 0; + } + + rate = div64_u64(NSEC_PER_SEC, state->period); + ret = clk_set_rate(chip->clk, rate); + if (ret) + return ret; + + return clk_set_duty_cycle(chip->clk, state->duty_cycle, state->period); +} + +static const struct pwm_ops pwm_clk_ops = { + .apply = pwm_clk_apply, + .owner = THIS_MODULE, +}; + +static int pwm_clk_probe(struct platform_device *pdev) +{ + struct pwm_clk_chip *chip; + int ret; + + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + chip->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(chip->clk)) { + dev_err(&pdev->dev, "Failed to get clock: %ld\n", PTR_ERR(chip->clk)); + return PTR_ERR(chip->clk); + } + + chip->chip.dev = &pdev->dev; + chip->chip.ops = &pwm_clk_ops; + chip->chip.of_xlate = of_pwm_xlate_with_flags; + chip->chip.of_pwm_n_cells = 2; + chip->chip.base = 0; + chip->chip.npwm = 1; + + ret = clk_prepare(chip->clk); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to prepare clock: %d\n", ret); + return ret; + } + + ret = pwmchip_add(&chip->chip); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to add pwm chip: %d\n", ret); + return ret; + } + + platform_set_drvdata(pdev, chip); + return 0; +} + +static int pwm_clk_remove(struct platform_device *pdev) +{ + struct pwm_clk_chip *chip = platform_get_drvdata(pdev); + + clk_unprepare(chip->clk); + + pwmchip_remove(&chip->chip); + return 0; +} + +static const struct of_device_id pwm_clk_dt_ids[] = { + { .compatible = "clk-pwm", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, pwm_clk_dt_ids); + +static struct platform_driver pwm_clk_driver = { + .driver = { + .name = "clk-pwm", + .of_match_table = pwm_clk_dt_ids, + }, + .probe = pwm_clk_probe, + .remove = pwm_clk_remove, +}; +module_platform_driver(pwm_clk_driver); + +MODULE_ALIAS("platform:clk-pwm"); +MODULE_AUTHOR("Nikita Travkin <nikita@trvn.ru>"); +MODULE_DESCRIPTION("Clock based PWM driver"); +MODULE_LICENSE("GPL v2");
Some systems have clocks exposed to external devices. If the clock controller supports duty-cycle configuration, such clocks can be used as pwm outputs. In fact PWM and CLK subsystems are interfaced with in a similar way and an "opposite" driver already exists (clk-pwm). Add a driver that would enable pwm devices to be used via clk subsystem. Signed-off-by: Nikita Travkin <nikita@trvn.ru> --- drivers/pwm/Kconfig | 10 ++++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-clk.c | 119 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+) create mode 100644 drivers/pwm/pwm-clk.c