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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <w3igi2jmva6mfa7anlieyp3iiwfzhsvi3t37wwcqqtzdy42fqn@btmdsfsmpw7r>
Date: Mon, 4 Nov 2024 10:32:06 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: George Stark <gnstark@...utedevices.com>
Cc: neil.armstrong@...aro.org, khilman@...libre.com, jbrunet@...libre.com, 
	martin.blumenstingl@...glemail.com, linux-pwm@...r.kernel.org, linux-amlogic@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, kernel@...utedevices.com
Subject: Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits

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.

On Wed, Oct 16, 2024 at 06:25:51PM +0300, George Stark wrote:
> Newer meson PWM IPs support constant and polarity bits. Support them to
> correctly implement constant and inverted output levels.
> 
> Using constant bit allows to have truly stable low or high output level.
> Since hi and low regs internally increment its values by 1 just writing
> zero to any of them gives 1 clock count impulse. If constant bit is set
> zero value in hi and low regs is not incremented.
> 
> Using polarity bit instead of swapping hi and low reg values allows to
> correctly identify inversion in .get_state().
> 
> Signed-off-by: George Stark <gnstark@...utedevices.com>
> ---
>  drivers/pwm/pwm-meson.c | 63 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 56 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 2ef632caebcc..974c3c74768c 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -6,7 +6,7 @@
>   * PWM output is achieved by calculating a clock that permits calculating
>   * two periods (low and high). The counter then has to be set to switch after
>   * N cycles for the first half period.
> - * The hardware has no "polarity" setting. This driver reverses the period
> + * Partly the hardware has no "polarity" setting. This driver reverses the period
>   * cycles (the low length is inverted with the high length) for
>   * PWM_POLARITY_INVERSED. This means that .get_state cannot read the polarity
>   * from the hardware.
> @@ -56,6 +56,10 @@
>  #define MISC_B_CLK_SEL_SHIFT	6
>  #define MISC_A_CLK_SEL_SHIFT	4
>  #define MISC_CLK_SEL_MASK	0x3
> +#define MISC_B_CONSTANT_EN	BIT(29)
> +#define MISC_A_CONSTANT_EN	BIT(28)
> +#define MISC_B_INVERT_EN	BIT(27)
> +#define MISC_A_INVERT_EN	BIT(26)
>  #define MISC_B_EN		BIT(1)
>  #define MISC_A_EN		BIT(0)
>  
> @@ -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.

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

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)