public inbox for devicetree@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v20 0/6] Add PWM support for IPQ chipsets
@ 2026-02-04 11:25 George Moussalem via B4 Relay
  2026-02-04 11:25 ` [PATCH v20 1/6] dt-bindings: pwm: add IPQ6018 binding George Moussalem via B4 Relay
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: George Moussalem via B4 Relay @ 2026-02-04 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Baruch Siach, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	George Moussalem, Devi Priya, Baruch Siach, Dmitry Baryshkov,
	Bjorn Andersson, Krzysztof Kozlowski

Add PWM driver and binding support for IPQ chipsets.
Also, add nodes to add support for pwm in ipq6018, ipq5018, ipq5332, and
ipq9574.

I've picked up work based on Devi's last submission (v15) which dates
back to 05 October 2023 as below SoCs are still active.

Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
Changes in v20:
- Updated IPQ_PWM_MAX_DIV macro definition to use FIELD_MAX
- Removed clk struct from ipq_pwm struct and added clk_rate field
  instead which is set during probe.
- Consolidated config_div_and_duty into ipq_pwm_apply
- Fixed arithmetic overflows in apply and get_state 
- Fixed off-by-one in divider calculation
- Enabled 100% relative duty cycle support
- Aligned continuation on next lines relative to opening parentheses
- Return 0 instead of ret in probe
- Link to v19: https://lore.kernel.org/r/20251128-ipq-pwm-v19-0-13bc704cc6a5@outlook.com

Changes in v19:
- Changed pwm-cells property in dt bindings from 2 to 3 as per Uwe's
  recommendation
- Added hardware notes and limitations based on own findings as
  requested. NOTE: there's no publically available datasheet though.
- Expanded comment on REG1_UPDATE to indicate that when this bit is set,
  values for div and pre-div take effect. The hardware automatically
  unsets it when the change is completed.
- Added newline between MACRO definition and next comment
- In config_div_and_duty, used mul_u64_u64_div_u64 to avoid overflow
- Removed unncessary restriction of pwm_div to MAX_DIV - 1 after testing
- Constrain pre_div to MAX_DIV is pre_div calculated is > MAX_DIV
- Use of mul_u64_u64_div_u64 in .apply
- Skip calculation of period and duty cycle when PWM_ENABLE REG is unset
- Set duty cycle to period value when calculated duty cycle > period to
  return a valid config
- Removed .npwm as it's taken care of in devm_pwmchip_alloc
- Added call to devm_clk_rate_exclusive_get to lock the clock rate
- Start all kernel messages with a capital letter and end with \n.
- Changed pwm-cells property in all dtsi from 2->3 for in scope IPQ SOCs 
- Link to v18: https://lore.kernel.org/r/20251029-ipq-pwm-v18-0-edbef8efbb8e@outlook.com

Changes in v18:
- Updated maintainer info in binding
- Squashed dt bindings patches into the first for adding compatibles for
  IPQ5018, IPQ5332, and IPQ9574
- Link to v17: https://lore.kernel.org/r/20251008-ipq-pwm-v17-0-9bd43edfc7f7@outlook.com

Changes in v17:
- Picked up RB tags from Dmitry and Rob
- Removed unnecessary code comments
- Corrected reg property in PWM node in ipq6018 DTS in line with
  expected nr of bytes for address and size cells
- Link to v16: https://lore.kernel.org/r/20251001-ipq-pwm-v16-0-300f237e0e68@outlook.com

Changes in v16:
- Removed reg description in bindings as the offset is not relative to
  the TCSR region anymore since simple-mfd support was dropped and PWM
  nodes defined as their own nodes, not child nodes. Updated the example
  too.
- Dropped patch to add simple-mfd support to the qcom,tcsr bindings
- Simplified code to calculate divs and duty cycle as per Uwe's comments
- Removed unused pwm_chip struct from ipq_pwm_chip struct
- Removed unnecessary cast as per Uwe's comment
- Replaced devm_clk_get & clk_prepare_enable by devm_clk_get_enabled
- Replaced pwmchip_add by devm_pwmchip_add and removed .remove function
- Removed .owner from driver struct
- Added compatibles to the bindings and nodes to the device trees to add
  PWM support in the IPQ5018, IPQ5332, and IPQ9574 SoCs
- Link to v15: https://lore.kernel.org/r/20231005160550.2423075-1-quic_devipriy@quicinc.com

Changes in v15:
- No change
- Link to v14: https://lore.kernel.org/r/20231005033053.2626465-1-quic_devipriy@quicinc.com

Changes in v14:
- Picked up the R-b tag
- Link to v13: https://lore.kernel.org/r/20231004090449.256229-1-quic_devipriy@quicinc.com

Changes in v13:
- Updated the file name to match the compatible
- Sorted the properties and updated the order in the required field
- Dropped the syscon node from examples
- Link to v12: https://lore.kernel.org/r/20230925065915.3467964-1-quic_devipriy@quicinc.com

Changes in v12:
- Picked up the R-b tag

Changes in v11:
- No change

Changes in v10:
- No change

Changes in v9:
- Add 'ranges' property to example (Rob)
- Drop label in example (Rob)

Changes in v8:
- Add size cell to 'reg' (Rob)

Changes in v7:
- Use 'reg' instead of 'offset' (Rob)
- Drop 'clock-names' and 'assigned-clock*' (Bjorn)
- Use single cell address/size in example node (Bjorn)
- Move '#pwm-cells' lower in example node (Bjorn)
- List 'reg' as required

Changes in v6:
- Device node is child of TCSR; remove phandle (Rob Herring)
- Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)

Changes in v5:
- Use qcom,pwm-regs for phandle instead of direct regs (Bjorn
    Andersson, Kathiravan T)

Changes in v4:
- Update the binding example node as well (Rob Herring's bot)

Changes in v3:
- s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)

Changes in v2:
- Make #pwm-cells const (Rob Herring)

---

---
Devi Priya (3):
      dt-bindings: pwm: add IPQ6018 binding
      pwm: driver for qualcomm ipq6018 pwm block
      arm64: dts: qcom: ipq6018: add pwm node

George Moussalem (3):
      arm64: dts: qcom: ipq5018: add pwm node
      arm64: dts: qcom: ipq5332: add pwm node
      arm64: dts: qcom: ipq9574: add pwm node

 .../devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml  |  51 +++++
 arch/arm64/boot/dts/qcom/ipq5018.dtsi              |  10 +
 arch/arm64/boot/dts/qcom/ipq5332.dtsi              |  10 +
 arch/arm64/boot/dts/qcom/ipq6018.dtsi              |  10 +
 arch/arm64/boot/dts/qcom/ipq9574.dtsi              |  10 +
 drivers/pwm/Kconfig                                |  12 ++
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-ipq.c                              | 239 +++++++++++++++++++++
 8 files changed, 343 insertions(+)
---
base-commit: 6db894fcdec4d214812f3e6f656639f1b0081127
change-id: 20250922-ipq-pwm-c8c75c147d52

Best regards,
-- 
George Moussalem <george.moussalem@outlook.com>



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v20 1/6] dt-bindings: pwm: add IPQ6018 binding
  2026-02-04 11:25 [PATCH v20 0/6] Add PWM support for IPQ chipsets George Moussalem via B4 Relay
@ 2026-02-04 11:25 ` George Moussalem via B4 Relay
  2026-02-04 11:25 ` [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block George Moussalem via B4 Relay
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: George Moussalem via B4 Relay @ 2026-02-04 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Baruch Siach, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	George Moussalem, Devi Priya, Baruch Siach, Bjorn Andersson,
	Krzysztof Kozlowski

From: Devi Priya <quic_devipriy@quicinc.com>

DT binding for the PWM block in Qualcomm IPQ6018 SoC.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Co-developed-by: Baruch Siach <baruch.siach@siklu.com>
Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 .../devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml  | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml b/Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..f9f1f652e7527bc8fb3d5fad51b0057ea53b3766
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/qcom,ipq6018-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/qcom,ipq6018-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm IPQ6018 PWM controller
+
+maintainers:
+  - George Moussalem <george.moussalem@outlook.com>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - qcom,ipq5018-pwm
+              - qcom,ipq5332-pwm
+              - qcom,ipq9574-pwm
+          - const: qcom,ipq6018-pwm
+      - const: qcom,ipq6018-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#pwm-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
+
+    pwm: pwm@1941010 {
+        compatible = "qcom,ipq6018-pwm";
+        reg = <0x01941010 0x20>;
+        clocks = <&gcc GCC_ADSS_PWM_CLK>;
+        assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+        assigned-clock-rates = <100000000>;
+        #pwm-cells = <3>;
+    };

-- 
2.52.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block
  2026-02-04 11:25 [PATCH v20 0/6] Add PWM support for IPQ chipsets George Moussalem via B4 Relay
  2026-02-04 11:25 ` [PATCH v20 1/6] dt-bindings: pwm: add IPQ6018 binding George Moussalem via B4 Relay
