[PATCH 00/15] usb: typec: tcpci: few TCPCi & TCPCi-Maxim cleanups (mostly genmask())

[PATCH 00/15] usb: typec: tcpci: few TCPCi & TCPCi-Maxim cleanups (mostly genmask())

[André Draszik]

While looking through the TCPCi drivers, it occurred to me that all of the
open-coded register bit manipulations can be simplified and made more
legible by using the standard GENMASK(), FIELD_GET(), and FIELD_PREP()
macros, which also is less prone to errors: e.g.

	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))

(arguably) is much harder to read and reason about than:

	if (FIELD_GET(TCPC_ROLE_CTRL_CC2, reg) != FIELD_GET(TCPC_ROLE_CTRL_CC1, reg))

and so on.

These patches do that. In addition, there are a few comment fixes and I
added in a conversion to using dev_err_probe() and
devm_add_action_or_reset() in the Maxim TCPCi driver.

I kept the patches separated by register or register field as appropriate to
simplify reviewing, allowing to focus on one field/register at a time.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
André Draszik (15):
      usb: typec: tcpci: fix a comment typo
      usb: typec: tcpm/tcpci_maxim: clarify a comment
      usb: typec: tcpci: use GENMASK() for TCPC_CC_STATUS_CC[12]
      usb: typec: tcpci: use GENMASK() for TCPC_ROLE_CTRL_CC[12]
      usb: typec: tcpci: use GENMASK() for TCPC_ROLE_CTRL_RP_VAL
      usb: typec: tcpci: use GENMASK() for TCPC_MSG_HDR_INFO_REV
      usb: typec: tcpci: use GENMASK() for TCPC_TRANSMIT register fields
      usb: typec: tcpm/tcpci_maxim: sort TCPC_ALERT_MASK values by bit
      usb: typec: tcpm/tcpci_maxim: simplify clearing of TCPC_ALERT_RX_BUF_OVF
      usb: typec: tcpm/tcpci_maxim: drop STATUS_CHECK()
      usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_CC_CTRL2 register
      usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_CC_CTRL3 register
      usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_ADC_CTRL1 register
      usb: typec: tcpm/tcpci_maxim: convert to dev_err_probe()
      usb: typec: tcpm/tcpci_maxim: use device managed TCPCI port deregistration

 drivers/usb/typec/anx7411.c                |   5 +-
 drivers/usb/typec/tcpm/maxim_contaminant.c |  46 +++++++------
 drivers/usb/typec/tcpm/tcpci.c             | 104 ++++++++++++++---------------
 drivers/usb/typec/tcpm/tcpci_maxim.h       |  18 ++---
 drivers/usb/typec/tcpm/tcpci_maxim_core.c  |  71 ++++++++++----------
 drivers/usb/typec/tcpm/tcpci_rt1711h.c     |  27 ++++----
 include/linux/usb/tcpci.h                  |  31 +++------
 7 files changed, 144 insertions(+), 158 deletions(-)
---
base-commit: 82d01fe6ee52086035b201cfa1410a3b04384257
change-id: 20240710-tcpc-cleanup-61e29124a87d

Best regards,
-- 
André Draszik <andre.draszik@linaro.org>

[PATCH 01/15] usb: typec: tcpci: fix a comment typo

[André Draszik]

autonously -> autonomously

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 include/linux/usb/tcpci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
index 0ab39b6ea205..d27fe0c22a8b 100644
--- a/include/linux/usb/tcpci.h
+++ b/include/linux/usb/tcpci.h
@@ -190,7 +190,7 @@ struct tcpci;
  *		Optional; Callback to perform chip specific operations when FRS
  *		is sourcing vbus.
  * @auto_discharge_disconnect:
- *		Optional; Enables TCPC to autonously discharge vbus on disconnect.
+ *		Optional; Enables TCPC to autonomously discharge vbus on disconnect.
  * @vbus_vsafe0v:
  *		optional; Set when TCPC can detect whether vbus is at VSAFE0V.
  * @set_partner_usb_comm_capable:

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 01/15] usb: typec: tcpci: fix a comment typo

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:08AM +0100, André Draszik wrote:
> autonously -> autonomously
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  include/linux/usb/tcpci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> index 0ab39b6ea205..d27fe0c22a8b 100644
> --- a/include/linux/usb/tcpci.h
> +++ b/include/linux/usb/tcpci.h
> @@ -190,7 +190,7 @@ struct tcpci;
>   *		Optional; Callback to perform chip specific operations when FRS
>   *		is sourcing vbus.
>   * @auto_discharge_disconnect:
> - *		Optional; Enables TCPC to autonously discharge vbus on disconnect.
> + *		Optional; Enables TCPC to autonomously discharge vbus on disconnect.
>   * @vbus_vsafe0v:
>   *		optional; Set when TCPC can detect whether vbus is at VSAFE0V.
>   * @set_partner_usb_comm_capable:
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 02/15] usb: typec: tcpm/tcpci_maxim: clarify a comment

[André Draszik]

We loop while the status is != 0, so rephrase the comment slightly for
clarity.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/tcpci_maxim_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
index eec3bcec119c..87102b4d060d 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
+++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
@@ -397,7 +397,7 @@ static irqreturn_t max_tcpci_irq(int irq, void *dev_id)
 	}
 	while (status) {
 		irq_return = _max_tcpci_irq(chip, status);
-		/* Do not return if the ALERT is already set. */
+		/* Do not return if a (new) ALERT is set (again). */
 		ret = max_tcpci_read16(chip, TCPC_ALERT, &status);
 		if (ret < 0)
 			break;

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 02/15] usb: typec: tcpm/tcpci_maxim: clarify a comment

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:09AM +0100, André Draszik wrote:
> We loop while the status is != 0, so rephrase the comment slightly for
> clarity.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpci_maxim_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> index eec3bcec119c..87102b4d060d 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> @@ -397,7 +397,7 @@ static irqreturn_t max_tcpci_irq(int irq, void *dev_id)
>  	}
>  	while (status) {
>  		irq_return = _max_tcpci_irq(chip, status);
> -		/* Do not return if the ALERT is already set. */
> +		/* Do not return if a (new) ALERT is set (again). */
>  		ret = max_tcpci_read16(chip, TCPC_ALERT, &status);
>  		if (ret < 0)
>  			break;
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

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

[André Draszik]

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@linaro.org>
---
 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

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@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

[PATCH 04/15] usb: typec: tcpci: use GENMASK() for TCPC_ROLE_CTRL_CC[12]

[André Draszik]

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@linaro.org>
---
 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

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

[Heikki Krogerus]

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@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

[PATCH 05/15] usb: typec: tcpci: use GENMASK() for TCPC_ROLE_CTRL_RP_VAL

[André Draszik]

Align the last remaining field TCPC_ROLE_CTRL_RP_VAL with the other
fields in the TCPC_ROLE_CTRL register by using GENMASK() and
FIELD_PREP().

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/tcpci.c         | 21 ++++++++++++---------
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 12 ++++++------
 include/linux/usb/tcpci.h              |  3 +--
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index cd71ece7b956..5ad05a5bbbd1 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -114,17 +114,20 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
 	case TYPEC_CC_RP_DEF:
 		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));
+		       | FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				    TCPC_ROLE_CTRL_RP_VAL_DEF));
 		break;
 	case TYPEC_CC_RP_1_5:
 		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));
+		       | FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				    TCPC_ROLE_CTRL_RP_VAL_1_5));
 		break;
 	case TYPEC_CC_RP_3_0:
 		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));
+		       | FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				    TCPC_ROLE_CTRL_RP_VAL_3_0));
 		break;
 	case TYPEC_CC_OPEN:
 	default:
@@ -194,16 +197,16 @@ static int tcpci_start_toggling(struct tcpc_dev *tcpc,
 	switch (cc) {
 	default:
 	case TYPEC_CC_RP_DEF:
-		reg |= (TCPC_ROLE_CTRL_RP_VAL_DEF <<
-			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				  TCPC_ROLE_CTRL_RP_VAL_DEF);
 		break;
 	case TYPEC_CC_RP_1_5:
-		reg |= (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
-			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				  TCPC_ROLE_CTRL_RP_VAL_1_5);
 		break;
 	case TYPEC_CC_RP_3_0:
-		reg |= (TCPC_ROLE_CTRL_RP_VAL_3_0 <<
-			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				  TCPC_ROLE_CTRL_RP_VAL_3_0);
 		break;
 	}
 
diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index bdb78d08b5b5..64f6dd0dc660 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -232,16 +232,16 @@ static int rt1711h_start_drp_toggling(struct tcpci *tcpci,
 	switch (cc) {
 	default:
 	case TYPEC_CC_RP_DEF:
-		reg |= (TCPC_ROLE_CTRL_RP_VAL_DEF <<
-			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				  TCPC_ROLE_CTRL_RP_VAL_DEF);
 		break;
 	case TYPEC_CC_RP_1_5:
-		reg |= (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
-			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				  TCPC_ROLE_CTRL_RP_VAL_1_5);
 		break;
 	case TYPEC_CC_RP_3_0:
-		reg |= (TCPC_ROLE_CTRL_RP_VAL_3_0 <<
-			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
+				  TCPC_ROLE_CTRL_RP_VAL_3_0);
 		break;
 	}
 
diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
index 552d074429f0..80652d4f722e 100644
--- a/include/linux/usb/tcpci.h
+++ b/include/linux/usb/tcpci.h
@@ -63,8 +63,7 @@
 
 #define TCPC_ROLE_CTRL			0x1a
 #define TCPC_ROLE_CTRL_DRP		BIT(6)
-#define TCPC_ROLE_CTRL_RP_VAL_SHIFT	4
-#define TCPC_ROLE_CTRL_RP_VAL_MASK	0x3
+#define TCPC_ROLE_CTRL_RP_VAL		GENMASK(5, 4)
 #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

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 05/15] usb: typec: tcpci: use GENMASK() for TCPC_ROLE_CTRL_RP_VAL

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:12AM +0100, André Draszik wrote:
> Align the last remaining field TCPC_ROLE_CTRL_RP_VAL with the other
> fields in the TCPC_ROLE_CTRL register by using GENMASK() and
> FIELD_PREP().
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpci.c         | 21 ++++++++++++---------
>  drivers/usb/typec/tcpm/tcpci_rt1711h.c | 12 ++++++------
>  include/linux/usb/tcpci.h              |  3 +--
>  3 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index cd71ece7b956..5ad05a5bbbd1 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -114,17 +114,20 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  	case TYPEC_CC_RP_DEF:
>  		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));
> +		       | FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				    TCPC_ROLE_CTRL_RP_VAL_DEF));
>  		break;
>  	case TYPEC_CC_RP_1_5:
>  		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));
> +		       | FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				    TCPC_ROLE_CTRL_RP_VAL_1_5));
>  		break;
>  	case TYPEC_CC_RP_3_0:
>  		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));
> +		       | FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				    TCPC_ROLE_CTRL_RP_VAL_3_0));
>  		break;
>  	case TYPEC_CC_OPEN:
>  	default:
> @@ -194,16 +197,16 @@ static int tcpci_start_toggling(struct tcpc_dev *tcpc,
>  	switch (cc) {
>  	default:
>  	case TYPEC_CC_RP_DEF:
> -		reg |= (TCPC_ROLE_CTRL_RP_VAL_DEF <<
> -			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> +		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				  TCPC_ROLE_CTRL_RP_VAL_DEF);
>  		break;
>  	case TYPEC_CC_RP_1_5:
> -		reg |= (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
> -			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> +		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				  TCPC_ROLE_CTRL_RP_VAL_1_5);
>  		break;
>  	case TYPEC_CC_RP_3_0:
> -		reg |= (TCPC_ROLE_CTRL_RP_VAL_3_0 <<
> -			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> +		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				  TCPC_ROLE_CTRL_RP_VAL_3_0);
>  		break;
>  	}
>  
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index bdb78d08b5b5..64f6dd0dc660 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -232,16 +232,16 @@ static int rt1711h_start_drp_toggling(struct tcpci *tcpci,
>  	switch (cc) {
>  	default:
>  	case TYPEC_CC_RP_DEF:
> -		reg |= (TCPC_ROLE_CTRL_RP_VAL_DEF <<
> -			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> +		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				  TCPC_ROLE_CTRL_RP_VAL_DEF);
>  		break;
>  	case TYPEC_CC_RP_1_5:
> -		reg |= (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
> -			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> +		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				  TCPC_ROLE_CTRL_RP_VAL_1_5);
>  		break;
>  	case TYPEC_CC_RP_3_0:
> -		reg |= (TCPC_ROLE_CTRL_RP_VAL_3_0 <<
> -			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> +		reg |= FIELD_PREP(TCPC_ROLE_CTRL_RP_VAL,
> +				  TCPC_ROLE_CTRL_RP_VAL_3_0);
>  		break;
>  	}
>  
> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> index 552d074429f0..80652d4f722e 100644
> --- a/include/linux/usb/tcpci.h
> +++ b/include/linux/usb/tcpci.h
> @@ -63,8 +63,7 @@
>  
>  #define TCPC_ROLE_CTRL			0x1a
>  #define TCPC_ROLE_CTRL_DRP		BIT(6)
> -#define TCPC_ROLE_CTRL_RP_VAL_SHIFT	4
> -#define TCPC_ROLE_CTRL_RP_VAL_MASK	0x3
> +#define TCPC_ROLE_CTRL_RP_VAL		GENMASK(5, 4)
>  #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
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 06/15] usb: typec: tcpci: use GENMASK() for TCPC_MSG_HDR_INFO_REV

[André Draszik]

Convert field TCPC_MSG_HDR_INFO_REV from register TCPC_MSG_HDR_INFO to
using GENMASK() and FIELD_PREP() so as to keep using a similar approach
for all fields.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/tcpci.c | 2 +-
 include/linux/usb/tcpci.h      | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 5ad05a5bbbd1..ad5c9d5bf6a9 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -456,7 +456,7 @@ static int tcpci_set_roles(struct tcpc_dev *tcpc, bool attached,
 	unsigned int reg;
 	int ret;
 
-	reg = PD_REV20 << TCPC_MSG_HDR_INFO_REV_SHIFT;
+	reg = FIELD_PREP(TCPC_MSG_HDR_INFO_REV, PD_REV20);
 	if (role == TYPEC_SOURCE)
 		reg |= TCPC_MSG_HDR_INFO_PWR_ROLE;
 	if (data == TYPEC_HOST)
diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
index 80652d4f722e..3cd61e9f73b3 100644
--- a/include/linux/usb/tcpci.h
+++ b/include/linux/usb/tcpci.h
@@ -129,9 +129,8 @@
 
 #define TCPC_MSG_HDR_INFO		0x2e
 #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
+#define TCPC_MSG_HDR_INFO_REV		GENMASK(2, 1)
 #define TCPC_MSG_HDR_INFO_PWR_ROLE	BIT(0)
-#define TCPC_MSG_HDR_INFO_REV_SHIFT	1
-#define TCPC_MSG_HDR_INFO_REV_MASK	0x3
 
 #define TCPC_RX_DETECT			0x2f
 #define TCPC_RX_DETECT_HARD_RESET	BIT(5)

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 06/15] usb: typec: tcpci: use GENMASK() for TCPC_MSG_HDR_INFO_REV

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:13AM +0100, André Draszik wrote:
> Convert field TCPC_MSG_HDR_INFO_REV from register TCPC_MSG_HDR_INFO to
> using GENMASK() and FIELD_PREP() so as to keep using a similar approach
> for all fields.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpci.c | 2 +-
>  include/linux/usb/tcpci.h      | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 5ad05a5bbbd1..ad5c9d5bf6a9 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -456,7 +456,7 @@ static int tcpci_set_roles(struct tcpc_dev *tcpc, bool attached,
>  	unsigned int reg;
>  	int ret;
>  
> -	reg = PD_REV20 << TCPC_MSG_HDR_INFO_REV_SHIFT;
> +	reg = FIELD_PREP(TCPC_MSG_HDR_INFO_REV, PD_REV20);
>  	if (role == TYPEC_SOURCE)
>  		reg |= TCPC_MSG_HDR_INFO_PWR_ROLE;
>  	if (data == TYPEC_HOST)
> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> index 80652d4f722e..3cd61e9f73b3 100644
> --- a/include/linux/usb/tcpci.h
> +++ b/include/linux/usb/tcpci.h
> @@ -129,9 +129,8 @@
>  
>  #define TCPC_MSG_HDR_INFO		0x2e
>  #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
> +#define TCPC_MSG_HDR_INFO_REV		GENMASK(2, 1)
>  #define TCPC_MSG_HDR_INFO_PWR_ROLE	BIT(0)
> -#define TCPC_MSG_HDR_INFO_REV_SHIFT	1
> -#define TCPC_MSG_HDR_INFO_REV_MASK	0x3
>  
>  #define TCPC_RX_DETECT			0x2f
>  #define TCPC_RX_DETECT_HARD_RESET	BIT(5)
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 07/15] usb: typec: tcpci: use GENMASK() for TCPC_TRANSMIT register fields

[André Draszik]

Convert all fields from register TCPC_TRANSMIT to using GENMASK() and
FIELD_PREP() so as to keep using a similar approach throughout the code
base and make it arguably easier to read.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/tcpci.c | 7 +++++--
 include/linux/usb/tcpci.h      | 6 ++----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index ad5c9d5bf6a9..b9ee9ccff99b 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -607,8 +607,11 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc, enum tcpm_transmit_type type
 	}
 
 	/* nRetryCount is 3 in PD2.0 spec where 2 in PD3.0 spec */
-	reg = ((negotiated_rev > PD_REV20 ? PD_RETRY_COUNT_3_0_OR_HIGHER : PD_RETRY_COUNT_DEFAULT)
-	       << TCPC_TRANSMIT_RETRY_SHIFT) | (type << TCPC_TRANSMIT_TYPE_SHIFT);
+	reg = FIELD_PREP(TCPC_TRANSMIT_RETRY,
+			 (negotiated_rev > PD_REV20
+			  ? PD_RETRY_COUNT_3_0_OR_HIGHER
+			  : PD_RETRY_COUNT_DEFAULT));
+	reg |= FIELD_PREP(TCPC_TRANSMIT_TYPE, type);
 	ret = regmap_write(tcpci->regmap, TCPC_TRANSMIT, reg);
 	if (ret < 0)
 		return ret;
diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
index 3cd61e9f73b3..f7f5cfbdef12 100644
--- a/include/linux/usb/tcpci.h
+++ b/include/linux/usb/tcpci.h
@@ -148,10 +148,8 @@
 #define TCPC_RX_DATA			0x34 /* through 0x4f */
 
 #define TCPC_TRANSMIT			0x50
-#define TCPC_TRANSMIT_RETRY_SHIFT	4
-#define TCPC_TRANSMIT_RETRY_MASK	0x3
-#define TCPC_TRANSMIT_TYPE_SHIFT	0
-#define TCPC_TRANSMIT_TYPE_MASK		0x7
+#define TCPC_TRANSMIT_RETRY		GENMASK(5, 4)
+#define TCPC_TRANSMIT_TYPE		GENMASK(2, 0)
 
 #define TCPC_TX_BYTE_CNT		0x51
 #define TCPC_TX_HDR			0x52

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 07/15] usb: typec: tcpci: use GENMASK() for TCPC_TRANSMIT register fields

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:14AM +0100, André Draszik wrote:
> Convert all fields from register TCPC_TRANSMIT to using GENMASK() and
> FIELD_PREP() so as to keep using a similar approach throughout the code
> base and make it arguably easier to read.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpci.c | 7 +++++--
>  include/linux/usb/tcpci.h      | 6 ++----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index ad5c9d5bf6a9..b9ee9ccff99b 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -607,8 +607,11 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc, enum tcpm_transmit_type type
>  	}
>  
>  	/* nRetryCount is 3 in PD2.0 spec where 2 in PD3.0 spec */
> -	reg = ((negotiated_rev > PD_REV20 ? PD_RETRY_COUNT_3_0_OR_HIGHER : PD_RETRY_COUNT_DEFAULT)
> -	       << TCPC_TRANSMIT_RETRY_SHIFT) | (type << TCPC_TRANSMIT_TYPE_SHIFT);
> +	reg = FIELD_PREP(TCPC_TRANSMIT_RETRY,
> +			 (negotiated_rev > PD_REV20
> +			  ? PD_RETRY_COUNT_3_0_OR_HIGHER
> +			  : PD_RETRY_COUNT_DEFAULT));
> +	reg |= FIELD_PREP(TCPC_TRANSMIT_TYPE, type);
>  	ret = regmap_write(tcpci->regmap, TCPC_TRANSMIT, reg);
>  	if (ret < 0)
>  		return ret;
> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> index 3cd61e9f73b3..f7f5cfbdef12 100644
> --- a/include/linux/usb/tcpci.h
> +++ b/include/linux/usb/tcpci.h
> @@ -148,10 +148,8 @@
>  #define TCPC_RX_DATA			0x34 /* through 0x4f */
>  
>  #define TCPC_TRANSMIT			0x50
> -#define TCPC_TRANSMIT_RETRY_SHIFT	4
> -#define TCPC_TRANSMIT_RETRY_MASK	0x3
> -#define TCPC_TRANSMIT_TYPE_SHIFT	0
> -#define TCPC_TRANSMIT_TYPE_MASK		0x7
> +#define TCPC_TRANSMIT_RETRY		GENMASK(5, 4)
> +#define TCPC_TRANSMIT_TYPE		GENMASK(2, 0)
>  
>  #define TCPC_TX_BYTE_CNT		0x51
>  #define TCPC_TX_HDR			0x52
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 08/15] usb: typec: tcpm/tcpci_maxim: sort TCPC_ALERT_MASK values by bit

