Hello George,
there are two minor things I dislike in this patch/driver. But I'm not
sure the alternatives are objectively considerably better. See below and
judge yourself.
@@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
u8 clk_div_shift;
u8 clk_en_shift;
u32 pwm_en_mask;
+ u32 const_en_mask;
+ u32 inv_en_mask;
} meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
{
.reg_offset = REG_PWM_A,
@@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
.clk_div_shift = MISC_A_CLK_DIV_SHIFT,
.clk_en_shift = MISC_A_CLK_EN_SHIFT,
.pwm_en_mask = MISC_A_EN,
+ .const_en_mask = MISC_A_CONSTANT_EN,
+ .inv_en_mask = MISC_A_INVERT_EN,
},
{
.reg_offset = REG_PWM_B,
@@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
.clk_div_shift = MISC_B_CLK_DIV_SHIFT,
.clk_en_shift = MISC_B_CLK_EN_SHIFT,
.pwm_en_mask = MISC_B_EN,
+ .const_en_mask = MISC_B_CONSTANT_EN,
+ .inv_en_mask = MISC_B_INVERT_EN,
}
};
So the generic register description describes the const and invert bits,
but it doesn't apply to all IPs. Thinking about that, I wonder why this
struct exists at all. I would have done this as follows:
#define MESON_PWM_REG_PWM(chan) (0 + 4 * (chan))
#define MESON_PWM_REG_MISC (8)
#define MESON_PWM_REG_MISC_EN(chan) BIT(chan)
#define MESON_PWM_REG_MISC_CLK_SEL(chan) GENMASK(5 + 2 * (chan), 4 + 2 * (chan))
....
and then use these constants directly (with pwm->hwpwm as parameter if
needed) in the code. I would expect this to result in more efficient and
smaller code.
@@ -227,6 +252,15 @@ static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
value = readl(meson->base + REG_MISC_AB);
value |= channel_data->pwm_en_mask;
+
+ if (meson->data->has_constant)
+ meson_pwm_assign_bit(&value, channel_data->const_en_mask,
+ channel->constant);
Personally I'd prefer:
value &= ~MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
if (meson->data->has_constant && channel->constant)
value |= MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
even though your variant only mentions the mask once. While it has this
repetition, it's clear what happens without having to know what
meson_pwm_assign_bit() does. Maybe that's subjective?
Best regards
Uwe