Re: [PATCH 2/5] phy: core: Define TBT phy_mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The tbt_phy_submode enum implicitly sets PHY_SUBMODE_TBT3 to 0, making it the hidden default submode for phy_set_mode().
- [Low] Using PHY_MODE_TBT as the generic mode with USB4 as a submode conceptually inverts the industry standard hierarchy.
--

commit f1ad33ca1ed62aa8d4ab806ced09908d4e9f3625
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

phy: core: Define TBT phy_mode

This commit introduces a new PHY mode for Thunderbolt (TBT) and USB4 PHYs
that require specific electrical and analog configurations. It defines
PHY_MODE_TBT along with PHY_SUBMODE_TBT3 and PHY_SUBMODE_USB4 submodes.

> diff --git a/include/linux/phy/phy-tbt.h b/include/linux/phy/phy-tbt.h
> new file mode 100644
> index 0000000000000..5f48059814e19
> --- /dev/null
> +++ b/include/linux/phy/phy-tbt.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#ifndef __PHY_TBT_H_
> +#define __PHY_TBT_H_
> +
> +enum tbt_phy_submode {
> +	PHY_SUBMODE_TBT3,

Since PHY_SUBMODE_TBT3 is the first element, it implicitly evaluates to 0.

The generic phy_set_mode(phy, mode) macro in include/linux/phy/phy.h wraps
phy_set_mode_ext(phy, mode, 0).

Does this mean any driver calling phy_set_mode(phy, PHY_MODE_TBT) will
silently default to requesting the legacy TBT3 electrical/timing
configuration instead of USB4?

Would it be safer to reserve 0 for an unspecified state (e.g.,
PHY_SUBMODE_UNSPECIFIED = 0) to force explicit submode selection, or to
make the modern standard (USB4) the default?

> +	PHY_SUBMODE_USB4,
> +};
> +
> +#endif

> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index ea47975e288ae..26a91c070f8ce 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h

[ ... ]

> @@ -45,6 +46,7 @@ enum phy_mode {
>  	PHY_MODE_LVDS,
>  	PHY_MODE_DP,
>  	PHY_MODE_HDMI,
> +	PHY_MODE_TBT,

Is using PHY_MODE_TBT as the generic mode with USB4 as a submode the best
hierarchy here?

USB4 is the foundational open standard established by the USB-IF, and the
Linux kernel USB and Type-C subsystems natively treat USB4 as the primary
mode (e.g., TYPEC_MODE_USB4).

Could grouping USB4 under a Thunderbolt PHY mode cause semantic confusion
and contradict existing kernel USB nomenclature? Might it be more
consistent to use PHY_MODE_USB4 as the primary mode, with TBT3 as a
legacy fallback submode?

>  };
>  
>  enum phy_media {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-topic-usb4phy-v1-0-71d827c49dca@oss.qualcomm.com?part=2