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

[PATCH 0/3] 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 a new count attribute to
the sysfs, Patch 2 adds the PWM driver and Patch 3 is for the
required Device Tree bindings documentation.

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

Thank you.

Best regards,
Vineetha

Lai, Poey Seng (2):
  pwm: Add count attribute in sysfs for Intel Keem Bay
  pwm: Add PWM driver for Intel Keem Bay

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

 Documentation/ABI/testing/sysfs-class-pwm          |   9 +
 .../devicetree/bindings/pwm/pwm-keembay.yaml       |  39 +++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/core.c                                 |   3 +-
 drivers/pwm/pwm-keembay.c                          | 308 +++++++++++++++++++++
 drivers/pwm/sysfs.c                                |  37 +++
 include/linux/pwm.h                                |   2 +
 8 files changed, 407 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
 create mode 100644 drivers/pwm/pwm-keembay.c

-- 
1.9.1

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

[vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w]

From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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 a new count attribute to
the sysfs, Patch 2 adds the PWM driver and Patch 3 is for the
required Device Tree bindings documentation.

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

Thank you.

Best regards,
Vineetha

Lai, Poey Seng (2):
  pwm: Add count attribute in sysfs for Intel Keem Bay
  pwm: Add PWM driver for Intel Keem Bay

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

 Documentation/ABI/testing/sysfs-class-pwm          |   9 +
 .../devicetree/bindings/pwm/pwm-keembay.yaml       |  39 +++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/core.c                                 |   3 +-
 drivers/pwm/pwm-keembay.c                          | 308 +++++++++++++++++++++
 drivers/pwm/sysfs.c                                |  37 +++
 include/linux/pwm.h                                |   2 +
 8 files changed, 407 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
 create mode 100644 drivers/pwm/pwm-keembay.c

-- 
1.9.1

[PATCH 1/3] pwm: Add count attribute in sysfs for Intel Keem Bay

[vineetha.g.jaya.kumaran]

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

In Keem Bay, the number of repetitions for the period/waveform
can be configured from userspace. This requires addition of a sysfs
attribute to get/set the repetition count. Setting this value to 0
will result in continuous repetition of the waveform until the
channel is disabled or reconfigured.

Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com>
Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
---
 Documentation/ABI/testing/sysfs-class-pwm |  9 ++++++++
 drivers/pwm/core.c                        |  3 ++-
 drivers/pwm/sysfs.c                       | 37 +++++++++++++++++++++++++++++++
 include/linux/pwm.h                       |  2 ++
 4 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-pwm b/Documentation/ABI/testing/sysfs-class-pwm
index c20e613..87e219f 100644
--- a/Documentation/ABI/testing/sysfs-class-pwm
+++ b/Documentation/ABI/testing/sysfs-class-pwm
@@ -86,3 +86,12 @@ Description:
 		Capture information about a PWM signal. The output format is a
 		pair unsigned integers (period and duty cycle), separated by a
 		single space.
+
+What:		/sys/class/pwm/pwmchipN/pwmX/count
+Date:		May 2020
+KernelVersion:	5.6
+Contact:	Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
+Description:
+		Sets the repetition count of a PWM waveform. A value of 0 will
+		result in continuous repetition of the waveform until the
+		channel is disabled or reconfigured.
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index bca0496..fd42fb6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -584,7 +584,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	if (state->period == pwm->state.period &&
 	    state->duty_cycle == pwm->state.duty_cycle &&
 	    state->polarity == pwm->state.polarity &&
-	    state->enabled == pwm->state.enabled)
+	    state->enabled == pwm->state.enabled &&
+	    state->count == pwm->state.count)
 		return 0;
 
 	if (chip->ops->apply) {
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 2389b86..3c474fa 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -215,11 +215,47 @@ static ssize_t capture_show(struct device *child,
 	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
 }
 
+static ssize_t count_store(struct device *child,
+			      struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_device *pwm = export->pwm;
+	struct pwm_state state;
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, &state);
+	state.count = val;
+	ret = pwm_apply_state(pwm, &state);
+	mutex_unlock(&export->lock);
+
+	return ret ? : size;
+}
+
+static ssize_t count_show(struct device *child,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return sprintf(buf, "%d\n", state.count);
+}
+
 static DEVICE_ATTR_RW(period);
 static DEVICE_ATTR_RW(duty_cycle);
 static DEVICE_ATTR_RW(enable);
 static DEVICE_ATTR_RW(polarity);
 static DEVICE_ATTR_RO(capture);
+static DEVICE_ATTR_RW(count);
 
 static struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
@@ -227,6 +263,7 @@ static ssize_t capture_show(struct device *child,
 	&dev_attr_enable.attr,
 	&dev_attr_polarity.attr,
 	&dev_attr_capture.attr,
+	&dev_attr_count.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2635b2a..c874559 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -52,12 +52,14 @@ enum {
  * struct pwm_state - state of a PWM channel
  * @period: PWM period (in nanoseconds)
  * @duty_cycle: PWM duty cycle (in nanoseconds)
+ * @count: Repeat count of PWM waveforms.
  * @polarity: PWM polarity
  * @enabled: PWM enabled status
  */
 struct pwm_state {
 	unsigned int period;
 	unsigned int duty_cycle;
+	unsigned int count;
 	enum pwm_polarity polarity;
 	bool enabled;
 };
-- 
1.9.1

[PATCH 1/3] pwm: Add count attribute in sysfs for Intel Keem Bay

[vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w]

From: "Lai, Poey Seng" <poey.seng.lai-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

In Keem Bay, the number of repetitions for the period/waveform
can be configured from userspace. This requires addition of a sysfs
attribute to get/set the repetition count. Setting this value to 0
will result in continuous repetition of the waveform until the
channel is disabled or reconfigured.

Signed-off-by: Lai, Poey Seng <poey.seng.lai-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-class-pwm |  9 ++++++++
 drivers/pwm/core.c                        |  3 ++-
 drivers/pwm/sysfs.c                       | 37 +++++++++++++++++++++++++++++++
 include/linux/pwm.h                       |  2 ++
 4 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-pwm b/Documentation/ABI/testing/sysfs-class-pwm
index c20e613..87e219f 100644
--- a/Documentation/ABI/testing/sysfs-class-pwm
+++ b/Documentation/ABI/testing/sysfs-class-pwm
@@ -86,3 +86,12 @@ Description:
 		Capture information about a PWM signal. The output format is a
 		pair unsigned integers (period and duty cycle), separated by a
 		single space.
+
+What:		/sys/class/pwm/pwmchipN/pwmX/count
+Date:		May 2020
+KernelVersion:	5.6
+Contact:	Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+Description:
+		Sets the repetition count of a PWM waveform. A value of 0 will
+		result in continuous repetition of the waveform until the
+		channel is disabled or reconfigured.
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index bca0496..fd42fb6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -584,7 +584,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	if (state->period == pwm->state.period &&
 	    state->duty_cycle == pwm->state.duty_cycle &&
 	    state->polarity == pwm->state.polarity &&
-	    state->enabled == pwm->state.enabled)
+	    state->enabled == pwm->state.enabled &&
+	    state->count == pwm->state.count)
 		return 0;
 
 	if (chip->ops->apply) {
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 2389b86..3c474fa 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -215,11 +215,47 @@ static ssize_t capture_show(struct device *child,
 	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
 }
 
+static ssize_t count_store(struct device *child,
+			      struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_device *pwm = export->pwm;
+	struct pwm_state state;
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, &state);
+	state.count = val;
+	ret = pwm_apply_state(pwm, &state);
+	mutex_unlock(&export->lock);
+
+	return ret ? : size;
+}
+
+static ssize_t count_show(struct device *child,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return sprintf(buf, "%d\n", state.count);
+}
+
 static DEVICE_ATTR_RW(period);
 static DEVICE_ATTR_RW(duty_cycle);
 static DEVICE_ATTR_RW(enable);
 static DEVICE_ATTR_RW(polarity);
 static DEVICE_ATTR_RO(capture);
+static DEVICE_ATTR_RW(count);
 
 static struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
@@ -227,6 +263,7 @@ static ssize_t capture_show(struct device *child,
 	&dev_attr_enable.attr,
 	&dev_attr_polarity.attr,
 	&dev_attr_capture.attr,
+	&dev_attr_count.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2635b2a..c874559 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -52,12 +52,14 @@ enum {
  * struct pwm_state - state of a PWM channel
  * @period: PWM period (in nanoseconds)
  * @duty_cycle: PWM duty cycle (in nanoseconds)
+ * @count: Repeat count of PWM waveforms.
  * @polarity: PWM polarity
  * @enabled: PWM enabled status
  */
 struct pwm_state {
 	unsigned int period;
 	unsigned int duty_cycle;
+	unsigned int count;
 	enum pwm_polarity polarity;
 	bool enabled;
 };
-- 
1.9.1

Re: [PATCH 1/3] pwm: Add count attribute in sysfs for Intel Keem Bay

[Uwe Kleine-König]

On Sun, May 17, 2020 at 09:52:38PM +0800, vineetha.g.jaya.kumaran@intel.com wrote:
> From: "Lai, Poey Seng" <poey.seng.lai@intel.com>
> 
> In Keem Bay, the number of repetitions for the period/waveform
> can be configured from userspace. This requires addition of a sysfs
> attribute to get/set the repetition count. Setting this value to 0
> will result in continuous repetition of the waveform until the
> channel is disabled or reconfigured.

There is nothing specific for Keem Bay in this patch, the only special
thing here is that this driver is the first implementor.

IMHO all drivers that don't support count should be changed to fail if
a count > 0 is passed and introducing support in the sysfs interface
should be a separate change.

Having said that I'm not convinced this is a good idea given that only
very few driver can support this interface. Also this needs
documentation, e.g. what is expected from .get_state().

You should also motivate what this functionality is used for in the
commit log and I'd prefer to see an in-tree user (apart from sysfs which
I don't count as such).

Best regards
Uwe

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

RE: [PATCH 1/3] pwm: Add count attribute in sysfs 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: Sunday, May 24, 2020 5:05 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>; Shevchenko,
> Andriy <andriy.shevchenko@intel.com>
> Subject: Re: [PATCH 1/3] pwm: Add count attribute in sysfs for Intel Keem Bay
> 
> On Sun, May 17, 2020 at 09:52:38PM +0800,
> vineetha.g.jaya.kumaran@intel.com wrote:
> > From: "Lai, Poey Seng" <poey.seng.lai@intel.com>
> >
> > In Keem Bay, the number of repetitions for the period/waveform can be
> > configured from userspace. This requires addition of a sysfs attribute
> > to get/set the repetition count. Setting this value to 0 will result
> > in continuous repetition of the waveform until the channel is disabled
> > or reconfigured.
> 
> There is nothing specific for Keem Bay in this patch, the only special thing here is
> that this driver is the first implementor.
> 
> IMHO all drivers that don't support count should be changed to fail if a count > 0
> is passed and introducing support in the sysfs interface should be a separate
> change.
> 
> Having said that I'm not convinced this is a good idea given that only very few
> driver can support this interface. Also this needs documentation, e.g. what is
> expected from .get_state().
> 
> You should also motivate what this functionality is used for in the commit log
> and I'd prefer to see an in-tree user (apart from sysfs which I don't count as
> such).
> 

Agreed, the wording used here is not accurate as this is not specific for Keem Bay.
Before submitting v2, I will cross-check about the use-case for this functionality, and if it 
can be implemented in some other less intrusive way to the framework (perhaps via a DT property?)

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

RE: [PATCH 1/3] pwm: Add count attribute in sysfs 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: Sunday, May 24, 2020 5:05 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>; Shevchenko,
> Andriy <andriy.shevchenko@intel.com>
> Subject: Re: [PATCH 1/3] pwm: Add count attribute in sysfs for Intel Keem Bay
> 
> On Sun, May 17, 2020 at 09:52:38PM +0800,
> vineetha.g.jaya.kumaran@intel.com wrote:
> > From: "Lai, Poey Seng" <poey.seng.lai@intel.com>
> >
> > In Keem Bay, the number of repetitions for the period/waveform can be
> > configured from userspace. This requires addition of a sysfs attribute
> > to get/set the repetition count. Setting this value to 0 will result
> > in continuous repetition of the waveform until the channel is disabled
> > or reconfigured.
> 
> There is nothing specific for Keem Bay in this patch, the only special thing here is
> that this driver is the first implementor.
> 
> IMHO all drivers that don't support count should be changed to fail if a count > 0
> is passed and introducing support in the sysfs interface should be a separate
> change.
> 
> Having said that I'm not convinced this is a good idea given that only very few
> driver can support this interface. Also this needs documentation, e.g. what is
> expected from .get_state().
> 
> You should also motivate what this functionality is used for in the commit log
> and I'd prefer to see an in-tree user (apart from sysfs which I don't count as
> such).
> 

Agreed, the wording used here is not accurate as this is not specific for Keem Bay.
Before submitting v2, I will cross-check about the use-case for this functionality, and if it 
can be implemented in some other less intrusive way to the framework (perhaps via a DT property?)

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

[PATCH 2/3] 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.

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 | 308 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)
 create mode 100644 drivers/pwm/pwm-keembay.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c13d146..5311975 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -569,4 +569,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
