Re: [PATCH 03/15] usb: typec: tcpci: use GENMASK() for TCPC_CC_STATUS_CC[12]

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:10AM +0100, André Draszik wrote:
> The existing code here, particularly in maxim_contaminant.c, is
> arguably quite hard to read due to the open-coded masking and shifting
> spanning multiple lines.
> 
> Use GENMASK() and FIELD_GET() instead, which arguably make the code
> much easier to follow.
> 
> While at it, use the symbolic name TCPC_CC_STATE_SRC_OPEN for one
> instance of open-coded 0x0.
> 
> Signed-off-by: André Draszik <andre.draszik@xxxxxxxxxx>

Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

> ---
>  drivers/usb/typec/tcpm/maxim_contaminant.c | 8 +++-----
>  drivers/usb/typec/tcpm/tcpci.c             | 7 +++----
>  drivers/usb/typec/tcpm/tcpci_rt1711h.c     | 7 +++----
>  include/linux/usb/tcpci.h                  | 8 +++-----
>  4 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
> index f8504a90da26..e7687aeb69c0 100644
> --- a/drivers/usb/typec/tcpm/maxim_contaminant.c
> +++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
> @@ -5,6 +5,7 @@
>   * USB-C module to reduce wakeups due to contaminants.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/device.h>
>  #include <linux/irqreturn.h>
>  #include <linux/module.h>
> @@ -48,11 +49,8 @@ enum fladc_select {
>  #define STATUS_CHECK(reg, mask, val)	(((reg) & (mask)) == (val))
>  
>  #define IS_CC_OPEN(cc_status) \
> -	(STATUS_CHECK((cc_status), TCPC_CC_STATUS_CC1_MASK << TCPC_CC_STATUS_CC1_SHIFT,  \
> -		      TCPC_CC_STATE_SRC_OPEN) && STATUS_CHECK((cc_status),               \
> -							      TCPC_CC_STATUS_CC2_MASK << \
> -							      TCPC_CC_STATUS_CC2_SHIFT,  \
> -							      TCPC_CC_STATE_SRC_OPEN))
> +	(FIELD_GET(TCPC_CC_STATUS_CC1, cc_status) == TCPC_CC_STATE_SRC_OPEN \
> +	 && FIELD_GET(TCPC_CC_STATUS_CC2, cc_status) == TCPC_CC_STATE_SRC_OPEN)
>  
>  static int max_contaminant_adc_to_mv(struct max_tcpci_chip *chip, enum fladc_select channel,
>  				     bool ua_src, u8 fladc)
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 8a18d561b063..ce11a154c7dc 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -5,6 +5,7 @@
>   * USB Type-C Port Controller Interface.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -241,12 +242,10 @@ static int tcpci_get_cc(struct tcpc_dev *tcpc,
>  	if (ret < 0)
>  		return ret;
>  
> -	*cc1 = tcpci_to_typec_cc((reg >> TCPC_CC_STATUS_CC1_SHIFT) &
> -				 TCPC_CC_STATUS_CC1_MASK,
> +	*cc1 = tcpci_to_typec_cc(FIELD_GET(TCPC_CC_STATUS_CC1, reg),
>  				 reg & TCPC_CC_STATUS_TERM ||
>  				 tcpc_presenting_rd(role_control, CC1));
> -	*cc2 = tcpci_to_typec_cc((reg >> TCPC_CC_STATUS_CC2_SHIFT) &
> -				 TCPC_CC_STATUS_CC2_MASK,
> +	*cc2 = tcpci_to_typec_cc(FIELD_GET(TCPC_CC_STATUS_CC2, reg),
>  				 reg & TCPC_CC_STATUS_TERM ||
>  				 tcpc_presenting_rd(role_control, CC2));
>  
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index 67422d45eb54..c6dbccf6b17a 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -5,6 +5,7 @@
>   * Richtek RT1711H Type-C Chip Driver
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/bits.h>
>  #include <linux/kernel.h>
>  #include <linux/mod_devicetable.h>
> @@ -195,12 +196,10 @@ static inline int rt1711h_init_cc_params(struct rt1711h_chip *chip, u8 status)
>  	if (ret < 0)
>  		return ret;
>  
> -	cc1 = tcpci_to_typec_cc((status >> TCPC_CC_STATUS_CC1_SHIFT) &
> -				TCPC_CC_STATUS_CC1_MASK,
> +	cc1 = tcpci_to_typec_cc(FIELD_GET(TCPC_CC_STATUS_CC1, status),
>  				status & TCPC_CC_STATUS_TERM ||
>  				tcpc_presenting_rd(role, CC1));
> -	cc2 = tcpci_to_typec_cc((status >> TCPC_CC_STATUS_CC2_SHIFT) &
> -				TCPC_CC_STATUS_CC2_MASK,
> +	cc2 = tcpci_to_typec_cc(FIELD_GET(TCPC_CC_STATUS_CC2, status),
>  				status & TCPC_CC_STATUS_TERM ||
>  				tcpc_presenting_rd(role, CC2));
>  
> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> index d27fe0c22a8b..31d21ccf662b 100644
> --- a/include/linux/usb/tcpci.h
> +++ b/include/linux/usb/tcpci.h
> @@ -92,11 +92,9 @@
>  #define TCPC_CC_STATUS_TERM		BIT(4)
>  #define TCPC_CC_STATUS_TERM_RP		0
>  #define TCPC_CC_STATUS_TERM_RD		1
> +#define TCPC_CC_STATUS_CC2		GENMASK(3, 2)
> +#define TCPC_CC_STATUS_CC1		GENMASK(1, 0)
>  #define TCPC_CC_STATE_SRC_OPEN		0
> -#define TCPC_CC_STATUS_CC2_SHIFT	2
> -#define TCPC_CC_STATUS_CC2_MASK		0x3
> -#define TCPC_CC_STATUS_CC1_SHIFT	0
> -#define TCPC_CC_STATUS_CC1_MASK		0x3
>  
>  #define TCPC_POWER_STATUS		0x1e
>  #define TCPC_POWER_STATUS_DBG_ACC_CON	BIT(7)
> @@ -256,7 +254,7 @@ static inline enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
>  		if (sink)
>  			return TYPEC_CC_RP_3_0;
>  		fallthrough;
> -	case 0x0:
> +	case TCPC_CC_STATE_SRC_OPEN:
>  	default:
>  		return TYPEC_CC_OPEN;
>  	}
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki