Re: [PATCH net-next 5/6] net: dsa: use swnode fixed-link if using default params

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Jul 15, 2022 at 05:01:48PM +0100, Russell King (Oracle) wrote:
> Create and use a swnode fixed-link specification for phylink if no
> parameters are given in DT for a fixed-link. This allows phylink to
> be used for "default" cases for DSA and CPU ports. Enable the use
> of phylink in all cases for DSA and CPU ports.

> Co-developed by Vladimir Oltean and myself.

Why not to use

  Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

?

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

...

> +static struct {
> +	unsigned long mask;
> +	int speed;
> +	int duplex;
> +} phylink_caps_params[] = {
> +	{ MAC_400000FD, SPEED_400000, DUPLEX_FULL },
> +	{ MAC_200000FD, SPEED_200000, DUPLEX_FULL },
> +	{ MAC_100000FD, SPEED_100000, DUPLEX_FULL },
> +	{ MAC_56000FD,  SPEED_56000,  DUPLEX_FULL },
> +	{ MAC_50000FD,  SPEED_50000,  DUPLEX_FULL },
> +	{ MAC_40000FD,  SPEED_40000,  DUPLEX_FULL },
> +	{ MAC_25000FD,  SPEED_25000,  DUPLEX_FULL },
> +	{ MAC_20000FD,  SPEED_20000,  DUPLEX_FULL },
> +	{ MAC_10000FD,  SPEED_10000,  DUPLEX_FULL },
> +	{ MAC_5000FD,   SPEED_5000,   DUPLEX_FULL },
> +	{ MAC_2500FD,   SPEED_2500,   DUPLEX_FULL },
> +	{ MAC_1000FD,   SPEED_1000,   DUPLEX_FULL },
> +	{ MAC_100FD,    SPEED_100,    DUPLEX_FULL },
> +	{ MAC_10FD,     SPEED_10,     DUPLEX_FULL },
> +	{ MAC_1000HD,   SPEED_1000,   DUPLEX_HALF },
> +	{ MAC_100HD,    SPEED_100,    DUPLEX_HALF },
> +	{ MAC_10HD,     SPEED_10,     DUPLEX_HALF },
> +};
> +
> +static int dsa_port_find_max_speed(unsigned long caps, int *speed, int *duplex)
> +{
> +	int i;
> +
> +	*speed = SPEED_UNKNOWN;
> +	*duplex = DUPLEX_UNKNOWN;
> +
> +	for (i = 0; i < ARRAY_SIZE(phylink_caps_params); i++) {
> +		if (caps & phylink_caps_params[i].mask) {
> +			*speed = phylink_caps_params[i].speed;
> +			*duplex = phylink_caps_params[i].duplex;

> +			break;

With the below check it's way too protective programming.

			return 0;

> +		}
> +	}
> +
> +	return *speed == SPEED_UNKNOWN ? -EINVAL : 0;

	return -EINVAL;

> +}

...

> +static struct fwnode_handle *dsa_port_get_fwnode(struct dsa_port *dp,
> +						 phy_interface_t mode)
> +{

> +	struct property_entry fixed_link_props[3] = { };
> +	struct property_entry port_props[3] = {};

A bit of consistency in the assignments?

Also it seems you are using up to 2 for the first one and only 1 in the second
one. IIUC it requires a terminator entry, so it means 3 and 2. Do we really
need 3 in the second case?

> +	struct fwnode_handle *fixed_link_fwnode;
> +	struct fwnode_handle *new_port_fwnode;
> +	struct device_node *dn = dp->dn;
> +	struct device_node *phy_node;
> +	int err, speed, duplex;
> +	unsigned long caps;
> +
> +	phy_node = of_parse_phandle(dn, "phy-handle", 0);

fwnode in the name, why not to use fwnode APIs?

	fwnode_find_reference();

> +	of_node_put(phy_node);
> +	if (phy_node || of_phy_is_fixed_link(dn))
> +		/* Nothing broken, nothing to fix.
> +		 * TODO: As discussed with Russell, maybe phylink could provide
> +		 * a more comprehensive helper to determine what constitutes a
> +		 * valid fwnode binding than this guerilla kludge.
> +		 */
> +		return of_fwnode_handle(dn);
> +
> +	if (mode == PHY_INTERFACE_MODE_NA)
> +		dsa_port_find_max_caps(dp, &mode, &caps);
> +	else
> +		caps = dp->pl_config.mac_capabilities &
> +		       phylink_interface_to_caps(mode);
> +
> +	err = dsa_port_find_max_speed(caps, &speed, &duplex);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed);
> +	if (duplex == DUPLEX_FULL)
> +		fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex");
> +
> +	port_props[0] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode));
> +
> +	new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> +	if (IS_ERR(new_port_fwnode))
> +		return new_port_fwnode;
> +
> +	/* Node needs to be named so that phylink's call to
> +	 * fwnode_get_named_child_node() finds it.
> +	 */
> +	fixed_link_fwnode = fwnode_create_named_software_node(fixed_link_props,
> +							      new_port_fwnode,
> +							      "fixed-link");
> +	if (IS_ERR(fixed_link_fwnode)) {
> +		fwnode_remove_software_node(new_port_fwnode);
> +		return fixed_link_fwnode;
> +	}
> +
> +	return new_port_fwnode;
> +}

-- 
With Best Regards,
Andy Shevchenko