+	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 a59c710..0c84ff2 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -55,3 +55,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 0000000..39c7310
--- /dev/null
+++ b/drivers/pwm/pwm-keembay.c
@@ -0,0 +1,308 @@
+// 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>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define TOTAL_PWM_CHANNELS		6
+#define LEAD_IN_DEFAULT			0
+#define PWM_COUNT_MAX			65535
+
+#define KEEMBAY_PWM_EN_BIT		31
+
+/* Mask */
+#define KEEMBAY_PWM_RPT_CNT_MASK	GENMASK(15, 0)
+#define KEEMBAY_PWM_LEAD_IN_MASK	GENMASK(30, 16)
+#define KEEMBAY_PWM_HIGH_MASK		GENMASK(31, 16)
+#define KEEMBAY_PWM_LOW_MASK		GENMASK(15, 0)
+
+/* PWM Register offset */
+#define PWM_LEADIN0_OFFSET		0x00
+#define PWM_LEADIN1_OFFSET		0x04
+#define PWM_LEADIN2_OFFSET		0x08
+#define PWM_LEADIN3_OFFSET		0x0c
+#define PWM_LEADIN4_OFFSET		0x10
+#define PWM_LEADIN5_OFFSET		0x14
+
+#define PWM_HIGHLOW0_OFFSET		0x20
+#define PWM_HIGHLOW1_OFFSET		0x24
+#define PWM_HIGHLOW2_OFFSET		0x28
+#define PWM_HIGHLOW3_OFFSET		0x2c
+#define PWM_HIGHLOW4_OFFSET		0x30
+#define PWM_HIGHLOW5_OFFSET		0x34
+
+struct keembay_pwm {
+	struct pwm_chip chip;
+	struct device *dev;
+	struct clk *clk;
+	void __iomem *regmap;
+};
+
+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_enable_channel(struct keembay_pwm *priv, int ch)
+{
+	u32 buff, offset;
+	void __iomem *address;
+
+	offset = PWM_LEADIN0_OFFSET + ch * 4;
+	address = priv->regmap + offset;
+	buff = readl(address);
+	buff |= BIT(KEEMBAY_PWM_EN_BIT);
+	writel(buff, address);
+}
+
+static inline void keembay_pwm_disable_channel(struct keembay_pwm *priv, int ch)
+{
+	u32 buff, offset;
+	void __iomem *address;
+
+	offset = PWM_LEADIN0_OFFSET + ch * 4;
+	address = priv->regmap + offset;
+	buff = readl(address);
+	buff &= ~BIT(KEEMBAY_PWM_EN_BIT);
+	writel(buff, address);
+}
+
+static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask,
+					   u32 val, u32 reg, int ch)
+{
+	u32 buff, offset, tmp;
+	void __iomem *address;
+
+	offset = reg + ch * 4;
+	address = priv->regmap + offset;
+	buff = readl(address);
+	tmp = buff & ~mask;
+	tmp |= FIELD_PREP(mask, val);
+	writel(tmp, address);
+}
+
+static inline u32 keembay_pwm_config_min(struct keembay_pwm *priv)
+{
+	unsigned long long divd, divs;
+
+	divd = NSEC_PER_SEC;
+	divs = clk_get_rate(priv->clk);
+	do_div(divd, divs);
+
+	return (u32)divd;
+}
+
+static inline u16 keembay_pwm_config_duty_cycle(struct keembay_pwm *priv,
+						int duty_ns, u32 ns_min)
+{
+	unsigned long long divd;
+
+	divd = duty_ns;
+	do_div(divd, ns_min);
+	if ((u16)divd == 0)
+		return 0;
+
+	return (u16)divd - 1;
+}
+
+static inline u16 keembay_pwm_config_period(struct keembay_pwm *priv,
+					    int period_ns,
+					    int duty_ns,
+					    u32 ns_min)
+{
+	unsigned long long divd;
+
+	divd = period_ns - duty_ns;
+	do_div(divd, ns_min);
+	if ((u16)divd == 0)
+		return 0;
+
+	return (u16)divd - 1;
+}
+
+/*
+ *	For calculating "high time" register value:
+ *	High time (quotient only) = duty_cycle / ns_min
+ *
+ *	For calculating "low time" register value:
+ *	Low time (quotient only) = (period - duty_cycle) / ns_min
+ *
+ *	All values used are in nanoseconds for calculation.
+ */
+static int keembay_pwm_config(struct keembay_pwm *priv, int ch,
+			      int duty_ns, int period_ns, int count)
+{
+	u32 ns_min;
+	u16 pwm_h_count, pwm_l_count;
+
+	/* Write to lead in */
+	keembay_pwm_update_bits(priv, KEEMBAY_PWM_LEAD_IN_MASK,
+				LEAD_IN_DEFAULT,
+				PWM_LEADIN0_OFFSET, ch);
+
+	/* Write the number of PWM pulse repetition */
+	keembay_pwm_update_bits(priv, KEEMBAY_PWM_RPT_CNT_MASK, count,
+				PWM_LEADIN0_OFFSET, ch);
+
+	/* Calculate min */
+	ns_min = keembay_pwm_config_min(priv);
+
+	/* For duty cycle */
+	pwm_h_count = keembay_pwm_config_duty_cycle(priv, duty_ns, ns_min);
+
+	/* Write to high registers */
+	keembay_pwm_update_bits(priv, KEEMBAY_PWM_HIGH_MASK, pwm_h_count,
+				PWM_HIGHLOW0_OFFSET, ch);
+
+	/* For period */
+	pwm_l_count = keembay_pwm_config_period(priv, period_ns, duty_ns,
+						ns_min);
+
+	/* Write to low registers */
+	keembay_pwm_update_bits(priv, KEEMBAY_PWM_LOW_MASK, pwm_l_count,
+				PWM_HIGHLOW0_OFFSET, ch);
+
+	return 0;
+}
+
+static int keembay_pwm_enable(struct keembay_pwm *priv, int ch)
+{
+	int ret;
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	/* Enable channel */
+	keembay_pwm_enable_channel(priv, ch);
+
+	return 0;
+}
+
+static void keembay_pwm_disable(struct keembay_pwm *priv, int ch)
+{
+	/* Disable channel */
+	keembay_pwm_disable_channel(priv, ch);
+
+	clk_disable(priv->clk);
+}
+
+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);
+
+	if (!state->enabled && pwm_is_enabled(pwm)) {
+		keembay_pwm_disable(priv, pwm->hwpwm);
+		return 0;
+	}
+
+	if (state->count > PWM_COUNT_MAX)
+		return -EINVAL;
+
+	if (state->polarity != pwm_get_polarity(pwm))
+		return -ENOSYS;
+
+	keembay_pwm_config(priv, pwm->hwpwm, state->duty_cycle,
+			   state->period, state->count);
+
+	if (state->enabled && !pwm_is_enabled(pwm))
+		return keembay_pwm_enable(priv, pwm->hwpwm);
+
+	return 0;
+}
+
+static const struct pwm_ops keembay_pwm_ops = {
+	.owner = THIS_MODULE,
+	.apply = keembay_pwm_apply,
+};
+
+static int keembay_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct keembay_pwm *priv;
+	int ret;
+
+	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 PTR_ERR(priv->clk);
+
+	/*
+	 * Prepare clock here, and carry out clock enabling/disabling
+	 * during channel enablement/disablement.
+	 * The clock will not be unprepared due to shared usage with GPIO.
+	 */
+	ret = clk_prepare(priv->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to prepare PWM clock\n");
+		return ret;
+	}
+
+	priv->regmap = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	priv->chip.base = -1;
+	priv->chip.dev = dev;
+	priv->chip.ops = &keembay_pwm_ops;
+	priv->chip.npwm = TOTAL_PWM_CHANNELS;
+
+	ret = pwmchip_add(&priv->chip);
+	if (ret < 0) {
+		dev_err(dev, "Failed to add PWM chip: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int keembay_pwm_remove(struct platform_device *pdev)
+{
+	struct keembay_pwm *priv = platform_get_drvdata(pdev);
+	unsigned int i;
+
+	for (i = 0; i < priv->chip.npwm; i++)
+		pwm_disable(&priv->chip.pwms[i]);
+
+	pwmchip_remove(&priv->chip);
+
+	return 0;
+}
+
+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:keembay");
+MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[Uwe Kleine-König]

Hello,

On Sun, May 17, 2020 at 09:52:39PM +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.
> 
> 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 | 308 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 318 insertions(+)
>  create mode 100644 drivers/pwm/pwm-keembay.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index c13d146..5311975 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -569,4 +569,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

Support for COMPILE_TEST would be nice here.

> +	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 a59c710..0c84ff2 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -55,3 +55,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 0000000..39c7310
> --- /dev/null
> +++ b/drivers/pwm/pwm-keembay.c
> @@ -0,0 +1,308 @@
> +// 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>

Is there publically available documentation? If so, please add a link
here.

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define TOTAL_PWM_CHANNELS		6
> +#define LEAD_IN_DEFAULT			0
> +#define PWM_COUNT_MAX			65535

please use a unique prefix for all defines in your driver. The names as
they are are too generic.

> +
> +#define KEEMBAY_PWM_EN_BIT		31
> +
> +/* Mask */
> +#define KEEMBAY_PWM_RPT_CNT_MASK	GENMASK(15, 0)
> +#define KEEMBAY_PWM_LEAD_IN_MASK	GENMASK(30, 16)
> +#define KEEMBAY_PWM_HIGH_MASK		GENMASK(31, 16)
> +#define KEEMBAY_PWM_LOW_MASK		GENMASK(15, 0)
> +
> +/* PWM Register offset */
> +#define PWM_LEADIN0_OFFSET		0x00
> +#define PWM_LEADIN1_OFFSET		0x04
> +#define PWM_LEADIN2_OFFSET		0x08
> +#define PWM_LEADIN3_OFFSET		0x0c
> +#define PWM_LEADIN4_OFFSET		0x10
> +#define PWM_LEADIN5_OFFSET		0x14
> +
> +#define PWM_HIGHLOW0_OFFSET		0x20
> +#define PWM_HIGHLOW1_OFFSET		0x24
> +#define PWM_HIGHLOW2_OFFSET		0x28
> +#define PWM_HIGHLOW3_OFFSET		0x2c
> +#define PWM_HIGHLOW4_OFFSET		0x30
> +#define PWM_HIGHLOW5_OFFSET		0x34
> +
> +struct keembay_pwm {
> +	struct pwm_chip chip;
> +	struct device *dev;
> +	struct clk *clk;
> +	void __iomem *regmap;

I'd call this member "base" instead of "regmap" as the latter has a
different meaning in the kernel.

> +};
> +
> +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_enable_channel(struct keembay_pwm *priv, int ch)
> +{
> +	u32 buff, offset;
> +	void __iomem *address;
> +
> +	offset = PWM_LEADIN0_OFFSET + ch * 4;
> +	address = priv->regmap + offset;
> +	buff = readl(address);
> +	buff |= BIT(KEEMBAY_PWM_EN_BIT);
> +	writel(buff, address);
> +}
> +
> +static inline void keembay_pwm_disable_channel(struct keembay_pwm *priv, int ch)
> +{
> +	u32 buff, offset;
> +	void __iomem *address;
> +
> +	offset = PWM_LEADIN0_OFFSET + ch * 4;
> +	address = priv->regmap + offset;
> +	buff = readl(address);
> +	buff &= ~BIT(KEEMBAY_PWM_EN_BIT);
> +	writel(buff, address);
> +}

The two functions above could make use of keembay_pwm_update_bits().

> +
> +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask,
> +					   u32 val, u32 reg, int ch)
> +{
> +	u32 buff, offset, tmp;
> +	void __iomem *address;
> +
> +	offset = reg + ch * 4;
> +	address = priv->regmap + offset;
> +	buff = readl(address);
> +	tmp = buff & ~mask;
> +	tmp |= FIELD_PREP(mask, val);
> +	writel(tmp, address);
> +}
> +
> +static inline u32 keembay_pwm_config_min(struct keembay_pwm *priv)
> +{
> +	unsigned long long divd, divs;
> +
> +	divd = NSEC_PER_SEC;
> +	divs = clk_get_rate(priv->clk);
> +	do_div(divd, divs);

Given that both NSEC_PER_SEC and the return value of clk_get_rate fit
into an unsigned long, this seems too much.

> +	return (u32)divd;
> +}
> +
> +static inline u16 keembay_pwm_config_duty_cycle(struct keembay_pwm *priv,
> +						int duty_ns, u32 ns_min)
> +{
> +	unsigned long long divd;
> +
> +	divd = duty_ns;
> +	do_div(divd, ns_min);
> +	if ((u16)divd == 0)
> +		return 0;
> +
> +	return (u16)divd - 1;

missing range checking.

> +}
> +
> +static inline u16 keembay_pwm_config_period(struct keembay_pwm *priv,
> +					    int period_ns,
> +					    int duty_ns,
> +					    u32 ns_min)
> +{
> +	unsigned long long divd;
> +
> +	divd = period_ns - duty_ns;
> +	do_div(divd, ns_min);
> +	if ((u16)divd == 0)
> +		return 0;
> +
> +	return (u16)divd - 1;

I wonder if both keembay_pwm_config_duty_cycle() and
keembay_pwm_config_period() would be easier to understand if they were not
separate functions but unrolled into their only user.

As above there is no range checking.

> +}
> +
> +/*
> + *	For calculating "high time" register value:
> + *	High time (quotient only) = duty_cycle / ns_min
> + *
> + *	For calculating "low time" register value:
> + *	Low time (quotient only) = (period - duty_cycle) / ns_min
> + *
> + *	All values used are in nanoseconds for calculation.
> + */
> +static int keembay_pwm_config(struct keembay_pwm *priv, int ch,
> +			      int duty_ns, int period_ns, int count)

this function is only called once, I wonder if it can better be folded
into its only user.

> +{
> +	u32 ns_min;
> +	u16 pwm_h_count, pwm_l_count;
> +
> +	/* Write to lead in */
> +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_LEAD_IN_MASK,
> +				LEAD_IN_DEFAULT,
> +				PWM_LEADIN0_OFFSET, ch);

What is the effect of this "lead in"?

> +	/* Write the number of PWM pulse repetition */
> +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_RPT_CNT_MASK, count,
> +				PWM_LEADIN0_OFFSET, ch);

