[PATCH 1/2] dt-bindings: pwm: add bindings for PWM modules inside QCOM PMICs

[PATCH 1/2] dt-bindings: pwm: add bindings for PWM modules inside QCOM PMICs

[Fenglin Wu]

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

Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
---
 .../devicetree/bindings/pwm/pwm-qcom.yaml          | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-qcom.yaml

diff --git a/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml b/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml
new file mode 100644
index 0000000..e8d8ed6
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-qcom.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. PMIC PWM bindings
+
+maintainers:
+  - Fenglin Wu <fenglinw@codeaurora.org>
+
+description:
+  PWM modules inside Qualcomm Technologies, Inc. PMICs can be accessed through
+  SPMI bus and normally one PMIC would have multiple PWM modules with adjacent
+  SPMI address space.
+
+Properties:
+  compatible:
+    const: qcom,pwm
+
+  reg:
+    description:
+      The SPMI address base of the PWM module, if there are multiple PWM
+      modules present with adjacent SPMI address space, only need to specify
+      the address base of the 1st PWM module.
+
+  "#pwm-cells":
+     # See pwm.yaml in this directory for a description of the cells format.
+     const: 2
+
+  qcom,num-channels:
+    description:
+      The number of the PWM channels (modules) with the adjacent SPMI address
+      space following the address base in "reg" property.
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - qcom,num-channels
+
+additionalProperties: false
+
+examples:
+  - |
+    pm8350c_pwm: pwms@e800 {
+        compatible = "qcom,pwm";
+        reg = <0xe800>;
+        #pwm-cells = <2>;
+        qcom,num-channels = <4>;
+    };
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] dt-bindings: pwm: add bindings for PWM modules inside QCOM PMICs

[Rob Herring]

On Tue, 27 Apr 2021 18:22:09 +0800, Fenglin Wu wrote:
> Add bindings for QCOM PMIC PWM modules which are accessed through SPMI
> bus.
> 
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> ---
>  .../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.

Re: [PATCH 1/2] dt-bindings: pwm: add bindings for PWM modules inside QCOM PMICs

[fenglinw]

On 2021-04-27 20:57, Rob Herring wrote:
> On Tue, 27 Apr 2021 18:22:09 +0800, Fenglin Wu wrote:
>> Add bindings for QCOM PMIC PWM modules which are accessed through SPMI
>> bus.
>> 
>> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
>> ---
>>  .../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.

Thanks for the information. It's my 1st time to write binding using yaml 
format, I hadn't noticed there was a checker can be used for sanity 
test. I will update and run the dt_binding_check at my side before 
uploading new patchset.

Re: [PATCH 1/2] dt-bindings: pwm: add bindings for PWM modules inside QCOM PMICs

[Thierry Reding]

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

On Tue, Apr 27, 2021 at 06:22:09PM +0800, Fenglin Wu wrote:
> Add bindings for QCOM PMIC PWM modules which are accessed through SPMI
> bus.
> 
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> ---
>  .../devicetree/bindings/pwm/pwm-qcom.yaml          | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-qcom.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml b/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml
> new file mode 100644
> index 0000000..e8d8ed6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-qcom.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/pwm-qcom.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. PMIC PWM bindings
> +
> +maintainers:
> +  - Fenglin Wu <fenglinw@codeaurora.org>
> +
> +description:
> +  PWM modules inside Qualcomm Technologies, Inc. PMICs can be accessed through
> +  SPMI bus and normally one PMIC would have multiple PWM modules with adjacent
> +  SPMI address space.
> +
> +Properties:
> +  compatible:
> +    const: qcom,pwm

This seems a bit vague. What if Qualcomm ever designs a different PWM?
How are you going to tell them apart? Typically this would include some
sort of ID for the SoC family, or the first SoC that this was introduced
on. That way you can more easily distinguish between different designs
later on.

> +
> +  reg:
> +    description:
> +      The SPMI address base of the PWM module, if there are multiple PWM
> +      modules present with adjacent SPMI address space, only need to specify
> +      the address base of the 1st PWM module.

That seems like an odd way to define these. It looks like this is a bus
with #address-cells = <1> and #size-cells = <0>. Such busses are usually
assumed to have a single address per device (see for example I2C). How
does the SPMI addressing work? Is there a specification somewhere?

Actually, Documentation/devicetree/bindings/spmi/spmi.yaml says that
SPMI child devices should have two address cells, so this seesm to be at
odds with that specification.

Thierry

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

[PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[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 <fenglinw@codeaurora.org>
---
 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

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8ae68d6..324ab5d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index d43b1e1..78316e9 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -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
diff --git a/drivers/pwm/pwm-qcom.c b/drivers/pwm/pwm-qcom.c
new file mode 100644
index 0000000..48bd823
--- /dev/null
+++ b/drivers/pwm/pwm-qcom.c
@@ -0,0 +1,585 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, The Linux Foundation. All rights reserved.
+ */
+
+#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)
+
+/* REG_PWM_FREQ_PREDIV_CLK */
+#define PWM_FREQ_PREDIV_MASK		GENMASK(6, 5)
+#define PWM_FREQ_PREDIV_SHIFT		5
+#define PWM_FREQ_EXPONENT_MASK		GENMASK(2, 0)
+
+/* REG_PWM_TYPE_CONFIG */
+#define PWM_EN_GLITCH_REMOVAL_MASK	BIT(5)
+
+/* 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)
+
+/* 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, 7};
+
+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,
+					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);
+	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);
+	if (rc < 0)
+		dev_err(pwm->chip->dev, "Update addr %#x to val %#x with mask %#x failed, rc=%d\n",
+				pwm->reg_base + addr, val, mask, rc);
+	mutex_unlock(&pwm->chip->rw_lock);
+
+	return rc;
+}
+
+static struct qcom_pwm_channel *pwm_dev_to_pwm_channel(struct pwm_chip *pwm_chip,
+				struct pwm_device *pwm_dev)
+{
+
+	struct qcom_pwm_chip *chip = container_of(pwm_chip,
+			struct qcom_pwm_chip, pwm_chip);
+	u32 chan_idx = pwm_dev->hwpwm;
+
+	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;
+}
+
+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);
+}
+
+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;
+
+	/* 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;
+	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;
+	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;
+
+	/*
+	 *              (2^pwm_size) * (2^pwm_exp) * prediv * NSEC_PER_SEC
+	 * pwm_period = ---------------------------------------------------
+	 *                               clk_freq_hz
+	 *
+	 * Searching the closest settings for the requested PWM period.
+	 */
+
+	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;
+			memcpy(pwm_config, &configs[n],
+					sizeof(struct qcom_pwm_config));
+		}
+	}
+
+	/* 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];
+			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",
+						duty_ns, period_ns);
+		return -EINVAL;
+	}
+
+	if (period_ns != pwm->current_period_ns)
+		__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);
+
+	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;
+
+	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)
+{
+	int rc;
+
+	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;
+	}
+
+	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);
+		if (rc < 0)
+			return rc;
+
+		pwm_dev->state.enabled = state->enabled;
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops qcom_pwm_ops = {
+	.apply = qcom_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+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;
+
+	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);
+		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;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	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;
+	}
+
+	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;
+	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;
+	}
+
+	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)
+{
+	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);
+
+	return rc;
+}
+
+static const struct of_device_id qcom_pwm_of_match[] = {
+	{ .compatible = "qcom,pwm"},
+	{ },
+};
+
+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,
+};
+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.

Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Uwe Kleine-König]

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

Hello,

On Tue, Apr 27, 2021 at 06:22:10PM +0800, Fenglin Wu wrote:
> 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 <fenglinw@codeaurora.org>
> ---
>  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
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8ae68d6..324ab5d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -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
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index d43b1e1..78316e9 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -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
> diff --git a/drivers/pwm/pwm-qcom.c b/drivers/pwm/pwm-qcom.c
> new file mode 100644
> index 0000000..48bd823
> --- /dev/null
> +++ b/drivers/pwm/pwm-qcom.c
> @@ -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.

> + */
> +
> +#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)
> +
> +/* 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.

> +#define PWM_FREQ_EXPONENT_MASK		GENMASK(2, 0)
> +
> +/* REG_PWM_TYPE_CONFIG */
> +#define PWM_EN_GLITCH_REMOVAL_MASK	BIT(5)
> +
> +/* 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.

> +
> +/* 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, 7};

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

> +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,
> +					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));
	
> +	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);
> +	if (rc < 0)
> +		dev_err(pwm->chip->dev, "Update addr %#x to val %#x with mask %#x failed, rc=%d\n",
> +				pwm->reg_base + addr, val, mask, rc);
> +	mutex_unlock(&pwm->chip->rw_lock);
> +
> +	return rc;
> +}
> +
> +static struct qcom_pwm_channel *pwm_dev_to_pwm_channel(struct pwm_chip *pwm_chip,
> +				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.

> +{
> +
> +	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.

> +	u32 chan_idx = pwm_dev->hwpwm;

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

> +	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;
> +}
> +
> +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?

> +}
> +
> +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;
> +
> +	/* 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.

> +	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;
> +	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;
> +
> +	/*
> +	 *              (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.

> +	 */
> +
> +	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;
> +			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.

