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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Jul 15, 2022 at 11:11:18PM +0300, Andy Shevchenko wrote:
> 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>

Ah, that's an official thing. Thanks.

> > 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;

Ok.

> > +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?

Probably not - that came from Vladimir's patch, and I removed the "reg"
property without fixing this up. Thanks for spotting.

> > +	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();

Marcin has a series converting DSA to use fwnode things - currently DSA
does not support ACPI, so converting it to fwnode doesn't make that much
sese until the proper ACPI patches get merged, which have now been
rebased on this series by Marcin in the expectation that these patches
would be merged... so I don't want to tred on Marcin's feet on that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!