Is this racy? E.g. if the PWM is already running and just after you
update the repetition count completes a period?

This writes to the same register as the lead in above. Can this be done
in a single register access?

> +	/* Calculate min */
> +	ns_min = keembay_pwm_config_min(priv);

What is "min"?

> +	/* For duty cycle */
> +	pwm_h_count = keembay_pwm_config_duty_cycle(priv, duty_ns, ns_min);

this is ineffective and less exact as possible. ns_min is the result of
a division and in keembay_pwm_config_duty_cycle you divide by it.

> +	/* Write to high registers */
> +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_HIGH_MASK, pwm_h_count,
> +				PWM_HIGHLOW0_OFFSET, ch);
> +
> +	/* For period */
> +	pwm_l_count = keembay_pwm_config_period(priv, period_ns, duty_ns,
> +						ns_min);
> +
> +	/* Write to low registers */
> +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_LOW_MASK, pwm_l_count,
> +				PWM_HIGHLOW0_OFFSET, ch);

Can you please explain in a comment what's the resulting wave form for a
given value of this register?

Can writing h_count and l_count be combined in a single register access?
(And if not, what happens if a period completes between the two
updates?)

How does the hardware behave on a change here? Is the currently running
period completed before the new values take effect?

> +
> +	return 0;
> +}
> +
> +static int keembay_pwm_enable(struct keembay_pwm *priv, int ch)
> +{
> +	int ret;
> +
> +	ret = clk_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable channel */
> +	keembay_pwm_enable_channel(priv, ch);
> +
> +	return 0;
> +}
> +
> +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch)
> +{
> +	/* Disable channel */
> +	keembay_pwm_disable_channel(priv, ch);
> +
> +	clk_disable(priv->clk);
> +}
> +
> +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);
> +
> +	if (!state->enabled && pwm_is_enabled(pwm)) {

Please don't call API functions in the driver.

> +		keembay_pwm_disable(priv, pwm->hwpwm);

Is a currently running period completed here? How does the output behave
on disable? (i.e. does it go in its inactive state?)

> +		return 0;
> +	}
> +
> +	if (state->count > PWM_COUNT_MAX)
> +		return -EINVAL;
> +
> +	if (state->polarity != pwm_get_polarity(pwm))

Using:

	if (state->polarity != PWM_POLARITY_NORMAL)

seems both more robust and easier to understand.

> +		return -ENOSYS;
> +
> +	keembay_pwm_config(priv, pwm->hwpwm, state->duty_cycle,
> +			   state->period, state->count);
> +
> +	if (state->enabled && !pwm_is_enabled(pwm))
> +		return keembay_pwm_enable(priv, pwm->hwpwm);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops keembay_pwm_ops = {
> +	.owner = THIS_MODULE,
> +	.apply = keembay_pwm_apply,

Please implement .get_state().

> +};
> +
> +static int keembay_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct keembay_pwm *priv;
> +	int ret;
> +
> +	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))

Error message please. (Something like:

		int ret = PTR_ERR(ret);

		if (ret != -EPROBE_DEFER)
			dev_err(pdev->dev, "Failed to get clk: %pE\n", priv->clk);

		return ret;
)

> +		return PTR_ERR(priv->clk);
> +
> +	/*
> +	 * Prepare clock here, and carry out clock enabling/disabling
> +	 * during channel enablement/disablement.
> +	 * The clock will not be unprepared due to shared usage with GPIO.

What has this clock to do with GPIO? If the GPIO drivers gets and
enables this clock itself there should be no problem.

> +	 */
> +	ret = clk_prepare(priv->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to prepare PWM clock\n");

Please include the error code in the message (preferably using %pE as
above).

> +		return ret;
> +	}
> +
> +	priv->regmap = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	priv->chip.base = -1;
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &keembay_pwm_ops;
> +	priv->chip.npwm = TOTAL_PWM_CHANNELS;
> +
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to add PWM chip: %d\n", ret);

%pE please.

> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static int keembay_pwm_remove(struct platform_device *pdev)
> +{
> +	struct keembay_pwm *priv = platform_get_drvdata(pdev);
> +	unsigned int i;
> +
> +	for (i = 0; i < priv->chip.npwm; i++)
> +		pwm_disable(&priv->chip.pwms[i]);

That's wrong. It's the responsibility of the consumer to disable the
clock.

> +
> +	pwmchip_remove(&priv->chip);

clk_unprepare missing here.

> +
> +	return 0;
> +}
> +
> +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:keembay");
> +MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
> +MODULE_LICENSE("GPL v2");

Best regards
Uwe

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

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

[G Jaya Kumaran, Vineetha]

Hello Uwe,

Thank you for taking the time to review this patch set.

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Sunday, May 24, 2020 5:41 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>; Shevchenko,
> Andriy <andriy.shevchenko@intel.com>
> Subject: Re: [PATCH 2/3] pwm: Add PWM driver for Intel Keem Bay
> 
> Hello,
> 
> On Sun, May 17, 2020 at 09:52:39PM +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.
> >
> > 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 | 308
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 318 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-keembay.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > c13d146..5311975 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -569,4 +569,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
> 
> Support for COMPILE_TEST would be nice here.
> 

I will add this in v2.

> > +	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
> > a59c710..0c84ff2 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -55,3 +55,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 0000000..39c7310
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-keembay.c
> > @@ -0,0 +1,308 @@
> > +// 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>
> 
> Is there publically available documentation? If so, please add a link here.
> 

Will check, and add it here if any are available.

> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define TOTAL_PWM_CHANNELS		6
> > +#define LEAD_IN_DEFAULT			0
> > +#define PWM_COUNT_MAX			65535
> 
> please use a unique prefix for all defines in your driver. The names as they are
> are too generic.
> 

I'll update the naming in v2, to match the mask defines.

> > +
> > +#define KEEMBAY_PWM_EN_BIT		31
> > +
> > +/* Mask */
> > +#define KEEMBAY_PWM_RPT_CNT_MASK	GENMASK(15, 0)
> > +#define KEEMBAY_PWM_LEAD_IN_MASK	GENMASK(30, 16)
> > +#define KEEMBAY_PWM_HIGH_MASK		GENMASK(31, 16)
> > +#define KEEMBAY_PWM_LOW_MASK		GENMASK(15, 0)
> > +
> > +/* PWM Register offset */
> > +#define PWM_LEADIN0_OFFSET		0x00
> > +#define PWM_LEADIN1_OFFSET		0x04
> > +#define PWM_LEADIN2_OFFSET		0x08
> > +#define PWM_LEADIN3_OFFSET		0x0c
> > +#define PWM_LEADIN4_OFFSET		0x10
> > +#define PWM_LEADIN5_OFFSET		0x14
> > +
> > +#define PWM_HIGHLOW0_OFFSET		0x20
> > +#define PWM_HIGHLOW1_OFFSET		0x24
> > +#define PWM_HIGHLOW2_OFFSET		0x28
> > +#define PWM_HIGHLOW3_OFFSET		0x2c
> > +#define PWM_HIGHLOW4_OFFSET		0x30
> > +#define PWM_HIGHLOW5_OFFSET		0x34
> > +
> > +struct keembay_pwm {
> > +	struct pwm_chip chip;
> > +	struct device *dev;
> > +	struct clk *clk;
> > +	void __iomem *regmap;
> 
> I'd call this member "base" instead of "regmap" as the latter has a different
> meaning in the kernel.
> 

OK, will update the naming.

> > +};
> > +
> > +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_enable_channel(struct keembay_pwm
> > +*priv, int ch) {
> > +	u32 buff, offset;
> > +	void __iomem *address;
> > +
> > +	offset = PWM_LEADIN0_OFFSET + ch * 4;
> > +	address = priv->regmap + offset;
> > +	buff = readl(address);
> > +	buff |= BIT(KEEMBAY_PWM_EN_BIT);
> > +	writel(buff, address);
> > +}
> > +
> > +static inline void keembay_pwm_disable_channel(struct keembay_pwm
> > +*priv, int ch) {
> > +	u32 buff, offset;
> > +	void __iomem *address;
> > +
> > +	offset = PWM_LEADIN0_OFFSET + ch * 4;
> > +	address = priv->regmap + offset;
> > +	buff = readl(address);
> > +	buff &= ~BIT(KEEMBAY_PWM_EN_BIT);
> > +	writel(buff, address);
> > +}
> 
> The two functions above could make use of keembay_pwm_update_bits().
> 