@ 2026-02-04 11:25 ` George Moussalem via B4 Relay
  2026-04-02 15:35   ` Uwe Kleine-König
  2026-02-04 11:25 ` [PATCH v20 3/6] arm64: dts: qcom: ipq6018: add pwm node George Moussalem via B4 Relay
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: George Moussalem via B4 Relay @ 2026-02-04 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Baruch Siach, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	George Moussalem, Devi Priya, Baruch Siach

From: Devi Priya <quic_devipriy@quicinc.com>

Driver for the PWM block in Qualcomm IPQ6018 line of SoCs. Based on
driver from downstream Codeaurora kernel tree. Removed support for older
(V1) variants because I have no access to that hardware.

Tested on IPQ5018 and IPQ6010 based hardware.

Co-developed-by: Baruch Siach <baruch.siach@siklu.com>
Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
---
 drivers/pwm/Kconfig   |  12 +++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-ipq.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 252 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 6f3147518376a04b6e890c4bf31e06e1af04054e..e8886a9b64d9689d86920cc0d897231a1cfc06cc 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -347,6 +347,18 @@ config PWM_INTEL_LGM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-intel-lgm.
 
+config PWM_IPQ
+	tristate "IPQ PWM support"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on HAVE_CLK && HAS_IOMEM
+	help
+	  Generic PWM framework driver for IPQ PWM block which supports
+	  4 pwm channels. Each of the these channels can be configured
+	  independent of each other.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ipq.
+
 config PWM_IQS620A
 	tristate "Azoteq IQS620A PWM support"
 	depends on MFD_IQS62X || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 0dc0d2b69025dbd27013285cd335d3cb1ca2ab3f..5630a521a7cffeb83ff8c8960e15eb23ddb1c9f8 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
 obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
 obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
 obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
+obj-$(CONFIG_PWM_IPQ)		+= pwm-ipq.o
 obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
new file mode 100644
index 0000000000000000000000000000000000000000..b944ecb456d59ecef1f59eb865cc3cc90cd82244
--- /dev/null
+++ b/drivers/pwm/pwm-ipq.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved.
+ *
+ * Hardware notes / Limitations:
+ * - The PWM controller has no publicly available datasheet.
+ * - Each of the four channels is programmed via two 32-bit registers
+ *   (REG0 and REG1 at 8-byte stride).
+ * - Period and duty-cycle reconfiguration is fully atomic: new divider,
+ *   pre-divider, and high-duration values are latched by setting the
+ *   UPDATE bit (bit 30 in REG1). The hardware applies the new settings
+ *   at the beginning of the next period without disabling the output,
+ *   so the currently running period is always completed.
+ * - On disable (clearing the ENABLE bit 31 in REG1), the hardware
+ *   finishes the current period before stopping the output. The pin
+ *   is then driven to the inactive (low) level.
+ * - Upon disabling, the hardware resets the pre-divider (PRE_DIV) and divider
+ *   fields (PWM_DIV) in REG0 and REG1 to 0x0000 and 0x0001 respectively.
+ * - Only normal polarity is supported.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/math64.h>
+#include <linux/of_device.h>
+#include <linux/bitfield.h>
+#include <linux/units.h>
+
+/* The frequency range supported is 1 Hz to clock rate */
+#define IPQ_PWM_MAX_PERIOD_NS	((u64)NSEC_PER_SEC)
+
+/*
+ * Two 32-bit registers for each PWM: REG0, and REG1.
+ * Base offset for PWM #i is at 8 * #i.
+ */
+#define IPQ_PWM_REG0			0
+#define IPQ_PWM_REG0_PWM_DIV		GENMASK(15, 0)
+#define IPQ_PWM_REG0_HI_DURATION	GENMASK(31, 16)
+
+#define IPQ_PWM_REG1			4
+#define IPQ_PWM_REG1_PRE_DIV		GENMASK(15, 0)
+
+/*
+ * The max value specified for each field is based on the number of bits
+ * in the pwm control register for that field (16-bit)
+ */
+#define IPQ_PWM_MAX_DIV			FIELD_MAX(IPQ_PWM_REG0_PWM_DIV)
+
+/*
+ * Enable bit is set to enable output toggling in pwm device.
+ * Update bit is set to trigger the change and is unset automatically
+ * to reflect the changed divider and high duration values in register.
+ */
+#define IPQ_PWM_REG1_UPDATE		BIT(30)
+#define IPQ_PWM_REG1_ENABLE		BIT(31)
+
+struct ipq_pwm_chip {
+	void __iomem *mem;
+	unsigned long clk_rate;
+};
+
+static struct ipq_pwm_chip *ipq_pwm_from_chip(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static unsigned int ipq_pwm_reg_read(struct pwm_device *pwm, unsigned int reg)
+{
+	struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(pwm->chip);
+	unsigned int off = 8 * pwm->hwpwm + reg;
+
+	return readl(ipq_chip->mem + off);
+}
+
+static void ipq_pwm_reg_write(struct pwm_device *pwm, unsigned int reg,
+			      unsigned int val)
+{
+	struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(pwm->chip);
+	unsigned int off = 8 * pwm->hwpwm + reg;
+
+	writel(val, ipq_chip->mem + off);
+}
+
+static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(chip);
+	unsigned int pre_div, pwm_div;
+	u64 period_ns, duty_ns;
+	unsigned long val = 0;
+	unsigned long hi_dur;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (!ipq_chip->clk_rate)
+		return -EINVAL;
+
+	if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC,
+					       ipq_chip->clk_rate))
+		return -ERANGE;
+
+	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
+	duty_ns = min(state->duty_cycle, period_ns);
+
+	pwm_div = IPQ_PWM_MAX_DIV - 1;
+	pre_div = mul_u64_u64_div_u64(period_ns, ipq_chip->clk_rate,
+				      (u64)NSEC_PER_SEC * (pwm_div + 1));
+	pre_div = (pre_div > 0) ? pre_div - 1 : 0;
+
+	if (pre_div > IPQ_PWM_MAX_DIV)
+		pre_div = IPQ_PWM_MAX_DIV;
+
+	/*
+	 * high duration = pwm duty * (pwm div + 1)
+	 * pwm duty = duty_ns / period_ns
+	 */
+	hi_dur = mul_u64_u64_div_u64(duty_ns, ipq_chip->clk_rate,
+				     (u64)(pre_div + 1) * NSEC_PER_SEC);
+
+	val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
+		FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
+	ipq_pwm_reg_write(pwm, IPQ_PWM_REG0, val);
+
+	val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div);
+	ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, val);
+
+	/* PWM enable toggle needs a separate write to REG1 */
+	val |= IPQ_PWM_REG1_UPDATE;
+	if (state->enabled)
+		val |= IPQ_PWM_REG1_ENABLE;
+	ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, val);
+
+	return 0;
+}
+
+static int ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			     struct pwm_state *state)
+{
+	struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(chip);
+	unsigned int pre_div, pwm_div, hi_dur;
+	u64 effective_div, hi_div;
+	u32 reg0, reg1;
+
+	reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_REG1);
+	state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
+
+	if (!state->enabled)
+		return 0;
+
+	reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_REG0);
+
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
+	hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0);
+	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
+
+	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
+	state->period = DIV64_U64_ROUND_UP(effective_div * NSEC_PER_SEC,
+					   ipq_chip->clk_rate);
+
+	hi_div = hi_dur * (pre_div + 1);
+	state->duty_cycle = DIV64_U64_ROUND_UP(hi_div * NSEC_PER_SEC,
+					       ipq_chip->clk_rate);
+
+	/*
+	 * ensure a valid config is passed back to PWM core in case duty_cycle
+	 * is > period (>100%)
+	 */
+	state->duty_cycle = min(state->duty_cycle, state->period);
+
+	return 0;
+}
+
+static const struct pwm_ops ipq_pwm_ops = {
+	.apply = ipq_pwm_apply,
+	.get_state = ipq_pwm_get_state,
+};
+
+static int ipq_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ipq_pwm_chip *pwm;
+	struct pwm_chip *chip;
+	struct clk *clk;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, 4, sizeof(*pwm));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	pwm = ipq_pwm_from_chip(chip);
+
+	pwm->mem = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pwm->mem))
+		return dev_err_probe(dev, PTR_ERR(pwm->mem),
+				     "Failed to acquire resource\n");
+
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Failed to get clock\n");
+
+	ret = devm_clk_rate_exclusive_get(dev, clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to lock clock rate\n");
+
+	pwm->clk_rate = clk_get_rate(clk);
+
+	chip->ops = &ipq_pwm_ops;
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to add pwm chip\n");
+
+	return 0;
+}
+
+static const struct of_device_id pwm_ipq_dt_match[] = {
+	{ .compatible = "qcom,ipq6018-pwm", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pwm_ipq_dt_match);
+
+static struct platform_driver ipq_pwm_driver = {
+	.driver = {
+		.name = "ipq-pwm",
+		.of_match_table = pwm_ipq_dt_match,
+	},
+	.probe = ipq_pwm_probe,
+};
+
+module_platform_driver(ipq_pwm_driver);
+
+MODULE_LICENSE("GPL");

-- 
2.52.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v20 3/6] arm64: dts: qcom: ipq6018: add pwm node
  2026-02-04 11:25 [PATCH v20 0/6] Add PWM support for IPQ chipsets George Moussalem via B4 Relay
  2026-02-04 11:25 ` [PATCH v20 1/6] dt-bindings: pwm: add IPQ6018 binding George Moussalem via B4 Relay
  2026-02-04 11:25 ` [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block George Moussalem via B4 Relay
@ 2026-02-04 11:25 ` George Moussalem via B4 Relay
  2026-02-04 12:24   ` Konrad Dybcio
  2026-02-04 11:25 ` [PATCH v20 4/6] arm64: dts: qcom: ipq5018: " George Moussalem via B4 Relay
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: George Moussalem via B4 Relay @ 2026-02-04 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Baruch Siach, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	George Moussalem, Devi Priya, Baruch Siach, Krzysztof Kozlowski

