| Message ID | f7291bab-eb51-3f2d-4eb4-78f6330242ef@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: meson: make full use of common clock framework | expand |
Hi Heiner, On Wed, Apr 12, 2023 at 9:23 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > Change single-bit values from mask to bit. This facilitates > CCF initialization for the clock gate in a follow-up patch. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> # meson8b-odroidc1, sm1-x96-air [...] > #define REG_MISC_AB 0x8 > -#define MISC_B_CLK_EN BIT(23) > -#define MISC_A_CLK_EN BIT(15) > +#define MISC_B_CLK_EN 23 > +#define MISC_A_CLK_EN 15 > #define MISC_CLK_DIV_MASK 0x7f > #define MISC_B_CLK_DIV_SHIFT 16 > #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_MASK 0x3 > -#define MISC_B_EN BIT(1) > -#define MISC_A_EN BIT(0) > +#define MISC_B_EN 1 > +#define MISC_A_EN 0 Personally I'm fine with this change but it's not how I would have done it: - I would have kept the BIT() macro for MISC_{A,B}_EN - then I would have renamed MISC_{A,}_CLK_EN to MISC_{A,B}_CLK_EN_SHIFT (to be consistent with _SHIFT of the mux and divider) and drop the BIT() macro there (like you did) This is possibly/likely personal preference, so my suggestion is to wait for some more feedback. Best regards, Martin
On Wed, Apr 12, 2023 at 10:47:05PM +0200, Martin Blumenstingl wrote: > Hi Heiner, > > On Wed, Apr 12, 2023 at 9:23 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > > > Change single-bit values from mask to bit. This facilitates > > CCF initialization for the clock gate in a follow-up patch. > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> # > meson8b-odroidc1, sm1-x96-air > > [...] > > #define REG_MISC_AB 0x8 > > -#define MISC_B_CLK_EN BIT(23) > > -#define MISC_A_CLK_EN BIT(15) > > +#define MISC_B_CLK_EN 23 > > +#define MISC_A_CLK_EN 15 > > #define MISC_CLK_DIV_MASK 0x7f > > #define MISC_B_CLK_DIV_SHIFT 16 > > #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_MASK 0x3 > > -#define MISC_B_EN BIT(1) > > -#define MISC_A_EN BIT(0) > > +#define MISC_B_EN 1 > > +#define MISC_A_EN 0 > Personally I'm fine with this change but it's not how I would have done it: > - I would have kept the BIT() macro for MISC_{A,B}_EN > - then I would have renamed MISC_{A,}_CLK_EN to > MISC_{A,B}_CLK_EN_SHIFT (to be consistent with _SHIFT of the mux and > divider) and drop the BIT() macro there (like you did) > > This is possibly/likely personal preference, so my suggestion is to > wait for some more feedback. It looks like these aren't used outside the meson_pwm_per_channel_data array, so why bother with a #define (and any potential inconsistencies) in the first place? Thierry
On 13.04.2023 11:06, Thierry Reding wrote: > On Wed, Apr 12, 2023 at 10:47:05PM +0200, Martin Blumenstingl wrote: >> Hi Heiner, >> >> On Wed, Apr 12, 2023 at 9:23 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >>> >>> Change single-bit values from mask to bit. This facilitates >>> CCF initialization for the clock gate in a follow-up patch. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> # >> meson8b-odroidc1, sm1-x96-air >> >> [...] >>> #define REG_MISC_AB 0x8 >>> -#define MISC_B_CLK_EN BIT(23) >>> -#define MISC_A_CLK_EN BIT(15) >>> +#define MISC_B_CLK_EN 23 >>> +#define MISC_A_CLK_EN 15 >>> #define MISC_CLK_DIV_MASK 0x7f >>> #define MISC_B_CLK_DIV_SHIFT 16 >>> #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_MASK 0x3 >>> -#define MISC_B_EN BIT(1) >>> -#define MISC_A_EN BIT(0) >>> +#define MISC_B_EN 1 >>> +#define MISC_A_EN 0 >> Personally I'm fine with this change but it's not how I would have done it: >> - I would have kept the BIT() macro for MISC_{A,B}_EN >> - then I would have renamed MISC_{A,}_CLK_EN to >> MISC_{A,B}_CLK_EN_SHIFT (to be consistent with _SHIFT of the mux and >> divider) and drop the BIT() macro there (like you did) >> >> This is possibly/likely personal preference, so my suggestion is to >> wait for some more feedback. > > It looks like these aren't used outside the meson_pwm_per_channel_data > array, so why bother with a #define (and any potential inconsistencies) > in the first place? > I think we follow a common pattern here and first define constants for all registers and bits/fields. Having the register layout defined in one place makes it easier to check it against the chip datasheet. However I'm not sure whether this is what you were referring to. > Thierry
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 2a86867c1..40a8709ff 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -49,16 +49,16 @@ #define PWM_HIGH_MASK GENMASK(31, 16) #define REG_MISC_AB 0x8 -#define MISC_B_CLK_EN BIT(23) -#define MISC_A_CLK_EN BIT(15) +#define MISC_B_CLK_EN 23 +#define MISC_A_CLK_EN 15 #define MISC_CLK_DIV_MASK 0x7f #define MISC_B_CLK_DIV_SHIFT 16 #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_MASK 0x3 -#define MISC_B_EN BIT(1) -#define MISC_A_EN BIT(0) +#define MISC_B_EN 1 +#define MISC_A_EN 0 #define MESON_NUM_PWMS 2 #define MESON_MAX_MUX_PARENTS 4 @@ -67,22 +67,22 @@ 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; + u8 clk_en_bit; + u8 pwm_en_bit; } 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, + .clk_en_bit = MISC_A_CLK_EN, + .pwm_en_bit = 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, + .clk_en_bit = MISC_B_CLK_EN, + .pwm_en_bit = MISC_B_EN, } }; @@ -231,7 +231,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm) value = readl(meson->base + REG_MISC_AB); 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; + value |= BIT(channel_data->clk_en_bit); writel(value, meson->base + REG_MISC_AB); value = FIELD_PREP(PWM_HIGH_MASK, channel->hi) | @@ -239,7 +239,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm) writel(value, meson->base + channel_data->reg_offset); value = readl(meson->base + REG_MISC_AB); - value |= channel_data->pwm_en_mask; + value |= BIT(channel_data->pwm_en_bit); writel(value, meson->base + REG_MISC_AB); spin_unlock_irqrestore(&meson->lock, flags); @@ -253,7 +253,7 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm) spin_lock_irqsave(&meson->lock, flags); value = readl(meson->base + REG_MISC_AB); - value &= ~meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_mask; + value &= ~BIT(meson_pwm_per_channel_data[pwm->hwpwm].pwm_en_bit); writel(value, meson->base + REG_MISC_AB); spin_unlock_irqrestore(&meson->lock, flags); @@ -335,7 +335,7 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, value = readl(meson->base + REG_MISC_AB); - tmp = channel_data->pwm_en_mask | channel_data->clk_en_mask; + tmp = BIT(channel_data->pwm_en_bit) | BIT(channel_data->clk_en_bit); state->enabled = (value & tmp) == tmp; tmp = value >> channel_data->clk_div_shift;
Change single-bit values from mask to bit. This facilitates CCF initialization for the clock gate in a follow-up patch. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/pwm/pwm-meson.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)