* [PATCH v2 01/14] pwm: meson: unify the parameter list of meson_pwm_{enable, disable}
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-11 15:29 ` [PATCH v2 01/14] pwm: meson: unify the parameter list of meson_pwm_{enable,disable} Uwe Kleine-König
2019-06-08 18:06 ` [PATCH v2 02/14] pwm: meson: use devm_clk_get_optional() to get the input clock Martin Blumenstingl
` (12 subsequent siblings)
13 siblings, 1 reply; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 01/14] pwm: meson: unify the parameter list of meson_pwm_{enable,disable}
2019-06-08 18:06 ` [PATCH v2 01/14] pwm: meson: unify the parameter list of meson_pwm_{enable, disable} Martin Blumenstingl
@ 2019-06-11 15:29 ` Uwe Kleine-König
0 siblings, 0 replies; 20+ messages in thread
From: Uwe Kleine-König @ 2019-06-11 15:29 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: linux-pwm, Neil Armstrong, linux-kernel, thierry.reding,
linux-amlogic, linux-arm-kernel
Hello Martin,
On Sat, Jun 08, 2019 at 08:06:13PM +0200, 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>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Reviewed-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/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 02/14] pwm: meson: use devm_clk_get_optional() to get the input clock
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 01/14] pwm: meson: unify the parameter list of meson_pwm_{enable, disable} Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-11 15:37 ` Uwe Kleine-König
2019-06-08 18:06 ` [PATCH v2 03/14] pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values Martin Blumenstingl
` (11 subsequent siblings)
13 siblings, 1 reply; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 02/14] pwm: meson: use devm_clk_get_optional() to get the input clock
2019-06-08 18:06 ` [PATCH v2 02/14] pwm: meson: use devm_clk_get_optional() to get the input clock Martin Blumenstingl
@ 2019-06-11 15:37 ` Uwe Kleine-König
0 siblings, 0 replies; 20+ messages in thread
From: Uwe Kleine-König @ 2019-06-11 15:37 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: linux-pwm, Neil Armstrong, linux-kernel, thierry.reding, kernel,
linux-amlogic, linux-arm-kernel
On Sat, Jun 08, 2019 at 08:06:14PM +0200, 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>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Nitpick: Put your S-o-b last. Otherwise this ends as
Sgned-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Rviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Sgned-off-by: Thierry Reding <thierry.reding@gmail.com>
and this implies that Thierry added Neil's tag. (Typos are done on
purpose to not confuse patchwork.)
Reviewed-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/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 03/14] pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 01/14] pwm: meson: unify the parameter list of meson_pwm_{enable, disable} Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 02/14] pwm: meson: use devm_clk_get_optional() to get the input clock Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-11 15:38 ` Uwe Kleine-König
2019-06-08 18:06 ` [PATCH v2 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK Martin Blumenstingl
` (10 subsequent siblings)
13 siblings, 1 reply; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 03/14] pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values
2019-06-08 18:06 ` [PATCH v2 03/14] pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values Martin Blumenstingl
@ 2019-06-11 15:38 ` Uwe Kleine-König
0 siblings, 0 replies; 20+ messages in thread
From: Uwe Kleine-König @ 2019-06-11 15:38 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: linux-pwm, Neil Armstrong, linux-kernel, thierry.reding,
linux-amlogic, linux-arm-kernel
On Sat, Jun 08, 2019 at 08:06:15PM +0200, 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>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Reviewed-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/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (2 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 03/14] pwm: meson: use GENMASK and FIELD_PREP for the lo and hi values Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-11 16:33 ` Uwe Kleine-König
2019-06-08 18:06 ` [PATCH v2 05/14] pwm: meson: don't duplicate the polarity internally Martin Blumenstingl
` (9 subsequent siblings)
13 siblings, 1 reply; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK
2019-06-08 18:06 ` [PATCH v2 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK Martin Blumenstingl
@ 2019-06-11 16:33 ` Uwe Kleine-König
2019-06-12 19:47 ` Martin Blumenstingl
0 siblings, 1 reply; 20+ messages in thread
From: Uwe Kleine-König @ 2019-06-11 16:33 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: linux-pwm, Neil Armstrong, Stephen Boyd, Michael Turquette,
linux-kernel, thierry.reding, linux-amlogic, linux-clk,
linux-arm-kernel
Hello,
[added clk-people to recipients]
On Sat, Jun 08, 2019 at 08:06:16PM +0200, 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>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.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;
IMHO clk_mux is ugly here. It could easily just take
.mask = 3 << mux_reg_shifts[i],
as input parameter instead of
.mask = 3,
.shift = mux_reg_shifts[i],
. Then the usage would be (IMHO) a bit more natural, the clock mask
could then be defined as:
#define MISC_CLK_SEL_MASK(i) GENMASK(5 + 2 * (i), 4 + 2 * (i))
and this value could just be passed to the clk_mux.
(OK, this could be done already now, and then we'd do
channel->mux.shift = ffs(MISC_CLK_SEL_MASK(i)) - 1;
channel->mux.mask = MISC_CLK_SEL_MASK(i) >> channel->mux.shift;
.)
Apart from that, I wonder if the pwm-meson driver should better use
clk_register_mux instead of open coding it. (Though there doesn't seem
to exists a devm_ variant of it.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK
2019-06-11 16:33 ` Uwe Kleine-König
@ 2019-06-12 19:47 ` Martin Blumenstingl
0 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-12 19:47 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, Neil Armstrong, Stephen Boyd, Michael Turquette,
linux-kernel, thierry.reding, linux-amlogic, linux-clk,
linux-arm-kernel
Hi Uwe,
On Tue, Jun 11, 2019 at 6:33 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
[...]
> > @@ -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;
>
> IMHO clk_mux is ugly here. It could easily just take
>
> .mask = 3 << mux_reg_shifts[i],
in most cases that would be even nicer to read because it could be expressed as:
.mask = GENMASK(5, 4)
so I like your idea in general
though I think it should not block this patch
[...]
> Apart from that, I wonder if the pwm-meson driver should better use
> clk_register_mux instead of open coding it. (Though there doesn't seem
> to exists a devm_ variant of it.)
I tried to use clk_register_mux in the past. it works but it's not as
nice to read as the open-coded variant because it takes 10 parameters.
I find it easier to read 13 separate lines compared to reading a
function call with 10 parameters
Martin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 05/14] pwm: meson: don't duplicate the polarity internally
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (3 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 04/14] pwm: meson: change MISC_CLK_SEL_WIDTH to MISC_CLK_SEL_MASK Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 06/14] pwm: meson: pass struct pwm_device to meson_pwm_calc() Martin Blumenstingl
` (8 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 06/14] pwm: meson: pass struct pwm_device to meson_pwm_calc()
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (4 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 05/14] pwm: meson: don't duplicate the polarity internally Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 07/14] pwm: meson: add the meson_pwm_channel data to struct meson_pwm Martin Blumenstingl
` (7 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 07/14] pwm: meson: add the meson_pwm_channel data to struct meson_pwm
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (5 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 06/14] pwm: meson: pass struct pwm_device to meson_pwm_calc() Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 08/14] pwm: meson: add the per-channel register offsets and bits in a struct Martin Blumenstingl
` (6 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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..a4ae3587a3ce 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_PWMS;
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 08/14] pwm: meson: add the per-channel register offsets and bits in a struct
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (6 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 07/14] pwm: meson: add the meson_pwm_channel data to struct meson_pwm Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 09/14] pwm: meson: move pwm_set_chip_data() to meson_pwm_request() Martin Blumenstingl
` (5 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/pwm/pwm-meson.c | 90 ++++++++++++++++-------------------------
1 file changed, 34 insertions(+), 56 deletions(-)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index a4ae3587a3ce..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;
--
2.21.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 09/14] pwm: meson: move pwm_set_chip_data() to meson_pwm_request()
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (7 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 08/14] pwm: meson: add the per-channel register offsets and bits in a struct Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 10/14] pwm: meson: simplify the calculation of the pre-divider and count Martin Blumenstingl
` (4 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 10/14] pwm: meson: simplify the calculation of the pre-divider and count
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (8 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 09/14] pwm: meson: move pwm_set_chip_data() to meson_pwm_request() Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 11/14] pwm: meson: read the full hardware state in meson_pwm_get_state() Martin Blumenstingl
` (3 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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.
Three 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%)
3) Theoretical case where pre_div is different
clock input: 24MHz (XTAL)
period: 2730624ns
duty cycle: 1365312ns
old algorithm: pre_div=1, cnt=32768
new algorithm: pre_div=0, cnt=65534
Using a scope (24MHz sampling rate) shows the actual difference:
- old: 2731000ns
- new: 2731000ns
(my scope is not precise enough to measure the difference if there's
any)
Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 11/14] pwm: meson: read the full hardware state in meson_pwm_get_state()
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (9 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 10/14] pwm: meson: simplify the calculation of the pre-divider and count Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 12/14] pwm: meson: don't cache struct pwm_state internally Martin Blumenstingl
` (2 subsequent siblings)
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 12/14] pwm: meson: don't cache struct pwm_state internally
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (10 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 11/14] pwm: meson: read the full hardware state in meson_pwm_get_state() Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 13/14] pwm: meson: add support PWM_POLARITY_INVERSED when disabling Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 14/14] pwm: meson: add documentation to the driver Martin Blumenstingl
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 13/14] pwm: meson: add support PWM_POLARITY_INVERSED when disabling
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (11 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 12/14] pwm: meson: don't cache struct pwm_state internally Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
2019-06-08 18:06 ` [PATCH v2 14/14] pwm: meson: add documentation to the driver Martin Blumenstingl
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 14/14] pwm: meson: add documentation to the driver
2019-06-08 18:06 [PATCH v2 00/14] pwm-meson: cleanups and improvements Martin Blumenstingl
` (12 preceding siblings ...)
2019-06-08 18:06 ` [PATCH v2 13/14] pwm: meson: add support PWM_POLARITY_INVERSED when disabling Martin Blumenstingl
@ 2019-06-08 18:06 ` Martin Blumenstingl
13 siblings, 0 replies; 20+ messages in thread
From: Martin Blumenstingl @ 2019-06-08 18:06 UTC (permalink / raw)
To: linux-amlogic, linux-pwm, thierry.reding
Cc: Martin Blumenstingl, Neil Armstrong, linux-kernel,
linux-arm-kernel, u.kleine-koenig
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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread