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@xxxxxxxxxx>
> ---
> 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'