Re: [PATCH net-next v2 01/10] net: freescale: ucc_geth: Drop support for the "interface" DT property

[Simon Horman <horms@kernel.org>]

On Thu, Nov 14, 2024 at 04:35:52PM +0100, Maxime Chevallier wrote:
> In april 2007, ucc_geth was converted to phylib with :
> 
> commit 728de4c927a3 ("ucc_geth: migrate ucc_geth to phylib").
> 
> In that commit, the device-tree property "interface", that could be used to
> retrieve the PHY interface mode was deprecated.
> 
> DTS files that still used that property were converted along the way, in
> the following commit, also dating from april 2007 :
> 
> commit 0fd8c47cccb1 ("[POWERPC] Replace undocumented interface properties in dts files")
> 
> 17 years later, there's no users of that property left and I hope it's
> safe to say we can remove support from that in the ucc_geth driver,
> making the probe() function a bit simpler.
> 
> Should there be any users that have a DT that was generated when 2.6.21 was
> cutting-edge, print an error message with hints on how to convert the
> devicetree if the 'interface' property is found.
> 
> With that property gone, we can greatly simplify the parsing of the
> phy-interface-mode from the devicetree by using of_get_phy_mode(),
> allowing the removal of the open-coded parsing in the driver.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> V2: No changes
> 
>  drivers/net/ethernet/freescale/ucc_geth.c | 63 +++++------------------
>  1 file changed, 12 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c

...

> @@ -3627,18 +3588,17 @@ static int ucc_geth_probe(struct platform_device* ofdev)
>  	/* Find the TBI PHY node.  If it's not there, we don't support SGMII */
>  	ug_info->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
>  
> -	/* get the phy interface type, or default to MII */
> -	prop = of_get_property(np, "phy-connection-type", NULL);
> -	if (!prop) {
> -		/* handle interface property present in old trees */
> -		prop = of_get_property(ug_info->phy_node, "interface", NULL);
> -		if (prop != NULL) {
> -			phy_interface = enet_to_phy_interface[*prop];
> -			max_speed = enet_to_speed[*prop];
> -		} else
> -			phy_interface = PHY_INTERFACE_MODE_MII;
> -	} else {
> -		phy_interface = to_phy_interface((const char *)prop);
> +	prop = of_get_property(ug_info->phy_node, "interface", NULL);
> +	if (prop) {
> +		dev_err(&ofdev->dev,
> +			"Device-tree property 'interface' is no longer supported. Please use 'phy-connection-type' instead.");
> +		goto err_put_tbi;

Hi Maxime,

This goto will result in err being returned by this function.
But here err is 0. Should it be set to an error value instead?

Flagged by Smatch.

> +	}
> +
> +	err = of_get_phy_mode(np, &phy_interface);
> +	if (err) {
> +		dev_err(&ofdev->dev, "Invalid phy-connection-type");
> +		goto err_put_tbi;
>  	}
>  
>  	/* get speed, or derive from PHY interface */
> @@ -3746,6 +3706,7 @@ static int ucc_geth_probe(struct platform_device* ofdev)
>  err_deregister_fixed_link:
>  	if (of_phy_is_fixed_link(np))
>  		of_phy_deregister_fixed_link(np);
> +err_put_tbi:
>  	of_node_put(ug_info->tbi_node);
>  	of_node_put(ug_info->phy_node);
>  	return err;
> -- 
> 2.47.0
> 
>