On Fri, Feb 21, 2020 at 01:49:17AM +0100, ansuelsmth@xxxxxxxxx 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
