Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits

From: George Stark
Date: Tue Nov 19 2024 - 07:51:59 EST


Hello Uwe

On 11/7/24 11:41, Uwe Kleine-König wrote:
On Wed, Nov 06, 2024 at 04:54:41PM +0300, George Stark wrote:
On 11/4/24 12:32, Uwe Kleine-König wrote:
@@ -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,
}
};

...

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?

Actually I also don't like meson_pwm_assign_bit() too match and I'm
surprised there's no something like this in the kernel already.
I again objdumped versions meson_pwm_assign_bit() vs double mask repetition.
Unconditional bit clearing takes only a single instruction:

// value &= ~channel_data->const_en_mask;
9ac: 0a250040 bic w0, w2, w5

So in the current series I could drop meson_pwm_assign_bit() and use:

value &= ~channel_data->const_en_mask;
if (meson->data->has_constant && channel->constant)
value |= channel_data->const_en_mask;

If it's decided now or later to drop meson_pwm_channel_data then
w\o meson_pwm_assign_bit() future patch will be line-to-line change.

What you think?

Sounds sensible.

While changing the patch to drop meson_pwm_assign_bit() one thing
concerned me on the approach:

value &= ~channel_data->const_en_mask;
if (meson->data->has_constant && channel->constant)
value |= channel_data->const_en_mask;

that we reset bit in the value var even if that bit is not supported on
the current SoC. I checked several datasheets for old SoCs and those bits are marked as unused (not even reserved) and I've never seen those
bits set. Still I'd offer to use precise condition for changing those bit. I'll send v3 let's discuss it again if you think I bother too much.


Best regards
Uwe

--
Best regards
George