| 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
| ||
|
Message-ID: <ydvitggvp63thed3gs3svktgyxfzzh44tcdejcqro7pl6t6trt@7xo6krgii44o>
Date: Tue, 13 May 2025 11:18:07 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: "Rafael V. Volkmer" <rafael.v.volkmer@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v4 1/4] pwm: tiehrpwm: replace manual bit definitions
with bitfield.h macros
On Sat, Apr 19, 2025 at 04:48:35PM -0300, Rafael V. Volkmer wrote:
> Simplify bit manipulation by replacing manual BIT(x) definitions with
> GENMASK() and FIELD_PREP() from <linux/bitfield.h>. This improves
> readability, consistency, and aligns with modern kernel practices
> while preserving existing functionality.
>
> Additionally, update set_prescale_div() to use FIELD_PREP() for
> TBCTL_CLKDIV_MASK and TBCTL_HSPCLKDIV_MASK instead of manually
> shifting bits. This makes the code more maintainable and avoids
> potential errors in bit field assignments.
>
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@...il.com>
> ---
> drivers/pwm/pwm-tiehrpwm.c | 191 ++++++++++++++++++++++---------------
> 1 file changed, 114 insertions(+), 77 deletions(-)
I reorder the patch a bit with the intend to not change semantics but to
make my points a bit clearer.
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 0125e73b98df..1ead1aa91a1a 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -13,85 +13,122 @@
> #include <linux/clk.h>
> #include <linux/pm_runtime.h>
> #include <linux/of.h>
> +#include <linux/bitfield.h>
>
> /* EHRPWM registers and bits definitions */
>
> /* Time base module registers */
> -#define TBCTL 0x00
> -#define TBPRD 0x0A
> +#define TBCTL 0x00
> +#define TBPRD 0x0A
It's unclear to me why you changed the indention here. While you might
consider that inconsistent as I hate indenting struct initializers, for
#defines I think it greatly improves readability. I'd also keep the
whitespace changes to a minimum to make the patch better readable.
> -#define TBCTL_PRDLD_MASK BIT(3)
> -#define TBCTL_PRDLD_SHDW 0
> -#define TBCTL_PRDLD_IMDT BIT(3)
> -#define TBCTL_CLKDIV_MASK (BIT(12) | BIT(11) | BIT(10) | BIT(9) | \
> - BIT(8) | BIT(7))
> -#define TBCTL_CTRMODE_MASK (BIT(1) | BIT(0))
> -#define TBCTL_CTRMODE_UP 0
> -#define TBCTL_CTRMODE_DOWN BIT(0)
> -#define TBCTL_CTRMODE_UPDOWN BIT(1)
> -#define TBCTL_CTRMODE_FREEZE (BIT(1) | BIT(0))
Huh, you reordered these?
> -#define TBCTL_HSPCLKDIV_SHIFT 7
> -#define TBCTL_CLKDIV_SHIFT 10
> -
> -#define CLKDIV_MAX 7
> -#define HSPCLKDIV_MAX 7
> -#define PERIOD_MAX 0xFFFF
> -
> -/* compare module registers */
> -#define CMPA 0x12
> -#define CMPB 0x14
> -
> -/* Action qualifier module registers */
> -#define AQCTLA 0x16
> -#define AQCTLB 0x18
> -#define AQSFRC 0x1A
> -#define AQCSFRC 0x1C
> -
> -#define AQCTL_CBU_MASK (BIT(9) | BIT(8))
> -#define AQCTL_CBU_FRCLOW BIT(8)
> -#define AQCTL_CBU_FRCHIGH BIT(9)
> -#define AQCTL_CBU_FRCTOGGLE (BIT(9) | BIT(8))
> -#define AQCTL_CAU_MASK (BIT(5) | BIT(4))
> -#define AQCTL_CAU_FRCLOW BIT(4)
> -#define AQCTL_CAU_FRCHIGH BIT(5)
> -#define AQCTL_CAU_FRCTOGGLE (BIT(5) | BIT(4))
> -#define AQCTL_PRD_MASK (BIT(3) | BIT(2))
> -#define AQCTL_PRD_FRCLOW BIT(2)
> -#define AQCTL_PRD_FRCHIGH BIT(3)
> -#define AQCTL_PRD_FRCTOGGLE (BIT(3) | BIT(2))
> -#define AQCTL_ZRO_MASK (BIT(1) | BIT(0))
> -#define AQCTL_ZRO_FRCLOW BIT(0)
> -#define AQCTL_ZRO_FRCHIGH BIT(1)
> -#define AQCTL_ZRO_FRCTOGGLE (BIT(1) | BIT(0))
> -
> -#define AQCTL_CHANA_POLNORMAL (AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | \
> - AQCTL_ZRO_FRCHIGH)
> -#define AQCTL_CHANA_POLINVERSED (AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | \
> - AQCTL_ZRO_FRCLOW)
> -#define AQCTL_CHANB_POLNORMAL (AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | \
> - AQCTL_ZRO_FRCHIGH)
> -#define AQCTL_CHANB_POLINVERSED (AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | \
> - AQCTL_ZRO_FRCLOW)
> -
> -#define AQSFRC_RLDCSF_MASK (BIT(7) | BIT(6))
> -#define AQSFRC_RLDCSF_ZRO 0
> -#define AQSFRC_RLDCSF_PRD BIT(6)
> -#define AQSFRC_RLDCSF_ZROPRD BIT(7)
> -#define AQSFRC_RLDCSF_IMDT (BIT(7) | BIT(6))
> -
> -#define AQCSFRC_CSFB_MASK (BIT(3) | BIT(2))
> -#define AQCSFRC_CSFB_FRCDIS 0
> -#define AQCSFRC_CSFB_FRCLOW BIT(2)
> -#define AQCSFRC_CSFB_FRCHIGH BIT(3)
> -#define AQCSFRC_CSFB_DISSWFRC (BIT(3) | BIT(2))
> -#define AQCSFRC_CSFA_MASK (BIT(1) | BIT(0))
> -#define AQCSFRC_CSFA_FRCDIS 0
> -#define AQCSFRC_CSFA_FRCLOW BIT(0)
> -#define AQCSFRC_CSFA_FRCHIGH BIT(1)
> -#define AQCSFRC_CSFA_DISSWFRC (BIT(1) | BIT(0))
> -
> -#define NUM_PWM_CHANNEL 2 /* EHRPWM channels */
> +/* TBCTL: CTRMODE field (bits [1:0]) */
> +#define TBCTL_CTRMODE_MASK GENMASK(1, 0)
> +#define TBCTL_CTRMODE_UP FIELD_PREP(TBCTL_CTRMODE_MASK, 0)
> +#define TBCTL_CTRMODE_DOWN FIELD_PREP(TBCTL_CTRMODE_MASK, 1)
> +#define TBCTL_CTRMODE_UPDOWN FIELD_PREP(TBCTL_CTRMODE_MASK, 2)
> +#define TBCTL_CTRMODE_FREEZE FIELD_PREP(TBCTL_CTRMODE_MASK, 3)
> +
> +/* TBCTL: PRDLD bit (bit [3]) */
> +#define TBCTL_PRDLD_MASK GENMASK(3, 3)
> +#define TBCTL_PRDLD_SHDW FIELD_PREP(TBCTL_PRDLD_MASK, 0) /* shadow load */
> +#define TBCTL_PRDLD_IMDT FIELD_PREP(TBCTL_PRDLD_MASK, 1) /* immediate */
> +
> +/* TBCTL: high‑speed prescale (bits [9:7]) */
> +#define TBCTL_HSPCLKDIV_MASK GENMASK(9, 7)
> +/* TBCTL: clock prescale (bits [12:10]) */
> +#define TBCTL_CLKDIV_MASK GENMASK(12, 10)
This is different than before. Please only change the way the constants
are defined and not their values.
> +
> +#define CLKDIV_MAX 7
> +#define HSPCLKDIV_MAX 7
> +#define PERIOD_MAX 0xFFFF
> +
> +/*
> + * ----------------------------------------------------------------
> + * Compare module registers
> + * ----------------------------------------------------------------
> + */
> +#define CMPA 0x12
> +#define CMPB 0x14
> +
> +/*
> + * ----------------------------------------------------------------
> + * Action Qualifier (AQ) module registers
> + * ----------------------------------------------------------------
> + */
> +#define AQCTLA 0x16
> +#define AQCTLB 0x18
> +#define AQSFRC 0x1A
> +#define AQCSFRC 0x1
> +
> +/* AQCTL: event-action fields */
> +/* ZRO = bits [1:0] */
> +/* PRD = bits [3:2] */
> +/* CAU = bits [5:4] */
> +/* CBU = bits [9:8] */
> +#define AQCTL_ZRO_MASK GENMASK(1, 0)
> +#define AQCTL_PRD_MASK GENMASK(3, 2)
> +#define AQCTL_CAU_MASK GENMASK(5, 4)
> +#define AQCTL_CBU_MASK GENMASK(9, 8)
> +
> +/* common action codes (2‑bit) */
> +#define AQCTL_FRCLOW 1
> +#define AQCTL_FRCHIGH 2
> +#define AQCTL_FRCTOGGLE 3
> +
> +/* ZRO actions */
> +#define AQCTL_ZRO_FRCLOW FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCLOW)
> +#define AQCTL_ZRO_FRCHIGH FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCHIGH)
> +#define AQCTL_ZRO_FRCTOGGLE FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCTOGGLE)
> +
> +/* PRD actions */
> +#define AQCTL_PRD_FRCLOW FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCLOW)
> +#define AQCTL_PRD_FRCHIGH FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCHIGH)
> +#define AQCTL_PRD_FRCTOGGLE FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCTOGGLE)
> +
> +/* CAU actions */
> +#define AQCTL_CAU_FRCLOW FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCLOW)
> +#define AQCTL_CAU_FRCHIGH FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCHIGH)
> +#define AQCTL_CAU_FRCTOGGLE FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCTOGGLE)
> +
> +/* CBU actions */
> +#define AQCTL_CBU_FRCLOW FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCLOW)
> +#define AQCTL_CBU_FRCHIGH FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCHIGH)
> +#define AQCTL_CBU_FRCTOGGLE FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCTOGGLE)
> +
> +/* predefined channel‑polarity combos */
> +#define AQCTL_CHANA_POLNORMAL \
> + (AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH)
> +#define AQCTL_CHANA_POLINVERSED \
> + (AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | AQCTL_ZRO_FRCLOW)
> +
> +#define AQCTL_CHANB_POLNORMAL \
> + (AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH)
> +#define AQCTL_CHANB_POLINVERSED \
> + (AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | AQCTL_ZRO_FRCLOW)
> +
> +/* AQSFRC: RLDCSF (bits [7:6]) */
> +#define AQSFRC_RLDCSF_MASK GENMASK(7, 6)
> +#define AQSFRC_RLDCSF_ZRO FIELD_PREP(AQSFRC_RLDCSF_MASK, 0)
> +#define AQSFRC_RLDCSF_PRD FIELD_PREP(AQSFRC_RLDCSF_MASK, 1)
> +#define AQSFRC_RLDCSF_ZROPRD FIELD_PREP(AQSFRC_RLDCSF_MASK, 2)
> +#define AQSFRC_RLDCSF_IMDT FIELD_PREP(AQSFRC_RLDCSF_MASK, 3)
> +
> +/* AQCSFRC: CSFB (bits [3:2]), CSFA (bits [1:0]) */
> +#define AQCSFRC_CSFB_MASK GENMASK(3, 2)
> +#define AQCSFRC_CSFA_MASK GENMASK(1, 0)
> +
> +#define AQCSFRC_CSFB_FRCDIS FIELD_PREP(AQCSFRC_CSFB_MASK, 0)
> +#define AQCSFRC_CSFB_FRCLOW FIELD_PREP(AQCSFRC_CSFB_MASK, 1)
> +#define AQCSFRC_CSFB_FRCHIGH FIELD_PREP(AQCSFRC_CSFB_MASK, 2)
> +#define AQCSFRC_CSFB_DISSWFRC FIELD_PREP(AQCSFRC_CSFB_MASK, 3)
> +
> +#define AQCSFRC_CSFA_FRCDIS FIELD_PREP(AQCSFRC_CSFA_MASK, 0)
> +#define AQCSFRC_CSFA_FRCLOW FIELD_PREP(AQCSFRC_CSFA_MASK, 1)
> +#define AQCSFRC_CSFA_FRCHIGH FIELD_PREP(AQCSFRC_CSFA_MASK, 2)
> +#define AQCSFRC_CSFA_DISSWFRC FIELD_PREP(AQCSFRC_CSFA_MASK, 3)
> +
> +/* Number of EHRPWM channels */
> +#define NUM_PWM_CHANNEL 2U
That U is quite useless, a plain 2 does well.
> struct ehrpwm_context {
> u16 tbctl;
> @@ -167,8 +204,8 @@ static int set_prescale_div(unsigned long rqst_prescaler, u16 *prescale_div,
> *prescale_div = (1 << clkdiv) *
> (hspclkdiv ? (hspclkdiv * 2) : 1);
> if (*prescale_div > rqst_prescaler) {
> - *tb_clk_div = (clkdiv << TBCTL_CLKDIV_SHIFT) |
> - (hspclkdiv << TBCTL_HSPCLKDIV_SHIFT);
> + *tb_clk_div = FIELD_PREP(TBCTL_CLKDIV_MASK, clkdiv) |
> + FIELD_PREP(TBCTL_HSPCLKDIV_MASK, hspclkdiv);
I'd like to have this change in a separate patch. Is it obvious that the
new variant has the same semantic as the old? Or does this introduce
changes for some inputs?
Best regards
Uwe
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists