Re: [PATCH v2 phy-next 02/15] phy: lynx-28g: move lane mode helpers to new core module

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: sashiko-reviews@lists.linux.dev
Cc: vkoul@kernel.org, olteanv@gmail.com,
	linux-phy@lists.infradead.org, neil.armstrong@linaro.org
Subject: Re: [PATCH v2 phy-next 02/15] phy: lynx-28g: move lane mode helpers to new core module
Date: Tue, 2 Jun 2026 17:38:45 +0300	[thread overview]
Message-ID: <20260602143845.iwhimj5utn6kidx5@skbuf> (raw)
In-Reply-To: <20260529172706.50EC21F00893@smtp.kernel.org>

On Fri, May 29, 2026 at 05:27:05PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] The enum values for `lynx_lane_mode` are globally exposed without proper namespacing, risking build failures due to collisions.
> - [Low] Invisible Kconfig symbol contains useless 'help' text.
> --
> 
> commit afcf6c44814cd194adbadc7748b22b52095d7d42
> Author: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> phy: lynx-28g: move lane mode helpers to new core module
> 
> This commit extracts lane mode helpers and shared structures from the
> lynx-28g driver into a new core module. The lane modes are exported to
> a public header to allow coordination with the upcoming lynx-10g driver.
> 
> > diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
> > index 81f53564ee156..a87429f634ea8 100644
> > --- a/drivers/phy/freescale/Kconfig
> > +++ b/drivers/phy/freescale/Kconfig
> > @@ -51,11 +51,18 @@ config PHY_FSL_SAMSUNG_HDMI_PHY
> >  	  Enable this to add support for the Samsung HDMI PHY in i.MX8MP.
> >  endif
> >  
> > +config PHY_FSL_LYNX_CORE
> > +	tristate
> > +	help
> > +	  Enable this to add common support code for NXP Lynx 10G and Lynx 28G
> > +	  SerDes blocks.
> 
> [Severity: Low]
> This isn't a bug, but does this invisible Kconfig symbol need a help text?
> 
> Since this tristate doesn't have a prompt string, it won't be visible in
> menuconfig, making the help text inaccessible to users.

Will remove help text.

> > diff --git a/include/soc/fsl/phy-fsl-lynx.h b/include/soc/fsl/phy-fsl-lynx.h
> > new file mode 100644
> > index 0000000000000..92e8272d5ae15
> > --- /dev/null
> > +++ b/include/soc/fsl/phy-fsl-lynx.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/* Copyright 2023-2026 NXP */
> > +
> > +#ifndef __PHY_FSL_LYNX_H_
> > +#define __PHY_FSL_LYNX_H_
> > +
> > +enum lynx_lane_mode {
> > +	LANE_MODE_UNKNOWN,
> > +	LANE_MODE_1000BASEX_SGMII,
> > +	LANE_MODE_10GBASER,
> > +	LANE_MODE_USXGMII,
> > +	LANE_MODE_25GBASER,
> > +	LANE_MODE_MAX,
> > +};
> 
> [Severity: Low]
> Since this header is now globally accessible under include/soc/, could these
> generic enum values cause namespace collisions?
> 
> Values like LANE_MODE_UNKNOWN and LANE_MODE_MAX lack a subsystem or
> driver-specific prefix (like LYNX_ or FSL_). If another subsystem defines
> a macro or enum with the same generic name, it might lead to build failures.

In a vacuum this is a valid point, but unless I am being told by a human
to make this refactoring, the risk of namespacing conflicts will have to
be there. The churn resulting from renaming these will be painful for me
to manage and will result in additional patches too. At least we can go
with the lane mode variables named this way and rename them later, if it
ever becomes a practical problem (hopefully I can upstream the rest of
the code by then, so I don't have to resolve merge conflicts in tens of
patches).

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy