[PATCH 00/14] pwm-meson: cleanups and improvements

[PATCH 00/14] pwm-meson: cleanups and improvements

[Martin Blumenstingl]

This series consists of various cleanups and improvements for the
pwm-meson driver.

Patches 1 to 6 are small code cleanups with the goal of making the code
easier to read.

Patches 7 to 9 are reworking the way the per-channel settings are
accessed. This is a first preparation step for adding full support to
meson_pwm_get_state() in the pwm-meson driver. Patch 7 makes struct
meson_pwm_channel accessible from struct meson_pwm because
meson_pwm_get_state() cannot use pwm_get_chip_data(). Patch 8 removes
redundant switch/case statements and ensures that we don't have to
add another redundant one for the upcoming full meson_pwm_get_state()
implementation. Patch 9 gets rid of meson_pwm_add_channels() and moves
the pwm_set_chip_data() call to meson_pwm_request() (like all other PWM
drivers do - except two).

Patch 10 is based on a suggestion by Uwe to simplify the calculation of
the values which the PWM IP requires. The nice benefit of this is that
we have an easier calculation which we can do "in reverse" for the
meson_pwm_get_state() (which calculates nanoseconds from the hardware
values).

Patch 11 implements reading the period and duty cycle in the
meson_pwm_get_state() callback.

Patch 12 removes some internal caching which we don't need anymore now
meson_pwm_get_state() is fully implemented. The PWM core now takes care
of not calling pwm_ops.apply() if "nothing has changed".

Patch 13 adds support for PWM_POLARITY_INVERSED when disabling the
output as suggested by Uwe.

Patch 14 completes this series by adding some documentation to the
driver. Thanks to Neil for summarizing how the hardware works
internally.

Due to the changed PWM calculation in patch 10 I have verified that
we don't break any existing boards. The patch itself contains two
examples which show that the new calculation improves precision. I
made screenshots of the measurements in pulseview [0] for the second
case ("PWM LED on Khadas VIM"):
- old algorithm: [1]
- old algorithm: [2]

Dependencies:
This series applies on top of Neil's patch "pwm: pwm-meson: update with
SPDX Licence identifier" [3]


[0] https://sigrok.org/wiki/PulseView
[1] https://abload.de/img/old-algormjs9.png
[2] https://abload.de/img/new-algo4ckjo.png
[3] https://patchwork.kernel.org/patch/10951319/


Martin Blumenstingl (14):
  pwm: meson: unify the parameter list of meson_pwm_{enable,disable}
  pwm: meson: use devm_clk_get_optional() to get the input clock
  pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values
  pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK
  pwm: meson: don't duplicate the polarity internally
  pwm: meson: pass struct pwm_device to meson_pwm_calc()
  pwm: meson: add the meson_pwm_channel data to struct meson_pwm
  pwm: meson: add the per-channel register offsets and bits in a struct
  pwm: meson: move pwm_set_chip_data() to meson_pwm_request()
  pwm: meson: simplify the calculation of the pre-divider and count
  pwm: meson: read the full hardware state in meson_pwm_get_state()
  pwm: meson: don't cache struct pwm_state internally
  pwm: meson: add support PWM_POLARITY_INVERSED when disabling
  pwm: meson: add documentation to the driver

 drivers/pwm/pwm-meson.c | 323 +++++++++++++++++++++-------------------
 1 file changed, 169 insertions(+), 154 deletions(-)

-- 
2.21.0

[PATCH 01/14] pwm: meson: unify the parameter list of meson_pwm_{enable,disable}

[Martin Blumenstingl]

This is a preparation for a future cleanup. Pass struct pwm_device
instead of passing the individual values required by each function as
these can be obtained for each struct pwm_device instance.

As a nice side-effect the driver now uses "switch (pwm->hwpwm)"
everywhere. Before some functions used "switch (id)" while others used
"switch (pwm->hwpwm)".

No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 5fef7e925282..3fbbc4128ce8 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -183,15 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson,
 	return 0;
 }
 
-static void meson_pwm_enable(struct meson_pwm *meson,
-			     struct meson_pwm_channel *channel,
-			     unsigned int id)
+static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
 {
+	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
 	u32 value, clk_shift, clk_enable, enable;
 	unsigned int offset;
 	unsigned long flags;
 
-	switch (id) {
+	switch (pwm->hwpwm) {
 	case 0:
 		clk_shift = MISC_A_CLK_DIV_SHIFT;
 		clk_enable = MISC_A_CLK_EN;
@@ -228,12 +227,12 @@ static void meson_pwm_enable(struct meson_pwm *meson,
 	spin_unlock_irqrestore(&meson->lock, flags);
 }
 
-static void meson_pwm_disable(struct meson_pwm *meson, unsigned int id)
+static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
 {
 	u32 value, enable;
 	unsigned long flags;
 
-	switch (id) {
+	switch (pwm->hwpwm) {
 	case 0:
 		enable = MISC_A_EN;
 		break;
@@ -266,7 +265,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -EINVAL;
 
 	if (!state->enabled) {
-		meson_pwm_disable(meson, pwm->hwpwm);
+		meson_pwm_disable(meson, pwm);
 		channel->state.enabled = false;
 
 		return 0;
@@ -293,7 +292,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	if (state->enabled && !channel->state.enabled) {
-		meson_pwm_enable(meson, channel, pwm->hwpwm);
+		meson_pwm_enable(meson, pwm);
 		channel->state.enabled = true;
 	}
 
-- 
2.21.0

[PATCH 02/14] pwm: meson: use devm_clk_get_optional() to get the input clock

[Martin Blumenstingl]

Simplify the code which fetches the input clock for a PWM channel by
using devm_clk_get_optional().
This comes with a small functional change: previously all errors except
EPROBE_DEFER were ignored. Now all other errors are also treated as
errors. If no input clock is present devm_clk_get_optional() will return
NULL instead of an error which matches the behavior of the old code.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 3fbbc4128ce8..35b38c7201c3 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -474,14 +474,9 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
 
 		snprintf(name, sizeof(name), "clkin%u", i);
 
-		channel->clk_parent = devm_clk_get(dev, name);
-		if (IS_ERR(channel->clk_parent)) {
-			err = PTR_ERR(channel->clk_parent);
-			if (err == -EPROBE_DEFER)
-				return err;
-
-			channel->clk_parent = NULL;
-		}
+		channel->clk_parent = devm_clk_get_optional(dev, name);
+		if (IS_ERR(channel->clk_parent))
+			return PTR_ERR(channel->clk_parent);
 	}
 
 	return 0;
-- 
2.21.0

[PATCH 03/14] pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values

[Martin Blumenstingl]

meson_pwm_calc() ensures that "lo" is always less than 16 bits wide
(otherwise it would overflow into the "hi" part of the REG_PWM_{A,B}
register).
Use GENMASK and FIELD_PREP for the lo and hi values to make it easier to
spot how wide these are internally. Additionally this is a preparation
step for the .get_state() implementation where the GENMASK() for lo and
hi becomes handy because it can be used with FIELD_GET() to extract the
values from the register REG_PWM_{A,B} register.

No functional changes intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 35b38c7201c3..c62a3ac924d0 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -5,6 +5,8 @@
  * Copyright (C) 2014 Amlogic, Inc.
  */
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
@@ -20,7 +22,8 @@
 
 #define REG_PWM_A		0x0
 #define REG_PWM_B		0x4
-#define PWM_HIGH_SHIFT		16
+#define PWM_LOW_MASK		GENMASK(15, 0)
+#define PWM_HIGH_MASK		GENMASK(31, 16)
 
 #define REG_MISC_AB		0x8
 #define MISC_B_CLK_EN		BIT(23)
@@ -217,7 +220,8 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
 	value |= clk_enable;
 	writel(value, meson->base + REG_MISC_AB);
 
-	value = (channel->hi << PWM_HIGH_SHIFT) | channel->lo;
+	value = FIELD_PREP(PWM_HIGH_MASK, channel->hi) |
+		FIELD_PREP(PWM_LOW_MASK, channel->lo);
 	writel(value, meson->base + offset);
 
 	value = readl(meson->base + REG_MISC_AB);
-- 
2.21.0

[PATCH 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK

[Martin Blumenstingl]

MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
a bit-mask. Rename and change the macro to be a bit-mask so that
conversion is not needed anymore. No functional changes intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index c62a3ac924d0..84b28ba0f903 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -33,7 +33,7 @@
 #define MISC_A_CLK_DIV_SHIFT	8
 #define MISC_B_CLK_SEL_SHIFT	6
 #define MISC_A_CLK_SEL_SHIFT	4
-#define MISC_CLK_SEL_WIDTH	2
+#define MISC_CLK_SEL_MASK	0x3
 #define MISC_B_EN		BIT(1)
 #define MISC_A_EN		BIT(0)
 
@@ -463,7 +463,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
 
 		channel->mux.reg = meson->base + REG_MISC_AB;
 		channel->mux.shift = mux_reg_shifts[i];
-		channel->mux.mask = BIT(MISC_CLK_SEL_WIDTH) - 1;
+		channel->mux.mask = MISC_CLK_SEL_MASK;
 		channel->mux.flags = 0;
 		channel->mux.lock = &meson->lock;
 		channel->mux.table = NULL;
-- 
2.21.0

[PATCH 05/14] pwm: meson: don't duplicate the polarity internally

[Martin Blumenstingl]

Let meson_pwm_calc() use the polarity from struct pwm_state directly.
This removes a level of indirection where meson_pwm_apply() first had to
set a driver-internal inverter mask which was then only used by
meson_pwm_calc().

Instead of adding the polarity as parameter to meson_pwm_calc() switch
to struct pwm_state directly to make it easier to see where the
parameters are actually coming from.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 84b28ba0f903..39ea119add7b 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -63,7 +63,6 @@ struct meson_pwm {
 	struct pwm_chip chip;
 	const struct meson_pwm_data *data;
 	void __iomem *base;
-	u8 inverter_mask;
 	/*
 	 * Protects register (write) access to the REG_MISC_AB register
 	 * that is shared between the two PWMs.
@@ -116,14 +115,17 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 }
 
 static int meson_pwm_calc(struct meson_pwm *meson,
-			  struct meson_pwm_channel *channel, unsigned int id,
-			  unsigned int duty, unsigned int period)
+			  struct meson_pwm_channel *channel,
+			  struct pwm_state *state)
 {
-	unsigned int pre_div, cnt, duty_cnt;
+	unsigned int duty, period, pre_div, cnt, duty_cnt;
 	unsigned long fin_freq = -1;
 	u64 fin_ps;
 
-	if (~(meson->inverter_mask >> id) & 0x1)
+	duty = state->duty_cycle;
+	period = state->period;
+
+	if (state->polarity == PWM_POLARITY_INVERSED)
 		duty = period - duty;
 
 	if (period == channel->state.period &&
@@ -278,15 +280,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->period != channel->state.period ||
 	    state->duty_cycle != channel->state.duty_cycle ||
 	    state->polarity != channel->state.polarity) {
-		if (state->polarity != channel->state.polarity) {
-			if (state->polarity == PWM_POLARITY_NORMAL)
-				meson->inverter_mask |= BIT(pwm->hwpwm);
-			else
-				meson->inverter_mask &= ~BIT(pwm->hwpwm);
-		}
-
-		err = meson_pwm_calc(meson, channel, pwm->hwpwm,
-				     state->duty_cycle, state->period);
+		err = meson_pwm_calc(meson, channel, state);
 		if (err < 0)
 			return err;
 
@@ -520,7 +514,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
 	meson->chip.of_pwm_n_cells = 3;
 
 	meson->data = of_device_get_match_data(&pdev->dev);
-	meson->inverter_mask = BIT(meson->chip.npwm) - 1;
 
 	channels = devm_kcalloc(&pdev->dev, meson->chip.npwm,
 				sizeof(*channels), GFP_KERNEL);
-- 
2.21.0

[PATCH 06/14] pwm: meson: pass struct pwm_device to meson_pwm_calc()

[Martin Blumenstingl]

meson_pwm_calc() is the last function that accepts a struct
meson_pwm_channel. meson_pwm_enable(), meson_pwm_disable() and
meson_pwm_apply() for example are all taking a struct pwm_device as
parameter. When they need the struct meson_pwm_channel these functions
simply call pwm_get_chip_data() internally.

Make meson_pwm_calc() consistent with the other functions in the
meson-pwm driver by passing struct pwm_device to it as well. The value
of the "id" parameter is actually pwm->hwpwm, but the driver never read
the "id" parameter, which is why there's no replacement for it in the
new code.

No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 39ea119add7b..d6eb4d04d5c9 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -114,10 +114,10 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 		clk_disable_unprepare(channel->clk);
 }
 
-static int meson_pwm_calc(struct meson_pwm *meson,
-			  struct meson_pwm_channel *channel,
+static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 			  struct pwm_state *state)
 {
+	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
 	unsigned int duty, period, pre_div, cnt, duty_cnt;
 	unsigned long fin_freq = -1;
 	u64 fin_ps;
@@ -280,7 +280,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->period != channel->state.period ||
 	    state->duty_cycle != channel->state.duty_cycle ||
 	    state->polarity != channel->state.polarity) {
-		err = meson_pwm_calc(meson, channel, state);
+		err = meson_pwm_calc(meson, pwm, state);
 		if (err < 0)
 			return err;
 
-- 
2.21.0

[PATCH 07/14] pwm: meson: add the meson_pwm_channel data to struct meson_pwm

[Martin Blumenstingl]

Make struct meson_pwm_channel accessible from struct meson_pwm.

PWM core has a limitation: per-channel data can only be set after
pwmchip_add() is called. However, pwmchip_add() internally calls
pwm_ops.get_state(). If pwm_ops.get_state() needs access to the
per-channel data it has to obtain it from struct pwm_chip and struct
pwm_device's hwpwm information.

Add a struct meson_pwm_channel for each PWM channel to struct meson_pwm
so the pwm_ops.get_state() callback can be implemented as it needs
access to the clock from struct meson_pwm_channel.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index d6eb4d04d5c9..d1718f54ecec 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -37,6 +37,8 @@
 #define MISC_B_EN		BIT(1)
 #define MISC_A_EN		BIT(0)
 
+#define MESON_NUM_PWMS		2
+
 static const unsigned int mux_reg_shifts[] = {
 	MISC_A_CLK_SEL_SHIFT,
 	MISC_B_CLK_SEL_SHIFT
@@ -62,6 +64,7 @@ struct meson_pwm_data {
 struct meson_pwm {
 	struct pwm_chip chip;
 	const struct meson_pwm_data *data;
+	struct meson_pwm_channel channels[MESON_NUM_PWMS];
 	void __iomem *base;
 	/*
 	 * Protects register (write) access to the REG_MISC_AB register
@@ -435,8 +438,7 @@ static const struct of_device_id meson_pwm_matches[] = {
 };
 MODULE_DEVICE_TABLE(of, meson_pwm_matches);
 
-static int meson_pwm_init_channels(struct meson_pwm *meson,
-				   struct meson_pwm_channel *channels)
+static int meson_pwm_init_channels(struct meson_pwm *meson)
 {
 	struct device *dev = meson->chip.dev;
 	struct clk_init_data init;
@@ -445,7 +447,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
 	int err;
 
 	for (i = 0; i < meson->chip.npwm; i++) {
-		struct meson_pwm_channel *channel = &channels[i];
+		struct meson_pwm_channel *channel = &meson->channels[i];
 
 		snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
 
@@ -480,18 +482,16 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
 	return 0;
 }
 
-static void meson_pwm_add_channels(struct meson_pwm *meson,
-				   struct meson_pwm_channel *channels)
+static void meson_pwm_add_channels(struct meson_pwm *meson)
 {
 	unsigned int i;
 
 	for (i = 0; i < meson->chip.npwm; i++)
-		pwm_set_chip_data(&meson->chip.pwms[i], &channels[i]);
+		pwm_set_chip_data(&meson->chip.pwms[i], &meson->channels[i]);
 }
 
 static int meson_pwm_probe(struct platform_device *pdev)
 {
-	struct meson_pwm_channel *channels;
 	struct meson_pwm *meson;
 	struct resource *regs;
 	int err;
@@ -509,18 +509,13 @@ static int meson_pwm_probe(struct platform_device *pdev)
 	meson->chip.dev = &pdev->dev;
 	meson->chip.ops = &meson_pwm_ops;
 	meson->chip.base = -1;
-	meson->chip.npwm = 2;
+	meson->chip.npwm = MESON_NUM_PWM;
 	meson->chip.of_xlate = of_pwm_xlate_with_flags;
 	meson->chip.of_pwm_n_cells = 3;
 
 	meson->data = of_device_get_match_data(&pdev->dev);
 
-	channels = devm_kcalloc(&pdev->dev, meson->chip.npwm,
-				sizeof(*channels), GFP_KERNEL);
-	if (!channels)
-		return -ENOMEM;
-
-	err = meson_pwm_init_channels(meson, channels);
+	err = meson_pwm_init_channels(meson);
 	if (err < 0)
 		return err;
 
@@ -530,7 +525,7 @@ static int meson_pwm_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	meson_pwm_add_channels(meson, channels);
+	meson_pwm_add_channels(meson);
 
 	platform_set_drvdata(pdev, meson);
 
-- 
2.21.0

[PATCH 08/14] pwm: meson: add the per-channel register offsets and bits in a struct

[Martin Blumenstingl]

Introduce struct meson_pwm_channel_data which contains the per-channel
offsets for the PWM register and REG_MISC_AB bits. Replace the existing
switch (pwm->hwpwm) statements with an access to the new struct.

This simplifies the code and will make it easier to implement
pwm_ops.get_state() because the switch-case which all per-channel
registers and offsets (as previously implemented in meson_pwm_enable())
doesn't have to be duplicated.

No functional changes intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 92 ++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 57 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index d1718f54ecec..ac7e188155fd 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -39,9 +39,27 @@
 
 #define MESON_NUM_PWMS		2
 
-static const unsigned int mux_reg_shifts[] = {
-	MISC_A_CLK_SEL_SHIFT,
-	MISC_B_CLK_SEL_SHIFT
+static struct meson_pwm_channel_data {
+	u8		reg_offset;
+	u8		clk_sel_shift;
+	u8		clk_div_shift;
+	u32		clk_en_mask;
+	u32		pwm_en_mask;
+} meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
+	{
+		.reg_offset	= REG_PWM_A,
+		.clk_sel_shift	= MISC_A_CLK_SEL_SHIFT,
+		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
+		.clk_en_mask	= MISC_A_CLK_EN,
+		.pwm_en_mask	= MISC_A_EN,
+	},
+	{
+		.reg_offset	= REG_PWM_B,
+		.clk_sel_shift	= MISC_B_CLK_SEL_SHIFT,
+		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
+		.clk_en_mask	= MISC_B_CLK_EN,
+		.pwm_en_mask	= MISC_B_EN,
+	}
 };
 
 struct meson_pwm_channel {
@@ -194,43 +212,26 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
 {
 	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
-	u32 value, clk_shift, clk_enable, enable;
-	unsigned int offset;
+	struct meson_pwm_channel_data *channel_data;
 	unsigned long flags;
+	u32 value;
 
-	switch (pwm->hwpwm) {
-	case 0:
-		clk_shift = MISC_A_CLK_DIV_SHIFT;
-		clk_enable = MISC_A_CLK_EN;
-		enable = MISC_A_EN;
-		offset = REG_PWM_A;
-		break;
-
-	case 1:
-		clk_shift = MISC_B_CLK_DIV_SHIFT;
-		clk_enable = MISC_B_CLK_EN;
-		enable = MISC_B_EN;
-		offset = REG_PWM_B;
-		break;
-
-	default:
-		return;
-	}
+	channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
 
 	spin_lock_irqsave(&meson->lock, flags);
 
 	value = readl(meson->base + REG_MISC_AB);
-	value &= ~(MISC_CLK_DIV_MASK << clk_shift);
-	value |= channel->pre_div << clk_shift;
-	value |= clk_enable;
+	value &= ~(MISC_CLK_DIV_MASK << channel_data->clk_div_shift);
+	value |= channel->pre_div << channel_data->clk_div_shift;
+	value |= channel_data->clk_en_mask;
 	writel(value, meson->base + REG_MISC_AB);
 
 	value = FIELD_PREP(PWM_HIGH_MASK, channel->hi) |
 		FIELD_PREP(PWM_LOW_MASK, channel->lo);
-	writel(value, meson->base + offset);
+	writel(value, meson->base + channel_data->reg_offset);
 
 	value = readl(meson->base + REG_MISC_AB);
-	value |= enable;
+	value |= channel_data->pwm_en_mask;
 	writel(value, meson->base + REG_MISC_AB);
 
 	spin_unlock_irqrestore(&meson->lock, flags);
@@ -238,26 +239,13 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
 
 static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
 {
-	u32 value, enable;
 	unsigned long flags;
-
-	switch (pwm->hwpwm) {
-	case 0:
-		enable = MISC_A_EN;
-		break;
-
-	case 1:
-		enable = MISC_B_EN;
-		break;
-
-	default:
-		return;
-	}
+	u32 value;
 
 	spin_lock_irqsave(&meson->lock, flags);
 
 	value = readl(meson->base + REG_MISC_AB);
-	value &= ~enable;
+	value &= ~meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
 	writel(value, meson->base + REG_MISC_AB);
 
 	spin_unlock_irqrestore(&meson->lock, flags);
@@ -309,18 +297,7 @@ static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (!state)
 		return;
 
-	switch (pwm->hwpwm) {
-	case 0:
-		mask = MISC_A_EN;
-		break;
-
-	case 1:
-		mask = MISC_B_EN;
-		break;
-
-	default:
-		return;
-	}
+	mask = meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
 
 	value = readl(meson->base + REG_MISC_AB);
 	state->enabled = (value & mask) != 0;
@@ -458,7 +435,8 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
 		init.num_parents = meson->data->num_parents;
 
 		channel->mux.reg = meson->base + REG_MISC_AB;
-		channel->mux.shift = mux_reg_shifts[i];
+		channel->mux.shift =
+				meson_pwm_per_channel_data[i].clk_sel_shift;
 		channel->mux.mask = MISC_CLK_SEL_MASK;
 		channel->mux.flags = 0;
 		channel->mux.lock = &meson->lock;
@@ -509,7 +487,7 @@ static int meson_pwm_probe(struct platform_device *pdev)
 	meson->chip.dev = &pdev->dev;
 	meson->chip.ops = &meson_pwm_ops;
 	meson->chip.base = -1;
-	meson->chip.npwm = MESON_NUM_PWM;
+	meson->chip.npwm = MESON_NUM_PWMS;
 	meson->chip.of_xlate = of_pwm_xlate_with_flags;
 	meson->chip.of_pwm_n_cells = 3;
 
-- 
2.21.0

[PATCH 09/14] pwm: meson: move pwm_set_chip_data() to meson_pwm_request()

[Martin Blumenstingl]

All existing PWM drivers (except pwm-meson and two other ones) call
pwm_set_chip_data() from their pwm_ops.request() callback. Now that we
can access the struct meson_pwm_channel from struct meson_pwm we can do
the same.

Move the call to pwm_set_chip_data() to meson_pwm_request() and drop the
custom meson_pwm_add_channels(). This makes the implementation
consistent with other drivers and makes it slightly more obvious
thatpwm_get_chip_data() cannot be used from pwm_ops.get_state() (because
that's called by the PWM core before pwm_ops.request()).

No functional changes intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index ac7e188155fd..27915d6475e3 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -98,12 +98,16 @@ static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
 
 static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
+	struct meson_pwm *meson = to_meson_pwm(chip);
+	struct meson_pwm_channel *channel;
 	struct device *dev = chip->dev;
 	int err;
 
-	if (!channel)
-		return -ENODEV;
+	channel = pwm_get_chip_data(pwm);
+	if (channel)
+		return 0;
+
+	channel = &meson->channels[pwm->hwpwm];
 
 	if (channel->clk_parent) {
 		err = clk_set_parent(channel->clk, channel->clk_parent);
@@ -124,7 +128,7 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	chip->ops->get_state(chip, pwm, &channel->state);
 
-	return 0;
+	return pwm_set_chip_data(pwm, channel);
 }
 
 static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -460,14 +464,6 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
 	return 0;
 }
 
-static void meson_pwm_add_channels(struct meson_pwm *meson)
-{
-	unsigned int i;
-
-	for (i = 0; i < meson->chip.npwm; i++)
-		pwm_set_chip_data(&meson->chip.pwms[i], &meson->channels[i]);
-}
-
 static int meson_pwm_probe(struct platform_device *pdev)
 {
 	struct meson_pwm *meson;
@@ -503,8 +499,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	meson_pwm_add_channels(meson);
-
 	platform_set_drvdata(pdev, meson);
 
 	return 0;
-- 
2.21.0

[PATCH 10/14] pwm: meson: simplify the calculation of the pre-divider and count

[Martin Blumenstingl]

Replace the loop to calculate the pre-divider and count with two
separate div64_u64() calculations. This makes the code easier to read
and improves the precision.

Two example cases:
1) 32.768kHz LPO clock for the SDIO wifi chip on Khadas VIM
   clock input: 500MHz (FCLK_DIV4)
   period: 30518ns
   duty cycle: 15259ns
old algorithm: pre_div=0, cnt=15259
new algorithm: pre_div=0, cnt=15259
(no difference in calculated values)

2) PWM LED on Khadas VIM
   clock input: 24MHz (XTAL)
   period: 7812500ns
   duty cycle: 7812500ns
old algorithm: pre_div=2, cnt=62004
new algorithm: pre_div=2, cnt=62500
Using a scope (24MHz sampling rate) shows the actual difference:
- old: 7753000ns, off by -59500ns (0.7616%)
- new: 7815000ns, off by +2500ns (0.032%)

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 27915d6475e3..9afa1e5aaebf 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -12,6 +12,7 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -145,7 +146,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
 	unsigned int duty, period, pre_div, cnt, duty_cnt;
 	unsigned long fin_freq = -1;
-	u64 fin_ps;
 
 	duty = state->duty_cycle;
 	period = state->period;
@@ -164,24 +164,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 	}
 
 	dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
-	fin_ps = (u64)NSEC_PER_SEC * 1000;
-	do_div(fin_ps, fin_freq);
-
-	/* Calc pre_div with the period */
-	for (pre_div = 0; pre_div <= MISC_CLK_DIV_MASK; pre_div++) {
-		cnt = DIV_ROUND_CLOSEST_ULL((u64)period * 1000,
-					    fin_ps * (pre_div + 1));
-		dev_dbg(meson->chip.dev, "fin_ps=%llu pre_div=%u cnt=%u\n",
-			fin_ps, pre_div, cnt);
-		if (cnt <= 0xffff)
-			break;
-	}
 
+	pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL);
 	if (pre_div > MISC_CLK_DIV_MASK) {
 		dev_err(meson->chip.dev, "unable to get period pre_div\n");
 		return -EINVAL;
 	}
 
+	cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1));
+	if (cnt > 0xffff) {
+		dev_err(meson->chip.dev, "unable to get period cnt\n");
+		return -EINVAL;
+	}
+
 	dev_dbg(meson->chip.dev, "period=%u pre_div=%u cnt=%u\n", period,
 		pre_div, cnt);
 
@@ -195,8 +190,8 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 		channel->lo = cnt;
 	} else {
 		/* Then check is we can have the duty with the same pre_div */
-		duty_cnt = DIV_ROUND_CLOSEST_ULL((u64)duty * 1000,
-						 fin_ps * (pre_div + 1));
+		duty_cnt = div64_u64(fin_freq * (u64)duty,
+				     NSEC_PER_SEC * (pre_div + 1));
 		if (duty_cnt > 0xffff) {
 			dev_err(meson->chip.dev, "unable to get duty cycle\n");
 			return -EINVAL;
-- 
2.21.0

[PATCH 11/14] pwm: meson: read the full hardware state in meson_pwm_get_state()

[Martin Blumenstingl]

Update the meson_pwm_get_state() implementation to take care of all
information in the registers instead of only reading the "enabled"
state.

The PWM output is only enabled if two conditions are met:
1. the per-channel clock is enabled
2. the PWM output is enabled

Calculate the PWM period and duty cycle using the reverse formula which
we already have in meson_pwm_calc() and update struct pwm_state with the
results.

As result of this /sys/kernel/debug/pwm now shows the PWM state set by
the bootloader (or firmware) after booting Linux.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 52 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 9afa1e5aaebf..010212166d5d 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -287,19 +287,65 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+static unsigned int meson_pwm_cnt_to_ns(struct pwm_chip *chip,
+					struct pwm_device *pwm, u32 cnt)
+{
+	struct meson_pwm *meson = to_meson_pwm(chip);
+	struct meson_pwm_channel *channel;
+	unsigned long fin_freq;
+	u32 fin_ns;
+
+	/* to_meson_pwm() can only be used after .get_state() is called */
+	channel = &meson->channels[pwm->hwpwm];
+
+	fin_freq = clk_get_rate(channel->clk);
+	if (fin_freq == 0)
+		return 0;
+
+	fin_ns = div_u64(NSEC_PER_SEC, fin_freq);
+
+	return cnt * fin_ns * (channel->pre_div + 1);
+}
+
 static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 				struct pwm_state *state)
 {
 	struct meson_pwm *meson = to_meson_pwm(chip);
-	u32 value, mask;
+	struct meson_pwm_channel_data *channel_data;
+	struct meson_pwm_channel *channel;
+	u32 value, tmp;
 
 	if (!state)
 		return;
 
-	mask = meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
+	channel = &meson->channels[pwm->hwpwm];
+	channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
 
 	value = readl(meson->base + REG_MISC_AB);
-	state->enabled = (value & mask) != 0;
+
+	tmp = channel_data->pwm_en_mask | channel_data->clk_en_mask;
+	state->enabled = (value & tmp) == tmp;
+
+	tmp = value >> channel_data->clk_div_shift;
+	channel->pre_div = FIELD_GET(MISC_CLK_DIV_MASK, tmp);
+
+	value = readl(meson->base + channel_data->reg_offset);
+
+	channel->lo = FIELD_GET(PWM_LOW_MASK, value);
+	channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
+
+	if (channel->lo == 0) {
+		state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
+		state->duty_cycle = state->period;
+	} else if (channel->lo >= channel->hi) {
+		state->period = meson_pwm_cnt_to_ns(chip, pwm,
+						    channel->lo + channel->hi);
+		state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm,
+							channel->hi);
+	} else {
+		state->period = 0;
+		state->duty_cycle = 0;
+	}
 }
 
 static const struct pwm_ops meson_pwm_ops = {
-- 
2.21.0

[PATCH 12/14] pwm: meson: don't cache struct pwm_state internally

[Martin Blumenstingl]

The PWM core already caches the "current struct pwm_state" as the
"current state of the hardware registers" inside struct pwm_device.

Drop the struct pwm_state from struct meson_pwm_channel in favour of the
struct pwm_state in struct pwm_device. While here also drop any checks
based on the pwm_state because the PWM core already takes care of this.

No functional changes intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 010212166d5d..900d362ec3c9 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -68,8 +68,6 @@ struct meson_pwm_channel {
 	unsigned int lo;
 	u8 pre_div;
 
-	struct pwm_state state;
-
 	struct clk *clk_parent;
 	struct clk_mux mux;
 	struct clk *clk;
@@ -127,8 +125,6 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 		return err;
 	}
 
-	chip->ops->get_state(chip, pwm, &channel->state);
-
 	return pwm_set_chip_data(pwm, channel);
 }
 
@@ -153,10 +149,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 	if (state->polarity == PWM_POLARITY_INVERSED)
 		duty = period - duty;
 
-	if (period == channel->state.period &&
-	    duty == channel->state.duty_cycle)
-		return 0;
-
 	fin_freq = clk_get_rate(channel->clk);
 	if (fin_freq == 0) {
 		dev_err(meson->chip.dev, "invalid source clock frequency\n");
@@ -253,7 +245,6 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
 static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			   struct pwm_state *state)
 {
-	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
 	struct meson_pwm *meson = to_meson_pwm(chip);
 	int err = 0;
 
@@ -262,26 +253,12 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	if (!state->enabled) {
 		meson_pwm_disable(meson, pwm);
-		channel->state.enabled = false;
-
-		return 0;
-	}
-
-	if (state->period != channel->state.period ||
-	    state->duty_cycle != channel->state.duty_cycle ||
-	    state->polarity != channel->state.polarity) {
+	} else {
 		err = meson_pwm_calc(meson, pwm, state);
 		if (err < 0)
 			return err;
 
-		channel->state.polarity = state->polarity;
-		channel->state.period = state->period;
-		channel->state.duty_cycle = state->duty_cycle;
-	}
-
-	if (state->enabled && !channel->state.enabled) {
 		meson_pwm_enable(meson, pwm);
-		channel->state.enabled = true;
 	}
 
 	return 0;
-- 
2.21.0

[PATCH 13/14] pwm: meson: add support PWM_POLARITY_INVERSED when disabling

[Martin Blumenstingl]

meson_pwm_apply() has to consider the PWM polarity when disabling the
output.
With enabled=false and polarity=PWM_POLARITY_NORMAL the output needs to
be LOW. The driver already supports this.
With enabled=false and polarity=PWM_POLARITY_INVERSED the output needs
to be HIGH. Implement this in the driver by internally enabling the
output with the same settings that we already use for "period == duty".

This fixes a PWM API violation which expects that the driver honors the
polarity also for enabled=false. Due to the IP block not supporting this
natively we only get "an as close as possible" to 100% HIGH signal (in
my test setup with input clock of 24MHz and measuring the output with a
logic analyzer at 24MHz sampling rate I got a duty cycle of 99.998475%
on a Khadas VIM).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 900d362ec3c9..bb48ba85f756 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -245,6 +245,7 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
 static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			   struct pwm_state *state)
 {
+	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
 	struct meson_pwm *meson = to_meson_pwm(chip);
 	int err = 0;
 
@@ -252,7 +253,27 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -EINVAL;
 
 	if (!state->enabled) {
-		meson_pwm_disable(meson, pwm);
+		if (state->polarity == PWM_POLARITY_INVERSED) {
+			/*
+			 * This IP block revision doesn't have an "always high"
+			 * setting which we can use for "inverted disabled".
+			 * Instead we achieve this using the same settings
+			 * that we use a pre_div of 0 (to get the shortest
+			 * possible duration for one "count") and
+			 * "period == duty_cycle". This results in a signal
+			 * which is LOW for one "count", while being HIGH for
+			 * the rest of the (so the signal is HIGH for slightly
+			 * less than 100% of the period, but this is the best
+			 * we can achieve).
+			 */
+			channel->pre_div = 0;
+			channel->hi = ~0;
+			channel->lo = 0;
+
+			meson_pwm_enable(meson, pwm);
+		} else {
+			meson_pwm_disable(meson, pwm);
+		}
 	} else {
 		err = meson_pwm_calc(meson, pwm, state);
 		if (err < 0)
-- 
2.21.0

[PATCH 14/14] pwm: meson: add documentation to the driver

[Martin Blumenstingl]

Add a link to the datasheet and a short summary how the hardware works.
The goal is to make it easier for other developers to understand why the
pwm-meson driver is implemented the way it is.

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Co-authored-by: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/pwm/pwm-meson.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index bb48ba85f756..6a978caba483 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -1,5 +1,23 @@
 // SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
 /*
+ * PWM controller driver for Amlogic Meson SoCs.
+ *
+ * This PWM is only a set of Gates, Dividers and Counters:
+ * PWM output is achieved by calculating a clock that permits calculating
+ * two periods (low and high). The counter then has to be set to switch after
+ * N cycles for the first half period.
+ * The hardware has no "polarity" setting. This driver reverses the period
+ * cycles (the low length is inverted with the high length) for
+ * PWM_POLARITY_INVERSED. This means that .get_state cannot read the polarity
+ * from the hardware.
+ * Setting the duty cycle will disable and re-enable the PWM output.
+ * Disabling the PWM stops the output immediately (without waiting for the
+ * current period to complete first).
+ *
+ * The public S922X datasheet contains some documentation for this PWM
+ * controller starting on page 1084:
+ * https://dl.khadas.com/Hardware/VIM2/Datasheet/S912_Datasheet_V0.220170314publicversion-Wesion.pdf
+ *
  * Copyright (c) 2016 BayLibre, SAS.
  * Author: Neil Armstrong <narmstrong@baylibre.com>
  * Copyright (C) 2014 Amlogic, Inc.
-- 
2.21.0

Re: [PATCH 10/14] pwm: meson: simplify the calculation of the pre-divider and count

[Uwe Kleine-König]

On Sat, May 25, 2019 at 08:11:29PM +0200, Martin Blumenstingl wrote:
> Replace the loop to calculate the pre-divider and count with two
> separate div64_u64() calculations. This makes the code easier to read
> and improves the precision.
> 
> Two example cases:
> 1) 32.768kHz LPO clock for the SDIO wifi chip on Khadas VIM
>    clock input: 500MHz (FCLK_DIV4)
>    period: 30518ns
>    duty cycle: 15259ns
> old algorithm: pre_div=0, cnt=15259
> new algorithm: pre_div=0, cnt=15259
> (no difference in calculated values)
> 
> 2) PWM LED on Khadas VIM
>    clock input: 24MHz (XTAL)
>    period: 7812500ns
>    duty cycle: 7812500ns
> old algorithm: pre_div=2, cnt=62004
> new algorithm: pre_div=2, cnt=62500
> Using a scope (24MHz sampling rate) shows the actual difference:
> - old: 7753000ns, off by -59500ns (0.7616%)
> - new: 7815000ns, off by +2500ns (0.032%)
> 
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 27915d6475e3..9afa1e5aaebf 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -12,6 +12,7 @@
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/math64.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -145,7 +146,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>  	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
>  	unsigned int duty, period, pre_div, cnt, duty_cnt;
>  	unsigned long fin_freq = -1;
> -	u64 fin_ps;
>  
>  	duty = state->duty_cycle;
>  	period = state->period;
> @@ -164,24 +164,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>  	}
>  
>  	dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
> -	fin_ps = (u64)NSEC_PER_SEC * 1000;
> -	do_div(fin_ps, fin_freq);
> -
> -	/* Calc pre_div with the period */
> -	for (pre_div = 0; pre_div <= MISC_CLK_DIV_MASK; pre_div++) {
> -		cnt = DIV_ROUND_CLOSEST_ULL((u64)period * 1000,
> -					    fin_ps * (pre_div + 1));
> -		dev_dbg(meson->chip.dev, "fin_ps=%llu pre_div=%u cnt=%u\n",
> -			fin_ps, pre_div, cnt);
> -		if (cnt <= 0xffff)
> -			break;
> -	}
>  
> +	pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL);
>  	if (pre_div > MISC_CLK_DIV_MASK) {
>  		dev_err(meson->chip.dev, "unable to get period pre_div\n");
>  		return -EINVAL;
>  	}
>  
> +	cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1));
> +	if (cnt > 0xffff) {
> +		dev_err(meson->chip.dev, "unable to get period cnt\n");
> +		return -EINVAL;
> +	}
> +

There is a slight modification in the calculation of pre_div that isn't
catched by the examples above.

Before this patch we had:

	pick smallest pre_div such that
		round_closest(period * 1000 / (round_down(1e12 / fin_freq) * (pre_div + 1)) <= 0xffff

New approach is:

	pre_div = round_down(fin_freq * period / (1e9 * 0xffff))

An advantage of the new approach is better as it rounds only once and is
easier.

Consider fin_freq = 99990001 and period = 655355, then the old algorithm
picks pre_div = 1 while the new picks pre_div = 0.

I didn't continue here to check which are the resulting waveforms, I
assume they are different though.

As there is currently no definition what is a "better" approximation for
a given requested pair (duty_cycle, period) I cannot say if these
changes are good or not.

And that's a pity, so I still think there should be a documented
definition that lays down how a lowlevel driver should round. Without
that a consumer that cares about fine differences can not rely an the
abstraction provided by the PWM framework because each low-level driver
might behave differently.

@Thierry: So can you please continue the discussion about this topic.
The longer this is delayed the more patches are created and submitted
that eventually might be wrong which is a waste of developer and
reviewer time.

Assuming the people who care about meson don't object after reading this
I wouldn't want to stop this patch going in though. So:

	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

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

Re: [PATCH 01/14] pwm: meson: unify the parameter list of meson_pwm_{enable, disable}

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> This is a preparation for a future cleanup. Pass struct pwm_device
> instead of passing the individual values required by each function as
> these can be obtained for each struct pwm_device instance.
> 
> As a nice side-effect the driver now uses "switch (pwm->hwpwm)"
> everywhere. Before some functions used "switch (id)" while others used
> "switch (pwm->hwpwm)".
> 
> No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 5fef7e925282..3fbbc4128ce8 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -183,15 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson,
>  	return 0;
>  }
>  
> -static void meson_pwm_enable(struct meson_pwm *meson,
> -			     struct meson_pwm_channel *channel,
> -			     unsigned int id)
> +static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>  {
> +	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
>  	u32 value, clk_shift, clk_enable, enable;
>  	unsigned int offset;
>  	unsigned long flags;
>  
> -	switch (id) {
> +	switch (pwm->hwpwm) {
>  	case 0:
>  		clk_shift = MISC_A_CLK_DIV_SHIFT;
>  		clk_enable = MISC_A_CLK_EN;
> @@ -228,12 +227,12 @@ static void meson_pwm_enable(struct meson_pwm *meson,
>  	spin_unlock_irqrestore(&meson->lock, flags);
>  }
>  
> -static void meson_pwm_disable(struct meson_pwm *meson, unsigned int id)
> +static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
>  {
>  	u32 value, enable;
>  	unsigned long flags;
>  
> -	switch (id) {
> +	switch (pwm->hwpwm) {
>  	case 0:
>  		enable = MISC_A_EN;
>  		break;
> @@ -266,7 +265,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return -EINVAL;
>  
>  	if (!state->enabled) {
> -		meson_pwm_disable(meson, pwm->hwpwm);
> +		meson_pwm_disable(meson, pwm);
>  		channel->state.enabled = false;
>  
>  		return 0;
> @@ -293,7 +292,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	}
>  
>  	if (state->enabled && !channel->state.enabled) {
> -		meson_pwm_enable(meson, channel, pwm->hwpwm);
> +		meson_pwm_enable(meson, pwm);
>  		channel->state.enabled = true;
>  	}
>  
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 02/14] pwm: meson: use devm_clk_get_optional() to get the input clock

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Simplify the code which fetches the input clock for a PWM channel by
> using devm_clk_get_optional().
> This comes with a small functional change: previously all errors except
> EPROBE_DEFER were ignored. Now all other errors are also treated as
> errors. If no input clock is present devm_clk_get_optional() will return
> NULL instead of an error which matches the behavior of the old code.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 3fbbc4128ce8..35b38c7201c3 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -474,14 +474,9 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
>  
>  		snprintf(name, sizeof(name), "clkin%u", i);
>  
> -		channel->clk_parent = devm_clk_get(dev, name);
> -		if (IS_ERR(channel->clk_parent)) {
> -			err = PTR_ERR(channel->clk_parent);
> -			if (err == -EPROBE_DEFER)
> -				return err;
> -
> -			channel->clk_parent = NULL;
> -		}
> +		channel->clk_parent = devm_clk_get_optional(dev, name);
> +		if (IS_ERR(channel->clk_parent))
> +			return PTR_ERR(channel->clk_parent);
>  	}
>  
>  	return 0;
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 03/14] pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> meson_pwm_calc() ensures that "lo" is always less than 16 bits wide
> (otherwise it would overflow into the "hi" part of the REG_PWM_{A,B}
> register).
> Use GENMASK and FIELD_PREP for the lo and hi values to make it easier to
> spot how wide these are internally. Additionally this is a preparation
> step for the .get_state() implementation where the GENMASK() for lo and
> hi becomes handy because it can be used with FIELD_GET() to extract the
> values from the register REG_PWM_{A,B} register.
> 
> No functional changes intended.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 35b38c7201c3..c62a3ac924d0 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -5,6 +5,8 @@
>   * Copyright (C) 2014 Amlogic, Inc.
>   */
>  
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/err.h>
> @@ -20,7 +22,8 @@
>  
>  #define REG_PWM_A		0x0
>  #define REG_PWM_B		0x4
> -#define PWM_HIGH_SHIFT		16
> +#define PWM_LOW_MASK		GENMASK(15, 0)
> +#define PWM_HIGH_MASK		GENMASK(31, 16)
>  
>  #define REG_MISC_AB		0x8
>  #define MISC_B_CLK_EN		BIT(23)
> @@ -217,7 +220,8 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>  	value |= clk_enable;
>  	writel(value, meson->base + REG_MISC_AB);
>  
> -	value = (channel->hi << PWM_HIGH_SHIFT) | channel->lo;
> +	value = FIELD_PREP(PWM_HIGH_MASK, channel->hi) |
> +		FIELD_PREP(PWM_LOW_MASK, channel->lo);
>  	writel(value, meson->base + offset);
>  
>  	value = readl(meson->base + REG_MISC_AB);
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
> a bit-mask. Rename and change the macro to be a bit-mask so that
> conversion is not needed anymore. No functional changes intended.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index c62a3ac924d0..84b28ba0f903 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -33,7 +33,7 @@
>  #define MISC_A_CLK_DIV_SHIFT	8
>  #define MISC_B_CLK_SEL_SHIFT	6
>  #define MISC_A_CLK_SEL_SHIFT	4
> -#define MISC_CLK_SEL_WIDTH	2
> +#define MISC_CLK_SEL_MASK	0x3

NIT I would have used GENMASK here

>  #define MISC_B_EN		BIT(1)
>  #define MISC_A_EN		BIT(0)
>  
> @@ -463,7 +463,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
>  
>  		channel->mux.reg = meson->base + REG_MISC_AB;
>  		channel->mux.shift = mux_reg_shifts[i];
> -		channel->mux.mask = BIT(MISC_CLK_SEL_WIDTH) - 1;
> +		channel->mux.mask = MISC_CLK_SEL_MASK;
>  		channel->mux.flags = 0;
>  		channel->mux.lock = &meson->lock;
>  		channel->mux.table = NULL;
> 

Anyway, it's still correct :

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 05/14] pwm: meson: don't duplicate the polarity internally

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Let meson_pwm_calc() use the polarity from struct pwm_state directly.
> This removes a level of indirection where meson_pwm_apply() first had to
> set a driver-internal inverter mask which was then only used by
> meson_pwm_calc().
> 
> Instead of adding the polarity as parameter to meson_pwm_calc() switch
> to struct pwm_state directly to make it easier to see where the
> parameters are actually coming from.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 84b28ba0f903..39ea119add7b 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -63,7 +63,6 @@ struct meson_pwm {
>  	struct pwm_chip chip;
>  	const struct meson_pwm_data *data;
>  	void __iomem *base;
> -	u8 inverter_mask;
>  	/*
>  	 * Protects register (write) access to the REG_MISC_AB register
>  	 * that is shared between the two PWMs.
> @@ -116,14 +115,17 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  }
>  
>  static int meson_pwm_calc(struct meson_pwm *meson,
> -			  struct meson_pwm_channel *channel, unsigned int id,
> -			  unsigned int duty, unsigned int period)
> +			  struct meson_pwm_channel *channel,
> +			  struct pwm_state *state)
>  {
> -	unsigned int pre_div, cnt, duty_cnt;
> +	unsigned int duty, period, pre_div, cnt, duty_cnt;
>  	unsigned long fin_freq = -1;
>  	u64 fin_ps;
>  
> -	if (~(meson->inverter_mask >> id) & 0x1)
> +	duty = state->duty_cycle;
> +	period = state->period;
> +
> +	if (state->polarity == PWM_POLARITY_INVERSED)
>  		duty = period - duty;
>  
>  	if (period == channel->state.period &&
> @@ -278,15 +280,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (state->period != channel->state.period ||
>  	    state->duty_cycle != channel->state.duty_cycle ||
>  	    state->polarity != channel->state.polarity) {
> -		if (state->polarity != channel->state.polarity) {
> -			if (state->polarity == PWM_POLARITY_NORMAL)
> -				meson->inverter_mask |= BIT(pwm->hwpwm);
> -			else
> -				meson->inverter_mask &= ~BIT(pwm->hwpwm);
> -		}
> -
> -		err = meson_pwm_calc(meson, channel, pwm->hwpwm,
> -				     state->duty_cycle, state->period);
> +		err = meson_pwm_calc(meson, channel, state);
>  		if (err < 0)
>  			return err;
>  
> @@ -520,7 +514,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
>  	meson->chip.of_pwm_n_cells = 3;
>  
>  	meson->data = of_device_get_match_data(&pdev->dev);
> -	meson->inverter_mask = BIT(meson->chip.npwm) - 1;
>  
>  	channels = devm_kcalloc(&pdev->dev, meson->chip.npwm,
>  				sizeof(*channels), GFP_KERNEL);
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 06/14] pwm: meson: pass struct pwm_device to meson_pwm_calc()

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> meson_pwm_calc() is the last function that accepts a struct
> meson_pwm_channel. meson_pwm_enable(), meson_pwm_disable() and
> meson_pwm_apply() for example are all taking a struct pwm_device as
> parameter. When they need the struct meson_pwm_channel these functions
> simply call pwm_get_chip_data() internally.
> 
> Make meson_pwm_calc() consistent with the other functions in the
> meson-pwm driver by passing struct pwm_device to it as well. The value
> of the "id" parameter is actually pwm->hwpwm, but the driver never read
> the "id" parameter, which is why there's no replacement for it in the
> new code.
> 
> No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 39ea119add7b..d6eb4d04d5c9 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -114,10 +114,10 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  		clk_disable_unprepare(channel->clk);
>  }
>  
> -static int meson_pwm_calc(struct meson_pwm *meson,
> -			  struct meson_pwm_channel *channel,
> +static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>  			  struct pwm_state *state)
>  {
> +	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
>  	unsigned int duty, period, pre_div, cnt, duty_cnt;
>  	unsigned long fin_freq = -1;
>  	u64 fin_ps;
> @@ -280,7 +280,7 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (state->period != channel->state.period ||
>  	    state->duty_cycle != channel->state.duty_cycle ||
>  	    state->polarity != channel->state.polarity) {
> -		err = meson_pwm_calc(meson, channel, state);
> +		err = meson_pwm_calc(meson, pwm, state);
>  		if (err < 0)
>  			return err;
>  
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 07/14] pwm: meson: add the meson_pwm_channel data to struct meson_pwm

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Make struct meson_pwm_channel accessible from struct meson_pwm.
> 
> PWM core has a limitation: per-channel data can only be set after
> pwmchip_add() is called. However, pwmchip_add() internally calls
> pwm_ops.get_state(). If pwm_ops.get_state() needs access to the
> per-channel data it has to obtain it from struct pwm_chip and struct
> pwm_device's hwpwm information.
> 
> Add a struct meson_pwm_channel for each PWM channel to struct meson_pwm
> so the pwm_ops.get_state() callback can be implemented as it needs
> access to the clock from struct meson_pwm_channel.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index d6eb4d04d5c9..d1718f54ecec 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -37,6 +37,8 @@
>  #define MISC_B_EN		BIT(1)
>  #define MISC_A_EN		BIT(0)
>  
> +#define MESON_NUM_PWMS		2
> +
>  static const unsigned int mux_reg_shifts[] = {
>  	MISC_A_CLK_SEL_SHIFT,
>  	MISC_B_CLK_SEL_SHIFT
> @@ -62,6 +64,7 @@ struct meson_pwm_data {
>  struct meson_pwm {
>  	struct pwm_chip chip;
>  	const struct meson_pwm_data *data;
> +	struct meson_pwm_channel channels[MESON_NUM_PWMS];
>  	void __iomem *base;
>  	/*
>  	 * Protects register (write) access to the REG_MISC_AB register
> @@ -435,8 +438,7 @@ static const struct of_device_id meson_pwm_matches[] = {
>  };
>  MODULE_DEVICE_TABLE(of, meson_pwm_matches);
>  
> -static int meson_pwm_init_channels(struct meson_pwm *meson,
> -				   struct meson_pwm_channel *channels)
> +static int meson_pwm_init_channels(struct meson_pwm *meson)
>  {
>  	struct device *dev = meson->chip.dev;
>  	struct clk_init_data init;
> @@ -445,7 +447,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
>  	int err;
>  
>  	for (i = 0; i < meson->chip.npwm; i++) {
> -		struct meson_pwm_channel *channel = &channels[i];
> +		struct meson_pwm_channel *channel = &meson->channels[i];
>  
>  		snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
>  
> @@ -480,18 +482,16 @@ static int meson_pwm_init_channels(struct meson_pwm *meson,
>  	return 0;
>  }
>  
> -static void meson_pwm_add_channels(struct meson_pwm *meson,
> -				   struct meson_pwm_channel *channels)
> +static void meson_pwm_add_channels(struct meson_pwm *meson)
>  {
>  	unsigned int i;
>  
>  	for (i = 0; i < meson->chip.npwm; i++)
> -		pwm_set_chip_data(&meson->chip.pwms[i], &channels[i]);
> +		pwm_set_chip_data(&meson->chip.pwms[i], &meson->channels[i]);
>  }
>  
>  static int meson_pwm_probe(struct platform_device *pdev)
>  {
> -	struct meson_pwm_channel *channels;
>  	struct meson_pwm *meson;
>  	struct resource *regs;
>  	int err;
> @@ -509,18 +509,13 @@ static int meson_pwm_probe(struct platform_device *pdev)
>  	meson->chip.dev = &pdev->dev;
>  	meson->chip.ops = &meson_pwm_ops;
>  	meson->chip.base = -1;
> -	meson->chip.npwm = 2;
> +	meson->chip.npwm = MESON_NUM_PWM;
>  	meson->chip.of_xlate = of_pwm_xlate_with_flags;
>  	meson->chip.of_pwm_n_cells = 3;
>  
>  	meson->data = of_device_get_match_data(&pdev->dev);
>  
> -	channels = devm_kcalloc(&pdev->dev, meson->chip.npwm,
> -				sizeof(*channels), GFP_KERNEL);
> -	if (!channels)
> -		return -ENOMEM;
> -
> -	err = meson_pwm_init_channels(meson, channels);
> +	err = meson_pwm_init_channels(meson);
>  	if (err < 0)
>  		return err;
>  
> @@ -530,7 +525,7 @@ static int meson_pwm_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	meson_pwm_add_channels(meson, channels);
> +	meson_pwm_add_channels(meson);
>  
>  	platform_set_drvdata(pdev, meson);
>  
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 08/14] pwm: meson: add the per-channel register offsets and bits in a struct

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Introduce struct meson_pwm_channel_data which contains the per-channel
> offsets for the PWM register and REG_MISC_AB bits. Replace the existing
> switch (pwm->hwpwm) statements with an access to the new struct.
> 
> This simplifies the code and will make it easier to implement
> pwm_ops.get_state() because the switch-case which all per-channel
> registers and offsets (as previously implemented in meson_pwm_enable())
> doesn't have to be duplicated.
> 
> No functional changes intended.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 92 ++++++++++++++++-------------------------
>  1 file changed, 35 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index d1718f54ecec..ac7e188155fd 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -39,9 +39,27 @@
>  
>  #define MESON_NUM_PWMS		2
>  
> -static const unsigned int mux_reg_shifts[] = {
> -	MISC_A_CLK_SEL_SHIFT,
> -	MISC_B_CLK_SEL_SHIFT
> +static struct meson_pwm_channel_data {
> +	u8		reg_offset;
> +	u8		clk_sel_shift;
> +	u8		clk_div_shift;
> +	u32		clk_en_mask;
> +	u32		pwm_en_mask;
> +} meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
> +	{
> +		.reg_offset	= REG_PWM_A,
> +		.clk_sel_shift	= MISC_A_CLK_SEL_SHIFT,
> +		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
> +		.clk_en_mask	= MISC_A_CLK_EN,
> +		.pwm_en_mask	= MISC_A_EN,
> +	},
> +	{
> +		.reg_offset	= REG_PWM_B,
> +		.clk_sel_shift	= MISC_B_CLK_SEL_SHIFT,
> +		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
> +		.clk_en_mask	= MISC_B_CLK_EN,
> +		.pwm_en_mask	= MISC_B_EN,
> +	}
>  };
>  
>  struct meson_pwm_channel {
> @@ -194,43 +212,26 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>  static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>  {
>  	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> -	u32 value, clk_shift, clk_enable, enable;
> -	unsigned int offset;
> +	struct meson_pwm_channel_data *channel_data;
>  	unsigned long flags;
> +	u32 value;
>  
> -	switch (pwm->hwpwm) {
> -	case 0:
> -		clk_shift = MISC_A_CLK_DIV_SHIFT;
> -		clk_enable = MISC_A_CLK_EN;
> -		enable = MISC_A_EN;
> -		offset = REG_PWM_A;
> -		break;
> -
> -	case 1:
> -		clk_shift = MISC_B_CLK_DIV_SHIFT;
> -		clk_enable = MISC_B_CLK_EN;
> -		enable = MISC_B_EN;
> -		offset = REG_PWM_B;
> -		break;
> -
> -	default:
> -		return;
> -	}
> +	channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
>  
>  	spin_lock_irqsave(&meson->lock, flags);
>  
>  	value = readl(meson->base + REG_MISC_AB);
> -	value &= ~(MISC_CLK_DIV_MASK << clk_shift);
> -	value |= channel->pre_div << clk_shift;
> -	value |= clk_enable;
> +	value &= ~(MISC_CLK_DIV_MASK << channel_data->clk_div_shift);
> +	value |= channel->pre_div << channel_data->clk_div_shift;
> +	value |= channel_data->clk_en_mask;
>  	writel(value, meson->base + REG_MISC_AB);
>  
>  	value = FIELD_PREP(PWM_HIGH_MASK, channel->hi) |
>  		FIELD_PREP(PWM_LOW_MASK, channel->lo);
> -	writel(value, meson->base + offset);
> +	writel(value, meson->base + channel_data->reg_offset);
>  
>  	value = readl(meson->base + REG_MISC_AB);
> -	value |= enable;
> +	value |= channel_data->pwm_en_mask;
>  	writel(value, meson->base + REG_MISC_AB);
>  
>  	spin_unlock_irqrestore(&meson->lock, flags);
> @@ -238,26 +239,13 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>  
>  static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
>  {
> -	u32 value, enable;
>  	unsigned long flags;
> -
> -	switch (pwm->hwpwm) {
> -	case 0:
> -		enable = MISC_A_EN;
> -		break;
> -
> -	case 1:
> -		enable = MISC_B_EN;
> -		break;
> -
> -	default:
> -		return;
> -	}
> +	u32 value;
>  
>  	spin_lock_irqsave(&meson->lock, flags);
>  
>  	value = readl(meson->base + REG_MISC_AB);
> -	value &= ~enable;
> +	value &= ~meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
>  	writel(value, meson->base + REG_MISC_AB);
>  
>  	spin_unlock_irqrestore(&meson->lock, flags);
> @@ -309,18 +297,7 @@ static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (!state)
>  		return;
>  
> -	switch (pwm->hwpwm) {
> -	case 0:
> -		mask = MISC_A_EN;
> -		break;
> -
> -	case 1:
> -		mask = MISC_B_EN;
> -		break;
> -
> -	default:
> -		return;
> -	}
> +	mask = meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
>  
>  	value = readl(meson->base + REG_MISC_AB);
>  	state->enabled = (value & mask) != 0;
> @@ -458,7 +435,8 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>  		init.num_parents = meson->data->num_parents;
>  
>  		channel->mux.reg = meson->base + REG_MISC_AB;
> -		channel->mux.shift = mux_reg_shifts[i];
> +		channel->mux.shift =
> +				meson_pwm_per_channel_data[i].clk_sel_shift;
>  		channel->mux.mask = MISC_CLK_SEL_MASK;
>  		channel->mux.flags = 0;
>  		channel->mux.lock = &meson->lock;
> @@ -509,7 +487,7 @@ static int meson_pwm_probe(struct platform_device *pdev)
>  	meson->chip.dev = &pdev->dev;
>  	meson->chip.ops = &meson_pwm_ops;
>  	meson->chip.base = -1;
> -	meson->chip.npwm = MESON_NUM_PWM;
> +	meson->chip.npwm = MESON_NUM_PWMS;
>  	meson->chip.of_xlate = of_pwm_xlate_with_flags;
>  	meson->chip.of_pwm_n_cells = 3;
>  
> 

This looks a little over-engineered, but it's correct :
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 09/14] pwm: meson: move pwm_set_chip_data() to meson_pwm_request()

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> All existing PWM drivers (except pwm-meson and two other ones) call
> pwm_set_chip_data() from their pwm_ops.request() callback. Now that we
> can access the struct meson_pwm_channel from struct meson_pwm we can do
> the same.
> 
> Move the call to pwm_set_chip_data() to meson_pwm_request() and drop the
> custom meson_pwm_add_channels(). This makes the implementation
> consistent with other drivers and makes it slightly more obvious
> thatpwm_get_chip_data() cannot be used from pwm_ops.get_state() (because
> that's called by the PWM core before pwm_ops.request()).
> 
> No functional changes intended.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index ac7e188155fd..27915d6475e3 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -98,12 +98,16 @@ static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
>  
>  static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> -	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> +	struct meson_pwm *meson = to_meson_pwm(chip);
> +	struct meson_pwm_channel *channel;
>  	struct device *dev = chip->dev;
>  	int err;
>  
> -	if (!channel)
> -		return -ENODEV;
> +	channel = pwm_get_chip_data(pwm);
> +	if (channel)
> +		return 0;
> +
> +	channel = &meson->channels[pwm->hwpwm];
>  
>  	if (channel->clk_parent) {
>  		err = clk_set_parent(channel->clk, channel->clk_parent);
> @@ -124,7 +128,7 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  	chip->ops->get_state(chip, pwm, &channel->state);
>  
> -	return 0;
> +	return pwm_set_chip_data(pwm, channel);
>  }
>  
>  static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -460,14 +464,6 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>  	return 0;
>  }
>  
> -static void meson_pwm_add_channels(struct meson_pwm *meson)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < meson->chip.npwm; i++)
> -		pwm_set_chip_data(&meson->chip.pwms[i], &meson->channels[i]);
> -}
> -
>  static int meson_pwm_probe(struct platform_device *pdev)
>  {
>  	struct meson_pwm *meson;
> @@ -503,8 +499,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	meson_pwm_add_channels(meson);
> -
>  	platform_set_drvdata(pdev, meson);
>  
>  	return 0;
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 11/14] pwm: meson: read the full hardware state in meson_pwm_get_state()

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Update the meson_pwm_get_state() implementation to take care of all
> information in the registers instead of only reading the "enabled"
> state.
> 
> The PWM output is only enabled if two conditions are met:
> 1. the per-channel clock is enabled
> 2. the PWM output is enabled
> 
> Calculate the PWM period and duty cycle using the reverse formula which
> we already have in meson_pwm_calc() and update struct pwm_state with the
> results.
> 
> As result of this /sys/kernel/debug/pwm now shows the PWM state set by
> the bootloader (or firmware) after booting Linux.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 52 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 9afa1e5aaebf..010212166d5d 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -287,19 +287,65 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return 0;
>  }
>  
> +static unsigned int meson_pwm_cnt_to_ns(struct pwm_chip *chip,
> +					struct pwm_device *pwm, u32 cnt)
> +{
> +	struct meson_pwm *meson = to_meson_pwm(chip);
> +	struct meson_pwm_channel *channel;
> +	unsigned long fin_freq;
> +	u32 fin_ns;
> +
> +	/* to_meson_pwm() can only be used after .get_state() is called */
> +	channel = &meson->channels[pwm->hwpwm];
> +
> +	fin_freq = clk_get_rate(channel->clk);
> +	if (fin_freq == 0)
> +		return 0;
> +
> +	fin_ns = div_u64(NSEC_PER_SEC, fin_freq);
> +
> +	return cnt * fin_ns * (channel->pre_div + 1);
> +}
> +
>  static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  				struct pwm_state *state)
>  {
>  	struct meson_pwm *meson = to_meson_pwm(chip);
> -	u32 value, mask;
> +	struct meson_pwm_channel_data *channel_data;
> +	struct meson_pwm_channel *channel;
> +	u32 value, tmp;
>  
>  	if (!state)
>  		return;
>  
> -	mask = meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask;
> +	channel = &meson->channels[pwm->hwpwm];
> +	channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
>  
>  	value = readl(meson->base + REG_MISC_AB);
> -	state->enabled = (value & mask) != 0;
> +
> +	tmp = channel_data->pwm_en_mask | channel_data->clk_en_mask;
> +	state->enabled = (value & tmp) == tmp;
> +
> +	tmp = value >> channel_data->clk_div_shift;
> +	channel->pre_div = FIELD_GET(MISC_CLK_DIV_MASK, tmp);
> +
> +	value = readl(meson->base + channel_data->reg_offset);
> +
> +	channel->lo = FIELD_GET(PWM_LOW_MASK, value);
> +	channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
> +
> +	if (channel->lo == 0) {
> +		state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
> +		state->duty_cycle = state->period;
> +	} else if (channel->lo >= channel->hi) {
> +		state->period = meson_pwm_cnt_to_ns(chip, pwm,
> +						    channel->lo + channel->hi);
> +		state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm,
> +							channel->hi);
> +	} else {
> +		state->period = 0;
> +		state->duty_cycle = 0;
> +	}
>  }
>  
>  static const struct pwm_ops meson_pwm_ops = {
> 

Thanks for that !!!

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 12/14] pwm: meson: don't cache struct pwm_state internally

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> The PWM core already caches the "current struct pwm_state" as the
> "current state of the hardware registers" inside struct pwm_device.
> 
> Drop the struct pwm_state from struct meson_pwm_channel in favour of the
> struct pwm_state in struct pwm_device. While here also drop any checks
> based on the pwm_state because the PWM core already takes care of this.
> 
> No functional changes intended.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 25 +------------------------
>  1 file changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 010212166d5d..900d362ec3c9 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -68,8 +68,6 @@ struct meson_pwm_channel {
>  	unsigned int lo;
>  	u8 pre_div;
>  
> -	struct pwm_state state;
> -
>  	struct clk *clk_parent;
>  	struct clk_mux mux;
>  	struct clk *clk;
> @@ -127,8 +125,6 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  		return err;
>  	}
>  
> -	chip->ops->get_state(chip, pwm, &channel->state);
> -
>  	return pwm_set_chip_data(pwm, channel);
>  }
>  
> @@ -153,10 +149,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>  	if (state->polarity == PWM_POLARITY_INVERSED)
>  		duty = period - duty;
>  
> -	if (period == channel->state.period &&
> -	    duty == channel->state.duty_cycle)
> -		return 0;
> -
>  	fin_freq = clk_get_rate(channel->clk);
>  	if (fin_freq == 0) {
>  		dev_err(meson->chip.dev, "invalid source clock frequency\n");
> @@ -253,7 +245,6 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
>  static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			   struct pwm_state *state)
>  {
> -	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
>  	struct meson_pwm *meson = to_meson_pwm(chip);
>  	int err = 0;
>  
> @@ -262,26 +253,12 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	if (!state->enabled) {
>  		meson_pwm_disable(meson, pwm);
> -		channel->state.enabled = false;
> -
> -		return 0;
> -	}
> -
> -	if (state->period != channel->state.period ||
> -	    state->duty_cycle != channel->state.duty_cycle ||
> -	    state->polarity != channel->state.polarity) {
> +	} else {
>  		err = meson_pwm_calc(meson, pwm, state);
>  		if (err < 0)
>  			return err;
>  
> -		channel->state.polarity = state->polarity;
> -		channel->state.period = state->period;
> -		channel->state.duty_cycle = state->duty_cycle;
> -	}
> -
> -	if (state->enabled && !channel->state.enabled) {
>  		meson_pwm_enable(meson, pwm);
> -		channel->state.enabled = true;
>  	}
>  
>  	return 0;
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 13/14] pwm: meson: add support PWM_POLARITY_INVERSED when disabling

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> meson_pwm_apply() has to consider the PWM polarity when disabling the
> output.
> With enabled=false and polarity=PWM_POLARITY_NORMAL the output needs to
> be LOW. The driver already supports this.
> With enabled=false and polarity=PWM_POLARITY_INVERSED the output needs
> to be HIGH. Implement this in the driver by internally enabling the
> output with the same settings that we already use for "period == duty".
> 
> This fixes a PWM API violation which expects that the driver honors the
> polarity also for enabled=false. Due to the IP block not supporting this
> natively we only get "an as close as possible" to 100% HIGH signal (in
> my test setup with input clock of 24MHz and measuring the output with a
> logic analyzer at 24MHz sampling rate I got a duty cycle of 99.998475%
> on a Khadas VIM).
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 900d362ec3c9..bb48ba85f756 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -245,6 +245,7 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
>  static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			   struct pwm_state *state)
>  {
> +	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
>  	struct meson_pwm *meson = to_meson_pwm(chip);
>  	int err = 0;
>  
> @@ -252,7 +253,27 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return -EINVAL;
>  
>  	if (!state->enabled) {
> -		meson_pwm_disable(meson, pwm);
> +		if (state->polarity == PWM_POLARITY_INVERSED) {
> +			/*
> +			 * This IP block revision doesn't have an "always high"
> +			 * setting which we can use for "inverted disabled".
> +			 * Instead we achieve this using the same settings
> +			 * that we use a pre_div of 0 (to get the shortest
> +			 * possible duration for one "count") and
> +			 * "period == duty_cycle". This results in a signal
> +			 * which is LOW for one "count", while being HIGH for
> +			 * the rest of the (so the signal is HIGH for slightly
> +			 * less than 100% of the period, but this is the best
> +			 * we can achieve).
> +			 */
> +			channel->pre_div = 0;
> +			channel->hi = ~0;
> +			channel->lo = 0;
> +
> +			meson_pwm_enable(meson, pwm);
> +		} else {
> +			meson_pwm_disable(meson, pwm);
> +		}
>  	} else {
>  		err = meson_pwm_calc(meson, pwm, state);
>  		if (err < 0)
> 

While not perfect, it almost fills the gap.
Another way would be to use a specific pinctrl state setting the pin
in GPIO output in high level, but this implementation could stay
if the pinctrl state isn't available.

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 14/14] pwm: meson: add documentation to the driver

[Neil Armstrong]

On 25/05/2019 20:11, Martin Blumenstingl wrote:
> Add a link to the datasheet and a short summary how the hardware works.
> The goal is to make it easier for other developers to understand why the
> pwm-meson driver is implemented the way it is.
> 
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Co-authored-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/pwm/pwm-meson.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index bb48ba85f756..6a978caba483 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -1,5 +1,23 @@
>  // SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
>  /*
> + * PWM controller driver for Amlogic Meson SoCs.
> + *
> + * This PWM is only a set of Gates, Dividers and Counters:
> + * PWM output is achieved by calculating a clock that permits calculating
> + * two periods (low and high). The counter then has to be set to switch after
> + * N cycles for the first half period.
> + * The hardware has no "polarity" setting. This driver reverses the period
> + * cycles (the low length is inverted with the high length) for
> + * PWM_POLARITY_INVERSED. This means that .get_state cannot read the polarity
> + * from the hardware.
> + * Setting the duty cycle will disable and re-enable the PWM output.
> + * Disabling the PWM stops the output immediately (without waiting for the
> + * current period to complete first).
> + *
> + * The public S922X datasheet contains some documentation for this PWM
> + * controller starting on page 1084:
> + * https://dl.khadas.com/Hardware/VIM2/Datasheet/S912_Datasheet_V0.220170314publicversion-Wesion.pdf
> + *
>   * Copyright (c) 2016 BayLibre, SAS.
>   * Author: Neil Armstrong <narmstrong@baylibre.com>
>   * Copyright (C) 2014 Amlogic, Inc.
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 10/14] pwm: meson: simplify the calculation of the pre-divider and count

[Neil Armstrong]

On 26/05/2019 21:41, Uwe Kleine-König wrote:
> On Sat, May 25, 2019 at 08:11:29PM +0200, Martin Blumenstingl wrote:
>> Replace the loop to calculate the pre-divider and count with two
>> separate div64_u64() calculations. This makes the code easier to read
>> and improves the precision.
>>
>> Two example cases:
>> 1) 32.768kHz LPO clock for the SDIO wifi chip on Khadas VIM
>>    clock input: 500MHz (FCLK_DIV4)
>>    period: 30518ns
>>    duty cycle: 15259ns
>> old algorithm: pre_div=0, cnt=15259
>> new algorithm: pre_div=0, cnt=15259
>> (no difference in calculated values)
>>
>> 2) PWM LED on Khadas VIM
>>    clock input: 24MHz (XTAL)
>>    period: 7812500ns
>>    duty cycle: 7812500ns
>> old algorithm: pre_div=2, cnt=62004
>> new algorithm: pre_div=2, cnt=62500
>> Using a scope (24MHz sampling rate) shows the actual difference:
>> - old: 7753000ns, off by -59500ns (0.7616%)
>> - new: 7815000ns, off by +2500ns (0.032%)
>>
>> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/pwm/pwm-meson.c | 25 ++++++++++---------------
>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 27915d6475e3..9afa1e5aaebf 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/err.h>
>>  #include <linux/io.h>
>>  #include <linux/kernel.h>
>> +#include <linux/math64.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> @@ -145,7 +146,6 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>  	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
>>  	unsigned int duty, period, pre_div, cnt, duty_cnt;
>>  	unsigned long fin_freq = -1;
>> -	u64 fin_ps;
>>  
>>  	duty = state->duty_cycle;
>>  	period = state->period;
>> @@ -164,24 +164,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>  	}
>>  
>>  	dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
>> -	fin_ps = (u64)NSEC_PER_SEC * 1000;
>> -	do_div(fin_ps, fin_freq);
>> -
>> -	/* Calc pre_div with the period */
>> -	for (pre_div = 0; pre_div <= MISC_CLK_DIV_MASK; pre_div++) {
>> -		cnt = DIV_ROUND_CLOSEST_ULL((u64)period * 1000,
>> -					    fin_ps * (pre_div + 1));
>> -		dev_dbg(meson->chip.dev, "fin_ps=%llu pre_div=%u cnt=%u\n",
>> -			fin_ps, pre_div, cnt);
>> -		if (cnt <= 0xffff)
>> -			break;
>> -	}
>>  
>> +	pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL);
>>  	if (pre_div > MISC_CLK_DIV_MASK) {
>>  		dev_err(meson->chip.dev, "unable to get period pre_div\n");
>>  		return -EINVAL;
>>  	}
>>  
>> +	cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1));
>> +	if (cnt > 0xffff) {
>> +		dev_err(meson->chip.dev, "unable to get period cnt\n");
>> +		return -EINVAL;
>> +	}
>> +
> 
> There is a slight modification in the calculation of pre_div that isn't
> catched by the examples above.
> 
> Before this patch we had:
> 
> 	pick smallest pre_div such that
> 		round_closest(period * 1000 / (round_down(1e12 / fin_freq) * (pre_div + 1)) <= 0xffff
> 
> New approach is:
> 
> 	pre_div = round_down(fin_freq * period / (1e9 * 0xffff))
> 
> An advantage of the new approach is better as it rounds only once and is
> easier.
> 
> Consider fin_freq = 99990001 and period = 655355, then the old algorithm
> picks pre_div = 1 while the new picks pre_div = 0.
> 
> I didn't continue here to check which are the resulting waveforms, I
> assume they are different though.
> 
> As there is currently no definition what is a "better" approximation for
> a given requested pair (duty_cycle, period) I cannot say if these
> changes are good or not.
> 
> And that's a pity, so I still think there should be a documented
> definition that lays down how a lowlevel driver should round. Without
> that a consumer that cares about fine differences can not rely an the
> abstraction provided by the PWM framework because each low-level driver
> might behave differently.
> 
> @Thierry: So can you please continue the discussion about this topic.
> The longer this is delayed the more patches are created and submitted
> that eventually might be wrong which is a waste of developer and
> reviewer time.
> 
> Assuming the people who care about meson don't object after reading this
> I wouldn't want to stop this patch going in though. So:
> 
> 	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Best regards
> Uwe
> 

I don't have a strong view on this, Martin showed similar or much greater
accuracy in the 2 principal use cases of the driver, so I'm ok with it.

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK

[Martin Blumenstingl]

Hi Neil,

On Mon, May 27, 2019 at 2:26 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> On 25/05/2019 20:11, Martin Blumenstingl wrote:
> > MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
> > a bit-mask. Rename and change the macro to be a bit-mask so that
> > conversion is not needed anymore. No functional changes intended.
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > ---
> >  drivers/pwm/pwm-meson.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > index c62a3ac924d0..84b28ba0f903 100644
> > --- a/drivers/pwm/pwm-meson.c
> > +++ b/drivers/pwm/pwm-meson.c
> > @@ -33,7 +33,7 @@
> >  #define MISC_A_CLK_DIV_SHIFT 8
> >  #define MISC_B_CLK_SEL_SHIFT 6
> >  #define MISC_A_CLK_SEL_SHIFT 4
> > -#define MISC_CLK_SEL_WIDTH   2
> > +#define MISC_CLK_SEL_MASK    0x3
>
> NIT I would have used GENMASK here
that was my initial idea but I decided against it.
the variant I came up with was: #define MISC_CLK_SEL_MASK    GENMASK(1, 0)

however, the actual offset is either 4 or 6 (depending on the PWM channel)
and I felt that duplicating the macro would just make it more complicated
so instead I chose to be consistent with MISC_CLK_DIV_MASK

Let me know if you would like me to change it (then I prefer to update
MISC_CLK_DIV_MASK as well).


Martin

Re: [PATCH 13/14] pwm: meson: add support PWM_POLARITY_INVERSED when disabling

[Martin Blumenstingl]

Hi Neil,

On Mon, May 27, 2019 at 2:33 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> On 25/05/2019 20:11, Martin Blumenstingl wrote:
> > meson_pwm_apply() has to consider the PWM polarity when disabling the
> > output.
> > With enabled=false and polarity=PWM_POLARITY_NORMAL the output needs to
> > be LOW. The driver already supports this.
> > With enabled=false and polarity=PWM_POLARITY_INVERSED the output needs
> > to be HIGH. Implement this in the driver by internally enabling the
> > output with the same settings that we already use for "period == duty".
> >
> > This fixes a PWM API violation which expects that the driver honors the
> > polarity also for enabled=false. Due to the IP block not supporting this
> > natively we only get "an as close as possible" to 100% HIGH signal (in
> > my test setup with input clock of 24MHz and measuring the output with a
> > logic analyzer at 24MHz sampling rate I got a duty cycle of 99.998475%
> > on a Khadas VIM).
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > ---
> >  drivers/pwm/pwm-meson.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > index 900d362ec3c9..bb48ba85f756 100644
> > --- a/drivers/pwm/pwm-meson.c
> > +++ b/drivers/pwm/pwm-meson.c
> > @@ -245,6 +245,7 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
> >  static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >                          struct pwm_state *state)
> >  {
> > +     struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
> >       struct meson_pwm *meson = to_meson_pwm(chip);
> >       int err = 0;
> >
> > @@ -252,7 +253,27 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >               return -EINVAL;
> >
> >       if (!state->enabled) {
> > -             meson_pwm_disable(meson, pwm);
> > +             if (state->polarity == PWM_POLARITY_INVERSED) {
> > +                     /*
> > +                      * This IP block revision doesn't have an "always high"
> > +                      * setting which we can use for "inverted disabled".
> > +                      * Instead we achieve this using the same settings
> > +                      * that we use a pre_div of 0 (to get the shortest
> > +                      * possible duration for one "count") and
> > +                      * "period == duty_cycle". This results in a signal
> > +                      * which is LOW for one "count", while being HIGH for
> > +                      * the rest of the (so the signal is HIGH for slightly
> > +                      * less than 100% of the period, but this is the best
> > +                      * we can achieve).
> > +                      */
> > +                     channel->pre_div = 0;
> > +                     channel->hi = ~0;
> > +                     channel->lo = 0;
> > +
> > +                     meson_pwm_enable(meson, pwm);
> > +             } else {
> > +                     meson_pwm_disable(meson, pwm);
> > +             }
> >       } else {
> >               err = meson_pwm_calc(meson, pwm, state);
> >               if (err < 0)
> >
>
> While not perfect, it almost fills the gap.
> Another way would be to use a specific pinctrl state setting the pin
> in GPIO output in high level, but this implementation could stay
> if the pinctrl state isn't available.
I just noticed that Amlogic updated the PWM IP block in G12A:
it now supports "constant enable" (REG_MISC_AB bits 28 and 29) as well
as PWM_POLARITY_INVERSED (REG_MISC_AB bits 26 and 27) natively!

I like your idea of having a specific pinctrl state.
we can implement that for anything older than G12A once we actually need it.
for G12A we can do better thanks to the updated IP block


Martin

Re: [PATCH 07/14] pwm: meson: add the meson_pwm_channel data to struct meson_pwm

[Martin Blumenstingl]

On Sat, May 25, 2019 at 8:11 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
[...]
> @@ -509,18 +509,13 @@ static int meson_pwm_probe(struct platform_device *pdev)
>         meson->chip.dev = &pdev->dev;
>         meson->chip.ops = &meson_pwm_ops;
>         meson->chip.base = -1;
> -       meson->chip.npwm = 2;
> +       meson->chip.npwm = MESON_NUM_PWM;
I messed this up during rebase, the macro name is actually MESON_NUM_PWMS
I'll re-send a fix so these patches can be bisected cleanly (patch 8
fixes this typo accidentally)

Re: [PATCH 08/14] pwm: meson: add the per-channel register offsets and bits in a struct

[Martin Blumenstingl]

Hi Neil,

On Mon, May 27, 2019 at 2:28 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
[...]
> This looks a little over-engineered, but it's correct :
my main motivation was to "not copy the 20 line switch/case statement
from meson_pwm_enable() to meson_pwm_get_state()"
I extended the idea that already existed for the "mux_reg_shifts"
array and made it work for "more than one value"

please speak up if you have another idea in mind, maybe that makes the
result even better


Martin

Re: [PATCH 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK

[Uwe Kleine-König]

On Mon, May 27, 2019 at 07:46:43PM +0200, Martin Blumenstingl wrote:
> Hi Neil,
> 
> On Mon, May 27, 2019 at 2:26 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >
> > On 25/05/2019 20:11, Martin Blumenstingl wrote:
> > > MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
> > > a bit-mask. Rename and change the macro to be a bit-mask so that
> > > conversion is not needed anymore. No functional changes intended.
> > >
> > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > ---
> > >  drivers/pwm/pwm-meson.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > > index c62a3ac924d0..84b28ba0f903 100644
> > > --- a/drivers/pwm/pwm-meson.c
> > > +++ b/drivers/pwm/pwm-meson.c
> > > @@ -33,7 +33,7 @@
> > >  #define MISC_A_CLK_DIV_SHIFT 8
> > >  #define MISC_B_CLK_SEL_SHIFT 6
> > >  #define MISC_A_CLK_SEL_SHIFT 4
> > > -#define MISC_CLK_SEL_WIDTH   2
> > > +#define MISC_CLK_SEL_MASK    0x3
> >
> > NIT I would have used GENMASK here
> that was my initial idea but I decided against it.
> the variant I came up with was: #define MISC_CLK_SEL_MASK    GENMASK(1, 0)
> 
> however, the actual offset is either 4 or 6 (depending on the PWM channel)
> and I felt that duplicating the macro would just make it more complicated
> so instead I chose to be consistent with MISC_CLK_DIV_MASK

An option would be:

	#define MISC_CLK_SEL_MASK(hwid)		GENMASK(1 + 4 * (hwid), 0 + 4 * (hwid))

(Note I didn't check a manual to the 4 above is probably wrong.)

Best regards
Uwe

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

Re: [PATCH 08/14] pwm: meson: add the per-channel register offsets and bits in a struct

[Neil Armstrong]

On 27/05/2019 19:57, Martin Blumenstingl wrote:
> Hi Neil,
> 
> On Mon, May 27, 2019 at 2:28 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
> [...]
>> This looks a little over-engineered, but it's correct :
> my main motivation was to "not copy the 20 line switch/case statement
> from meson_pwm_enable() to meson_pwm_get_state()"
> I extended the idea that already existed for the "mux_reg_shifts"
> array and made it work for "more than one value"
> 
> please speak up if you have another idea in mind, maybe that makes the
> result even better

Sorry if I was misunderstood, your solution is correct, and I don't have any
simpler ideas in mind !

Neil

> 
> 
> Martin
> 

Re: [PATCH 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK

[Martin Blumenstingl]

Hi Uwe,

On Mon, May 27, 2019 at 8:00 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Mon, May 27, 2019 at 07:46:43PM +0200, Martin Blumenstingl wrote:
> > Hi Neil,
> >
> > On Mon, May 27, 2019 at 2:26 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
> > >
> > > On 25/05/2019 20:11, Martin Blumenstingl wrote:
> > > > MISC_CLK_SEL_WIDTH is only used in one place where it's converted into
> > > > a bit-mask. Rename and change the macro to be a bit-mask so that
> > > > conversion is not needed anymore. No functional changes intended.
> > > >
> > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > > ---
> > > >  drivers/pwm/pwm-meson.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > > > index c62a3ac924d0..84b28ba0f903 100644
> > > > --- a/drivers/pwm/pwm-meson.c
> > > > +++ b/drivers/pwm/pwm-meson.c
> > > > @@ -33,7 +33,7 @@
> > > >  #define MISC_A_CLK_DIV_SHIFT 8
> > > >  #define MISC_B_CLK_SEL_SHIFT 6
> > > >  #define MISC_A_CLK_SEL_SHIFT 4
> > > > -#define MISC_CLK_SEL_WIDTH   2
> > > > +#define MISC_CLK_SEL_MASK    0x3
> > >
> > > NIT I would have used GENMASK here
> > that was my initial idea but I decided against it.
> > the variant I came up with was: #define MISC_CLK_SEL_MASK    GENMASK(1, 0)
> >
> > however, the actual offset is either 4 or 6 (depending on the PWM channel)
> > and I felt that duplicating the macro would just make it more complicated
> > so instead I chose to be consistent with MISC_CLK_DIV_MASK
>
> An option would be:
>
>         #define MISC_CLK_SEL_MASK(hwid)         GENMASK(1 + 4 * (hwid), 0 + 4 * (hwid))
>
> (Note I didn't check a manual to the 4 above is probably wrong.)
that (or at least something similar) will work
the catch here is: we use it to initialize the mux clock and the
common clock framework expects us to set "shift" and "mask", while
mask starts at bit 0 instead of shift

this is how the current code is being used at the moment:
  channel->mux.shift = meson_pwm_per_channel_data[i].clk_sel_shift;
  channel->mux.mask = MISC_CLK_SEL_MASK;

so with MISC_CLK_SEL_MASK this would become:
  channel->mux.shift = meson_pwm_per_channel_data[i].clk_sel_shift;
  channel->mux.mask = MISC_CLK_SEL_MASK(i) >> channel->mux.shift;

or we could dynamically determine the "shift" using ffs or friends

my own brain parses the variant from the patch best
I'm happy to change it though if we can find something "better"


Martin