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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Hi Neil,
On Mon, May 27, 2019 at 2:28 PM Neil Armstrong <[email protected]> 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
On 27/05/2019 19:57, Martin Blumenstingl wrote:
> Hi Neil,
>
> On Mon, May 27, 2019 at 2:28 PM Neil Armstrong <[email protected]> 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
>