Re: [RFC PATCH v2 net-next 15/15] net: pcs: lynx: use MTIP AN/LT block for copper backplanes
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
Date: 2023-10-03 13:14:49
Also in:
linux-devicetree, linux-phy, lkml
On Sat, Sep 23, 2023 at 04:49:04PM +0300, Vladimir Oltean wrote:
+static int lynx_pcs_parse_fwnode(struct lynx_pcs *lynx)
+{
+ struct fwnode_handle *node = lynx->mdio->dev.fwnode;
+ enum mtip_model model = MTIP_MODEL_AUTODETECT;
+ struct device_node *np = to_of_node(node);
+ struct mdio_device *mdio = lynx->mdio;
+ struct device *dev = &mdio->dev;
+ struct phy *phy;
+ int i, err;
+
+ if (!node)
+ return 0;
+
+ lynx->backplane_mode = fwnode_property_present(node, "fsl,backplane-mode");
+ if (!lynx->backplane_mode)
+ return 0;
+
+ if (fwnode_device_is_compatible(node, "fsl,lx2160a-lynx-pcs"))
+ model = MTIP_MODEL_LX2160A;
+
+ lynx->num_lanes = of_count_phandle_with_args(np, "phys", "#phy-cells");
+ if (lynx->num_lanes < 0)
+ return lynx->num_lanes;Is it possible for ->num_lanes to be zero at this point? If that is possible, then ->anlt[PRIMARY_LANE] will be NULL but ->backplane_mode will be set, so won't that cause the mtip_* calls above to pass a NULL pointer into those functions? Is that safe? Should we trap that case here? If that's correct, then I don't see any point in storing ->backplane_mode, since we can then use ->num_lanes > PRIMARY_LANE or similar instead.
+ + if (WARN_ON(lynx->num_lanes > MAX_NUM_LANES)) + return -EINVAL;
Do we need to use WARN_ON() here, or would it be better to print a short error-level message?
+
+ for (i = 0; i < lynx->num_lanes; i++) {
+ phy = devm_of_phy_get_by_index(dev, np, i);
+ if (IS_ERR(phy))
+ return dev_err_probe(dev, PTR_ERR(phy),
+ "Failed to get SerDes PHY %d\n", i);
+
+ lynx->anlt[i] = mtip_backplane_create(mdio, phy, model);
+ if (IS_ERR(lynx->anlt[i])) {
+ err = PTR_ERR(lynx->anlt[i]);
+
+ while (i-- > 0)
+ mtip_backplane_destroy(lynx->anlt[i]);
+
+ return err;
+ }
+ }
+
+ for (i = 1; i < lynx->num_lanes; i++) {
+ err = mtip_backplane_add_subordinate(lynx->anlt[PRIMARY_LANE],
+ lynx->anlt[i]);
+ if (WARN_ON(err)) {Again, does this need to be a backtrace-producing WARN_ON()?
quoted hunk ↗ jump to hunk
+ /* Too many SerDes lanes in the device tree? */ + for (i = 0; i < lynx->num_lanes; i++) + mtip_backplane_destroy(lynx->anlt[i]); + return err; + } + } + + return 0; +} + static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) { struct lynx_pcs *lynx; + int err; lynx = kzalloc(sizeof(*lynx), GFP_KERNEL); if (!lynx)@@ -327,6 +451,12 @@ static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) lynx->pcs.neg_mode = true; lynx->pcs.poll = true; + err = lynx_pcs_parse_fwnode(lynx); + if (err) { + kfree(lynx); + return ERR_PTR(err); + } + return lynx_to_phylink_pcs(lynx); }@@ -392,6 +522,11 @@ EXPORT_SYMBOL_GPL(lynx_pcs_create_fwnode); void lynx_pcs_destroy(struct phylink_pcs *pcs) { struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); + int i; + + if (lynx->backplane_mode) + for (i = 0; i < lynx->num_lanes; i++) + mtip_backplane_destroy(lynx->anlt[i]);
Won't ->num_lanes only be non-zero when ->backplane_mode is set, so isn't the test for ->backplane_mode redundant here?
mdio_device_put(lynx->mdio); kfree(lynx); -- 2.34.1
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!