Re: [PATCH net-next v2 3/8] net: stmmac: mdio: simplify MDC clock divisor lookup

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Mar 05, 2026 at 10:42:37AM +0000, Russell King (Oracle) wrote:
> As each lookup now iterates over each table in the same way, simplfy
> the code to select the table, and then walk that table.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 29 +++++++------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index 6292911fb54b..c9f0b8b601d2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -535,6 +535,7 @@ static const struct stmmac_clk_rate stmmac_xgmac_csr_to_mdc[] = {
>   */
>  static u32 stmmac_clk_csr_set(struct stmmac_priv *priv)
>  {
> +	const struct stmmac_clk_rate *rates;
>  	unsigned long clk_rate;
>  	u32 value = ~0;
>  	int i;
> @@ -548,25 +549,17 @@ static u32 stmmac_clk_csr_set(struct stmmac_priv *priv)
>  	 * the frequency of clk_csr_i. So we do not change the default
>  	 * divider.
>  	 */
> -	for (i = 0; stmmac_std_csr_to_mdc[i].rate; i++)
> -		if (clk_rate > stmmac_std_csr_to_mdc[i].rate)
> -			break;
> -	if (stmmac_std_csr_to_mdc[i].cr != (u8)~0)
> -		value = stmmac_std_csr_to_mdc[i].cr;
> -
> -	if (priv->plat->flags & STMMAC_FLAG_HAS_SUN8I) {
> -		for (i = 0; stmmac_sun8i_csr_to_mdc[i].rate; i++)
> -			if (clk_rate > stmmac_sun8i_csr_to_mdc[i].rate)
> -				break;
> -		value = stmmac_sun8i_csr_to_mdc[i].cr;
> -	}
> +	rates = stmmac_std_csr_to_mdc;
> +	if (priv->plat->flags & STMMAC_FLAG_HAS_SUN8I)
> +		rates = stmmac_sun8i_csr_to_mdc;
> +	if (priv->plat->core_type == DWMAC_CORE_XGMAC)
> +		rates = stmmac_xgmac_csr_to_mdc;
>  
> -	if (priv->plat->core_type == DWMAC_CORE_XGMAC) {
> -		for (i = 0; stmmac_xgmac_csr_to_mdc[i].rate; i++)
> -			if (clk_rate > stmmac_xgmac_csr_to_mdc[i].rate)
> -				break;
> -		value = stmmac_xgmac_csr_to_mdc[i].cr;
> -	}
> +	for (i = 0; rates[i].rate; i++)
> +		if (clk_rate > rates[i].rate)
> +			break;
> +	if (rates[i].cr != (u8)~0)
> +		value = rates[i].cr;

Looking at the AI review, it seems to me that the AI review is
incorrect.

With reference to the full original code which can be seen in:
https://patchwork.kernel.org/project/netdevbpf/patch/E1vy69y-0000000Btwd-3oq7@rmk-PC.armlinux.org.uk/

If we look at the original code, then we can see that the intention
is that if clk_rate is smaller than the last real entry for the
sun8i and xgmac cases, the value should end up as zero - and this is
exactly what the new code does. So, the behaviour is preserved, even
though AI thinks it isn't.

AI seems to think that reaching the last entry for sun8i and xgmac
means we have an invalid rate. That is where AI is going wrong -
that is an incorrect "assumption".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!