[PATCH V15 0/4] Add PWM support for IPQ chipsets

[PATCH V15 0/4] Add PWM support for IPQ chipsets

[Devi Priya]

Add PWM driver and binding support for IPQ chipsets.
Also, add support for pwm node in ipq6018.

V15:
Detailed Change logs are added to the respective patches.

V14 can be found at:
https://lore.kernel.org/linux-arm-msm/20231005043127.2690639-1-quic_devipriy@quicinc.com/

Devi Priya (4):
  pwm: driver for qualcomm ipq6018 pwm block
  dt-bindings: pwm: add IPQ6018 binding
  dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018
  arm64: dts: qcom: ipq6018: add pwm node

 .../devicetree/bindings/mfd/qcom,tcsr.yaml    | 112 +++++--
 .../bindings/pwm/qcom,ipq6018-pwm.yaml        |  45 +++
 arch/arm64/boot/dts/qcom/ipq6018.dtsi         |  15 +-
 drivers/pwm/Kconfig                           |  12 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-ipq.c                         | 282 ++++++++++++++++++
 6 files changed, 435 insertions(+), 32 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
 create mode 100644 drivers/pwm/pwm-ipq.c

-- 
2.34.1

[PATCH V15 1/4] pwm: driver for qualcomm ipq6018 pwm block

[Devi Priya]

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 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>
---
V15:
  No change

V14:
  No change

V13:
  No change

V12:

  Fix the below clang warning for overflow check reported by kernel test robot
  drivers/pwm/pwm-ipq.c:122:11: warning: result of comparison of constant 16000000000
  with expression of type 'unsigned long' is always false [-Wtautological-constant-out-of-range-compare]
>>            if (rate > 16ULL * GIGA)

v11:

Address comment from Uwe Kleine-König:

  Drop redundant registers field comments

  Fix period limit check in .apply

  Clarify the comment explaining skip of pre_div > pwm_div values

  Add explicit check for clock rate within limit

  Add comment explaining the selection of initial pre_div

  Use pwm_div division with remainder instead of separate diff calculation

  Round up duty_cycle calculation in .get_state

v10:

  Restore round up in pwm_div calculation; otherwise diff is always <=
  0, so only bingo match works

  Don't overwrite min_diff on every loop iteration

v9:

Address comment from Uwe Kleine-König:

  Use period_ns*rate in dividers calculation for better accuracy

  Round down pre_div and pwm_div

  Add a comment explaining why pwm_div can't underflow

  Add a comment explaining why pre_div > pwm_div end the search loop

  Drop 'CFG_' from register macros

  Rename to_ipq_pwm_chip() to ipq_pwm_from_chip()

  Change bare 'unsigned' to 'unsigned int'

  Clarify the comment on separate REG1 write for enable/disable
  Round up the period value in .get_state

  Use direct readl/writel so no need to check for regmap errors

v7:

  Change 'offset' to 'reg' for the tcsr offset (Rob)

  Drop clock name; there is only one clock (Bjorn)

  Simplify probe failure code path (Bjorn)

v6:

Address Uwe Kleine-König review comments:

  Drop IPQ_PWM_MAX_DEVICES

  Rely on assigned-clock-rates; drop IPQ_PWM_CLK_SRC_FREQ

  Simplify register offset calculation

  Calculate duty cycle more precisely

  Refuse to set inverted polarity

  Drop redundant IPQ_PWM_REG1_ENABLE bit clear

  Remove x1000 factor in pwm_div calculation, use rate directly, and round up

  Choose initial pre_div such that pwm_div < IPQ_PWM_MAX_DIV

  Ensure pre_div <= pwm_div

  Rename close_ to best_

  Explain in comment why effective_div doesn't overflow

  Limit pwm_div to IPQ_PWM_MAX_DIV - 1 to allow 100% duty cycle

  Disable clock only after pwmchip_remove()

  const pwm_ops

Other changes:

  Add missing linux/bitfield.h header include (kernel test robot)

  Adjust code for PWM device node under TCSR (Rob Herring)

v5:

  Use &tcsr_q6 syscon to access registers (Bjorn Andersson)
  
  Address Uwe Kleine-König review comments:

  Implement .get_state()

  Add IPQ_PWM_ prefix to local macros

  Use GENMASK/BIT/FIELD_PREP for register fields access

  Make type of config_div_and_duty() parameters consistent

  Derive IPQ_PWM_MIN_PERIOD_NS from IPQ_PWM_CLK_SRC_FREQ

  Integrate enable/disable into config_div_and_duty() to save register read,
  and reduce frequency glitch on update

  Use min() instead of min_t()

  Fix comment format

  Use dev_err_probe() to indicate probe step failure

  Add missing clk_disable_unprepare() in .remove

  Don't set .owner

v4:

  Use div64_u64() to fix link for 32-bit targets ((kernel test robot
  <lkp@intel.com>, Uwe Kleine-König)

v3:

  s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)

  Fix integer overflow on 32-bit targets (kernel test robot <lkp@intel.com>)

v2:

Address Uwe Kleine-König review comments:

  Fix period calculation when out of range

  Don't set period larger than requested

  Remove PWM disable on configuration change

  Implement .apply instead of non-atomic .config/.enable/.disable

  Don't modify PWM on .request/.free

  Check pwm_div underflow

  Fix various code and comment formatting issues

Other changes:
  
  Use u64 divisor safe division

  Remove now empty .request/.free

 drivers/pwm/Kconfig   |  12 ++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-ipq.c | 282 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+)
 create mode 100644 drivers/pwm/pwm-ipq.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8ebcddf91f7b..c2d51680823a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -282,6 +282,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 c822389c2a24..1b69e8cb2b91 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -24,6 +24,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 000000000000..5dbe46bb56d6
--- /dev/null
+++ b/drivers/pwm/pwm-ipq.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved.
+ */
+
+#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)
+
+/*
+ * The max value specified for each field is based on the number of bits
+ * in the pwm control register for that field
+ */
+#define IPQ_PWM_MAX_DIV		0xFFFF
+
+/*
+ * 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)
+/*
+ * Enable bit is set to enable output toggling in pwm device.
+ * Update bit is set 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 {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *mem;
+};
+
+static struct ipq_pwm_chip *ipq_pwm_from_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ipq_pwm_chip, 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 void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div,
+				unsigned int pwm_div, unsigned long rate, u64 duty_ns,
+				bool enable)
+{
+	unsigned long hi_dur;
+	unsigned long val = 0;
+
+	/*
+	 * high duration = pwm duty * (pwm div + 1)
+	 * pwm duty = duty_ns / period_ns
+	 */
+	hi_dur = div64_u64(duty_ns * rate, (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 (enable)
+		val |= IPQ_PWM_REG1_ENABLE;
+	ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, val);
+}
+
+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, best_pre_div, best_pwm_div;
+	unsigned long rate = clk_get_rate(ipq_chip->clk);
+	u64 period_ns, duty_ns, period_rate;
+	u64 min_diff;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC, rate))
+		return -ERANGE;
+
+	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
+	duty_ns = min(state->duty_cycle, period_ns);
+
+	/*
+	 * period_ns is 1G or less. As long as rate is less than 16 GHz,
+	 * period_rate does not overflow. Make that explicit.
+	 */
+	if ((unsigned long long)rate > 16ULL * GIGA)
+		return -EINVAL;
+	period_rate = period_ns * rate;
+	best_pre_div = IPQ_PWM_MAX_DIV;
+	best_pwm_div = IPQ_PWM_MAX_DIV;
+	/*
+	 * We don't need to consider pre_div values smaller than
+	 *
+	 *                              period_rate
+	 *  pre_div_min := ------------------------------------
+	 *                 NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1)
+	 *
+	 * because pre_div = pre_div_min results in a better
+	 * approximation.
+	 */
+	pre_div = div64_u64(period_rate,
+			    (u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1));
+	min_diff = period_rate;
+
+	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
+		u64 remainder;
+
+		pwm_div = div64_u64_rem(period_rate,
+					(u64)NSEC_PER_SEC * (pre_div + 1), &remainder);
+		/* pwm_div is unsigned; the check below catches underflow */
+		pwm_div--;
+
+		/*
+		 * Swapping values for pre_div and pwm_div produces the same
+		 * period length. So we can skip all settings with pre_div >
+		 * pwm_div which results in bigger constraints for selecting
+		 * the duty_cycle than with the two values swapped.
+		 */
+		if (pre_div > pwm_div)
+			break;
+
+		/*
+		 * Make sure we can do 100% duty cycle where
+		 * hi_dur == pwm_div + 1
+		 */
+		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
+			continue;
+
+		if (remainder < min_diff) {
+			best_pre_div = pre_div;
+			best_pwm_div = pwm_div;
+			min_diff = remainder;
+
+			if (min_diff == 0) /* bingo */
+				break;
+		}
+	}
+
+	/* config divider values for the closest possible frequency */
+	config_div_and_duty(pwm, best_pre_div, best_pwm_div,
+			    rate, duty_ns, state->enabled);
+
+	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 long rate = clk_get_rate(ipq_chip->clk);
+	unsigned int pre_div, pwm_div, hi_dur;
+	u64 effective_div, hi_div;
+	u32 reg0, reg1;
+
+	reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_REG0);
+	reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_REG1);
+
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
+
+	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);
+
+	/* No overflow here, both pre_div and pwm_div <= 0xffff */
+	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
+	state->period = DIV64_U64_ROUND_UP(effective_div * NSEC_PER_SEC, rate);
+
+	hi_div = hi_dur * (pre_div + 1);
+	state->duty_cycle = DIV64_U64_ROUND_UP(hi_div * NSEC_PER_SEC, rate);
+
+	return 0;
+}
+
+static const struct pwm_ops ipq_pwm_ops = {
+	.apply = ipq_pwm_apply,
+	.get_state = ipq_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int ipq_pwm_probe(struct platform_device *pdev)
+{
+	struct ipq_pwm_chip *pwm;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pwm);
+
+	pwm->mem = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pwm->mem))
+		return dev_err_probe(dev, PTR_ERR(pwm->mem),
+				"regs map failed");
+
+	pwm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk),
+				"failed to get clock");
+
+	ret = clk_prepare_enable(pwm->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "clock enable failed");
+
+	pwm->chip.dev = dev;
+	pwm->chip.ops = &ipq_pwm_ops;
+	pwm->chip.npwm = 4;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "pwmchip_add() failed\n");
+		clk_disable_unprepare(pwm->clk);
+	}
+
+	return ret;
+}
+
+static int ipq_pwm_remove(struct platform_device *pdev)
+{
+	struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&pwm->chip);
+	clk_disable_unprepare(pwm->clk);
+
+	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,
+	.remove = ipq_pwm_remove,
+};
+
+module_platform_driver(ipq_pwm_driver);
+
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.34.1

Re: [PATCH V15 1/4] pwm: driver for qualcomm ipq6018 pwm block

[Uwe Kleine-König]

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

On Thu, Oct 05, 2023 at 09:35:47PM +0530, Devi Priya wrote:
> diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
> new file mode 100644
> index 000000000000..5dbe46bb56d6
> --- /dev/null
> +++ b/drivers/pwm/pwm-ipq.c
> @@ -0,0 +1,282 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved.
> + */
> +
> +#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)
> +
> +/*
> + * The max value specified for each field is based on the number of bits
> + * in the pwm control register for that field
> + */
> +#define IPQ_PWM_MAX_DIV		0xFFFF
> +
> +/*
> + * 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)
> +/*
> + * Enable bit is set to enable output toggling in pwm device.
> + * Update bit is set 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 {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *mem;
> +};
> +
> +static struct ipq_pwm_chip *ipq_pwm_from_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct ipq_pwm_chip, 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 void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div,
> +				unsigned int pwm_div, unsigned long rate, u64 duty_ns,
> +				bool enable)
> +{
> +	unsigned long hi_dur;
> +	unsigned long val = 0;
> +
> +	/*
> +	 * high duration = pwm duty * (pwm div + 1)
> +	 * pwm duty = duty_ns / period_ns
> +	 */
> +	hi_dur = div64_u64(duty_ns * rate, (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 (enable)
> +		val |= IPQ_PWM_REG1_ENABLE;
> +	ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, val);
> +}
> +
> +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, best_pre_div, best_pwm_div;
> +	unsigned long rate = clk_get_rate(ipq_chip->clk);
> +	u64 period_ns, duty_ns, period_rate;
> +	u64 min_diff;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC, rate))
> +		return -ERANGE;
> +
> +	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
> +	duty_ns = min(state->duty_cycle, period_ns);
> +
> +	/*
> +	 * period_ns is 1G or less. As long as rate is less than 16 GHz,
> +	 * period_rate does not overflow. Make that explicit.
> +	 */
> +	if ((unsigned long long)rate > 16ULL * GIGA)
> +		return -EINVAL;
> +	period_rate = period_ns * rate;
> +	best_pre_div = IPQ_PWM_MAX_DIV;
> +	best_pwm_div = IPQ_PWM_MAX_DIV;
> +	/*
> +	 * We don't need to consider pre_div values smaller than
> +	 *
> +	 *                              period_rate
> +	 *  pre_div_min := ------------------------------------
> +	 *                 NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1)
> +	 *
> +	 * because pre_div = pre_div_min results in a better
> +	 * approximation.
> +	 */
> +	pre_div = div64_u64(period_rate,
> +			    (u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1));

Consider period_ns = 0x10000 and rate = NSEC_PER_SEC. Then we get
pre_div = 1 ...

> +	min_diff = period_rate;
> +
> +	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
> +		u64 remainder;
> +
> +		pwm_div = div64_u64_rem(period_rate,
> +					(u64)NSEC_PER_SEC * (pre_div + 1), &remainder);
> +		/* pwm_div is unsigned; the check below catches underflow */
> +		pwm_div--;

... pwm_div then will be 0x7fff and remainder is 0. This is wrong, isn't
it? I think you lack a -1 in the assignment of pre_div above. And/or you
need to round up the division?

Having said that the loop here is quite heavy. I'd opt for restricing
pwm_div to 0xfffe (to get a finegrained domain for HI_DURATION) and then
the appropriate value for pre_div can be computed in a single division.
While being more coarse for the domain of periods, a simple algorithm is
worth quite a lot (for reviewers of your code and also for consumers
that benefit from a small runtime of .apply()) and you get a
fine-grained domain for duty_cycle. In my experience outweighs the
increased precision for the period.

> +		/*
> +		 * Swapping values for pre_div and pwm_div produces the same
> +		 * period length. So we can skip all settings with pre_div >
> +		 * pwm_div which results in bigger constraints for selecting
> +		 * the duty_cycle than with the two values swapped.
> +		 */
> +		if (pre_div > pwm_div)
> +			break;
> +
> +		/*
> +		 * Make sure we can do 100% duty cycle where
> +		 * hi_dur == pwm_div + 1
> +		 */
> +		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
> +			continue;
> +
> +		if (remainder < min_diff) {
> +			best_pre_div = pre_div;
> +			best_pwm_div = pwm_div;
> +			min_diff = remainder;
> +
> +			if (min_diff == 0) /* bingo */
> +				break;
> +		}
> +	}
> +
> +	/* config divider values for the closest possible frequency */
> +	config_div_and_duty(pwm, best_pre_div, best_pwm_div,
> +			    rate, duty_ns, state->enabled);
> +
> +	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 long rate = clk_get_rate(ipq_chip->clk);
> +	unsigned int pre_div, pwm_div, hi_dur;
> +	u64 effective_div, hi_div;
> +	u32 reg0, reg1;
> +
> +	reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_REG0);
> +	reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_REG1);
> +
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
> +
> +	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);
> +
> +	/* No overflow here, both pre_div and pwm_div <= 0xffff */
> +	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);

