[PATCH v5 0/2] Add PWM support for Intel Keem Bay SoC

[PATCH v5 0/2] Add PWM support for Intel Keem Bay SoC

[vineetha.g.jaya.kumaran]

From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com>

Hi,

This patch set enables support for PWM on the Intel Keem Bay SoC.
Keem Bay is an ARM based SoC, and the GPIO module allows
configuration of 6 PWM outputs. Patch 1 adds the PWM driver and
Patch 2 is for the required Device Tree bindings documentation.

This driver was tested on the Keem Bay evaluation module board.

Thank you.

Best regards,
Vineetha

Changes since v4:
-Add co-developed-by tag
-Include mod_devicetable.h and remove of.h
-Update comment with correct calulation for high/low time
-Fix missing return from dev_err_probe

Changes since v3:
-Removed variable for address and calculate in place instead
-Utilized u32_replace_bits() when updating KMB_PWM_LEADIN_OFFSET
-Utilized dev_err_probe() for error reporting
-Updated comments to use physical units
-Updated error check for pwmchip_add()

Changes since v2:
-Include documentation about HW limitation/behaviour
-Use hex values for KMB_PWM_COUNT_MAX
-Redefine register macros
-Utilize FIELD_GET/FIELD_PREP for calculating pwm_l/h_count and pwm_count
-Round up duty cycle/period values
-Get current hardware state in .apply instead of cached values
-Do a polarity check before .enabled
-Round high time/low time to closest value
-Set enable bit in KMB_PWM_LEADIN_OFFSET to 0 in probe
-Correct the naming for MODULE_ALIAS
-Add additionalProperties: false in DT bindings

Changes since v1:
-Updated licensing info, "clocks" property and example in DT bindings
-Updated name of DT bindings document to match compatible string
-Removed 1 patch for addition of new sysfs attribute "count"
-Added support for COMPILE_TEST in Kconfig
-Updated naming of defines and regmap attribute
-Updated calculation of waveform high time and low time
-Added range checking for waveform high/low time
-Implemented .get_state
-Removed register writes for lead-in and count values (left to default)
-Updated register access to single-access
-Folded keembay_pwm_enable/disable_channel, keembay_pwm_config_period/duty_cycle,
 and keembay_pwm_config into keembay_pwm_apply
-Updated error messages/error codes
-Removed pwm_disable from keembay_pwm_remove
-Removed clk_prepare/clk_enable/clk_disable from driver

Lai, Poey Seng (1):
  pwm: Add PWM driver for Intel Keem Bay

Vineetha G. Jaya Kumaran (1):
  dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

 .../devicetree/bindings/pwm/intel,keembay-pwm.yaml |  47 +++++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-keembay.c                          | 228 +++++++++++++++++++++
 4 files changed, 285 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
 create mode 100644 drivers/pwm/pwm-keembay.c

-- 
1.9.1

[PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay

[vineetha.g.jaya.kumaran]

From: "Lai, Poey Seng" <poey.seng.lai@intel.com>

Enable PWM support for the Intel Keem Bay SoC.

Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com>
Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
---
 drivers/pwm/Kconfig       |   9 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-keembay.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 238 insertions(+)
 create mode 100644 drivers/pwm/pwm-keembay.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7dbcf69..0a68a167 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -560,4 +560,13 @@ config PWM_ZX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-zx.
 
+config PWM_KEEMBAY
+	tristate "Intel Keem Bay PWM driver"
+	depends on ARM64 || COMPILE_TEST
+	help
+	  The platform driver for Intel Keem Bay PWM controller.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-keembay.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 2c2ba0a..293e48f 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
 obj-$(CONFIG_PWM_ZX)		+= pwm-zx.o
+obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
new file mode 100644
index 00000000..3c7481f
--- /dev/null
+++ b/drivers/pwm/pwm-keembay.c
@@ -0,0 +1,228 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Keem Bay PWM driver
+ *
+ * Copyright (C) 2020 Intel Corporation
+ * Authors: Lai Poey Seng <poey.seng.lai@intel.com>
+ *          Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
+ *
+ * Limitation:
+ * - Upon disabling a channel, the currently running
+ *   period will not be completed. However, upon
+ *   reconfiguration of the duty cycle/period, the
+ *   currently running period will be completed first.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define KMB_TOTAL_PWM_CHANNELS		6
+#define KMB_PWM_COUNT_MAX		0xffff
+#define KMB_PWM_EN_BIT			BIT(31)
+
+/* Mask */
+#define KMB_PWM_HIGH_MASK		GENMASK(31, 16)
+#define KMB_PWM_LOW_MASK		GENMASK(15, 0)
+#define KMB_PWM_COUNT_MASK		GENMASK(31, 0)
+
+/* PWM Register offset */
+#define KMB_PWM_LEADIN_OFFSET(ch)	(0x00 + 4 * (ch))
+#define KMB_PWM_HIGHLOW_OFFSET(ch)	(0x20 + 4 * (ch))
+
+struct keembay_pwm {
+	struct pwm_chip chip;
+	struct device *dev;
+	struct clk *clk;
+	void __iomem *base;
+};
+
+static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip)
+{
+	return container_of(chip, struct keembay_pwm, chip);
+}
+
+static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask,
+					   u32 val, u32 offset)
+{
+	u32 buff = readl(priv->base + offset);
+
+	buff = u32_replace_bits(buff, val, mask);
+	writel(buff, priv->base + offset);
+}
+
+static void keembay_pwm_enable(struct keembay_pwm *priv, int ch)
+{
+	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1,
+				KMB_PWM_LEADIN_OFFSET(ch));
+}
+
+static void keembay_pwm_disable(struct keembay_pwm *priv, int ch)
+{
+	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0,
+				KMB_PWM_LEADIN_OFFSET(ch));
+}
+
+static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
+	unsigned long long pwm_h_count, pwm_l_count;
+	unsigned long clk_rate;
+	u32 buff;
+
+	clk_rate = clk_get_rate(priv->clk);
+
+	/* Read channel enabled status */
+	buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
+	if (buff & KMB_PWM_EN_BIT)
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	/* Read period and duty cycle */
+	buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
+	pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC;
+	pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
+	state->period = DIV_ROUND_UP_ULL(pwm_h_count + pwm_l_count, clk_rate);
+}
+
+static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
+	struct pwm_state current_state;
+	u16 pwm_h_count, pwm_l_count;
+	unsigned long long div;
+	unsigned long clk_rate;
+	u32 pwm_count = 0;
+
+	keembay_pwm_get_state(chip, pwm, &current_state);
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -ENOSYS;
+
+	if (!state->enabled && current_state.enabled) {
+		keembay_pwm_disable(priv, pwm->hwpwm);
+		return 0;
+	}
+
+	/*
+	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain
+	 * the high time of the waveform, while the last 16 bits contain
+	 * the low time of the waveform, in terms of clock cycles.
+	 *
+	 * high time = clock rate * duty cycle
+	 * low time =  clock rate * (period - duty cycle)
+	 *
+	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
+	 * high time = 500MHz * 30us = 0x3A98
+	 * low time = 500MHz * 20us = 0x2710
+	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
+	 */
+
+	clk_rate = clk_get_rate(priv->clk);
+
+	/* Configure waveform high time */
+	div = clk_rate * state->duty_cycle;
+	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
+	if (div > KMB_PWM_COUNT_MAX)
+		return -ERANGE;
+
+	pwm_h_count = div;
+
+	/* Configure waveform low time */
+	div = clk_rate * (state->period - state->duty_cycle);
+	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
+	if (div > KMB_PWM_COUNT_MAX)
+		return -ERANGE;
+
+	pwm_l_count = div;
+
+	pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) |
+		    FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
+
+	writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
+
+	if (state->enabled && !current_state.enabled)
+		keembay_pwm_enable(priv, pwm->hwpwm);
+
+	return 0;
+}
+
+static const struct pwm_ops keembay_pwm_ops = {
+	.owner = THIS_MODULE,
+	.apply = keembay_pwm_apply,
+	.get_state = keembay_pwm_get_state,
+};
+
+static int keembay_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct keembay_pwm *priv;
+	int ret, ch;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n");
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->chip.base = -1;
+	priv->chip.dev = dev;
+	priv->chip.ops = &keembay_pwm_ops;
+	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
+
+	ret = pwmchip_add(&priv->chip);
+	if (ret) {
+		dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Ensure enable bit for each channel is cleared at boot */
+	for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++)
+		keembay_pwm_disable(priv, ch);
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int keembay_pwm_remove(struct platform_device *pdev)
+{
+	struct keembay_pwm *priv = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&priv->chip);
+}
+
+static const struct of_device_id keembay_pwm_of_match[] = {
+	{ .compatible = "intel,keembay-pwm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, keembay_pwm_of_match);
+
+static struct platform_driver keembay_pwm_driver = {
+	.probe	= keembay_pwm_probe,
+	.remove	= keembay_pwm_remove,
+	.driver	= {
+		.name = "pwm-keembay",
+		.of_match_table = keembay_pwm_of_match,
+	},
+};
+module_platform_driver(keembay_pwm_driver);
+
+MODULE_ALIAS("platform:pwm-keembay");
+MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v5 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

[vineetha.g.jaya.kumaran]

From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com>

Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
---
 .../devicetree/bindings/pwm/intel,keembay-pwm.yaml | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
new file mode 100644
index 00000000..a374334
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2020 Intel Corporation
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/intel,keembay-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Keem Bay PWM Device Tree Bindings
+
+maintainers:
+  - Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    enum:
+      - intel,keembay-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - '#pwm-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #define KEEM_BAY_A53_GPIO
+
+    pwm@203200a0 {
+      compatible = "intel,keembay-pwm";
+      reg = <0x203200a0 0xe8>;
+      clocks = <&scmi_clk KEEM_BAY_A53_GPIO>;
+      #pwm-cells = <2>;
+    };
-- 
1.9.1

Re: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay

[Andy Shevchenko]

On Wed, Aug 26, 2020 at 06:25:58PM +0800, vineetha.g.jaya.kumaran@intel.com wrote:
> From: "Lai, Poey Seng" <poey.seng.lai@intel.com>
> 
> Enable PWM support for the Intel Keem Bay SoC.

LGTM, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com>
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> ---
>  drivers/pwm/Kconfig       |   9 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-keembay.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/pwm/pwm-keembay.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf69..0a68a167 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -560,4 +560,13 @@ config PWM_ZX
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-zx.
>  
> +config PWM_KEEMBAY
> +	tristate "Intel Keem Bay PWM driver"
> +	depends on ARM64 || COMPILE_TEST
> +	help
> +	  The platform driver for Intel Keem Bay PWM controller.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-keembay.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a..293e48f 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -54,3 +54,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
>  obj-$(CONFIG_PWM_ZX)		+= pwm-zx.o
> +obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
> diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
> new file mode 100644
> index 00000000..3c7481f
> --- /dev/null
> +++ b/drivers/pwm/pwm-keembay.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Keem Bay PWM driver
> + *
> + * Copyright (C) 2020 Intel Corporation
> + * Authors: Lai Poey Seng <poey.seng.lai@intel.com>
> + *          Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> + *
> + * Limitation:
> + * - Upon disabling a channel, the currently running
> + *   period will not be completed. However, upon
> + *   reconfiguration of the duty cycle/period, the
> + *   currently running period will be completed first.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define KMB_TOTAL_PWM_CHANNELS		6
> +#define KMB_PWM_COUNT_MAX		0xffff
> +#define KMB_PWM_EN_BIT			BIT(31)
> +
> +/* Mask */
> +#define KMB_PWM_HIGH_MASK		GENMASK(31, 16)
> +#define KMB_PWM_LOW_MASK		GENMASK(15, 0)
> +#define KMB_PWM_COUNT_MASK		GENMASK(31, 0)
> +
> +/* PWM Register offset */
> +#define KMB_PWM_LEADIN_OFFSET(ch)	(0x00 + 4 * (ch))
> +#define KMB_PWM_HIGHLOW_OFFSET(ch)	(0x20 + 4 * (ch))
> +
> +struct keembay_pwm {
> +	struct pwm_chip chip;
> +	struct device *dev;
> +	struct clk *clk;
> +	void __iomem *base;
> +};
> +
> +static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct keembay_pwm, chip);
> +}
> +
> +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask,
> +					   u32 val, u32 offset)
> +{
> +	u32 buff = readl(priv->base + offset);
> +
> +	buff = u32_replace_bits(buff, val, mask);
> +	writel(buff, priv->base + offset);
> +}
> +
> +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch)
> +{
> +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1,
> +				KMB_PWM_LEADIN_OFFSET(ch));
> +}
> +
> +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch)
> +{
> +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0,
> +				KMB_PWM_LEADIN_OFFSET(ch));
> +}
> +
> +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  struct pwm_state *state)
> +{
> +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> +	unsigned long long pwm_h_count, pwm_l_count;
> +	unsigned long clk_rate;
> +	u32 buff;
> +
> +	clk_rate = clk_get_rate(priv->clk);
> +
> +	/* Read channel enabled status */
> +	buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
> +	if (buff & KMB_PWM_EN_BIT)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +
> +	/* Read period and duty cycle */
> +	buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> +	pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC;
> +	pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC;
> +	state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
> +	state->period = DIV_ROUND_UP_ULL(pwm_h_count + pwm_l_count, clk_rate);
> +}
> +
> +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> +	struct pwm_state current_state;
> +	u16 pwm_h_count, pwm_l_count;
> +	unsigned long long div;
> +	unsigned long clk_rate;
> +	u32 pwm_count = 0;
> +
> +	keembay_pwm_get_state(chip, pwm, &current_state);
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -ENOSYS;
> +
> +	if (!state->enabled && current_state.enabled) {
> +		keembay_pwm_disable(priv, pwm->hwpwm);
> +		return 0;
> +	}
> +
> +	/*
> +	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain
> +	 * the high time of the waveform, while the last 16 bits contain
> +	 * the low time of the waveform, in terms of clock cycles.
> +	 *
> +	 * high time = clock rate * duty cycle
> +	 * low time =  clock rate * (period - duty cycle)
> +	 *
> +	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
> +	 * high time = 500MHz * 30us = 0x3A98
> +	 * low time = 500MHz * 20us = 0x2710
> +	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
> +	 */
> +
> +	clk_rate = clk_get_rate(priv->clk);
> +
> +	/* Configure waveform high time */
> +	div = clk_rate * state->duty_cycle;
> +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> +	if (div > KMB_PWM_COUNT_MAX)
> +		return -ERANGE;
> +
> +	pwm_h_count = div;
> +
> +	/* Configure waveform low time */
> +	div = clk_rate * (state->period - state->duty_cycle);
> +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> +	if (div > KMB_PWM_COUNT_MAX)
> +		return -ERANGE;
> +
> +	pwm_l_count = div;
> +
> +	pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) |
> +		    FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
> +
> +	writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> +
> +	if (state->enabled && !current_state.enabled)
> +		keembay_pwm_enable(priv, pwm->hwpwm);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops keembay_pwm_ops = {
> +	.owner = THIS_MODULE,
> +	.apply = keembay_pwm_apply,
> +	.get_state = keembay_pwm_get_state,
> +};
> +
> +static int keembay_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct keembay_pwm *priv;
> +	int ret, ch;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n");
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->chip.base = -1;
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &keembay_pwm_ops;
> +	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> +
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret) {
> +		dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	/* Ensure enable bit for each channel is cleared at boot */
> +	for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++)
> +		keembay_pwm_disable(priv, ch);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static int keembay_pwm_remove(struct platform_device *pdev)
> +{
> +	struct keembay_pwm *priv = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&priv->chip);
> +}
> +
> +static const struct of_device_id keembay_pwm_of_match[] = {
> +	{ .compatible = "intel,keembay-pwm" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, keembay_pwm_of_match);
> +
> +static struct platform_driver keembay_pwm_driver = {
> +	.probe	= keembay_pwm_probe,
> +	.remove	= keembay_pwm_remove,
> +	.driver	= {
> +		.name = "pwm-keembay",
> +		.of_match_table = keembay_pwm_of_match,
> +	},
> +};
> +module_platform_driver(keembay_pwm_driver);
> +
> +MODULE_ALIAS("platform:pwm-keembay");
> +MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.9.1
> 

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay

[G Jaya Kumaran, Vineetha]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, August 26, 2020 6:52 PM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com>
> Cc: thierry.reding@gmail.com; u.kleine-koenig@pengutronix.de;
> robh+dt@kernel.org; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>; Raja Subramanian, Lakshmi
> Bai <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay
> 
> On Wed, Aug 26, 2020 at 06:25:58PM +0800,
> vineetha.g.jaya.kumaran@intel.com wrote:
> > From: "Lai, Poey Seng" <poey.seng.lai@intel.com>
> >
> > Enable PWM support for the Intel Keem Bay SoC.
> 
> LGTM, FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 

Thanks Andy.

> > Co-developed-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@intel.com>
> > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com>
> > Signed-off-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@intel.com>
> > ---
> >  drivers/pwm/Kconfig       |   9 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-keembay.c | 228
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 238 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-keembay.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 7dbcf69..0a68a167 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -560,4 +560,13 @@ config PWM_ZX
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-zx.
> >
> > +config PWM_KEEMBAY
> > +	tristate "Intel Keem Bay PWM driver"
> > +	depends on ARM64 || COMPILE_TEST
> > +	help
> > +	  The platform driver for Intel Keem Bay PWM controller.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-keembay.
> > +
> >  endif
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 2c2ba0a..293e48f 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -54,3 +54,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
> >  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
> >  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> >  obj-$(CONFIG_PWM_ZX)		+= pwm-zx.o
> > +obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
> > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
> new
> > file mode 100644 index 00000000..3c7481f
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-keembay.c
> > @@ -0,0 +1,228 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel Keem Bay PWM driver
> > + *
> > + * Copyright (C) 2020 Intel Corporation
> > + * Authors: Lai Poey Seng <poey.seng.lai@intel.com>
> > + *          Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> > + *
> > + * Limitation:
> > + * - Upon disabling a channel, the currently running
> > + *   period will not be completed. However, upon
> > + *   reconfiguration of the duty cycle/period, the
> > + *   currently running period will be completed first.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define KMB_TOTAL_PWM_CHANNELS		6
> > +#define KMB_PWM_COUNT_MAX		0xffff
> > +#define KMB_PWM_EN_BIT			BIT(31)
> > +
> > +/* Mask */
> > +#define KMB_PWM_HIGH_MASK		GENMASK(31, 16)
> > +#define KMB_PWM_LOW_MASK		GENMASK(15, 0)
> > +#define KMB_PWM_COUNT_MASK		GENMASK(31, 0)
> > +
> > +/* PWM Register offset */
> > +#define KMB_PWM_LEADIN_OFFSET(ch)	(0x00 + 4 * (ch))
> > +#define KMB_PWM_HIGHLOW_OFFSET(ch)	(0x20 + 4 * (ch))
> > +
> > +struct keembay_pwm {
> > +	struct pwm_chip chip;
> > +	struct device *dev;
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +};
> > +
> > +static inline struct keembay_pwm *to_keembay_pwm_dev(struct
> pwm_chip
> > +*chip) {
> > +	return container_of(chip, struct keembay_pwm, chip); }
> > +
> > +static inline void keembay_pwm_update_bits(struct keembay_pwm
> *priv, u32 mask,
> > +					   u32 val, u32 offset)
> > +{
> > +	u32 buff = readl(priv->base + offset);
> > +
> > +	buff = u32_replace_bits(buff, val, mask);
> > +	writel(buff, priv->base + offset);
> > +}
> > +
> > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) {
> > +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1,
> > +				KMB_PWM_LEADIN_OFFSET(ch));
> > +}
> > +
> > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) {
> > +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0,
> > +				KMB_PWM_LEADIN_OFFSET(ch));
> > +}
> > +
> > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > +				  struct pwm_state *state)
> > +{
> > +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> > +	unsigned long long pwm_h_count, pwm_l_count;
> > +	unsigned long clk_rate;
> > +	u32 buff;
> > +
> > +	clk_rate = clk_get_rate(priv->clk);
> > +
> > +	/* Read channel enabled status */
> > +	buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm-
> >hwpwm));
> > +	if (buff & KMB_PWM_EN_BIT)
> > +		state->enabled = true;
> > +	else
> > +		state->enabled = false;
> > +
> > +	/* Read period and duty cycle */
> > +	buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm-
> >hwpwm));
> > +	pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) *
> NSEC_PER_SEC;
> > +	pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) *
> NSEC_PER_SEC;
> > +	state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
> > +	state->period = DIV_ROUND_UP_ULL(pwm_h_count +
> pwm_l_count,
> > +clk_rate); }
> > +
> > +static int keembay_pwm_apply(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > +			     const struct pwm_state *state) {
> > +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> > +	struct pwm_state current_state;
> > +	u16 pwm_h_count, pwm_l_count;
> > +	unsigned long long div;
> > +	unsigned long clk_rate;
> > +	u32 pwm_count = 0;
> > +
> > +	keembay_pwm_get_state(chip, pwm, &current_state);
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -ENOSYS;
> > +
> > +	if (!state->enabled && current_state.enabled) {
> > +		keembay_pwm_disable(priv, pwm->hwpwm);
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register
> contain
> > +	 * the high time of the waveform, while the last 16 bits contain
> > +	 * the low time of the waveform, in terms of clock cycles.
> > +	 *
> > +	 * high time = clock rate * duty cycle
> > +	 * low time =  clock rate * (period - duty cycle)
> > +	 *
> > +	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
> > +	 * high time = 500MHz * 30us = 0x3A98
> > +	 * low time = 500MHz * 20us = 0x2710
> > +	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
> > +	 */
> > +
> > +	clk_rate = clk_get_rate(priv->clk);
> > +
> > +	/* Configure waveform high time */
> > +	div = clk_rate * state->duty_cycle;
> > +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> > +	if (div > KMB_PWM_COUNT_MAX)
> > +		return -ERANGE;
> > +
> > +	pwm_h_count = div;
> > +
> > +	/* Configure waveform low time */
> > +	div = clk_rate * (state->period - state->duty_cycle);
> > +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> > +	if (div > KMB_PWM_COUNT_MAX)
> > +		return -ERANGE;
> > +
> > +	pwm_l_count = div;
> > +
> > +	pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK,
> pwm_h_count) |
> > +		    FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
> > +
> > +	writel(pwm_count, priv->base +
> KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> > +
> > +	if (state->enabled && !current_state.enabled)
> > +		keembay_pwm_enable(priv, pwm->hwpwm);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pwm_ops keembay_pwm_ops = {
> > +	.owner = THIS_MODULE,
> > +	.apply = keembay_pwm_apply,
> > +	.get_state = keembay_pwm_get_state,
> > +};
> > +
> > +static int keembay_pwm_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct keembay_pwm *priv;
> > +	int ret, ch;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->clk))
> > +		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to
> get
> > +clock\n");
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	priv->chip.base = -1;
> > +	priv->chip.dev = dev;
> > +	priv->chip.ops = &keembay_pwm_ops;
> > +	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> > +
> > +	ret = pwmchip_add(&priv->chip);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to add PWM chip: %pe\n",
> ERR_PTR(ret));
> > +		return ret;
> > +	}
> > +
> > +	/* Ensure enable bit for each channel is cleared at boot */
> > +	for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++)
> > +		keembay_pwm_disable(priv, ch);
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return 0;
> > +}
> > +
> > +static int keembay_pwm_remove(struct platform_device *pdev) {
> > +	struct keembay_pwm *priv = platform_get_drvdata(pdev);
> > +
> > +	return pwmchip_remove(&priv->chip);
> > +}
> > +
> > +static const struct of_device_id keembay_pwm_of_match[] = {
> > +	{ .compatible = "intel,keembay-pwm" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, keembay_pwm_of_match);
> > +
> > +static struct platform_driver keembay_pwm_driver = {
> > +	.probe	= keembay_pwm_probe,
> > +	.remove	= keembay_pwm_remove,
> > +	.driver	= {
> > +		.name = "pwm-keembay",
> > +		.of_match_table = keembay_pwm_of_match,
> > +	},
> > +};
> > +module_platform_driver(keembay_pwm_driver);
> > +
> > +MODULE_ALIAS("platform:pwm-keembay");
> > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
> MODULE_LICENSE("GPL
> > +v2");
> > --
> > 1.9.1
> >
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay

[Uwe Kleine-König]

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

Hello,

On Wed, Aug 26, 2020 at 06:25:58PM +0800, vineetha.g.jaya.kumaran@intel.com wrote:
> From: "Lai, Poey Seng" <poey.seng.lai@intel.com>
> 
> Enable PWM support for the Intel Keem Bay SoC.
> 
> Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com>
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> ---
>  drivers/pwm/Kconfig       |   9 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-keembay.c | 228 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/pwm/pwm-keembay.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf69..0a68a167 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -560,4 +560,13 @@ config PWM_ZX
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-zx.
>  
> +config PWM_KEEMBAY
> +	tristate "Intel Keem Bay PWM driver"
> +	depends on ARM64 || COMPILE_TEST
> +	help
> +	  The platform driver for Intel Keem Bay PWM controller.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-keembay.

The symbols in drivers/pwm/Kconfig are ordered alphabetically.

> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a..293e48f 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -54,3 +54,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
>  obj-$(CONFIG_PWM_ZX)		+= pwm-zx.o
> +obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o

ditto.

> diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
> new file mode 100644
> index 00000000..3c7481f
> --- /dev/null
> +++ b/drivers/pwm/pwm-keembay.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Keem Bay PWM driver
> + *
> + * Copyright (C) 2020 Intel Corporation
> + * Authors: Lai Poey Seng <poey.seng.lai@intel.com>
> + *          Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> + *
> + * Limitation:

Please make this "Limitations:" for consitency with other drivers. This
allows something like

	for f in drivers/pwm/pwm-*; do echo $f; sed -nr '/Limitations:/,/\*\/?$/p' $f; done

to work nicely.

> + * - Upon disabling a channel, the currently running
> + *   period will not be completed. However, upon
> + *   reconfiguration of the duty cycle/period, the
> + *   currently running period will be completed first.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define KMB_TOTAL_PWM_CHANNELS		6
> +#define KMB_PWM_COUNT_MAX		0xffff
> +#define KMB_PWM_EN_BIT			BIT(31)
> +
> +/* Mask */
> +#define KMB_PWM_HIGH_MASK		GENMASK(31, 16)
> +#define KMB_PWM_LOW_MASK		GENMASK(15, 0)
> +#define KMB_PWM_COUNT_MASK		GENMASK(31, 0)
> +
> +/* PWM Register offset */
> +#define KMB_PWM_LEADIN_OFFSET(ch)	(0x00 + 4 * (ch))
> +#define KMB_PWM_HIGHLOW_OFFSET(ch)	(0x20 + 4 * (ch))
> +
> +struct keembay_pwm {
> +	struct pwm_chip chip;
> +	struct device *dev;
> +	struct clk *clk;
> +	void __iomem *base;
> +};
> +
> +static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct keembay_pwm, chip);
> +}
> +
> +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask,
> +					   u32 val, u32 offset)
> +{
> +	u32 buff = readl(priv->base + offset);
> +
> +	buff = u32_replace_bits(buff, val, mask);
> +	writel(buff, priv->base + offset);
> +}
> +
> +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch)
> +{
> +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1,
> +				KMB_PWM_LEADIN_OFFSET(ch));
> +}
> +
> +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch)
> +{
> +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0,
> +				KMB_PWM_LEADIN_OFFSET(ch));
> +}
> +
> +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  struct pwm_state *state)
> +{
> +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> +	unsigned long long pwm_h_count, pwm_l_count;
> +	unsigned long clk_rate;
> +	u32 buff;
> +
> +	clk_rate = clk_get_rate(priv->clk);

clk_get_rate() must only be called when the clock is enabled. Unless I
miss something this isn't ensured here.

> +
> +	/* Read channel enabled status */
> +	buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
> +	if (buff & KMB_PWM_EN_BIT)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +
> +	/* Read period and duty cycle */
> +	buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> +	pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC;
> +	pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC;
> +	state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
> +	state->period = DIV_ROUND_UP_ULL(pwm_h_count + pwm_l_count, clk_rate);
> +}
> +
> +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> +	struct pwm_state current_state;
> +	u16 pwm_h_count, pwm_l_count;
> +	unsigned long long div;
> +	unsigned long clk_rate;
> +	u32 pwm_count = 0;
> +
> +	keembay_pwm_get_state(chip, pwm, &current_state);