[André Draszik]

This makes it easier to see what's missing and what's being programmed.

While at it, add brackets to help with formatting and remove a comment
that doesn't add much value.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/tcpci_maxim_core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
index 87102b4d060d..ad9bb61fd9e0 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
+++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
@@ -97,11 +97,13 @@ static void max_tcpci_init_regs(struct max_tcpci_chip *chip)
 	if (ret < 0)
 		return;
 
-	alert_mask = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_TX_FAILED |
-		TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_RX_STATUS | TCPC_ALERT_CC_STATUS |
-		TCPC_ALERT_VBUS_DISCNCT | TCPC_ALERT_RX_BUF_OVF | TCPC_ALERT_POWER_STATUS |
-		/* Enable Extended alert for detecting Fast Role Swap Signal */
-		TCPC_ALERT_EXTND | TCPC_ALERT_EXTENDED_STATUS | TCPC_ALERT_FAULT;
+	alert_mask = (TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_DISCARDED |
+		      TCPC_ALERT_TX_FAILED | TCPC_ALERT_RX_HARD_RST |
+		      TCPC_ALERT_RX_STATUS | TCPC_ALERT_POWER_STATUS |
+		      TCPC_ALERT_CC_STATUS |
+		      TCPC_ALERT_EXTND | TCPC_ALERT_EXTENDED_STATUS |
+		      TCPC_ALERT_VBUS_DISCNCT | TCPC_ALERT_RX_BUF_OVF |
+		      TCPC_ALERT_FAULT);
 
 	ret = max_tcpci_write16(chip, TCPC_ALERT_MASK, alert_mask);
 	if (ret < 0) {

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 08/15] usb: typec: tcpm/tcpci_maxim: sort TCPC_ALERT_MASK values by bit

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:15AM +0100, André Draszik wrote:
> This makes it easier to see what's missing and what's being programmed.
> 
> While at it, add brackets to help with formatting and remove a comment
> that doesn't add much value.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpci_maxim_core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> index 87102b4d060d..ad9bb61fd9e0 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> @@ -97,11 +97,13 @@ static void max_tcpci_init_regs(struct max_tcpci_chip *chip)
>  	if (ret < 0)
>  		return;
>  
> -	alert_mask = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_TX_FAILED |
> -		TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_RX_STATUS | TCPC_ALERT_CC_STATUS |
> -		TCPC_ALERT_VBUS_DISCNCT | TCPC_ALERT_RX_BUF_OVF | TCPC_ALERT_POWER_STATUS |
> -		/* Enable Extended alert for detecting Fast Role Swap Signal */
> -		TCPC_ALERT_EXTND | TCPC_ALERT_EXTENDED_STATUS | TCPC_ALERT_FAULT;
> +	alert_mask = (TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_DISCARDED |
> +		      TCPC_ALERT_TX_FAILED | TCPC_ALERT_RX_HARD_RST |
> +		      TCPC_ALERT_RX_STATUS | TCPC_ALERT_POWER_STATUS |
> +		      TCPC_ALERT_CC_STATUS |
> +		      TCPC_ALERT_EXTND | TCPC_ALERT_EXTENDED_STATUS |
> +		      TCPC_ALERT_VBUS_DISCNCT | TCPC_ALERT_RX_BUF_OVF |
> +		      TCPC_ALERT_FAULT);
>  
>  	ret = max_tcpci_write16(chip, TCPC_ALERT_MASK, alert_mask);
>  	if (ret < 0) {
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 09/15] usb: typec: tcpm/tcpci_maxim: simplify clearing of TCPC_ALERT_RX_BUF_OVF

