[PATCH v2 0/2] Add ZTE ZX PWM device driver support

[PATCH v2 0/2] Add ZTE ZX PWM device driver support

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

The series adds the dt-bindings document and device driver for PWM
controller found on ZTE ZX family SoCs.

Changes for v2:
 - Add Rob's ACK for dt-bindings patch
 - Use inline function rather than macro for to_zx_pwm_chip()
 - Convert zx_pwm_ops to implement atomic callbacks .apply() and .get_state()

Shawn Guo (2):
  dt-bindings: pwm: add bindings doc for ZTE ZX PWM controller
  pwm: add ZTE ZX PWM device driver

 Documentation/devicetree/bindings/pwm/pwm-zx.txt |  22 ++
 drivers/pwm/Kconfig                              |   9 +
 drivers/pwm/Makefile                             |   1 +
 drivers/pwm/pwm-zx.c                             | 270 +++++++++++++++++++++++
 4 files changed, 302 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-zx.txt
 create mode 100644 drivers/pwm/pwm-zx.c

-- 
1.9.1

[PATCH v2 1/2] dt-bindings: pwm: add bindings doc for ZTE ZX PWM controller

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

It adds bindings document for ZTE ZX PWM controller.  The device
has two clocks: PCLK and WCLK.  The PCLK is for register access, and
WCLK is the reference clock for calculating period and duty cycles.
Also, the device supports polarity configuration, so #pwm-cells should
be 3.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pwm/pwm-zx.txt | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-zx.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-zx.txt b/Documentation/devicetree/bindings/pwm/pwm-zx.txt
new file mode 100644
index 000000000000..a6bcc75c9164
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-zx.txt
@@ -0,0 +1,22 @@
+ZTE ZX PWM controller
+
+Required properties:
+ - compatible: Should be "zte,zx296718-pwm".
+ - reg: Physical base address and length of the controller's registers.
+ - clocks : The phandle and specifier referencing the controller's clocks.
+ - clock-names: "pclk" for PCLK, "wclk" for WCLK to the PWM controller.  The
+   PCLK is for register access, while WCLK is the reference clock for
+   calculating period and duty cycles.
+ - #pwm-cells: Should be 3. See pwm.txt in this directory for a description of
+   the cells format.
+
+Example:
+
+	pwm: pwm at 1439000 {
+		compatible = "zte,zx296718-pwm";
+		reg = <0x1439000 0x1000>;
+		clocks = <&lsp1crm LSP1_PWM_PCLK>,
+			 <&lsp1crm LSP1_PWM_WCLK>;
+		clock-names = "pclk", "wclk";
+		#pwm-cells = <3>;
+	};
-- 
1.9.1

[PATCH v2 1/2] dt-bindings: pwm: add bindings doc for ZTE ZX PWM controller

[Thierry Reding]

On Thu, Jul 27, 2017 at 04:23:36PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> It adds bindings document for ZTE ZX PWM controller.  The device
> has two clocks: PCLK and WCLK.  The PCLK is for register access, and
> WCLK is the reference clock for calculating period and duty cycles.
> Also, the device supports polarity configuration, so #pwm-cells should
> be 3.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-zx.txt | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-zx.txt

Applied to for-4.14/drivers, thanks.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/5095ab28/attachment.sig>

[PATCH v2 2/2] pwm: add ZTE ZX PWM device driver

[Shawn Guo]

From: Shawn Guo <shawn.guo@linaro.org>

It adds PWM device driver for ZTE ZX family SoCs.  The PWM controller
supports 4 devices with polarity configuration.

The driver has been tested with pwm-regulator support to scale core
voltage via cpufreq.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pwm/Kconfig  |   9 ++
 drivers/pwm/Makefile |   1 +
 drivers/pwm/pwm-zx.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+)
 create mode 100644 drivers/pwm/pwm-zx.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 313c10789ca2..e98175331a69 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -500,4 +500,13 @@ config PWM_VT8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-vt8500.
 
+config PWM_ZX
+	tristate "ZTE ZX PWM support"
+	depends on ARCH_ZX
+	help
+	  Generic PWM framework driver for ZTE ZX family SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-zx.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 93da1f79a3b8..75ab74094d7b 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -49,3 +49,4 @@ obj-$(CONFIG_PWM_TIPWMSS)	+= pwm-tipwmss.o
 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