This can be moved after the test for state->polarity.

> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -ENOSYS;
> +
> +	if (!state->enabled && current_state.enabled) {
> +		keembay_pwm_disable(priv, pwm->hwpwm);
> +		return 0;
> +	}

if (!state->enabled) {
	if (current_state.enabled)
		keembay_pwm_disable(priv, pwm->hwpwm);
	return 0;
}

> +
> +	/*
> +	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain
> +	 * the high time of the waveform, while the last 16 bits contain
> +	 * the low time of the waveform, in terms of clock cycles.
> +	 *
> +	 * high time = clock rate * duty cycle
> +	 * low time =  clock rate * (period - duty cycle)
> +	 *
> +	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
> +	 * high time = 500MHz * 30us = 0x3A98
> +	 * low time = 500MHz * 20us = 0x2710
> +	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
> +	 */
> +
> +	clk_rate = clk_get_rate(priv->clk);
> +
> +	/* Configure waveform high time */
> +	div = clk_rate * state->duty_cycle;

Since v5.9-rc1 (commit a9d887dc1c60ed67f2271d66560cdcf864c4a578)
state->duty_cycle is a 64 bit type. So div being unsigned long isn't
big enough on some platforms.

> +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> +	if (div > KMB_PWM_COUNT_MAX)
> +		return -ERANGE;
> +
> +	pwm_h_count = div;
> +
> +	/* Configure waveform low time */
> +	div = clk_rate * (state->period - state->duty_cycle);
> +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> +	if (div > KMB_PWM_COUNT_MAX)
> +		return -ERANGE;
> +
> +	pwm_l_count = div;
> +
> +	pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) |
> +		    FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
> +
> +	writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> +
> +	if (state->enabled && !current_state.enabled)
> +		keembay_pwm_enable(priv, pwm->hwpwm);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops keembay_pwm_ops = {
> +	.owner = THIS_MODULE,
> +	.apply = keembay_pwm_apply,
> +	.get_state = keembay_pwm_get_state,
> +};
> +
> +static int keembay_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct keembay_pwm *priv;
> +	int ret, ch;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n");
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->chip.base = -1;
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &keembay_pwm_ops;
> +	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> +
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret) {
> +		dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	/* Ensure enable bit for each channel is cleared at boot */
> +	for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++)
> +		keembay_pwm_disable(priv, ch);

.probe() is not supposed to change the state of the PWM.

> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}

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 v5 1/2] pwm: Add PWM driver for Intel Keem Bay

[G Jaya Kumaran, Vineetha]

Hi Uwe,

> -----Original Message-----
> From: linux-pwm-owner@vger.kernel.org <linux-pwm-
> owner@vger.kernel.org> On Behalf Of Uwe Kleine-König
> Sent: Wednesday, September 2, 2020 2:26 PM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com>
> Cc: thierry.reding@gmail.com; robh+dt@kernel.org; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan
> Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>;
> andriy.shevchenko@linux.intel.com; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay
> 
> Hello,
> 
> On Wed, Aug 26, 2020 at 06:25:58PM +0800,
> vineetha.g.jaya.kumaran@intel.com wrote:
> > From: "Lai, Poey Seng" <poey.seng.lai@intel.com>
> >
> > Enable PWM support for the Intel Keem Bay SoC.
> >
> > Co-developed-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@intel.com>
> > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com>
> > Signed-off-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@intel.com>
> > ---
> >  drivers/pwm/Kconfig       |   9 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-keembay.c | 228
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 238 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-keembay.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 7dbcf69..0a68a167 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -560,4 +560,13 @@ config PWM_ZX
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-zx.
> >
> > +config PWM_KEEMBAY
> > +	tristate "Intel Keem Bay PWM driver"
> > +	depends on ARM64 || COMPILE_TEST
> > +	help
> > +	  The platform driver for Intel Keem Bay PWM controller.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-keembay.
> 
> The symbols in drivers/pwm/Kconfig are ordered alphabetically.
> 

OK, I will reorder this.

> > +
> >  endif
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 2c2ba0a..293e48f 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -54,3 +54,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
> >  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
> >  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> >  obj-$(CONFIG_PWM_ZX)		+= pwm-zx.o
> > +obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
> 
> ditto.
> 
> > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
> new
> > file mode 100644 index 00000000..3c7481f
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-keembay.c
> > @@ -0,0 +1,228 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel Keem Bay PWM driver
> > + *
> > + * Copyright (C) 2020 Intel Corporation
> > + * Authors: Lai Poey Seng <poey.seng.lai@intel.com>
> > + *          Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> > + *
> > + * Limitation:
> 
> Please make this "Limitations:" for consitency with other drivers. This allows
> something like
> 
> 	for f in drivers/pwm/pwm-*; do echo $f; sed -nr
> '/Limitations:/,/\*\/?$/p' $f; done
> 
> to work nicely.
> 

OK, will update this to "Limitations:"

> > + * - Upon disabling a channel, the currently running
> > + *   period will not be completed. However, upon
> > + *   reconfiguration of the duty cycle/period, the
> > + *   currently running period will be completed first.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define KMB_TOTAL_PWM_CHANNELS		6
> > +#define KMB_PWM_COUNT_MAX		0xffff
> > +#define KMB_PWM_EN_BIT			BIT(31)
> > +
> > +/* Mask */
> > +#define KMB_PWM_HIGH_MASK		GENMASK(31, 16)
> > +#define KMB_PWM_LOW_MASK		GENMASK(15, 0)
> > +#define KMB_PWM_COUNT_MASK		GENMASK(31, 0)
> > +
> > +/* PWM Register offset */
> > +#define KMB_PWM_LEADIN_OFFSET(ch)	(0x00 + 4 * (ch))
> > +#define KMB_PWM_HIGHLOW_OFFSET(ch)	(0x20 + 4 * (ch))
> > +
> > +struct keembay_pwm {
> > +	struct pwm_chip chip;
> > +	struct device *dev;
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +};
> > +
> > +static inline struct keembay_pwm *to_keembay_pwm_dev(struct
> pwm_chip
> > +*chip) {
> > +	return container_of(chip, struct keembay_pwm, chip); }
> > +
> > +static inline void keembay_pwm_update_bits(struct keembay_pwm
> *priv, u32 mask,
> > +					   u32 val, u32 offset)
> > +{
> > +	u32 buff = readl(priv->base + offset);
> > +
> > +	buff = u32_replace_bits(buff, val, mask);
> > +	writel(buff, priv->base + offset);
> > +}
> > +
> > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) {
> > +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1,
> > +				KMB_PWM_LEADIN_OFFSET(ch));
> > +}
> > +
> > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) {
> > +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0,
> > +				KMB_PWM_LEADIN_OFFSET(ch));
> > +}
> > +
> > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > +				  struct pwm_state *state)
> > +{
> > +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> > +	unsigned long long pwm_h_count, pwm_l_count;
> > +	unsigned long clk_rate;
> > +	u32 buff;
> > +
> > +	clk_rate = clk_get_rate(priv->clk);
> 
> clk_get_rate() must only be called when the clock is enabled. Unless I miss
> something this isn't ensured here.
> 

My understanding is this would not be a problem, as according to databook, the GPIO block clock is auto-enabled, 
and also we are not doing any disabling in the driver for it. 

> > +
> > +	/* Read channel enabled status */
> > +	buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm-
> >hwpwm));
> > +	if (buff & KMB_PWM_EN_BIT)
> > +		state->enabled = true;
> > +	else
> > +		state->enabled = false;
> > +
> > +	/* Read period and duty cycle */
> > +	buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm-
> >hwpwm));
> > +	pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) *
> NSEC_PER_SEC;
> > +	pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) *
> NSEC_PER_SEC;
> > +	state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
> > +	state->period = DIV_ROUND_UP_ULL(pwm_h_count +
> pwm_l_count,
> > +clk_rate); }
> > +
> > +static int keembay_pwm_apply(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > +			     const struct pwm_state *state) {
> > +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> > +	struct pwm_state current_state;
> > +	u16 pwm_h_count, pwm_l_count;
> > +	unsigned long long div;
> > +	unsigned long clk_rate;
> > +	u32 pwm_count = 0;
> > +
> > +	keembay_pwm_get_state(chip, pwm, &current_state);
> 
> This can be moved after the test for state->polarity.
> 

OK, will move this.

> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -ENOSYS;
> > +
> > +	if (!state->enabled && current_state.enabled) {
> > +		keembay_pwm_disable(priv, pwm->hwpwm);
> > +		return 0;
> > +	}
> 
> if (!state->enabled) {
> 	if (current_state.enabled)
> 		keembay_pwm_disable(priv, pwm->hwpwm);
> 	return 0;
> }
> 

OK, I will update this.

> > +
> > +	/*
> > +	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register
> contain
> > +	 * the high time of the waveform, while the last 16 bits contain
> > +	 * the low time of the waveform, in terms of clock cycles.
> > +	 *
> > +	 * high time = clock rate * duty cycle
> > +	 * low time =  clock rate * (period - duty cycle)
> > +	 *
> > +	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
> > +	 * high time = 500MHz * 30us = 0x3A98
> > +	 * low time = 500MHz * 20us = 0x2710
> > +	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
> > +	 */
> > +
> > +	clk_rate = clk_get_rate(priv->clk);
> > +
> > +	/* Configure waveform high time */
> > +	div = clk_rate * state->duty_cycle;
> 
> Since v5.9-rc1 (commit a9d887dc1c60ed67f2271d66560cdcf864c4a578)
> state->duty_cycle is a 64 bit type. So div being unsigned long isn't
> big enough on some platforms.
> 

div is 64-bit here, so I guess I can keep it as is?

> > +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> > +	if (div > KMB_PWM_COUNT_MAX)
> > +		return -ERANGE;
> > +
> > +	pwm_h_count = div;
> > +
> > +	/* Configure waveform low time */
> > +	div = clk_rate * (state->period - state->duty_cycle);
> > +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> > +	if (div > KMB_PWM_COUNT_MAX)
> > +		return -ERANGE;
> > +
> > +	pwm_l_count = div;
> > +
> > +	pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK,
> pwm_h_count) |
> > +		    FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
> > +
> > +	writel(pwm_count, priv->base +
> KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> > +
> > +	if (state->enabled && !current_state.enabled)
> > +		keembay_pwm_enable(priv, pwm->hwpwm);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pwm_ops keembay_pwm_ops = {
> > +	.owner = THIS_MODULE,
> > +	.apply = keembay_pwm_apply,
> > +	.get_state = keembay_pwm_get_state,
> > +};
> > +
> > +static int keembay_pwm_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct keembay_pwm *priv;
> > +	int ret, ch;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->clk))
> > +		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to
> get
> > +clock\n");
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	priv->chip.base = -1;
> > +	priv->chip.dev = dev;
> > +	priv->chip.ops = &keembay_pwm_ops;
> > +	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> > +
> > +	ret = pwmchip_add(&priv->chip);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to add PWM chip: %pe\n",
> ERR_PTR(ret));
> > +		return ret;
> > +	}
> > +
> > +	/* Ensure enable bit for each channel is cleared at boot */
> > +	for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++)
> > +		keembay_pwm_disable(priv, ch);
> 
> .probe() is not supposed to change the state of the PWM.
> 

