On Wed, Jul 10, 2024 at 11:36:11AM +0100, André Draszik wrote:
> All this open-coded shifting and masking is quite hard to read, in
> particular the if-statement in tcpci_apply_rc().
>
> Declare TCPC_ROLE_CTRL_CC[12] using GENMASK() which allows using
> FIELD_GET() and FIELD_PREP() to arguably make the code more legible.
>
> Signed-off-by: André Draszik <andre.draszik@xxxxxxxxxx>
Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/typec/anx7411.c | 5 ++-
> drivers/usb/typec/tcpm/tcpci.c | 73 +++++++++++++++-------------------
> drivers/usb/typec/tcpm/tcpci_rt1711h.c | 8 ++--
> include/linux/usb/tcpci.h | 9 ++---
> 4 files changed, 43 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/usb/typec/anx7411.c b/drivers/usb/typec/anx7411.c
> index b12a07edc71b..78b0d856cfc1 100644
> --- a/drivers/usb/typec/anx7411.c
> +++ b/drivers/usb/typec/anx7411.c
> @@ -6,6 +6,7 @@
> * Copyright(c) 2022, Analogix Semiconductor. All rights reserved.
> *
> */
> +#include <linux/bitfield.h>
> #include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/interrupt.h>
> @@ -884,8 +885,8 @@ static void anx7411_chip_standby(struct anx7411_data *ctx)
> OCM_RESET);
> ret |= anx7411_reg_write(ctx->tcpc_client, ANALOG_CTRL_10, 0x80);
> /* Set TCPC to RD and DRP enable */
> - cc1 = TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT;
> - cc2 = TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT;
> + cc1 = FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RD);
> + cc2 = FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RD);
> ret |= anx7411_reg_write(ctx->tcpc_client, TCPC_ROLE_CTRL,
> TCPC_ROLE_CTRL_DRP | cc1 | cc2);
>
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index ce11a154c7dc..cd71ece7b956 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -104,45 +104,42 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>
> switch (cc) {
> case TYPEC_CC_RA:
> - reg = (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg = (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RA)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RA));
> break;
> case TYPEC_CC_RD:
> - reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg = (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RD)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RD));
> break;
> case TYPEC_CC_RP_DEF:
> - reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) |
> - (TCPC_ROLE_CTRL_RP_VAL_DEF <<
> - TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> + reg = (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RP)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RP)
> + | (TCPC_ROLE_CTRL_RP_VAL_DEF << TCPC_ROLE_CTRL_RP_VAL_SHIFT));
> break;
> case TYPEC_CC_RP_1_5:
> - reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) |
> - (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
> - TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> + reg = (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RP)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RP)
> + | (TCPC_ROLE_CTRL_RP_VAL_1_5 << TCPC_ROLE_CTRL_RP_VAL_SHIFT));
> break;
> case TYPEC_CC_RP_3_0:
> - reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) |
> - (TCPC_ROLE_CTRL_RP_VAL_3_0 <<
> - TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> + reg = (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RP)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RP)
> + | (TCPC_ROLE_CTRL_RP_VAL_3_0 << TCPC_ROLE_CTRL_RP_VAL_SHIFT));
> break;
> case TYPEC_CC_OPEN:
> default:
> - reg = (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg = (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_OPEN)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_OPEN));
> break;
> }
>
> if (vconn_pres) {
> if (polarity == TYPEC_POLARITY_CC2) {
> - reg &= ~(TCPC_ROLE_CTRL_CC1_MASK << TCPC_ROLE_CTRL_CC1_SHIFT);
> - reg |= (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT);
> + reg &= ~TCPC_ROLE_CTRL_CC1;
> + reg |= FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_OPEN);
> } else {
> - reg &= ~(TCPC_ROLE_CTRL_CC2_MASK << TCPC_ROLE_CTRL_CC2_SHIFT);
> - reg |= (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg &= ~TCPC_ROLE_CTRL_CC2;
> + reg |= FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_OPEN);
> }
> }
>
> @@ -168,15 +165,11 @@ static int tcpci_apply_rc(struct tcpc_dev *tcpc, enum typec_cc_status cc,
> * APPLY_RC state is when ROLE_CONTROL.CC1 != ROLE_CONTROL.CC2 and vbus autodischarge on
> * disconnect is disabled. Bail out when ROLE_CONTROL.CC1 != ROLE_CONTROL.CC2.
> */
> - if (((reg & (TCPC_ROLE_CTRL_CC2_MASK << TCPC_ROLE_CTRL_CC2_SHIFT)) >>
> - TCPC_ROLE_CTRL_CC2_SHIFT) !=
> - ((reg & (TCPC_ROLE_CTRL_CC1_MASK << TCPC_ROLE_CTRL_CC1_SHIFT)) >>
> - TCPC_ROLE_CTRL_CC1_SHIFT))
> + if (FIELD_GET(TCPC_ROLE_CTRL_CC2, reg) != FIELD_GET(TCPC_ROLE_CTRL_CC1, reg))
> return 0;
>
> return regmap_update_bits(tcpci->regmap, TCPC_ROLE_CTRL, polarity == TYPEC_POLARITY_CC1 ?
> - TCPC_ROLE_CTRL_CC2_MASK << TCPC_ROLE_CTRL_CC2_SHIFT :
> - TCPC_ROLE_CTRL_CC1_MASK << TCPC_ROLE_CTRL_CC1_SHIFT,
> + TCPC_ROLE_CTRL_CC2 : TCPC_ROLE_CTRL_CC1,
> TCPC_ROLE_CTRL_CC_OPEN);
> }
>
> @@ -215,11 +208,11 @@ static int tcpci_start_toggling(struct tcpc_dev *tcpc,
> }
>
> if (cc == TYPEC_CC_RD)
> - reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg |= (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RD)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RD));
> else
> - reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg |= (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RP)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RP));
> ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> if (ret < 0)
> return ret;
> @@ -281,28 +274,28 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
> reg = reg & ~TCPC_ROLE_CTRL_DRP;
>
> if (polarity == TYPEC_POLARITY_CC2) {
> - reg &= ~(TCPC_ROLE_CTRL_CC2_MASK << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg &= ~TCPC_ROLE_CTRL_CC2;
> /* Local port is source */
> if (cc2 == TYPEC_CC_RD)
> /* Role control would have the Rp setting when DRP was enabled */
> - reg |= TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT;
> + reg |= FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RP);
> else
> - reg |= TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT;
> + reg |= FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RD);
> } else {
> - reg &= ~(TCPC_ROLE_CTRL_CC1_MASK << TCPC_ROLE_CTRL_CC1_SHIFT);
> + reg &= ~TCPC_ROLE_CTRL_CC1;
> /* Local port is source */
> if (cc1 == TYPEC_CC_RD)
> /* Role control would have the Rp setting when DRP was enabled */
> - reg |= TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT;
> + reg |= FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RP);
> else
> - reg |= TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT;
> + reg |= FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RD);
> }
> }
>
> if (polarity == TYPEC_POLARITY_CC2)
> - reg |= TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT;
> + reg |= FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_OPEN);
> else
> - reg |= TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC2_SHIFT;
> + reg |= FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_OPEN);
> ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> if (ret < 0)
> return ret;
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index c6dbccf6b17a..bdb78d08b5b5 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -246,11 +246,11 @@ static int rt1711h_start_drp_toggling(struct tcpci *tcpci,
> }
>
> if (cc == TYPEC_CC_RD)
> - reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg |= (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RD)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RD));
> else
> - reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
> - (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT);
> + reg |= (FIELD_PREP(TCPC_ROLE_CTRL_CC1, TCPC_ROLE_CTRL_CC_RP)
> + | FIELD_PREP(TCPC_ROLE_CTRL_CC2, TCPC_ROLE_CTRL_CC_RP));
>
> ret = rt1711h_write8(chip, TCPC_ROLE_CTRL, reg);
> if (ret < 0)
> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> index 31d21ccf662b..552d074429f0 100644
> --- a/include/linux/usb/tcpci.h
> +++ b/include/linux/usb/tcpci.h
> @@ -68,10 +68,8 @@
> #define TCPC_ROLE_CTRL_RP_VAL_DEF 0x0
> #define TCPC_ROLE_CTRL_RP_VAL_1_5 0x1
> #define TCPC_ROLE_CTRL_RP_VAL_3_0 0x2
> -#define TCPC_ROLE_CTRL_CC2_SHIFT 2
> -#define TCPC_ROLE_CTRL_CC2_MASK 0x3
> -#define TCPC_ROLE_CTRL_CC1_SHIFT 0
> -#define TCPC_ROLE_CTRL_CC1_MASK 0x3
> +#define TCPC_ROLE_CTRL_CC2 GENMASK(3, 2)
> +#define TCPC_ROLE_CTRL_CC1 GENMASK(1, 0)
> #define TCPC_ROLE_CTRL_CC_RA 0x0
> #define TCPC_ROLE_CTRL_CC_RP 0x1
> #define TCPC_ROLE_CTRL_CC_RD 0x2
> @@ -176,8 +174,7 @@
>
> #define tcpc_presenting_rd(reg, cc) \
> (!(TCPC_ROLE_CTRL_DRP & (reg)) && \
> - (((reg) & (TCPC_ROLE_CTRL_## cc ##_MASK << TCPC_ROLE_CTRL_## cc ##_SHIFT)) == \
> - (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_## cc ##_SHIFT)))
> + FIELD_GET(TCPC_ROLE_CTRL_## cc, reg) == TCPC_ROLE_CTRL_CC_RD)
>
> struct tcpci;
>
>
> --
> 2.45.2.803.g4e1b14247a-goog
--
heikki
