[PATCH v3 0/2] pwm: Introduce pwm driver for the Loongson family chips

[PATCH v3 0/2] pwm: Introduce pwm driver for the Loongson family chips

[Binbin Zhou]

Hi all:

This patchset introduce a generic PWM framework driver for Loongson family.
Each PWM has one pulse width output signal and one pulse input signal to be measured.

It can be found on Loongson-2K series cpus and Loongson LS7A bridge chips.

Thanks.

-------
V3:
patch (1/2):
 - Add Reviewed-by tag from Krzysztof, thanks.
patch (2/2):
 - Several code stlye adjustments, such as line breaks.

Link to V2:
https://lore.kernel.org/all/cover.1712732719.git.zhoubinbin@loongson.cn/

v2:
- Remove the dts-related patches and update dts at once after all
relevant drivers are complete.
patch (1/2):
 - The dt-binding filename should match compatible, rename it as
   loongson,ls7a-pwm.yaml;
 - Update binding description;
 - Add description for each pwm cell;
 - Drop '#pwm-cells' from required, for pwm.yaml makes it required already.

Link to v1:
https://lore.kernel.org/linux-pwm/cover.1711953223.git.zhoubinbin@loongson.cn/

Binbin Zhou (2):
  dt-bindings: pwm: Add Loongson PWM controller
  pwm: Add Loongson PWM controller support

 .../bindings/pwm/loongson,ls7a-pwm.yaml       |  66 ++++
 MAINTAINERS                                   |   7 +
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-loongson.c                    | 298 ++++++++++++++++++
 5 files changed, 382 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
 create mode 100644 drivers/pwm/pwm-loongson.c

-- 
2.43.0

[PATCH v3 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Binbin Zhou]