Sorry, I think misunderstood one of your comments in V2 and added this.
The reset value of the enable bit (and all other bits) in the LEADIN register is 0, so this may not be needed. 
If it's ok, I'll remove it.

> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return 0;
> > +}
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Thank you,
Vineetha

Re: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay

[Uwe Kleine-König]

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

Hello,

On Fri, Sep 04, 2020 at 09:42:37AM +0000, G Jaya Kumaran, Vineetha wrote:
> > > +	clk_rate = clk_get_rate(priv->clk);
> > 
> > clk_get_rate() must only be called when the clock is enabled. Unless I miss
> > something this isn't ensured here.
> 
> My understanding is this would not be a problem, as according to
> databook, the GPIO block clock is auto-enabled, and also we are not
> doing any disabling in the driver for it. 

It might not be a problem on your hardware today. But this is an API
requirement.

> > > +	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain
> > > +	 * the high time of the waveform, while the last 16 bits contain
> > > +	 * the low time of the waveform, in terms of clock cycles.
> > > +	 *
> > > +	 * high time = clock rate * duty cycle
> > > +	 * low time =  clock rate * (period - duty cycle)
> > > +	 *
> > > +	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
> > > +	 * high time = 500MHz * 30us = 0x3A98
> > > +	 * low time = 500MHz * 20us = 0x2710
> > > +	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
> > > +	 */
> > > +
> > > +	clk_rate = clk_get_rate(priv->clk);
> > > +
> > > +	/* Configure waveform high time */
> > > +	div = clk_rate * state->duty_cycle;
> > 
> > Since v5.9-rc1 (commit a9d887dc1c60ed67f2271d66560cdcf864c4a578)
> > state->duty_cycle is a 64 bit type. So div being unsigned long isn't
> > big enough on some platforms.
> 
> div is 64-bit here, so I guess I can keep it as is?

