Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

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

On Wed, Jun 17, 2020 at 08:54:23PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 17, 2020 at 8:49 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
> 
> ...
> 
> > > -     ret = of_address_to_resource(np, 0, &res);
> > > -     if (ret) {
> > > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +     if (!res) {
> > >               dev_err(&pdev->dev, "could not obtain address\n");
> > > -             return ret;
> > > +             return -EINVAL;
> > >       }
> >
> > I think, as you're completely rewriting the resource handling, it would
> > be a good idea to switch over to using devm_* stuff here.
> >
> >         void __iomem *regs;
> >
> >         regs = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(regs))
> {
> >                 dev_err(&pdev->dev, "could not map resource: %pe\n",
> >                         regs);
> 
> And just in case, this message is dup. The API has few of them
> depending on the error conditions.

I did try to check for that, but it seems it was rather buried.  This
seems to have been a common mistake, as I've seen patches removing
such things from various drivers, but I'm never sure which require that
treatment.

Maybe adding such details to the kerneldoc for the functions (maybe
actually _writing_ some kernel doc to describe what the functions are
doing) would be a good start to prevent this kind of thing...

There's a difference between the lazy kerneldoc style of:

/**
 * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
 *                                  device
 *
 * @pdev: platform device to use both for memory resource lookup as well as
 *        resource management
 * @index: resource index
 */

and the "lets try to be informative" style:

/**
 * phy_lookup_setting - lookup a PHY setting
 * @speed: speed to match
 * @duplex: duplex to match
 * @mask: allowed link modes
 * @exact: an exact match is required
 *
 * Search the settings array for a setting that matches the speed and
 * duplex, and which is supported.
 *
 * If @exact is unset, either an exact match or %NULL for no match will
 * be returned.
 *
 * If @exact is set, an exact match, the fastest supported setting at
 * or below the specified speed, the slowest supported setting, or if
 * they all fail, %NULL will be returned.
 */

;)

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