This cast is not needed.

> +	state->period = DIV64_U64_ROUND_UP(effective_div * NSEC_PER_SEC, rate);
> +
> +	hi_div = hi_dur * (pre_div + 1);
> +	state->duty_cycle = DIV64_U64_ROUND_UP(hi_div * NSEC_PER_SEC, rate);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops ipq_pwm_ops = {
> +	.apply = ipq_pwm_apply,
> +	.get_state = ipq_pwm_get_state,
> +	.owner = THIS_MODULE,

With 384461abcab6 in next
(https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next,
currently for-next~30), the .owner line should be dropped.
..ooOO(You claimed to have dropped that for v5 already in the changelog
above.  Hmm?!)

> +};
> +
> +static int ipq_pwm_probe(struct platform_device *pdev)
> +{
> +	struct ipq_pwm_chip *pwm;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pwm);
> +
> +	pwm->mem = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pwm->mem))
> +		return dev_err_probe(dev, PTR_ERR(pwm->mem),
> +				"regs map failed");
> +
> +	pwm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(pwm->clk),
> +				"failed to get clock");
> +
> +	ret = clk_prepare_enable(pwm->clk);

devm_clk_get_enabled()

> +	if (ret)
> +		return dev_err_probe(dev, ret, "clock enable failed");
> +
> +	pwm->chip.dev = dev;
> +	pwm->chip.ops = &ipq_pwm_ops;
> +	pwm->chip.npwm = 4;
> +
> +	ret = pwmchip_add(&pwm->chip);

devm_pwmchip_add()

> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "pwmchip_add() failed\n");
> +		clk_disable_unprepare(pwm->clk);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ipq_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&pwm->chip);
> +	clk_disable_unprepare(pwm->clk);
> +
> +	return 0;
> +}

With the above suggestions .remove can be dropped. If there is still a
reason to keep it in the next revision, please switch to .remove_new().

> +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,
> +	.remove = ipq_pwm_remove,
> +};
> +
> +module_platform_driver(ipq_pwm_driver);
> +
> +MODULE_LICENSE("Dual BSD/GPL");

I don't oppose to this dual licensing, but I wonder what the motivation
is. Seeing that the copyright is assigned to the Linux Foundation, is
this related to Zephyr? (Which however uses an Apache license.)

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 --]

[PATCH V15 2/4] dt-bindings: pwm: add IPQ6018 binding

[Devi Priya]

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>
---
v15:

  No change

v14:

  Picked up the R-b tag

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

v12:

  Picked up the R-b tag

v11:

  No change

v10:

  No change

v9:

  Add 'ranges' property to example (Rob)

  Drop label in example (Rob)

v8:

  Add size cell to 'reg' (Rob)

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

v6:

  Device node is child of TCSR; remove phandle (Rob Herring)

  Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)

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

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

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

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

 .../bindings/pwm/qcom,ipq6018-pwm.yaml        | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml

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 000000000000..6d0d7ed271f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
@@ -0,0 +1,45 @@
+# 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:
+  - Baruch Siach <baruch@tkos.co.il>
+
+properties:
+  compatible:
+    const: qcom,ipq6018-pwm
+
+  reg:
+    description: Offset of PWM register in the TCSR block.
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#pwm-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
+
+    pwm: pwm@a010 {
+        compatible = "qcom,ipq6018-pwm";
+        reg = <0xa010 0x20>;
+        clocks = <&gcc GCC_ADSS_PWM_CLK>;
+        assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+        assigned-clock-rates = <100000000>;
+        #pwm-cells = <2>;
+    };
-- 
2.34.1

Re: [PATCH V15 2/4] dt-bindings: pwm: add IPQ6018 binding

[Uwe Kleine-König]

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

Hello,

On Thu, Oct 05, 2023 at 09:35:48PM +0530, Devi Priya wrote:
> 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>
> ---
> v15:
> 
>   No change
> 
> v14:
> 
>   Picked up the R-b tag
> 
> 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
> 
> v12:
> 
>   Picked up the R-b tag
> 
> v11:
> 
>   No change
> 
> v10:
> 
>   No change
> 
> v9:
> 
>   Add 'ranges' property to example (Rob)
> 
>   Drop label in example (Rob)
> 
> v8:
> 
>   Add size cell to 'reg' (Rob)
> 
> 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
> 
> v6:
> 
>   Device node is child of TCSR; remove phandle (Rob Herring)
> 
>   Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)
> 
> v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn
>     Andersson, Kathiravan T)
> 
> v4: Update the binding example node as well (Rob Herring's bot)
> 
> v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)
> 
> v2: Make #pwm-cells const (Rob Herring)
> 
>  .../bindings/pwm/qcom,ipq6018-pwm.yaml        | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
> 
> 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 000000000000..6d0d7ed271f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
> @@ -0,0 +1,45 @@
> +# 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:
> +  - Baruch Siach <baruch@tkos.co.il>

Not being very fluent in dt and binding yaml I wonder if adding

	allOf:
	  - $ref: pwm.yaml#

would be beneficial?!

> +properties:
> +  compatible:
> +    const: qcom,ipq6018-pwm
> +
> +  reg:
> +    description: Offset of PWM register in the TCSR block.
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 2

The driver only supports normal polarity. Is this a shortcoming of the
driver, or is the hardware incapable to do that, too?

If it's only the former I'd want #pwm-cells = <3> here. For ease of use
I'd not oppose if you pick #pwm-cells = <3> even if the hardware can
only do normal polarity.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - "#pwm-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
> +
> +    pwm: pwm@a010 {
> +        compatible = "qcom,ipq6018-pwm";
> +        reg = <0xa010 0x20>;
> +        clocks = <&gcc GCC_ADSS_PWM_CLK>;
> +        assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
> +        assigned-clock-rates = <100000000>;
> +        #pwm-cells = <2>;
> +    };

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 V15 2/4] dt-bindings: pwm: add IPQ6018 binding

[Rob Herring]

On Wed, Oct 18, 2023 at 3:46 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Thu, Oct 05, 2023 at 09:35:48PM +0530, Devi Priya wrote:
> > 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>
> > ---
> > v15:
> >
> >   No change
> >
> > v14:
> >
> >   Picked up the R-b tag
> >
> > 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
> >
> > v12:
> >
> >   Picked up the R-b tag
> >
> > v11:
> >
> >   No change
> >
> > v10:
> >
> >   No change
> >
> > v9:
> >
> >   Add 'ranges' property to example (Rob)
> >
> >   Drop label in example (Rob)
> >
> > v8:
> >
> >   Add size cell to 'reg' (Rob)
> >
> > 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
> >
> > v6:
> >
> >   Device node is child of TCSR; remove phandle (Rob Herring)
> >
> >   Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)
> >
> > v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn
> >     Andersson, Kathiravan T)
> >
> > v4: Update the binding example node as well (Rob Herring's bot)
> >
> > v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)
> >
> > v2: Make #pwm-cells const (Rob Herring)
> >
> >  .../bindings/pwm/qcom,ipq6018-pwm.yaml        | 45 +++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
> >
> > 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 000000000000..6d0d7ed271f7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
> > @@ -0,0 +1,45 @@
> > +# 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:
> > +  - Baruch Siach <baruch@tkos.co.il>
>
> Not being very fluent in dt and binding yaml I wonder if adding
>
>         allOf:
>           - $ref: pwm.yaml#
>
> would be beneficial?!

Not really because the only thing you pick up is #pwm-cells, but
that's still needed here since that varies by binding. A reference
generally becomes useful when there are child nodes (e.g. a bus
binding) or multiple properties.

> > +properties:
> > +  compatible:
> > +    const: qcom,ipq6018-pwm
> > +
> > +  reg:
> > +    description: Offset of PWM register in the TCSR block.
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  "#pwm-cells":
> > +    const: 2
>
> The driver only supports normal polarity. Is this a shortcoming of the
> driver, or is the hardware incapable to do that, too?
>
> If it's only the former I'd want #pwm-cells = <3> here. For ease of use
> I'd not oppose if you pick #pwm-cells = <3> even if the hardware can
> only do normal polarity.

Devi, Can we get an answer here soon.

The MFD part has been applied and it references this schema causing
warnings. So this needs to land or MFD schema reverted.

Rob

Re: [PATCH V15 2/4] dt-bindings: pwm: add IPQ6018 binding

[Kathiravan Thirumoorthy]