unsigned long isn't 64 bits wide on all platforms.

> > > +	/* Ensure enable bit for each channel is cleared at boot */
> > > +	for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++)
> > > +		keembay_pwm_disable(priv, ch);
> > 
> > .probe() is not supposed to change the state of the PWM.
> > 
> 
> Sorry, I think misunderstood one of your comments in V2 and added this.
> The reset value of the enable bit (and all other bits) in the LEADIN register is 0, so this may not be needed. 
> If it's ok, I'll remove it.

I think the right approach is to set the LEADIN register to 0 in
.apply().

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 v5 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

[Uwe Kleine-König]

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

On Wed, Aug 26, 2020 at 06:25:59PM +0800, vineetha.g.jaya.kumaran@intel.com wrote:
> From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com>
> 
> Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

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 v5 1/2] pwm: Add PWM driver for Intel Keem Bay

[G Jaya Kumaran, Vineetha]

> -----Original Message-----
> From: linux-pwm-owner@vger.kernel.org <linux-pwm-
> owner@vger.kernel.org> On Behalf Of Uwe Kleine-König
> Sent: Saturday, September 5, 2020 4:26 AM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com>
> Cc: thierry.reding@gmail.com; robh+dt@kernel.org; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan
> Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>;
> andriy.shevchenko@linux.intel.com; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>; mgross@linux.intel.com;
> Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@intel.com>
> Subject: Re: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay
> 
> Hello,
> 
> On Fri, Sep 04, 2020 at 09:42:37AM +0000, G Jaya Kumaran, Vineetha wrote:
> > > > +	clk_rate = clk_get_rate(priv->clk);
> > >
> > > clk_get_rate() must only be called when the clock is enabled. Unless
> > > I miss something this isn't ensured here.
> >
> > My understanding is this would not be a problem, as according to
> > databook, the GPIO block clock is auto-enabled, and also we are not
> > doing any disabling in the driver for it.
> 
> It might not be a problem on your hardware today. But this is an API
> requirement.
> 

