Re: [PATCH net-next 13/15] net: dsa: microchip: bypass dev_ops for port_setup()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: microchip: bypass dev_ops for port_setup()

This commit removes an indirection layer for KSZ switches by connecting
the port_setup operations directly to dsa_switch_ops, bypassing the
ksz_dev_ops callback mechanism.

> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 7385aa4e788a1..2f63a3a7d6ceb 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
[ ... ]
> @@ -1309,6 +1309,23 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  	ksz_pwrite8(dev, port, regs[REG_PORT_PME_CTRL], 0);
>  }
>  
> +int ksz9477_dsa_port_setup(struct dsa_switch *ds, int port)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	int ret;
> +
> +	if (!dsa_is_user_port(ds, port))
> +		return 0;
> +
> +	ksz9477_port_setup(dev, port, false);
> +
> +	ret = ksz9477_set_default_prio_queue_mapping(dev, port);
> +	if (ret)
> +		return ret;
> +
> +	return ksz_dcb_init_port(dev, port);
> +};

This isn't a bug, but there's a stray semicolon at the end of the function definition.

[ ... ]
> diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> index ecb072cd99819..aff0c2735c307 100644
> --- a/drivers/net/dsa/microchip/lan937x_main.c
> +++ b/drivers/net/dsa/microchip/lan937x_main.c
[ ... ]
> @@ -823,7 +822,7 @@ const struct dsa_switch_ops lan937x_switch_ops = {
>  	.phy_read		= ksz_phy_read16,
>  	.phy_write		= ksz_phy_write16,
>  	.phylink_get_caps	= lan937x_phylink_get_caps,
> -	.port_setup		= ksz_port_setup,
> +	.port_setup		= ksz9477_dsa_port_setup,

Is it intentional to use ksz9477_dsa_port_setup() for LAN937x switches here?

Previously, the shared ksz_port_setup() dynamically dispatched the
initialization via dev->dev_ops->port_setup(), which correctly resolved to
lan937x_port_setup() for LAN937x devices.

Since the new ksz9477_dsa_port_setup() function explicitly hardcodes a call
to ksz9477_port_setup(), won't this cause LAN937x ports to skip their
specific initialization logic and instead receive incorrect register
configurations meant for KSZ9477 hardware?
-- 
pw-bot: cr