diff mbox

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

Message ID 20081031164934.20490.67539.stgit@Programuotojas.82-135-208-232.ip.zebra.lt
State Superseded, archived
Delegated to: Jeff Garzik
Headers show

Commit Message

Paulius Zaleckas Oct. 31, 2008, 4:49 p.m. UTC 
make mdio-gpio work with non OpenFirmware gpio implementation.

Aditional changes to mdio-gpio:
- use gpio_request() and gpio_free()
- place irq[] array in struct mdio_gpio_info
- add module description, author and license
- if NO_IRQ is not defined define it to 0
- add note about compiling this driver as module
- rename mdc and mdio function (were ugly names)
- change MII to MDIO in bus name
- add __init __exit to module (un)loading functions
- probe fails if no phys added to the bus
- kzalloc bitbang with sizeof(*bitbang)

Laurent, please test this driver under OF.

Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
Cc: Laurent Pinchart <laurentp@cse-semaphore.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Mike Frysinger <vapier.adi@gmail.com>
---

 drivers/net/phy/Kconfig     |    5 +
 drivers/net/phy/mdio-gpio.c |  188 +++++++++++++++++++++++++++++++++----------
 include/linux/mdio-gpio.h   |   30 +++++++
 3 files changed, 180 insertions(+), 43 deletions(-)
 create mode 100644 include/linux/mdio-gpio.h



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Grant Likely Oct. 31, 2008, 5:18 p.m. UTC | #1 
On Fri, Oct 31, 2008 at 10:49 AM, Paulius Zaleckas
<paulius.zaleckas@teltonika.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.
Mike Frysinger Oct. 31, 2008, 10:07 p.m. UTC | #2 
On Fri, Oct 31, 2008 at 12:49, Paulius Zaleckas wrote:
> +#ifndef NO_IRQ
> +#define NO_IRQ  0
> +#endif

discussions elsewhere indicate this should die.
-if (irq != NO_IRQ)
+if (irq)
-mike
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 0318077..c4c5a2f 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -86,8 +86,11 @@  config MDIO_BITBANG
 
 config MDIO_GPIO
 	tristate "Support for GPIO lib-based bitbanged MDIO buses"
-	depends on MDIO_BITBANG && OF_GPIO
+	depends on MDIO_BITBANG && GENERIC_GPIO
 	---help---
 	  Supports GPIO lib-based MDIO busses.
 
+	  To compile this driver as a module, choose M here: the module
+	  will be called mdio-gpio.
+
 endif # PHYLIB
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 2ff9775..a7b94ac 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -1,9 +1,12 @@ 
 /*
- * OpenFirmware GPIO based MDIO bitbang driver.
+ * GPIO based MDIO bitbang driver.
+ * Supports OpenFirmware.
  *
  * Copyright (c) 2008 CSE Semaphore Belgium.
  *  by Laurent Pinchart <laurentp@cse-semaphore.com>
  *
+ * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
+ *
  * Based on earlier work by
  *
  * Copyright (c) 2003 Intracom S.A.
@@ -22,12 +25,23 @@ 
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/mdio-bitbang.h>
-#include <linux/of_gpio.h>
-#include <linux/of_platform.h>
+#ifdef CONFIG_OF_GPIO
+# include <linux/of_gpio.h>
+# include <linux/of_platform.h>
+#else
+# include <linux/platform_device.h>
+# include <linux/gpio.h>
+# include <linux/mdio-gpio.h>
+#endif
+
+#ifndef NO_IRQ
+#define NO_IRQ  0
+#endif
 
 struct mdio_gpio_info {
 	struct mdiobb_ctrl ctrl;
 	int mdc, mdio;
+	int irq[PHY_MAX_ADDR];
 };
 
 static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
@@ -41,7 +55,7 @@  static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
 		gpio_direction_input(bitbang->mdio);
 }
 
-static int mdio_read(struct mdiobb_ctrl *ctrl)
+static int mdio_get(struct mdiobb_ctrl *ctrl)
 {
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
@@ -49,7 +63,7 @@  static int mdio_read(struct mdiobb_ctrl *ctrl)
 	return gpio_get_value(bitbang->mdio);
 }
 
-static void mdio(struct mdiobb_ctrl *ctrl, int what)
+static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
 {
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
@@ -57,7 +71,7 @@  static void mdio(struct mdiobb_ctrl *ctrl, int what)
 	gpio_set_value(bitbang->mdio, what);
 }
 
-static void mdc(struct mdiobb_ctrl *ctrl, int what)
+static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 {
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
@@ -67,12 +81,13 @@  static void mdc(struct mdiobb_ctrl *ctrl, int what)
 
 static struct mdiobb_ops mdio_gpio_ops = {
 	.owner = THIS_MODULE,
-	.set_mdc = mdc,
+	.set_mdc = mdc_set,
 	.set_mdio_dir = mdio_dir,
-	.set_mdio_data = mdio,
-	.get_mdio_data = mdio_read,
+	.set_mdio_data = mdio_set,
+	.get_mdio_data = mdio_get,
 };
 
+#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
 
-	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;
 }
 
+#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
 	struct mii_bus *new_bus;
 	struct mdio_gpio_info *bitbang;
 	int ret = -ENOMEM;
 	int i;
 
-	bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL);
+#ifndef CONFIG_OF_GPIO
+	pdata = dev->platform_data;
+	if (pdata == NULL)
+		goto out;
+#endif
+
+	bitbang = kzalloc(sizeof(*bitbang), GFP_KERNEL);
 	if (!bitbang)
 		goto out;
 
@@ -124,36 +165,66 @@  static int __devinit mdio_ofgpio_probe(struct of_device *ofdev,
 	if (!new_bus)
 		goto out_free_bitbang;
 
-	new_bus->name = "GPIO Bitbanged MII",
+	new_bus->name = "GPIO Bitbanged MDIO",
 
+#ifdef CONFIG_OF_GPIO
 	ret = mdio_ofgpio_bitbang_init(new_bus, ofdev->node);
 	if (ret)
 		goto out_free_bus;
+#else
+	mdio_gpio_bitbang_init(new_bus, pdata, pdev->id);
+#endif
 
-	new_bus->phy_mask = ~0;
-	new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
-	if (!new_bus->irq)
+	ret = -ENODEV;
+
+	if (gpio_request(bitbang->mdc, "mdc"))
 		goto out_free_bus;
 
+	if (gpio_request(bitbang->mdio, "mdio"))
+		goto out_free_mdc;
+
+	new_bus->phy_mask = ~0;
+	new_bus->irq = bitbang->irq;
+	new_bus->parent = dev;
+
 	for (i = 0; i < PHY_MAX_ADDR; i++)
-		new_bus->irq[i] = -1;
+		new_bus->irq[i] = PHY_POLL;
 
+#ifdef CONFIG_OF_GPIO
 	while ((np = of_get_next_child(ofdev->node, np)))
-		if (!strcmp(np->type, "ethernet-phy"))
-			add_phy(new_bus, np);
+		if (!strcmp(np->type, "ethernet-phy")) {
+			const u32 *data;
+			int len;
+
+			data = of_get_property(np, "reg", &len);
+			if (!data || len != 4)
+				continue;
+
+			add_phy(new_bus, *data,
+				of_irq_to_resource(np, 0, NULL));
+		}
+#else
+	for (i = 0; i < pdata->nr_phys; i++)
+		add_phy(new_bus, pdata->phys[i].addr, pdata->phys[i].irq);
+#endif
 
-	new_bus->parent = &ofdev->dev;
-	dev_set_drvdata(&ofdev->dev, new_bus);
+	if (new_bus->phy_mask == ~0)
+		goto out_free_gpio;
+
+	dev_set_drvdata(dev, new_bus);
 
 	ret = mdiobus_register(new_bus);
 	if (ret)
-		goto out_free_irqs;
+		goto out_free_all;
 
 	return 0;
 
-out_free_irqs:
-	dev_set_drvdata(&ofdev->dev, NULL);
-	kfree(new_bus->irq);
+out_free_all:
+	dev_set_drvdata(dev, NULL);
+out_free_gpio:
+	gpio_free(bitbang->mdio);
+out_free_mdc:
+	gpio_free(bitbang->mdc);
 out_free_bus:
 	free_mdio_bitbang(new_bus);
 out_free_bitbang:
@@ -162,20 +233,29 @@  out:
 	return ret;
 }
 
+#ifdef CONFIG_OF_GPIO
 static int mdio_ofgpio_remove(struct of_device *ofdev)
 {
-	struct mii_bus *bus = dev_get_drvdata(&ofdev->dev);
+	struct device *dev = &ofdev->dev;
+#else
+static int __devexit mdio_gpio_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+#endif
+	struct mii_bus *bus = dev_get_drvdata(dev);
 	struct mdio_gpio_info *bitbang = bus->priv;
 
 	mdiobus_unregister(bus);
-	kfree(bus->irq);
 	free_mdio_bitbang(bus);
-	dev_set_drvdata(&ofdev->dev, NULL);
+	dev_set_drvdata(dev, NULL);
+	gpio_free(bitbang->mdc);
+	gpio_free(bitbang->mdio);
 	kfree(bitbang);
 
 	return 0;
 }
 
+#ifdef CONFIG_OF_GPIO
 static struct of_device_id mdio_ofgpio_match[] = {
 	{
 		.compatible = "virtual,mdio-gpio",
@@ -189,16 +269,40 @@  static struct of_platform_driver mdio_ofgpio_driver = {
 	.probe = mdio_ofgpio_probe,
 	.remove = mdio_ofgpio_remove,
 };
+#else
+static struct platform_driver mdio_gpio_driver = {
+	.probe = mdio_gpio_probe,
+	.remove = __devexit_p(mdio_gpio_remove),
+	.driver		= {
+		.name	= "mdio-gpio",
+		.owner	= THIS_MODULE,
+	},
+};
+#endif
 
-static int mdio_ofgpio_init(void)
+static int __init mdio_gpio_init(void)
 {
+#ifdef CONFIG_OF_GPIO
 	return of_register_platform_driver(&mdio_ofgpio_driver);
+#else
+	return platform_driver_register(&mdio_gpio_driver);
+#endif
 }
+module_init(mdio_gpio_init);
 
-static void mdio_ofgpio_exit(void)
+static void __exit mdio_gpio_exit(void)
 {
+#ifdef CONFIG_OF_GPIO
 	of_unregister_platform_driver(&mdio_ofgpio_driver);
+#else
+	platform_driver_unregister(&mdio_gpio_driver);
+#endif
 }
-
-module_init(mdio_ofgpio_init);
-module_exit(mdio_ofgpio_exit);
+module_exit(mdio_gpio_exit);
+
+#ifndef CONFIG_OF_GPIO
+MODULE_ALIAS("platform:mdio-gpio");
+#endif
+MODULE_AUTHOR("Laurent Pinchart, Paulius Zaleckas");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Generic driver for MDIO bus emulation using GPIO");
diff --git a/include/linux/mdio-gpio.h b/include/linux/mdio-gpio.h
new file mode 100644
index 0000000..5ca24ed
--- /dev/null
+++ b/include/linux/mdio-gpio.h
@@ -0,0 +1,30 @@ 
+/*
+ * MDIO-GPIO bus platform data structures
+ *
+ * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#ifndef __LINUX_MDIO_GPIO_H
+#define __LINUX_MDIO_GPIO_H
+
+struct mdio_gpio_phy {
+	/* PHY address on MDIO bus */
+	unsigned int addr;
+	/* preconfigured irq connected to PHY or -1 if no irq */
+	int irq;
+};
+
+struct mdio_gpio_platform_data {
+	/* GPIO numbers for bus pins */
+	unsigned int mdc;
+	unsigned int mdio;
+
+	unsigned int nr_phys;
+	struct mdio_gpio_phy *phys;
+};
+
+#endif /* __LINUX_MDIO_GPIO_H */