On 10/20/2023 8:44 PM, Rob Herring wrote:
> On Wed, Oct 18, 2023 at 3:46 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>> Hello,
>>
>> On Thu, Oct 05, 2023 at 09:35:48PM +0530, Devi Priya wrote:
>>> 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>
>>> ---
>>> v15:
>>>
>>>    No change
>>>
>>> v14:
>>>
>>>    Picked up the R-b tag
>>>
>>> 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
>>>
>>> v12:
>>>
>>>    Picked up the R-b tag
>>>
>>> v11:
>>>
>>>    No change
>>>
>>> v10:
>>>
>>>    No change
>>>
>>> v9:
>>>
>>>    Add 'ranges' property to example (Rob)
>>>
>>>    Drop label in example (Rob)
>>>
>>> v8:
>>>
>>>    Add size cell to 'reg' (Rob)
>>>
>>> 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
>>>
>>> v6:
>>>
>>>    Device node is child of TCSR; remove phandle (Rob Herring)
>>>
>>>    Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)
>>>
>>> v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn
>>>      Andersson, Kathiravan T)
>>>
>>> v4: Update the binding example node as well (Rob Herring's bot)
>>>
>>> v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)
>>>
>>> v2: Make #pwm-cells const (Rob Herring)
>>>
>>>   .../bindings/pwm/qcom,ipq6018-pwm.yaml        | 45 +++++++++++++++++++
>>>   1 file changed, 45 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
>>>
>>> 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 000000000000..6d0d7ed271f7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pwm/qcom,ipq6018-pwm.yaml
>>> @@ -0,0 +1,45 @@
>>> +# 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:
>>> +  - Baruch Siach <baruch@tkos.co.il>
>> Not being very fluent in dt and binding yaml I wonder if adding
>>
>>          allOf:
>>            - $ref: pwm.yaml#
>>
>> would be beneficial?!
> Not really because the only thing you pick up is #pwm-cells, but
> that's still needed here since that varies by binding. A reference
> generally becomes useful when there are child nodes (e.g. a bus
> binding) or multiple properties.
>
>>> +properties:
>>> +  compatible:
>>> +    const: qcom,ipq6018-pwm
>>> +
>>> +  reg:
>>> +    description: Offset of PWM register in the TCSR block.
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  "#pwm-cells":
>>> +    const: 2
>> The driver only supports normal polarity. Is this a shortcoming of the
>> driver, or is the hardware incapable to do that, too?
>>
>> If it's only the former I'd want #pwm-cells = <3> here. For ease of use
>> I'd not oppose if you pick #pwm-cells = <3> even if the hardware can
>> only do normal polarity.
> Devi, Can we get an answer here soon.


Rob, Devi is on vacation for couple of weeks. I don't have much idea on 
this as of now, how can I help here, if needed?


>
> The MFD part has been applied and it references this schema causing
> warnings. So this needs to land or MFD schema reverted.
>
> Rob

Re: [PATCH V15 2/4] dt-bindings: pwm: add IPQ6018 binding

[Uwe Kleine-König]

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

Hello,

On Fri, Oct 20, 2023 at 10:14:48AM -0500, Rob Herring wrote:
> On Wed, Oct 18, 2023 at 3:46 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Oct 05, 2023 at 09:35:48PM +0530, Devi Priya wrote:
> > > +  "#pwm-cells":
> > > +    const: 2
> >
> > The driver only supports normal polarity. Is this a shortcoming of the
> > driver, or is the hardware incapable to do that, too?
> >
> > If it's only the former I'd want #pwm-cells = <3> here. For ease of use
> > I'd not oppose if you pick #pwm-cells = <3> even if the hardware can
> > only do normal polarity.
> 
> Devi, Can we get an answer here soon.
> 
> The MFD part has been applied and it references this schema causing
> warnings. So this needs to land or MFD schema reverted.

Or the reference to the pwm stuff deleted from the mfd binding?

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 --]

[PATCH V15 3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018

[Devi Priya]

Update the binding to include pwm as the child node to TCSR block and
add simple-mfd support for IPQ6018.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
v15:

 Picked up the R-b tag

v14:

Addressed comments suggested by krzysztof

 Added type: object to patternProperties and added the complete path to
 pwm.yaml in the reference

 Disallow pwm for targets other than ipq6018

 Moved ranges property just after reg in the examples

v13:

 Added simple-mfd support for IPQ6018 based devices

 Added support to include pwm as the child node to TCSR

 Included syscon node found on IPQ6018 to the examples

 .../devicetree/bindings/mfd/qcom,tcsr.yaml    | 112 +++++++++++++-----
 1 file changed, 81 insertions(+), 31 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml b/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
index 33c3d023a106..f836b973e382 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
@@ -15,50 +15,100 @@ description:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - qcom,msm8976-tcsr
-          - qcom,msm8998-tcsr
-          - qcom,qcs404-tcsr
-          - qcom,sc7180-tcsr
-          - qcom,sc7280-tcsr
-          - qcom,sc8280xp-tcsr
-          - qcom,sdm630-tcsr
-          - qcom,sdm845-tcsr
-          - qcom,sdx55-tcsr
-          - qcom,sdx65-tcsr
-          - qcom,sm4450-tcsr
-          - qcom,sm8150-tcsr
-          - qcom,sm8450-tcsr
-          - qcom,tcsr-apq8064
-          - qcom,tcsr-apq8084
-          - qcom,tcsr-ipq5332
-          - qcom,tcsr-ipq6018
-          - qcom,tcsr-ipq8064
-          - qcom,tcsr-ipq8074
-          - qcom,tcsr-ipq9574
-          - qcom,tcsr-mdm9615
-          - qcom,tcsr-msm8226
-          - qcom,tcsr-msm8660
-          - qcom,tcsr-msm8916
-          - qcom,tcsr-msm8953
-          - qcom,tcsr-msm8960
-          - qcom,tcsr-msm8974
-          - qcom,tcsr-msm8996
-      - const: syscon
+    oneOf:
+      - items:
+          - enum:
+              - qcom,msm8976-tcsr
+              - qcom,msm8998-tcsr
+              - qcom,qcs404-tcsr
+              - qcom,sc7180-tcsr
+              - qcom,sc7280-tcsr
+              - qcom,sc8280xp-tcsr
+              - qcom,sdm630-tcsr
+              - qcom,sdm845-tcsr
+              - qcom,sdx55-tcsr
+              - qcom,sdx65-tcsr
+              - qcom,sm4450-tcsr
+              - qcom,sm8150-tcsr
+              - qcom,sm8450-tcsr
+              - qcom,tcsr-apq8064
+              - qcom,tcsr-apq8084
+              - qcom,tcsr-ipq5332
+              - qcom,tcsr-ipq8064
+              - qcom,tcsr-ipq8074
+              - qcom,tcsr-ipq9574
+              - qcom,tcsr-mdm9615
+              - qcom,tcsr-msm8226
+              - qcom,tcsr-msm8660
+              - qcom,tcsr-msm8916
+              - qcom,tcsr-msm8953
+              - qcom,tcsr-msm8960
+              - qcom,tcsr-msm8974
+              - qcom,tcsr-msm8996
+          - const: syscon
+      - items:
+          - const: qcom,tcsr-ipq6018
+          - const: syscon
+          - const: simple-mfd
 
   reg:
     maxItems: 1
 
+  ranges: true
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+patternProperties:
+  "pwm@[a-f0-9]+$":
+    type: object
+    $ref: /schemas/pwm/qcom,ipq6018-pwm.yaml
+
 required:
   - compatible
   - reg
 
+allOf:
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - qcom,tcsr-ipq6018
+    then:
+      patternProperties:
+        "pwm@[a-f0-9]+$": false
+
 additionalProperties: false
 
 examples:
+  # Example 1 - Syscon node found on MSM8960
   - |
     syscon@1a400000 {
         compatible = "qcom,tcsr-msm8960", "syscon";
         reg = <0x1a400000 0x100>;
     };
+  # Example 2 - Syscon node found on IPQ6018
+  - |
+    #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
+
+    syscon@1937000 {
+        compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd";
+        reg = <0x01937000 0x21000>;
+        ranges = <0 0x1937000 0x21000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        pwm: pwm@a010 {
+            compatible = "qcom,ipq6018-pwm";
+            reg = <0xa010 0x20>;
+            clocks = <&gcc GCC_ADSS_PWM_CLK>;
+            assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+            assigned-clock-rates = <100000000>;
+            #pwm-cells = <2>;
+        };
+    };
-- 
2.34.1

Re: (subset) [PATCH V15 3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018

[Lee Jones]

On Thu, 05 Oct 2023 21:35:49 +0530, Devi Priya wrote:
> Update the binding to include pwm as the child node to TCSR block and
> add simple-mfd support for IPQ6018.
> 
> 

Applied, thanks!

[3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018
      commit: b4a32d218d424b81a58fbd419e1114b1c1f76168

--
Lee Jones [李琼斯]

Re: (subset) [PATCH V15 3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018

[Rob Herring]

On Thu, Oct 12, 2023 at 5:06 AM Lee Jones <lee@kernel.org> wrote:
>
> On Thu, 05 Oct 2023 21:35:49 +0530, Devi Priya wrote:
> > Update the binding to include pwm as the child node to TCSR block and
> > add simple-mfd support for IPQ6018.
> >
> >
>
> Applied, thanks!
>
> [3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018
>       commit: b4a32d218d424b81a58fbd419e1114b1c1f76168

This is dependent on patch 2 being applied.

Rob

Re: (subset) [PATCH V15 3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018

[Lee Jones]

On Thu, 19 Oct 2023, Rob Herring wrote:

> On Thu, Oct 12, 2023 at 5:06 AM Lee Jones <lee@kernel.org> wrote:
> >
> > On Thu, 05 Oct 2023 21:35:49 +0530, Devi Priya wrote:
> > > Update the binding to include pwm as the child node to TCSR block and
> > > add simple-mfd support for IPQ6018.
> > >
> > >
> >
> > Applied, thanks!
> >
> > [3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018
> >       commit: b4a32d218d424b81a58fbd419e1114b1c1f76168
> 
> This is dependent on patch 2 being applied.

I'll pull it for now then.

Unapplied, thanks.

-- 
Lee Jones [李琼斯]

[PATCH V15 4/4] arm64: dts: qcom: ipq6018: add pwm node

[Devi Priya]

Describe the PWM block on IPQ6018.

The PWM is in the TCSR area. Make &tcsr "simple-mfd" compatible, and add
&pwm as child of &tcsr.

Add also ipq6018 specific compatible string.

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>
---
v15:

  Fixed the indentation of pwm node

v14:

  Moved ranges just after reg as suggested by Krzysztof

  Picked up the R-b tag

v13:

  No change

v12: 

  No change

v11:

  No change

v10:

  No change

v9:

  Add 'ranges' property (Rob)

v8:

  Add size cell to 'reg' (Rob)

v7:

  Use 'reg' instead of 'offset' (Rob)

  Add qcom,tcsr-ipq6018 (Rob)

  Drop clock-names (Bjorn)

v6:

  Make the PWM node child of TCSR (Rob Herring)

  Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)

v5: Use qcom,pwm-regs for TCSR phandle instead of direct regs

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

 arch/arm64/boot/dts/qcom/ipq6018.dtsi | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index e59b9df96c7e..18f9fffba08b 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -390,8 +390,21 @@ tcsr_mutex: hwlock@1905000 {
 		};
 
 		tcsr: syscon@1937000 {
-			compatible = "qcom,tcsr-ipq6018", "syscon";
+			compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd";
 			reg = <0x0 0x01937000 0x0 0x21000>;
+			ranges = <0x0 0x0 0x01937000 0x21000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			pwm: pwm@a010 {
+				compatible = "qcom,ipq6018-pwm";
+				reg = <0xa010 0x20>;
+				clocks = <&gcc GCC_ADSS_PWM_CLK>;
+				assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+				assigned-clock-rates = <100000000>;
+				#pwm-cells = <2>;
+				status = "disabled";
+			};
 		};
 
 		usb2: usb@70f8800 {
-- 
2.34.1

Re: [PATCH V15 0/4] Add PWM support for IPQ chipsets

[Lee Jones]

On Thu, 05 Oct 2023 21:35:46 +0530, Devi Priya wrote:
> Add PWM driver and binding support for IPQ chipsets.
> Also, add support for pwm node in ipq6018.
> 
> V15:
> Detailed Change logs are added to the respective patches.
> 
> V14 can be found at:
> https://lore.kernel.org/linux-arm-msm/20231005043127.2690639-1-quic_devipriy@quicinc.com/
> 
> [...]

Applied, thanks!

[1/4] pwm: driver for qualcomm ipq6018 pwm block
      (no commit info)
[2/4] dt-bindings: pwm: add IPQ6018 binding
      (no commit info)
[3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018
      commit: b4a32d218d424b81a58fbd419e1114b1c1f76168
[4/4] arm64: dts: qcom: ipq6018: add pwm node
      (no commit info)

--
Lee Jones [李琼斯]

Re: [PATCH V15 0/4] Add PWM support for IPQ chipsets

[Lee Jones]

On Thu, 12 Oct 2023, Lee Jones wrote:

> On Thu, 05 Oct 2023 21:35:46 +0530, Devi Priya wrote:
> > Add PWM driver and binding support for IPQ chipsets.
> > Also, add support for pwm node in ipq6018.
> > 
> > V15:
> > Detailed Change logs are added to the respective patches.
> > 
> > V14 can be found at:
> > https://lore.kernel.org/linux-arm-msm/20231005043127.2690639-1-quic_devipriy@quicinc.com/
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/4] pwm: driver for qualcomm ipq6018 pwm block
>       (no commit info)
> [2/4] dt-bindings: pwm: add IPQ6018 binding
>       (no commit info)
> [3/4] dt-bindings: mfd: qcom,tcsr: Add simple-mfd support for IPQ6018
>       commit: b4a32d218d424b81a58fbd419e1114b1c1f76168
> [4/4] arm64: dts: qcom: ipq6018: add pwm node
>       (no commit info)

Disregard - tooling error!

-- 
Lee Jones [李琼斯]

Re: [PATCH V15 0/4] Add PWM support for IPQ chipsets

[Krzysztof Kozlowski]

On 05/10/2023 18:05, Devi Priya wrote:
> Add PWM driver and binding support for IPQ chipsets.
> Also, add support for pwm node in ipq6018.
> 

You need to clearly mark dependencies. Next is now broken because of
this patchset.

Best regards,
Krzysztof

Re: [PATCH V15 0/4] Add PWM support for IPQ chipsets

[Uwe Kleine-König]

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

Hello,

On Wed, Oct 18, 2023 at 06:29:30PM +0200, Krzysztof Kozlowski wrote:
> On 05/10/2023 18:05, Devi Priya wrote:
> > Add PWM driver and binding support for IPQ chipsets.
> > Also, add support for pwm node in ipq6018.
> > 
> 
> You need to clearly mark dependencies.

This is something I wouldn't blame Devi for. The dependency is not very
obvious and its kind of normal and expected for a patch series to have
dependencies. *shrug*

> Next is now broken because of this patchset.

If I understand correctly this affects "only" the dtb check targets,
right? Is this bad enough that it needs an urgent fix? I would expect it
doesn't hurt much, am I right here?

I just looked into patch #2 and had a few comments for it.

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 V15 0/4] Add PWM support for IPQ chipsets

[Krzysztof Kozlowski]

On 18/10/2023 22:52, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Oct 18, 2023 at 06:29:30PM +0200, Krzysztof Kozlowski wrote:
>> On 05/10/2023 18:05, Devi Priya wrote:
>>> Add PWM driver and binding support for IPQ chipsets.
>>> Also, add support for pwm node in ipq6018.
>>>
>>
>> You need to clearly mark dependencies.
> 
> This is something I wouldn't blame Devi for. The dependency is not very
> obvious and its kind of normal and expected for a patch series to have
> dependencies. *shrug*
> 
>> Next is now broken because of this patchset.
> 
> If I understand correctly this affects "only" the dtb check targets,
> right? Is this bad enough that it needs an urgent fix? I would expect it
> doesn't hurt much, am I right here?
> 
> I just looked into patch #2 and had a few comments for it.

Check/Tests of bindings (make dt_binding_check) has warnings because of
missing PWM schema.

Best regards,
Krzysztof