> +	/* 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.

> +			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",
> +						duty_ns, period_ns);
> +		return -EINVAL;
> +	}
> +
> +	if (period_ns != pwm->current_period_ns)
> +		__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).

> +	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;
> +
> +	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/;

> +{
> +	int rc;
> +

You have to check for polarity here.

> +	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.

> +	}
> +
> +	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.

> +		if (rc < 0)
> +			return rc;
> +
> +		pwm_dev->state.enabled = state->enabled;
> +	}
> +
> +	return 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.

> +};
> +
> +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;
> +
> +	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?

> +		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;
> +
> +	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.

> +	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.

> +	}
> +
> +	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).

> +	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");


> +	}
> +
> +	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)
> +{
> +	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.

> +	return rc;
> +}
> +
> +static const struct of_device_id qcom_pwm_of_match[] = {
> +	{ .compatible = "qcom,pwm"},
> +	{ },
> +};
> +
> +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.

> +};
> +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.
> 
> 

-- 
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 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[fenglinw]

On 2021-04-28 01:07, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Apr 27, 2021 at 06:22:10PM +0800, Fenglin Wu wrote:
>> 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 <fenglinw@codeaurora.org>
>> ---
>>  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
>> 
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 8ae68d6..324ab5d 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -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
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index d43b1e1..78316e9 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -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
>> diff --git a/drivers/pwm/pwm-qcom.c b/drivers/pwm/pwm-qcom.c
>> new file mode 100644
>> index 0000000..48bd823
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-qcom.c
>> @@ -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.
> 
Thanks for reviewing the patch. Unfortunately, I just checked 
internally,
the datasheet was not shared outside so I couldn't give 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:
Sure, I will add a section to describe the HW properties.
> 
>  - Does the hardware complete the currently running period on
>    reconfiguration? (If that's configurable, please enable this
>    behaviour)

Yes, this is configurable, PWM_EN_GLITCH_REMOVAL bit is for this purpose
and it's enabled by default.
> 
>  - 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

>  - 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.
> 
>  - Does the hardware support both polarities?
> 
No, it's only support normal polarity.

> Please stick to the format used in pwm-sifive to be easily greppable.
sure, I will.
> 
>> + */
>> +
>> +#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)
>> +
>> +/* 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.
Thanks for the suggestion. I will update them all to use
FIELD_PREP(mask, val) for masking and shifting values.
> 
>> +#define PWM_FREQ_EXPONENT_MASK		GENMASK(2, 0)
>> +
>> +/* REG_PWM_TYPE_CONFIG */
>> +#define PWM_EN_GLITCH_REMOVAL_MASK	BIT(5)
>> +
>> +/* 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.

sure, I will think about a better way to define the register and bit 
fields
and make sure QCOM_PWM_ prefix is present.
> 
>> +
>> +/* 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, 
>> 7};
> 
> Please also use a driver specific prefix for variables and function
> names.

ACKed!
> 
>> +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,
>> +					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));
> 
Thanks for the suggestion, will check it this way.

>> +	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);
>> +	if (rc < 0)
>> +		dev_err(pwm->chip->dev, "Update addr %#x to val %#x with mask %#x 
>> failed, rc=%d\n",
>> +				pwm->reg_base + addr, val, mask, rc);
>> +	mutex_unlock(&pwm->chip->rw_lock);
>> +
>> +	return rc;
>> +}
>> +
>> +static struct qcom_pwm_channel *pwm_dev_to_pwm_channel(struct 
>> pwm_chip *pwm_chip,
>> +				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.
> 
ACKed!

>> +{
>> +
>> +	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.
> 
ACKed!

>> +	u32 chan_idx = pwm_dev->hwpwm;
> 
> hwpwm is an unsigned int, I suggest making chan_idx an unsigned int,
> too.
> 
ACKed!

>> +	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;
>> +}
>> +
>> +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.

>> +}
>> +
>> +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;
>> +
>> +	/* 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.
> 
ACKed!

>> +	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;
>> +	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;
>> +
>> +	/*
>> +	 *              (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?

>> +	 */
>> +
>> +	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;
>> +			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.

>> +	/* 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.

I agree with you, but I thought it might be more meaningful to store the
physical values in "struct qcom_pwm_config" instead of the array index .

> 
>> +			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",
>> +						duty_ns, period_ns);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (period_ns != pwm->current_period_ns)
>> +		__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).

ACKed, will use (pwm_config->best_period_ns) to calculate the duty.

> 
>> +	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;
>> +
>> +	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/;

ACKed!
> 
>> +{
>> +	int rc;
>> +
> 
> You have to check for polarity here.
> 
The PWM module can only support normal polarity, should I just ignore it
or return an error code if PWM_POLARITY_INVERSED is requested?

>> +	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.
> 
ACKed!

>> +	}
>> +
>> +	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.
> 
ACKed!

>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		pwm_dev->state.enabled = state->enabled;
>> +	}
>> +
>> +	return 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.
> 
ACKed, I will add this in next patchset.

>> +};
>> +
>> +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;
>> +
>> +	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.

> 
>> +		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;
>> +
>> +	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?

>> +	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.

Acked!

> 
>> +	}
>> +
>> +	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).

ACKed!
> 
>> +	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");
> 
> 
ACKed!

>> +	}
>> +
>> +	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)
>> +{
>> +	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.
> 
ACKed!

>> +	return rc;
>> +}
>> +
>> +static const struct of_device_id qcom_pwm_of_match[] = {
>> +	{ .compatible = "qcom,pwm"},
>> +	{ },
>> +};
>> +
>> +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.
> 
>> +};
>> +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.
>> 
>> 

Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Uwe Kleine-König]

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

Hello,

On Wed, Apr 28, 2021 at 08:42:58PM +0800, fenglinw@codeaurora.org wrote:
> On 2021-04-28 01:07, Uwe Kleine-König wrote:
> > 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

Is this only because you clear PWM_EN_GLITCH_REMOVAL on disable?

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

Inactive as in "outputs a 0" and not as in "driver is disabled", right?
(Not sure this is a well defined term given that the output isn't
normally externally visible.)

> > > +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.

Then there is no reason to unset this bit when the PWM is disabled and
the setting can better be done once in .probe()?

> > > +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;
> > > +
> > > +	/*
> > > +	 *              (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?

My wording was bad, you should pick the biggest period not bigger than
the requested period. So in your example you should pick 0.99ms.

And if your options are 0.90ms and 1.01 ms and the request is for 1 ms,
pick 0.90ms. I'm working on a series that allows a consumer to make an
informed choice. One precondition for that is that .apply picks a
setting according to the above algorithm though.

> > > +	 */
> > > +
> > > +	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;
> > > +			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.

OK, so we have:

              (2^pwm_size) * (2^pwm_exp) * prediv * NSEC_PER_SEC
 pwm_period = ---------------------------------------------------
                               clk_freq_hz

with pwm_size being either 6 or 9, pwm_exp is an integer in the range
[0..7], prediv is one of 1, 3, 5, or 6 and clk_freq_hz is one of 1024,
32768 or 19200000, right? And picking 9 for pwm_size has the advantage
that the duty-cycle setting has a finer resolution, right?

(BTW, I wonder about the choice for prediv, one of the set {1, 3, 5, 7}
would make more sense in my eyes and additionally it might even be a tad
cheaper to implement in hardware.)

This is indeed a strange formula that hardly allows to implement the
picking of parameters in a clever way.

I wonder if the hardware can emit a 100% duty cycle.

> > > +	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.

But if all channels are known to have the same subtype, you don't need
to test them all individually and a single member in qcom_pwm_chip
indicating the type would be enough.

> > > +		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;
> > > +
> > > +	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?

You can do the following:

	struct qcom_pwm_channel {
		...
	};

	struct qcom_pwm_chip {
		...
		struct qcom_pwm_channel channel[];
	};

	qc = devm_kzalloc(&pdev->dev, sizeof(*qc) + num_channels * sizeof(struct qcom_pwm_channel), GFP_KERNEL);

Then the individual channels can be accessed using qc->channel[i].

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 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Thierry Reding]

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

On Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Apr 27, 2021 at 06:22:10PM +0800, Fenglin Wu wrote:
[...]
> > diff --git a/drivers/pwm/pwm-qcom.c b/drivers/pwm/pwm-qcom.c
[...]
> > +#define PWM_FREQ_EXPONENT_MASK		GENMASK(2, 0)
> > +
> > +/* REG_PWM_TYPE_CONFIG */
> > +#define PWM_EN_GLITCH_REMOVAL_MASK	BIT(5)
> > +
> > +/* 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.

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.

> > +/* 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, 7};
> 
> Please also use a driver specific prefix for variables and function
> names.

Again, these are local symbols and there's no need for namespacing. The
only case where this would need to change is if the symbols started
conflicting with global ones, but until that happens, let's just keep
the names short and concise.

Thierry

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

Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Uwe Kleine-König]

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

Hello,

On Wed, Apr 28, 2021 at 07:46:56PM +0200, Thierry Reding wrote:
> On Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-König wrote:
> > 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.
> 
> 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.

> 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.
 
Best regards
Uwe

[1] A few posts that I found quickly:
    https://lore.kernel.org/lkml/YH2k5xnD%2F+CKnMBQ@hovoldconsulting.com/
    https://lore.kernel.org/lkml/CAPDyKFpg1qJD0r54sBC3hCoFey_+gwAL1n2o-aGwnAzAan5p7w@mail.gmail.com/
    https://lore.kernel.org/lkml/CAMpxmJW770v6JLdveEe1hkgNEJByVyArhorSyUZBYOyFiVyOeg@mail.gmail.com/
    https://lore.kernel.org/linux-can/fe0a8a9b-35c6-8f23-5968-0b14abb6078d@pengutronix.de/
    https://lore.kernel.org/netdev/20190327084422.4209-16-maxime.chevallier@bootlin.com/

-- 
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 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Lee Jones]

On Thu, 29 Apr 2021, Uwe Kleine-König wrote:

> Hello,
> 
> On Wed, Apr 28, 2021 at 07:46:56PM +0200, Thierry Reding wrote:
> > On Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-König wrote:
> > > 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.
> > 
> > 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.
> 
> > 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.

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.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Thierry Reding]

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

On Thu, Apr 29, 2021 at 08:06:53AM +0100, Lee Jones wrote:
> On Thu, 29 Apr 2021, Uwe Kleine-König wrote:
> 
> > Hello,
> > 
> > On Wed, Apr 28, 2021 at 07:46:56PM +0200, Thierry Reding wrote:
> > > On Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-König wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > > 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.
> 
> 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.

Thierry

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

Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Lee Jones]

On Thu, 29 Apr 2021, Thierry Reding wrote:

> On Thu, Apr 29, 2021 at 08:06:53AM +0100, Lee Jones wrote:
> > On Thu, 29 Apr 2021, Uwe Kleine-König wrote:
> > 
> > > Hello,
> > > 
> > > On Wed, Apr 28, 2021 at 07:46:56PM +0200, Thierry Reding wrote:
> > > > On Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-König wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > > 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.
> > 
> > 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.

I think you were very specific:

 "Again, these are local symbols and there's no need for namespacing. The
  only case where this would need to change is if the symbols started
  conflicting with global ones, but until that happens, let's just keep
  the names short and concise."

:)

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

I personally like namespacing defines too since it makes local ones
easily distinguishable from defines pulled in from API's header
files.

But at the end of the day, it's your train-set.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Thierry Reding]

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

On Thu, Apr 29, 2021 at 08:52:13AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Apr 28, 2021 at 07:46:56PM +0200, Thierry Reding wrote:
> > On Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-König wrote:
> > > 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.
> > 
> > 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.

I've said this elsewhere, and I specifically didn't comment on that
request, that I think namespacing for static functions does make sense
because they may show up in backtraces.

Register definitions, however, are never going to show up in a
backtrace. The only place where you will ever see them is within the
source file where the context is already plenty clear. The same goes
for locally scoped variables.

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

But those tools are primarily useful to find cross-references of
symbols. If you have symbols that are local to a single file, then it's
much easier to open that file in an editor than run the tools.

>                                                            It is
> sensible because it's harder than necessary to spot the error in
> 
> 	writel(PWM_EN_GLITCH_REMOVAL_MASK, base + REG_ENABLE_CONTROL);

I don't think it's sensible to rely on naming to detect errors in this
kind of construct. Either you write it correctly and the code does what
it's supposed to, or it isn't correct and the code will be broken.

> . 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".

Yes, sure, if you consider everyone incapable of making reasonable
decisions, then by all means, let's make it as easy as possible. Maybe
while at it you want to go and propagate those rules across the entire
kernel. Good luck with that.

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

Again, functions are an exception where the prefix makes sense.

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

Well, I'm sure I could find plenty of examples of maintainers *not*
requesting common prefixes because they disagree with your opinion.
Maybe you should think about why it's called a "bubble".

This is totally subjective and there aren't any clear rules. As such I
don't think we should enforce it. The one exception that we all seem to
agree on is static functions because they can show up in traces, but for
the rest I'm not going to enforce the common prefix.

Thierry

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