Re: [PATCH net-next v3 6/6] net: dsa: microchip: parse PHY config from device tree

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Oleksij,

On Tue,  5 Nov 2024 10:09:44 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Introduce ksz_parse_dt_phy_config() to validate and parse PHY
> configuration from the device tree for KSZ switches. This function
> ensures proper setup of internal PHYs by checking `phy-handle`
> properties, verifying expected PHY IDs, and handling parent node
> mismatches. Sets the PHY mask on the MII bus if validation is
> successful. Returns -EINVAL on configuration errors.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 80 ++++++++++++++++++++++++--
>  1 file changed, 74 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 3909b55857430..cd1a466504180 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2373,6 +2373,77 @@ static void ksz_irq_phy_free(struct ksz_device *dev)
>  			irq_dispose_mapping(ds->user_mii_bus->irq[phy]);
>  }
> 
> +/**
> + * ksz_parse_dt_phy_config - Parse and validate PHY configuration from DT
> + * @dev: pointer to the KSZ device structure
> + * @bus: pointer to the MII bus structure
> + * @mdio_np: pointer to the MDIO node in the device tree
> + *
> + * This function parses and validates PHY configurations for each user port
> + * defined in the device tree for a KSZ switch device. It verifies that the
> + * `phy-handle` properties are correctly set and that the internal PHYs match
> + * expected IDs and parent nodes. Sets up the PHY mask in the MII bus if all
> + * validations pass. Logs error messages for any mismatches or missing data.
> + *
> + * Return: 0 on success, or a negative error code on failure.
> + */
> +static int ksz_parse_dt_phy_config(struct ksz_device *dev, struct mii_bus *bus,
> +				   struct device_node *mdio_np)
> +{
> +	struct device_node *phy_node, *phy_parent_node;
> +	bool phys_are_valid = true;
> +	struct dsa_port *dp;
> +	u32 phy_id;
> +	int ret;
> +
> +	dsa_switch_for_each_user_port(dp, dev->ds) {
> +		if (!dev->info->internal_phy[dp->index])
> +			continue;
> +
> +		phy_node = of_parse_phandle(dp->dn, "phy-handle", 0);
> +		if (!phy_node) {
> +			dev_err(dev->dev, "failed to parse phy-handle for port %d.\n",
> +				dp->index);
> +			phys_are_valid = false;
> +			continue;
> +		}
> +
> +		phy_parent_node = of_get_parent(phy_node);
> +		if (!phy_parent_node) {
> +			dev_err(dev->dev, "failed to get PHY-parent node for port %d\n",
> +				dp->index);
> +			phys_are_valid = false;
> +		} else if (dev->info->internal_phy[dp->index] &&
> +			   phy_parent_node != mdio_np) {

There's a check a few lines above that guarantees that at this point
dev->info->internal_phy[dp->index] will always evaluate as true,
so you could simplify that condition a bit :)

> +			dev_err(dev->dev, "PHY-parent node mismatch for port %d, expected %pOF, got %pOF\n",
> +				dp->index, mdio_np, phy_parent_node);
> +			phys_are_valid = false;
> +		} else {
> +			ret = of_property_read_u32(phy_node, "reg", &phy_id);
> +			if (ret < 0) {
> +				dev_err(dev->dev, "failed to read PHY ID for port %d. Error %d\n",
> +					dp->index, ret);
> +				phys_are_valid = false;
> +			} else if (phy_id != dev->phy_addr_map[dp->index]) {
> +				dev_err(dev->dev, "PHY ID mismatch for port %d, expected 0x%x, got 0x%x\n",
> +					dp->index, dev->phy_addr_map[dp->index],
> +					phy_id);

In this context, PHY ID might be a bit misleading, as PHY ID usually
refers to the identifier (OUI + model id used at probe to select the
driver). May I suggest phy_addr instead ?

Thanks,

Maxime