lkml.org 
[lkml]   [2020]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: R: [PATCH v3 1/2] net: mdio: add ipq8064 mdio driver
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

\
 
 \ /
  Last update: 2020-02-21 12:53    [W:0.933 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site