Received-SPF: pass (google.com: domain of linux-kernel+bounces-204087-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 6 Jun 2024 13:11:37 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>
Cc: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, 
	Russell King <linux@armlinux.org.uk>, Alexandre Torgue <alexandre.torgue@foss.st.com>, 
	Jose Abreu <joabreu@synopsys.com>, Jose Abreu <Jose.Abreu@synopsys.com>, 
	Vladimir Oltean <olteanv@gmail.com>, Florian Fainelli <f.fainelli@gmail.com>, 
	Maxime Chevallier <maxime.chevallier@bootlin.com>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	Sagar Cheluvegowda <quic_scheluve@quicinc.com>, Andrew Halaney <ahalaney@redhat.com>, 
	Jiawen Wu <jiawenwu@trustnetic.com>, Mengyuan Lou <mengyuanlou@net-swift.com>, 
	Tomer Maimon <tmaimon77@gmail.com>, openbmc@lists.ozlabs.org, netdev@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2 04/10] net: pcs: xpcs: Convert xpcs_compat to
 dw_xpcs_compat
Message-ID: <dwustofr63k6d76oayale5w6smjjjy43clu2hjphav2i5fro7q@gy65zuplacxx>
References: <20240602143636.5839-1-fancer.lancer@gmail.com>
 <20240602143636.5839-5-fancer.lancer@gmail.com>
 <4a6aa0ba-a5ff-4d28-8ad4-12d461e44381@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4a6aa0ba-a5ff-4d28-8ad4-12d461e44381@quicinc.com>

Hi Abhishek

On Wed, Jun 05, 2024 at 12:15:54PM -0700, Abhishek Chauhan (ABC) wrote:
> 
> > @@ -482,7 +482,7 @@ static int xpcs_config_aneg_c73(struct dw_xpcs *xpcs,
> >  
> >  static int xpcs_aneg_done_c73(struct dw_xpcs *xpcs,
> >  			      struct phylink_link_state *state,
> > -			      const struct xpcs_compat *compat, u16 an_stat1)
> > +			      const struct dw_xpcs_compat *compat, u16 an_stat1)
> >  {
> >  	int ret;
> >  
> > @@ -607,7 +607,7 @@ static int xpcs_validate(struct phylink_pcs *pcs, unsigned long *supported,
> >  			 const struct phylink_link_state *state)
> >  {
> >  	__ETHTOOL_DECLARE_LINK_MODE_MASK(xpcs_supported) = { 0, };
> > -	const struct xpcs_compat *compat;
> > +	const struct dw_xpcs_compat *compat;
> >  	struct dw_xpcs *xpcs;
> >  	int i;
> >  
> > @@ -633,7 +633,7 @@ void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces)
> >  	int i, j;
> >  
> >  	for (i = 0; i < DW_XPCS_INTERFACE_MAX; i++) {
> > -		const struct xpcs_compat *compat = &xpcs->desc->compat[i];
> > +		const struct dw_xpcs_compat *compat = &xpcs->desc->compat[i];
> >  
> >  		for (j = 0; j < compat->num_interfaces; j++)
> >  			__set_bit(compat->interface[j], interfaces);
> > @@ -850,7 +850,7 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
> >  int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
> >  		   const unsigned long *advertising, unsigned int neg_mode)
> >  {
> > -	const struct xpcs_compat *compat;
> > +	const struct dw_xpcs_compat *compat;
> >  	int ret;
> >  
> >  	compat = xpcs_find_compat(xpcs->desc, interface);
> > @@ -915,7 +915,7 @@ static int xpcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> >  
> >  static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
> >  			      struct phylink_link_state *state,
> > -			      const struct xpcs_compat *compat)
> > +			      const struct dw_xpcs_compat *compat)
> >  {
> >  	bool an_enabled;
> >  	int pcs_stat1;
> > @@ -1115,7 +1115,7 @@ static void xpcs_get_state(struct phylink_pcs *pcs,
> >  			   struct phylink_link_state *state)
> >  {
> >  	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
> > -	const struct xpcs_compat *compat;
> > +	const struct dw_xpcs_compat *compat;
> >  	int ret;
> >  
> >  	compat = xpcs_find_compat(xpcs->desc, state->interface);
> > @@ -1269,7 +1269,7 @@ static u32 xpcs_get_id(struct dw_xpcs *xpcs)
> >  	return 0xffffffff;
> >  }
> >  
> > -static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> > +static const struct dw_xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> >  	[DW_XPCS_USXGMII] = {
> >  		.supported = xpcs_usxgmii_features,
> >  		.interface = xpcs_usxgmii_interfaces,
> > @@ -1314,7 +1314,7 @@ static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> >  	},
> >  };
> >  
> Serge, Thank you for raising these patches. Minor comments which shows warning on my workspace. 
> 

> WARNING: line length of 82 exceeds 80 columns
> #153: FILE: drivers/net/pcs/pcs-xpcs.c:1272:
> +static const struct dw_xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> 
> WARNING: line length of 85 exceeds 80 columns
> #162: FILE: drivers/net/pcs/pcs-xpcs.c:1317:
> +static const struct dw_xpcs_compat nxp_sja1105_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> 
> WARNING: line length of 85 exceeds 80 columns
> #171: FILE: drivers/net/pcs/pcs-xpcs.c:1327:
> +static const struct dw_xpcs_compat nxp_sja1110_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> 

My checkpatch didn't warn about that even with the strict argument
specified.

Note there is just 3 and 6 characters over the preferable limit.
Splitting the lines will make the code less readable (in some extent).

So from that perspective it's ok to exceed 80 characters limit in this
case and not to break the generic kernel coding style convention.
Unless the networking subsystem has a more strict requirement in this
matter.

-Serge(y)

> > -static const struct xpcs_compat nxp_sja1105_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> > +static const struct dw_xpcs_compat nxp_sja1105_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> >  	[DW_XPCS_SGMII] = {
> >  		.supported = xpcs_sgmii_features,
> >  		.interface = xpcs_sgmii_interfaces,
> > @@ -1324,7 +1324,7 @@ static const struct xpcs_compat nxp_sja1105_xpcs_compat[DW_XPCS_INTERFACE_MAX] =
> >  	},
> >  };
> >  
> > -static const struct xpcs_compat nxp_sja1110_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> > +static const struct dw_xpcs_compat nxp_sja1110_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
> >  	[DW_XPCS_SGMII] = {
> >  		.supported = xpcs_sgmii_features,
> >  		.interface = xpcs_sgmii_interfaces,
> > @@ -1418,7 +1418,7 @@ static int xpcs_init_id(struct dw_xpcs *xpcs)
> >  
> >  static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface)
> >  {
> > -	const struct xpcs_compat *compat;
> > +	const struct dw_xpcs_compat *compat;
> >  
> >  	compat = xpcs_find_compat(xpcs->desc, interface);
> >  	if (!compat)