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

[George Stark]

Hello Uwe

Thanks for the review.

On 11/4/24 12:32, Uwe Kleine-König wrote:
> 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.

I've been looking into this driver for more than a year and got used to
it so much so never thought about changing the foundations :) Although 
it's an interesting thought.

1. I took meson_pwm_enable() without
const patches and reimplemented it using only defines (e.g. w/o local
var channel_data) and objdumped current and new versions. New version
turned out to be one instruction longer (arm64, gcc, default -O2). So 
total difference in executable code may be not that significant although
we can win in C-code line count.

2. Things like
#define MISC_B_EN		BIT(1)
#define MISC_A_EN		BIT(0)
is more straightforward and can be matched to the datasheet easier
comparing to (a + b * (chan)) things.

So I'm not sure either.

>> @@ -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?
> 

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?

> Best regards
> Uwe

-- 
Best regards
George