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
