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

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

[Ben Zong-You Xie]

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.

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

Ben Zong-You Xie (2):
  dt-bindings: pwm: add atcpit100-pwm
  pwm: atcpit100: add Andes PWM driver support

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

-- 
2.34.1

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

[Ben Zong-You Xie]

Document devicetree bindings for Andes atcpit100-pwm.

Signed-off-by: Ben Zong-You Xie <ben717@andestech.com>
---
 .../bindings/pwm/andestech,atcpit100-pwm.yaml | 52 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 57 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..6952663f134b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/andestech,atcpit100-pwm.yaml
@@ -0,0 +1,52 @@
+# 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: PWM driver for Andes atcpit100
+
+maintainers:
+  - Ben Zong-You Xie <ben717@andestech.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: andestech,atcpit100-pwm
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+  andestech,clock-source:
+    description:
+      Clock Source for each PIT channel.
+      0 - external clock
+      1 - APB clock.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 4
+    maxItems: 4
+    items:
+      minimum: 0
+      maximum: 1
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - andestech,clock-source
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    pwm@f0400000 {
+        compatible = "andestech,atcpit100-pwm";
+        reg = <0xf0400000 0x1000>;
+        #pwm-cells = <2>;
+        andestech,clock-source = <0 1 0 0>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index a506fa4f6825..ebbc7edcf077 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3551,6 +3551,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

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

[Ben Zong-You Xie]

Add PWM driver suuport for Andes atcpit100.

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 | 240 ++++++++++++++++++++++++++++++++++++
 4 files changed, 259 insertions(+)
 create mode 100644 drivers/pwm/pwm-atcpit100.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ebbc7edcf077..39c6e1f21339 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3555,6 +3555,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..cf83e8702d60
--- /dev/null
+++ b/drivers/pwm/pwm-atcpit100.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#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_MAX			4
+#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 CLK_EXTERNAL				32768
+#define CLK_APB					60000000
+#define CYCLE_MIN				0x01
+#define CYCLE_MAX				0x010000
+
+struct atcpit100_pwm {
+	struct regmap *regmap;
+	u32 clk_src[ATCPIT100_CHANNEL_MAX];
+};
+
+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 of_atcpit100_pwm_set_clk_src(struct atcpit100_pwm *ap,
+					struct device_node *np)
+{
+	int ret;
+
+	for (int i = 0; i < ATCPIT100_CHANNEL_MAX; i++) {
+		ret = of_property_read_u32_index(np, "andestech,clock-source",
+						 i, &(ap->clk_src[i]));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int atcpit100_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
+				bool enable)
+{
+	unsigned int channel = pwm->hwpwm;
+	unsigned int enable_bit = ATCPIT100_CHANNEL_ENABLE_PWM(channel);
+	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
+
+	pwm->state.enabled = enable;
+	return regmap_update_bits(ap->regmap, ATCPIT100_CHANNEL_ENABLE,
+				  enable_bit, enable ? enable_bit : 0);
+}
+
+static int atcpit100_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{
+	int ret;
+	u64 period_cycle;
+	u64 duty_cycle;
+	u16 pwm_high;
+	u16 pwm_low;
+	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
+	unsigned int ctrl_val = 0;
+	unsigned int channel = pwm->hwpwm;
+	u64 rate = ap->clk_src[channel] ? CLK_APB : CLK_EXTERNAL;
+
+	/* cycle count = clock rate * time */
+	period_cycle = mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC);
+	duty_cycle = mul_u64_u64_div_u64(rate, state->duty_cycle,
+					 NSEC_PER_SEC);
+	if (period_cycle < CYCLE_MIN || period_cycle > CYCLE_MAX ||
+	    duty_cycle < CYCLE_MIN || duty_cycle > CYCLE_MAX) {
+		dev_err(pwmchip_parent(chip),
+			"channel%d: period cycles = 0x%llx, duty cycles = 0x%llx\n",
+			channel, period_cycle, duty_cycle);
+		return -EINVAL;
+	}
+
+	/*
+	 * In the PWM mode, the high period is (PWM16_Hi + 1) cycles, and the
+	 * low period is (PWM16_Lo + 1) cycles.
+	 * For example, if period is 30 cycles and duty_cycle is 10 cycles,
+	 * PWM16_Hi = 10 - 1 = 9, PWM16_Lo = 30 - 10 - 1 = 19.
+	 */
+	pwm_high = duty_cycle - 1;
+	pwm_low = period_cycle - duty_cycle - 1;
+
+	/* Set control register. */
+	ctrl_val |= ATCPIT100_CHANNEL_CTRL_MODE_PWM;
+	ctrl_val |= ap->clk_src[channel] ? 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;
+
+	/* Set reload register. */
+	ret = regmap_write(ap->regmap, ATCPIT100_CHANNEL_RELOAD(channel),
+			   (pwm_high << 16) | pwm_low);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int atcpit100_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			       const struct pwm_state *state)
+{
+	int ret;
+
+	/* ATCPIT100 PWM driver now only supports normal polarity. */
+	if (state->polarity != PWM_POLARITY_NORMAL) {
+		dev_err(pwmchip_parent(chip),
+			"only supports normal polarity now\n");
+		return -EINVAL;
+	}
+
+	if (!state->enabled) {
+		if (pwm->state.enabled)
+			return atcpit100_pwm_enable(chip, pwm, 0);
+
+		return 0;
+	}
+
+	ret = atcpit100_pwm_config(chip, pwm, state);
+	if (ret)
+		return ret;
+
+	return atcpit100_pwm_enable(chip, pwm, 1);
+}
+
+static int atcpit100_pwm_get_state(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   struct pwm_state *state)
+{
+	int ret;
+	unsigned int reload_val;
+	u16 pwm_high;
+	u16 pwm_low;
+	unsigned int channel = pwm->hwpwm;
+	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
+	u64 rate = ap->clk_src[channel] ? CLK_APB : CLK_EXTERNAL;
+
+	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 = reload_val >> 16;
+	pwm_low = reload_val & 0xFFFF;
+	state->duty_cycle = mul_u64_u64_div_u64(pwm_high + 1, NSEC_PER_SEC,
+						rate);
+	state->period = mul_u64_u64_div_u64(pwm_low + pwm_high + 1,
+					    NSEC_PER_SEC, rate);
+
+	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 device_node *np = dev->of_node;
+	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);
+
+	/*
+	 * Each channel can select two different clock sources by toggling the
+	 * third bit in its control register. 0 means using an external clock,
+	 * and 1 means using APB clock from APB bus. Select the clock source for
+	 * each channel by DTS.
+	 */
+	ret = of_atcpit100_pwm_set_clk_src(ap, np);
+	if (ret)
+		return ret;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	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");
+
+	return 0;
+}
+
+static const struct of_device_id atcpit100_pwm_dt[] = {
+	{ .compatible = "andestech,atcpit100-pwm" },
+	{ /* 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("Andes Technology Corporation <ben717@andestech.com>");
+MODULE_DESCRIPTION("Andes ATCPIT100 PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

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

[Uwe Kleine-König]

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

On Mon, Oct 28, 2024 at 06:27:20PM +0800, Ben Zong-You Xie wrote:
> +  "#pwm-cells":
> +    const: 2

Please use 3 here.

> +  andestech,clock-source:
> +    description:
> +      Clock Source for each PIT channel.
> +      0 - external clock
> +      1 - APB clock.

Is this something that could better be represented by
assigned-clock-parents?

Best regards
Uwe

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

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

[Uwe Kleine-König]

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

On Mon, Oct 28, 2024 at 06:27:21PM +0800, Ben Zong-You Xie wrote:
> Add PWM driver suuport for Andes atcpit100.

s/uup/upp/

> 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..cf83e8702d60
> --- /dev/null
> +++ b/drivers/pwm/pwm-atcpit100.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Please add a comment here to list hardware limitations (such that the
gist is available from the output in

	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c

. Check how other newer driver do it.) Questions to answer there (at
least) include:

 - How does the hardware behave when disabled? (Typical: HighZ, constant
   0, constant inactive)
 - Are there glitches during reconfiguration?

> +#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_MAX			4
> +#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 CLK_EXTERNAL				32768
> +#define CLK_APB					60000000
> +#define CYCLE_MIN				0x01
> +#define CYCLE_MAX				0x010000

Please give these last 4 constants a name that makes it clear that they
belong to this driver. I.e. use "ATCPIT100_" as prefix.

CYCLE_MIN is one because you have to write numcycles - 1 to the
respective register, right? I would have use a plain 1 in the
implementation then.

> +struct atcpit100_pwm {
> +	struct regmap *regmap;
> +	u32 clk_src[ATCPIT100_CHANNEL_MAX];
> +};
> +
> +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 of_atcpit100_pwm_set_clk_src(struct atcpit100_pwm *ap,
> +					struct device_node *np)
> +{
> +	int ret;
> +
> +	for (int i = 0; i < ATCPIT100_CHANNEL_MAX; i++) {
> +		ret = of_property_read_u32_index(np, "andestech,clock-source",
> +						 i, &(ap->clk_src[i]));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

Instead of having that statically configured in the dtb, it would be
beneficial to switch the parent depending on the requested setting.

> +static int atcpit100_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> +				bool enable)
> +{
> +	unsigned int channel = pwm->hwpwm;
> +	unsigned int enable_bit = ATCPIT100_CHANNEL_ENABLE_PWM(channel);
> +	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
> +
> +	pwm->state.enabled = enable;

The pwm core cares for that. Please drop that assignment.

> +	return regmap_update_bits(ap->regmap, ATCPIT100_CHANNEL_ENABLE,
> +				  enable_bit, enable ? enable_bit : 0);
> +}
> +
> +static int atcpit100_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +				const struct pwm_state *state)
> +{
> +	int ret;
> +	u64 period_cycle;
> +	u64 duty_cycle;
> +	u16 pwm_high;
> +	u16 pwm_low;
> +	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
> +	unsigned int ctrl_val = 0;
> +	unsigned int channel = pwm->hwpwm;
> +	u64 rate = ap->clk_src[channel] ? CLK_APB : CLK_EXTERNAL;

Huh, I would have expected a call to clk_get_rate() here.

> +	/* cycle count = clock rate * time */
> +	period_cycle = mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC);
> +	duty_cycle = mul_u64_u64_div_u64(rate, state->duty_cycle,
> +					 NSEC_PER_SEC);
> +	if (period_cycle < CYCLE_MIN || period_cycle > CYCLE_MAX ||
> +	    duty_cycle < CYCLE_MIN || duty_cycle > CYCLE_MAX) {
> +		dev_err(pwmchip_parent(chip),
> +			"channel%d: period cycles = 0x%llx, duty cycles = 0x%llx\n",
> +			channel, period_cycle, duty_cycle);
> +		return -EINVAL;
> +	}

Don't error out on period_cycle > CYCLE_MAX or duty_cycle > CYCLE_MAX.
Just continue with period_cycle = CYCLE_MAX (and duty_cycle = CYCLE_MAX
resp.).

> +	/*
> +	 * In the PWM mode, the high period is (PWM16_Hi + 1) cycles, and the
> +	 * low period is (PWM16_Lo + 1) cycles.
> +	 * For example, if period is 30 cycles and duty_cycle is 10 cycles,
> +	 * PWM16_Hi = 10 - 1 = 9, PWM16_Lo = 30 - 10 - 1 = 19.
> +	 */
> +	pwm_high = duty_cycle - 1;
> +	pwm_low = period_cycle - duty_cycle - 1;

If period_cycle == duty_cycle surprising things happen?

I guess the hardware can neither do a 0% nor a 100% relative duty cycle?
That's something to document in the above mentioned Limitations
paragraph.

> +
> +	/* Set control register. */
> +	ctrl_val |= ATCPIT100_CHANNEL_CTRL_MODE_PWM;
> +	ctrl_val |= ap->clk_src[channel] ? 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;

What happens to the output here? I guess it might already change and so
it might emit a wave form that is neither the old nor the new one?
(=> Limitations)

> +
> +	/* Set reload register. */
> +	ret = regmap_write(ap->regmap, ATCPIT100_CHANNEL_RELOAD(channel),
> +			   (pwm_high << 16) | pwm_low);

Please define proper bitfield accessors. E.g.

	ATCPIT100_CHANNEL_RELOAD_HIGH	GENMASK(31, 16)
	...

and then use FIELD_PREP to assign.

> +	if (ret)
> +		return ret;
> +
> +	return 0;

This can be abbreviated to

	return regmap_write(....);

> +}
> +
> +static int atcpit100_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       const struct pwm_state *state)
> +{
> +	int ret;
> +
> +	/* ATCPIT100 PWM driver now only supports normal polarity. */
> +	if (state->polarity != PWM_POLARITY_NORMAL) {
> +		dev_err(pwmchip_parent(chip),
> +			"only supports normal polarity now\n");

No error message in .apply() please. Returning an error code is enough.

> +		return -EINVAL;
> +	}
> +
> +	if (!state->enabled) {
> +		if (pwm->state.enabled)
> +			return atcpit100_pwm_enable(chip, pwm, 0);
> +
> +		return 0;
> +	}
> +
> +	ret = atcpit100_pwm_config(chip, pwm, state);
> +	if (ret)
> +		return ret;
> +
> +	return atcpit100_pwm_enable(chip, pwm, 1);
> +}
> +
> +static int atcpit100_pwm_get_state(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   struct pwm_state *state)
> +{
> +	int ret;
> +	unsigned int reload_val;
> +	u16 pwm_high;
> +	u16 pwm_low;
> +	unsigned int channel = pwm->hwpwm;
> +	struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
> +	u64 rate = ap->clk_src[channel] ? CLK_APB : CLK_EXTERNAL;
> +
> +	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 = reload_val >> 16;
> +	pwm_low = reload_val & 0xFFFF;
> +	state->duty_cycle = mul_u64_u64_div_u64(pwm_high + 1, NSEC_PER_SEC,
> +						rate);
> +	state->period = mul_u64_u64_div_u64(pwm_low + pwm_high + 1,
> +					    NSEC_PER_SEC, rate);
> +

Please enable PWM_DEBUG and test extensively. Hint: You have to round up
here.

> +	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 device_node *np = dev->of_node;
> +	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);
> +
> +	/*
> +	 * Each channel can select two different clock sources by toggling the
> +	 * third bit in its control register. 0 means using an external clock,
> +	 * and 1 means using APB clock from APB bus. Select the clock source for
> +	 * each channel by DTS.
> +	 */
> +	ret = of_atcpit100_pwm_set_clk_src(ap, np);
> +	if (ret)
> +		return ret;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	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");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id atcpit100_pwm_dt[] = {
> +	{ .compatible = "andestech,atcpit100-pwm" },
> +	{ /* 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);

empty new line here

> +MODULE_AUTHOR("Andes Technology Corporation <ben717@andestech.com>");

I would have expected your name here given that's also your email
address.

> +MODULE_DESCRIPTION("Andes ATCPIT100 PWM driver");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

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

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

[Krzysztof Kozlowski]

On 28/10/2024 11:27, Ben Zong-You Xie wrote:
> +---
> +$id: http://devicetree.org/schemas/pwm/andestech,atcpit100-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: PWM driver for Andes atcpit100

Driver as Linux driver?
> +
> +maintainers:
> +  - Ben Zong-You Xie <ben717@andestech.com>
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: andestech,atcpit100-pwm
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +  andestech,clock-source:
> +    description:
> +      Clock Source for each PIT channel.
> +      0 - external clock
> +      1 - APB clock.

You don't take APB clock here... neither external. Where is the clocks
property?

Why assigned-clocks cannot work here?


> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 4
> +    maxItems: 4
> +    items:
> +      minimum: 0
> +      maximum: 1

Best regards,
Krzysztof