Agreed, will update them to reflect this.

> > +
> > +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32
> mask,
> > +					   u32 val, u32 reg, int ch)
> > +{
> > +	u32 buff, offset, tmp;
> > +	void __iomem *address;
> > +
> > +	offset = reg + ch * 4;
> > +	address = priv->regmap + offset;
> > +	buff = readl(address);
> > +	tmp = buff & ~mask;
> > +	tmp |= FIELD_PREP(mask, val);
> > +	writel(tmp, address);
> > +}
> > +
> > +static inline u32 keembay_pwm_config_min(struct keembay_pwm *priv) {
> > +	unsigned long long divd, divs;
> > +
> > +	divd = NSEC_PER_SEC;
> > +	divs = clk_get_rate(priv->clk);
> > +	do_div(divd, divs);
> 
> Given that both NSEC_PER_SEC and the return value of clk_get_rate fit into an
> unsigned long, this seems too much.
> 

Noted, will modify to use a different data type.

> > +	return (u32)divd;
> > +}
> > +
> > +static inline u16 keembay_pwm_config_duty_cycle(struct keembay_pwm
> *priv,
> > +						int duty_ns, u32 ns_min)
> > +{
> > +	unsigned long long divd;
> > +
> > +	divd = duty_ns;
> > +	do_div(divd, ns_min);
> > +	if ((u16)divd == 0)
> > +		return 0;
> > +
> > +	return (u16)divd - 1;
> 
> missing range checking.
> 

Will add this in.

> > +}
> > +
> > +static inline u16 keembay_pwm_config_period(struct keembay_pwm *priv,
> > +					    int period_ns,
> > +					    int duty_ns,
> > +					    u32 ns_min)
> > +{
> > +	unsigned long long divd;
> > +
> > +	divd = period_ns - duty_ns;
> > +	do_div(divd, ns_min);
> > +	if ((u16)divd == 0)
> > +		return 0;
> > +
> > +	return (u16)divd - 1;
> 
> I wonder if both keembay_pwm_config_duty_cycle() and
> keembay_pwm_config_period() would be easier to understand if they were not
> separate functions but unrolled into their only user.
> 
> As above there is no range checking.
> 

OK - will move this into keembay_pwm_apply, and add the range check as well.

> > +}
> > +
> > +/*
> > + *	For calculating "high time" register value:
> > + *	High time (quotient only) = duty_cycle / ns_min
> > + *
> > + *	For calculating "low time" register value:
> > + *	Low time (quotient only) = (period - duty_cycle) / ns_min
> > + *
> > + *	All values used are in nanoseconds for calculation.
> > + */
> > +static int keembay_pwm_config(struct keembay_pwm *priv, int ch,
> > +			      int duty_ns, int period_ns, int count)
> 
> this function is only called once, I wonder if it can better be folded into its only
> user.
> 

Will move this into keembay_pwm_apply.

> > +{
> > +	u32 ns_min;
> > +	u16 pwm_h_count, pwm_l_count;
> > +
> > +	/* Write to lead in */
> > +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_LEAD_IN_MASK,
> > +				LEAD_IN_DEFAULT,
> > +				PWM_LEADIN0_OFFSET, ch);
> 
> What is the effect of this "lead in"?
> 

It is used to specify a low period between the trigger(enable bit set) and start of the waveform.
So the PWM output remains low for the number of clock cycles specified by this LEAD_IN value.

> > +	/* Write the number of PWM pulse repetition */
> > +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_RPT_CNT_MASK,
> count,
> > +				PWM_LEADIN0_OFFSET, ch);
> 
> Is this racy? E.g. if the PWM is already running and just after you update the
> repetition count completes a period?
> 

The repetition count will only take effect once the channel is disabled and reenabled again.
So, it will not affect the currently running waveform.

> This writes to the same register as the lead in above. Can this be done in a single
> register access?
> 

Yes -- will change this to single access instead. 

> > +	/* Calculate min */
> > +	ns_min = keembay_pwm_config_min(priv);
> 
> What is "min"?
> 

This refers to the ns equivalent of 1 clock cycle.
However, since the calculation for the high/low time will be changed in v2 (based on further comments below), 
I will remove this.

> > +	/* For duty cycle */
> > +	pwm_h_count = keembay_pwm_config_duty_cycle(priv, duty_ns,
> ns_min);
> 
> this is ineffective and less exact as possible. ns_min is the result of a division and
> in keembay_pwm_config_duty_cycle you divide by it.
> 

Understood - will change the formula used to get the high time/low time to something like below instead:
value = clk_rate * state->duty_cycle;
pwm_h_count = do_div(value, NSEC_PER_SEC);


> > +	/* Write to high registers */
> > +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_HIGH_MASK,
> pwm_h_count,
> > +				PWM_HIGHLOW0_OFFSET, ch);
> > +
> > +	/* For period */
> > +	pwm_l_count = keembay_pwm_config_period(priv, period_ns, duty_ns,
> > +						ns_min);
> > +
> > +	/* Write to low registers */
> > +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_LOW_MASK,
> pwm_l_count,
> > +				PWM_HIGHLOW0_OFFSET, ch);
> 
> Can you please explain in a comment what's the resulting wave form for a given
> value of this register?
> 

Okay, I will add a comment in the driver explaining this.
In summary, the upper/lower 16 bits of the register represent the waveform's high/low time in number of clock cycles - 1.
e.g. for a period of 70000ns, duty cycle of 35000ns and clock rate of 500MHz:
The resulting register value would be 0x445B445B.

> Can writing h_count and l_count be combined in a single register access?
> (And if not, what happens if a period completes between the two
> updates?)
> 
> How does the hardware behave on a change here? Is the currently running
> period completed before the new values take effect?
> 

The write can be combined, I will make this change for v2. 
As for the HW behaviour, yes, the current period will
be completed before the new configuration takes effect.

> > +
> > +	return 0;
> > +}
> > +
> > +static int keembay_pwm_enable(struct keembay_pwm *priv, int ch) {
> > +	int ret;
> > +
> > +	ret = clk_enable(priv->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Enable channel */
> > +	keembay_pwm_enable_channel(priv, ch);
> > +
> > +	return 0;
> > +}
> > +
> > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) {
> > +	/* Disable channel */
> > +	keembay_pwm_disable_channel(priv, ch);
> > +
> > +	clk_disable(priv->clk);
> > +}
> > +
> > +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);
> > +
> > +	if (!state->enabled && pwm_is_enabled(pwm)) {
> 
> Please don't call API functions in the driver.
> 

Noted, will change this in v2.

> > +		keembay_pwm_disable(priv, pwm->hwpwm);
> 
> Is a currently running period completed here? How does the output behave on
> disable? (i.e. does it go in its inactive state?)
> 

Upon disable (the "enable" bit is cleared), the output signal is stopped/inactive.
The currently running period will not be completed.

> > +		return 0;
> > +	}
> > +
> > +	if (state->count > PWM_COUNT_MAX)
> > +		return -EINVAL;
> > +
> > +	if (state->polarity != pwm_get_polarity(pwm))
> 
> Using:
> 
> 	if (state->polarity != PWM_POLARITY_NORMAL)
> 
> seems both more robust and easier to understand.
> 

Will update this.