From: Devi Priya <quic_devipriy@quicinc.com>

Describe the PWM block on IPQ6018.

Although PWM is in the TCSR area, make pwm its own node as simple-mfd
has been removed from the bindings and as such hardware components
should have its own node.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Co-developed-by: Baruch Siach <baruch.siach@siklu.com>
Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 arch/arm64/boot/dts/qcom/ipq6018.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index 40f1c262126eff3761430a47472b52d27f961040..7866844cc09fd2c2c2f512ce2c8fa7826fabc7aa 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -413,6 +413,16 @@ tcsr: syscon@1937000 {
 			reg = <0x0 0x01937000 0x0 0x21000>;
 		};
 
+		pwm: pwm@1941010 {
+			compatible = "qcom,ipq6018-pwm";
+			reg = <0x0 0x01941010 0x0 0x20>;
+			clocks = <&gcc GCC_ADSS_PWM_CLK>;
+			assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+			assigned-clock-rates = <100000000>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		usb2: usb@70f8800 {
 			compatible = "qcom,ipq6018-dwc3", "qcom,dwc3";
 			reg = <0x0 0x070f8800 0x0 0x400>;

-- 
2.52.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v20 4/6] arm64: dts: qcom: ipq5018: add pwm node
  2026-02-04 11:25 [PATCH v20 0/6] Add PWM support for IPQ chipsets George Moussalem via B4 Relay
                   ` (2 preceding siblings ...)
  2026-02-04 11:25 ` [PATCH v20 3/6] arm64: dts: qcom: ipq6018: add pwm node George Moussalem via B4 Relay
@ 2026-02-04 11:25 ` George Moussalem via B4 Relay
  2026-02-04 12:25   ` Konrad Dybcio
  2026-02-04 11:25 ` [PATCH v20 5/6] arm64: dts: qcom: ipq5332: " George Moussalem via B4 Relay
  2026-02-04 11:25 ` [PATCH v20 6/6] arm64: dts: qcom: ipq9574: " George Moussalem via B4 Relay
  5 siblings, 1 reply; 14+ messages in thread
From: George Moussalem via B4 Relay @ 2026-02-04 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Baruch Siach, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	George Moussalem, Dmitry Baryshkov

From: George Moussalem <george.moussalem@outlook.com>

Describe the PWM block on IPQ5018.

Although PWM is in the TCSR area, make pwm its own node as simple-mfd
has been removed from the bindings and as such hardware components
should have its own node.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 arch/arm64/boot/dts/qcom/ipq5018.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
index 6f8004a22a1ffdb6da0be410b772be5fe0473eef..edff89257468cc5535a68321123c8a6cd0fb5adb 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
@@ -453,6 +453,16 @@ tcsr: syscon@1937000 {
 			reg = <0x01937000 0x21000>;
 		};
 
+		pwm: pwm@1941010 {
+			compatible = "qcom,ipq5018-pwm", "qcom,ipq6018-pwm";
+			reg = <0x01941010 0x20>;
+			clocks = <&gcc GCC_ADSS_PWM_CLK>;
+			assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+			assigned-clock-rates = <100000000>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		sdhc_1: mmc@7804000 {
 			compatible = "qcom,ipq5018-sdhci", "qcom,sdhci-msm-v5";
 			reg = <0x7804000 0x1000>;

-- 
2.52.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v20 5/6] arm64: dts: qcom: ipq5332: add pwm node
  2026-02-04 11:25 [PATCH v20 0/6] Add PWM support for IPQ chipsets George Moussalem via B4 Relay
                   ` (3 preceding siblings ...)
  2026-02-04 11:25 ` [PATCH v20 4/6] arm64: dts: qcom: ipq5018: " George Moussalem via B4 Relay
@ 2026-02-04 11:25 ` George Moussalem via B4 Relay
  2026-02-04 12:25   ` Konrad Dybcio
  2026-02-04 11:25 ` [PATCH v20 6/6] arm64: dts: qcom: ipq9574: " George Moussalem via B4 Relay
  5 siblings, 1 reply; 14+ messages in thread
From: George Moussalem via B4 Relay @ 2026-02-04 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Baruch Siach, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	George Moussalem, Dmitry Baryshkov

From: George Moussalem <george.moussalem@outlook.com>

Describe the PWM block on IPQ5332.

Although PWM is in the TCSR area, make pwm its own node as simple-mfd
has been removed from the bindings and as such hardware components
should have its own node.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index 45fc512a3bab221c0d99f819294abf63369987da..e58051f6c1c4fd8ef85cbcc9b98433f599377c7a 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -334,6 +334,16 @@ tcsr: syscon@1937000 {
 			reg = <0x01937000 0x21000>;
 		};
 
+		pwm: pwm@1941010 {
+			compatible = "qcom,ipq5332-pwm", "qcom,ipq6018-pwm";
+			reg = <0x01941010 0x20>;
+			clocks = <&gcc GCC_ADSS_PWM_CLK>;
+			assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+			assigned-clock-rates = <100000000>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		sdhc: mmc@7804000 {
 			compatible = "qcom,ipq5332-sdhci", "qcom,sdhci-msm-v5";
 			reg = <0x07804000 0x1000>, <0x07805000 0x1000>;

-- 
2.52.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v20 6/6] arm64: dts: qcom: ipq9574: add pwm node
  2026-02-04 11:25 [PATCH v20 0/6] Add PWM support for IPQ chipsets George Moussalem via B4 Relay
                   ` (4 preceding siblings ...)
  2026-02-04 11:25 ` [PATCH v20 5/6] arm64: dts: qcom: ipq5332: " George Moussalem via B4 Relay
@ 2026-02-04 11:25 ` George Moussalem via B4 Relay
  2026-02-04 12:26   ` Konrad Dybcio
  5 siblings, 1 reply; 14+ messages in thread
From: George Moussalem via B4 Relay @ 2026-02-04 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Baruch Siach, Bjorn Andersson, Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	George Moussalem, Dmitry Baryshkov

From: George Moussalem <george.moussalem@outlook.com>

Describe the PWM block on IPQ9574.

Although PWM is in the TCSR area, make pwm its own node as simple-mfd
has been removed from the bindings and as such hardware components
should have its own node.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: George Moussalem <george.moussalem@outlook.com>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index d7278f2137ac58305cc4e82c1d6c26c08bc7a405..31f4bcc92c986e1c0b02ea56acdf707af92b7f84 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -449,6 +449,16 @@ tcsr: syscon@1937000 {
 			reg = <0x01937000 0x21000>;
 		};
 
+		pwm: pwm@1941010 {
+			compatible = "qcom,ipq9574-pwm", "qcom,ipq6018-pwm";
+			reg = <0x01941010 0x20>;
+			clocks = <&gcc GCC_ADSS_PWM_CLK>;
+			assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+			assigned-clock-rates = <100000000>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		sdhc_1: mmc@7804000 {
 			compatible = "qcom,ipq9574-sdhci", "qcom,sdhci-msm-v5";
 			reg = <0x07804000 0x1000>,

-- 
2.52.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v20 3/6] arm64: dts: qcom: ipq6018: add pwm node
  2026-02-04 11:25 ` [PATCH v20 3/6] arm64: dts: qcom: ipq6018: add pwm node George Moussalem via B4 Relay
@ 2026-02-04 12:24   ` Konrad Dybcio
  0 siblings, 0 replies; 14+ messages in thread
From: Konrad Dybcio @ 2026-02-04 12:24 UTC (permalink / raw)
  To: george.moussalem, Uwe Kleine-König, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Baruch Siach, Bjorn Andersson,
	Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel, Devi Priya,
	Baruch Siach, Krzysztof Kozlowski

On 2/4/26 12:25 PM, George Moussalem via B4 Relay wrote:
> From: Devi Priya <quic_devipriy@quicinc.com>
> 
> Describe the PWM block on IPQ6018.
> 
> Although PWM is in the TCSR area, make pwm its own node as simple-mfd
> has been removed from the bindings and as such hardware components
> should have its own node.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Co-developed-by: Baruch Siach <baruch.siach@siklu.com>
> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq6018.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> index 40f1c262126eff3761430a47472b52d27f961040..7866844cc09fd2c2c2f512ce2c8fa7826fabc7aa 100644
> --- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> @@ -413,6 +413,16 @@ tcsr: syscon@1937000 {
>  			reg = <0x0 0x01937000 0x0 0x21000>;
>  		};
>  
> +		pwm: pwm@1941010 {
> +			compatible = "qcom,ipq6018-pwm";
> +			reg = <0x0 0x01941010 0x0 0x20>;
> +			clocks = <&gcc GCC_ADSS_PWM_CLK>;
> +			assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
> +			assigned-clock-rates = <100000000>;
> +			#pwm-cells = <3>;
> +			status = "disabled";

I don't think there's a reason to disable it by default, but given the
version this patchset is on, I'll be happy to see a post-merge fixup

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v20 4/6] arm64: dts: qcom: ipq5018: add pwm node
  2026-02-04 11:25 ` [PATCH v20 4/6] arm64: dts: qcom: ipq5018: " George Moussalem via B4 Relay
@ 2026-02-04 12:25   ` Konrad Dybcio
  0 siblings, 0 replies; 14+ messages in thread
From: Konrad Dybcio @ 2026-02-04 12:25 UTC (permalink / raw)
  To: george.moussalem, Uwe Kleine-König, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Baruch Siach, Bjorn Andersson,
	Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	Dmitry Baryshkov

On 2/4/26 12:25 PM, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> Describe the PWM block on IPQ5018.
> 
> Although PWM is in the TCSR area, make pwm its own node as simple-mfd
> has been removed from the bindings and as such hardware components
> should have its own node.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v20 5/6] arm64: dts: qcom: ipq5332: add pwm node
  2026-02-04 11:25 ` [PATCH v20 5/6] arm64: dts: qcom: ipq5332: " George Moussalem via B4 Relay
@ 2026-02-04 12:25   ` Konrad Dybcio
  0 siblings, 0 replies; 14+ messages in thread
From: Konrad Dybcio @ 2026-02-04 12:25 UTC (permalink / raw)
  To: george.moussalem, Uwe Kleine-König, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Baruch Siach, Bjorn Andersson,
	Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	Dmitry Baryshkov

On 2/4/26 12:25 PM, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> Describe the PWM block on IPQ5332.
> 
> Although PWM is in the TCSR area, make pwm its own node as simple-mfd
> has been removed from the bindings and as such hardware components
> should have its own node.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v20 6/6] arm64: dts: qcom: ipq9574: add pwm node
  2026-02-04 11:25 ` [PATCH v20 6/6] arm64: dts: qcom: ipq9574: " George Moussalem via B4 Relay
@ 2026-02-04 12:26   ` Konrad Dybcio
  0 siblings, 0 replies; 14+ messages in thread
From: Konrad Dybcio @ 2026-02-04 12:26 UTC (permalink / raw)
  To: george.moussalem, Uwe Kleine-König, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Baruch Siach, Bjorn Andersson,
	Konrad Dybcio
  Cc: linux-arm-msm, linux-pwm, devicetree, linux-kernel,
	Dmitry Baryshkov

On 2/4/26 12:25 PM, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> Describe the PWM block on IPQ9574.
> 
> Although PWM is in the TCSR area, make pwm its own node as simple-mfd
> has been removed from the bindings and as such hardware components
> should have its own node.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block
  2026-02-04 11:25 ` [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block George Moussalem via B4 Relay
@ 2026-04-02 15:35   ` Uwe Kleine-König
  2026-04-03 10:40     ` George Moussalem
  0 siblings, 1 reply; 14+ messages in thread
From: Uwe Kleine-König @ 2026-04-02 15:35 UTC (permalink / raw)
  To: george.moussalem
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Baruch Siach,
	Bjorn Andersson, Konrad Dybcio, linux-arm-msm, linux-pwm,
	devicetree, linux-kernel, Devi Priya, Baruch Siach

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

Hello,

I applied the patch and reviewed it in my editor. Here is the resulting
diff:

diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
index b944ecb456d5..4818d0170d53 100644
--- a/drivers/pwm/pwm-ipq.c
+++ b/drivers/pwm/pwm-ipq.c
@@ -97,9 +97,10 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->polarity != PWM_POLARITY_NORMAL)
 		return -EINVAL;
 
-	if (!ipq_chip->clk_rate)
-		return -EINVAL;
-
+	/*
+	 * XXX Why? A comment please. (Is this already covered by the checks
+	 * below?)
+	 */
 	if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC,
 					       ipq_chip->clk_rate))
 		return -ERANGE;
@@ -107,18 +108,29 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
 	duty_ns = min(state->duty_cycle, period_ns);
 
+	/*
+	 * Pick the maximal value for PWM_DIV that still allows a
+	 * 100% relative duty cycle. This allows a fine grained
+	 * selection of duty cycles.
+	 */
 	pwm_div = IPQ_PWM_MAX_DIV - 1;
+
+	/*
+	 * XXX mul_u64_u64_div_u64 returns an u64, this might overflow the
+	 * unsigned int pre_div.
+	 */
 	pre_div = mul_u64_u64_div_u64(period_ns, ipq_chip->clk_rate,
 				      (u64)NSEC_PER_SEC * (pwm_div + 1));
-	pre_div = (pre_div > 0) ? pre_div - 1 : 0;
+
+	if (!pre_div)
+		return -ERANGE;
+
+	pre_div -= 1;
 
 	if (pre_div > IPQ_PWM_MAX_DIV)
 		pre_div = IPQ_PWM_MAX_DIV;
 
-	/*
-	 * high duration = pwm duty * (pwm div + 1)
-	 * pwm duty = duty_ns / period_ns
-	 */
+	/* pwm duty = HI_DUR * (PRE_DIV + 1) / clk_rate */
 	hi_dur = mul_u64_u64_div_u64(duty_ns, ipq_chip->clk_rate,
 				     (u64)(pre_div + 1) * NSEC_PER_SEC);
 