diff --git a/drivers/pwm/pwm-zx.c b/drivers/pwm/pwm-zx.c
new file mode 100644
index 000000000000..39d5e6bf0510
--- /dev/null
+++ b/drivers/pwm/pwm-zx.c
@@ -0,0 +1,270 @@
+/*
+ * Copyright (C) 2017 Sanechips Technology Co., Ltd.
+ * Copyright 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+#define ZX_PWM_MODE		0x0
+#define ZX_PWM_CLKDIV_SHIFT	2
+#define ZX_PWM_CLKDIV_MASK	GENMASK(11, 2)
+#define ZX_PWM_CLKDIV(x)	(((x) << ZX_PWM_CLKDIV_SHIFT) & \
+					 ZX_PWM_CLKDIV_MASK)
+#define ZX_PWM_POLAR		BIT(1)
+#define ZX_PWM_EN		BIT(0)
+#define ZX_PWM_PERIOD		0x4
+#define ZX_PWM_DUTY		0x8
+
+#define ZX_PWM_NUM		4
+#define ZX_PWM_CLKDIV_MAX	1023
+#define ZX_PWM_PERIOD_MAX	65535
+
+struct zx_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *pclk;
+	struct clk *wclk;
+	void __iomem *base;
+};
+
+static inline struct zx_pwm_chip *to_zx_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct zx_pwm_chip, chip);
+}
+
+static inline u32 zx_pwm_readl(struct zx_pwm_chip *zpc, unsigned int hwpwm,
+			       unsigned int offset)
+{
+	return readl(zpc->base + (hwpwm + 1) * 0x10 + offset);
+}
+
+static inline void zx_pwm_writel(struct zx_pwm_chip *zpc, unsigned int hwpwm,
+				 unsigned int offset, u32 val)
+{
+	writel(val, zpc->base + (hwpwm + 1) * 0x10 + offset);
+}
+
+static void zx_pwm_set_mask(struct zx_pwm_chip *zpc, unsigned int hwpwm,
+			    unsigned int offset, u32 mask, u32 value)
+{
+	u32 data;
+
+	data = zx_pwm_readl(zpc, hwpwm, offset);
+	data &= ~mask;
+	data |= value & mask;
+	zx_pwm_writel(zpc, hwpwm, offset, data);
+}
+
+static void zx_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			     struct pwm_state *state)
+{
+	struct zx_pwm_chip *zpc = to_zx_pwm_chip(chip);
+	unsigned long rate;
+	int div;
+	u64 tmp;
+	u32 val;
+
+	val = zx_pwm_readl(zpc, pwm->hwpwm, ZX_PWM_MODE);
+	state->polarity = (val & ZX_PWM_POLAR) ? PWM_POLARITY_NORMAL :
+						 PWM_POLARITY_INVERSED;
+	state->enabled = (val & ZX_PWM_EN) ? true : false;
+
+	div = (val & ZX_PWM_CLKDIV_MASK) >> ZX_PWM_CLKDIV_SHIFT;
+	rate = clk_get_rate(zpc->wclk);
+
+	tmp = zx_pwm_readl(zpc, pwm->hwpwm, ZX_PWM_PERIOD);
+	tmp *= div * NSEC_PER_SEC;
+	state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
+
+	tmp = zx_pwm_readl(zpc, pwm->hwpwm, ZX_PWM_DUTY);
+	tmp *= div * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
+}
+
+static int zx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			 unsigned int duty_ns, unsigned int period_ns)
+{
+	struct zx_pwm_chip *zpc = to_zx_pwm_chip(chip);
+	unsigned int period_cycles;
+	unsigned int duty_cycles;
+	unsigned long long c;
+	unsigned long rate;
+	int div = 1;
+
+	/* Find out the best divider */
+	rate = clk_get_rate(zpc->wclk);
+	while (1) {
+		c = rate / div;
+		c = c * period_ns;
+		do_div(c, NSEC_PER_SEC);
+		if (c < ZX_PWM_PERIOD_MAX)
+			break;
+		div++;
+		if (div > ZX_PWM_CLKDIV_MAX)
+			return -ERANGE;
+	}
+
+	/* Calculate duty cycles */
+	period_cycles = c;
+	c *= duty_ns;
+	do_div(c, period_ns);
+	duty_cycles = c;
+
+	/*
+	 * If the pwm is being enabled, we have to temporarily disable it
+	 * before configuring the registers.
+	 */
+	if (pwm_is_enabled(pwm))
+		zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE, ZX_PWM_EN, 0);
+
+	/* Set up registers */
+	zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE, ZX_PWM_CLKDIV_MASK,
+			ZX_PWM_CLKDIV(div));
+	zx_pwm_writel(zpc, pwm->hwpwm, ZX_PWM_PERIOD, period_cycles);
+	zx_pwm_writel(zpc, pwm->hwpwm, ZX_PWM_DUTY, duty_cycles);
+
+	/* Re-enable the pwm if needed */
+	if (pwm_is_enabled(pwm))
+		zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE,
+				ZX_PWM_EN, ZX_PWM_EN);
+
+	return 0;
+}
+
+static int zx_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			struct pwm_state *state)
+{
+	struct zx_pwm_chip *zpc = to_zx_pwm_chip(chip);
+	struct pwm_state cstate;
+	int ret;
+
+	pwm_get_state(pwm, &cstate);
+
+	if (state->polarity != cstate.polarity)
+		zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE, ZX_PWM_POLAR,
+				(state->polarity == PWM_POLARITY_INVERSED) ?
+				 0 : ZX_PWM_POLAR);
+
+	if (state->period != cstate.period ||
+	    state->duty_cycle != cstate.duty_cycle) {
+		ret = zx_pwm_config(chip, pwm, state->duty_cycle,
+				    state->period);
+		if (ret)
+			return ret;
+	}
+
+	if (state->enabled != cstate.enabled) {
+		if (state->enabled) {
+			ret = clk_prepare_enable(zpc->wclk);
+			if (ret)
+				return ret;
+			zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE,
+					ZX_PWM_EN, ZX_PWM_EN);
+		} else {
+			zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE,
+					ZX_PWM_EN, 0);
+			clk_disable_unprepare(zpc->wclk);
+		}
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops zx_pwm_ops = {
+	.apply = zx_pwm_apply,
+	.get_state = zx_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int zx_pwm_probe(struct platform_device *pdev)
+{
+	struct zx_pwm_chip *zpc;
+	struct resource *res;
+	int ret;
+	int i;
+
+	zpc = devm_kzalloc(&pdev->dev, sizeof(*zpc), GFP_KERNEL);
+	if (!zpc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	zpc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(zpc->base))
+		return PTR_ERR(zpc->base);
+
+	zpc->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(zpc->pclk))
+		return PTR_ERR(zpc->pclk);
+
+	zpc->wclk = devm_clk_get(&pdev->dev, "wclk");
+	if (IS_ERR(zpc->wclk))
+		return PTR_ERR(zpc->wclk);
+
+	ret = clk_prepare_enable(zpc->pclk);
+	if (ret)
+		return ret;
+
+	/*
+	 * PWM devices may be enabled by firmware, and let's disable all of
+	 * them initially to save power.
+	 */
+	for (i = 0; i < ZX_PWM_NUM; i++)
+		zx_pwm_set_mask(zpc, i, ZX_PWM_MODE, ZX_PWM_EN, 0);
+
+	zpc->chip.dev = &pdev->dev;
+	zpc->chip.ops = &zx_pwm_ops;
+	zpc->chip.base = -1;
+	zpc->chip.npwm = ZX_PWM_NUM;
+	zpc->chip.of_xlate = of_pwm_xlate_with_flags;
+	zpc->chip.of_pwm_n_cells = 3;
+
+	ret = pwmchip_add(&zpc->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, zpc);
+
+	return 0;
+}
+
+static int zx_pwm_remove(struct platform_device *pdev)
+{
+	struct zx_pwm_chip *zpc = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(zpc->pclk);
+
+	return pwmchip_remove(&zpc->chip);
+}
+
+static const struct of_device_id zx_pwm_dt_ids[] = {
+	{ .compatible = "zte,zx296718-pwm", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, zx_pwm_dt_ids);
+
+static struct platform_driver zx_pwm_driver = {
+	.driver = {
+		.name = "zx-pwm",
+		.of_match_table = zx_pwm_dt_ids,
+	},
+	.probe = zx_pwm_probe,
+	.remove = zx_pwm_remove,
+};
+module_platform_driver(zx_pwm_driver);
+
+MODULE_ALIAS("platform:zx-pwm");
+MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
+MODULE_DESCRIPTION("ZTE ZX PWM Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v2 2/2] pwm: add ZTE ZX PWM device driver

[Thierry Reding]

On Thu, Jul 27, 2017 at 04:23:37PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> It adds PWM device driver for ZTE ZX family SoCs.  The PWM controller
> supports 4 devices with polarity configuration.
> 
> The driver has been tested with pwm-regulator support to scale core
> voltage via cpufreq.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/pwm/Kconfig  |   9 ++
>  drivers/pwm/Makefile |   1 +
>  drivers/pwm/pwm-zx.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 280 insertions(+)
>  create mode 100644 drivers/pwm/pwm-zx.c

I've applied this, with some bikeshedding applied. See below for the
changes I made.

I should say, though, that this is pretty much perfect and the below
changes are just because I'm having an extra nit-picky day.

> diff --git a/drivers/pwm/pwm-zx.c b/drivers/pwm/pwm-zx.c
> new file mode 100644
> index 000000000000..39d5e6bf0510
> --- /dev/null
> +++ b/drivers/pwm/pwm-zx.c
> @@ -0,0 +1,270 @@
> +/*
> + * Copyright (C) 2017 Sanechips Technology Co., Ltd.
> + * Copyright 2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +#define ZX_PWM_MODE		0x0
> +#define ZX_PWM_CLKDIV_SHIFT	2
> +#define ZX_PWM_CLKDIV_MASK	GENMASK(11, 2)
> +#define ZX_PWM_CLKDIV(x)	(((x) << ZX_PWM_CLKDIV_SHIFT) & \
> +					 ZX_PWM_CLKDIV_MASK)
> +#define ZX_PWM_POLAR		BIT(1)
> +#define ZX_PWM_EN		BIT(0)
> +#define ZX_PWM_PERIOD		0x4
> +#define ZX_PWM_DUTY		0x8
> +
> +#define ZX_PWM_NUM		4
> +#define ZX_PWM_CLKDIV_MAX	1023
> +#define ZX_PWM_PERIOD_MAX	65535
> +
> +struct zx_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *pclk;
> +	struct clk *wclk;
> +	void __iomem *base;
> +};
> +
> +static inline struct zx_pwm_chip *to_zx_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct zx_pwm_chip, chip);
> +}
> +
> +static inline u32 zx_pwm_readl(struct zx_pwm_chip *zpc, unsigned int hwpwm,
> +			       unsigned int offset)
> +{
> +	return readl(zpc->base + (hwpwm + 1) * 0x10 + offset);
> +}
> +
> +static inline void zx_pwm_writel(struct zx_pwm_chip *zpc, unsigned int hwpwm,
> +				 unsigned int offset, u32 val)
> +{
> +	writel(val, zpc->base + (hwpwm + 1) * 0x10 + offset);
> +}
> +
> +static void zx_pwm_set_mask(struct zx_pwm_chip *zpc, unsigned int hwpwm,
> +			    unsigned int offset, u32 mask, u32 value)
> +{
> +	u32 data;
> +
> +	data = zx_pwm_readl(zpc, hwpwm, offset);
> +	data &= ~mask;
> +	data |= value & mask;
> +	zx_pwm_writel(zpc, hwpwm, offset, data);
> +}
> +
> +static void zx_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     struct pwm_state *state)
> +{
> +	struct zx_pwm_chip *zpc = to_zx_pwm_chip(chip);
> +	unsigned long rate;
> +	int div;

I made this unsigned because it can't ever be negative.

> +	u64 tmp;
> +	u32 val;
> +
> +	val = zx_pwm_readl(zpc, pwm->hwpwm, ZX_PWM_MODE);
> +	state->polarity = (val & ZX_PWM_POLAR) ? PWM_POLARITY_NORMAL :
> +						 PWM_POLARITY_INVERSED;
> +	state->enabled = (val & ZX_PWM_EN) ? true : false;

I turned the above into:

	if (...)
		state->... = ...;
	else
		state->... = ...;

because I think that makes it more readable.

> +
> +	div = (val & ZX_PWM_CLKDIV_MASK) >> ZX_PWM_CLKDIV_SHIFT;
> +	rate = clk_get_rate(zpc->wclk);
> +
> +	tmp = zx_pwm_readl(zpc, pwm->hwpwm, ZX_PWM_PERIOD);
> +	tmp *= div * NSEC_PER_SEC;
> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> +	tmp = zx_pwm_readl(zpc, pwm->hwpwm, ZX_PWM_DUTY);
> +	tmp *= div * NSEC_PER_SEC;
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +}
> +
> +static int zx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 unsigned int duty_ns, unsigned int period_ns)
> +{
> +	struct zx_pwm_chip *zpc = to_zx_pwm_chip(chip);
> +	unsigned int period_cycles;
> +	unsigned int duty_cycles;

Collapsed the above two lines.

> +	unsigned long long c;
> +	unsigned long rate;
> +	int div = 1;

Made this unsigned because, again, it can't ever be negative.

> +
> +	/* Find out the best divider */
> +	rate = clk_get_rate(zpc->wclk);
> +	while (1) {
> +		c = rate / div;
> +		c = c * period_ns;
> +		do_div(c, NSEC_PER_SEC);
> +		if (c < ZX_PWM_PERIOD_MAX)
> +			break;
> +		div++;
> +		if (div > ZX_PWM_CLKDIV_MAX)
> +			return -ERANGE;

Added some blank lines in the above for readability.

> +	}
> +
> +	/* Calculate duty cycles */
> +	period_cycles = c;
> +	c *= duty_ns;
> +	do_div(c, period_ns);
> +	duty_cycles = c;
> +
> +	/*
> +	 * If the pwm is being enabled, we have to temporarily disable it
> +	 * before configuring the registers.
> +	 */

pwm -> PWM in prose because it is an abbreviation.

> +	if (pwm_is_enabled(pwm))
> +		zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE, ZX_PWM_EN, 0);
> +
> +	/* Set up registers */
> +	zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE, ZX_PWM_CLKDIV_MASK,
> +			ZX_PWM_CLKDIV(div));
> +	zx_pwm_writel(zpc, pwm->hwpwm, ZX_PWM_PERIOD, period_cycles);
> +	zx_pwm_writel(zpc, pwm->hwpwm, ZX_PWM_DUTY, duty_cycles);
> +
> +	/* Re-enable the pwm if needed */