Add Loongson PWM controller binding with DT schema format using
json-schema.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/pwm/loongson,ls7a-pwm.yaml       | 66 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml b/Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
new file mode 100644
index 000000000000..46814773e0cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/loongson,ls7a-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson PWM Controller
+
+maintainers:
+  - Binbin Zhou <zhoubinbin@loongson.cn>
+
+description:
+  The Loongson PWM has one pulse width output signal and one pulse input
+  signal to be measured.
+  It can be found on Loongson-2K series cpus and Loongson LS7A bridge chips.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - const: loongson,ls7a-pwm
+      - items:
+          - enum:
+              - loongson,ls2k0500-pwm
+              - loongson,ls2k1000-pwm
+              - loongson,ls2k2000-pwm
+          - const: loongson,ls7a-pwm
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  '#pwm-cells':
+    description:
+      The first cell must have a value of 0, which specifies the PWM output signal;
+      The second cell is the period in nanoseconds;
+      The third cell flag supported by this binding is PWM_POLARITY_INVERTED.
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/loongson,ls2k-clk.h>
+    pwm@1fe22000 {
+        compatible = "loongson,ls2k1000-pwm", "loongson,ls7a-pwm";
+        reg = <0x1fe22000 0x10>;
+        interrupt-parent = <&liointc0>;
+        interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clk LOONGSON2_APB_CLK>;
+        #pwm-cells = <3>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index c23fda1aa1f0..ecef2744726d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12751,6 +12751,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
 F:	drivers/i2c/busses/i2c-ls2x.c
 
+LOONGSON PWM DRIVER
+M:	Binbin Zhou <zhoubinbin@loongson.cn>
+L:	linux-pwm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
+
 LOONGSON-2 SOC SERIES CLOCK DRIVER
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-clk@vger.kernel.org
-- 
2.43.0

[PATCH v3 2/2] pwm: Add Loongson PWM controller support

[Binbin Zhou]

This commit adds a generic PWM framework driver for the PWM controller
found on Loongson family chips.

Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 MAINTAINERS                |   1 +
 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-loongson.c | 298 +++++++++++++++++++++++++++++++++++++
 4 files changed, 310 insertions(+)
 create mode 100644 drivers/pwm/pwm-loongson.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ecef2744726d..d32da7c77f0e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12756,6 +12756,7 @@ M:	Binbin Zhou <zhoubinbin@loongson.cn>
 L:	linux-pwm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
+F:	drivers/pwm/pwm-loongson.c
 
 LOONGSON-2 SOC SERIES CLOCK DRIVER
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..bb163c65e5ae 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -324,6 +324,16 @@ config PWM_KEEMBAY
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-keembay.
 
+config PWM_LOONGSON
+	tristate "Loongson PWM support"
+	depends on MACH_LOONGSON64
+	help
+	  Generic PWM framework driver for Loongson family.
+	  It can be found on Loongson-2K series cpu and Loongson LS7A bridge chips.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-loongson.
+
 config PWM_LP3943
 	tristate "TI/National Semiconductor LP3943 PWM support"
 	depends on MFD_LP3943
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..bffa49500277 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
 obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
+obj-$(CONFIG_PWM_LOONGSON)	+= pwm-loongson.o
 obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
 obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
new file mode 100644
index 000000000000..5ac79a69acd3
--- /dev/null
+++ b/drivers/pwm/pwm-loongson.c
@@ -0,0 +1,298 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Loongson PWM driver
+ *
+ * Author: Juxin Gao <gaojuxin@loongson.cn>
+ * Further cleanup and restructuring by:
+ *         Binbin Zhou <zhoubinbin@loongson.cn>
+ *
+ * Copyright (C) 2017-2024 Loongson Technology Corporation Limited.
+ */
+
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/units.h>
+
+/* Loongson PWM registers */
+#define PWM_DUTY	0x4 /* Low Pulse Buffer Register */
+#define PWM_PERIOD	0x8 /* Pulse Period Buffer Register */
+#define PWM_CTRL	0xc /* Control Register */
+
+/* Control register bits */
+#define PWM_CTRL_EN	BIT(0)  /* Counter Enable Bit */
+#define PWM_CTRL_OE	BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
+#define PWM_CTRL_SINGLE	BIT(4)  /* Single Pulse Control Bit */
+#define PWM_CTRL_INTE	BIT(5)  /* Interrupt Enable Bit */
+#define PWM_CTRL_INT	BIT(6)  /* Interrupt Bit */
+#define PWM_CTRL_RST	BIT(7)  /* Counter Reset Bit */
+#define PWM_CTRL_CAPTE	BIT(8)  /* Measurement Pulse Enable Bit */
+#define PWM_CTRL_INVERT	BIT(9)  /* Output flip-flop Enable Bit */
+#define PWM_CTRL_DZONE	BIT(10) /* Anti-dead Zone Enable Bit */
+
+#define PWM_FREQ_STD       (50 * HZ_PER_KHZ)
+
+struct pwm_loongson_ddata {
+	struct pwm_chip	chip;
+	struct clk	*clk;
+	void __iomem	*base;
+	/* The following for PM */
+	u32		ctrl;
+	u32		duty;
+	u32		period;
+};
+
+static inline struct pwm_loongson_ddata *to_pwm_loongson_ddata(struct pwm_chip *chip)
+{
+	return container_of(chip, struct pwm_loongson_ddata, chip);
+}
+
+static inline u32 pwm_loongson_readl(struct pwm_loongson_ddata *ddata, u64 offset)
+{
+	return readl(ddata->base + offset);
+}
+
+static inline void pwm_loongson_writel(struct pwm_loongson_ddata *ddata,
+				       u32 val, u64 offset)
+{
+	writel(val, ddata->base + offset);
+}
+
+static int pwm_loongson_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				     enum pwm_polarity polarity)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	u16 val;
+
+	val = pwm_loongson_readl(ddata, PWM_CTRL);
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		/* Duty cycle defines LOW period of PWM */
+		val |= PWM_CTRL_INVERT;
+	else
+		/* Duty cycle defines HIGH period of PWM */
+		val &= ~PWM_CTRL_INVERT;
+
+	pwm_loongson_writel(ddata, val, PWM_CTRL);
+
+	return 0;
+}
+
+static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	u32 val;
+
+	if (pwm->state.polarity == PWM_POLARITY_NORMAL)
+		pwm_loongson_writel(ddata, ddata->period, PWM_DUTY);
+	else if (pwm->state.polarity == PWM_POLARITY_INVERSED)
+		pwm_loongson_writel(ddata, 0, PWM_DUTY);
+
+	val = pwm_loongson_readl(ddata, PWM_CTRL);
+	val &= ~PWM_CTRL_EN;
+	pwm_loongson_writel(ddata, val, PWM_CTRL);
+}
+
+static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	u32 val;
+
+	pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
+	pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
+
+	val = pwm_loongson_readl(ddata, PWM_CTRL);
+	val |= PWM_CTRL_EN;
+	pwm_loongson_writel(ddata, val, PWM_CTRL);
+
+	return 0;
+}
+
+static u32 pwm_loongson_set_config(struct pwm_loongson_ddata *ddata, int ns,
+				   u64 clk_rate, u64 offset)
+{
+	u32 val;
+	u64 c;
+
+	c = clk_rate * ns;
+	do_div(c, NSEC_PER_SEC);
+	val = c < 1 ? 1 : c;
+
+	pwm_loongson_writel(ddata, val, offset);
+
+	return val;
+}
+
+static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			       int duty_ns, int period_ns)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	struct device *dev = chip->dev;
+	u64 clk_rate;
+
+	if (duty_ns > NANOHZ_PER_HZ || period_ns > NANOHZ_PER_HZ)
+		return -ERANGE;
+
+	clk_rate = has_acpi_companion(dev) ? PWM_FREQ_STD : clk_get_rate(ddata->clk);
+
+	ddata->duty = pwm_loongson_set_config(ddata, duty_ns, clk_rate, PWM_DUTY);
+	ddata->period = pwm_loongson_set_config(ddata, period_ns, clk_rate, PWM_PERIOD);
+
+	return 0;
+}
+
+static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      const struct pwm_state *state)
+{
+	int err;
+	bool enabled = pwm->state.enabled;
+
+	if (state->polarity != pwm->state.polarity) {
+		if (enabled) {
+			pwm_loongson_disable(chip, pwm);
+			enabled = false;
+		}
+
+		err = pwm_loongson_set_polarity(chip, pwm, state->polarity);
+		if (err)
+			return err;
+	}
+
+	if (!state->enabled) {
+		if (enabled)
+			pwm_loongson_disable(chip, pwm);
+		return 0;
+	}
+
+	err = pwm_loongson_config(chip, pwm, state->duty_cycle, state->period);
+	if (err)
+		return err;
+
+	if (!enabled)
+		err = pwm_loongson_enable(chip, pwm);
+
+	return err;
+}
+
+static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	u32 period, duty, ctrl;
+	u64 ns;
+
+	ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
+	state->polarity = (ctrl & PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	state->enabled = (ctrl & PWM_CTRL_EN) ? true : false;
+
+	duty = pwm_loongson_readl(ddata, PWM_DUTY);
+	ns = duty * NSEC_PER_SEC;
+	state->duty_cycle = do_div(ns, duty);
+
+	period = pwm_loongson_readl(ddata, PWM_PERIOD);
+	ns = period * NSEC_PER_SEC;
+	state->period = do_div(ns, period);
+
+	ddata->ctrl = ctrl;
+	ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
+	ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
+
+	return 0;
+}
+
+static const struct pwm_ops pwm_loongson_ops = {
+	.apply = pwm_loongson_apply,
+	.get_state = pwm_loongson_get_state,
+};
+
+static int pwm_loongson_probe(struct platform_device *pdev)
+{
+	struct pwm_loongson_ddata *ddata;
+	struct device *dev = &pdev->dev;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	ddata->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ddata->base))
+		return PTR_ERR(ddata->base);
+
+	if (!has_acpi_companion(dev)) {
+		ddata->clk = devm_clk_get_enabled(dev, NULL);
+		if (IS_ERR(ddata->clk))
+			return PTR_ERR(ddata->clk);
+	}
+
+	ddata->chip.dev = dev;
+	ddata->chip.ops = &pwm_loongson_ops;
+	ddata->chip.npwm = 1;
+	platform_set_drvdata(pdev, ddata);
+
+	return devm_pwmchip_add(dev, &ddata->chip);
+}
+
+static int pwm_loongson_suspend(struct device *dev)
+{
+	struct pwm_loongson_ddata *ddata = dev_get_drvdata(dev);
+
+	ddata->ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
+	ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
+	ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
+
+	clk_disable_unprepare(ddata->clk);
+
+	return 0;
+}
+
+static int pwm_loongson_resume(struct device *dev)
+{
+	struct pwm_loongson_ddata *ddata = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(ddata->clk);
+	if (ret)
+		return ret;
+
+	pwm_loongson_writel(ddata, ddata->ctrl, PWM_CTRL);
+	pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
+	pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pwm_loongson_pm_ops, pwm_loongson_suspend,
+				pwm_loongson_resume);
+
+static const struct of_device_id pwm_loongson_of_ids[] = {
+	{ .compatible = "loongson,ls7a-pwm" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, pwm_loongson_of_ids);
+
+static const struct acpi_device_id pwm_loongson_acpi_ids[] = {
+	{ "LOON0006" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, pwm_loongson_acpi_ids);
+
+static struct platform_driver pwm_loongson_driver = {
+	.probe  = pwm_loongson_probe,
+	.driver = {
+		.name   = "loongson-pwm",
+		.pm	= pm_ptr(&pwm_loongson_pm_ops),
+		.of_match_table   = pwm_loongson_of_ids,
+		.acpi_match_table = pwm_loongson_acpi_ids,
+	},
+};
+module_platform_driver(pwm_loongson_driver);
+
+MODULE_DESCRIPTION("Loongson PWM driver");
+MODULE_AUTHOR("Loongson Technology Corporation Limited.");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v3 2/2] pwm: Add Loongson PWM controller support

[Uwe Kleine-König]

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

Hello,

sorry for taking so long to get back to your patch. reviewing new
drivers is quite time consuming which makes me often fail to review in a
timely manner.

On Tue, Apr 16, 2024 at 09:55:15AM +0800, Binbin Zhou wrote:
> This commit adds a generic PWM framework driver for the PWM controller
> found on Loongson family chips.
> 
> Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  MAINTAINERS                |   1 +
>  drivers/pwm/Kconfig        |  10 ++
>  drivers/pwm/Makefile       |   1 +
>  drivers/pwm/pwm-loongson.c | 298 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 310 insertions(+)
>  create mode 100644 drivers/pwm/pwm-loongson.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ecef2744726d..d32da7c77f0e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12756,6 +12756,7 @@ M:	Binbin Zhou <zhoubinbin@loongson.cn>
>  L:	linux-pwm@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
> +F:	drivers/pwm/pwm-loongson.c
>  
>  LOONGSON-2 SOC SERIES CLOCK DRIVER
>  M:	Yinbo Zhu <zhuyinbo@loongson.cn>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..bb163c65e5ae 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -324,6 +324,16 @@ config PWM_KEEMBAY
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-keembay.
>  
> +config PWM_LOONGSON
> +	tristate "Loongson PWM support"
> +	depends on MACH_LOONGSON64

Something with || COMPILE_TEST would be nice.

> +	help
> +	  Generic PWM framework driver for Loongson family.
> +	  It can be found on Loongson-2K series cpu and Loongson LS7A bridge chips.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-loongson.
> +
>  config PWM_LP3943
>  	tristate "TI/National Semiconductor LP3943 PWM support"
>  	depends on MFD_LP3943
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c5ec9e168ee7..bffa49500277 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
>  obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>  obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
> +obj-$(CONFIG_PWM_LOONGSON)	+= pwm-loongson.o
>  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
>  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
>  obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
> diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> new file mode 100644
> index 000000000000..5ac79a69acd3
> --- /dev/null
> +++ b/drivers/pwm/pwm-loongson.c
> @@ -0,0 +1,298 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Loongson PWM driver
> + *
> + * Author: Juxin Gao <gaojuxin@loongson.cn>
> + * Further cleanup and restructuring by:
> + *         Binbin Zhou <zhoubinbin@loongson.cn>
> + *
> + * Copyright (C) 2017-2024 Loongson Technology Corporation Limited.

A paragraph about the hardware capabilities here please. Please answer
the following questions:

 - How does the hardware behave on disable? (Does it complete the
   currently running period? Is the output still driven then? If yes,
   which level?)

 - How does the hardware behave on configuration changes? (Does it
   complete the currently running period? Are there some glitches
   expected (like driving an output corresponding to the old period
   length but the new duty_cycle or similar).

 - Are there any restrictions like: Cannot do 100% relative duty (or
   0%)?

Stick to the format used in most other drivers such that

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

emits the requested info.

> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/units.h>
> +
> +/* Loongson PWM registers */
> +#define PWM_DUTY	0x4 /* Low Pulse Buffer Register */
> +#define PWM_PERIOD	0x8 /* Pulse Period Buffer Register */
> +#define PWM_CTRL	0xc /* Control Register */

Please use a driver specific prefix for these defines. PWM_DUTY is quite
generic otherwise.

> +
> +/* Control register bits */
> +#define PWM_CTRL_EN	BIT(0)  /* Counter Enable Bit */
> +#define PWM_CTRL_OE	BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
> +#define PWM_CTRL_SINGLE	BIT(4)  /* Single Pulse Control Bit */
> +#define PWM_CTRL_INTE	BIT(5)  /* Interrupt Enable Bit */
> +#define PWM_CTRL_INT	BIT(6)  /* Interrupt Bit */
> +#define PWM_CTRL_RST	BIT(7)  /* Counter Reset Bit */
> +#define PWM_CTRL_CAPTE	BIT(8)  /* Measurement Pulse Enable Bit */
> +#define PWM_CTRL_INVERT	BIT(9)  /* Output flip-flop Enable Bit */
> +#define PWM_CTRL_DZONE	BIT(10) /* Anti-dead Zone Enable Bit */
> +
> +#define PWM_FREQ_STD       (50 * HZ_PER_KHZ)
> +
> +struct pwm_loongson_ddata {
> +	struct pwm_chip	chip;
> +	struct clk	*clk;
> +	void __iomem	*base;
> +	/* The following for PM */
> +	u32		ctrl;
> +	u32		duty;
> +	u32		period;

This needs updating to cope for commit 05947224ff46 ("pwm: Ensure that
pwm_chips are allocated using pwmchip_alloc()")

Also I'm not a fan of aligning the member names. If you feel strong
about it, keep it as is, however.

> +};
> +
> +static inline struct pwm_loongson_ddata *to_pwm_loongson_ddata(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct pwm_loongson_ddata, chip);
> +}
> +
> +static inline u32 pwm_loongson_readl(struct pwm_loongson_ddata *ddata, u64 offset)

I don't know about the calling convention on loongson, but I'd expect
offset to be an unsigned int only, given that (I guess) only PWM_CTRL
and friends are passed here.

> +{
> +	return readl(ddata->base + offset);
> +}
> +
> +static inline void pwm_loongson_writel(struct pwm_loongson_ddata *ddata,
> +				       u32 val, u64 offset)
> +{
> +	writel(val, ddata->base + offset);
> +}
> +
> +static int pwm_loongson_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				     enum pwm_polarity polarity)
> +{
> +	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +	u16 val;
> +
> +	val = pwm_loongson_readl(ddata, PWM_CTRL);
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		/* Duty cycle defines LOW period of PWM */
> +		val |= PWM_CTRL_INVERT;
> +	else
> +		/* Duty cycle defines HIGH period of PWM */
> +		val &= ~PWM_CTRL_INVERT;
> +
> +	pwm_loongson_writel(ddata, val, PWM_CTRL);
> +
> +	return 0;
> +}
> +
> +static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +	u32 val;
> +
> +	if (pwm->state.polarity == PWM_POLARITY_NORMAL)
> +		pwm_loongson_writel(ddata, ddata->period, PWM_DUTY);
> +	else if (pwm->state.polarity == PWM_POLARITY_INVERSED)
> +		pwm_loongson_writel(ddata, 0, PWM_DUTY);
> +
> +	val = pwm_loongson_readl(ddata, PWM_CTRL);
> +	val &= ~PWM_CTRL_EN;
> +	pwm_loongson_writel(ddata, val, PWM_CTRL);

Technically it's not needed to configure the duty. A consumer who
expects a certain behaviour is supposed to not disable the PWM.

> +}
> +
> +static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +	u32 val;
> +
> +	pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
> +	pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);

pwm_loongson_enable() is called from pwm_loongson_apply() and PWM_DUTY and
PWM_PERIOD were already written there. So please either only write it
once, or add a code comment about why writing twice is needed.

> +	val = pwm_loongson_readl(ddata, PWM_CTRL);
> +	val |= PWM_CTRL_EN;
> +	pwm_loongson_writel(ddata, val, PWM_CTRL);
> +
> +	return 0;
> +}
> +
> +static u32 pwm_loongson_set_config(struct pwm_loongson_ddata *ddata, int ns,
> +				   u64 clk_rate, u64 offset)
> +{
> +	u32 val;
> +	u64 c;
> +
> +	c = clk_rate * ns;

That migth overflow?!

> +	do_div(c, NSEC_PER_SEC);
> +	val = c < 1 ? 1 : c;

That smells fishy. If a period (or duty_cycle) is requested that is too
small to be implemented, let .apply() return -EINVAL.

> +	pwm_loongson_writel(ddata, val, offset);
> +
> +	return val;
> +}
> +
> +static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       int duty_ns, int period_ns)
> +{
> +	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +	struct device *dev = chip->dev;
> +	u64 clk_rate;
> +
> +	if (duty_ns > NANOHZ_PER_HZ || period_ns > NANOHZ_PER_HZ)
> +		return -ERANGE;

Nope, that's wrong. Please configure the biggest possible period not
bigger than the requested period. So something like:

	period_ns = min(period_ns, NANOHZ_PER_HZ);

; ditto for duty_cycle.

> +	clk_rate = has_acpi_companion(dev) ? PWM_FREQ_STD : clk_get_rate(ddata->clk);
> +
> +	ddata->duty = pwm_loongson_set_config(ddata, duty_ns, clk_rate, PWM_DUTY);
> +	ddata->period = pwm_loongson_set_config(ddata, period_ns, clk_rate, PWM_PERIOD);
> +
> +	return 0;
> +}
> +
> +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      const struct pwm_state *state)
> +{
> +	int err;
> +	bool enabled = pwm->state.enabled;
> +
> +	if (state->polarity != pwm->state.polarity) {
> +		if (enabled) {
> +			pwm_loongson_disable(chip, pwm);
> +			enabled = false;
> +		}
> +
> +		err = pwm_loongson_set_polarity(chip, pwm, state->polarity);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (!state->enabled) {
> +		if (enabled)
> +			pwm_loongson_disable(chip, pwm);
> +		return 0;
> +	}
> +
> +	err = pwm_loongson_config(chip, pwm, state->duty_cycle, state->period);

state->duty_cycle is an u64, however it's truncated to an int here.

> +	if (err)
> +		return err;
> +
> +	if (!enabled)
> +		err = pwm_loongson_enable(chip, pwm);
> +
> +	return err;
> +}
> +
> +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  struct pwm_state *state)
> +{
> +	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +	u32 period, duty, ctrl;
> +	u64 ns;
> +
> +	ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
> +	state->polarity = (ctrl & PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	state->enabled = (ctrl & PWM_CTRL_EN) ? true : false;
> +
> +	duty = pwm_loongson_readl(ddata, PWM_DUTY);
> +	ns = duty * NSEC_PER_SEC;
> +	state->duty_cycle = do_div(ns, duty);
> +
> +	period = pwm_loongson_readl(ddata, PWM_PERIOD);
> +	ns = period * NSEC_PER_SEC;
> +	state->period = do_div(ns, period);
> +
> +	ddata->ctrl = ctrl;
> +	ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> +	ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);

The rounding looks wrong. Did you test with PWM_DEBUG enabled?

I think the value assigned to ddata->period and the other members isn't
used. Unless I'm mistaken, please drop the assignment.

> +	return 0;
> +}
> +
> +static const struct pwm_ops pwm_loongson_ops = {
> +	.apply = pwm_loongson_apply,
> +	.get_state = pwm_loongson_get_state,
> +};
> +
> +static int pwm_loongson_probe(struct platform_device *pdev)
> +{
> +	struct pwm_loongson_ddata *ddata;
> +	struct device *dev = &pdev->dev;
> +
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ddata->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ddata->base))
> +		return PTR_ERR(ddata->base);
> +
> +	if (!has_acpi_companion(dev)) {
> +		ddata->clk = devm_clk_get_enabled(dev, NULL);
> +		if (IS_ERR(ddata->clk))
> +			return PTR_ERR(ddata->clk);

error message with dev_err_probe() please.

> +	}
> +
> +	ddata->chip.dev = dev;
> +	ddata->chip.ops = &pwm_loongson_ops;
> +	ddata->chip.npwm = 1;
> +	platform_set_drvdata(pdev, ddata);

The effect of platform_set_drvdata is used in .suspend below, however
there you use dev_get_drvdata on &pdev->dev. For symmetry I suggest to
use dev_set_drvdata(dev, ddata) here.

> +	return devm_pwmchip_add(dev, &ddata->chip);

error message iwth dev_err_probe() please (if it fails).

> +}
> +
> [...]
> +static struct platform_driver pwm_loongson_driver = {
> +	.probe  = pwm_loongson_probe,
> +	.driver = {
> +		.name   = "loongson-pwm",
> +		.pm	= pm_ptr(&pwm_loongson_pm_ops),
> +		.of_match_table   = pwm_loongson_of_ids,
> +		.acpi_match_table = pwm_loongson_acpi_ids,

This alignment looks really ugly. Please use a single space before the
=. (Or if you must, properly align the =.)

> +	},
> +};
> +module_platform_driver(pwm_loongson_driver);
> +
> +MODULE_DESCRIPTION("Loongson PWM driver");
> +MODULE_AUTHOR("Loongson Technology Corporation Limited.");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v3 2/2] pwm: Add Loongson PWM controller support

[Binbin Zhou]

Hi Uwe:

Thanks for your detailed review.

On Thu, May 23, 2024 at 10:31 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> sorry for taking so long to get back to your patch. reviewing new
> drivers is quite time consuming which makes me often fail to review in a
> timely manner.
>
> On Tue, Apr 16, 2024 at 09:55:15AM +0800, Binbin Zhou wrote:
> > This commit adds a generic PWM framework driver for the PWM controller
> > found on Loongson family chips.
> >
> > Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
> > Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  MAINTAINERS                |   1 +
> >  drivers/pwm/Kconfig        |  10 ++
> >  drivers/pwm/Makefile       |   1 +
> >  drivers/pwm/pwm-loongson.c | 298 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 310 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-loongson.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ecef2744726d..d32da7c77f0e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12756,6 +12756,7 @@ M:    Binbin Zhou <zhoubinbin@loongson.cn>
> >  L:   linux-pwm@vger.kernel.org
> >  S:   Maintained
> >  F:   Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
> > +F:   drivers/pwm/pwm-loongson.c
> >
> >  LOONGSON-2 SOC SERIES CLOCK DRIVER
> >  M:   Yinbo Zhu <zhuyinbo@loongson.cn>
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 4b956d661755..bb163c65e5ae 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -324,6 +324,16 @@ config PWM_KEEMBAY
> >         To compile this driver as a module, choose M here: the module
> >         will be called pwm-keembay.
> >
> > +config PWM_LOONGSON
> > +     tristate "Loongson PWM support"
> > +     depends on MACH_LOONGSON64
>
> Something with || COMPILE_TEST would be nice.

