Re: [PATCH 2/2] usb: typec: add support for PTN36502 redriver

[Luca Weiss]

On Fri Oct 20, 2023 at 9:18 AM CEST, Neil Armstrong wrote:
> On 20/10/2023 08:13, Luca Weiss wrote:
> > On Tue Oct 17, 2023 at 10:34 AM CEST, Heikki Krogerus wrote:
> >> Hi,
> >>
> >> On Fri, Oct 13, 2023 at 04:24:48PM +0200, Luca Weiss wrote:
> >>> Add a driver for the NXP PTN36502 Type-C USB 3.1 Gen 1 and DisplayPort
> >>> v1.2 combo redriver.
> >>>
> >>> Signed-off-by: Luca Weiss <luca.weiss@xxxxxxxxxxxxx>
> >>
> >> Looks OK to me, but couple of nitpicks below. With those fixed:
> >>
> >> Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> >>
> >>> ---
> >>>   drivers/usb/typec/mux/Kconfig    |  10 +
> >>>   drivers/usb/typec/mux/Makefile   |   1 +
> >>>   drivers/usb/typec/mux/ptn36502.c | 421 +++++++++++++++++++++++++++++++++++++++
> >>>   3 files changed, 432 insertions(+)
> >>>

[snip]

> >>> +#define PTN36502_CHIP_ID_REG				0x00
> >>> +#define PTN36502_CHIP_ID				0x02
> >>> +
> >>> +#define PTN36502_CHIP_REVISION_REG			0x01
> >>> +#define PTN36502_CHIP_REVISION_BASE(val)		FIELD_GET(GENMASK(7, 4), (val))
> >>> +#define PTN36502_CHIP_REVISION_METAL(val)		FIELD_GET(GENMASK(3, 0), (val))
> >>> +
> >>> +#define PTN36502_DP_LINK_CTRL_REG			0x06
> >>> +#define PTN36502_DP_LINK_CTRL_LANES_2			(2 << 2)
> >>> +#define PTN36502_DP_LINK_CTRL_LANES_4			(3 << 2)
> >>> +#define PTN36502_DP_LINK_CTRL_LINK_RATE_5_4GBPS		(2 << 0)
> >>> +
> >>> +/* Registers for lane 0 (0x07) to lane 3 (0x0a) have the same layout */
> >>> +#define PTN36502_DP_LANE_CTRL_REG(n)			(0x07 + (n))
> >>> +#define PTN36502_DP_LANE_CTRL_RX_GAIN_3DB		(2<<4)
> >>> +#define PTN36502_DP_LANE_CTRL_TX_SWING_800MVPPD		(2<<2)
> >>> +#define PTN36502_DP_LANE_CTRL_PRE_EMPHASIS_3_5DB	(1<<0)
> >>> +
> >>> +#define PTN36502_MODE_CTRL1_REG				0x0b
> >>> +#define PTN36502_MODE_CTRL1_PLUG_ORIENT_REVERSE		(1<<5)
> >>> +#define PTN36502_MODE_CTRL1_AUX_CROSSBAR_SW_ON		(1<<3)
> >>> +#define PTN36502_MODE_CTRL1_MODE_OFF			(0<<0)
> >>> +#define PTN36502_MODE_CTRL1_MODE_USB_ONLY		(1<<0)
> >>> +#define PTN36502_MODE_CTRL1_MODE_USB_DP			(2<<0)
> >>> +#define PTN36502_MODE_CTRL1_MODE_DP			(3<<0)
> >>> +
> >>> +#define PTN36502_DEVICE_CTRL_REG			0x0d
> >>> +#define PTN36502_DEVICE_CTRL_AUX_MONITORING_EN		(1<<7)
> >>
> >> You have couple of different styles here. Please try to always use
> >> BIT() and GENMASK() macros when possible. At the very least put spaces
> >> around << and >>.
> > 
> > Hi Heikki,
> > 
> > I was wondering when writing that whether GENMASK was actually proper
> > use for values you write to registers, when not actually used as a
> > bitmask.
> > 
> > Since the datasheet refers to e.g. with TX_SWING_800MVPPD (2<<2) that
> > you write a '2' to the correct bits of this register. But when using
> > BIT(3) kind of hides this relationship if someone refers back to the
> > datasheet. Or same with "3<<2" -> GENMASK(3, 2) or whatever.
>
> The proper way is to define the MASK for the field GENMASK(3, 2) and then
> use FIELD_PREP(GENMASK(3, 2), 2) to write 2 in this field.
>
> You could replace with:
> #define PTN36502_DP_LANE_CTRL_TX_SWING_MASK		GENMASK(3, 2)
> #define PTN36502_DP_LANE_CTRL_TX_SWING_800MVPPD		(2)
>
> and in the code
> lane_ctrl_val = FIELD_PREP(PTN36502_DP_LANE_CTRL_RX_GAIN_MASK,
> 			   PTN36502_DP_LANE_CTRL_RX_GAIN_3DB) |
> 		FIELD_PREP(PTN36502_DP_LANE_CTRL_TX_SWING_MASK,
> 			   PTN36502_DP_LANE_CTRL_TX_SWING_800MVPPD) |
> 		FIELD_PREP(PTN36502_DP_LANE_CTRL_PRE_EMPHASIS_MASK,
> 			   PTN36502_DP_LANE_CTRL_PRE_EMPHASIS_3_5DB);
>
> It's a little more verbose but it's much clearer and defines stuff correctly,
> no confusion possible.

Thanks for the advise, I'll update for v2!

Regards
Luca