> > +		return -ENOSYS;
> > +
> > +	keembay_pwm_config(priv, pwm->hwpwm, state->duty_cycle,
> > +			   state->period, state->count);
> > +
> > +	if (state->enabled && !pwm_is_enabled(pwm))
> > +		return keembay_pwm_enable(priv, pwm->hwpwm);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pwm_ops keembay_pwm_ops = {
> > +	.owner = THIS_MODULE,
> > +	.apply = keembay_pwm_apply,
> 
> Please implement .get_state().
> 

Will add this in v2.

> > +};
> > +
> > +static int keembay_pwm_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct keembay_pwm *priv;
> > +	int ret;
> > +
> > +	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))
> 
> Error message please. (Something like:
> 
> 		int ret = PTR_ERR(ret);
> 
> 		if (ret != -EPROBE_DEFER)
> 			dev_err(pdev->dev, "Failed to get clk: %pE\n", priv-
> >clk);
> 
> 		return ret;
> )
> 

Noted, I will add this in.

> > +		return PTR_ERR(priv->clk);
> > +
> > +	/*
> > +	 * Prepare clock here, and carry out clock enabling/disabling
> > +	 * during channel enablement/disablement.
> > +	 * The clock will not be unprepared due to shared usage with GPIO.
> 
> What has this clock to do with GPIO? If the GPIO drivers gets and enables this
> clock itself there should be no problem.
> 

The PWM outputs are actually part of the GPIO block, hence use the same clock as it.
The current behaviour is unpreparing causes the system to hang - I will cross-check regarding this.

> > +	 */
> > +	ret = clk_prepare(priv->clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to prepare PWM clock\n");
> 
> Please include the error code in the message (preferably using %pE as above).
> 

Noted, will update.

> > +		return ret;
> > +	}
> > +
> > +	priv->regmap = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->regmap))
> > +		return PTR_ERR(priv->regmap);
> > +
> > +	priv->chip.base = -1;
> > +	priv->chip.dev = dev;
> > +	priv->chip.ops = &keembay_pwm_ops;
> > +	priv->chip.npwm = TOTAL_PWM_CHANNELS;
> > +
> > +	ret = pwmchip_add(&priv->chip);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to add PWM chip: %d\n", ret);
> 
> %pE please.
> 

Will update.

> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return 0;
> > +}
> > +
> > +static int keembay_pwm_remove(struct platform_device *pdev) {
> > +	struct keembay_pwm *priv = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < priv->chip.npwm; i++)
> > +		pwm_disable(&priv->chip.pwms[i]);
> 
> That's wrong. It's the responsibility of the consumer to disable the clock.
> 

Noted, will remove this.

> > +
> > +	pwmchip_remove(&priv->chip);
> 
> clk_unprepare missing here.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +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:keembay");
> > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
> MODULE_LICENSE("GPL
> > +v2");
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[G Jaya Kumaran, Vineetha]

Hello Uwe,

Thank you for taking the time to review this patch set.

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Sent: Sunday, May 24, 2020 5:41 AM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
> pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Wan Mohamad, Wan
> Ahmad Zainie <wan.ahmad.zainie.wan.mohamad-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Shevchenko,
> Andriy <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Subject: Re: [PATCH 2/3] pwm: Add PWM driver for Intel Keem Bay
> 
> Hello,
> 
> On Sun, May 17, 2020 at 09:52:39PM +0800,
> vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
> > From: "Lai, Poey Seng" <poey.seng.lai-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> > Enable PWM support for the Intel Keem Bay SoC.
> >
> > Signed-off-by: Lai, Poey Seng <poey.seng.lai-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/pwm/Kconfig       |   9 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-keembay.c | 308
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 318 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-keembay.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > c13d146..5311975 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -569,4 +569,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
> 
> Support for COMPILE_TEST would be nice here.
> 

I will add this in v2.

> > +	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
> > a59c710..0c84ff2 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -55,3 +55,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 0000000..39c7310
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-keembay.c
> > @@ -0,0 +1,308 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel Keem Bay PWM driver
> > + *
> > + * Copyright (C) 2020 Intel Corporation
> > + * Authors: Lai Poey Seng <poey.seng.lai-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > + *          Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran-ral2JQCrhuE@public.gmane.orgm>
> 
> Is there publically available documentation? If so, please add a link here.
> 

Will check, and add it here if any are available.

> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define TOTAL_PWM_CHANNELS		6
> > +#define LEAD_IN_DEFAULT			0
> > +#define PWM_COUNT_MAX			65535
> 
> please use a unique prefix for all defines in your driver. The names as they are
> are too generic.
> 

I'll update the naming in v2, to match the mask defines.

> > +
> > +#define KEEMBAY_PWM_EN_BIT		31
> > +
> > +/* Mask */
> > +#define KEEMBAY_PWM_RPT_CNT_MASK	GENMASK(15, 0)
> > +#define KEEMBAY_PWM_LEAD_IN_MASK	GENMASK(30, 16)
> > +#define KEEMBAY_PWM_HIGH_MASK		GENMASK(31, 16)
> > +#define KEEMBAY_PWM_LOW_MASK		GENMASK(15, 0)
> > +
> > +/* PWM Register offset */
> > +#define PWM_LEADIN0_OFFSET		0x00
> > +#define PWM_LEADIN1_OFFSET		0x04
> > +#define PWM_LEADIN2_OFFSET		0x08
> > +#define PWM_LEADIN3_OFFSET		0x0c
> > +#define PWM_LEADIN4_OFFSET		0x10
> > +#define PWM_LEADIN5_OFFSET		0x14
> > +
> > +#define PWM_HIGHLOW0_OFFSET		0x20
> > +#define PWM_HIGHLOW1_OFFSET		0x24
> > +#define PWM_HIGHLOW2_OFFSET		0x28
> > +#define PWM_HIGHLOW3_OFFSET		0x2c
> > +#define PWM_HIGHLOW4_OFFSET		0x30
> > +#define PWM_HIGHLOW5_OFFSET		0x34
> > +
> > +struct keembay_pwm {
> > +	struct pwm_chip chip;
> > +	struct device *dev;
> > +	struct clk *clk;
> > +	void __iomem *regmap;
> 
> I'd call this member "base" instead of "regmap" as the latter has a different
> meaning in the kernel.
> 

OK, will update the naming.

> > +};
> > +
> > +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_enable_channel(struct keembay_pwm
> > +*priv, int ch) {
> > +	u32 buff, offset;
> > +	void __iomem *address;
> > +
> > +	offset = PWM_LEADIN0_OFFSET + ch * 4;
> > +	address = priv->regmap + offset;
> > +	buff = readl(address);
> > +	buff |= BIT(KEEMBAY_PWM_EN_BIT);
> > +	writel(buff, address);
> > +}
> > +
> > +static inline void keembay_pwm_disable_channel(struct keembay_pwm
> > +*priv, int ch) {
> > +	u32 buff, offset;
> > +	void __iomem *address;
> > +
> > +	offset = PWM_LEADIN0_OFFSET + ch * 4;
> > +	address = priv->regmap + offset;
> > +	buff = readl(address);
> > +	buff &= ~BIT(KEEMBAY_PWM_EN_BIT);
> > +	writel(buff, address);
> > +}
> 
> The two functions above could make use of keembay_pwm_update_bits().
> 

Agreed, will update them to reflect this.

> > +
> > +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32
> mask,
> > +					   u32 val, u32 reg, int ch)
> > +{
> > +	u32 buff, offset, tmp;
> > +	void __iomem *address;
> > +
> > +	offset = reg + ch * 4;
> > +	address = priv->regmap + offset;
> > +	buff = readl(address);
> > +	tmp = buff & ~mask;
> > +	tmp |= FIELD_PREP(mask, val);
> > +	writel(tmp, address);
> > +}
> > +
> > +static inline u32 keembay_pwm_config_min(struct keembay_pwm *priv) {
> > +	unsigned long long divd, divs;
> > +
> > +	divd = NSEC_PER_SEC;
> > +	divs = clk_get_rate(priv->clk);
> > +	do_div(divd, divs);
> 
> Given that both NSEC_PER_SEC and the return value of clk_get_rate fit into an
> unsigned long, this seems too much.
> 

Noted, will modify to use a different data type.

> > +	return (u32)divd;
> > +}
> > +
> > +static inline u16 keembay_pwm_config_duty_cycle(struct keembay_pwm
> *priv,
> > +						int duty_ns, u32 ns_min)
> > +{
> > +	unsigned long long divd;
> > +
> > +	divd = duty_ns;
> > +	do_div(divd, ns_min);
> > +	if ((u16)divd == 0)
> > +		return 0;
> > +
> > +	return (u16)divd - 1;
> 
> missing range checking.
> 

Will add this in.

> > +}
> > +
> > +static inline u16 keembay_pwm_config_period(struct keembay_pwm *priv,
> > +					    int period_ns,
> > +					    int duty_ns,
> > +					    u32 ns_min)
> > +{
> > +	unsigned long long divd;
> > +
> > +	divd = period_ns - duty_ns;
> > +	do_div(divd, ns_min);
> > +	if ((u16)divd == 0)
> > +		return 0;
> > +
> > +	return (u16)divd - 1;
> 
> I wonder if both keembay_pwm_config_duty_cycle() and
> keembay_pwm_config_period() would be easier to understand if they were not
> separate functions but unrolled into their only user.
> 
> As above there is no range checking.
> 

OK - will move this into keembay_pwm_apply, and add the range check as well.

> > +}
> > +
> > +/*
> > + *	For calculating "high time" register value:
> > + *	High time (quotient only) = duty_cycle / ns_min
> > + *
> > + *	For calculating "low time" register value:
> > + *	Low time (quotient only) = (period - duty_cycle) / ns_min
> > + *
> > + *	All values used are in nanoseconds for calculation.
> > + */
> > +static int keembay_pwm_config(struct keembay_pwm *priv, int ch,
> > +			      int duty_ns, int period_ns, int count)
> 
> this function is only called once, I wonder if it can better be folded into its only
> user.
> 