@@ -161,6 +173,10 @@ static int ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
 
 	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
+
+	/*
+	 * effective_div <= 0x100000000, so the multiplication doesn't overflow.
+	 */
 	state->period = DIV64_U64_ROUND_UP(effective_div * NSEC_PER_SEC,
 					   ipq_chip->clk_rate);
 
@@ -210,6 +226,8 @@ static int ipq_pwm_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, ret, "Failed to lock clock rate\n");
 
 	pwm->clk_rate = clk_get_rate(clk);
+	if (!pwm->clk_rate)
+		return dev_err_probe(dev, -EINVAL, "Failed due to clock rate being zero\n");
 
 	chip->ops = &ipq_pwm_ops;
 

Comments with XXX need more code adaptions (or a comment why my concern
isn't justified).

Best regards
Uwe

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block
  2026-04-02 15:35   ` Uwe Kleine-König
@ 2026-04-03 10:40     ` George Moussalem
  2026-04-04 21:09       ` Uwe Kleine-König
  0 siblings, 1 reply; 14+ messages in thread
From: George Moussalem @ 2026-04-03 10:40 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Baruch Siach,
	Bjorn Andersson, Konrad Dybcio, linux-arm-msm, linux-pwm,
	devicetree, linux-kernel, Devi Priya, Baruch Siach

Hi Uwe,

On 4/2/2026 5:35 PM, Uwe Kleine-König wrote:
> Hello,
> 
> I applied the patch and reviewed it in my editor. Here is the resulting
> diff:
> 
> diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
> index b944ecb456d5..4818d0170d53 100644
> --- a/drivers/pwm/pwm-ipq.c
> +++ b/drivers/pwm/pwm-ipq.c
> @@ -97,9 +97,10 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (state->polarity != PWM_POLARITY_NORMAL)
>  		return -EINVAL;
>  
> -	if (!ipq_chip->clk_rate)
> -		return -EINVAL;
> -
> +	/*
> +	 * XXX Why? A comment please. (Is this already covered by the checks
> +	 * below?)
> +	 */

This check can be safely removed as it is indeed covered by the check
where the period_ns is limited to IPQ_PWM_MAX_PERIOD_NS which equals to
NSEC_PER_SEC as per macro definition above.

>  	if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC,
>  					       ipq_chip->clk_rate))
>  		return -ERANGE;
> @@ -107,18 +108,29 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
>  	duty_ns = min(state->duty_cycle, period_ns);
>  
> +	/*
> +	 * Pick the maximal value for PWM_DIV that still allows a
> +	 * 100% relative duty cycle. This allows a fine grained
> +	 * selection of duty cycles.
> +	 */
>  	pwm_div = IPQ_PWM_MAX_DIV - 1;
> +
> +	/*
> +	 * XXX mul_u64_u64_div_u64 returns an u64, this might overflow the
> +	 * unsigned int pre_div.
> +	 */

Theoretically, yes, but in practice it won't due to above constraints.
Take the max period of 10^9 (NSEC_PER_SEC) * max clock rate of 10^9 (1
GHz), then the numerator becomes 10^18. Divide that by 10^9
(NSEC_PER_SEC) * 65,535 (IPQ_PWM_MAX_DIV) and that fits well into a
32-bit integer.

>  	pre_div = mul_u64_u64_div_u64(period_ns, ipq_chip->clk_rate,
>  				      (u64)NSEC_PER_SEC * (pwm_div + 1));
> -	pre_div = (pre_div > 0) ? pre_div - 1 : 0;
> +
> +	if (!pre_div)
> +		return -ERANGE;
> +
> +	pre_div -= 1;
>  
>  	if (pre_div > IPQ_PWM_MAX_DIV)
>  		pre_div = IPQ_PWM_MAX_DIV;
>  
> -	/*
> -	 * high duration = pwm duty * (pwm div + 1)
> -	 * pwm duty = duty_ns / period_ns
> -	 */
> +	/* pwm duty = HI_DUR * (PRE_DIV + 1) / clk_rate */
>  	hi_dur = mul_u64_u64_div_u64(duty_ns, ipq_chip->clk_rate,
>  				     (u64)(pre_div + 1) * NSEC_PER_SEC);
>  
> @@ -161,6 +173,10 @@ static int ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
>  
>  	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
> +
> +	/*
> +	 * effective_div <= 0x100000000, so the multiplication doesn't overflow.
> +	 */
>  	state->period = DIV64_U64_ROUND_UP(effective_div * NSEC_PER_SEC,
>  					   ipq_chip->clk_rate);
>  
> @@ -210,6 +226,8 @@ static int ipq_pwm_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, ret, "Failed to lock clock rate\n");
>  
>  	pwm->clk_rate = clk_get_rate(clk);
> +	if (!pwm->clk_rate)
> +		return dev_err_probe(dev, -EINVAL, "Failed due to clock rate being zero\n");
>  
>  	chip->ops = &ipq_pwm_ops;
>  
> 
> Comments with XXX need more code adaptions (or a comment why my concern
> isn't justified).

Do you want me to send a v21 or can you apply the diff in your tree with
above deletion and comment?

> 
> Best regards
> Uwe

Best regards,
George


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block
  2026-04-03 10:40     ` George Moussalem
@ 2026-04-04 21:09       ` Uwe Kleine-König
  0 siblings, 0 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2026-04-04 21:09 UTC (permalink / raw)
  To: George Moussalem
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Baruch Siach,
	Bjorn Andersson, Konrad Dybcio, linux-arm-msm, linux-pwm,
	devicetree, linux-kernel, Devi Priya, Baruch Siach

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

Hello George,

On Fri, Apr 03, 2026 at 12:40:32PM +0200, George Moussalem wrote:
> On 4/2/2026 5:35 PM, Uwe Kleine-König wrote:
> > diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
> > index b944ecb456d5..4818d0170d53 100644
> > --- a/drivers/pwm/pwm-ipq.c
> > +++ b/drivers/pwm/pwm-ipq.c
> > @@ -97,9 +97,10 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	if (state->polarity != PWM_POLARITY_NORMAL)
> >  		return -EINVAL;
> >  
> > -	if (!ipq_chip->clk_rate)
> > -		return -EINVAL;
> > -
> > +	/*
> > +	 * XXX Why? A comment please. (Is this already covered by the checks
> > +	 * below?)
> > +	 */
> 
> This check can be safely removed as it is indeed covered by the check
> where the period_ns is limited to IPQ_PWM_MAX_PERIOD_NS which equals to
> NSEC_PER_SEC as per macro definition above.
> 
> >  	if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC,
> >  					       ipq_chip->clk_rate))
> >  		return -ERANGE;
> > @@ -107,18 +108,29 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
> >  	duty_ns = min(state->duty_cycle, period_ns);
> >  
> > +	/*
> > +	 * Pick the maximal value for PWM_DIV that still allows a
> > +	 * 100% relative duty cycle. This allows a fine grained
> > +	 * selection of duty cycles.
> > +	 */
> >  	pwm_div = IPQ_PWM_MAX_DIV - 1;
> > +
> > +	/*
> > +	 * XXX mul_u64_u64_div_u64 returns an u64, this might overflow the
> > +	 * unsigned int pre_div.
> > +	 */
> 
> Theoretically, yes, but in practice it won't due to above constraints.
> Take the max period of 10^9 (NSEC_PER_SEC) * max clock rate of 10^9 (1
> GHz), then the numerator becomes 10^18. Divide that by 10^9
> (NSEC_PER_SEC) * 65,535 (IPQ_PWM_MAX_DIV) and that fits well into a
> 32-bit integer.

OK, please put that in a comment.

> Do you want me to send a v21 or can you apply the diff in your tree with
> above deletion and comment?

Yes, please send a v21.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2026-04-04 21:09 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-04 11:25 [PATCH v20 0/6] Add PWM support for IPQ chipsets George Moussalem via B4 Relay
2026-02-04 11:25 ` [PATCH v20 1/6] dt-bindings: pwm: add IPQ6018 binding George Moussalem via B4 Relay
2026-02-04 11:25 ` [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block George Moussalem via B4 Relay
2026-04-02 15:35   ` Uwe Kleine-König
2026-04-03 10:40     ` George Moussalem
2026-04-04 21:09       ` Uwe Kleine-König
2026-02-04 11:25 ` [PATCH v20 3/6] arm64: dts: qcom: ipq6018: add pwm node George Moussalem via B4 Relay
2026-02-04 12:24   ` Konrad Dybcio
2026-02-04 11:25 ` [PATCH v20 4/6] arm64: dts: qcom: ipq5018: " George Moussalem via B4 Relay
2026-02-04 12:25   ` Konrad Dybcio
2026-02-04 11:25 ` [PATCH v20 5/6] arm64: dts: qcom: ipq5332: " George Moussalem via B4 Relay
2026-02-04 12:25   ` Konrad Dybcio
2026-02-04 11:25 ` [PATCH v20 6/6] arm64: dts: qcom: ipq9574: " George Moussalem via B4 Relay
2026-02-04 12:26   ` Konrad Dybcio

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox