Re: R: [PATCH v3 1/2] net: mdio: add ipq8064 mdio driver

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Fri, Feb 21, 2020 at 01:49:17AM +0100, ansuelsmth@gmail.com wrote:
> > On Fri, Feb 21, 2020 at 12:26:21AM +0100, Ansuel Smith wrote:
> > > +static int
> > > +ipq8064_mdio_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device_node *np = pdev->dev.of_node;
> > > +	struct ipq8064_mdio *priv;
> > > +	struct mii_bus *bus;
> > > +	int ret;
> > > +
> > > +	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
> > > +	if (!bus)
> > > +		return -ENOMEM;
> > > +
> > > +	bus->name = "ipq8064_mdio_bus";
> > > +	bus->read = ipq8064_mdio_read;
> > > +	bus->write = ipq8064_mdio_write;
> > > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev-
> > >dev));
> > > +	bus->parent = &pdev->dev;
> > > +
> > > +	priv = bus->priv;
> > > +	priv->base = syscon_node_to_regmap(np);
> > > +	if (IS_ERR_OR_NULL(priv->base)) {
> > > +		priv->base = syscon_regmap_lookup_by_phandle(np,
> > "master");
> > > +		if (IS_ERR_OR_NULL(priv->base)) {
> > > +			dev_err(&pdev->dev, "master phandle not
> > found\n");
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > 
> > I'm curious why you've kept this as-is given my comments?
> > 
> > If you don't agree with them, it would be helpful to reply to the
> > review email giving the reasons why.
>
> I read your command and now I understand what you mean. Since they both
> never return NULL the IS_ERR_OR_NULL is wrong and only IS_ERR should be
> used. Correct me if I'm wrong.
> About the error propagation, should I return the
> syscon_regmap_lookup_by_phandle
> error or I can keep the EINVAL error? 

Hi,

You probably want something like:

	priv->base = syscon_node_to_regmap(np);
	if (IS_ERR(priv->base) && priv->base != ERR_PTR(-EPROBE_DEFER))
		priv->base = syscon_regmap_lookup_by_phandle(np, "master");

	if (priv->base == ERR_PTR(-EPROBE_DEFER)) {
		return -EPROBE_DEFER;
	} else if (IS_ERR(priv->base)) {
		dev_err(&pdev->dev, "error getting syscon regmap, error=%pe\n",
			priv->base);
		return PTR_ERR(priv->base);
	}

Please ensure that you test the above, including the case where you
should fall through to syscon_regmap_lookup_by_phandle().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up