OK..
>
> > +     help
> > +       Generic PWM framework driver for Loongson family.
> > +       It can be found on Loongson-2K series cpu and Loongson LS7A bridge chips.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called pwm-loongson.
> > +
> >  config PWM_LP3943
> >       tristate "TI/National Semiconductor LP3943 PWM support"
> >       depends on MFD_LP3943
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index c5ec9e168ee7..bffa49500277 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -28,6 +28,7 @@ obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o
> >  obj-$(CONFIG_PWM_IQS620A)    += pwm-iqs620a.o
> >  obj-$(CONFIG_PWM_JZ4740)     += pwm-jz4740.o
> >  obj-$(CONFIG_PWM_KEEMBAY)    += pwm-keembay.o
> > +obj-$(CONFIG_PWM_LOONGSON)   += pwm-loongson.o
> >  obj-$(CONFIG_PWM_LP3943)     += pwm-lp3943.o
> >  obj-$(CONFIG_PWM_LPC18XX_SCT)        += pwm-lpc18xx-sct.o
> >  obj-$(CONFIG_PWM_LPC32XX)    += pwm-lpc32xx.o
> > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> > new file mode 100644
> > index 000000000000..5ac79a69acd3
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-loongson.c
> > @@ -0,0 +1,298 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Loongson PWM driver
> > + *
> > + * Author: Juxin Gao <gaojuxin@loongson.cn>
> > + * Further cleanup and restructuring by:
> > + *         Binbin Zhou <zhoubinbin@loongson.cn>
> > + *
> > + * Copyright (C) 2017-2024 Loongson Technology Corporation Limited.
>
> A paragraph about the hardware capabilities here please. Please answer
> the following questions:
>
>  - How does the hardware behave on disable? (Does it complete the
>    currently running period? Is the output still driven then? If yes,
>    which level?)
>
>  - How does the hardware behave on configuration changes? (Does it
>    complete the currently running period? Are there some glitches
>    expected (like driving an output corresponding to the old period
>    length but the new duty_cycle or similar).
>
>  - Are there any restrictions like: Cannot do 100% relative duty (or
>    0%)?
>
> Stick to the format used in most other drivers such that
>
>         sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-loongson.c
>
> emits the requested info.
>

OK, I will add it.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/units.h>
> > +
> > +/* Loongson PWM registers */
> > +#define PWM_DUTY     0x4 /* Low Pulse Buffer Register */
> > +#define PWM_PERIOD   0x8 /* Pulse Period Buffer Register */
> > +#define PWM_CTRL     0xc /* Control Register */
>
> Please use a driver specific prefix for these defines. PWM_DUTY is quite
> generic otherwise.

OK, I will rename these defines as LOONGSON_*.
>
> > +
> > +/* Control register bits */
> > +#define PWM_CTRL_EN  BIT(0)  /* Counter Enable Bit */
> > +#define PWM_CTRL_OE  BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
> > +#define PWM_CTRL_SINGLE      BIT(4)  /* Single Pulse Control Bit */
> > +#define PWM_CTRL_INTE        BIT(5)  /* Interrupt Enable Bit */
> > +#define PWM_CTRL_INT BIT(6)  /* Interrupt Bit */
> > +#define PWM_CTRL_RST BIT(7)  /* Counter Reset Bit */
> > +#define PWM_CTRL_CAPTE       BIT(8)  /* Measurement Pulse Enable Bit */
> > +#define PWM_CTRL_INVERT      BIT(9)  /* Output flip-flop Enable Bit */
> > +#define PWM_CTRL_DZONE       BIT(10) /* Anti-dead Zone Enable Bit */
> > +
> > +#define PWM_FREQ_STD       (50 * HZ_PER_KHZ)
> > +
> > +struct pwm_loongson_ddata {
> > +     struct pwm_chip chip;
> > +     struct clk      *clk;
> > +     void __iomem    *base;
> > +     /* The following for PM */
> > +     u32             ctrl;
> > +     u32             duty;
> > +     u32             period;
>
> This needs updating to cope for commit 05947224ff46 ("pwm: Ensure that
> pwm_chips are allocated using pwmchip_alloc()")
>
> Also I'm not a fan of aligning the member names. If you feel strong
> about it, keep it as is, however.
>

Yes, I have refactored this part based on commit 05947224ff46 ("pwm:
Ensure that pwm_chips are allocated using pwmchip_alloc()").
> > +};
> > +
> > +static inline struct pwm_loongson_ddata *to_pwm_loongson_ddata(struct pwm_chip *chip)
> > +{
> > +     return container_of(chip, struct pwm_loongson_ddata, chip);
> > +}
> > +
> > +static inline u32 pwm_loongson_readl(struct pwm_loongson_ddata *ddata, u64 offset)
>
> I don't know about the calling convention on loongson, but I'd expect
> offset to be an unsigned int only, given that (I guess) only PWM_CTRL
> and friends are passed here.
>

Emm...
Actually, unsigned int should be enough.
> > +{
> > +     return readl(ddata->base + offset);
> > +}
> > +
> > +static inline void pwm_loongson_writel(struct pwm_loongson_ddata *ddata,
> > +                                    u32 val, u64 offset)
> > +{
> > +     writel(val, ddata->base + offset);
> > +}
> > +
> > +static int pwm_loongson_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                                  enum pwm_polarity polarity)
> > +{
> > +     struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +     u16 val;
> > +
> > +     val = pwm_loongson_readl(ddata, PWM_CTRL);
> > +
> > +     if (polarity == PWM_POLARITY_INVERSED)
> > +             /* Duty cycle defines LOW period of PWM */
> > +             val |= PWM_CTRL_INVERT;
> > +     else
> > +             /* Duty cycle defines HIGH period of PWM */
> > +             val &= ~PWM_CTRL_INVERT;
> > +
> > +     pwm_loongson_writel(ddata, val, PWM_CTRL);
> > +
> > +     return 0;
> > +}
> > +
> > +static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +     struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +     u32 val;
> > +
> > +     if (pwm->state.polarity == PWM_POLARITY_NORMAL)
> > +             pwm_loongson_writel(ddata, ddata->period, PWM_DUTY);
> > +     else if (pwm->state.polarity == PWM_POLARITY_INVERSED)
> > +             pwm_loongson_writel(ddata, 0, PWM_DUTY);
> > +
> > +     val = pwm_loongson_readl(ddata, PWM_CTRL);
> > +     val &= ~PWM_CTRL_EN;
> > +     pwm_loongson_writel(ddata, val, PWM_CTRL);
>
> Technically it's not needed to configure the duty. A consumer who
> expects a certain behaviour is supposed to not disable the PWM.
>

Emm, this is really not necessary.
> > +}
> > +
> > +static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +     struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +     u32 val;
> > +
> > +     pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
> > +     pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
>
> pwm_loongson_enable() is called from pwm_loongson_apply() and PWM_DUTY and
> PWM_PERIOD were already written there. So please either only write it
> once, or add a code comment about why writing twice is needed.
>

Sorry, it's my fault. I will keep these regs written only once.
> > +     val = pwm_loongson_readl(ddata, PWM_CTRL);
> > +     val |= PWM_CTRL_EN;
> > +     pwm_loongson_writel(ddata, val, PWM_CTRL);
> > +
> > +     return 0;
> > +}
> > +
> > +static u32 pwm_loongson_set_config(struct pwm_loongson_ddata *ddata, int ns,
> > +                                u64 clk_rate, u64 offset)
> > +{
> > +     u32 val;
> > +     u64 c;
> > +
> > +     c = clk_rate * ns;
>
> That migth overflow?!
>
> > +     do_div(c, NSEC_PER_SEC);
> > +     val = c < 1 ? 1 : c;
>
> That smells fishy. If a period (or duty_cycle) is requested that is too
> small to be implemented, let .apply() return -EINVAL.
>

In fact, I'm going to rewrite this part, drop the
pwm_loongson_set_config(), and calulate the duty and period in
pwm_loongson_config(), something like:

        /* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */
        ctx.duty = mul_u64_u64_div_u64(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
        pwm_loongson_writel(ddata, ctx.duty, LOONGSON_PWM_REG_DUTY);

        /* period = period_ns * ddata->clk_rate / NSEC_PER_SEC */
        ctx.period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate,
NSEC_PER_SEC);
        pwm_loongson_writel(ddata, ctx.period, LOONGSON_PWM_REG_PERIOD);


> > +     pwm_loongson_writel(ddata, val, offset);
> > +
> > +     return val;
> > +}
> > +
> > +static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                            int duty_ns, int period_ns)
> > +{
> > +     struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +     struct device *dev = chip->dev;
> > +     u64 clk_rate;
> > +
> > +     if (duty_ns > NANOHZ_PER_HZ || period_ns > NANOHZ_PER_HZ)
> > +             return -ERANGE;
>
> Nope, that's wrong. Please configure the biggest possible period not
> bigger than the requested period. So something like:
>
>         period_ns = min(period_ns, NANOHZ_PER_HZ);
>
> ; ditto for duty_cycle.

OK.. I will do it in .apply().
>
> > +     clk_rate = has_acpi_companion(dev) ? PWM_FREQ_STD : clk_get_rate(ddata->clk);
> > +
> > +     ddata->duty = pwm_loongson_set_config(ddata, duty_ns, clk_rate, PWM_DUTY);
> > +     ddata->period = pwm_loongson_set_config(ddata, period_ns, clk_rate, PWM_PERIOD);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                           const struct pwm_state *state)
> > +{
> > +     int err;
> > +     bool enabled = pwm->state.enabled;
> > +
> > +     if (state->polarity != pwm->state.polarity) {
> > +             if (enabled) {
> > +                     pwm_loongson_disable(chip, pwm);
> > +                     enabled = false;
> > +             }
> > +
> > +             err = pwm_loongson_set_polarity(chip, pwm, state->polarity);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     if (!state->enabled) {
> > +             if (enabled)
> > +                     pwm_loongson_disable(chip, pwm);
> > +             return 0;
> > +     }
> > +
> > +     err = pwm_loongson_config(chip, pwm, state->duty_cycle, state->period);
>
> state->duty_cycle is an u64, however it's truncated to an int here.
>

OK, I will fix it.
> > +     if (err)
> > +             return err;
> > +
> > +     if (!enabled)
> > +             err = pwm_loongson_enable(chip, pwm);
> > +
> > +     return err;
> > +}
> > +
> > +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                               struct pwm_state *state)
> > +{
> > +     struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +     u32 period, duty, ctrl;
> > +     u64 ns;
> > +
> > +     ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
> > +     state->polarity = (ctrl & PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > +     state->enabled = (ctrl & PWM_CTRL_EN) ? true : false;
> > +
> > +     duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > +     ns = duty * NSEC_PER_SEC;
> > +     state->duty_cycle = do_div(ns, duty);
> > +
> > +     period = pwm_loongson_readl(ddata, PWM_PERIOD);
> > +     ns = period * NSEC_PER_SEC;
> > +     state->period = do_div(ns, period);
> > +
> > +     ddata->ctrl = ctrl;
> > +     ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > +     ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
>
> The rounding looks wrong. Did you test with PWM_DEBUG enabled?
>
> I think the value assigned to ddata->period and the other members isn't
> used. Unless I'm mistaken, please drop the assignment.
>

The period, duty and ctrl are prepared for PM. I plan to put these
three parameters separately into the pwm_loongson_context structure. I
think it will look clearer:

struct pwm_loongson_context {
        u32 ctrl;
        u32 duty;
        u32 period;
};

> > +     return 0;
> > +}
> > +
> > +static const struct pwm_ops pwm_loongson_ops = {
> > +     .apply = pwm_loongson_apply,
> > +     .get_state = pwm_loongson_get_state,
> > +};
> > +
> > +static int pwm_loongson_probe(struct platform_device *pdev)
> > +{
> > +     struct pwm_loongson_ddata *ddata;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > +     if (!ddata)
> > +             return -ENOMEM;
> > +
> > +     ddata->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(ddata->base))
> > +             return PTR_ERR(ddata->base);
> > +
> > +     if (!has_acpi_companion(dev)) {
> > +             ddata->clk = devm_clk_get_enabled(dev, NULL);
> > +             if (IS_ERR(ddata->clk))
> > +                     return PTR_ERR(ddata->clk);
>
> error message with dev_err_probe() please.

OK, I will do it.
>
> > +     }
> > +
> > +     ddata->chip.dev = dev;
> > +     ddata->chip.ops = &pwm_loongson_ops;
> > +     ddata->chip.npwm = 1;
> > +     platform_set_drvdata(pdev, ddata);
>
> The effect of platform_set_drvdata is used in .suspend below, however
> there you use dev_get_drvdata on &pdev->dev. For symmetry I suggest to
> use dev_set_drvdata(dev, ddata) here.
>
> > +     return devm_pwmchip_add(dev, &ddata->chip);
>
> error message iwth dev_err_probe() please (if it fails).

OK, I will add it.
>
> > +}
> > +
> > [...]
> > +static struct platform_driver pwm_loongson_driver = {
> > +     .probe  = pwm_loongson_probe,
> > +     .driver = {
> > +             .name   = "loongson-pwm",
> > +             .pm     = pm_ptr(&pwm_loongson_pm_ops),
> > +             .of_match_table   = pwm_loongson_of_ids,
> > +             .acpi_match_table = pwm_loongson_acpi_ids,
>
> This alignment looks really ugly. Please use a single space before the
> =. (Or if you must, properly align the =.)
>
OK., I will keep  a single space before the =.

Thanks.
Binbin
> > +     },
> > +};
> > +module_platform_driver(pwm_loongson_driver);
> > +
> > +MODULE_DESCRIPTION("Loongson PWM driver");
> > +MODULE_AUTHOR("Loongson Technology Corporation Limited.");
> > +MODULE_LICENSE("GPL");
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 2/2] pwm: Add Loongson PWM controller support

