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 <[email protected]>
--
Changes in v2:
- Address Uwe's review comments:
- Round set clk rate up
- Add a description with limitations of the driver
- Disable and unprepare clock before removing pwmchip
Changes in v3:
- Use 64bit version of div round up
- Address Uwe's review comments:
- Reword the limitations to avoid incorrect claims
- Move the clk_enabled flag assignment
- Drop unnecessary statements
Changes in v5:
- add missed returns
Changes in v6:
- Unprepare the clock on error
- Drop redundant limitations points
Changes in v7:
- Rename some variables to be in line with common naming
--
It seems like my mailserver wasn't able to send the last review
response to Uwe's so I'll repeat here that afaict clk.h has all the
methods stubbed out so compiling without HAVE_CLK is possible.
Sorry for a long delay with sending this since v6.
---
drivers/pwm/Kconfig | 10 +++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-clk.c | 141 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 152 insertions(+)
create mode 100644 drivers/pwm/pwm-clk.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 904de8d61828..60d13a949bc5 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 5c08bdb817b4..7bf1a29f02b8 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..357d0c50dedd
--- /dev/null
+++ b/drivers/pwm/pwm-clk.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Clock based PWM controller
+ *
+ * Copyright (c) 2021 Nikita Travkin <[email protected]>
+ *
+ * This is an "adapter" driver that allows PWM consumers to use
+ * system clocks with duty cycle control as PWM outputs.
+ *
+ * Limitations:
+ * - Due to the fact that exact behavior depends on the underlying
+ * clock driver, various limitations are possible.
+ * - Underlying clock may not be able to give 0% or 100% duty cycle
+ * (constant off or on), exact behavior will depend on the clock.
+ * - When the PWM is disabled, the clock will be disabled as well,
+ * line state will depend on the clock.
+ * - The clk API doesn't expose the necessary calls to implement
+ * .get_state().
+ */
+
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/pwm.h>
+
+struct pwm_clk_chip {
+ struct pwm_chip chip;
+ struct clk *clk;
+ bool clk_enabled;
+};
+
+#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip)
+
+static int pwm_clk_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct pwm_clk_chip *pcchip = to_pwm_clk_chip(chip);
+ int ret;
+ u32 rate;
+ u64 period = state->period;
+ u64 duty_cycle = state->duty_cycle;
+
+ if (!state->enabled) {
+ if (pwm->state.enabled) {
+ clk_disable(pcchip->clk);
+ pcchip->clk_enabled = false;
+ }
+ return 0;
+ } else if (!pwm->state.enabled) {
+ ret = clk_enable(pcchip->clk);
+ if (ret)
+ return ret;
+ pcchip->clk_enabled = true;
+ }
+
+ rate = DIV64_U64_ROUND_UP(NSEC_PER_SEC, period);
+ ret = clk_set_rate(pcchip->clk, rate);
+ if (ret)
+ return ret;
+
+ if (state->polarity == PWM_POLARITY_INVERSED)
+ duty_cycle = period - duty_cycle;
+
+ return clk_set_duty_cycle(pcchip->clk, duty_cycle, 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 *pcchip;
+ int ret;
+
+ pcchip = devm_kzalloc(&pdev->dev, sizeof(*pcchip), GFP_KERNEL);
+ if (!pcchip)
+ return -ENOMEM;
+
+ pcchip->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pcchip->clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(pcchip->clk),
+ "Failed to get clock\n");
+
+ pcchip->chip.dev = &pdev->dev;
+ pcchip->chip.ops = &pwm_clk_ops;
+ pcchip->chip.npwm = 1;
+
+ ret = clk_prepare(pcchip->clk);
+ if (ret < 0)
+ return dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
+
+ ret = pwmchip_add(&pcchip->chip);
+ if (ret < 0) {
+ clk_unprepare(pcchip->clk);
+ return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
+ }
+
+ platform_set_drvdata(pdev, pcchip);
+ return 0;
+}
+
+static int pwm_clk_remove(struct platform_device *pdev)
+{
+ struct pwm_clk_chip *pcchip = platform_get_drvdata(pdev);
+
+ pwmchip_remove(&pcchip->chip);
+
+ if (pcchip->clk_enabled)
+ clk_disable(pcchip->clk);
+
+ clk_unprepare(pcchip->clk);
+
+ 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 = "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:pwm-clk");
+MODULE_AUTHOR("Nikita Travkin <[email protected]>");
+MODULE_DESCRIPTION("Clock based PWM driver");
+MODULE_LICENSE("GPL");
--
2.35.3
Hello,
On Sun, Jun 12, 2022 at 06:22:03PM +0500, Nikita Travkin wrote:
> 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 <[email protected]>
> --
>
> Changes in v2:
> - Address Uwe's review comments:
> - Round set clk rate up
> - Add a description with limitations of the driver
> - Disable and unprepare clock before removing pwmchip
> Changes in v3:
> - Use 64bit version of div round up
> - Address Uwe's review comments:
> - Reword the limitations to avoid incorrect claims
> - Move the clk_enabled flag assignment
> - Drop unnecessary statements
> Changes in v5:
> - add missed returns
> Changes in v6:
> - Unprepare the clock on error
> - Drop redundant limitations points
> Changes in v7:
> - Rename some variables to be in line with common naming
>
> --
> It seems like my mailserver wasn't able to send the last review
> response to Uwe's so I'll repeat here that afaict clk.h has all the
> methods stubbed out so compiling without HAVE_CLK is possible.
> Sorry for a long delay with sending this since v6.
>
> ---
> drivers/pwm/Kconfig | 10 +++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-clk.c | 141 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 152 insertions(+)
> create mode 100644 drivers/pwm/pwm-clk.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 904de8d61828..60d13a949bc5 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 5c08bdb817b4..7bf1a29f02b8 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..357d0c50dedd
> --- /dev/null
> +++ b/drivers/pwm/pwm-clk.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Clock based PWM controller
> + *
> + * Copyright (c) 2021 Nikita Travkin <[email protected]>
> + *
> + * This is an "adapter" driver that allows PWM consumers to use
> + * system clocks with duty cycle control as PWM outputs.
> + *
> + * Limitations:
> + * - Due to the fact that exact behavior depends on the underlying
> + * clock driver, various limitations are possible.
> + * - Underlying clock may not be able to give 0% or 100% duty cycle
> + * (constant off or on), exact behavior will depend on the clock.
> + * - When the PWM is disabled, the clock will be disabled as well,
> + * line state will depend on the clock.
> + * - The clk API doesn't expose the necessary calls to implement
> + * .get_state().
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/pwm.h>
> +
> +struct pwm_clk_chip {
> + struct pwm_chip chip;
> + struct clk *clk;
> + bool clk_enabled;
> +};
> +
> +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip)
> +
> +static int pwm_clk_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct pwm_clk_chip *pcchip = to_pwm_clk_chip(chip);
> + int ret;
> + u32 rate;
> + u64 period = state->period;
> + u64 duty_cycle = state->duty_cycle;
> +
> + if (!state->enabled) {
> + if (pwm->state.enabled) {
> + clk_disable(pcchip->clk);
> + pcchip->clk_enabled = false;
> + }
> + return 0;
> + } else if (!pwm->state.enabled) {
> + ret = clk_enable(pcchip->clk);
> + if (ret)
> + return ret;
> + pcchip->clk_enabled = true;
> + }
Maybe point out here that this introduces a glitch that cannot be
prevented. Something like:
/*
* We have to enable the clk before setting the rate and
* duty_cycle, that however results in a window where the clk is
* on with a (potentially) different setting. Also setting
* period and duty_cycle are two separate calls, so that
* probably isn't atomic either.
*/
> + rate = DIV64_U64_ROUND_UP(NSEC_PER_SEC, period);
> + ret = clk_set_rate(pcchip->clk, rate);
> + if (ret)
> + return ret;
> +
> + if (state->polarity == PWM_POLARITY_INVERSED)
> + duty_cycle = period - duty_cycle;
> +
> + return clk_set_duty_cycle(pcchip->clk, duty_cycle, 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 *pcchip;
> + int ret;
> +
> + pcchip = devm_kzalloc(&pdev->dev, sizeof(*pcchip), GFP_KERNEL);
> + if (!pcchip)
> + return -ENOMEM;
> +
> + pcchip->clk = devm_clk_get(&pdev->dev, NULL);
You can use devm_clk_get_prepared() here and drop the clk_prepare()
below and the clk_unprepare in .remove().
> + if (IS_ERR(pcchip->clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(pcchip->clk),
> + "Failed to get clock\n");
> +
> + pcchip->chip.dev = &pdev->dev;
> + pcchip->chip.ops = &pwm_clk_ops;
> + pcchip->chip.npwm = 1;
> +
> + ret = clk_prepare(pcchip->clk);
> + if (ret < 0)
> + return dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
> +
> + ret = pwmchip_add(&pcchip->chip);
> + if (ret < 0) {
> + clk_unprepare(pcchip->clk);
> + return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
> + }
> +
> + platform_set_drvdata(pdev, pcchip);
> + return 0;
> +}
> +
> +static int pwm_clk_remove(struct platform_device *pdev)
> +{
> + struct pwm_clk_chip *pcchip = platform_get_drvdata(pdev);
> +
> + pwmchip_remove(&pcchip->chip);
> +
> + if (pcchip->clk_enabled)
> + clk_disable(pcchip->clk);
> +
> + clk_unprepare(pcchip->clk);
> +
> + 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 = "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:pwm-clk");
> +MODULE_AUTHOR("Nikita Travkin <[email protected]>");
> +MODULE_DESCRIPTION("Clock based PWM driver");
> +MODULE_LICENSE("GPL");
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi,
Uwe Kleine-König писал(а) 01.07.2022 12:50:
> Hello,
>
> On Sun, Jun 12, 2022 at 06:22:03PM +0500, Nikita Travkin wrote:
>> 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 <[email protected]>
>> --
>>
>> Changes in v2:
>> - Address Uwe's review comments:
>> - Round set clk rate up
>> - Add a description with limitations of the driver
>> - Disable and unprepare clock before removing pwmchip
>> Changes in v3:
>> - Use 64bit version of div round up
>> - Address Uwe's review comments:
>> - Reword the limitations to avoid incorrect claims
>> - Move the clk_enabled flag assignment
>> - Drop unnecessary statements
>> Changes in v5:
>> - add missed returns
>> Changes in v6:
>> - Unprepare the clock on error
>> - Drop redundant limitations points
>> Changes in v7:
>> - Rename some variables to be in line with common naming
>>
>> --
>> It seems like my mailserver wasn't able to send the last review
>> response to Uwe's so I'll repeat here that afaict clk.h has all the
>> methods stubbed out so compiling without HAVE_CLK is possible.
>> Sorry for a long delay with sending this since v6.
>>
>> ---
>> drivers/pwm/Kconfig | 10 +++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-clk.c | 141 ++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 152 insertions(+)
>> create mode 100644 drivers/pwm/pwm-clk.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 904de8d61828..60d13a949bc5 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 5c08bdb817b4..7bf1a29f02b8 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..357d0c50dedd
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-clk.c
>> @@ -0,0 +1,141 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Clock based PWM controller
>> + *
>> + * Copyright (c) 2021 Nikita Travkin <[email protected]>
>> + *
>> + * This is an "adapter" driver that allows PWM consumers to use
>> + * system clocks with duty cycle control as PWM outputs.
>> + *
>> + * Limitations:
>> + * - Due to the fact that exact behavior depends on the underlying
>> + * clock driver, various limitations are possible.
>> + * - Underlying clock may not be able to give 0% or 100% duty cycle
>> + * (constant off or on), exact behavior will depend on the clock.
>> + * - When the PWM is disabled, the clock will be disabled as well,
>> + * line state will depend on the clock.
>> + * - The clk API doesn't expose the necessary calls to implement
>> + * .get_state().
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/math64.h>
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/pwm.h>
>> +
>> +struct pwm_clk_chip {
>> + struct pwm_chip chip;
>> + struct clk *clk;
>> + bool clk_enabled;
>> +};
>> +
>> +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip)
>> +
>> +static int pwm_clk_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> + struct pwm_clk_chip *pcchip = to_pwm_clk_chip(chip);
>> + int ret;
>> + u32 rate;
>> + u64 period = state->period;
>> + u64 duty_cycle = state->duty_cycle;
>> +
>> + if (!state->enabled) {
>> + if (pwm->state.enabled) {
>> + clk_disable(pcchip->clk);
>> + pcchip->clk_enabled = false;
>> + }
>> + return 0;
>> + } else if (!pwm->state.enabled) {
>> + ret = clk_enable(pcchip->clk);
>> + if (ret)
>> + return ret;
>> + pcchip->clk_enabled = true;
>> + }
>
> Maybe point out here that this introduces a glitch that cannot be
> prevented. Something like:
>
> /*
> * We have to enable the clk before setting the rate and
> * duty_cycle, that however results in a window where the clk is
> * on with a (potentially) different setting. Also setting
> * period and duty_cycle are two separate calls, so that
> * probably isn't atomic either.
> */
>
Thanks for the suggestion! Will add.
>> + rate = DIV64_U64_ROUND_UP(NSEC_PER_SEC, period);
>> + ret = clk_set_rate(pcchip->clk, rate);
>> + if (ret)
>> + return ret;
>> +
>> + if (state->polarity == PWM_POLARITY_INVERSED)
>> + duty_cycle = period - duty_cycle;
>> +
>> + return clk_set_duty_cycle(pcchip->clk, duty_cycle, 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 *pcchip;
>> + int ret;
>> +
>> + pcchip = devm_kzalloc(&pdev->dev, sizeof(*pcchip), GFP_KERNEL);
>> + if (!pcchip)
>> + return -ENOMEM;
>> +
>> + pcchip->clk = devm_clk_get(&pdev->dev, NULL);
>
> You can use devm_clk_get_prepared() here and drop the clk_prepare()
> below and the clk_unprepare in .remove().
>
Here I spent a bit of time trying to remember why I thought
I've already looked at this, but after figuring out that this
devm helper didn't even exist earlier (I only see it in clk-next)
I remembered considering a totally different thing (being
clk_disable_unprepare in the _remove, which doesn't play well)
Given that this seems to be absent from 5.19-rc6, I'm afraid adding
it here will upset the 0day as well as possibly cause issues in case
both are taken for the same merge window...
On the other hand it takes me quite a while to provide replies for
this series (the trend I'm not happy with) so maybe 3-4 weeks
will indeed pass for 5.20-rc1 to have it...
I think I will try to send a new version with just the comment
added shortly in case it's still not too late for the next merge
window and you can feel free to nack it if you think it already is :)
Thanks for your review,
Nikita
>> + if (IS_ERR(pcchip->clk))
>> + return dev_err_probe(&pdev->dev, PTR_ERR(pcchip->clk),
>> + "Failed to get clock\n");
>> +
>> + pcchip->chip.dev = &pdev->dev;
>> + pcchip->chip.ops = &pwm_clk_ops;
>> + pcchip->chip.npwm = 1;
>> +
>> + ret = clk_prepare(pcchip->clk);
>> + if (ret < 0)
>> + return dev_err_probe(&pdev->dev, ret, "Failed to prepare clock\n");
>> +
>> + ret = pwmchip_add(&pcchip->chip);
>> + if (ret < 0) {
>> + clk_unprepare(pcchip->clk);
>> + return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
>> + }
>> +
>> + platform_set_drvdata(pdev, pcchip);
>> + return 0;
>> +}
>> +
>> +static int pwm_clk_remove(struct platform_device *pdev)
>> +{
>> + struct pwm_clk_chip *pcchip = platform_get_drvdata(pdev);
>> +
>> + pwmchip_remove(&pcchip->chip);
>> +
>> + if (pcchip->clk_enabled)
>> + clk_disable(pcchip->clk);
>> +
>> + clk_unprepare(pcchip->clk);
>> +
>> + 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 = "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:pwm-clk");
>> +MODULE_AUTHOR("Nikita Travkin <[email protected]>");
>> +MODULE_DESCRIPTION("Clock based PWM driver");
>> +MODULE_LICENSE("GPL");
>
> Best regards
> Uwe
On Mon, Jul 11, 2022 at 10:55:09AM +0500, Nikita Travkin wrote:
> Uwe Kleine-König писал(а) 01.07.2022 12:50:
> > On Sun, Jun 12, 2022 at 06:22:03PM +0500, Nikita Travkin wrote:
> >> 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 <[email protected]>
> >> --
> >>
> >> Changes in v2:
> >> - Address Uwe's review comments:
> >> - Round set clk rate up
> >> - Add a description with limitations of the driver
> >> - Disable and unprepare clock before removing pwmchip
> >> Changes in v3:
> >> - Use 64bit version of div round up
> >> - Address Uwe's review comments:
> >> - Reword the limitations to avoid incorrect claims
> >> - Move the clk_enabled flag assignment
> >> - Drop unnecessary statements
> >> Changes in v5:
> >> - add missed returns
> >> Changes in v6:
> >> - Unprepare the clock on error
> >> - Drop redundant limitations points
> >> Changes in v7:
> >> - Rename some variables to be in line with common naming
> >>
> >> --
> >> It seems like my mailserver wasn't able to send the last review
> >> response to Uwe's so I'll repeat here that afaict clk.h has all the
> >> methods stubbed out so compiling without HAVE_CLK is possible.
> >> Sorry for a long delay with sending this since v6.
FTR: The only problems I have with mail sending in this thread is to
"[email protected]", I added a 'g' for this mail to that address
:-) Otherwise if you diagnose to have problems with the pengutronix
server accepting your mail, I'd like to hear about that.
> >> + pcchip = devm_kzalloc(&pdev->dev, sizeof(*pcchip), GFP_KERNEL);
> >> + if (!pcchip)
> >> + return -ENOMEM;
> >> +
> >> + pcchip->clk = devm_clk_get(&pdev->dev, NULL);
> >
> > You can use devm_clk_get_prepared() here and drop the clk_prepare()
> > below and the clk_unprepare in .remove().
>
> Here I spent a bit of time trying to remember why I thought
> I've already looked at this, but after figuring out that this
> devm helper didn't even exist earlier (I only see it in clk-next)
> I remembered considering a totally different thing (being
> clk_disable_unprepare in the _remove, which doesn't play well)
>
> Given that this seems to be absent from 5.19-rc6, I'm afraid adding
> it here will upset the 0day as well as possibly cause issues in case
> both are taken for the same merge window...
Pass --base with a sensible parameter to git-format-patch (or
git-send-email) to make the 0day bots happy.
> On the other hand it takes me quite a while to provide replies for
> this series (the trend I'm not happy with) so maybe 3-4 weeks
> will indeed pass for 5.20-rc1 to have it...
It's not me who merges PWM patches but Thierry. I don't know his plans
and if he would be willing to pick up a new driver for the next cycle.
You might still get lucky with a fast next iteration.
If you want ignore the devm_clk_get_prepared() suggestion, we can still
convert to that once both patches hit Linus Torvald's tree.
> I think I will try to send a new version with just the comment
> added shortly in case it's still not too late for the next merge
> window and you can feel free to nack it if you think it already is :)
I think the driver looks good otherwise, so I don't expect to have more
feedback in the next round.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Uwe Kleine-König писал(а) 11.07.2022 12:03:
> On Mon, Jul 11, 2022 at 10:55:09AM +0500, Nikita Travkin wrote:
>> Uwe Kleine-König писал(а) 01.07.2022 12:50:
>> > On Sun, Jun 12, 2022 at 06:22:03PM +0500, Nikita Travkin wrote:
>> >> 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 <[email protected]>
>> >> --
>> >>
>> >> Changes in v2:
>> >> - Address Uwe's review comments:
>> >> - Round set clk rate up
>> >> - Add a description with limitations of the driver
>> >> - Disable and unprepare clock before removing pwmchip
>> >> Changes in v3:
>> >> - Use 64bit version of div round up
>> >> - Address Uwe's review comments:
>> >> - Reword the limitations to avoid incorrect claims
>> >> - Move the clk_enabled flag assignment
>> >> - Drop unnecessary statements
>> >> Changes in v5:
>> >> - add missed returns
>> >> Changes in v6:
>> >> - Unprepare the clock on error
>> >> - Drop redundant limitations points
>> >> Changes in v7:
>> >> - Rename some variables to be in line with common naming
>> >>
>> >> --
>> >> It seems like my mailserver wasn't able to send the last review
>> >> response to Uwe's so I'll repeat here that afaict clk.h has all the
>> >> methods stubbed out so compiling without HAVE_CLK is possible.
>> >> Sorry for a long delay with sending this since v6.
>
> FTR: The only problems I have with mail sending in this thread is to
> "[email protected]", I added a 'g' for this mail to that address
> :-) Otherwise if you diagnose to have problems with the pengutronix
> server accepting your mail, I'd like to hear about that.
I think the problem for me was that my server, for some reason,
insisted on using ipv6 which it has no access to. I think it might
just be some (very) temporary problem on my end as now the mails go
fine again.
And the missing "g" is just the artifact of me preferring to manually
build the git send-email command every time, then forgetting I made
this typo when replying...
>
>> >> + pcchip = devm_kzalloc(&pdev->dev, sizeof(*pcchip), GFP_KERNEL);
>> >> + if (!pcchip)
>> >> + return -ENOMEM;
>> >> +
>> >> + pcchip->clk = devm_clk_get(&pdev->dev, NULL);
>> >
>> > You can use devm_clk_get_prepared() here and drop the clk_prepare()
>> > below and the clk_unprepare in .remove().
>>
>> Here I spent a bit of time trying to remember why I thought
>> I've already looked at this, but after figuring out that this
>> devm helper didn't even exist earlier (I only see it in clk-next)
>> I remembered considering a totally different thing (being
>> clk_disable_unprepare in the _remove, which doesn't play well)
>>
>> Given that this seems to be absent from 5.19-rc6, I'm afraid adding
>> it here will upset the 0day as well as possibly cause issues in case
>> both are taken for the same merge window...
>
> Pass --base with a sensible parameter to git-format-patch (or
> git-send-email) to make the 0day bots happy.
Thanks for the suggestion!
>
>> On the other hand it takes me quite a while to provide replies for
>> this series (the trend I'm not happy with) so maybe 3-4 weeks
>> will indeed pass for 5.20-rc1 to have it...
>
> It's not me who merges PWM patches but Thierry. I don't know his plans
> and if he would be willing to pick up a new driver for the next cycle.
> You might still get lucky with a fast next iteration.
>
> If you want ignore the devm_clk_get_prepared() suggestion, we can still
> convert to that once both patches hit Linus Torvald's tree.
Yes, I decided to immediately send a v8 instead of giving a bit
of time for the discussion to settle (which seem to often end up
too long) as there was no functional change otherwise.
Thanks,
Nikita
>
>> I think I will try to send a new version with just the comment
>> added shortly in case it's still not too late for the next merge
>> window and you can feel free to nack it if you think it already is :)
>
> I think the driver looks good otherwise, so I don't expect to have more
> feedback in the next round.
>
> Best regards
> Uwe