* [PATCH v6 2/3] pwm: add Andes PWM driver support
@ 2026-06-25 10:36 ` Ben Zong-You Xie via B4 Relay
0 siblings, 0 replies; 13+ messages in thread
From: Ben Zong-You Xie via B4 Relay @ 2026-06-25 10:36 UTC (permalink / raw)
To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-pwm, devicetree, linux-kernel, Ben Zong-You Xie
From: Ben Zong-You Xie <ben717@andestech.com>
Add a driver for the PWM controller found in Andes AE350 platforms and
QiLai SoCs.
The Andes PWM controller features:
- 4 independent channels.
- Dual clock source support (APB clock and external clock) to provide
a flexible range of frequencies.
- Support for normal and inversed polarity.
The driver implements the .apply() and .get_state() callbacks. Since the
clock source of each channel can be selected by programming the
register, clock selection logic is implemented to prioritize the
external clock to maximize the supported period range, falling back to
the APB clock for higher frequency requirements.
Signed-off-by: Ben Zong-You Xie <ben717@andestech.com>
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-andes.c | 343 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 354 insertions(+)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e8886a9b64d9..52dee4b7f081 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -73,6 +73,16 @@ config PWM_AIROHA
To compile this driver as a module, choose M here: the module
will be called pwm-airoha.
+config PWM_ANDES
+ tristate "Andes PWM support"
+ depends on ARCH_ANDES || COMPILE_TEST
+ help
+ Generic PWM framework driver for Andes platform, such as QiLai SoC
+ and AE350 platform.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-andes.
+
config PWM_APPLE
tristate "Apple SoC PWM support"
depends on ARCH_APPLE || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 5630a521a7cf..c92369ee251d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_PWM) += core.o
obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
obj-$(CONFIG_PWM_ADP5585) += pwm-adp5585.o
obj-$(CONFIG_PWM_AIROHA) += pwm-airoha.o
+obj-$(CONFIG_PWM_ANDES) += pwm-andes.o
obj-$(CONFIG_PWM_APPLE) += pwm-apple.o
obj-$(CONFIG_PWM_ARGON_FAN_HAT) += pwm-argon-fan-hat.o
obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
diff --git a/drivers/pwm/pwm-andes.c b/drivers/pwm/pwm-andes.c
new file mode 100644
index 000000000000..580e673d2cff
--- /dev/null
+++ b/drivers/pwm/pwm-andes.c
@@ -0,0 +1,343 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Andes PWM, used in Andes AE350 platform and QiLai SoC
+ *
+ * Copyright (C) 2026 Andes Technology Corporation.
+ *
+ * Limitations:
+ * - When disabling a channel, the current period is not completed and the
+ * output is driven to the PARK level (low when ANDES_PWM_CH_CTRL_PARK is
+ * clear, high when it is set).
+ * - The current period will be completed first if reconfiguring.
+ * - Further, if the reconfiguration changes the clock source, the output will
+ * not be the old one nor the new one. And the output will be the new one
+ * after writing to the reload register.
+ * - The hardware cannot run a 0% or 100% relative duty cycle; the driver
+ * emulates these by disabling the channel and parking the output at the
+ * constant level.
+ * - A period or duty cycle larger than the selected clock can represent is
+ * rounded down to the largest achievable value rather than rejected.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#define ANDES_PWM_CH_ENABLE 0x1C
+#define ANDES_PWM_CH_ENABLE_PWM(ch) BIT(3 + (4 * (ch)))
+
+#define ANDES_PWM_CH_CTRL(ch) (0x20 + (0x10 * (ch)))
+#define ANDES_PWM_CH_CTRL_MODE_PWM BIT(2)
+#define ANDES_PWM_CH_CTRL_CLK BIT(3)
+#define ANDES_PWM_CH_CTRL_PARK BIT(4)
+#define ANDES_PWM_CH_CTRL_MASK GENMASK(4, 0)
+
+#define ANDES_PWM_CH_RELOAD(ch) (0x24 + (0x10 * (ch)))
+#define ANDES_PWM_CH_RELOAD_HIGH GENMASK(31, 16)
+#define ANDES_PWM_CH_RELOAD_LOW GENMASK(15, 0)
+
+#define ANDES_PWM_CH_COUNTER(ch) (0x28 + (0x10 * (ch)))
+
+#define ANDES_PWM_CH_MAX 4
+#define ANDES_PWM_CYCLE_MIN 1
+#define ANDES_PWM_CYCLE_MAX 0x10000
+
+struct andes_pwm {
+ struct regmap *regmap;
+ struct clk *pclk;
+ struct clk *extclk;
+ unsigned int pclk_rate;
+ unsigned int extclk_rate;
+};
+
+static const struct regmap_config andes_pwm_regmap_config = {
+ .name = "andes_pwm",
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .pad_bits = 0,
+ .max_register = ANDES_PWM_CH_COUNTER(ANDES_PWM_CH_MAX - 1),
+ .cache_type = REGCACHE_NONE,
+};
+
+static inline struct andes_pwm *andes_pwm_from_chip(struct pwm_chip *chip)
+{
+ return pwmchip_get_drvdata(chip);
+}
+
+static int andes_pwm_enable(struct pwm_chip *chip, unsigned int channel,
+ bool enable)
+{
+ struct andes_pwm *ap = andes_pwm_from_chip(chip);
+
+ return regmap_assign_bits(ap->regmap, ANDES_PWM_CH_ENABLE,
+ ANDES_PWM_CH_ENABLE_PWM(channel), enable);
+}
+
+/*
+ * Hold the output at a constant level by parking the disabled channel. A
+ * disabled channel drives its output to the PARK level (low when @high is
+ * false, high when @high is true), which is used to emulate a 0% or 100%
+ * relative duty cycle.
+ */
+static int andes_pwm_park(struct pwm_chip *chip, unsigned int channel,
+ bool high)
+{
+ struct andes_pwm *ap = andes_pwm_from_chip(chip);
+
+ regmap_assign_bits(ap->regmap, ANDES_PWM_CH_CTRL(channel),
+ ANDES_PWM_CH_CTRL_PARK, high);
+
+ return andes_pwm_enable(chip, channel, false);
+}
+
+static int andes_pwm_config(struct pwm_chip *chip, unsigned int channel,
+ const struct pwm_state *state)
+{
+ struct andes_pwm *ap = andes_pwm_from_chip(chip);
+ unsigned int clk_rate = ap->extclk_rate;
+ unsigned int ctrl = ANDES_PWM_CH_CTRL_MODE_PWM;
+ bool use_pclk = false;
+ u64 high_cycles;
+ u64 low_cycles;
+ u64 period_cycles;
+ u64 duty_cycles;
+ u32 reload;
+
+ /*
+ * Reload register for PWM mode:
+ *
+ * 31 : 16 15 : 0
+ * PWM16_Hi | PWM16_Lo
+ *
+ * The high duration is (PWM16_Hi + 1) cycles and the low duration is
+ * (PWM16_Lo + 1) cycles, so each phase spans ANDES_PWM_CYCLE_MIN to
+ * ANDES_PWM_CYCLE_MAX cycles. The hardware period (their sum) can reach
+ * 2 * ANDES_PWM_CYCLE_MAX cycles, but the PWM core requires the period
+ * to be chosen from the requested period alone, independent of the duty
+ * cycle. That holds only while both phases stay within
+ * ANDES_PWM_CYCLE_MAX for every duty split, so the usable period is
+ * capped at ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN cycles.
+ *
+ * The controller has two clock sources, the APB clock and an external
+ * clock. Since the external clock frequency must be slower than the APB
+ * clock, it is tried first for its wider period range; the APB clock is
+ * used only when the external clock is too fast to represent the period
+ * (it resolves fewer than two cycles) or is absent.
+ */
+ period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
+ NSEC_PER_SEC);
+ if (period_cycles < 2 * ANDES_PWM_CYCLE_MIN) {
+ use_pclk = true;
+ clk_rate = ap->pclk_rate;
+ period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
+ NSEC_PER_SEC);
+ if (period_cycles < 2 * ANDES_PWM_CYCLE_MIN)
+ return -EINVAL;
+ }
+
+ /*
+ * Round the period down to the largest value representable for every
+ * duty cycle, so the chosen period depends on the requested period
+ * alone. With both phases capped at ANDES_PWM_CYCLE_MAX, that bound is
+ * ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN cycles.
+ */
+ period_cycles = min_t(u64, period_cycles,
+ ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN);
+
+ /* The duty cycle cannot exceed the (possibly clamped) period. */
+ duty_cycles = mul_u64_u64_div_u64(clk_rate, state->duty_cycle,
+ NSEC_PER_SEC);
+ duty_cycles = min_t(u64, duty_cycles, period_cycles);
+ if (state->polarity == PWM_POLARITY_INVERSED) {
+ low_cycles = duty_cycles;
+ high_cycles = period_cycles - low_cycles;
+ } else {
+ high_cycles = duty_cycles;
+ low_cycles = period_cycles - high_cycles;
+ }
+
+ /*
+ * A zero-length phase means a 0% or 100% relative duty cycle, which the
+ * hardware cannot run. Emit the matching constant level by parking the
+ * channel: high_cycles == 0 stays low, low_cycles == 0 stays high.
+ */
+ if (!high_cycles)
+ return andes_pwm_park(chip, channel, false);
+ if (!low_cycles)
+ return andes_pwm_park(chip, channel, true);
+
+ /*
+ * If changing the clock source here, the output will not be the old one
+ * nor the new one. And the output will be the new one after writing to
+ * the reload register.
+ */
+ ctrl |= use_pclk ? ANDES_PWM_CH_CTRL_CLK : 0;
+ ctrl |= (state->polarity == PWM_POLARITY_INVERSED) ?
+ ANDES_PWM_CH_CTRL_PARK : 0;
+ regmap_update_bits(ap->regmap, ANDES_PWM_CH_CTRL(channel),
+ ANDES_PWM_CH_CTRL_MASK, ctrl);
+ reload = FIELD_PREP(ANDES_PWM_CH_RELOAD_HIGH, high_cycles - 1) |
+ FIELD_PREP(ANDES_PWM_CH_RELOAD_LOW, low_cycles - 1);
+ regmap_write(ap->regmap, ANDES_PWM_CH_RELOAD(channel), reload);
+ return andes_pwm_enable(chip, channel, true);
+}
+
+static int andes_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ unsigned int channel = pwm->hwpwm;
+
+ if (!state->enabled) {
+ if (pwm->state.enabled)
+ andes_pwm_enable(chip, channel, false);
+
+ return 0;
+ }
+
+ return andes_pwm_config(chip, channel, state);
+}
+
+static int andes_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct andes_pwm *ap = andes_pwm_from_chip(chip);
+ unsigned int channel = pwm->hwpwm;
+ unsigned int ctrl;
+ unsigned int clk_rate;
+ unsigned int reload;
+ u64 high_cycles;
+ u64 low_cycles;
+
+ regmap_read(ap->regmap, ANDES_PWM_CH_CTRL(channel), &ctrl);
+ clk_rate = FIELD_GET(ANDES_PWM_CH_CTRL_CLK, ctrl) ? ap->pclk_rate
+ : ap->extclk_rate;
+ if (!clk_rate) {
+ /*
+ * The selected clock source is unavailable, so the channel
+ * cannot be running; report it as disabled and avoid the
+ * division by zero below.
+ */
+ state->enabled = false;
+ state->period = 0;
+ state->duty_cycle = 0;
+ return 0;
+ }
+
+ state->enabled = regmap_test_bits(ap->regmap, ANDES_PWM_CH_ENABLE,
+ ANDES_PWM_CH_ENABLE_PWM(channel)) > 0;
+ state->polarity = FIELD_GET(ANDES_PWM_CH_CTRL_PARK, ctrl) ?
+ PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+ regmap_read(ap->regmap, ANDES_PWM_CH_RELOAD(channel), &reload);
+ high_cycles = FIELD_GET(ANDES_PWM_CH_RELOAD_HIGH, reload) + 1;
+ low_cycles = FIELD_GET(ANDES_PWM_CH_RELOAD_LOW, reload) + 1;
+
+ /*
+ * high_cycles and low_cycles are each at most ANDES_PWM_CYCLE_MAX
+ * (0x10000, 17 bits) and NSEC_PER_SEC is below 2^30, so the products
+ * below are safe from 64-bit overflow.
+ */
+ if (state->polarity == PWM_POLARITY_INVERSED)
+ state->duty_cycle = DIV_ROUND_UP_ULL(low_cycles * NSEC_PER_SEC,
+ clk_rate);
+ else
+ state->duty_cycle = DIV_ROUND_UP_ULL(high_cycles * NSEC_PER_SEC,
+ clk_rate);
+
+ state->period = DIV_ROUND_UP_ULL((high_cycles + low_cycles) *
+ NSEC_PER_SEC, clk_rate);
+
+ return 0;
+}
+
+static const struct pwm_ops andes_pwm_ops = {
+ .apply = andes_pwm_apply,
+ .get_state = andes_pwm_get_state,
+};
+
+static int andes_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pwm_chip *chip;
+ struct andes_pwm *ap;
+ void __iomem *reg_base;
+ unsigned long pclk_rate;
+ unsigned long extclk_rate;
+ int ret;
+
+ chip = devm_pwmchip_alloc(dev, ANDES_PWM_CH_MAX, sizeof(*ap));
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
+
+ ap = andes_pwm_from_chip(chip);
+ reg_base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(reg_base))
+ return dev_err_probe(dev, PTR_ERR(reg_base),
+ "Failed to map I/O space\n");
+
+ ap->pclk = devm_clk_get_enabled(dev, "pclk");
+ if (IS_ERR(ap->pclk))
+ return dev_err_probe(dev, PTR_ERR(ap->pclk),
+ "Failed to get APB clock\n");
+
+ ap->extclk = devm_clk_get_optional_enabled(dev, "extclk");
+ if (IS_ERR(ap->extclk))
+ return dev_err_probe(dev, PTR_ERR(ap->extclk),
+ "Failed to get external clock\n");
+
+ /*
+ * If the clock rate is greater than 10^9, there may be an overflow when
+ * calculating the cycles in andes_pwm_config()
+ */
+ pclk_rate = clk_get_rate(ap->pclk);
+ extclk_rate = clk_get_rate(ap->extclk);
+
+ ap->pclk_rate = pclk_rate > NSEC_PER_SEC ? 0 : pclk_rate;
+ ap->extclk_rate = extclk_rate > NSEC_PER_SEC ? 0 : extclk_rate;
+
+ if (!ap->pclk_rate && !ap->extclk_rate)
+ return dev_err_probe(dev, -EINVAL,
+ "No usable clock: pclk %lu Hz, extclk %lu Hz\n",
+ pclk_rate, extclk_rate);
+
+ ap->regmap = devm_regmap_init_mmio(dev, reg_base,
+ &andes_pwm_regmap_config);
+ if (IS_ERR(ap->regmap))
+ return dev_err_probe(dev, PTR_ERR(ap->regmap),
+ "Failed to initialize regmap\n");
+
+ chip->ops = &andes_pwm_ops;
+ ret = devm_pwmchip_add(dev, chip);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
+
+ return 0;
+}
+
+static const struct of_device_id andes_pwm_of_match[] = {
+ { .compatible = "andestech,ae350-pwm" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, andes_pwm_of_match);
+
+static struct platform_driver andes_pwm_driver = {
+ .driver = {
+ .name = "andes_pwm",
+ .of_match_table = andes_pwm_of_match,
+ },
+ .probe = andes_pwm_probe,
+};
+module_platform_driver(andes_pwm_driver);
+
+MODULE_AUTHOR("Ben Zong-You Xie <ben717@andestech.com>");
+MODULE_DESCRIPTION("Andes PWM driver");
+MODULE_LICENSE("GPL");
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 2/3] pwm: add Andes PWM driver support
2026-06-25 10:36 ` Ben Zong-You Xie via B4 Relay
(?)
@ 2026-06-25 10:46 ` sashiko-bot
2026-08-10 4:50 ` Uwe Kleine-König
-1 siblings, 1 reply; 13+ messages in thread
From: sashiko-bot @ 2026-06-25 10:46 UTC (permalink / raw)
To: Ben Zong-You Xie via B4 Relay; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver fails to update the hardware PARK level to the inactive state when disabling the PWM channel, leaving the output stuck ON or at the wrong polarity.
--
commit 6823cc7ba3d3233c8090db2e53dc0a9b52b74a25
Author: Ben Zong-You Xie <ben717@andestech.com>
pwm: add Andes PWM driver support
Add a driver for the PWM controller found in Andes AE350 platforms and
QiLai SoCs.
> diff --git a/drivers/pwm/pwm-andes.c b/drivers/pwm/pwm-andes.c
> new file mode 100644
> index 0000000000000..580e673d2cff0
> --- /dev/null
> +++ b/drivers/pwm/pwm-andes.c
[ ... ]
> +static int andes_pwm_config(struct pwm_chip *chip, unsigned int channel,
> + const struct pwm_state *state)
> +{
[ ... ]
> + /*
> + * A zero-length phase means a 0% or 100% relative duty cycle, which the
> + * hardware cannot run. Emit the matching constant level by parking the
> + * channel: high_cycles == 0 stays low, low_cycles == 0 stays high.
> + */
> + if (!high_cycles)
> + return andes_pwm_park(chip, channel, false);
> + if (!low_cycles)
> + return andes_pwm_park(chip, channel, true);
[Severity: High]
When configuring a 100 percent duty cycle, this code sets the PARK bit to the
active state to emulate the output. If the channel is subsequently disabled,
could the output remain stuck ON?
> +
> + /*
> + * If changing the clock source here, the output will not be the old one
[ ... ]
> +static int andes_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + unsigned int channel = pwm->hwpwm;
> +
> + if (!state->enabled) {
> + if (pwm->state.enabled)
> + andes_pwm_enable(chip, channel, false);
> +
> + return 0;
> + }
[Severity: High]
Does this early return path disable the channel but fail to update the
hardware's PARK bit to reflect the inactive state of the current polarity?
If the PWM was previously configured to 100 percent duty cycle where the PARK
bit was set active, it appears bypassing the PARK bit configuration on the
disable path leaves the output driven at the active level.
Additionally, if the polarity changes while the channel is disabled, will the
hardware's inactive level fail to update?
> +
> + return andes_pwm_config(chip, channel, state);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-andes-pwm-v6-0-3aef11711017@andestech.com?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 2/3] pwm: add Andes PWM driver support
2026-06-25 10:46 ` sashiko-bot
@ 2026-08-10 4:50 ` Uwe Kleine-König
0 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2026-08-10 4:50 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Ben Zong-You Xie via B4 Relay, conor+dt, robh, devicetree
[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]
Hello,
On Thu, Jun 25, 2026 at 10:46:13AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The driver fails to update the hardware PARK level to the inactive state when disabling the PWM channel, leaving the output stuck ON or at the wrong polarity.
I didn't look in detail into the patch yet, but I think the review
feedback here is misleading. There is no promise about a hardware state
for a disabled channel. The only objective is to save power.
Having said that, it's *nice* to drive the inactive level, but if that
isn't easily possible (e.g. because the output goes to high-Z on
disable) the primary objective is to save power.
So I guess the feedback is OK, but not "High".
> Additionally, if the polarity changes while the channel is disabled, will the
> hardware's inactive level fail to update?
This is in the same category. It's a bit surprising, but well in the
limits of what a PWM consumer must expect.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v6 2/3] pwm: add Andes PWM driver support
2026-06-25 10:36 ` Ben Zong-You Xie via B4 Relay
(?)
(?)
@ 2026-08-10 8:19 ` Uwe Kleine-König
2026-08-11 10:11 ` Ben Zong-You Xie
-1 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2026-08-10 8:19 UTC (permalink / raw)
To: ben717
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-pwm,
devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 13807 bytes --]
On Thu, Jun 25, 2026 at 06:36:00PM +0800, Ben Zong-You Xie via B4 Relay wrote:
> From: Ben Zong-You Xie <ben717@andestech.com>
>
> Add a driver for the PWM controller found in Andes AE350 platforms and
> QiLai SoCs.
>
> The Andes PWM controller features:
> - 4 independent channels.
> - Dual clock source support (APB clock and external clock) to provide
> a flexible range of frequencies.
> - Support for normal and inversed polarity.
>
> The driver implements the .apply() and .get_state() callbacks. Since the
> clock source of each channel can be selected by programming the
> register, clock selection logic is implemented to prioritize the
> external clock to maximize the supported period range, falling back to
> the APB clock for higher frequency requirements.
>
> Signed-off-by: Ben Zong-You Xie <ben717@andestech.com>
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-andes.c | 343 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 354 insertions(+)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index e8886a9b64d9..52dee4b7f081 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -73,6 +73,16 @@ config PWM_AIROHA
> To compile this driver as a module, choose M here: the module
> will be called pwm-airoha.
>
> +config PWM_ANDES
> + tristate "Andes PWM support"
> + depends on ARCH_ANDES || COMPILE_TEST
Here are missing dependencies. At least REGMAP.
> + help
> + Generic PWM framework driver for Andes platform, such as QiLai SoC
> + and AE350 platform.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-andes.
> +
> config PWM_APPLE
> tristate "Apple SoC PWM support"
> depends on ARCH_APPLE || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 5630a521a7cf..c92369ee251d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_PWM) += core.o
> obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
> obj-$(CONFIG_PWM_ADP5585) += pwm-adp5585.o
> obj-$(CONFIG_PWM_AIROHA) += pwm-airoha.o
> +obj-$(CONFIG_PWM_ANDES) += pwm-andes.o
> obj-$(CONFIG_PWM_APPLE) += pwm-apple.o
> obj-$(CONFIG_PWM_ARGON_FAN_HAT) += pwm-argon-fan-hat.o
> obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
> diff --git a/drivers/pwm/pwm-andes.c b/drivers/pwm/pwm-andes.c
> new file mode 100644
> index 000000000000..580e673d2cff
> --- /dev/null
> +++ b/drivers/pwm/pwm-andes.c
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Andes PWM, used in Andes AE350 platform and QiLai SoC
> + *
> + * Copyright (C) 2026 Andes Technology Corporation.
> + *
> + * Limitations:
> + * - When disabling a channel, the current period is not completed and the
> + * output is driven to the PARK level (low when ANDES_PWM_CH_CTRL_PARK is
> + * clear, high when it is set).
> + * - The current period will be completed first if reconfiguring.
> + * - Further, if the reconfiguration changes the clock source, the output will
> + * not be the old one nor the new one. And the output will be the new one
> + * after writing to the reload register.
> + * - The hardware cannot run a 0% or 100% relative duty cycle; the driver
> + * emulates these by disabling the channel and parking the output at the
> + * constant level.
> + * - A period or duty cycle larger than the selected clock can represent is
> + * rounded down to the largest achievable value rather than rejected.
The last item isn't a (hardware) property, but the right thing to do for
PWM drivers. So you can drop that.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/math64.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
I wonder what <linux/time.h> is used for.
> [...]
> +/*
> + * Hold the output at a constant level by parking the disabled channel. A
> + * disabled channel drives its output to the PARK level (low when @high is
> + * false, high when @high is true), which is used to emulate a 0% or 100%
> + * relative duty cycle.
> + */
> +static int andes_pwm_park(struct pwm_chip *chip, unsigned int channel,
> + bool high)
> +{
> + struct andes_pwm *ap = andes_pwm_from_chip(chip);
> +
> + regmap_assign_bits(ap->regmap, ANDES_PWM_CH_CTRL(channel),
> + ANDES_PWM_CH_CTRL_PARK, high);
Some calls to regmap_assign_bits() are checked, others are not. Please
make this consistent.
> + return andes_pwm_enable(chip, channel, false);
> +}
> +
> +static int andes_pwm_config(struct pwm_chip *chip, unsigned int channel,
> + const struct pwm_state *state)
> +{
> + struct andes_pwm *ap = andes_pwm_from_chip(chip);
> + unsigned int clk_rate = ap->extclk_rate;
> + unsigned int ctrl = ANDES_PWM_CH_CTRL_MODE_PWM;
> + bool use_pclk = false;
> + u64 high_cycles;
> + u64 low_cycles;
> + u64 period_cycles;
> + u64 duty_cycles;
> + u32 reload;
> +
> + /*
> + * Reload register for PWM mode:
> + *
> + * 31 : 16 15 : 0
> + * PWM16_Hi | PWM16_Lo
> + *
> + * The high duration is (PWM16_Hi + 1) cycles and the low duration is
> + * (PWM16_Lo + 1) cycles, so each phase spans ANDES_PWM_CYCLE_MIN to
> + * ANDES_PWM_CYCLE_MAX cycles. The hardware period (their sum) can reach
> + * 2 * ANDES_PWM_CYCLE_MAX cycles, but the PWM core requires the period
> + * to be chosen from the requested period alone, independent of the duty
> + * cycle. That holds only while both phases stay within
> + * ANDES_PWM_CYCLE_MAX for every duty split, so the usable period is
> + * capped at ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN cycles.
> + *
> + * The controller has two clock sources, the APB clock and an external
> + * clock. Since the external clock frequency must be slower than the APB
> + * clock, it is tried first for its wider period range; the APB clock is
> + * used only when the external clock is too fast to represent the period
> + * (it resolves fewer than two cycles) or is absent.
> + */
> + period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
> + NSEC_PER_SEC);
> + if (period_cycles < 2 * ANDES_PWM_CYCLE_MIN) {
With period_cycles = 1 you can only have duty_cycle = 0 or 1 which is
representable by the hardware (configuring either constant high or
constant low output).
> + use_pclk = true;
> + clk_rate = ap->pclk_rate;
> + period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
> + NSEC_PER_SEC);
> + if (period_cycles < 2 * ANDES_PWM_CYCLE_MIN)
> + return -EINVAL;
> + }
> +
> + /*
> + * Round the period down to the largest value representable for every
> + * duty cycle, so the chosen period depends on the requested period
> + * alone. With both phases capped at ANDES_PWM_CYCLE_MAX, that bound is
> + * ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN cycles.
> + */
> + period_cycles = min_t(u64, period_cycles,
> + ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN);
> +
> + /* The duty cycle cannot exceed the (possibly clamped) period. */
> + duty_cycles = mul_u64_u64_div_u64(clk_rate, state->duty_cycle,
> + NSEC_PER_SEC);
> + duty_cycles = min_t(u64, duty_cycles, period_cycles);
empty line here please
> + if (state->polarity == PWM_POLARITY_INVERSED) {
> + low_cycles = duty_cycles;
> + high_cycles = period_cycles - low_cycles;
> + } else {
> + high_cycles = duty_cycles;
> + low_cycles = period_cycles - high_cycles;
> + }
> +
> + /*
> + * A zero-length phase means a 0% or 100% relative duty cycle, which the
> + * hardware cannot run. Emit the matching constant level by parking the
> + * channel: high_cycles == 0 stays low, low_cycles == 0 stays high.
> + */
> + if (!high_cycles)
> + return andes_pwm_park(chip, channel, false);
> + if (!low_cycles)
> + return andes_pwm_park(chip, channel, true);
> +
> + /*
> + * If changing the clock source here, the output will not be the old one
s/not/neither/
> + * nor the new one. And the output will be the new one after writing to
> + * the reload register.
I'd write:
A change of clock source takes effect immediately, modifying the
current output. Otherwise there is no glitch as the currently
running period is completed before the new settings take effect.
> + */
> + ctrl |= use_pclk ? ANDES_PWM_CH_CTRL_CLK : 0;
> + ctrl |= (state->polarity == PWM_POLARITY_INVERSED) ?
> + ANDES_PWM_CH_CTRL_PARK : 0;
> +
> + regmap_update_bits(ap->regmap, ANDES_PWM_CH_CTRL(channel),
> + ANDES_PWM_CH_CTRL_MASK, ctrl);
> + reload = FIELD_PREP(ANDES_PWM_CH_RELOAD_HIGH, high_cycles - 1) |
> + FIELD_PREP(ANDES_PWM_CH_RELOAD_LOW, low_cycles - 1);
> + regmap_write(ap->regmap, ANDES_PWM_CH_RELOAD(channel), reload);
empty line here
> + return andes_pwm_enable(chip, channel, true);
> +}
> +
> +static int andes_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + unsigned int channel = pwm->hwpwm;
> +
> + if (!state->enabled) {
> + if (pwm->state.enabled)
> + andes_pwm_enable(chip, channel, false);
> +
> + return 0;
> + }
> +
> + return andes_pwm_config(chip, channel, state);
> +}
> +
> +static int andes_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct andes_pwm *ap = andes_pwm_from_chip(chip);
> + unsigned int channel = pwm->hwpwm;
> + unsigned int ctrl;
> + unsigned int clk_rate;
> + unsigned int reload;
> + u64 high_cycles;
> + u64 low_cycles;
> +
> + regmap_read(ap->regmap, ANDES_PWM_CH_CTRL(channel), &ctrl);
> + clk_rate = FIELD_GET(ANDES_PWM_CH_CTRL_CLK, ctrl) ? ap->pclk_rate
> + : ap->extclk_rate;
> + if (!clk_rate) {
> + /*
> + * The selected clock source is unavailable, so the channel
> + * cannot be running; report it as disabled and avoid the
> + * division by zero below.
> + */
> + state->enabled = false;
> + state->period = 0;
> + state->duty_cycle = 0;
> + return 0;
> + }
> +
> + state->enabled = regmap_test_bits(ap->regmap, ANDES_PWM_CH_ENABLE,
> + ANDES_PWM_CH_ENABLE_PWM(channel)) > 0;
> + state->polarity = FIELD_GET(ANDES_PWM_CH_CTRL_PARK, ctrl) ?
> + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> + regmap_read(ap->regmap, ANDES_PWM_CH_RELOAD(channel), &reload);
> + high_cycles = FIELD_GET(ANDES_PWM_CH_RELOAD_HIGH, reload) + 1;
> + low_cycles = FIELD_GET(ANDES_PWM_CH_RELOAD_LOW, reload) + 1;
> +
> + /*
> + * high_cycles and low_cycles are each at most ANDES_PWM_CYCLE_MAX
> + * (0x10000, 17 bits) and NSEC_PER_SEC is below 2^30, so the products
> + * below are safe from 64-bit overflow.
> + */
> + if (state->polarity == PWM_POLARITY_INVERSED)
> + state->duty_cycle = DIV_ROUND_UP_ULL(low_cycles * NSEC_PER_SEC,
> + clk_rate);
> + else
> + state->duty_cycle = DIV_ROUND_UP_ULL(high_cycles * NSEC_PER_SEC,
> + clk_rate);
This can be simplified a bit to:
if (state->polarity == PWM_POLARITY_INVERSED)
duty_cycles = low_cycles;
else
duty_cycles = high_cycles;
stat->duty_cycle = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC,
clk_rate);
> + state->period = DIV_ROUND_UP_ULL((high_cycles + low_cycles) *
> + NSEC_PER_SEC, clk_rate);
> +
> + return 0;
> +}
> [...]
> +static int andes_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pwm_chip *chip;
> + struct andes_pwm *ap;
> + void __iomem *reg_base;
> + unsigned long pclk_rate;
> + unsigned long extclk_rate;
> + int ret;
> +
> + chip = devm_pwmchip_alloc(dev, ANDES_PWM_CH_MAX, sizeof(*ap));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + ap = andes_pwm_from_chip(chip);
> + reg_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(reg_base))
> + return dev_err_probe(dev, PTR_ERR(reg_base),
> + "Failed to map I/O space\n");
> +
> + ap->pclk = devm_clk_get_enabled(dev, "pclk");
> + if (IS_ERR(ap->pclk))
> + return dev_err_probe(dev, PTR_ERR(ap->pclk),
> + "Failed to get APB clock\n");
> +
> + ap->extclk = devm_clk_get_optional_enabled(dev, "extclk");
> + if (IS_ERR(ap->extclk))
> + return dev_err_probe(dev, PTR_ERR(ap->extclk),
> + "Failed to get external clock\n");
> +
> + /*
> + * If the clock rate is greater than 10^9, there may be an overflow when
> + * calculating the cycles in andes_pwm_config()
> + */
> + pclk_rate = clk_get_rate(ap->pclk);
> + extclk_rate = clk_get_rate(ap->extclk);
Please call devm_clk_rate_exclusive_get() to ensure the clk rates are
not changed behind your back.
> + ap->pclk_rate = pclk_rate > NSEC_PER_SEC ? 0 : pclk_rate;
> + ap->extclk_rate = extclk_rate > NSEC_PER_SEC ? 0 : extclk_rate;
> +
> + if (!ap->pclk_rate && !ap->extclk_rate)
> + return dev_err_probe(dev, -EINVAL,
> + "No usable clock: pclk %lu Hz, extclk %lu Hz\n",
> + pclk_rate, extclk_rate);
> +
> + ap->regmap = devm_regmap_init_mmio(dev, reg_base,
> + &andes_pwm_regmap_config);
> + if (IS_ERR(ap->regmap))
> + return dev_err_probe(dev, PTR_ERR(ap->regmap),
> + "Failed to initialize regmap\n");
> +
> + chip->ops = &andes_pwm_ops;
I think you can add:
chip->atomic = true;
> + ret = devm_pwmchip_add(dev, chip);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
> +
> + return 0;
> +}
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v6 2/3] pwm: add Andes PWM driver support
2026-08-10 8:19 ` Uwe Kleine-König
@ 2026-08-11 10:11 ` Ben Zong-You Xie
0 siblings, 0 replies; 13+ messages in thread
From: Ben Zong-You Xie @ 2026-08-11 10:11 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-pwm,
devicetree, linux-kernel
Hi Uwe,
Thanks for the review.
On Mon, Aug 10, 2026 at 10:19:49AM +0200, Uwe Kleine-König wrote:
> Subject: Re: [PATCH v6 2/3] pwm: add Andes PWM driver support
>
> On Thu, Jun 25, 2026 at 06:36:00PM +0800, Ben Zong-You Xie via B4 Relay wrote:
> > From: Ben Zong-You Xie <ben717@andestech.com>
> >
> > Add a driver for the PWM controller found in Andes AE350 platforms and
> > QiLai SoCs.
> >
> > The Andes PWM controller features:
> > - 4 independent channels.
> > - Dual clock source support (APB clock and external clock) to provide
> > a flexible range of frequencies.
> > - Support for normal and inversed polarity.
> >
> > The driver implements the .apply() and .get_state() callbacks. Since the
> > clock source of each channel can be selected by programming the
> > register, clock selection logic is implemented to prioritize the
> > external clock to maximize the supported period range, falling back to
> > the APB clock for higher frequency requirements.
> >
> > Signed-off-by: Ben Zong-You Xie <ben717@andestech.com>
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-andes.c | 343 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 354 insertions(+)
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index e8886a9b64d9..52dee4b7f081 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -73,6 +73,16 @@ config PWM_AIROHA
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-airoha.
> >
> > +config PWM_ANDES
> > + tristate "Andes PWM support"
> > + depends on ARCH_ANDES || COMPILE_TEST
>
> Here are missing dependencies. At least REGMAP.
>
Right. v7 will have:
depends on ARCH_ANDES || COMPILE_TEST
depends on HAS_IOMEM && COMMON_CLK
select REGMAP_MMIO
> > + help
> > + Generic PWM framework driver for Andes platform, such as QiLai SoC
> > + and AE350 platform.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-andes.
> > +
> > config PWM_APPLE
> > tristate "Apple SoC PWM support"
> > depends on ARCH_APPLE || COMPILE_TEST
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 5630a521a7cf..c92369ee251d 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -3,6 +3,7 @@ obj-$(CONFIG_PWM) += core.o
> > obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
> > obj-$(CONFIG_PWM_ADP5585) += pwm-adp5585.o
> > obj-$(CONFIG_PWM_AIROHA) += pwm-airoha.o
> > +obj-$(CONFIG_PWM_ANDES) += pwm-andes.o
> > obj-$(CONFIG_PWM_APPLE) += pwm-apple.o
> > obj-$(CONFIG_PWM_ARGON_FAN_HAT) += pwm-argon-fan-hat.o
> > obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
> > diff --git a/drivers/pwm/pwm-andes.c b/drivers/pwm/pwm-andes.c
> > new file mode 100644
> > index 000000000000..580e673d2cff
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-andes.c
> > @@ -0,0 +1,343 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for Andes PWM, used in Andes AE350 platform and QiLai SoC
> > + *
> > + * Copyright (C) 2026 Andes Technology Corporation.
> > + *
> > + * Limitations:
> > + * - When disabling a channel, the current period is not completed and the
> > + * output is driven to the PARK level (low when ANDES_PWM_CH_CTRL_PARK is
> > + * clear, high when it is set).
> > + * - The current period will be completed first if reconfiguring.
> > + * - Further, if the reconfiguration changes the clock source, the output will
> > + * not be the old one nor the new one. And the output will be the new one
> > + * after writing to the reload register.
> > + * - The hardware cannot run a 0% or 100% relative duty cycle; the driver
> > + * emulates these by disabling the channel and parking the output at the
> > + * constant level.
> > + * - A period or duty cycle larger than the selected clock can represent is
> > + * rounded down to the largest achievable value rather than rejected.
>
> The last item isn't a (hardware) property, but the right thing to do for
> PWM drivers. So you can drop that.
>
Will drop in v7.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/math64.h>
> > +#include <linux/minmax.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/time.h>
> > +#include <linux/types.h>
>
> I wonder what <linux/time.h> is used for.
>
NSEC_PER_SEC.
> > [...]
> > +/*
> > + * Hold the output at a constant level by parking the disabled channel. A
> > + * disabled channel drives its output to the PARK level (low when @high is
> > + * false, high when @high is true), which is used to emulate a 0% or 100%
> > + * relative duty cycle.
> > + */
> > +static int andes_pwm_park(struct pwm_chip *chip, unsigned int channel,
> > + bool high)
> > +{
> > + struct andes_pwm *ap = andes_pwm_from_chip(chip);
> > +
> > + regmap_assign_bits(ap->regmap, ANDES_PWM_CH_CTRL(channel),
> > + ANDES_PWM_CH_CTRL_PARK, high);
>
> Some calls to regmap_assign_bits() are checked, others are not. Please
> make this consistent.
>
Will make the check consistent in v7.
> > + return andes_pwm_enable(chip, channel, false);
> > +}
> > +
> > +static int andes_pwm_config(struct pwm_chip *chip, unsigned int channel,
> > + const struct pwm_state *state)
> > +{
> > + struct andes_pwm *ap = andes_pwm_from_chip(chip);
> > + unsigned int clk_rate = ap->extclk_rate;
> > + unsigned int ctrl = ANDES_PWM_CH_CTRL_MODE_PWM;
> > + bool use_pclk = false;
> > + u64 high_cycles;
> > + u64 low_cycles;
> > + u64 period_cycles;
> > + u64 duty_cycles;
> > + u32 reload;
> > +
> > + /*
> > + * Reload register for PWM mode:
> > + *
> > + * 31 : 16 15 : 0
> > + * PWM16_Hi | PWM16_Lo
> > + *
> > + * The high duration is (PWM16_Hi + 1) cycles and the low duration is
> > + * (PWM16_Lo + 1) cycles, so each phase spans ANDES_PWM_CYCLE_MIN to
> > + * ANDES_PWM_CYCLE_MAX cycles. The hardware period (their sum) can reach
> > + * 2 * ANDES_PWM_CYCLE_MAX cycles, but the PWM core requires the period
> > + * to be chosen from the requested period alone, independent of the duty
> > + * cycle. That holds only while both phases stay within
> > + * ANDES_PWM_CYCLE_MAX for every duty split, so the usable period is
> > + * capped at ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN cycles.
> > + *
> > + * The controller has two clock sources, the APB clock and an external
> > + * clock. Since the external clock frequency must be slower than the APB
> > + * clock, it is tried first for its wider period range; the APB clock is
> > + * used only when the external clock is too fast to represent the period
> > + * (it resolves fewer than two cycles) or is absent.
> > + */
> > + period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
> > + NSEC_PER_SEC);
> > + if (period_cycles < 2 * ANDES_PWM_CYCLE_MIN) {
>
> With period_cycles = 1 you can only have duty_cycle = 0 or 1 which is
> representable by the hardware (configuring either constant high or
> constant low output).
>
Right, v7 will only reject zero:
if (period_cycles < 2 * ANDES_PWM_CYCLE_MIN && ap->pclk_rate) {
use_pclk = true;
clk_rate = ap->pclk_rate;
period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
NSEC_PER_SEC);
}
if (!period_cycles)
return -EINVAL;
I still switch to pclk at two cycles though. If extclk only resolves one
cycle, pclk may well manage a real waveform, which beats parking.
> > + use_pclk = true;
> > + clk_rate = ap->pclk_rate;
> > + period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
> > + NSEC_PER_SEC);
> > + if (period_cycles < 2 * ANDES_PWM_CYCLE_MIN)
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * Round the period down to the largest value representable for every
> > + * duty cycle, so the chosen period depends on the requested period
> > + * alone. With both phases capped at ANDES_PWM_CYCLE_MAX, that bound is
> > + * ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN cycles.
> > + */
> > + period_cycles = min_t(u64, period_cycles,
> > + ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN);
> > +
> > + /* The duty cycle cannot exceed the (possibly clamped) period. */
> > + duty_cycles = mul_u64_u64_div_u64(clk_rate, state->duty_cycle,
> > + NSEC_PER_SEC);
> > + duty_cycles = min_t(u64, duty_cycles, period_cycles);
>
> empty line here please
>
Will add in v7.
> > + if (state->polarity == PWM_POLARITY_INVERSED) {
> > + low_cycles = duty_cycles;
> > + high_cycles = period_cycles - low_cycles;
> > + } else {
> > + high_cycles = duty_cycles;
> > + low_cycles = period_cycles - high_cycles;
> > + }
> > +
> > + /*
> > + * A zero-length phase means a 0% or 100% relative duty cycle, which the
> > + * hardware cannot run. Emit the matching constant level by parking the
> > + * channel: high_cycles == 0 stays low, low_cycles == 0 stays high.
> > + */
> > + if (!high_cycles)
> > + return andes_pwm_park(chip, channel, false);
> > + if (!low_cycles)
> > + return andes_pwm_park(chip, channel, true);
> > +
> > + /*
> > + * If changing the clock source here, the output will not be the old one
>
> s/not/neither/
>
> > + * nor the new one. And the output will be the new one after writing to
> > + * the reload register.
>
> I'd write:
>
> A change of clock source takes effect immediately, modifying the
> current output. Otherwise there is no glitch as the currently
> running period is completed before the new settings take effect.
>
Will take your wording. The Limitations block at the top described the same
thing the same way, so I replaced it there too.
> > + */
> > + ctrl |= use_pclk ? ANDES_PWM_CH_CTRL_CLK : 0;
> > + ctrl |= (state->polarity == PWM_POLARITY_INVERSED) ?
> > + ANDES_PWM_CH_CTRL_PARK : 0;
> > +
> > + regmap_update_bits(ap->regmap, ANDES_PWM_CH_CTRL(channel),
> > + ANDES_PWM_CH_CTRL_MASK, ctrl);
> > + reload = FIELD_PREP(ANDES_PWM_CH_RELOAD_HIGH, high_cycles - 1) |
> > + FIELD_PREP(ANDES_PWM_CH_RELOAD_LOW, low_cycles - 1);
> > + regmap_write(ap->regmap, ANDES_PWM_CH_RELOAD(channel), reload);
>
> empty line here
>
Will add in v7.
> > + return andes_pwm_enable(chip, channel, true);
> > +}
> > +
> > +static int andes_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + unsigned int channel = pwm->hwpwm;
> > +
> > + if (!state->enabled) {
> > + if (pwm->state.enabled)
> > + andes_pwm_enable(chip, channel, false);
> > +
> > + return 0;
> > + }
> > +
> > + return andes_pwm_config(chip, channel, state);
> > +}
> > +
> > +static int andes_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct andes_pwm *ap = andes_pwm_from_chip(chip);
> > + unsigned int channel = pwm->hwpwm;
> > + unsigned int ctrl;
> > + unsigned int clk_rate;
> > + unsigned int reload;
> > + u64 high_cycles;
> > + u64 low_cycles;
> > +
> > + regmap_read(ap->regmap, ANDES_PWM_CH_CTRL(channel), &ctrl);
> > + clk_rate = FIELD_GET(ANDES_PWM_CH_CTRL_CLK, ctrl) ? ap->pclk_rate
> > + : ap->extclk_rate;
> > + if (!clk_rate) {
> > + /*
> > + * The selected clock source is unavailable, so the channel
> > + * cannot be running; report it as disabled and avoid the
> > + * division by zero below.
> > + */
> > + state->enabled = false;
> > + state->period = 0;
> > + state->duty_cycle = 0;
> > + return 0;
> > + }
> > +
> > + state->enabled = regmap_test_bits(ap->regmap, ANDES_PWM_CH_ENABLE,
> > + ANDES_PWM_CH_ENABLE_PWM(channel)) > 0;
> > + state->polarity = FIELD_GET(ANDES_PWM_CH_CTRL_PARK, ctrl) ?
> > + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > + regmap_read(ap->regmap, ANDES_PWM_CH_RELOAD(channel), &reload);
> > + high_cycles = FIELD_GET(ANDES_PWM_CH_RELOAD_HIGH, reload) + 1;
> > + low_cycles = FIELD_GET(ANDES_PWM_CH_RELOAD_LOW, reload) + 1;
> > +
> > + /*
> > + * high_cycles and low_cycles are each at most ANDES_PWM_CYCLE_MAX
> > + * (0x10000, 17 bits) and NSEC_PER_SEC is below 2^30, so the products
> > + * below are safe from 64-bit overflow.
> > + */
> > + if (state->polarity == PWM_POLARITY_INVERSED)
> > + state->duty_cycle = DIV_ROUND_UP_ULL(low_cycles * NSEC_PER_SEC,
> > + clk_rate);
> > + else
> > + state->duty_cycle = DIV_ROUND_UP_ULL(high_cycles * NSEC_PER_SEC,
> > + clk_rate);
>
> This can be simplified a bit to:
>
> if (state->polarity == PWM_POLARITY_INVERSED)
> duty_cycles = low_cycles;
> else
> duty_cycles = high_cycles;
>
> stat->duty_cycle = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC,
> clk_rate);
>
Will apply in v7.
> > + state->period = DIV_ROUND_UP_ULL((high_cycles + low_cycles) *
> > + NSEC_PER_SEC, clk_rate);
> > +
> > + return 0;
> > +}
> > [...]
> > +static int andes_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct pwm_chip *chip;
> > + struct andes_pwm *ap;
> > + void __iomem *reg_base;
> > + unsigned long pclk_rate;
> > + unsigned long extclk_rate;
> > + int ret;
> > +
> > + chip = devm_pwmchip_alloc(dev, ANDES_PWM_CH_MAX, sizeof(*ap));
> > + if (IS_ERR(chip))
> > + return PTR_ERR(chip);
> > +
> > + ap = andes_pwm_from_chip(chip);
> > + reg_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(reg_base))
> > + return dev_err_probe(dev, PTR_ERR(reg_base),
> > + "Failed to map I/O space\n");
> > +
> > + ap->pclk = devm_clk_get_enabled(dev, "pclk");
> > + if (IS_ERR(ap->pclk))
> > + return dev_err_probe(dev, PTR_ERR(ap->pclk),
> > + "Failed to get APB clock\n");
> > +
> > + ap->extclk = devm_clk_get_optional_enabled(dev, "extclk");
> > + if (IS_ERR(ap->extclk))
> > + return dev_err_probe(dev, PTR_ERR(ap->extclk),
> > + "Failed to get external clock\n");
> > +
> > + /*
> > + * If the clock rate is greater than 10^9, there may be an overflow when
> > + * calculating the cycles in andes_pwm_config()
> > + */
> > + pclk_rate = clk_get_rate(ap->pclk);
> > + extclk_rate = clk_get_rate(ap->extclk);
>
> Please call devm_clk_rate_exclusive_get() to ensure the clk rates are
> not changed behind your back.
>
Will do this for both clocks, before clk_get_rate().
> > + ap->pclk_rate = pclk_rate > NSEC_PER_SEC ? 0 : pclk_rate;
> > + ap->extclk_rate = extclk_rate > NSEC_PER_SEC ? 0 : extclk_rate;
> > +
> > + if (!ap->pclk_rate && !ap->extclk_rate)
> > + return dev_err_probe(dev, -EINVAL,
> > + "No usable clock: pclk %lu Hz, extclk %lu Hz\n",
> > + pclk_rate, extclk_rate);
> > +
> > + ap->regmap = devm_regmap_init_mmio(dev, reg_base,
> > + &andes_pwm_regmap_config);
> > + if (IS_ERR(ap->regmap))
> > + return dev_err_probe(dev, PTR_ERR(ap->regmap),
> > + "Failed to initialize regmap\n");
> > +
> > + chip->ops = &andes_pwm_ops;
>
> I think you can add:
>
> chip->atomic = true;
>
Right, will add in v7.
> > + ret = devm_pwmchip_add(dev, chip);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
> > +
> > + return 0;
> > +}
>
> Best regards
> Uwe
Thanks,
Ben
^ permalink raw reply [flat|nested] 13+ messages in thread