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

[George Stark <gnstark@salutedevices.com>]

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