Add QCOM PMIC PWM driver

Add bindings for QCOM PMIC PWM modules which are accessed through SPMI bus.

Signed-off-by Fenglin Wu <[email protected]>

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

Quoted 11 lines · Fenglin Wu

Add bindings for QCOM PMIC PWM modules which are accessed through SPMI bus.

Signed-off-by Fenglin Wu <[email protected]>

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

Documentation/devicetree/bindings/pwm/pwm-qcom.yaml

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/pwm/pwm-qcom.yaml:29:6: [warning] wrong indentation: expected 4 but found 5 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml:
Additional properties are not allowed ('Properties' was unexpected) /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml:
Additional properties are not allowed ('Properties' was unexpected) /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml:
'anyOf' conditional failed, one must be fixed:
'properties' is a required property
'patternProperties' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml:
ignoring, error in schema:
warning: no schema found in file:
./Documentation/devicetree/bindings/pwm/pwm-qcom.yaml Documentation/devicetree/bindings/pwm/pwm-qcom.example.dts:21.13-28:
Warning (reg_format): /example-0/pwms@e800:reg: property has invalid
length (4 bytes) (#address-cells == 1, #size-cells == 1) Documentation/devicetree/bindings/pwm/pwm-qcom.example.dt.yaml:
Warning (pci_device_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/pwm/pwm-qcom.example.dt.yaml:
Warning (pci_device_bus_num): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/pwm/pwm-qcom.example.dt.yaml:
Warning (simple_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/pwm/pwm-qcom.example.dt.yaml:
Warning (i2c_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/pwm/pwm-qcom.example.dt.yaml:
Warning (spi_bus_reg): Failed prerequisite 'reg_format' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/pwm-qcom.example.dt.yaml:
example-0: pwms@e800:reg:0: [59392] is too short
From schema:
/usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml Documentation/devicetree/bindings/pwm/pwm-qcom.example.dt.yaml:0:0:
/example-0/pwms@e800: failed to match any schema with compatible:
['qcom,pwm']

See https://patchwork.ozlabs.org/patch/1470623

This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit.

Add bindings for QCOM PMIC PWM modules which are accessed through SPMI bus.

Signed-off-by Fenglin Wu <[email protected]>

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

Documentation/devicetree/bindings/pwm/pwm-qcom.yaml new file +9 -0

new file mode 100644
index 0000000..e8d8ed6
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-qcom.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. PMIC PWM bindings
+
+maintainers:

PWM modules present in QCOM PMICs are controlled through SPMI bus. Normally, it would have several PWM modules together with adjacent register space and each PWM module can be controlled independently.

Signed-off-by Fenglin Wu <[email protected]>

---
 drivers/pwm/Kconfig    |   9 +
 drivers/pwm/Makefile   |   1 +
 drivers/pwm/pwm-qcom.c | 585 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 595 insertions(+)
 create mode 100644 drivers/pwm/pwm-qcom.c

drivers/pwm/Kconfig +9 -0

index 8ae68d6..324ab5d 100644
@@ -423,6 +423,15 @@ config PWM_PXA
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-pxa.
 
+config PWM_QCOM
+	tristate "Qcom PMIC PWM support"
+	depends on MFD_SPMI_PMIC && OF
+	help
+	  Generic PWM framework driver for PWM module in QCOM PMIC chips
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-qcom.
+
 config PWM_RCAR
 	tristate "Renesas R-Car PWM support"
 	depends on ARCH_RENESAS || COMPILE_TEST

drivers/pwm/Makefile +1 -0

index d43b1e1..78316e9 100644
@@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
+obj-$(CONFIG_PWM_QCOM)		+= pwm-qcom.o
 obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o

drivers/pwm/pwm-qcom.c new file +0 -0

new file mode 100644
index 0000000..48bd823

drivers/pwm/pwm-qcom.c new file +32 -0

+ */
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+/* PWM module registers */
+#define REG_PERPH_SUBTYPE		0x05
+#define REG_PWM_SIZE_CLK		0x41
+#define REG_PWM_FREQ_PREDIV_CLK		0x42
+#define REG_PWM_TYPE_CONFIG		0x43
+#define REG_PWM_VALUE_LSB		0x44
+#define REG_PWM_VALUE_MSB		0x45
+#define REG_ENABLE_CONTROL		0x46
+#define REG_PWM_SYNC			0x47
+
+/* REG_PERPH_SUBTYPE */
+#define SUBTYPE_PWM			0x0b
+#define SUBTYPE_PWM_LITE		0x11
+

drivers/pwm/pwm-qcom.c new file +4 -0

+#define PWM_FREQ_EXPONENT_MASK		GENMASK(2, 0)
+
+/* REG_PWM_TYPE_CONFIG */
+#define PWM_EN_GLITCH_REMOVAL_MASK	BIT(5)

drivers/pwm/pwm-qcom.c new file +29 -0

+struct qcom_pwm_config {
+	u32	pwm_size;
+	u32	pwm_clk;
+	u32	prediv;
+	u32	clk_exp;
+	u16	pwm_value;
+	u64	best_period_ns;
+};
+
+struct qcom_pwm_channel {
+	struct qcom_pwm_chip	*chip;
+	struct qcom_pwm_config	pwm_config;
+	u32			chan_idx;
+	u32			reg_base;
+	u8			subtype;
+	u64			current_period_ns;
+	u64			current_duty_ns;
+};
+
+struct qcom_pwm_chip {
+	struct pwm_chip		pwm_chip;
+	struct regmap		*regmap;
+	struct device		*dev;
+	struct qcom_pwm_channel	*pwms;
+	struct mutex		rw_lock;
+	u32			num_channels;
+};
+
+static int qcom_pwm_channel_read(struct qcom_pwm_channel *pwm,

drivers/pwm/pwm-qcom.c new file +29 -0

+	else
+		*val = (u8)tmp;
+	mutex_unlock(&pwm->chip->rw_lock);
+
+	return rc;
+}
+
+static int qcom_pwm_channel_write(struct qcom_pwm_channel *pwm, u16 addr, u8 val)
+{
+	int rc;
+
+	mutex_lock(&pwm->chip->rw_lock);
+	rc = regmap_write(pwm->chip->regmap, pwm->reg_base + addr, val);
+	if (rc < 0)
+		dev_err(pwm->chip->dev, "Write addr %#x with value %#x failed, rc=%d\n",
+				pwm->reg_base + addr, val, rc);
+	mutex_unlock(&pwm->chip->rw_lock);
+
+	return rc;
+}
+
+static int qcom_pwm_channel_masked_write(struct qcom_pwm_channel *pwm,
+				u16 addr, u8 mask, u8 val)
+{
+	int rc;
+
+	mutex_lock(&pwm->chip->rw_lock);
+	rc = regmap_update_bits(pwm->chip->regmap, pwm->reg_base + addr,
+							mask, val);

drivers/pwm/pwm-qcom.c new file +20 -0

+	if (chan_idx >= chip->num_channels) {
+		dev_err(chip->dev, "hw index %u out of range [0-%u]\n",
+				chan_idx, chip->num_channels - 1);
+		return NULL;
+	}
+
+	return &chip->pwms[chan_idx];
+}
+
+static int __find_index_in_array(int data, const int array[], int length)
+{
+	int i;
+
+	for (i = 0; i < length; i++) {
+		if (data == array[i])
+			return i;
+	}
+
+	return -ENOENT;
+}

drivers/pwm/pwm-qcom.c new file +21 -0

+}
+
+static int qcom_pwm_channel_config(struct qcom_pwm_channel *pwm)
+{
+	int rc;
+	u8 val, mask, shift;
+	int pwm_size_idx, pwm_clk_idx, prediv_idx, clk_exp_idx;
+
+	pwm_size_idx = __find_index_in_array(pwm->pwm_config.pwm_size,
+			pwm_size, ARRAY_SIZE(pwm_size));
+	pwm_clk_idx = __find_index_in_array(pwm->pwm_config.pwm_clk,
+			clk_freq_hz, ARRAY_SIZE(clk_freq_hz));
+	prediv_idx = __find_index_in_array(pwm->pwm_config.prediv,
+			clk_prediv, ARRAY_SIZE(clk_prediv));
+	clk_exp_idx = __find_index_in_array(pwm->pwm_config.clk_exp,
+			pwm_exponent, ARRAY_SIZE(pwm_exponent));
+
+	if (pwm_size_idx < 0 || pwm_clk_idx < 0
+			|| prediv_idx < 0 || clk_exp_idx < 0)
+		return -EINVAL;
+

drivers/pwm/pwm-qcom.c new file +43 -0

+	mask |= PWM_CLK_FREQ_SEL_MASK;
+	rc = qcom_pwm_channel_masked_write(pwm, REG_PWM_SIZE_CLK, mask, val);
+	if (rc < 0)
+		return rc;
+
+	val = prediv_idx << PWM_FREQ_PREDIV_SHIFT | clk_exp_idx;
+	mask = PWM_FREQ_PREDIV_MASK | PWM_FREQ_EXPONENT_MASK;
+	rc = qcom_pwm_channel_masked_write(pwm, REG_PWM_FREQ_PREDIV_CLK, mask, val);
+	if (rc < 0)
+		return rc;
+
+	val = pwm->pwm_config.pwm_value >> 8;
+	mask = PWM_VALUE_MSB_MASK;
+	rc = qcom_pwm_channel_masked_write(pwm, REG_PWM_VALUE_MSB, mask, val);
+	if (rc < 0)
+		return rc;
+
+	val = pwm->pwm_config.pwm_value & PWM_VALUE_LSB_MASK;
+	rc = qcom_pwm_channel_write(pwm, REG_PWM_VALUE_LSB, val);
+	if (rc < 0)
+		return rc;
+
+	val = PWM_VALUE_SYNC;
+	return qcom_pwm_channel_write(pwm, REG_PWM_SYNC, val);
+}
+
+static int qcom_pwm_channel_enable(struct qcom_pwm_channel *pwm, bool en)
+{
+	u8 mask, val;
+
+	mask = EN_MODULE_BIT;
+	val = en ? EN_MODULE_BIT : 0;
+	return qcom_pwm_channel_masked_write(pwm,
+			REG_ENABLE_CONTROL, mask, val);
+}
+
+static void __qcom_pwm_calc_pwm_period(u64 period_ns,
+			struct qcom_pwm_config *pwm_config)
+{
+	struct qcom_pwm_config configs[NUM_PWM_SIZE];
+	int i, j, m, n;
+	u64 tmp1, tmp2;
+	u64 clk_period_ns = 0, pwm_clk_period_ns;

drivers/pwm/pwm-qcom.c new file +37 -0

+	 */
+
+	for (n = 0; n < ARRAY_SIZE(pwm_size); n++) {
+		pwm_clk_period_ns = period_ns >> pwm_size[n];
+		for (i = ARRAY_SIZE(clk_freq_hz) - 1; i >= 0; i--) {
+			for (j = 0; j < ARRAY_SIZE(clk_prediv); j++) {
+				for (m = 0; m < ARRAY_SIZE(pwm_exponent); m++) {
+					tmp1 = 1 << pwm_exponent[m];
+					tmp1 *= clk_prediv[j];
+					tmp2 = NSEC_PER_SEC;
+					do_div(tmp2, clk_freq_hz[i]);
+
+					clk_period_ns = tmp1 * tmp2;
+
+					clk_delta_ns = abs(pwm_clk_period_ns
+						- clk_period_ns);
+					/*
+					 * Find the closest setting for
+					 * PWM frequency predivide value
+					 */
+					if (clk_delta_ns < min_clk_delta_ns) {
+						min_clk_delta_ns
+							= clk_delta_ns;
+						configs[n].pwm_clk
+							= clk_freq_hz[i];
+						configs[n].prediv
+							= clk_prediv[j];
+						configs[n].clk_exp
+							= pwm_exponent[m];
+						configs[n].pwm_size
+							= pwm_size[n];
+						configs[n].best_period_ns
+							= clk_period_ns;
+					}
+				}
+			}
+		}

drivers/pwm/pwm-qcom.c new file +40 -0

+			pwm_config->clk_exp -= pwm_size_step;
+		}
+	}
+
+	pr_debug("PWM setting for period_ns %llu: pwm_clk = %dHZ, prediv = %d, exponent = %d, pwm_size = %d\n",
+			period_ns, pwm_config->pwm_clk, pwm_config->prediv,
+			pwm_config->clk_exp, pwm_config->pwm_size);
+	pr_debug("Actual period: %lluns\n", pwm_config->best_period_ns);
+}
+
+static void __qcom_pwm_calc_pwm_duty(u64 period_ns, u64 duty_ns,
+			struct qcom_pwm_config *pwm_config)
+{
+	u16 pwm_value, max_pwm_value;
+	u64 tmp;
+
+	tmp = (u64)duty_ns << pwm_config->pwm_size;
+	pwm_value = (u16)div64_u64(tmp, period_ns);
+
+	max_pwm_value = (1 << pwm_config->pwm_size) - 1;
+	if (pwm_value > max_pwm_value)
+		pwm_value = max_pwm_value;
+
+	pwm_config->pwm_value = pwm_value;
+}
+
+static int qcom_pwm_config(struct pwm_chip *pwm_chip,
+		struct pwm_device *pwm_dev, u64 duty_ns, u64 period_ns)
+{
+	struct qcom_pwm_channel *pwm;
+	int rc;
+
+	pwm = pwm_dev_to_pwm_channel(pwm_chip, pwm_dev);
+	if (pwm == NULL) {
+		dev_err(pwm_chip->dev, "PWM channel not found\n");
+		return -ENODEV;
+	}
+
+	if (duty_ns > period_ns) {
+		dev_err(pwm_chip->dev, "Duty %llu ns is larger than period %llu ns\n",

drivers/pwm/pwm-qcom.c new file +46 -0

+	rc = qcom_pwm_channel_config(pwm);
+	if (rc < 0) {
+		dev_err(pwm_chip->dev, "Config PWM channel %u failed, rc=%d\n",
+						pwm->chan_idx, rc);
+		return rc;
+	}
+
+	pwm->current_period_ns = period_ns;
+	pwm->current_duty_ns = duty_ns;
+	return 0;
+}
+
+static int qcom_pwm_enable(struct pwm_chip *pwm_chip,
+				struct pwm_device *pwm_dev)
+{
+	struct qcom_pwm_channel *pwm;
+	int rc = 0;
+
+	pwm = pwm_dev_to_pwm_channel(pwm_chip, pwm_dev);
+	if (pwm == NULL) {
+		dev_err(pwm_chip->dev, "PWM channel not found\n");
+		return -ENODEV;
+	}
+
+	rc = qcom_pwm_channel_write(pwm, REG_PWM_SYNC, PWM_VALUE_SYNC);
+	if (rc < 0)
+		return rc;
+
+	rc = qcom_pwm_channel_set_glitch_removal(pwm, true);
+	if (rc < 0)
+		return rc;
+
+	return qcom_pwm_channel_enable(pwm, true);
+}
+
+static int qcom_pwm_disable(struct pwm_chip *pwm_chip,
+				struct pwm_device *pwm_dev)
+{
+	struct qcom_pwm_channel *pwm;
+	int rc;
+
+	pwm = pwm_dev_to_pwm_channel(pwm_chip, pwm_dev);
+	if (pwm == NULL) {
+		dev_err(pwm_chip->dev, "PWM channel not found\n");
+		return -ENODEV;
+	}

drivers/pwm/pwm-qcom.c new file +30 -0

+};
+
+static int qcom_pwm_parse_dt(struct qcom_pwm_chip *chip)
+{
+	int rc = 0, i;
+	const __be32 *addr;
+	u32 base;
+
+	addr = of_get_address(chip->dev->of_node, 0, NULL, NULL);
+	if (!addr) {
+		dev_err(chip->dev, "Get PWM device address failed, rc=%d\n",
+				rc);
+		return -EINVAL;
+	}
+
+	base = be32_to_cpu(*addr);
+	rc = of_property_read_u32(chip->dev->of_node, "qcom,num-channels",
+			&chip->num_channels);
+	if (rc < 0) {
+		dev_err(chip->dev, "Failed to get qcom,num-channels, rc=%d\n",
+				rc);
+		return rc;
+	}
+
+	if (chip->num_channels == 0) {
+		dev_err(chip->dev, "No PWM channel specified\n");
+		return rc;
+	}
+
+	chip->pwms = devm_kcalloc(chip->dev, chip->num_channels,

drivers/pwm/pwm-qcom.c new file +8 -0

+		if (rc < 0) {
+			dev_err(chip->dev, "Read PWM channel %d subtype failed, rc=%d\n",
+					i, rc);
+			return rc;
+		}
+	}
+
+	return 0;

drivers/pwm/pwm-qcom.c new file +10 -0

+	}
+
+	return 0;
+err_out:
+	mutex_destroy(&chip->rw_lock);
+	dev_set_drvdata(chip->dev, NULL);
+	return rc;
+}
+
+static int qcom_pwm_remove(struct platform_device *pdev)

drivers/pwm/pwm-qcom.c new file +5 -0

+	return rc;
+}
+
+static const struct of_device_id qcom_pwm_of_match[] = {
+	{ .compatible = "qcom,pwm"},

drivers/pwm/pwm-qcom.c new file +5 -0

+};
+module_platform_driver(qcom_pwm_driver);
+
+MODULE_DESCRIPTION("QCOM PWM driver");
+MODULE_LICENSE("GPL v2");

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Hello,

Quoted 49 lines · Fenglin Wu

PWM modules present in QCOM PMICs are controlled through SPMI bus. Normally, it would have several PWM modules together with adjacent register space and each PWM module can be controlled independently.

Signed-off-by Fenglin Wu <[email protected]>

---
 drivers/pwm/Kconfig    |   9 +
 drivers/pwm/Makefile   |   1 +
 drivers/pwm/pwm-qcom.c | 585 
+++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 595 insertions(+)
 create mode 100644 drivers/pwm/pwm-qcom.c

drivers/pwm/Kconfig +9 -0

index 8ae68d6..324ab5d 100644
@@ -423,6 +423,15 @@ config PWM_PXA
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-pxa.
 
+config PWM_QCOM
+	tristate "Qcom PMIC PWM support"
+	depends on MFD_SPMI_PMIC && OF
+	help
+	  Generic PWM framework driver for PWM module in QCOM PMIC chips
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-qcom.
+
 config PWM_RCAR
 	tristate "Renesas R-Car PWM support"
 	depends on ARCH_RENESAS || COMPILE_TEST

drivers/pwm/Makefile +1 -0

index d43b1e1..78316e9 100644
@@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
+obj-$(CONFIG_PWM_QCOM)		+= pwm-qcom.o
 obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o

drivers/pwm/pwm-qcom.c new file +0 -1

new file mode 100644
index 0000000..48bd823
--- /dev/null

drivers/pwm/pwm-qcom.c line 3

Quoted 5 lines

+++ b/drivers/pwm/pwm-qcom.c

drivers/pwm/pwm-qcom.c new file +3 -0

@@ -0,0 +1,585 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, The Linux Foundation. All rights reserved.

If there is a publicly available reference manual describing this hardware, please add a link here.

Also please add a section (like in the pwm-sifive driver for example) describing the relevant properties. Interesting are answers to the questions:

- Does the hardware complete the currently running period on reconfiguration? (If that's configurable, please enable this behaviour)

- Does the hardware complete the currently running period when the PWM is disabled?

- Does the output pin pull to the inactive level when the PWM is disabled?

- Does the hardware support both polarities?

Please stick to the format used in pwm-sifive to be easily greppable.

Quoted 39 lines

drivers/pwm/pwm-qcom.c new file +38 -0

+ */
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+/* PWM module registers */
+#define REG_PERPH_SUBTYPE		0x05
+#define REG_PWM_SIZE_CLK		0x41
+#define REG_PWM_FREQ_PREDIV_CLK		0x42
+#define REG_PWM_TYPE_CONFIG		0x43
+#define REG_PWM_VALUE_LSB		0x44
+#define REG_PWM_VALUE_MSB		0x45
+#define REG_ENABLE_CONTROL		0x46
+#define REG_PWM_SYNC			0x47
+
+/* REG_PERPH_SUBTYPE */
+#define SUBTYPE_PWM			0x0b
+#define SUBTYPE_PWM_LITE		0x11
+
+/* REG_PWM_SIZE_CLK */
+#define PWM_LITE_PWM_SIZE_MASK		BIT(4)
+#define PWM_LITE_PWM_SIZE_SHIFT		4
+#define PWM_SIZE_MASK			BIT(2)
+#define PWM_SIZE_SHIFT			2
+#define PWM_CLK_FREQ_SEL_MASK		GENMASK(1, 0)

drivers/pwm/pwm-qcom.c line 45

Quoted 4 lines

drivers/pwm/pwm-qcom.c new file +4 -0

+
+/* REG_PWM_FREQ_PREDIV_CLK */
+#define PWM_FREQ_PREDIV_MASK		GENMASK(6, 5)
+#define PWM_FREQ_PREDIV_SHIFT		5

It should be possible to not need the _SHIFT define as it can be deferred from the mask value. In turn you can also drop the _MASK suffix shortening the define names.

Quoted 5 lines

drivers/pwm/pwm-qcom.c new file +4 -0

+#define PWM_FREQ_EXPONENT_MASK		GENMASK(2, 0)
+
+/* REG_PWM_TYPE_CONFIG */
+#define PWM_EN_GLITCH_REMOVAL_MASK	BIT(5)

drivers/pwm/pwm-qcom.c line 59

Quoted 10 lines

drivers/pwm/pwm-qcom.c new file +10 -0

+
+/* REG_PWM_VALUE */
+#define PWM_VALUE_LSB_MASK		GENMASK(7, 0)
+#define PWM_VALUE_MSB_MASK		BIT(0)
+
+/* REG_ENABLE_CONTROL */
+#define EN_MODULE_BIT			BIT(7)
+
+/* REG_PWM_SYNC */
+#define PWM_VALUE_SYNC			BIT(0)

I would like to see the register definition to use a common prefix (like QCOM_PWM_) and that the names of bit fields include the register name. So something like:

	#define QCOM_PWM_PWM_SIZE_CLK		0x41
	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)

even if the names are quite long, its usage is less error prone. Maybe it makes sense to drop the duplicated PWM (but only if all or no register contains PWM in its name according to the reference manual). Also maybe QCOM_PWM_PWMSIZECLK_FREQSEL might be a good choice. I let you judge about the details.

Quoted 13 lines

drivers/pwm/pwm-qcom.c new file +12 -0

+
+/* constant definitions */
+#define REG_SIZE_PER_CHANN		0x100
+#define NUM_PWM_SIZE			2
+#define NUM_PWM_CLK			3
+#define NUM_CLK_PREDIV			4
+#define NUM_PWM_EXP			8
+
+static const int pwm_size[NUM_PWM_SIZE] = {6, 9};
+static const int clk_freq_hz[NUM_PWM_CLK] = {1024, 32768, 19200000};
+static const int clk_prediv[NUM_CLK_PREDIV] = {1, 3, 5, 6};
+static const int pwm_exponent[NUM_PWM_EXP] = {0, 1, 2, 3, 4, 5, 6, 

Quoted 1 line

7};

Please also use a driver specific prefix for variables and function names.

Quoted 30 lines

drivers/pwm/pwm-qcom.c new file +29 -0

+struct qcom_pwm_config {
+	u32	pwm_size;
+	u32	pwm_clk;
+	u32	prediv;
+	u32	clk_exp;
+	u16	pwm_value;
+	u64	best_period_ns;
+};
+
+struct qcom_pwm_channel {
+	struct qcom_pwm_chip	*chip;
+	struct qcom_pwm_config	pwm_config;
+	u32			chan_idx;
+	u32			reg_base;
+	u8			subtype;
+	u64			current_period_ns;
+	u64			current_duty_ns;
+};
+
+struct qcom_pwm_chip {
+	struct pwm_chip		pwm_chip;
+	struct regmap		*regmap;
+	struct device		*dev;
+	struct qcom_pwm_channel	*pwms;
+	struct mutex		rw_lock;
+	u32			num_channels;
+};
+
+static int qcom_pwm_channel_read(struct qcom_pwm_channel *pwm,

drivers/pwm/pwm-qcom.c line 111

Quoted 10 lines

drivers/pwm/pwm-qcom.c new file +10 -0

+					u16 addr, u8 *val)
+{
+	int rc;
+	unsigned int tmp;
+
+	mutex_lock(&pwm->chip->rw_lock);
+	rc = regmap_read(pwm->chip->regmap, pwm->reg_base + addr, &tmp);
+	if (rc < 0)
+		dev_err(pwm->chip->dev, "Read addr %#x failed, rc=%d\n",
+				pwm->reg_base + addr, rc);

Do you know that you can emit the symbolic error code using %pe? This yields better readable error messages. That would be something like:

		dev_err(pwm->chip->dev, "Read addr %#x failed, rc=%pe\n",
				pwm->reg_base + addr, ERR_PTR(rc));

Quoted 39 lines

drivers/pwm/pwm-qcom.c new file +8 -0

+	else
+		*val = (u8)tmp;
+	mutex_unlock(&pwm->chip->rw_lock);
+
+	return rc;
+}
+
+static int qcom_pwm_channel_write(struct qcom_pwm_channel *pwm, u16 

addr, u8 val)

drivers/pwm/pwm-qcom.c new file +7 -0

+{
+	int rc;
+
+	mutex_lock(&pwm->chip->rw_lock);
+	rc = regmap_write(pwm->chip->regmap, pwm->reg_base + addr, val);
+	if (rc < 0)
+		dev_err(pwm->chip->dev, "Write addr %#x with value %#x failed, 

rc=%d\n",

drivers/pwm/pwm-qcom.c new file +7 -0

+				pwm->reg_base + addr, val, rc);
+	mutex_unlock(&pwm->chip->rw_lock);
+
+	return rc;
+}
+
+static int qcom_pwm_channel_masked_write(struct qcom_pwm_channel 

*pwm,

drivers/pwm/pwm-qcom.c new file +9 -0

+				u16 addr, u8 mask, u8 val)
+{
+	int rc;
+
+	mutex_lock(&pwm->chip->rw_lock);
+	rc = regmap_update_bits(pwm->chip->regmap, pwm->reg_base + addr,
+							mask, val);
+	if (rc < 0)
+		dev_err(pwm->chip->dev, "Update addr %#x to val %#x with mask %#x 

failed, rc=%d\n",

drivers/pwm/pwm-qcom.c new file +4 -0

+				pwm->reg_base + addr, val, mask, rc);
+	mutex_unlock(&pwm->chip->rw_lock);
+
+	return rc;

drivers/pwm/pwm-qcom.c

Quoted 5 lines

drivers/pwm/pwm-qcom.c new file +3 -0

+}
+
+static struct qcom_pwm_channel *pwm_dev_to_pwm_channel(struct 

pwm_chip *pwm_chip,

drivers/pwm/pwm-qcom.c new file +1 -0

+				struct pwm_device *pwm_dev)

Please use "chip" as name for the first parameter which is the usual choice in the PWM core and also the drivers.

drivers/pwm/pwm-qcom.c line 154

Quoted 4 lines

drivers/pwm/pwm-qcom.c new file +4 -0

+{
+
+	struct qcom_pwm_chip *chip = container_of(pwm_chip,
+			struct qcom_pwm_chip, pwm_chip);

You will have to pick a different name here accordingly. I'd suggest ddata or qc.

drivers/pwm/pwm-qcom.c line 155

Quoted 1 line

drivers/pwm/pwm-qcom.c new file +1 -0

+	u32 chan_idx = pwm_dev->hwpwm;

hwpwm is an unsigned int, I suggest making chan_idx an unsigned int, too.

Quoted 25 lines

drivers/pwm/pwm-qcom.c new file +10 -0

+	if (chan_idx >= chip->num_channels) {
+		dev_err(chip->dev, "hw index %u out of range [0-%u]\n",
+				chan_idx, chip->num_channels - 1);
+		return NULL;
+	}
+
+	return &chip->pwms[chan_idx];
+}
+
+static int __find_index_in_array(int data, const int array[], int 

length)

drivers/pwm/pwm-qcom.c new file +14 -0

+{
+	int i;
+
+	for (i = 0; i < length; i++) {
+		if (data == array[i])
+			return i;
+	}
+
+	return -ENOENT;
+}
+
+static int qcom_pwm_channel_set_glitch_removal(
+		struct qcom_pwm_channel *pwm, bool en)
+{

drivers/pwm/pwm-qcom.c line 186

Quoted 6 lines

drivers/pwm/pwm-qcom.c new file +6 -0

+	u8 mask, val;
+
+	val = en ? PWM_EN_GLITCH_REMOVAL_MASK : 0;
+	mask = PWM_EN_GLITCH_REMOVAL_MASK;
+	return qcom_pwm_channel_masked_write(pwm,
+			REG_PWM_TYPE_CONFIG, mask, val);

What is the effect of this setting?

Quoted 21 lines

drivers/pwm/pwm-qcom.c new file +21 -0

+}
+
+static int qcom_pwm_channel_config(struct qcom_pwm_channel *pwm)
+{
+	int rc;
+	u8 val, mask, shift;
+	int pwm_size_idx, pwm_clk_idx, prediv_idx, clk_exp_idx;
+
+	pwm_size_idx = __find_index_in_array(pwm->pwm_config.pwm_size,
+			pwm_size, ARRAY_SIZE(pwm_size));
+	pwm_clk_idx = __find_index_in_array(pwm->pwm_config.pwm_clk,
+			clk_freq_hz, ARRAY_SIZE(clk_freq_hz));
+	prediv_idx = __find_index_in_array(pwm->pwm_config.prediv,
+			clk_prediv, ARRAY_SIZE(clk_prediv));
+	clk_exp_idx = __find_index_in_array(pwm->pwm_config.clk_exp,
+			pwm_exponent, ARRAY_SIZE(pwm_exponent));
+
+	if (pwm_size_idx < 0 || pwm_clk_idx < 0
+			|| prediv_idx < 0 || clk_exp_idx < 0)
+		return -EINVAL;
+

drivers/pwm/pwm-qcom.c line 217

Quoted 10 lines

drivers/pwm/pwm-qcom.c new file +10 -0

+	/* pwm_clk_idx is 1 bit lower than the register value */
+	pwm_clk_idx += 1;
+	shift = PWM_SIZE_SHIFT;
+	mask = PWM_SIZE_MASK;
+	if (pwm->subtype == SUBTYPE_PWM_LITE) {
+		shift = PWM_LITE_PWM_SIZE_SHIFT;
+		mask = PWM_LITE_PWM_SIZE_MASK;
+	}
+
+	val = pwm_size_idx << shift | pwm_clk_idx;

If you use

	val = FIELD_PREP(pwm_size_idx, mask) | pwm_clk_idx;

you don't need the shift variable.

Quoted 51 lines

drivers/pwm/pwm-qcom.c new file +2 -0

+	mask |= PWM_CLK_FREQ_SEL_MASK;
+	rc = qcom_pwm_channel_masked_write(pwm, REG_PWM_SIZE_CLK, mask, 

val);

drivers/pwm/pwm-qcom.c new file +6 -0

+	if (rc < 0)
+		return rc;
+
+	val = prediv_idx << PWM_FREQ_PREDIV_SHIFT | clk_exp_idx;
+	mask = PWM_FREQ_PREDIV_MASK | PWM_FREQ_EXPONENT_MASK;
+	rc = qcom_pwm_channel_masked_write(pwm, REG_PWM_FREQ_PREDIV_CLK, 

mask, val);

drivers/pwm/pwm-qcom.c new file +6 -0

+	if (rc < 0)
+		return rc;
+
+	val = pwm->pwm_config.pwm_value >> 8;
+	mask = PWM_VALUE_MSB_MASK;
+	rc = qcom_pwm_channel_masked_write(pwm, REG_PWM_VALUE_MSB, mask, 

val);

drivers/pwm/pwm-qcom.c new file +13 -0

+	if (rc < 0)
+		return rc;
+
+	val = pwm->pwm_config.pwm_value & PWM_VALUE_LSB_MASK;
+	rc = qcom_pwm_channel_write(pwm, REG_PWM_VALUE_LSB, val);
+	if (rc < 0)
+		return rc;
+
+	val = PWM_VALUE_SYNC;
+	return qcom_pwm_channel_write(pwm, REG_PWM_SYNC, val);
+}
+
+static int qcom_pwm_channel_enable(struct qcom_pwm_channel *pwm, bool 

en)

drivers/pwm/pwm-qcom.c new file +20 -0

+{
+	u8 mask, val;
+
+	mask = EN_MODULE_BIT;
+	val = en ? EN_MODULE_BIT : 0;
+	return qcom_pwm_channel_masked_write(pwm,
+			REG_ENABLE_CONTROL, mask, val);
+}
+
+static void __qcom_pwm_calc_pwm_period(u64 period_ns,
+			struct qcom_pwm_config *pwm_config)
+{
+	struct qcom_pwm_config configs[NUM_PWM_SIZE];
+	int i, j, m, n;
+	u64 tmp1, tmp2;
+	u64 clk_period_ns = 0, pwm_clk_period_ns;
+	u64 clk_delta_ns = U64_MAX, min_clk_delta_ns = U64_MAX;
+	u64 pwm_period_delta = U64_MAX, min_pwm_period_delta = U64_MAX;
+	int pwm_size_step;
+

drivers/pwm/pwm-qcom.c line 270

Quoted 6 lines

drivers/pwm/pwm-qcom.c new file +6 -0

+	/*
+	 *              (2^pwm_size) * (2^pwm_exp) * prediv * NSEC_PER_SEC
+	 * pwm_period = ---------------------------------------------------
+	 *                               clk_freq_hz
+	 *
+	 * Searching the closest settings for the requested PWM period.

Please don't pick the nearest setting, but the next smallest one.

Quoted 43 lines

drivers/pwm/pwm-qcom.c new file +43 -0

+	 */
+
+	for (n = 0; n < ARRAY_SIZE(pwm_size); n++) {
+		pwm_clk_period_ns = period_ns >> pwm_size[n];
+		for (i = ARRAY_SIZE(clk_freq_hz) - 1; i >= 0; i--) {
+			for (j = 0; j < ARRAY_SIZE(clk_prediv); j++) {
+				for (m = 0; m < ARRAY_SIZE(pwm_exponent); m++) {
+					tmp1 = 1 << pwm_exponent[m];
+					tmp1 *= clk_prediv[j];
+					tmp2 = NSEC_PER_SEC;
+					do_div(tmp2, clk_freq_hz[i]);
+
+					clk_period_ns = tmp1 * tmp2;
+
+					clk_delta_ns = abs(pwm_clk_period_ns
+						- clk_period_ns);
+					/*
+					 * Find the closest setting for
+					 * PWM frequency predivide value
+					 */
+					if (clk_delta_ns < min_clk_delta_ns) {
+						min_clk_delta_ns
+							= clk_delta_ns;
+						configs[n].pwm_clk
+							= clk_freq_hz[i];
+						configs[n].prediv
+							= clk_prediv[j];
+						configs[n].clk_exp
+							= pwm_exponent[m];
+						configs[n].pwm_size
+							= pwm_size[n];
+						configs[n].best_period_ns
+							= clk_period_ns;
+					}
+				}
+			}
+		}
+
+		configs[n].best_period_ns *= 1 << pwm_size[n];
+		/* Find the closest setting for PWM period */
+		pwm_period_delta = min_clk_delta_ns << pwm_size[n];
+		if (pwm_period_delta < min_pwm_period_delta) {
+			min_pwm_period_delta = pwm_period_delta;

drivers/pwm/pwm-qcom.c line 317

Quoted 4 lines

drivers/pwm/pwm-qcom.c new file +4 -0

+			memcpy(pwm_config, &configs[n],
+					sizeof(struct qcom_pwm_config));
+		}
+	}

Huh, this is complicated. It would be great if this could be simplified. If you provide a reference manual or at least a .get_state function, I can try to advise.

drivers/pwm/pwm-qcom.c line 325

Quoted 7 lines

drivers/pwm/pwm-qcom.c new file +7 -0

+	/* Larger PWM size can achieve better resolution for PWM duty */
+	for (n = ARRAY_SIZE(pwm_size) - 1; n > 0; n--) {
+		if (pwm_config->pwm_size >= pwm_size[n])
+			break;
+		pwm_size_step = pwm_size[n] - pwm_config->pwm_size;
+		if (pwm_config->clk_exp >= pwm_size_step) {
+			pwm_config->pwm_size = pwm_size[n];

If you store n instead of pwm_size[n] finding n in qcom_pwm_channel_config becomes easier and you can drop __find_index_in_array.

Quoted 48 lines

drivers/pwm/pwm-qcom.c new file +5 -0

+			pwm_config->clk_exp -= pwm_size_step;
+		}
+	}
+
+	pr_debug("PWM setting for period_ns %llu: pwm_clk = %dHZ, prediv = 

%d, exponent = %d, pwm_size = %d\n",

drivers/pwm/pwm-qcom.c new file +35 -0

+			period_ns, pwm_config->pwm_clk, pwm_config->prediv,
+			pwm_config->clk_exp, pwm_config->pwm_size);
+	pr_debug("Actual period: %lluns\n", pwm_config->best_period_ns);
+}
+
+static void __qcom_pwm_calc_pwm_duty(u64 period_ns, u64 duty_ns,
+			struct qcom_pwm_config *pwm_config)
+{
+	u16 pwm_value, max_pwm_value;
+	u64 tmp;
+
+	tmp = (u64)duty_ns << pwm_config->pwm_size;
+	pwm_value = (u16)div64_u64(tmp, period_ns);
+
+	max_pwm_value = (1 << pwm_config->pwm_size) - 1;
+	if (pwm_value > max_pwm_value)
+		pwm_value = max_pwm_value;
+
+	pwm_config->pwm_value = pwm_value;
+}
+
+static int qcom_pwm_config(struct pwm_chip *pwm_chip,
+		struct pwm_device *pwm_dev, u64 duty_ns, u64 period_ns)
+{
+	struct qcom_pwm_channel *pwm;
+	int rc;
+
+	pwm = pwm_dev_to_pwm_channel(pwm_chip, pwm_dev);
+	if (pwm == NULL) {
+		dev_err(pwm_chip->dev, "PWM channel not found\n");
+		return -ENODEV;
+	}
+
+	if (duty_ns > period_ns) {
+		dev_err(pwm_chip->dev, "Duty %llu ns is larger than period %llu 

ns\n",

drivers/pwm/pwm-qcom.c new file +5 -0

+						duty_ns, period_ns);
+		return -EINVAL;
+	}
+
+	if (period_ns != pwm->current_period_ns)

drivers/pwm/pwm-qcom.c line 375

Quoted 5 lines

drivers/pwm/pwm-qcom.c new file +5 -0

+		__qcom_pwm_calc_pwm_period(period_ns, &pwm->pwm_config);
+
+	if (period_ns != pwm->current_period_ns ||
+			duty_ns != pwm->current_duty_ns)
+		__qcom_pwm_calc_pwm_duty(period_ns, duty_ns, &pwm->pwm_config);

You're losing precision here by using the requested period length (instead of the time that is implemented in hardware).

Quoted 51 lines

drivers/pwm/pwm-qcom.c new file +50 -0

+	rc = qcom_pwm_channel_config(pwm);
+	if (rc < 0) {
+		dev_err(pwm_chip->dev, "Config PWM channel %u failed, rc=%d\n",
+						pwm->chan_idx, rc);
+		return rc;
+	}
+
+	pwm->current_period_ns = period_ns;
+	pwm->current_duty_ns = duty_ns;
+	return 0;
+}
+
+static int qcom_pwm_enable(struct pwm_chip *pwm_chip,
+				struct pwm_device *pwm_dev)
+{
+	struct qcom_pwm_channel *pwm;
+	int rc = 0;
+
+	pwm = pwm_dev_to_pwm_channel(pwm_chip, pwm_dev);
+	if (pwm == NULL) {
+		dev_err(pwm_chip->dev, "PWM channel not found\n");
+		return -ENODEV;
+	}
+
+	rc = qcom_pwm_channel_write(pwm, REG_PWM_SYNC, PWM_VALUE_SYNC);
+	if (rc < 0)
+		return rc;
+
+	rc = qcom_pwm_channel_set_glitch_removal(pwm, true);
+	if (rc < 0)
+		return rc;
+
+	return qcom_pwm_channel_enable(pwm, true);
+}
+
+static int qcom_pwm_disable(struct pwm_chip *pwm_chip,
+				struct pwm_device *pwm_dev)
+{
+	struct qcom_pwm_channel *pwm;
+	int rc;
+
+	pwm = pwm_dev_to_pwm_channel(pwm_chip, pwm_dev);
+	if (pwm == NULL) {
+		dev_err(pwm_chip->dev, "PWM channel not found\n");
+		return -ENODEV;
+	}
+
+	rc = qcom_pwm_channel_enable(pwm, false);
+	if (rc < 0)
+		return rc;

drivers/pwm/pwm-qcom.c line 432

Quoted 6 lines

drivers/pwm/pwm-qcom.c new file +6 -0

+
+	return qcom_pwm_channel_set_glitch_removal(pwm, false);
+}
+
+static int qcom_pwm_apply(struct pwm_chip *pwm_chip,
+		struct pwm_device *pwm_dev, const struct pwm_state *state)

s/pwm_chip/chip/; s/pwm_dev/pwm/;

drivers/pwm/pwm-qcom.c line 106

Quoted 4 lines

drivers/pwm/pwm-qcom.c new file +3 -0

+{
+	int rc;
+

You have to check for polarity here.

drivers/pwm/pwm-qcom.c line 444

Quoted 9 lines

drivers/pwm/pwm-qcom.c new file +9 -0

+	if (state->period != pwm_dev->state.period ||
+			state->duty_cycle != pwm_dev->state.duty_cycle) {
+		rc = qcom_pwm_config(pwm_chip, pwm_dev,
+				state->duty_cycle, state->period);
+		if (rc < 0)
+			return rc;
+
+		pwm_dev->state.duty_cycle = state->duty_cycle;
+		pwm_dev->state.period = state->period;

The core takes care for this, please drop these two assignments.

drivers/pwm/pwm-qcom.c line 451

Quoted 7 lines

drivers/pwm/pwm-qcom.c new file +7 -0

+	}
+
+	if (state->enabled != pwm_dev->state.enabled) {
+		if (state->enabled)
+			rc = qcom_pwm_enable(pwm_chip, pwm_dev);
+		else
+			rc = qcom_pwm_disable(pwm_chip, pwm_dev);

Please handle state->enabled = false before configuring duty/period to prevent a glitch.

Quoted 7 lines

drivers/pwm/pwm-qcom.c new file +7 -0

+		if (rc < 0)
+			return rc;
+
+		pwm_dev->state.enabled = state->enabled;
+	}
+
+	return 0;

drivers/pwm/pwm-qcom.c line 463

Quoted 5 lines

drivers/pwm/pwm-qcom.c new file +5 -0

+}
+
+static const struct pwm_ops qcom_pwm_ops = {
+	.apply = qcom_pwm_apply,
+	.owner = THIS_MODULE,

Please implement .get_status and test your driver with CONFIG_PWM_ENABLED.

Quoted 34 lines

drivers/pwm/pwm-qcom.c new file +34 -0

+};
+
+static int qcom_pwm_parse_dt(struct qcom_pwm_chip *chip)
+{
+	int rc = 0, i;
+	const __be32 *addr;
+	u32 base;
+
+	addr = of_get_address(chip->dev->of_node, 0, NULL, NULL);
+	if (!addr) {
+		dev_err(chip->dev, "Get PWM device address failed, rc=%d\n",
+				rc);
+		return -EINVAL;
+	}
+
+	base = be32_to_cpu(*addr);
+	rc = of_property_read_u32(chip->dev->of_node, "qcom,num-channels",
+			&chip->num_channels);
+	if (rc < 0) {
+		dev_err(chip->dev, "Failed to get qcom,num-channels, rc=%d\n",
+				rc);
+		return rc;
+	}
+
+	if (chip->num_channels == 0) {
+		dev_err(chip->dev, "No PWM channel specified\n");
+		return rc;
+	}
+
+	chip->pwms = devm_kcalloc(chip->dev, chip->num_channels,
+			sizeof(*chip->pwms), GFP_KERNEL);
+	if (!chip->pwms)
+		return -ENOMEM;
+

drivers/pwm/pwm-qcom.c line 503

Quoted 6 lines

drivers/pwm/pwm-qcom.c new file +6 -0

+	for (i = 0; i < chip->num_channels; i++) {
+		chip->pwms[i].chip = chip;
+		chip->pwms[i].chan_idx = i;
+		chip->pwms[i].reg_base = base + i * REG_SIZE_PER_CHANN;
+		rc = qcom_pwm_channel_read(&chip->pwms[i], REG_PERPH_SUBTYPE,
+				&chip->pwms[i].subtype);

Can a single device have channels with different sub-types?

Quoted 15 lines

drivers/pwm/pwm-qcom.c new file +14 -0

+		if (rc < 0) {
+			dev_err(chip->dev, "Read PWM channel %d subtype failed, rc=%d\n",
+					i, rc);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static int qcom_pwm_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct qcom_pwm_chip *chip;

drivers/pwm/pwm-qcom.c line 521

Quoted 4 lines

drivers/pwm/pwm-qcom.c new file +4 -0

+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;

If you parse qcom,num-channels already before allocating driver data you can allocate driver and per channel data in a single chunk, making some operations simpler and maybe even save some memory.

drivers/pwm/pwm-qcom.c line 527

Quoted 5 lines

drivers/pwm/pwm-qcom.c new file +5 -0

+	chip->dev = &pdev->dev;
+	chip->regmap = dev_get_regmap(chip->dev->parent, NULL);
+	if (!chip->regmap) {
+		dev_err(chip->dev, "Getting regmap failed\n");
+		return -EINVAL;

ENODEV instead of EINVAL is more usual I think. Please use

	return dev_err_probe(chip->dev, -ENODEV, "....");

and also use dev_err_probe in the other error paths.

drivers/pwm/pwm-qcom.c line 537

Quoted 11 lines

drivers/pwm/pwm-qcom.c new file +10 -0

+	}
+
+	rc = qcom_pwm_parse_dt(chip);
+	if (rc < 0)
+		return rc;
+
+	mutex_init(&chip->rw_lock);
+	dev_set_drvdata(chip->dev, chip);
+	chip->pwm_chip.dev = chip->dev;
+	chip->pwm_chip.base = -1;

This shouldn't be done any more since commit f9a8ee8c8bcd ("pwm: Always allocate PWM chip base ID dynamically") (which currently sits in next).

drivers/pwm/pwm-qcom.c line 544

Quoted 8 lines

drivers/pwm/pwm-qcom.c new file +7 -0

+	chip->pwm_chip.npwm = chip->num_channels;
+	chip->pwm_chip.ops = &qcom_pwm_ops;
+
+	rc = pwmchip_add(&chip->pwm_chip);
+	if (rc < 0) {
+		dev_err(chip->dev, "Add pwmchip failed, rc=%d\n", rc);
+		goto err_out;

The cleanups done after this goto are not necessary. Just use

	return dev_err_probe(chip->dev, rc, "Add pwmchip failed\n");

Quoted 10 lines

drivers/pwm/pwm-qcom.c new file +10 -0

+	}
+
+	return 0;
+err_out:
+	mutex_destroy(&chip->rw_lock);
+	dev_set_drvdata(chip->dev, NULL);
+	return rc;
+}
+
+static int qcom_pwm_remove(struct platform_device *pdev)

drivers/pwm/pwm-qcom.c line 564

Quoted 10 lines

drivers/pwm/pwm-qcom.c new file +10 -0

+{
+	struct qcom_pwm_chip *chip = dev_get_drvdata(&pdev->dev);
+	int rc = 0;
+
+	rc = pwmchip_remove(&chip->pwm_chip);
+	if (rc < 0)
+		dev_err(chip->dev, "Remove pwmchip failed, rc=%d\n", rc);
+
+	mutex_destroy(&chip->rw_lock);
+	dev_set_drvdata(chip->dev, NULL);

The driver core cares for

	dev_set_drvdata(chip->dev, NULL);

Also destroying the mutes isn't usually done.

Quoted 5 lines

drivers/pwm/pwm-qcom.c new file +5 -0

+	return rc;
+}
+
+static const struct of_device_id qcom_pwm_of_match[] = {
+	{ .compatible = "qcom,pwm"},

drivers/pwm/pwm-qcom.c line 580

Quoted 10 lines

drivers/pwm/pwm-qcom.c new file +10 -0

+	{ },
+};
+
+static struct platform_driver qcom_pwm_driver = {
+	.driver		= {
+		.name		= "qcom,pwm",
+		.of_match_table	= qcom_pwm_of_match,
+	},
+	.probe		= qcom_pwm_probe,
+	.remove		= qcom_pwm_remove,

I'm not a big fan of aligning the =. It looks ugly with that big white space before = (and using a smaller space is bad if you later have to initialize a member with a longer name).

But no hard veto from my side here, if you want to stick to that layout.

Quoted 10 lines

drivers/pwm/pwm-qcom.c new file +5 -1

+};
+module_platform_driver(qcom_pwm_driver);
+
+MODULE_DESCRIPTION("QCOM PWM driver");
+MODULE_LICENSE("GPL v2");
--

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Quoted 3 lines · Uwe Kleine-König

On Tue, Apr 27, 2021 at 06:22:10PM +0800, Fenglin Wu wrote:
- Does the hardware complete the currently running period when the PWM is disabled?

No, the output stops immediately as soon as the PWM channel is disabled

Quoted 2 lines

- Does the output pin pull to the inactive level when the PWM is disabled?

Actually, the QCOM PMIC PWM module doesn't have physical pin out. Its output is normally connected to other hardware module in the same PMIC as internal signals, such as: control signal for LED module for scaling LED brightness, input signal for vibrator module for vibration strength control. It's output can also be routed through PMIC GPIO or other pin using internal DTEST lines, and that depends on HW connection, and it will also need addition configuration in the GPIO module or the DTEST and that's outside of the PWM module scope.

For the output signal itself, it's always inactive when the PWM module is disabled.

Quoted 11 lines

drivers/pwm/pwm-qcom.c line 186

Quoted 9 lines

drivers/pwm/pwm-qcom.c new file +9 -0

+static int qcom_pwm_channel_set_glitch_removal(
+		struct qcom_pwm_channel *pwm, bool en)
+{
+	u8 mask, val;
+
+	val = en ? PWM_EN_GLITCH_REMOVAL_MASK : 0;
+	mask = PWM_EN_GLITCH_REMOVAL_MASK;
+	return qcom_pwm_channel_masked_write(pwm,
+			REG_PWM_TYPE_CONFIG, mask, val);

What is the effect of this setting?

As I explained at the beginning, enable this setting would garantee the PWM module complete current period before swtiching to the new settings.

Quoted 11 lines

Quoted 11 lines

drivers/pwm/pwm-qcom.c new file +11 -0

+static void __qcom_pwm_calc_pwm_period(u64 period_ns,
+			struct qcom_pwm_config *pwm_config)
+{
+	struct qcom_pwm_config configs[NUM_PWM_SIZE];
+	int i, j, m, n;
+	u64 tmp1, tmp2;
+	u64 clk_period_ns = 0, pwm_clk_period_ns;
+	u64 clk_delta_ns = U64_MAX, min_clk_delta_ns = U64_MAX;
+	u64 pwm_period_delta = U64_MAX, min_pwm_period_delta = U64_MAX;
+	int pwm_size_step;
+

Quoted 8 lines

drivers/pwm/pwm-qcom.c line 270

Quoted 6 lines

drivers/pwm/pwm-qcom.c new file +6 -0

+	/*
+	 *              (2^pwm_size) * (2^pwm_exp) * prediv * NSEC_PER_SEC
+	 * pwm_period = ---------------------------------------------------
+	 *                               clk_freq_hz
+	 *
+	 * Searching the closest settings for the requested PWM period.

Please don't pick the nearest setting, but the next smallest one.

I am not quite sure here. You can see from the equation above, there are 4 settings can impact PWM period calculation and each setting has an array of values. We can't easily sort out the sequence of settings to achieve an ascending
or descending PWM periods and choose the closest one or the next smallest one,
instead, the logic below is to walk through all of the settings and find the closest one.
I am also confused about not choosing the nearest settings but the next smallest one, let's say if we are trying to achieve 1ms PWM period, and there are three settings can get 0.90ms, 0.99ms, 1.05ms respectively should we choose 0.99ms which is the closest one, or 1.05ms which is the next
smallest one?

Quoted 43 lines

Quoted 43 lines

drivers/pwm/pwm-qcom.c new file +43 -0

+	 */
+
+	for (n = 0; n < ARRAY_SIZE(pwm_size); n++) {
+		pwm_clk_period_ns = period_ns >> pwm_size[n];
+		for (i = ARRAY_SIZE(clk_freq_hz) - 1; i >= 0; i--) {
+			for (j = 0; j < ARRAY_SIZE(clk_prediv); j++) {
+				for (m = 0; m < ARRAY_SIZE(pwm_exponent); m++) {
+					tmp1 = 1 << pwm_exponent[m];
+					tmp1 *= clk_prediv[j];
+					tmp2 = NSEC_PER_SEC;
+					do_div(tmp2, clk_freq_hz[i]);
+
+					clk_period_ns = tmp1 * tmp2;
+
+					clk_delta_ns = abs(pwm_clk_period_ns
+						- clk_period_ns);
+					/*
+					 * Find the closest setting for
+					 * PWM frequency predivide value
+					 */
+					if (clk_delta_ns < min_clk_delta_ns) {
+						min_clk_delta_ns
+							= clk_delta_ns;
+						configs[n].pwm_clk
+							= clk_freq_hz[i];
+						configs[n].prediv
+							= clk_prediv[j];
+						configs[n].clk_exp
+							= pwm_exponent[m];
+						configs[n].pwm_size
+							= pwm_size[n];
+						configs[n].best_period_ns
+							= clk_period_ns;
+					}
+				}
+			}
+		}
+
+		configs[n].best_period_ns *= 1 << pwm_size[n];
+		/* Find the closest setting for PWM period */
+		pwm_period_delta = min_clk_delta_ns << pwm_size[n];
+		if (pwm_period_delta < min_pwm_period_delta) {
+			min_pwm_period_delta = pwm_period_delta;

Quoted 8 lines

drivers/pwm/pwm-qcom.c line 317

Quoted 4 lines

drivers/pwm/pwm-qcom.c new file +4 -0

+			memcpy(pwm_config, &configs[n],
+					sizeof(struct qcom_pwm_config));
+		}
+	}

Huh, this is complicated. It would be great if this could be simplified. If you provide a reference manual or at least a .get_state function, I can try to advise.

Hmm, I am not sure how to describe the HW implementation here, but as I explained, there are four parameters impacting the PWM period calculation with different way, so the code logic here is trying to walk through all of the the settings and find the one which can achieve the closest PWM period.

Quoted 8 lines

drivers/pwm/pwm-qcom.c line 503

Quoted 6 lines

drivers/pwm/pwm-qcom.c new file +6 -0

+	for (i = 0; i < chip->num_channels; i++) {
+		chip->pwms[i].chip = chip;
+		chip->pwms[i].chan_idx = i;
+		chip->pwms[i].reg_base = base + i * REG_SIZE_PER_CHANN;
+		rc = qcom_pwm_channel_read(&chip->pwms[i], REG_PERPH_SUBTYPE,
+				&chip->pwms[i].subtype);

Can a single device have channels with different sub-types?

Hmm, it has the possibility. Normally, in one PMIC, all PWM modules should have the same sub-type of PWM modules. But since each PWM module is accessed independantly, so we will need to check the sub-type here for each PWM module.

Quoted 14 lines

Quoted 14 lines

drivers/pwm/pwm-qcom.c new file +14 -0

+		if (rc < 0) {
+			dev_err(chip->dev, "Read PWM channel %d subtype failed, rc=%d\n",
+					i, rc);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static int qcom_pwm_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct qcom_pwm_chip *chip;

Quoted 8 lines

drivers/pwm/pwm-qcom.c line 521

Quoted 4 lines

drivers/pwm/pwm-qcom.c new file +4 -0

+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;

If you parse qcom,num-channels already before allocating driver data you can allocate driver and per channel data in a single chunk, making some operations simpler and maybe even save some memory.

In a single chunk do you mean by calling devm_zalloc() once? Can you let me know how to do that? The per channel data is anothe pointer which is different from the driver data, how can we make sure two different pointers can be allocated in the same chunk of memory?

Hello,

On Tue, Apr 27, 2021 at 06:22:10PM +0800, Fenglin Wu wrote:

Quoted 1 line

drivers/pwm/pwm-qcom.c +0 -0

Quoted 4 lines

drivers/pwm/pwm-qcom.c +4 -0

+#define PWM_FREQ_EXPONENT_MASK		GENMASK(2, 0)
+
+/* REG_PWM_TYPE_CONFIG */
+#define PWM_EN_GLITCH_REMOVAL_MASK	BIT(5)

drivers/pwm/pwm-qcom.c line 59

Quoted 10 lines

drivers/pwm/pwm-qcom.c +10 -0

+
+/* REG_PWM_VALUE */
+#define PWM_VALUE_LSB_MASK		GENMASK(7, 0)
+#define PWM_VALUE_MSB_MASK		BIT(0)
+
+/* REG_ENABLE_CONTROL */
+#define EN_MODULE_BIT			BIT(7)
+
+/* REG_PWM_SYNC */
+#define PWM_VALUE_SYNC			BIT(0)

I would like to see the register definition to use a common prefix (like QCOM_PWM_) and that the names of bit fields include the register name. So something like:

	#define QCOM_PWM_PWM_SIZE_CLK		0x41
	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)

even if the names are quite long, its usage is less error prone. Maybe it makes sense to drop the duplicated PWM (but only if all or no register contains PWM in its name according to the reference manual). Also maybe QCOM_PWM_PWMSIZECLK_FREQSEL might be a good choice. I let you judge about the details.

Quoted 6 lines

drivers/pwm/pwm-qcom.c +6 -0

+/* constant definitions */
+#define REG_SIZE_PER_CHANN		0x100
+#define NUM_PWM_SIZE			2
+#define NUM_PWM_CLK			3
+#define NUM_CLK_PREDIV			4
+#define NUM_PWM_EXP			8

drivers/pwm/pwm-qcom.c line 71

Quoted 5 lines

drivers/pwm/pwm-qcom.c +5 -0

+
+static const int pwm_size[NUM_PWM_SIZE] = {6, 9};
+static const int clk_freq_hz[NUM_PWM_CLK] = {1024, 32768, 19200000};
+static const int clk_prediv[NUM_CLK_PREDIV] = {1, 3, 5, 6};
+static const int pwm_exponent[NUM_PWM_EXP] = {0, 1, 2, 3, 4, 5, 6, 7};

Please also use a driver specific prefix for variables and function names.

Quoted 4 lines · Uwe Kleine-König

I would like to see the register definition to use a common prefix (like QCOM_PWM_) and that the names of bit fields include the register name. So something like:

Quoted 8 lines

	#define QCOM_PWM_PWM_SIZE_CLK		0x41
	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)

even if the names are quite long, its usage is less error prone. Maybe it makes sense to drop the duplicated PWM (but only if all or no register contains PWM in its name according to the reference manual). Also maybe QCOM_PWM_PWMSIZECLK_FREQSEL might be a good choice. I let you judge about the details.

Please stop requesting this. A common prefix is good for namespacing symbols, but these defines are used only within this file, so there's no need to namespace them.

Forcing everyone to use a specific prefix is just going to add a bunch of characters but doesn't actually add any value.

Hello,

Quoted 14 lines · Thierry Reding

Quoted 4 lines · Uwe Kleine-König

I would like to see the register definition to use a common prefix (like QCOM_PWM_) and that the names of bit fields include the register name. So something like:

Quoted 8 lines

	#define QCOM_PWM_PWM_SIZE_CLK		0x41
	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)

even if the names are quite long, its usage is less error prone. Maybe it makes sense to drop the duplicated PWM (but only if all or no register contains PWM in its name according to the reference manual). Also maybe QCOM_PWM_PWMSIZECLK_FREQSEL might be a good choice. I let you judge about the details.

Quoted 3 lines

Please stop requesting this. A common prefix is good for namespacing symbols, but these defines are used only within this file, so there's no need to namespace them.

I do consider it important. The goal of my review comments is to improve the drivers according to what I consider sensible even if that might not fit your metrics.

Consistent name(space)ing is sensible because the names of static functions are used in backtraces. It is sensible because tools like ctags, etags and cscope work better when names are unique. It is sensible because it's harder than necessary to spot the error in

	writel(PWM_EN_GLITCH_REMOVAL_MASK, base + REG_ENABLE_CONTROL);

. It is sensible because the rule "Use namespacing for all symbols" is easier than "Use namespacing for symbols that might conflict with (present or future) names in the core or that might appear in user visible messages like backtraces or KASAN reports". It's sensible because then it's obvious when reading a code line that the symbol is driver specific. It is useful to have a common prefix for driver functions because that makes it easier to select them for tracing.

Quoted 2 lines

Forcing everyone to use a specific prefix is just going to add a bunch of characters but doesn't actually add any value.

That's your opinion and I disagree. I do see a value and the "burden" of these additional characters is quite worth its costs. In my bubble most people also see this value. This includes the coworkers I talked to, several other maintainers also insist on common prefixes[1] and it matches what my software engineering professor taught me during my studies. I also agree that longer names are more annoying than short ones, but that doesn't outweigh the advantages in my eyes and a good editor helps here.

Quoted 43 lines · Uwe Kleine-König

Hello,

Quoted 14 lines · Thierry Reding

Quoted 4 lines · Uwe Kleine-König

I would like to see the register definition to use a common prefix (like QCOM_PWM_) and that the names of bit fields include the register name. So something like:

Quoted 8 lines

	#define QCOM_PWM_PWM_SIZE_CLK		0x41
	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)

even if the names are quite long, its usage is less error prone. Maybe it makes sense to drop the duplicated PWM (but only if all or no register contains PWM in its name according to the reference manual). Also maybe QCOM_PWM_PWMSIZECLK_FREQSEL might be a good choice. I let you judge about the details.

Quoted 3 lines

Please stop requesting this. A common prefix is good for namespacing symbols, but these defines are used only within this file, so there's no need to namespace them.

I do consider it important. The goal of my review comments is to improve the drivers according to what I consider sensible even if that might not fit your metrics.

Consistent name(space)ing is sensible because the names of static functions are used in backtraces. It is sensible because tools like ctags, etags and cscope work better when names are unique. It is sensible because it's harder than necessary to spot the error in

	writel(PWM_EN_GLITCH_REMOVAL_MASK, base + REG_ENABLE_CONTROL);

. It is sensible because the rule "Use namespacing for all symbols" is easier than "Use namespacing for symbols that might conflict with (present or future) names in the core or that might appear in user visible messages like backtraces or KASAN reports". It's sensible because then it's obvious when reading a code line that the symbol is driver specific. It is useful to have a common prefix for driver functions because that makes it easier to select them for tracing.

Quoted 2 lines

Forcing everyone to use a specific prefix is just going to add a bunch of characters but doesn't actually add any value.

Quoted 8 lines

That's your opinion and I disagree. I do see a value and the "burden" of these additional characters is quite worth its costs. In my bubble most people also see this value. This includes the coworkers I talked to, several other maintainers also insist on common prefixes[1] and it matches what my software engineering professor taught me during my studies. I also agree that longer names are more annoying than short ones, but that doesn't outweigh the advantages in my eyes and a good editor helps here.

FWIW, I'm +1 for proper namespacing for the purposes of; tracing, logging and future proofing, even if it does add a few more chars. Less of a problem now the 80-char rule is waning.

Quoted 54 lines · Lee Jones

Quoted 51 lines · Uwe Kleine-König

Hello,

Quoted 14 lines · Thierry Reding

Quoted 4 lines · Uwe Kleine-König

I would like to see the register definition to use a common prefix (like QCOM_PWM_) and that the names of bit fields include the register name. So something like:

Quoted 8 lines

	#define QCOM_PWM_PWM_SIZE_CLK		0x41
	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)

even if the names are quite long, its usage is less error prone. Maybe it makes sense to drop the duplicated PWM (but only if all or no register contains PWM in its name according to the reference manual). Also maybe QCOM_PWM_PWMSIZECLK_FREQSEL might be a good choice. I let you judge about the details.

Quoted 3 lines

Please stop requesting this. A common prefix is good for namespacing symbols, but these defines are used only within this file, so there's no need to namespace them.

I do consider it important. The goal of my review comments is to improve the drivers according to what I consider sensible even if that might not fit your metrics.

Consistent name(space)ing is sensible because the names of static functions are used in backtraces. It is sensible because tools like ctags, etags and cscope work better when names are unique. It is sensible because it's harder than necessary to spot the error in

	writel(PWM_EN_GLITCH_REMOVAL_MASK, base + REG_ENABLE_CONTROL);

. It is sensible because the rule "Use namespacing for all symbols" is easier than "Use namespacing for symbols that might conflict with (present or future) names in the core or that might appear in user visible messages like backtraces or KASAN reports". It's sensible because then it's obvious when reading a code line that the symbol is driver specific. It is useful to have a common prefix for driver functions because that makes it easier to select them for tracing.

Quoted 2 lines

Forcing everyone to use a specific prefix is just going to add a bunch of characters but doesn't actually add any value.

That's your opinion and I disagree. I do see a value and the "burden" of these additional characters is quite worth its costs. In my bubble most people also see this value. This includes the coworkers I talked to, several other maintainers also insist on common prefixes[1] and it matches what my software engineering professor taught me during my studies. I also agree that longer names are more annoying than short ones, but that doesn't outweigh the advantages in my eyes and a good editor helps here.

Quoted 3 lines

FWIW, I'm +1 for proper namespacing for the purposes of; tracing, logging and future proofing, even if it does add a few more chars. Less of a problem now the 80-char rule is waning.

I've mentioned this in other threads before, but in retrospect I suppose I could've been more specific. For function names, even static ones, yes, I agree a common prefix is better.

But there's absolutely no reason to enforce it for register definitions or local variables because the symbols will never show up anywhere.

Hello,

Quoted 14 lines · Thierry Reding

Quoted 4 lines · Uwe Kleine-König

I would like to see the register definition to use a common prefix (like QCOM_PWM_) and that the names of bit fields include the register name. So something like:

Quoted 8 lines

	#define QCOM_PWM_PWM_SIZE_CLK		0x41
	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)

even if the names are quite long, its usage is less error prone. Maybe it makes sense to drop the duplicated PWM (but only if all or no register contains PWM in its name according to the reference manual). Also maybe QCOM_PWM_PWMSIZECLK_FREQSEL might be a good choice. I let you judge about the details.

Quoted 3 lines

Please stop requesting this. A common prefix is good for namespacing symbols, but these defines are used only within this file, so there's no need to namespace them.

I do consider it important. The goal of my review comments is to improve the drivers according to what I consider sensible even if that might not fit your metrics.

Consistent name(space)ing is sensible because the names of static functions are used in backtraces.

It is sensible because tools like
ctags, etags and cscope work better when names are unique.

It is
sensible because it's harder than necessary to spot the error in

	writel(PWM_EN_GLITCH_REMOVAL_MASK, base + REG_ENABLE_CONTROL);

. It is sensible because the rule "Use namespacing for all symbols" is easier than "Use namespacing for symbols that might conflict with (present or future) names in the core or that might appear in user visible messages like backtraces or KASAN reports".

It's sensible
because then it's obvious when reading a code line that the symbol is driver specific. It is useful to have a common prefix for driver functions because that makes it easier to select them for tracing.

Quoted 2 lines

Forcing everyone to use a specific prefix is just going to add a bunch of characters but doesn't actually add any value.

That's your opinion and I disagree. I do see a value and the "burden" of these additional characters is quite worth its costs. In my bubble most people also see this value. This includes the coworkers I talked to, several other maintainers also insist on common prefixes[1] and it matches what my software engineering professor taught me during my studies. I also agree that longer names are more annoying than short ones, but that doesn't outweigh the advantages in my eyes and a good editor helps here.

Add PWM driver to support PWM modules inside QCOM PMIC chips which are accessed through SPMI bus. Normally, there would be multiple PWM modules with adjacent address spaces present in one PMIC chip, and each PWM module has 0x100 size of address space. With this driver, a pwm_chip with multiple pwm_device individuals is created, and each pwm_device individual is corresponding to one PWM module.

Fenglin Wu (2):
dt-bindings: pwm: add bindings for PWM modules inside QCOM PMICs
pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

 .../devicetree/bindings/pwm/pwm-qcom.yaml          |  51 ++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-qcom.c                             | 585 +++++++++++++++++++++
 4 files changed, 646 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-qcom.yaml
 create mode 100644 drivers/pwm/pwm-qcom.c

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Quoted 6 lines

Add PWM driver to support PWM modules inside QCOM PMIC chips which are accessed through SPMI bus. Normally, there would be multiple PWM modules with adjacent address spaces present in one PMIC chip, and each PWM module has 0x100 size of address space. With this driver, a pwm_chip with multiple pwm_device individuals is created, and each pwm_device individual is corresponding to one PWM module.

Exposing this as individual pwm_chips will prevent us from enabling the LED related use cases (patterns and multicolor) that most versions of the hardware support.

I proposed [1] a while ago and think this is a better approach. I'll take some time to respin this and send out the next version.

[1] https://lore.kernel.org/linux-arm-msm/[email protected]/

Quoted 7 lines

Quoted 6 lines

Add PWM driver to support PWM modules inside QCOM PMIC chips which are accessed through SPMI bus. Normally, there would be multiple PWM modules with adjacent address spaces present in one PMIC chip, and each PWM module has 0x100 size of address space. With this driver, a pwm_chip with multiple pwm_device individuals is created, and each pwm_device individual is corresponding to one PWM module.

Quoted 8 lines

Exposing this as individual pwm_chips will prevent us from enabling the LED related use cases (patterns and multicolor) that most versions of the hardware support.

I proposed [1] a while ago and think this is a better approach. I'll take some time to respin this and send out the next version.

[1] https://lore.kernel.org/linux-arm-msm/[email protected]/

Hi Bjorn,
Yes, we came across this patch series but this driver (leds-qcom-lpg) is a combo one which provides support only for RGB LEDs (or TRI_LED module) along with PWM/LPG channels allocated for it. Say, if we've additional PWM channels on the same PMIC (that provides user-interface support) or another PMIC (non user-interface) that has multiple PWM channels that are not used for LED notifications, it would be good to have a separate PWM driver to support such channels IMHO. There are couple of use cases we've come across recently.

1. Using a PWM channel for controlling external LCD backlight controller
2. Using a PWM channel for controlling a haptics actuator

Quoted 21 lines · Subbaraman Narayanamurthy

Quoted 6 lines

Quoted 6 lines

Add PWM driver to support PWM modules inside QCOM PMIC chips which are accessed through SPMI bus. Normally, there would be multiple PWM modules with adjacent address spaces present in one PMIC chip, and each PWM module has 0x100 size of address space. With this driver, a pwm_chip with multiple pwm_device individuals is created, and each pwm_device individual is corresponding to one PWM module.

Quoted 6 lines

Exposing this as individual pwm_chips will prevent us from enabling the
LED related use cases (patterns and multicolor) that most versions of
the hardware support.
I proposed [1] a while ago and think this is a better approach. I'll
take some time to respin this and send out the next version.
[1] https://lore.kernel.org/linux-arm-msm/[email protected]/

Hi Bjorn,
Yes, we came across this patch series but this driver (leds-qcom-lpg) is a combo one which provides support only for RGB LEDs (or TRI_LED module) along with PWM/LPG channels allocated for it. Say, if we've additional PWM channels on the same PMIC (that provides user-interface support) or another PMIC (non user-interface) that has multiple PWM channels that are not used for LED notifications, it would be good to have a separate PWM driver to support such channels IMHO. There are couple of use cases we've come across recently.

Quoted 3 lines

1. Using a PWM channel for controlling external LCD backlight controller
2. Using a PWM channel for controlling a haptics actuator

The LPG driver, as it's currently written, support using each channel as a LED, part of a multicolor LED or as a pwm_chip. It's been tested on pm8916 (which doesn't have triled or the lut), pm*8994, pmi8996 and pm8150* in various combinations.

In particular the PWM-only modes that you describe here is how the driver has been used on db410c, for driving the "backlight GPIO" in the low-speed connector.

Regards,
Bjorn