Will move this into keembay_pwm_apply.

> > +{
> > +	u32 ns_min;
> > +	u16 pwm_h_count, pwm_l_count;
> > +
> > +	/* Write to lead in */
> > +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_LEAD_IN_MASK,
> > +				LEAD_IN_DEFAULT,
> > +				PWM_LEADIN0_OFFSET, ch);
> 
> What is the effect of this "lead in"?
> 

It is used to specify a low period between the trigger(enable bit set) and start of the waveform.
So the PWM output remains low for the number of clock cycles specified by this LEAD_IN value.

> > +	/* Write the number of PWM pulse repetition */
> > +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_RPT_CNT_MASK,
> count,
> > +				PWM_LEADIN0_OFFSET, ch);
> 
> Is this racy? E.g. if the PWM is already running and just after you update the
> repetition count completes a period?
> 

The repetition count will only take effect once the channel is disabled and reenabled again.
So, it will not affect the currently running waveform.

> This writes to the same register as the lead in above. Can this be done in a single
> register access?
> 

Yes -- will change this to single access instead. 

> > +	/* Calculate min */
> > +	ns_min = keembay_pwm_config_min(priv);
> 
> What is "min"?
> 

This refers to the ns equivalent of 1 clock cycle.
However, since the calculation for the high/low time will be changed in v2 (based on further comments below), 
I will remove this.

> > +	/* For duty cycle */
> > +	pwm_h_count = keembay_pwm_config_duty_cycle(priv, duty_ns,
> ns_min);
> 
> this is ineffective and less exact as possible. ns_min is the result of a division and
> in keembay_pwm_config_duty_cycle you divide by it.
> 

Understood - will change the formula used to get the high time/low time to something like below instead:
value = clk_rate * state->duty_cycle;
pwm_h_count = do_div(value, NSEC_PER_SEC);


> > +	/* Write to high registers */
> > +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_HIGH_MASK,
> pwm_h_count,
> > +				PWM_HIGHLOW0_OFFSET, ch);
> > +
> > +	/* For period */
> > +	pwm_l_count = keembay_pwm_config_period(priv, period_ns, duty_ns,
> > +						ns_min);
> > +
> > +	/* Write to low registers */
> > +	keembay_pwm_update_bits(priv, KEEMBAY_PWM_LOW_MASK,
> pwm_l_count,
> > +				PWM_HIGHLOW0_OFFSET, ch);
> 
> Can you please explain in a comment what's the resulting wave form for a given
> value of this register?
> 

Okay, I will add a comment in the driver explaining this.
In summary, the upper/lower 16 bits of the register represent the waveform's high/low time in number of clock cycles - 1.
e.g. for a period of 70000ns, duty cycle of 35000ns and clock rate of 500MHz:
The resulting register value would be 0x445B445B.

> Can writing h_count and l_count be combined in a single register access?
> (And if not, what happens if a period completes between the two
> updates?)
> 
> How does the hardware behave on a change here? Is the currently running
> period completed before the new values take effect?
> 

The write can be combined, I will make this change for v2. 
As for the HW behaviour, yes, the current period will
be completed before the new configuration takes effect.

> > +
> > +	return 0;
> > +}
> > +
> > +static int keembay_pwm_enable(struct keembay_pwm *priv, int ch) {
> > +	int ret;
> > +
> > +	ret = clk_enable(priv->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Enable channel */
> > +	keembay_pwm_enable_channel(priv, ch);
> > +
> > +	return 0;
> > +}
> > +
> > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) {
> > +	/* Disable channel */
> > +	keembay_pwm_disable_channel(priv, ch);
> > +
> > +	clk_disable(priv->clk);
> > +}
> > +
> > +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);
> > +
> > +	if (!state->enabled && pwm_is_enabled(pwm)) {
> 
> Please don't call API functions in the driver.
> 

Noted, will change this in v2.

> > +		keembay_pwm_disable(priv, pwm->hwpwm);
> 
> Is a currently running period completed here? How does the output behave on
> disable? (i.e. does it go in its inactive state?)
> 

Upon disable (the "enable" bit is cleared), the output signal is stopped/inactive.
The currently running period will not be completed.

> > +		return 0;
> > +	}
> > +
> > +	if (state->count > PWM_COUNT_MAX)
> > +		return -EINVAL;
> > +
> > +	if (state->polarity != pwm_get_polarity(pwm))
> 
> Using:
> 
> 	if (state->polarity != PWM_POLARITY_NORMAL)
> 
> seems both more robust and easier to understand.
> 

Will update this.

> > +		return -ENOSYS;
> > +
> > +	keembay_pwm_config(priv, pwm->hwpwm, state->duty_cycle,
> > +			   state->period, state->count);
> > +
> > +	if (state->enabled && !pwm_is_enabled(pwm))
> > +		return keembay_pwm_enable(priv, pwm->hwpwm);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pwm_ops keembay_pwm_ops = {
> > +	.owner = THIS_MODULE,
> > +	.apply = keembay_pwm_apply,
> 
> Please implement .get_state().
> 

Will add this in v2.

> > +};
> > +
> > +static int keembay_pwm_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct keembay_pwm *priv;
> > +	int ret;
> > +
> > +	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))
> 
> Error message please. (Something like:
> 
> 		int ret = PTR_ERR(ret);
> 
> 		if (ret != -EPROBE_DEFER)
> 			dev_err(pdev->dev, "Failed to get clk: %pE\n", priv-
> >clk);
> 
> 		return ret;
> )
> 

Noted, I will add this in.

> > +		return PTR_ERR(priv->clk);
> > +
> > +	/*
> > +	 * Prepare clock here, and carry out clock enabling/disabling
> > +	 * during channel enablement/disablement.
> > +	 * The clock will not be unprepared due to shared usage with GPIO.
> 
> What has this clock to do with GPIO? If the GPIO drivers gets and enables this
> clock itself there should be no problem.
> 

The PWM outputs are actually part of the GPIO block, hence use the same clock as it.
The current behaviour is unpreparing causes the system to hang - I will cross-check regarding this.

> > +	 */
> > +	ret = clk_prepare(priv->clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to prepare PWM clock\n");
> 
> Please include the error code in the message (preferably using %pE as above).
> 

Noted, will update.

> > +		return ret;
> > +	}
> > +
> > +	priv->regmap = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->regmap))
> > +		return PTR_ERR(priv->regmap);
> > +
> > +	priv->chip.base = -1;
> > +	priv->chip.dev = dev;
> > +	priv->chip.ops = &keembay_pwm_ops;
> > +	priv->chip.npwm = TOTAL_PWM_CHANNELS;
> > +
> > +	ret = pwmchip_add(&priv->chip);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to add PWM chip: %d\n", ret);
> 
> %pE please.
> 

Will update.

> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return 0;
> > +}
> > +
> > +static int keembay_pwm_remove(struct platform_device *pdev) {
> > +	struct keembay_pwm *priv = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < priv->chip.npwm; i++)
> > +		pwm_disable(&priv->chip.pwms[i]);
> 
> That's wrong. It's the responsibility of the consumer to disable the clock.
> 

Noted, will remove this.

> > +
> > +	pwmchip_remove(&priv->chip);
> 
> clk_unprepare missing here.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +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:keembay");
> > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
> MODULE_LICENSE("GPL
> > +v2");
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH 3/3] 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.

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

diff --git a/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml b/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
new file mode 100644
index 0000000..00968d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2020 Intel Corporation
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-keembay.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:
+    description:
+        phandle to the reference clock.
+
+required:
+ - compatible
+ - reg
+ - clocks
+
+examples:
+  - |
+    pwm@203200a0 {
+      compatible = "intel,keembay-pwm";
+      reg = <0x0 0x203200a0 0x0 0xe8>;
+      clocks = <&scmi_clk KEEM_BAY_A53_GPIO>;
+    };
-- 
1.9.1

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