[André Draszik]

There is no need for using the ternary if/else here, simply mask
TCPC_ALERT_RX_BUF_OVF as necessary, which arguably makes the code
easier to read.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/tcpci_maxim_core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
index ad9bb61fd9e0..5b5441db7047 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
+++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
@@ -193,9 +193,8 @@ static void process_rx(struct max_tcpci_chip *chip, u16 status)
 	 * Read complete, clear RX status alert bit.
 	 * Clear overflow as well if set.
 	 */
-	ret = max_tcpci_write16(chip, TCPC_ALERT, status & TCPC_ALERT_RX_BUF_OVF ?
-				TCPC_ALERT_RX_STATUS | TCPC_ALERT_RX_BUF_OVF :
-				TCPC_ALERT_RX_STATUS);
+	ret = max_tcpci_write16(chip, TCPC_ALERT,
+				TCPC_ALERT_RX_STATUS | (status & TCPC_ALERT_RX_BUF_OVF));
 	if (ret < 0)
 		return;
 
@@ -297,9 +296,8 @@ static irqreturn_t _max_tcpci_irq(struct max_tcpci_chip *chip, u16 status)
 	 * be cleared until we have successfully retrieved message.
 	 */
 	if (status & ~TCPC_ALERT_RX_STATUS) {
-		mask = status & TCPC_ALERT_RX_BUF_OVF ?
-			status & ~(TCPC_ALERT_RX_STATUS | TCPC_ALERT_RX_BUF_OVF) :
-			status & ~TCPC_ALERT_RX_STATUS;
+		mask = status & ~(TCPC_ALERT_RX_STATUS
+				  | (status & TCPC_ALERT_RX_BUF_OVF));
 		ret = max_tcpci_write16(chip, TCPC_ALERT, mask);
 		if (ret < 0) {
 			dev_err(chip->dev, "ALERT clear failed\n");

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 09/15] usb: typec: tcpm/tcpci_maxim: simplify clearing of TCPC_ALERT_RX_BUF_OVF

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:16AM +0100, André Draszik wrote:
> There is no need for using the ternary if/else here, simply mask
> TCPC_ALERT_RX_BUF_OVF as necessary, which arguably makes the code
> easier to read.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpci_maxim_core.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> index ad9bb61fd9e0..5b5441db7047 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> @@ -193,9 +193,8 @@ static void process_rx(struct max_tcpci_chip *chip, u16 status)
>  	 * Read complete, clear RX status alert bit.
>  	 * Clear overflow as well if set.
>  	 */
> -	ret = max_tcpci_write16(chip, TCPC_ALERT, status & TCPC_ALERT_RX_BUF_OVF ?
> -				TCPC_ALERT_RX_STATUS | TCPC_ALERT_RX_BUF_OVF :
> -				TCPC_ALERT_RX_STATUS);
> +	ret = max_tcpci_write16(chip, TCPC_ALERT,
> +				TCPC_ALERT_RX_STATUS | (status & TCPC_ALERT_RX_BUF_OVF));
>  	if (ret < 0)
>  		return;
>  
> @@ -297,9 +296,8 @@ static irqreturn_t _max_tcpci_irq(struct max_tcpci_chip *chip, u16 status)
>  	 * be cleared until we have successfully retrieved message.
>  	 */
>  	if (status & ~TCPC_ALERT_RX_STATUS) {
> -		mask = status & TCPC_ALERT_RX_BUF_OVF ?
> -			status & ~(TCPC_ALERT_RX_STATUS | TCPC_ALERT_RX_BUF_OVF) :
> -			status & ~TCPC_ALERT_RX_STATUS;
> +		mask = status & ~(TCPC_ALERT_RX_STATUS
> +				  | (status & TCPC_ALERT_RX_BUF_OVF));
>  		ret = max_tcpci_write16(chip, TCPC_ALERT, mask);
>  		if (ret < 0) {
>  			dev_err(chip->dev, "ALERT clear failed\n");
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 10/15] usb: typec: tcpm/tcpci_maxim: drop STATUS_CHECK()

[André Draszik]

Only one user of STATUS_CHECK() remains, and the code can actually be
made more legible by simply avoiding the use of that wrapper macro,
allowing to also drop the macro altogether.

Do so.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/maxim_contaminant.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
index e7687aeb69c0..8ac8eeade277 100644
--- a/drivers/usb/typec/tcpm/maxim_contaminant.c
+++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
@@ -46,8 +46,6 @@ enum fladc_select {
 #define READ1_SLEEP_MS			10
 #define READ2_SLEEP_MS			5
 
-#define STATUS_CHECK(reg, mask, val)	(((reg) & (mask)) == (val))
-
 #define IS_CC_OPEN(cc_status) \
 	(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)
@@ -368,7 +366,7 @@ bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect
 		}
 		return false;
 	} else if (chip->contaminant_state == DETECTED) {
-		if (STATUS_CHECK(cc_status, TCPC_CC_STATUS_TOGGLING, 0)) {
+		if (!(cc_status & TCPC_CC_STATUS_TOGGLING)) {
 			chip->contaminant_state = max_contaminant_detect_contaminant(chip);
 			if (chip->contaminant_state == DETECTED) {
 				max_contaminant_enable_dry_detection(chip);

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 10/15] usb: typec: tcpm/tcpci_maxim: drop STATUS_CHECK()

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:17AM +0100, André Draszik wrote:
> Only one user of STATUS_CHECK() remains, and the code can actually be
> made more legible by simply avoiding the use of that wrapper macro,
> allowing to also drop the macro altogether.
> 
> Do so.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/maxim_contaminant.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
> index e7687aeb69c0..8ac8eeade277 100644
> --- a/drivers/usb/typec/tcpm/maxim_contaminant.c
> +++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
> @@ -46,8 +46,6 @@ enum fladc_select {
>  #define READ1_SLEEP_MS			10
>  #define READ2_SLEEP_MS			5
>  
> -#define STATUS_CHECK(reg, mask, val)	(((reg) & (mask)) == (val))
> -
>  #define IS_CC_OPEN(cc_status) \
>  	(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)
> @@ -368,7 +366,7 @@ bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect
>  		}
>  		return false;
>  	} else if (chip->contaminant_state == DETECTED) {
> -		if (STATUS_CHECK(cc_status, TCPC_CC_STATUS_TOGGLING, 0)) {
> +		if (!(cc_status & TCPC_CC_STATUS_TOGGLING)) {
>  			chip->contaminant_state = max_contaminant_detect_contaminant(chip);
>  			if (chip->contaminant_state == DETECTED) {
>  				max_contaminant_enable_dry_detection(chip);
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 11/15] usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_CC_CTRL2 register

[André Draszik]

Convert register TCPC_VENDOR_CC_CTRL2 to using GENMASK() and
FIELD_PREP() so as to keep using a similar approach throughout the code
base and make it arguably easier to read.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/maxim_contaminant.c | 18 +++++++++++-------
 drivers/usb/typec/tcpm/tcpci_maxim.h       |  6 +++---
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
index 8ac8eeade277..f7acaa42329f 100644
--- a/drivers/usb/typec/tcpm/maxim_contaminant.c
+++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
@@ -116,13 +116,14 @@ static int max_contaminant_read_resistance_kohm(struct max_tcpci_chip *chip,
 	if (channel == CC1_SCALE1 || channel == CC2_SCALE1 || channel == CC1_SCALE2 ||
 	    channel == CC2_SCALE2) {
 		/* Enable 1uA current source */
-		ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCLPMODESEL_MASK,
-					 ULTRA_LOW_POWER_MODE);
+		ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCLPMODESEL,
+					 FIELD_PREP(CCLPMODESEL, ULTRA_LOW_POWER_MODE));
 		if (ret < 0)
 			return ret;
 
 		/* Enable 1uA current source */
-		ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL_MASK, UA_1_SRC);
+		ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL,
+					 FIELD_PREP(CCRPCTRL, UA_1_SRC));
 		if (ret < 0)
 			return ret;
 
@@ -176,7 +177,8 @@ static int max_contaminant_read_comparators(struct max_tcpci_chip *chip, u8 *ven
 	int ret;
 
 	/* Enable 80uA source */
-	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL_MASK, UA_80_SRC);
+	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL,
+				 FIELD_PREP(CCRPCTRL, UA_80_SRC));
 	if (ret < 0)
 		return ret;
 
@@ -209,7 +211,8 @@ static int max_contaminant_read_comparators(struct max_tcpci_chip *chip, u8 *ven
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL_MASK, 0);
+	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL,
+				 FIELD_PREP(CCRPCTRL, 0));
 	if (ret < 0)
 		return ret;
 
@@ -298,8 +301,9 @@ static int max_contaminant_enable_dry_detection(struct max_tcpci_chip *chip)
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCLPMODESEL_MASK,
-				 ULTRA_LOW_POWER_MODE);
+	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCLPMODESEL,
+				 FIELD_PREP(CCLPMODESEL,
+					    ULTRA_LOW_POWER_MODE));
 	if (ret < 0)
 		return ret;
 	ret = max_tcpci_read8(chip, TCPC_VENDOR_CC_CTRL2, &temp);
diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
index 78ff3b73ee7e..92c9a628ebe1 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim.h
+++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
@@ -20,9 +20,9 @@
 #define SBUOVPDIS                               BIT(7)
 #define CCOVPDIS                                BIT(6)
 #define SBURPCTRL                               BIT(5)
-#define CCLPMODESEL_MASK                        GENMASK(4, 3)
-#define ULTRA_LOW_POWER_MODE                    BIT(3)
-#define CCRPCTRL_MASK                           GENMASK(2, 0)
+#define CCLPMODESEL                             GENMASK(4, 3)
+#define ULTRA_LOW_POWER_MODE                    1
+#define CCRPCTRL                                GENMASK(2, 0)
 #define UA_1_SRC                                1
 #define UA_80_SRC                               3
 

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 11/15] usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_CC_CTRL2 register

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:18AM +0100, André Draszik wrote:
> Convert register TCPC_VENDOR_CC_CTRL2 to using GENMASK() and
> FIELD_PREP() so as to keep using a similar approach throughout the code
> base and make it arguably easier to read.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/maxim_contaminant.c | 18 +++++++++++-------
>  drivers/usb/typec/tcpm/tcpci_maxim.h       |  6 +++---
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
> index 8ac8eeade277..f7acaa42329f 100644
> --- a/drivers/usb/typec/tcpm/maxim_contaminant.c
> +++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
> @@ -116,13 +116,14 @@ static int max_contaminant_read_resistance_kohm(struct max_tcpci_chip *chip,
>  	if (channel == CC1_SCALE1 || channel == CC2_SCALE1 || channel == CC1_SCALE2 ||
>  	    channel == CC2_SCALE2) {
>  		/* Enable 1uA current source */
> -		ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCLPMODESEL_MASK,
> -					 ULTRA_LOW_POWER_MODE);
> +		ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCLPMODESEL,
> +					 FIELD_PREP(CCLPMODESEL, ULTRA_LOW_POWER_MODE));
>  		if (ret < 0)
>  			return ret;
>  
>  		/* Enable 1uA current source */
> -		ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL_MASK, UA_1_SRC);
> +		ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL,
> +					 FIELD_PREP(CCRPCTRL, UA_1_SRC));
>  		if (ret < 0)
>  			return ret;
>  
> @@ -176,7 +177,8 @@ static int max_contaminant_read_comparators(struct max_tcpci_chip *chip, u8 *ven
>  	int ret;
>  
>  	/* Enable 80uA source */
> -	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL_MASK, UA_80_SRC);
> +	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL,
> +				 FIELD_PREP(CCRPCTRL, UA_80_SRC));
>  	if (ret < 0)
>  		return ret;
>  
> @@ -209,7 +211,8 @@ static int max_contaminant_read_comparators(struct max_tcpci_chip *chip, u8 *ven
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL_MASK, 0);
> +	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCRPCTRL,
> +				 FIELD_PREP(CCRPCTRL, 0));
>  	if (ret < 0)
>  		return ret;
>  
> @@ -298,8 +301,9 @@ static int max_contaminant_enable_dry_detection(struct max_tcpci_chip *chip)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCLPMODESEL_MASK,
> -				 ULTRA_LOW_POWER_MODE);
> +	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL2, CCLPMODESEL,
> +				 FIELD_PREP(CCLPMODESEL,
> +					    ULTRA_LOW_POWER_MODE));
>  	if (ret < 0)
>  		return ret;
>  	ret = max_tcpci_read8(chip, TCPC_VENDOR_CC_CTRL2, &temp);
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
> index 78ff3b73ee7e..92c9a628ebe1 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim.h
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
> @@ -20,9 +20,9 @@
>  #define SBUOVPDIS                               BIT(7)
>  #define CCOVPDIS                                BIT(6)
>  #define SBURPCTRL                               BIT(5)
> -#define CCLPMODESEL_MASK                        GENMASK(4, 3)
> -#define ULTRA_LOW_POWER_MODE                    BIT(3)
> -#define CCRPCTRL_MASK                           GENMASK(2, 0)
> +#define CCLPMODESEL                             GENMASK(4, 3)
> +#define ULTRA_LOW_POWER_MODE                    1
> +#define CCRPCTRL                                GENMASK(2, 0)
>  #define UA_1_SRC                                1
>  #define UA_80_SRC                               3
>  
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 12/15] usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_CC_CTRL3 register

[André Draszik]

Convert register TCPC_VENDOR_CC_CTRL3 to using GENMASK() so as to keep
using a similar approach throughout the code base and make it arguably
easier to read.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/maxim_contaminant.c | 9 +++++----
 drivers/usb/typec/tcpm/tcpci_maxim.h       | 9 +++------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
index f7acaa42329f..cf9887de96c9 100644
--- a/drivers/usb/typec/tcpm/maxim_contaminant.c
+++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
@@ -283,10 +283,11 @@ static int max_contaminant_enable_dry_detection(struct max_tcpci_chip *chip)
 	u8 temp;
 	int ret;
 
-	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL3, CCWTRDEB_MASK | CCWTRSEL_MASK
-				    | WTRCYCLE_MASK, CCWTRDEB_1MS << CCWTRDEB_SHIFT |
-				    CCWTRSEL_1V << CCWTRSEL_SHIFT | WTRCYCLE_4_8_S <<
-				    WTRCYCLE_SHIFT);
+	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL3,
+				 CCWTRDEB | CCWTRSEL | WTRCYCLE,
+				 FIELD_PREP(CCWTRDEB, CCWTRDEB_1MS)
+				 | FIELD_PREP(CCWTRSEL, CCWTRSEL_1V)
+				 | FIELD_PREP(WTRCYCLE, WTRCYCLE_4_8_S));
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
index 92c9a628ebe1..34076069444f 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim.h
+++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
@@ -27,15 +27,12 @@
 #define UA_80_SRC                               3
 
 #define TCPC_VENDOR_CC_CTRL3                    0x8e
-#define CCWTRDEB_MASK                           GENMASK(7, 6)
-#define CCWTRDEB_SHIFT                          6
+#define CCWTRDEB                                GENMASK(7, 6)
 #define CCWTRDEB_1MS                            1
-#define CCWTRSEL_MASK                           GENMASK(5, 3)
-#define CCWTRSEL_SHIFT                          3
+#define CCWTRSEL                                GENMASK(5, 3)
 #define CCWTRSEL_1V                             0x4
 #define CCLADDERDIS                             BIT(2)
-#define WTRCYCLE_MASK                           BIT(0)
-#define WTRCYCLE_SHIFT                          0
+#define WTRCYCLE                                GENMASK(0, 0)
 #define WTRCYCLE_2_4_S                          0
 #define WTRCYCLE_4_8_S                          1
 

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 12/15] usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_CC_CTRL3 register

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:19AM +0100, André Draszik wrote:
> Convert register TCPC_VENDOR_CC_CTRL3 to using GENMASK() so as to keep
> using a similar approach throughout the code base and make it arguably
> easier to read.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/maxim_contaminant.c | 9 +++++----
>  drivers/usb/typec/tcpm/tcpci_maxim.h       | 9 +++------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
> index f7acaa42329f..cf9887de96c9 100644
> --- a/drivers/usb/typec/tcpm/maxim_contaminant.c
> +++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
> @@ -283,10 +283,11 @@ static int max_contaminant_enable_dry_detection(struct max_tcpci_chip *chip)
>  	u8 temp;
>  	int ret;
>  
> -	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL3, CCWTRDEB_MASK | CCWTRSEL_MASK
> -				    | WTRCYCLE_MASK, CCWTRDEB_1MS << CCWTRDEB_SHIFT |
> -				    CCWTRSEL_1V << CCWTRSEL_SHIFT | WTRCYCLE_4_8_S <<
> -				    WTRCYCLE_SHIFT);
> +	ret = regmap_update_bits(regmap, TCPC_VENDOR_CC_CTRL3,
> +				 CCWTRDEB | CCWTRSEL | WTRCYCLE,
> +				 FIELD_PREP(CCWTRDEB, CCWTRDEB_1MS)
> +				 | FIELD_PREP(CCWTRSEL, CCWTRSEL_1V)
> +				 | FIELD_PREP(WTRCYCLE, WTRCYCLE_4_8_S));
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
> index 92c9a628ebe1..34076069444f 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim.h
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
> @@ -27,15 +27,12 @@
>  #define UA_80_SRC                               3
>  
>  #define TCPC_VENDOR_CC_CTRL3                    0x8e
> -#define CCWTRDEB_MASK                           GENMASK(7, 6)
> -#define CCWTRDEB_SHIFT                          6
> +#define CCWTRDEB                                GENMASK(7, 6)
>  #define CCWTRDEB_1MS                            1
> -#define CCWTRSEL_MASK                           GENMASK(5, 3)
> -#define CCWTRSEL_SHIFT                          3
> +#define CCWTRSEL                                GENMASK(5, 3)
>  #define CCWTRSEL_1V                             0x4
>  #define CCLADDERDIS                             BIT(2)
> -#define WTRCYCLE_MASK                           BIT(0)
> -#define WTRCYCLE_SHIFT                          0
> +#define WTRCYCLE                                GENMASK(0, 0)
>  #define WTRCYCLE_2_4_S                          0
>  #define WTRCYCLE_4_8_S                          1
>  
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 13/15] usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_ADC_CTRL1 register

[André Draszik]

Convert register TCPC_VENDOR_ADC_CTRL1 to using GENMASK() and
FIELD_PREP() so as to keep using a similar approach throughout the code
base and make it arguably easier to read.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/maxim_contaminant.c | 7 ++++---
 drivers/usb/typec/tcpm/tcpci_maxim.h       | 3 +--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
index cf9887de96c9..7bfec45e8f4f 100644
--- a/drivers/usb/typec/tcpm/maxim_contaminant.c
+++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
@@ -76,8 +76,8 @@ static int max_contaminant_read_adc_mv(struct max_tcpci_chip *chip, enum fladc_s
 	int ret;
 
 	/* Channel & scale select */
-	ret = regmap_update_bits(regmap, TCPC_VENDOR_ADC_CTRL1, ADCINSEL_MASK,
-				 channel << ADC_CHANNEL_OFFSET);
+	ret = regmap_update_bits(regmap, TCPC_VENDOR_ADC_CTRL1, ADCINSEL,
+				 FIELD_PREP(ADCINSEL, channel));
 	if (ret < 0)
 		return ret;
 
@@ -96,7 +96,8 @@ static int max_contaminant_read_adc_mv(struct max_tcpci_chip *chip, enum fladc_s
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_update_bits(regmap, TCPC_VENDOR_ADC_CTRL1, ADCINSEL_MASK, 0);
+	ret = regmap_update_bits(regmap, TCPC_VENDOR_ADC_CTRL1, ADCINSEL,
+				 FIELD_PREP(ADCINSEL, 0));
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
index 34076069444f..a41ca9e7ad08 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim.h
+++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
@@ -37,8 +37,7 @@
 #define WTRCYCLE_4_8_S                          1
 
 #define TCPC_VENDOR_ADC_CTRL1                   0x91
-#define ADCINSEL_MASK                           GENMASK(7, 5)
-#define ADC_CHANNEL_OFFSET                      5
+#define ADCINSEL                                GENMASK(7, 5)
 #define ADCEN                                   BIT(0)
 
 enum contamiant_state {

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 13/15] usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_ADC_CTRL1 register

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:20AM +0100, André Draszik wrote:
> Convert register TCPC_VENDOR_ADC_CTRL1 to using GENMASK() and
> FIELD_PREP() so as to keep using a similar approach throughout the code
> base and make it arguably easier to read.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/maxim_contaminant.c | 7 ++++---
>  drivers/usb/typec/tcpm/tcpci_maxim.h       | 3 +--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c
> index cf9887de96c9..7bfec45e8f4f 100644
> --- a/drivers/usb/typec/tcpm/maxim_contaminant.c
> +++ b/drivers/usb/typec/tcpm/maxim_contaminant.c
> @@ -76,8 +76,8 @@ static int max_contaminant_read_adc_mv(struct max_tcpci_chip *chip, enum fladc_s
>  	int ret;
>  
>  	/* Channel & scale select */
> -	ret = regmap_update_bits(regmap, TCPC_VENDOR_ADC_CTRL1, ADCINSEL_MASK,
> -				 channel << ADC_CHANNEL_OFFSET);
> +	ret = regmap_update_bits(regmap, TCPC_VENDOR_ADC_CTRL1, ADCINSEL,
> +				 FIELD_PREP(ADCINSEL, channel));
>  	if (ret < 0)
>  		return ret;
>  
> @@ -96,7 +96,8 @@ static int max_contaminant_read_adc_mv(struct max_tcpci_chip *chip, enum fladc_s
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = regmap_update_bits(regmap, TCPC_VENDOR_ADC_CTRL1, ADCINSEL_MASK, 0);
> +	ret = regmap_update_bits(regmap, TCPC_VENDOR_ADC_CTRL1, ADCINSEL,
> +				 FIELD_PREP(ADCINSEL, 0));
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h
> index 34076069444f..a41ca9e7ad08 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim.h
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim.h
> @@ -37,8 +37,7 @@
>  #define WTRCYCLE_4_8_S                          1
>  
>  #define TCPC_VENDOR_ADC_CTRL1                   0x91
> -#define ADCINSEL_MASK                           GENMASK(7, 5)
> -#define ADC_CHANNEL_OFFSET                      5
> +#define ADCINSEL                                GENMASK(7, 5)
>  #define ADCEN                                   BIT(0)
>  
>  enum contamiant_state {
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 14/15] usb: typec: tcpm/tcpci_maxim: convert to dev_err_probe()

[André Draszik]

dev_err_probe() exists as a useful helper ensuring standardized
error messages during .probe() and using it also helps to make
the code more legible.

Use it.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/tcpci_maxim_core.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
index 5b5441db7047..ee3e86797f17 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
+++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
@@ -484,17 +484,17 @@ static int max_tcpci_probe(struct i2c_client *client)
 
 	chip->client = client;
 	chip->data.regmap = devm_regmap_init_i2c(client, &max_tcpci_regmap_config);
-	if (IS_ERR(chip->data.regmap)) {
-		dev_err(&client->dev, "Regmap init failed\n");
-		return PTR_ERR(chip->data.regmap);
-	}
+	if (IS_ERR(chip->data.regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->data.regmap),
+				     "Regmap init failed\n");
 
 	chip->dev = &client->dev;
 	i2c_set_clientdata(client, chip);
 
 	ret = max_tcpci_read8(chip, TCPC_POWER_STATUS, &power_status);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to read TCPC_POWER_STATUS\n");
 
 	/* Chip level tcpci callbacks */
 	chip->data.set_vbus = max_tcpci_set_vbus;
@@ -511,10 +511,10 @@ static int max_tcpci_probe(struct i2c_client *client)
 
 	max_tcpci_init_regs(chip);
 	chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
-	if (IS_ERR(chip->tcpci)) {
-		dev_err(&client->dev, "TCPCI port registration failed\n");
-		return PTR_ERR(chip->tcpci);
-	}
+	if (IS_ERR(chip->tcpci))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->tcpci),
+				     "TCPCI port registration failed\n");
+
 	chip->port = tcpci_get_tcpm_port(chip->tcpci);
 	ret = max_tcpci_init_alert(chip, client);
 	if (ret < 0)
@@ -526,7 +526,8 @@ static int max_tcpci_probe(struct i2c_client *client)
 unreg_port:
 	tcpci_unregister_port(chip->tcpci);
 
-	return ret;
+	return dev_err_probe(&client->dev, ret,
+			     "Maxim TCPCI driver initialization failed\n");
 }
 
 static void max_tcpci_remove(struct i2c_client *client)

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 14/15] usb: typec: tcpm/tcpci_maxim: convert to dev_err_probe()

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:21AM +0100, André Draszik wrote:
> dev_err_probe() exists as a useful helper ensuring standardized
> error messages during .probe() and using it also helps to make
> the code more legible.
> 
> Use it.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpci_maxim_core.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> index 5b5441db7047..ee3e86797f17 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> @@ -484,17 +484,17 @@ static int max_tcpci_probe(struct i2c_client *client)
>  
>  	chip->client = client;
>  	chip->data.regmap = devm_regmap_init_i2c(client, &max_tcpci_regmap_config);
> -	if (IS_ERR(chip->data.regmap)) {
> -		dev_err(&client->dev, "Regmap init failed\n");
> -		return PTR_ERR(chip->data.regmap);
> -	}
> +	if (IS_ERR(chip->data.regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->data.regmap),
> +				     "Regmap init failed\n");
>  
>  	chip->dev = &client->dev;
>  	i2c_set_clientdata(client, chip);
>  
>  	ret = max_tcpci_read8(chip, TCPC_POWER_STATUS, &power_status);
>  	if (ret < 0)
> -		return ret;
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to read TCPC_POWER_STATUS\n");
>  
>  	/* Chip level tcpci callbacks */
>  	chip->data.set_vbus = max_tcpci_set_vbus;
> @@ -511,10 +511,10 @@ static int max_tcpci_probe(struct i2c_client *client)
>  
>  	max_tcpci_init_regs(chip);
>  	chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
> -	if (IS_ERR(chip->tcpci)) {
> -		dev_err(&client->dev, "TCPCI port registration failed\n");
> -		return PTR_ERR(chip->tcpci);
> -	}
> +	if (IS_ERR(chip->tcpci))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->tcpci),
> +				     "TCPCI port registration failed\n");
> +
>  	chip->port = tcpci_get_tcpm_port(chip->tcpci);
>  	ret = max_tcpci_init_alert(chip, client);
>  	if (ret < 0)
> @@ -526,7 +526,8 @@ static int max_tcpci_probe(struct i2c_client *client)
>  unreg_port:
>  	tcpci_unregister_port(chip->tcpci);
>  
> -	return ret;
> +	return dev_err_probe(&client->dev, ret,
> +			     "Maxim TCPCI driver initialization failed\n");
>  }
>  
>  static void max_tcpci_remove(struct i2c_client *client)
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

[PATCH 15/15] usb: typec: tcpm/tcpci_maxim: use device managed TCPCI port deregistration

[André Draszik]

Instead of open-coding the call to tcpci_unregister_port() in various
places, let's just register a hook using devm_add_action_or_reset() so
that it gets called automatically as and when necessary by the device
management APIs.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/usb/typec/tcpm/tcpci_maxim_core.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
index ee3e86797f17..7abfd29b4b01 100644
--- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
+++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
@@ -472,6 +472,11 @@ static bool max_tcpci_attempt_vconn_swap_discovery(struct tcpci *tcpci, struct t
 	return true;
 }
 
