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

[Uwe Kleine-König]

Hello George,

On Tue, Nov 19, 2024 at 03:51:34PM +0300, George Stark wrote:
> 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.

Usually writing zeros to unused bits is a safe procedure. If the
hardware we're talking to is a newer variant of the supported stuff, the
hardware engineer did something wrong if keeping the read bits or
writing them as zero is incompatible. So either way is fine for me.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20241119/26a0921d/attachment.sig>