[Rob Herring]

On Sun, 17 May 2020 21:52:40 +0800,  wrote:
> From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com>
> 
> Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> 
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> ---
>  .../devicetree/bindings/pwm/pwm-keembay.yaml       | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

Error: Documentation/devicetree/bindings/pwm/pwm-keembay.example.dts:22.31-32 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:312: recipe for target 'Documentation/devicetree/bindings/pwm/pwm-keembay.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/pwm/pwm-keembay.example.dt.yaml] Error 1
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

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

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

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

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

[Rob Herring]

On Sun, 17 May 2020 21:52:40 +0800,  wrote:
> From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> 
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/pwm/pwm-keembay.yaml       | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

Error: Documentation/devicetree/bindings/pwm/pwm-keembay.example.dts:22.31-32 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:312: recipe for target 'Documentation/devicetree/bindings/pwm/pwm-keembay.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/pwm/pwm-keembay.example.dt.yaml] Error 1
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

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

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

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

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

[G Jaya Kumaran, Vineetha]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Monday, May 18, 2020 10:19 PM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com>
> Cc: devicetree@vger.kernel.org; u.kleine-koenig@pengutronix.de; Wan
> Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>;
> robh+dt@kernel.org; linux-pwm@vger.kernel.org; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; thierry.reding@gmail.com
> Subject: Re: [PATCH 3/3] dt-bindings: pwm: keembay: Add bindings for Intel
> Keem Bay PWM
> 
> On Sun, 17 May 2020 21:52:40 +0800,  wrote:
> > From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com>
> >
> > Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> >
> > Signed-off-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@intel.com>
> > ---
> >  .../devicetree/bindings/pwm/pwm-keembay.yaml       | 39
> ++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> >
> 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> Error: Documentation/devicetree/bindings/pwm/pwm-
> keembay.example.dts:22.31-32 syntax error FATAL ERROR: Unable to parse
> input tree
> scripts/Makefile.lib:312: recipe for target
> 'Documentation/devicetree/bindings/pwm/pwm-keembay.example.dt.yaml'
> failed
> make[1]: *** [Documentation/devicetree/bindings/pwm/pwm-
> keembay.example.dt.yaml] Error 1
> Makefile:1300: recipe for target 'dt_binding_check' failed
> make: *** [dt_binding_check] Error 2
> 
> See https://patchwork.ozlabs.org/patch/1292157
> 
> If you already ran 'make dt_binding_check' and didn't see the above error(s),
> then make sure dt-schema is up to date:
> 
> pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --
> upgrade
> 
> Please check and re-submit.

Thank you for reviewing. Will check this again and make sure it passes before submitting v2.

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

[G Jaya Kumaran, Vineetha]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Monday, May 18, 2020 10:19 PM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com>
> Cc: devicetree@vger.kernel.org; u.kleine-koenig@pengutronix.de; Wan
> Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>;
> robh+dt@kernel.org; linux-pwm@vger.kernel.org; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; thierry.reding@gmail.com
> Subject: Re: [PATCH 3/3] dt-bindings: pwm: keembay: Add bindings for Intel
> Keem Bay PWM
> 
> On Sun, 17 May 2020 21:52:40 +0800,  wrote:
> > From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com>
> >
> > Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> >
> > Signed-off-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@intel.com>
> > ---
> >  .../devicetree/bindings/pwm/pwm-keembay.yaml       | 39
> ++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> >
> 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> Error: Documentation/devicetree/bindings/pwm/pwm-
> keembay.example.dts:22.31-32 syntax error FATAL ERROR: Unable to parse
> input tree
> scripts/Makefile.lib:312: recipe for target
> 'Documentation/devicetree/bindings/pwm/pwm-keembay.example.dt.yaml'
> failed
> make[1]: *** [Documentation/devicetree/bindings/pwm/pwm-
> keembay.example.dt.yaml] Error 1
> Makefile:1300: recipe for target 'dt_binding_check' failed
> make: *** [dt_binding_check] Error 2
> 
> See https://patchwork.ozlabs.org/patch/1292157
> 
> If you already ran 'make dt_binding_check' and didn't see the above error(s),
> then make sure dt-schema is up to date:
> 
> pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --
> upgrade
> 
> Please check and re-submit.

Thank you for reviewing. Will check this again and make sure it passes before submitting v2.

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

[Rob Herring]

On Sun, May 17, 2020 at 09:52:40PM +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.
> 
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> ---
>  .../devicetree/bindings/pwm/pwm-keembay.yaml       | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-keembay.yaml

Use compatible string for filename: intel,keembay-pwn.yaml

> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml b/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> new file mode 100644
> index 0000000..00968d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings:

(GPL-2.0-only OR BSD-2-Clause)

> +# Copyright (C) 2020 Intel Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/pwm-keembay.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:
> +    description:
> +        phandle to the reference clock.

How many clocks? (maxItems: 1?)

You can drop the description.

> +
> +required:
> + - compatible
> + - reg
> + - clocks
> +
> +examples:
> +  - |
> +    pwm@203200a0 {
> +      compatible = "intel,keembay-pwm";
> +      reg = <0x0 0x203200a0 0x0 0xe8>;
> +      clocks = <&scmi_clk KEEM_BAY_A53_GPIO>;
> +    };
> -- 
> 1.9.1
> 

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

[G Jaya Kumaran, Vineetha]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Monday, May 18, 2020 10:22 PM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com>
> Cc: thierry.reding@gmail.com; u.kleine-koenig@pengutronix.de; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan
> Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Shevchenko,
> Andriy <andriy.shevchenko@intel.com>
> Subject: Re: [PATCH 3/3] dt-bindings: pwm: keembay: Add bindings for Intel
> Keem Bay PWM
> 
> On Sun, May 17, 2020 at 09:52:40PM +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.
> >
> > Signed-off-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@intel.com>
> > ---
> >  .../devicetree/bindings/pwm/pwm-keembay.yaml       | 39
> ++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> 
> Use compatible string for filename: intel,keembay-pwn.yaml
> 

Will fix the filename in v2.

> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> > b/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> > new file mode 100644
> > index 0000000..00968d7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> > @@ -0,0 +1,39 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Dual license new bindings:
> 
> (GPL-2.0-only OR BSD-2-Clause)
> 

OK, will update the licensing info.

> > +# Copyright (C) 2020 Intel Corporation %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pwm/pwm-keembay.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:
> > +    description:
> > +        phandle to the reference clock.
> 
> How many clocks? (maxItems: 1?)
> 
> You can drop the description.
> 

1 clock is needed for this case, will add in maxItems and drop the description.

> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > +
> > +examples:
> > +  - |
> > +    pwm@203200a0 {
> > +      compatible = "intel,keembay-pwm";
> > +      reg = <0x0 0x203200a0 0x0 0xe8>;
> > +      clocks = <&scmi_clk KEEM_BAY_A53_GPIO>;
> > +    };
> > --
> > 1.9.1
> >

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

[G Jaya Kumaran, Vineetha]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Monday, May 18, 2020 10:22 PM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com>
> Cc: thierry.reding@gmail.com; u.kleine-koenig@pengutronix.de; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan
> Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Shevchenko,
> Andriy <andriy.shevchenko@intel.com>
> Subject: Re: [PATCH 3/3] dt-bindings: pwm: keembay: Add bindings for Intel
> Keem Bay PWM
> 
> On Sun, May 17, 2020 at 09:52:40PM +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.
> >
> > Signed-off-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@intel.com>
> > ---
> >  .../devicetree/bindings/pwm/pwm-keembay.yaml       | 39
> ++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> 
> Use compatible string for filename: intel,keembay-pwn.yaml
> 

Will fix the filename in v2.

> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> > b/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> > new file mode 100644
> > index 0000000..00968d7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-keembay.yaml
> > @@ -0,0 +1,39 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Dual license new bindings:
> 
> (GPL-2.0-only OR BSD-2-Clause)
> 

OK, will update the licensing info.

> > +# Copyright (C) 2020 Intel Corporation %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pwm/pwm-keembay.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:
> > +    description:
> > +        phandle to the reference clock.
> 
> How many clocks? (maxItems: 1?)
> 
> You can drop the description.
> 

1 clock is needed for this case, will add in maxItems and drop the description.

> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > +
> > +examples:
> > +  - |
> > +    pwm@203200a0 {
> > +      compatible = "intel,keembay-pwm";
> > +      reg = <0x0 0x203200a0 0x0 0xe8>;
> > +      clocks = <&scmi_clk KEEM_BAY_A53_GPIO>;
> > +    };
> > --
> > 1.9.1
> >