Re: [PATCH net-next] net: phy: adjust fixed_phy_register() return value

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Dear Petri Gynther,

Sorry for the late answer.

On Wed,  1 Oct 2014 14:45:09 -0700 (PDT), Petri Gynther wrote:
> Adjust fixed_phy_register() to return struct phy_device *, so that
> it becomes easy to use fixed PHYs without device tree support:
> 
>   phydev = fixed_phy_register(PHY_POLL, &fixed_phy_status, NULL);
>   fixed_phy_set_link_update(phydev, fixed_phy_link_update);
>   phy_connect_direct(netdev, phydev, handler_fn, phy_interface);
> 
> This change is a prerequisite for modifying bcmgenet driver to work
> without a device tree on Broadcom's MIPS-based 7xxx platforms.
> 
> Signed-off-by: Petri Gynther <pgynther@google.com>

On the principle, I'm obviously fine, but I have one comment below.


>  	/* New binding */
>  	fixed_link_node = of_get_child_by_name(np, "fixed-link");
> @@ -299,7 +300,8 @@ int of_phy_register_fixed_link(struct device_node *np)
>  		status.asym_pause = of_property_read_bool(fixed_link_node,
>  							  "asym-pause");
>  		of_node_put(fixed_link_node);
> -		return fixed_phy_register(PHY_POLL, &status, np);
> +		phy = fixed_phy_register(PHY_POLL, &status, np);
> +		return (!phy || IS_ERR(phy));
>  	}
>  
>  	/* Old binding */
> @@ -310,7 +312,8 @@ int of_phy_register_fixed_link(struct device_node *np)
>  		status.speed = be32_to_cpu(fixed_link_prop[2]);
>  		status.pause = be32_to_cpu(fixed_link_prop[3]);
>  		status.asym_pause = be32_to_cpu(fixed_link_prop[4]);
> -		return fixed_phy_register(PHY_POLL, &status, np);
> +		phy = fixed_phy_register(PHY_POLL, &status, np);
> +		return (!phy || IS_ERR(phy));

I am not sure this return (!phy || IS_ERR(phy)) is doing the right
thing. This function is supposed to return an error code on failure, or
0 on success. I don't see how your error handling returns an error code
on failure. What about doing the more explicit:

		phy = fixed_phy_register(PHY_POLL, &status, np);
		if (IS_ERR(phy))
			return PTR_ERR(phy);
		else
			return 0;

Or am I missing something?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com