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.
Best regards
Uwe