Ditto.

> +	if (pwm_is_enabled(pwm))
> +		zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE,
> +				ZX_PWM_EN, ZX_PWM_EN);
> +
> +	return 0;
> +}
> +
> +static int zx_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			struct pwm_state *state)
> +{
> +	struct zx_pwm_chip *zpc = to_zx_pwm_chip(chip);
> +	struct pwm_state cstate;
> +	int ret;
> +
> +	pwm_get_state(pwm, &cstate);
> +
> +	if (state->polarity != cstate.polarity)
> +		zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE, ZX_PWM_POLAR,
> +				(state->polarity == PWM_POLARITY_INVERSED) ?
> +				 0 : ZX_PWM_POLAR);
> +
> +	if (state->period != cstate.period ||
> +	    state->duty_cycle != cstate.duty_cycle) {
> +		ret = zx_pwm_config(chip, pwm, state->duty_cycle,
> +				    state->period);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (state->enabled != cstate.enabled) {
> +		if (state->enabled) {
> +			ret = clk_prepare_enable(zpc->wclk);
> +			if (ret)
> +				return ret;
> +			zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE,

Added a blank line between the above two for readability.

> +					ZX_PWM_EN, ZX_PWM_EN);
> +		} else {
> +			zx_pwm_set_mask(zpc, pwm->hwpwm, ZX_PWM_MODE,
> +					ZX_PWM_EN, 0);
> +			clk_disable_unprepare(zpc->wclk);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops zx_pwm_ops = {
> +	.apply = zx_pwm_apply,
> +	.get_state = zx_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int zx_pwm_probe(struct platform_device *pdev)
> +{
> +	struct zx_pwm_chip *zpc;
> +	struct resource *res;
> +	int ret;
> +	int i;

Made this unsigned.

> +
> +	zpc = devm_kzalloc(&pdev->dev, sizeof(*zpc), GFP_KERNEL);
> +	if (!zpc)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	zpc->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(zpc->base))
> +		return PTR_ERR(zpc->base);
> +
> +	zpc->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	if (IS_ERR(zpc->pclk))
> +		return PTR_ERR(zpc->pclk);
> +
> +	zpc->wclk = devm_clk_get(&pdev->dev, "wclk");
> +	if (IS_ERR(zpc->wclk))
> +		return PTR_ERR(zpc->wclk);
> +
> +	ret = clk_prepare_enable(zpc->pclk);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * PWM devices may be enabled by firmware, and let's disable all of
> +	 * them initially to save power.
> +	 */
> +	for (i = 0; i < ZX_PWM_NUM; i++)
> +		zx_pwm_set_mask(zpc, i, ZX_PWM_MODE, ZX_PWM_EN, 0);
> +
> +	zpc->chip.dev = &pdev->dev;
> +	zpc->chip.ops = &zx_pwm_ops;
> +	zpc->chip.base = -1;
> +	zpc->chip.npwm = ZX_PWM_NUM;
> +	zpc->chip.of_xlate = of_pwm_xlate_with_flags;
> +	zpc->chip.of_pwm_n_cells = 3;

I moved the initialization loop below here, then removed ZX_PWM_NUM and
hard-coded it to 4, then made the loop use zpc->chip.npwm.

The reason why I did that is to make it clearer that you should use the
available variables wherever possible and avoid macros if you don't have
to use them. This way we set the parameter (npwm) in a single place and
use the parameter, rather than the value in all other places. This also
avoids the macro becoming the parameter, because the macro may become
invalid at a later point (let's say on a new generation of the chip with
a different number of PWM channels).

> +	ret = pwmchip_add(&zpc->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, zpc);
> +
> +	return 0;
> +}
> +
> +static int zx_pwm_remove(struct platform_device *pdev)
> +{
> +	struct zx_pwm_chip *zpc = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(zpc->pclk);
> +
> +	return pwmchip_remove(&zpc->chip);
> +}

I've changed this to:

	ret = pwmchip_remove(&zpc->chip);
	clk_disable_unprepare(zpc->pclk);

	return ret

just in case we ever end up touching registers during pwmchip_remove(),
at which point pclk might need to be kept running until after it
finishes.

Like I said, all in all great work. Applied to for-4.14/drivers.

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/fd5381a5/attachment.sig>

[PATCH v2 2/2] pwm: add ZTE ZX PWM device driver

[Shawn Guo]

On Mon, Aug 21, 2017 at 08:03:52AM +0200, Thierry Reding wrote:
> On Thu, Jul 27, 2017 at 04:23:37PM +0800, Shawn Guo wrote:
> > From: Shawn Guo <shawn.guo@linaro.org>
> > 
> > It adds PWM device driver for ZTE ZX family SoCs.  The PWM controller
> > supports 4 devices with polarity configuration.
> > 
> > The driver has been tested with pwm-regulator support to scale core
> > voltage via cpufreq.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/pwm/Kconfig  |   9 ++
> >  drivers/pwm/Makefile |   1 +
> >  drivers/pwm/pwm-zx.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 280 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-zx.c
> 
> I've applied this, with some bikeshedding applied. See below for the
> changes I made.

Thanks, Thierry.  I appreciate the effort for those changes, and I'm
totally fine with them.

Shawn

[PATCH v2 0/2] Add ZTE ZX PWM device driver support

[Shawn Guo]

On Thu, Jul 27, 2017 at 04:23:35PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn.guo@linaro.org>
> 
> The series adds the dt-bindings document and device driver for PWM
> controller found on ZTE ZX family SoCs.
> 
> Changes for v2:
>  - Add Rob's ACK for dt-bindings patch
>  - Use inline function rather than macro for to_zx_pwm_chip()
>  - Convert zx_pwm_ops to implement atomic callbacks .apply() and .get_state()
> 
> Shawn Guo (2):
>   dt-bindings: pwm: add bindings doc for ZTE ZX PWM controller
>   pwm: add ZTE ZX PWM device driver

Hi Thierry,

We are getting close to 4.14 merge window.  Do you have any comments on
the series?

Shawn