netdev - Re: [PATCH 2/2] phylib: make mdio-gpio work without OF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fa686aa40810311018q71248550oa65a5f20ff3bb8fe@mail.gmail.com>
Date:	Fri, 31 Oct 2008 11:18:26 -0600
From:	"Grant Likely" <grant.likely@...retlab.ca>
To:	"Paulius Zaleckas" <paulius.zaleckas@...tonika.lt>
Cc:	netdev@...r.kernel.org, linux-arm-kernel@...ts.arm.linux.org.uk,
	linux-embedded@...r.kernel.org,
	"Laurent Pinchart" <laurentp@...-semaphore.com>,
	"Mike Frysinger" <vapier.adi@...il.com>
Subject: Re: [PATCH 2/2] phylib: make mdio-gpio work without OF

On Fri, Oct 31, 2008 at 10:49 AM, Paulius Zaleckas
<paulius.zaleckas@...tonika.lt> wrote:
> make mdio-gpio work with non OpenFirmware gpio implementation.
>

Looking good, but it has too many #ifdef blocks for my taste.  In
particular, if the driver is refactored a bit then all the OF stuff
can be contained within a single ifdef block, as can all the non-OF
stuff.  Comments below...

> +#ifdef CONFIG_OF_GPIO
>  static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
>                                          struct device_node *np)
>  {
> @@ -87,34 +102,60 @@ static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
>        snprintf(bus->id, MII_BUS_ID_SIZE, "%x", bitbang->mdc);
>        return 0;
>  }
> -
> -static void __devinit add_phy(struct mii_bus *bus, struct device_node *np)
> +#else
> +static void __devinit mdio_gpio_bitbang_init(struct mii_bus *bus,
> +                                        struct mdio_gpio_platform_data *pdata,
> +                                       int bus_id)
>  {
> -       const u32 *data;
> -       int len, id, irq;
> +       struct mdio_gpio_info *bitbang = bus->priv;
> +
> +       bitbang->mdc = pdata->mdc;
> +       bitbang->mdio = pdata->mdio;
> +
> +       snprintf(bus->id, MII_BUS_ID_SIZE, "phy%i", bus_id);
> +}
> +#endif

mdio_ofgpio_bitbang_init() is such short function and it is only
called once inside the probe() function.  Rather than duplicate it, it
can probably be moved inside the OF probe function and do the same
thing for the non-OF probe().

>
> -       data = of_get_property(np, "reg", &len);
> -       if (!data || len != 4)
> +static void __devinit add_phy(struct mii_bus *bus, unsigned int phy_addr,
> +                             int phy_irq)
> +{
> +       if (phy_addr >= PHY_MAX_ADDR) {
> +               dev_err(bus->parent,
> +                       "Failed to add phy with invalid address: 0x%x",
> +                       phy_addr);
>                return;
> +       }
>
> -       id = *data;
> -       bus->phy_mask &= ~(1 << id);
> +       bus->phy_mask &= ~(1 << phy_addr);
>
> -       irq = of_irq_to_resource(np, 0, NULL);
> -       if (irq != NO_IRQ)
> -               bus->irq[id] = irq;
> +       if (phy_irq != NO_IRQ)
> +               bus->irq[phy_addr] = phy_irq;
>  }

I like the refactoring of add_phy

>
> +#ifdef CONFIG_OF_GPIO
>  static int __devinit mdio_ofgpio_probe(struct of_device *ofdev,
>                                         const struct of_device_id *match)
>  {
>        struct device_node *np = NULL;
> +       struct device *dev = &ofdev->dev;
> +#else
> +static int __devinit mdio_gpio_probe(struct platform_device *pdev)
> +{
> +       struct mdio_gpio_platform_data *pdata;
> +       struct device *dev = &pdev->dev;
> +#endif

Instead of doing multiple #ifdef sections throughout the probe
function, use one #ifdef block for the OF stuff and another for all
the non-OF stuff.  You can factor out any non-trivial common code
blocks into shared helper functions.

Otherwise, looking good.

Thanks,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html