+static void max_tcpci_unregister_tcpci_port(void *tcpci)
+{
+	tcpci_unregister_port(tcpci);
+}
+
 static int max_tcpci_probe(struct i2c_client *client)
 {
 	int ret;
@@ -515,27 +520,21 @@ static int max_tcpci_probe(struct i2c_client *client)
 		return dev_err_probe(&client->dev, PTR_ERR(chip->tcpci),
 				     "TCPCI port registration failed\n");
 
+        ret = devm_add_action_or_reset(&client->dev,
+				       max_tcpci_unregister_tcpci_port,
+				       chip->tcpci);
+        if (ret)
+                return ret;
+
 	chip->port = tcpci_get_tcpm_port(chip->tcpci);
+
 	ret = max_tcpci_init_alert(chip, client);
 	if (ret < 0)
-		goto unreg_port;
+		return dev_err_probe(&client->dev, ret,
+				     "IRQ initialization failed\n");
 
 	device_init_wakeup(chip->dev, true);
 	return 0;
-
-unreg_port:
-	tcpci_unregister_port(chip->tcpci);
-
-	return dev_err_probe(&client->dev, ret,
-			     "Maxim TCPCI driver initialization failed\n");
-}
-
-static void max_tcpci_remove(struct i2c_client *client)
-{
-	struct max_tcpci_chip *chip = i2c_get_clientdata(client);
-
-	if (!IS_ERR_OR_NULL(chip->tcpci))
-		tcpci_unregister_port(chip->tcpci);
 }
 
 static const struct i2c_device_id max_tcpci_id[] = {
@@ -558,7 +557,6 @@ static struct i2c_driver max_tcpci_i2c_driver = {
 		.of_match_table = of_match_ptr(max_tcpci_of_match),
 	},
 	.probe = max_tcpci_probe,
-	.remove = max_tcpci_remove,
 	.id_table = max_tcpci_id,
 };
 module_i2c_driver(max_tcpci_i2c_driver);

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 15/15] usb: typec: tcpm/tcpci_maxim: use device managed TCPCI port deregistration

[Heikki Krogerus]

On Wed, Jul 10, 2024 at 11:36:22AM +0100, André Draszik wrote:
> Instead of open-coding the call to tcpci_unregister_port() in various
> places, let's just register a hook using devm_add_action_or_reset() so
> that it gets called automatically as and when necessary by the device
> management APIs.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpci_maxim_core.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> index ee3e86797f17..7abfd29b4b01 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c
> @@ -472,6 +472,11 @@ static bool max_tcpci_attempt_vconn_swap_discovery(struct tcpci *tcpci, struct t
>  	return true;
>  }
>  
> +static void max_tcpci_unregister_tcpci_port(void *tcpci)
> +{
> +	tcpci_unregister_port(tcpci);
> +}
> +
>  static int max_tcpci_probe(struct i2c_client *client)
>  {
>  	int ret;
> @@ -515,27 +520,21 @@ static int max_tcpci_probe(struct i2c_client *client)
>  		return dev_err_probe(&client->dev, PTR_ERR(chip->tcpci),
>  				     "TCPCI port registration failed\n");
>  
> +        ret = devm_add_action_or_reset(&client->dev,
> +				       max_tcpci_unregister_tcpci_port,
> +				       chip->tcpci);
> +        if (ret)
> +                return ret;
> +
>  	chip->port = tcpci_get_tcpm_port(chip->tcpci);
> +
>  	ret = max_tcpci_init_alert(chip, client);
>  	if (ret < 0)
> -		goto unreg_port;
> +		return dev_err_probe(&client->dev, ret,
> +				     "IRQ initialization failed\n");
>  
>  	device_init_wakeup(chip->dev, true);
>  	return 0;
> -
> -unreg_port:
> -	tcpci_unregister_port(chip->tcpci);
> -
> -	return dev_err_probe(&client->dev, ret,
> -			     "Maxim TCPCI driver initialization failed\n");
> -}
> -
> -static void max_tcpci_remove(struct i2c_client *client)
> -{
> -	struct max_tcpci_chip *chip = i2c_get_clientdata(client);
> -
> -	if (!IS_ERR_OR_NULL(chip->tcpci))
> -		tcpci_unregister_port(chip->tcpci);
>  }
>  
>  static const struct i2c_device_id max_tcpci_id[] = {
> @@ -558,7 +557,6 @@ static struct i2c_driver max_tcpci_i2c_driver = {
>  		.of_match_table = of_match_ptr(max_tcpci_of_match),
>  	},
>  	.probe = max_tcpci_probe,
> -	.remove = max_tcpci_remove,
>  	.id_table = max_tcpci_id,
>  };
>  module_i2c_driver(max_tcpci_i2c_driver);
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog

-- 
heikki

Re: [PATCH 00/15] usb: typec: tcpci: few TCPCi & TCPCi-Maxim cleanups (mostly genmask())

[André Draszik]

Hi,

On Wed, 2024-07-10 at 11:36 +0100, André Draszik wrote:
> While looking through the TCPCi drivers, it occurred to me that all of the
> open-coded register bit manipulations can be simplified and made more
> legible by using the standard GENMASK(), FIELD_GET(), and FIELD_PREP()
> macros, which also is less prone to errors: e.g.
> 
> 	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))
> 
> (arguably) is much harder to read and reason about than:
> 
> 	if (FIELD_GET(TCPC_ROLE_CTRL_CC2, reg) != FIELD_GET(TCPC_ROLE_CTRL_CC1, reg))
> 
> and so on.
> 
> These patches do that. In addition, there are a few comment fixes and I
> added in a conversion to using dev_err_probe() and
> devm_add_action_or_reset() in the Maxim TCPCi driver.
> 
> I kept the patches separated by register or register field as appropriate to
> simplify reviewing, allowing to focus on one field/register at a time.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> ---
> André Draszik (15):
>       usb: typec: tcpci: fix a comment typo
>       usb: typec: tcpm/tcpci_maxim: clarify a comment
>       usb: typec: tcpci: use GENMASK() for TCPC_CC_STATUS_CC[12]
>       usb: typec: tcpci: use GENMASK() for TCPC_ROLE_CTRL_CC[12]
>       usb: typec: tcpci: use GENMASK() for TCPC_ROLE_CTRL_RP_VAL
>       usb: typec: tcpci: use GENMASK() for TCPC_MSG_HDR_INFO_REV
>       usb: typec: tcpci: use GENMASK() for TCPC_TRANSMIT register fields
>       usb: typec: tcpm/tcpci_maxim: sort TCPC_ALERT_MASK values by bit
>       usb: typec: tcpm/tcpci_maxim: simplify clearing of TCPC_ALERT_RX_BUF_OVF
>       usb: typec: tcpm/tcpci_maxim: drop STATUS_CHECK()
>       usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_CC_CTRL2 register
>       usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_CC_CTRL3 register
>       usb: typec: tcpm/tcpci_maxim: use GENMASK() for TCPC_VENDOR_ADC_CTRL1 register
>       usb: typec: tcpm/tcpci_maxim: convert to dev_err_probe()
>       usb: typec: tcpm/tcpci_maxim: use device managed TCPCI port deregistration
> 
>  drivers/usb/typec/anx7411.c                |   5 +-
>  drivers/usb/typec/tcpm/maxim_contaminant.c |  46 +++++++------
>  drivers/usb/typec/tcpm/tcpci.c             | 104 ++++++++++++++---------------
>  drivers/usb/typec/tcpm/tcpci_maxim.h       |  18 ++---
>  drivers/usb/typec/tcpm/tcpci_maxim_core.c  |  71 ++++++++++----------
>  drivers/usb/typec/tcpm/tcpci_rt1711h.c     |  27 ++++----
>  include/linux/usb/tcpci.h                  |  31 +++------
>  7 files changed, 144 insertions(+), 158 deletions(-)

Any comments on this series?

Cheers,
Andre'