[Uwe Kleine-König]

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

Hello Binbin,

On Fri, May 24, 2024 at 02:29:35PM +0600, Binbin Zhou wrote:
> > > +     ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > > +     ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
> >
> > The rounding looks wrong. Did you test with PWM_DEBUG enabled?
> >
> > I think the value assigned to ddata->period and the other members isn't
> > used. Unless I'm mistaken, please drop the assignment.
> >
> 
> The period, duty and ctrl are prepared for PM. I plan to put these
> three parameters separately into the pwm_loongson_context structure. I
> think it will look clearer:
> 
> struct pwm_loongson_context {
>         u32 ctrl;
>         u32 duty;
>         u32 period;
> };

But .suspend() reads the value from the registers and rewrites these
three members itself, too. So the write in .apply() is unused and can be
dropped.

The suggestion to put this in a struct is nice. I'd call it something
with "suspend" though, maybe "pwm_loongson_suspend_store"?
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v3 2/2] pwm: Add Loongson PWM controller support

[Binbin Zhou]

Hi Uwe:

On Fri, May 24, 2024 at 10:01 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Binbin,
>
> On Fri, May 24, 2024 at 02:29:35PM +0600, Binbin Zhou wrote:
> > > > +     ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > > > +     ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
> > >
> > > The rounding looks wrong. Did you test with PWM_DEBUG enabled?
> > >
> > > I think the value assigned to ddata->period and the other members isn't
> > > used. Unless I'm mistaken, please drop the assignment.
> > >
> >
> > The period, duty and ctrl are prepared for PM. I plan to put these
> > three parameters separately into the pwm_loongson_context structure. I
> > think it will look clearer:
> >
> > struct pwm_loongson_context {
> >         u32 ctrl;
> >         u32 duty;
> >         u32 period;
> > };
>
> But .suspend() reads the value from the registers and rewrites these
> three members itself, too. So the write in .apply() is unused and can be
> dropped.

Yes, you are right, I will fix it.
>
> The suggestion to put this in a struct is nice. I'd call it something
> with "suspend" though, maybe "pwm_loongson_suspend_store"?
>

Ah, The name sounds more readable.

Thanks.
Binbin
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 0/2] pwm: Introduce pwm driver for the Loongson family chips

[Huacai Chen]

For the whole series,

Acked-by: Huacai Chen <chenhuacai@loongson.cn>

On Tue, Apr 16, 2024 at 9:55 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
>
> Hi all:
>
> This patchset introduce a generic PWM framework driver for Loongson family.
> Each PWM has one pulse width output signal and one pulse input signal to be measured.
>
> It can be found on Loongson-2K series cpus and Loongson LS7A bridge chips.
>
> Thanks.
>
> -------
> V3:
> patch (1/2):
>  - Add Reviewed-by tag from Krzysztof, thanks.
> patch (2/2):
>  - Several code stlye adjustments, such as line breaks.
>
> Link to V2:
> https://lore.kernel.org/all/cover.1712732719.git.zhoubinbin@loongson.cn/
>
> v2:
> - Remove the dts-related patches and update dts at once after all
> relevant drivers are complete.
> patch (1/2):
>  - The dt-binding filename should match compatible, rename it as
>    loongson,ls7a-pwm.yaml;
>  - Update binding description;
>  - Add description for each pwm cell;
>  - Drop '#pwm-cells' from required, for pwm.yaml makes it required already.
>
> Link to v1:
> https://lore.kernel.org/linux-pwm/cover.1711953223.git.zhoubinbin@loongson.cn/
>
> Binbin Zhou (2):
>   dt-bindings: pwm: Add Loongson PWM controller
>   pwm: Add Loongson PWM controller support
>
>  .../bindings/pwm/loongson,ls7a-pwm.yaml       |  66 ++++
>  MAINTAINERS                                   |   7 +
>  drivers/pwm/Kconfig                           |  10 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-loongson.c                    | 298 ++++++++++++++++++
>  5 files changed, 382 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-loongson.c
>
> --
> 2.43.0
>

