Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Fri, 20 Oct 2023 11:49:27 +0200
Message-Id: <CWD6889N9X4U.3N5P65PFK5XK1@fairphone.com>
Cc:     "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        "Rob Herring" <robh+dt@kernel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        "Conor Dooley" <conor+dt@kernel.org>,
        "Liam Girdwood" <lgirdwood@gmail.com>,
        "Mark Brown" <broonie@kernel.org>,
        "Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>,
        <~postmarketos/upstreaming@lists.sr.ht>,
        <phone-devel@vger.kernel.org>, <linux-usb@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH 2/2] usb: typec: add support for PTN36502 redriver
From:   "Luca Weiss" <luca.weiss@fairphone.com>
To:     <neil.armstrong@linaro.org>,
        "Heikki Krogerus" <heikki.krogerus@linux.intel.com>
References: <20231013-ptn36502-v1-0-98109a430efc@fairphone.com>
 <20231013-ptn36502-v1-2-98109a430efc@fairphone.com>
 <ZS5HAQrUObdhHSdV@kuha.fi.intel.com>
 <CWD1MKL68P2V.1QBJXJ0E5U171@fairphone.com>
 <f29c43c6-0e73-4c04-9180-6c6088ab1f8a@linaro.org>
In-Reply-To: <f29c43c6-0e73-4c04-9180-6c6088ab1f8a@linaro.org>
Precedence: bulk

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 DisplayPor=
t
> >>> v1.2 combo redriver.
> >>>
> >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> >>
> >> Looks OK to me, but couple of nitpicks below. With those fixed:
> >>
> >> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>
> >>> ---
> >>>   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 >>.
> >=20
> > Hi Heikki,
> >=20
> > 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.
> >=20
> > 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 =3D 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 correc=
tly,
> no confusion possible.

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

Regards
Luca