OK, will add clock enablement before using clk_get_rate().

> > > > +	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register
> contain
> > > > +	 * the high time of the waveform, while the last 16 bits contain
> > > > +	 * the low time of the waveform, in terms of clock cycles.
> > > > +	 *
> > > > +	 * high time = clock rate * duty cycle
> > > > +	 * low time =  clock rate * (period - duty cycle)
> > > > +	 *
> > > > +	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
> > > > +	 * high time = 500MHz * 30us = 0x3A98
> > > > +	 * low time = 500MHz * 20us = 0x2710
> > > > +	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
> > > > +	 */
> > > > +
> > > > +	clk_rate = clk_get_rate(priv->clk);
> > > > +
> > > > +	/* Configure waveform high time */
> > > > +	div = clk_rate * state->duty_cycle;
> > >
> > > Since v5.9-rc1 (commit a9d887dc1c60ed67f2271d66560cdcf864c4a578)
> > > state->duty_cycle is a 64 bit type. So div being unsigned long isn't
> > > big enough on some platforms.
> >
> > div is 64-bit here, so I guess I can keep it as is?
> 
> unsigned long isn't 64 bits wide on all platforms.
> 

Sorry, I meant I would keep it as is since div is an unsigned long long here, not an unsigned long.

> > > > +	/* Ensure enable bit for each channel is cleared at boot */
> > > > +	for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++)
> > > > +		keembay_pwm_disable(priv, ch);
> > >
> > > .probe() is not supposed to change the state of the PWM.
> > >
> >
> > Sorry, I think misunderstood one of your comments in V2 and added this.
> > The reset value of the enable bit (and all other bits) in the LEADIN register
> is 0, so this may not be needed.
> > If it's ok, I'll remove it.
> 
> I think the right approach is to set the LEADIN register to 0 in .apply().
> 

Okay, will do this.

> Best regards
> Uwe
> 

Thank you,
Vineetha

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