[v3 0/2] pwm: add PWM driver for atcpit100

[v3 0/2] pwm: add PWM driver for atcpit100

[Ben Zong-You Xie]

Re-introduce the driver for atcpit100. This device driver was previously
part of the Linux Kernel, but was removed due to the deprecation of the
NDS32 architecture [1]. Although Andes now dedicates our effort on RISC-V,
ATCPIT100 is still one of our peripheral platform IPs, and that's why we
are re-introducing it now.

This patch series includes DT-bindings(1/2) and PWM driver(2/2).

The ATCPIT100 Programmable Interval Timer (PIT) is a set of compact
multi-function timers, which can be used as pulse width modulators (PWM)
as well as simple timers. ATCPIT100 supports up to 4 PIT channels,
and each PIT channel may be a simple timer or PWM, or a combination of
the timer and the PWM.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aec499c75cf8e0b599be4d559e6922b613085f8f
---
Changes in v3:
(1/2):
 - modify the compatible string according to Krzysztof's review.
(2/2):
 - add a check for the clock rate to prevent the overflow warning by
   kernel test robot.
 - remove the unnecessary check in .apply() reported by kernel test
   robot.

Link to v2: https://lore.kernel.org/linux-pwm/20241202060147.1271264-1-ben717@andestech.com/T/#t

Changes in v2:
(1/2):
 - change "title" in the yaml file.
 - remove vendor-specific property, and add clocks property.
(2/2):
 - add a description for hardware limitations.
 - instead of statically configuring the clock source in the dtb,
   switch the clock parent depending on the requested setting.
 - have some minor changes according to Uwe's suggestion.

Link to v1: https://lore.kernel.org/linux-pwm/20241028102721.1961289-1-ben717@andestech.com/T/#t
---
Ben Zong-You Xie (2):
  dt-bindings: pwm: add atcpit100
  pwm: atcpit100: add Andes PWM driver support

 .../bindings/pwm/andestech,atcpit100-pwm.yaml |  51 +++
 MAINTAINERS                                   |   6 +
 drivers/pwm/Kconfig                           |  17 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-atcpit100.c                   | 296 ++++++++++++++++++
 5 files changed, 371 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
 create mode 100644 drivers/pwm/pwm-atcpit100.c

---
2.34.1

[v3 1/2] dt-bindings: pwm: add atcpit100

[Ben Zong-You Xie]

Document devicetree bindings for Andes PWM controller.

Signed-off-by: Ben Zong-You Xie <ben717@andestech.com>
---
 .../bindings/pwm/andestech,atcpit100-pwm.yaml | 51 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml b/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