Re: [PATCH v3 0/2] pwm: Introduce pwm driver for the Loongson family chips

[Binbin Zhou]

On Tue, Apr 16, 2024 at 7:55 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
>
> Hi all:
>
> This patchset introduce a generic PWM framework driver for Loongson family.
> Each PWM has one pulse width output signal and one pulse input signal to be measured.
>
> It can be found on Loongson-2K series cpus and Loongson LS7A bridge chips.

Hi Uwe:

Gentle ping.
The dt-binding part of the patchset looks good now, do you have any
comments on the driver part?

Thanks.
Binbin
>
> Thanks.
>
> -------
> V3:
> patch (1/2):
>  - Add Reviewed-by tag from Krzysztof, thanks.
> patch (2/2):
>  - Several code stlye adjustments, such as line breaks.
>
> Link to V2:
> https://lore.kernel.org/all/cover.1712732719.git.zhoubinbin@loongson.cn/
>
> v2:
> - Remove the dts-related patches and update dts at once after all
> relevant drivers are complete.
> patch (1/2):
>  - The dt-binding filename should match compatible, rename it as
>    loongson,ls7a-pwm.yaml;
>  - Update binding description;
>  - Add description for each pwm cell;
>  - Drop '#pwm-cells' from required, for pwm.yaml makes it required already.
>
> Link to v1:
> https://lore.kernel.org/linux-pwm/cover.1711953223.git.zhoubinbin@loongson.cn/
>
> Binbin Zhou (2):
>   dt-bindings: pwm: Add Loongson PWM controller
>   pwm: Add Loongson PWM controller support
>
>  .../bindings/pwm/loongson,ls7a-pwm.yaml       |  66 ++++
>  MAINTAINERS                                   |   7 +
>  drivers/pwm/Kconfig                           |  10 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-loongson.c                    | 298 ++++++++++++++++++
>  5 files changed, 382 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-loongson.c
>
> --
> 2.43.0
>