Re: [PATCH 3/4] phy: apple: atc: Add initial M3 series support

From: Sven Peter <sven@kernel.org>
To: Sasha Finkelstein <k@chaosmail.tech>, Janne Grunau <j@jannau.net>,
	Neal Gompa <neal@gompa.dev>, Vinod Koul <vkoul@kernel.org>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Manivannan Sadhasivam <mani@kernel.org>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>
Cc: asahi@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] phy: apple: atc: Add initial M3 series support
Date: Sun, 8 Nov 2026 19:15:43 +0100	[thread overview]
Message-ID: <2ec53558-07d8-4824-821b-03e39da672aa@kernel.org> (raw)
In-Reply-To: <20261108-t8122-phy-v1-3-c2783b2a53a8@chaosmail.tech>

Hi,

On 11/8/26 16:43, Sasha Finkelstein wrote:
> Adds initial support for the new t8122 phy. USB/TB only for now, DP
> will fail to negotiate
>
> Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
> ---

So I wonder if it's time to split this into multiple files now.
Maybe something like atc.c that does the core stuff (probing, 
phy/reset/etc. interfacing, mode transitions, tunables, etc.) and then 
atc-t8103.c and atc-t8112.c for the hardware specific parts since 
especially DP seems to be quite different between the two generations now.

>   drivers/phy/apple/atc.c | 382 +++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 297 insertions(+), 85 deletions(-)

[...]

> diff --git a/drivers/phy/apple/atc.c b/drivers/phy/apple/atc.c
> index 45f1280a6faa..256a08499646 100644
> --- a/drivers/phy/apple/atc.c
> +++ b/drivers/phy/apple/atc.c
> @@ -100,9 +100,15 @@
>   #define AUSPLL_FREQ_CFG 0x2224
>   #define AUSPLL_FREQ_REFCLK GENMASK(1, 0)
>   
> +#define AUS_COMMON_SHIM_BLK_BIAS_REG 0x0a00
> +#define AUS_COMMON_SHIM_BLK_BIAS_REG_BGBIAS_OV BIT(1)
> +
>   #define AUS_COMMON_SHIM_BLK_VREG 0x0a04
>   #define AUS_VREG_TRIM GENMASK(6, 2)
>   
[...]
>   	atcphy->mode = mode;
>   
> @@ -2201,6 +2387,7 @@ static int atcphy_probe_mux(struct apple_atcphy *atcphy)
>   
>   static int atcphy_load_tunables(struct apple_atcphy *atcphy)
>   {
> +	size_t tunable_count;
>   	struct {
>   		const char *dt_name;
>   		struct apple_tunable **tunable;
> @@ -2216,8 +2403,14 @@ static int atcphy_load_tunables(struct apple_atcphy *atcphy)
>   		{ "apple,tunable-lane0-dp", &atcphy->tunables.lane_dp[0], atcphy->res.core },
>   		{ "apple,tunable-lane1-dp", &atcphy->tunables.lane_dp[1], atcphy->res.core },
>   	};
> -
> -	for (int i = 0; i < ARRAY_SIZE(tunables); i++) {
> +	if (atcphy->hw->gen == ATCPHY_GENERATION_T8122) {
> +		tunable_count = ARRAY_SIZE(tunables) - 2;
> +		atcphy->tunables.lane_dp[0] = NULL;
> +		atcphy->tunables.lane_dp[1] = NULL;
> +	} else
> +		tunable_count = ARRAY_SIZE(tunables);

nit: I think else { .. } looks nicer when the if already has braces even 
though it's just a single line.

Sven