new file mode 100644
index 000000000000..20d099c1503f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/andestech,atcpit100-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Andes PWM controller
+
+maintainers:
+  - Ben Zong-You Xie <ben717@andestech.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: andestech,atcpit100
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: ext
+      - const: apb
+
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    pwm@f0400000 {
+      compatible = "andestech,atcpit100";
+      reg = <0xf0400000 0x1000>;
+      #pwm-cells = <3>;
+      clocks = <&smu 1>, <&smu 7>;
+      clock-names = "ext", "apb";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e930c7a58b1..bc54216d1835 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3588,6 +3588,11 @@ F:	drivers/power/reset/atc260x-poweroff.c
 F:	drivers/regulator/atc260x-regulator.c
 F:	include/linux/mfd/atc260x/*
 
+ATCPIT100 PWM DRIVER
+M:	Ben Zong-You Xie <ben717@andestech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
+
 ATHEROS 71XX/9XXX GPIO DRIVER
 M:	Alban Bedel <albeu@free.fr>
 S:	Maintained
-- 
2.34.1

Re: [v3 1/2] dt-bindings: pwm: add atcpit100

[Krzysztof Kozlowski]

On 23/01/2025 20:35, Ben Zong-You Xie wrote:
> Document devicetree bindings for Andes PWM controller.


Please use standard email subjects, so with the PATCH keyword in the
title. `git format-patch -vX` helps here to create proper versioned
patches. Another useful tool is b4. Skipping the PATCH keyword makes
filtering of emails more difficult thus making the review process less
convenient.

> 
> Signed-off-by: Ben Zong-You Xie <ben717@andestech.com>
> ---
>  .../bindings/pwm/andestech,atcpit100-pwm.yaml | 51 +++++++++++++++++++
>  MAINTAINERS                                   |  5 ++
>  2 files changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml b/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
> new file mode 100644
> index 000000000000..20d099c1503f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml

Filename: andestech,atcpit100.yaml

> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2

Best regards,
Krzysztof

[v3 2/2] pwm: atcpit100: add Andes PWM driver support

[Ben Zong-You Xie]

Add PWM driver support for atcpit100 on Andes AE350 platform.

Signed-off-by: Ben Zong-You Xie <ben717@andestech.com>
---
 MAINTAINERS                 |   1 +
 drivers/pwm/Kconfig         |  17 +++
 drivers/pwm/Makefile        |   1 +
 drivers/pwm/pwm-atcpit100.c | 296 ++++++++++++++++++++++++++++++++++++
 4 files changed, 315 insertions(+)
 create mode 100644 drivers/pwm/pwm-atcpit100.c

diff --git a/MAINTAINERS b/MAINTAINERS
index bc54216d1835..04cb6f1d3638 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3592,6 +3592,7 @@ ATCPIT100 PWM DRIVER
 M:	Ben Zong-You Xie <ben717@andestech.com>
 S:	Supported
 F:	Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
+F:	drivers/pwm/pwm-atcpit100.c
 
 ATHEROS 71XX/9XXX GPIO DRIVER
 M:	Alban Bedel <albeu@free.fr>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..f45ff74fb44e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -66,6 +66,23 @@ config PWM_APPLE
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-apple.
 
+config PWM_ATCPIT100
+	tristate "Andes ATCPIT100 PWM support"
+	depends on OF && HAS_IOMEM
+	depends on RISCV || COMPILE_TEST
+	select REGMAP_MMIO
+	help
+	  Generic PWM framework driver for ATCPIT100 on Andes AE350 platform
+
+	  The ATCPIT100 Programmable Interval Timer (PIT) is a set of compact
+	  multi-function timers, which can be used as pulse width
+	  modulators (PWM) as well as simple timers. ATCPIT100 supports up to 4
+	  PIT channels. Each PIT channel can be a simple timer or PWM, or a
+	  combination of timer and PWM.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-atcpit100.
+
 config PWM_ATMEL
 	tristate "Atmel PWM support"
 	depends on ARCH_AT91 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..ad6e803f12d0 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_APPLE)		+= pwm-apple.o
+obj-$(CONFIG_PWM_ATCPIT100)	+= pwm-atcpit100.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
diff --git a/drivers/pwm/pwm-atcpit100.c b/drivers/pwm/pwm-atcpit100.c
new file mode 100644
index 000000000000..d24858844540
--- /dev/null
+++ b/drivers/pwm/pwm-atcpit100.c
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM driver for ATCPIT100 on Andes AE350 platform
+ *
+ * Copyright (C) 2025 Andes Technology Corporation.
+ *
+ * Limitations:
+ * - When disabling a channel, the current period will not be completed, and the
+ *   output will be constant zero.
+ * - 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
+ *   once writing to the reload register.
+ * - The hardware can neither do a 0% nor a 100% relative duty cycle.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/math64.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 ATCPIT100_CHANNEL_ENABLE		0x1C
+#define ATCPIT100_CHANNEL_ENABLE_PWM(ch)	BIT(3 + (4 * ch))
+
+#define ATCPIT100_CHANNEL_CTRL(ch)		(0x20 + (0x10 * ch))
+#define ATCPIT100_CHANNEL_CTRL_MODE_PWM		0x04
+#define ATCPIT100_CHANNEL_CTRL_CLK		BIT(3)
+#define ATCPIT100_CHANNEL_CTRL_MASK		GENMASK(4, 0)
+
+#define ATCPIT100_CHANNEL_RELOAD(ch)		(0x24 + (0x10 * ch))
+#define ATCPIT100_CHANNEL_RELOAD_HIGH		GENMASK(31, 16)
+#define ATCPIT100_CHANNEL_RELOAD_LOW		GENMASK(15, 0)
+
+#define ATCPIT100_CHANNEL_MAX			4
+#define ATCPIT100_CYCLE_MIN			1
+#define ATCPIT100_CYCLE_MAX			0x10000
+#define ATCPIT100_IS_VALID_PERIOD(p)		\
+		in_range(p, min_period, max_period - min_period + 1)
+
+enum atcpit100_clk {
+	ATCPIT100_CLK_EXT = 0,
+	ATCPIT100_CLK_APB,
+	NUM_ATCPIT100_CLK
+};
+
+struct atcpit100_pwm {
+	unsigned long clk_rate[NUM_ATCPIT100_CLK];
+	struct regmap *regmap;
+	struct clk *ext_clk;
+	struct clk *apb_clk;
+};
+
+static const struct regmap_config atcpit100_pwm_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+};
+
+static inline struct atcpit100_pwm *to_atcpit100_pwm(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static int atcpit100_pwm_enable(struct pwm_chip *chip, unsigned int channel,
+				bool enable)
+{
+	unsigned int enable_bit = ATCPIT100_CHANNEL_ENABLE_PWM(channel);
+	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
+
+	return regmap_update_bits(ap->regmap, ATCPIT100_CHANNEL_ENABLE,
+				  enable_bit, enable ? enable_bit : 0);
+}
+
+static int atcpit100_pwm_config(struct pwm_chip *chip, unsigned int channel,
+				const struct pwm_state *state)
+{
+	int clk;
+	int ret;
+	unsigned int reload_val;
+	u64 max_period;
+	u64 min_period;
+	u64 high_cycle;
+	u64 low_cycle;
+	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
+	unsigned int ctrl_val = ATCPIT100_CHANNEL_CTRL_MODE_PWM;
+	u64 high_period = state->duty_cycle;
+	u64 low_period = state->period - high_period;
+
+	/*
+	 * Reload register for PWM mode:
+	 *
+	 *		31 : 16    15 : 0
+	 *		PWM16_Hi | PWM16_Lo
+	 *
+	 * In the PWM mode, the high period is (PWM16_Hi + 1) cycles, and the
+	 * low period is (PWM16_Lo + 1) cycles. Since we need to write
+	 * "numcycles - 1" to the register, the valid range of numcycles will
+	 * be between 1 to 0x10000. Calculate the possible periods that satisfy
+	 * the above restriction:
+	 *
+	 *	Let m = 1, M = 0x10000,
+	 *	m <= floor(cycle) <= M
+	 * <=>	m <= floor(rate * period / NSEC_PER_SEC) <= M
+	 * <=>	m <= rate * period / NSEC_PER_SEC < M + 1
+	 * <=>	m * NSEC_PER_SEC / rate <= period < (M + 1) * NSEC_PER_SEC / rate
+	 * <=>	ceil(m * NSEC_PER_SEC / rate) <= period <= ceil((M + 1) * NSEC_PER_SEC / rate) - 1
+	 *
+	 * Since there are two clock sources for ATCPIT100 on AE350 platform, if
+	 * the period is not valid for the first clock source, then the second
+	 * clock source will be checked. Reject the request when both clock
+	 * sources are not valid for the settings.
+	 */
+	for (clk = ATCPIT100_CLK_EXT; clk < NUM_ATCPIT100_CLK; clk++) {
+		if (ap->clk_rate[clk] == 0)
+			continue;
+
+		max_period =
+			DIV64_U64_ROUND_UP((ATCPIT100_CYCLE_MAX + 1) *
+					   NSEC_PER_SEC, ap->clk_rate[clk]) - 1;
+		min_period =
+			DIV64_U64_ROUND_UP(ATCPIT100_CYCLE_MIN * NSEC_PER_SEC,
+					   ap->clk_rate[clk]);
+
+		if (ATCPIT100_IS_VALID_PERIOD(high_period) &&
+		    ATCPIT100_IS_VALID_PERIOD(low_period))
+			break;
+	}
+
+	if (clk == NUM_ATCPIT100_CLK)
+		return -EINVAL;
+
+	/*
+	 * Once changing the clock source here, the output will be neither the
+	 * old one nor the new one until writing to the reload register.
+	 */
+	ctrl_val |= clk ? ATCPIT100_CHANNEL_CTRL_CLK : 0;
+	ret = regmap_update_bits(ap->regmap, ATCPIT100_CHANNEL_CTRL(channel),
+				 ATCPIT100_CHANNEL_CTRL_MASK, ctrl_val);
+	if (ret)
+		return ret;
+
+	high_cycle = mul_u64_u64_div_u64(ap->clk_rate[clk], high_period,
+					 NSEC_PER_SEC);
+	low_cycle = mul_u64_u64_div_u64(ap->clk_rate[clk], low_period,
+					NSEC_PER_SEC);
+	reload_val = FIELD_PREP(ATCPIT100_CHANNEL_RELOAD_HIGH, high_cycle - 1) |
+		     FIELD_PREP(ATCPIT100_CHANNEL_RELOAD_LOW, low_cycle - 1);
+
+	return regmap_write(ap->regmap, ATCPIT100_CHANNEL_RELOAD(channel),
+			    reload_val);
+}
+
+static int atcpit100_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			       const struct pwm_state *state)
+{
+	int ret;
+	unsigned int channel = pwm->hwpwm;
+
+	/* ATCPIT100 PWM driver now only supports normal polarity. */
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (!state->enabled) {
+		if (pwm->state.enabled)
+			return atcpit100_pwm_enable(chip, channel, false);
+
+		return 0;
+	}
+
+	ret = atcpit100_pwm_config(chip, channel, state);
+	if (ret)
+		return ret;
+
+	return atcpit100_pwm_enable(chip, channel, true);
+}
+
+static int atcpit100_pwm_get_state(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   struct pwm_state *state)
+{
+	int clk;
+	int ret;
+	unsigned int ctrl_val;
+	unsigned int reload_val;
+	u16 pwm_high;
+	u16 pwm_low;
+	unsigned int channel = pwm->hwpwm;
+	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
+
+	ret = regmap_read(ap->regmap, ATCPIT100_CHANNEL_CTRL(channel),
+			  &ctrl_val);
+	if (ret)
+		return ret;
+
+	clk = (ctrl_val & ATCPIT100_CHANNEL_CTRL_CLK) ? ATCPIT100_CLK_APB
+						      : ATCPIT100_CLK_EXT;
+	state->enabled =
+		regmap_test_bits(ap->regmap, ATCPIT100_CHANNEL_ENABLE,
+				 ATCPIT100_CHANNEL_ENABLE_PWM(channel));
+	state->polarity = PWM_POLARITY_NORMAL;
+	ret = regmap_read(ap->regmap, ATCPIT100_CHANNEL_RELOAD(channel),
+			  &reload_val);
+	if (ret)
+		return ret;
+
+	pwm_high = FIELD_GET(ATCPIT100_CHANNEL_RELOAD_HIGH, reload_val);
+	pwm_low = FIELD_GET(ATCPIT100_CHANNEL_RELOAD_LOW, reload_val);
+	state->duty_cycle =
+		DIV64_U64_ROUND_UP((pwm_high + 1) * NSEC_PER_SEC,
+				   ap->clk_rate[clk]);
+	state->period =
+		state->duty_cycle +
+		DIV64_U64_ROUND_UP((pwm_low + 1) * NSEC_PER_SEC,
+				   ap->clk_rate[clk]);
+
+	return 0;
+}
+
+static const struct pwm_ops atcpit_pwm_ops = {
+	.apply = atcpit100_pwm_apply,
+	.get_state = atcpit100_pwm_get_state,
+};
+
+static int atcpit100_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct atcpit100_pwm *ap;
+	struct pwm_chip *chip;
+	void __iomem *base;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, ATCPIT100_CHANNEL_MAX, sizeof(*ap));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	ap = to_atcpit100_pwm(chip);
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	ap->ext_clk = devm_clk_get_enabled(dev, "ext");
+	ap->clk_rate[ATCPIT100_CLK_EXT] = ap->ext_clk ?
+					  clk_get_rate(ap->ext_clk) : 0;
+	ap->apb_clk = devm_clk_get_enabled(dev, "apb");
+	ap->clk_rate[ATCPIT100_CLK_APB] = ap->apb_clk ?
+					  clk_get_rate(ap->apb_clk) : 0;
+	if (IS_ERR(ap->ext_clk) && IS_ERR(ap->apb_clk)) {
+		return dev_err_probe(dev, PTR_ERR(ap->ext_clk),
+				     "failed to obtain any clock\n");
+	}
+
+	if (ap->clk_rate[ATCPIT100_CLK_EXT] > NSEC_PER_SEC ||
+	    ap->clk_rate[ATCPIT100_CLK_APB] > NSEC_PER_SEC)
+		return dev_err_probe(dev, -EINVAL, "pwm clock out of range\n");
+
+	ap->regmap = devm_regmap_init_mmio(dev, base,
+					   &atcpit100_pwm_regmap_config);
+	if (IS_ERR(ap->regmap)) {
+		return dev_err_probe(dev, PTR_ERR(ap->regmap),
+				     "failed to init register map\n");
+	}
+
+	chip->ops = &atcpit_pwm_ops;
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add pwm chip\n");
+
+	dev_info(dev, "pwm driver probed\n");
+	return 0;
+}
+
+static const struct of_device_id atcpit100_pwm_dt[] = {
+	{ .compatible = "andestech,atcpit100" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, atcpit100_pwm_dt);
+
+static struct platform_driver atcpit100_pwm_driver = {
+	.driver = {
+		.name = "atcpit100-pwm",
+		.of_match_table = atcpit100_pwm_dt,
+	},
+	.probe = atcpit100_pwm_probe,
+};
+module_platform_driver(atcpit100_pwm_driver);
+
+MODULE_AUTHOR("Ben Zong-You Xie <ben717@andestech.com>");
+MODULE_DESCRIPTION("Andes ATCPIT100 PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [v3 2/2] pwm: atcpit100: add Andes PWM driver support

[Krzysztof Kozlowski]

On 23/01/2025 20:35, Ben Zong-You Xie wrote:
>  
> +config PWM_ATCPIT100
> +	tristate "Andes ATCPIT100 PWM support"
> +	depends on OF && HAS_IOMEM
> +	depends on RISCV || COMPILE_TEST
> +	select REGMAP_MMIO
> +	help
> +	  Generic PWM framework driver for ATCPIT100 on Andes AE350 platform


Is AE350 a type of a SoC? Looks like. "depends on RISCV" is wrong -
there is nothing RISC-V specific here. You must depend on given
SoC/platform.

> +
> +	  The ATCPIT100 Programmable Interval Timer (PIT) is a set of compact
> +	  multi-function timers, which can be used as pulse width
> +	  modulators (PWM) as well as simple timers. ATCPIT100 supports up to 4
> +	  PIT channels. Each PIT channel can be a simple timer or PWM, or a
> +	  combination of timer and PWM.
> +
> +	  To compile this driver as a module, choose M here: the module


...

> +static int atcpit100_pwm_get_state(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   struct pwm_state *state)
> +{
> +	int clk;
> +	int ret;
> +	unsigned int ctrl_val;
> +	unsigned int reload_val;
> +	u16 pwm_high;
> +	u16 pwm_low;
> +	unsigned int channel = pwm->hwpwm;
> +	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
> +
> +	ret = regmap_read(ap->regmap, ATCPIT100_CHANNEL_CTRL(channel),
> +			  &ctrl_val);
> +	if (ret)
> +		return ret;
> +
> +	clk = (ctrl_val & ATCPIT100_CHANNEL_CTRL_CLK) ? ATCPIT100_CLK_APB
> +						      : ATCPIT100_CLK_EXT;
> +	state->enabled =

Don't wrap here...
> +		regmap_test_bits(ap->regmap, ATCPIT100_CHANNEL_ENABLE,

but wrap at arguments.

> +				 ATCPIT100_CHANNEL_ENABLE_PWM(channel));
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	ret = regmap_read(ap->regmap, ATCPIT100_CHANNEL_RELOAD(channel),
> +			  &reload_val);
> +	if (ret)
> +		return ret;
> +
> +	pwm_high = FIELD_GET(ATCPIT100_CHANNEL_RELOAD_HIGH, reload_val);
> +	pwm_low = FIELD_GET(ATCPIT100_CHANNEL_RELOAD_LOW, reload_val);
> +	state->duty_cycle =
> +		DIV64_U64_ROUND_UP((pwm_high + 1) * NSEC_PER_SEC,
> +				   ap->clk_rate[clk]);
> +	state->period =
> +		state->duty_cycle +
> +		DIV64_U64_ROUND_UP((pwm_low + 1) * NSEC_PER_SEC,
> +				   ap->clk_rate[clk]);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops atcpit_pwm_ops = {
> +	.apply = atcpit100_pwm_apply,
> +	.get_state = atcpit100_pwm_get_state,
> +};
> +
> +static int atcpit100_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct atcpit100_pwm *ap;
> +	struct pwm_chip *chip;
> +	void __iomem *base;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, ATCPIT100_CHANNEL_MAX, sizeof(*ap));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	ap = to_atcpit100_pwm(chip);
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	ap->ext_clk = devm_clk_get_enabled(dev, "ext");
> +	ap->clk_rate[ATCPIT100_CLK_EXT] = ap->ext_clk ?
> +					  clk_get_rate(ap->ext_clk) : 0;
> +	ap->apb_clk = devm_clk_get_enabled(dev, "apb");
> +	ap->clk_rate[ATCPIT100_CLK_APB] = ap->apb_clk ?
> +					  clk_get_rate(ap->apb_clk) : 0;
> +	if (IS_ERR(ap->ext_clk) && IS_ERR(ap->apb_clk)) {

Drop {}. See Linux coding style.

> +		return dev_err_probe(dev, PTR_ERR(ap->ext_clk),
> +				     "failed to obtain any clock\n");
> +	}
> +
> +	if (ap->clk_rate[ATCPIT100_CLK_EXT] > NSEC_PER_SEC ||
> +	    ap->clk_rate[ATCPIT100_CLK_APB] > NSEC_PER_SEC)
> +		return dev_err_probe(dev, -EINVAL, "pwm clock out of range\n");
> +
> +	ap->regmap = devm_regmap_init_mmio(dev, base,
> +					   &atcpit100_pwm_regmap_config);
> +	if (IS_ERR(ap->regmap)) {

Drop {}. See Linux coding style.


> +		return dev_err_probe(dev, PTR_ERR(ap->regmap),
> +				     "failed to init register map\n");
> +	}
> +
> +	chip->ops = &atcpit_pwm_ops;
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add pwm chip\n");
> +
> +	dev_info(dev, "pwm driver probed\n");


Drop all such "success" messages.


Best regards,
Krzysztof

Re: [v3 2/2] pwm: atcpit100: add Andes PWM driver support

[Ben Zong-You Xie]

On Fri, Jan 24, 2025 at 08:30:48AM +0100, Krzysztof Kozlowski wrote:
> [EXTERNAL MAIL]
> 
> On 23/01/2025 20:35, Ben Zong-You Xie wrote:
> >
> > +config PWM_ATCPIT100
> > +     tristate "Andes ATCPIT100 PWM support"
> > +     depends on OF && HAS_IOMEM
> > +     depends on RISCV || COMPILE_TEST
> > +     select REGMAP_MMIO
> > +     help
> > +       Generic PWM framework driver for ATCPIT100 on Andes AE350 platform
> 
> 
> Is AE350 a type of a SoC? Looks like. "depends on RISCV" is wrong -
> there is nothing RISC-V specific here. You must depend on given
> SoC/platform.
> 

Hi Krzysztof,

AE350 is not a SoC. It's just a reference platform to verify Andes CPUs
on FPGA. For further information on AE350, please refer to [1].

Also, I will remove "depends on RISCV" and fix the coding style problems
in the next patch. Thanks for your review.

[1] https://www.andestech.com/en/products-solutions/andeshape-platforms/ae350-axi-based-platform-pre-integrated-with-n25f-nx25f-a25-ax25/

Best regards,
Ben

> > +
> > +       The ATCPIT100 Programmable Interval Timer (PIT) is a set of compact
> > +       multi-function timers, which can be used as pulse width
> > +       modulators (PWM) as well as simple timers. ATCPIT100 supports up to 4
> > +       PIT channels. Each PIT channel can be a simple timer or PWM, or a
> > +       combination of timer and PWM.
> > +
> > +       To compile this driver as a module, choose M here: the module
> 
> 
> ...
> 

Re: [v3 2/2] pwm: atcpit100: add Andes PWM driver support

[Uwe Kleine-König]

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

Hello Ben,

On Thu, Feb 06, 2025 at 04:08:58PM +0800, Ben Zong-You Xie wrote:
> Also, I will remove "depends on RISCV" [...]

Krzysztof didn't say so explicitly, but I think he wanted the dependency
to be more restrictive, not less. At least that would be my concern
here.

The rational is: Don't annoy people who work on ARM or x86 with
questions about this driver that they don't need anyhow. If there is no
stricter dependency possible, I'm happy with

	depends on RISCV || COMPILE_TEST

. If however your driver is implemented in an FPGA, adding a dependency
on a set of SoCs that have the known FPGA that provides that device
would be a good idea.

Best regards
Uwe

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

Re: [v3 2/2] pwm: atcpit100: add Andes PWM driver support

[Krzysztof Kozlowski]

On 06/02/2025 09:08, Ben Zong-You Xie wrote:
> On Fri, Jan 24, 2025 at 08:30:48AM +0100, Krzysztof Kozlowski wrote:
>> [EXTERNAL MAIL]
>>
>> On 23/01/2025 20:35, Ben Zong-You Xie wrote:
>>>
>>> +config PWM_ATCPIT100
>>> +     tristate "Andes ATCPIT100 PWM support"
>>> +     depends on OF && HAS_IOMEM
>>> +     depends on RISCV || COMPILE_TEST
>>> +     select REGMAP_MMIO
>>> +     help
>>> +       Generic PWM framework driver for ATCPIT100 on Andes AE350 platform
>>
>>
>> Is AE350 a type of a SoC? Looks like. "depends on RISCV" is wrong -
>> there is nothing RISC-V specific here. You must depend on given
>> SoC/platform.
>>
> 
> Hi Krzysztof,
> 
> AE350 is not a SoC. It's just a reference platform to verify Andes CPUs
> on FPGA. For further information on AE350, please refer to [1].

Then what is the SoC?

> 
> Also, I will remove "depends on RISCV" and fix the coding style problems

No, read the review again and implement it:

"You must depend on given SoC/platform."



Best regards,
Krzysztof