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
