Re: [PATCH] [88q2xxx] Add support for handling master/slave in forced mode

[Jakub Kicinski <kuba@kernel.org>]

A few drive-by comments...

On Wed, 20 Aug 2025 11:11:43 -0700 Ilya A. Evenbach wrote:
> In-Reply-To: <20250820181143.2288755-1-ievenbach@aurora.tech>

Please start a new thread for each new revision.
Change subject to [PATCH vN], where vN is the revision.
Add

v1: https://lore.kernel.org/20250820181143.2288755-2-ievenbach@aurora.tech

under the --- for reference to older versions

> Cc: dima.fedrau@gmail.com, "Ilya A. Evenbach" <ievenbach@aurora.tech>

The cc list is still pretty sparse

> Subject: [PATCH] [88q2xxx] Add support for handling master/slave in forced mode

the [88q2xxx] will be stripped by git when applying. Please look thru
the history of the file and find the correct way to prefix.

> 88q2xxx PHYs have non-standard way of setting master/slave in
> forced mode.
> This change adds support for changing and reporting this setting
> correctly through ethtool.

imperative mood:

 Add support for changing...


> +/* Marvell vendor PMA/PMD control for forced master/slave when AN is disabled */
> +#define PMAPMD_MVL_PMAPMD_CTL				0x0834
> +#define MASTER_MODE					BIT(14)
> +#define MODE_MASK					BIT(14)

Other defines in context seem to be prefixed with  MV88Q2XXX_
please prefer prefixing local defines.

>  struct mv88q2xxx_priv {
>  	bool enable_led0;
>  };
> @@ -377,13 +382,57 @@ static int mv88q2xxx_read_link(struct phy_device *phydev)
>  static int mv88q2xxx_read_master_slave_state(struct phy_device *phydev)
>  {
>  	int ret;
> +	int adv_l, adv_m, stat, stat2;

nit: in networking we like variable declaration lines to be sorted longest to shortest.
-- 
pw-bot: cr