| Message ID | 20081027105318.21923.24436.stgit@Programuotojas.82-135-208-232.ip.zebra.lt |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jeff Garzik |
| Headers | show |
On Mon, Oct 27, 2008 at 12:53:22PM +0200, Paulius Zaleckas wrote: > Useful for machines where PHY control is connected to GPIO. > This driver also supports interrupts from PHY. > > Changes since v1: > - fixed releasing of gpio (thanks to Sascha Hauer) > - fixed compiling due to bus->dev to bus->parent change > > Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt> > --- Well structured driver. Comments below. > diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c > new file mode 100644 > index 0000000..585897b > --- /dev/null > +++ b/drivers/net/phy/mdio-gpio.c > @@ -0,0 +1,187 @@ > +/* > + * GPIO based MDIO bitbang driver. > + * > + * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt> > + * > + * Based on mdio-ofgpio.c: > + * > + * Copyright (c) 2008 CSE Semaphore Belgium. > + * by Laurent Pinchart <laurentp@cse-semaphore.com> > + * > + * Copyright (c) 2003 Intracom S.A. > + * by Pantelis Antoniou <panto@intracom.gr> > + * > + * 2005 (c) MontaVista Software, Inc. > + * Vitaly Bordug <vbordug@ru.mvista.com> > + * > + * 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. > + */ > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/platform_device.h> > +#include <linux/gpio.h> > +#include <linux/mdio-bitbang.h> > +#include <linux/mdio-gpio.h> Missing: MODULE_AUTHOR() MODULE_LICENSE() MODULE_DESCRIPTION() > +static int __devinit mdio_gpio_probe(struct platform_device *pdev) > +{ > + struct mii_bus *new_bus; > + struct mdio_gpio_info *bitbang; > + struct mdio_gpio_platform_data *pdata; > + int ret = -ENOMEM; > + int i; > + > + pdata = pdev->dev.platform_data; > + if (pdata == NULL) > + goto out; > + > + bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL); > + if (!bitbang) > + goto out; > + > + bitbang->ctrl.ops = &mdio_gpio_ops; > + bitbang->mdc = pdata->mdc; > + bitbang->mdio = pdata->mdio; > + > + if (gpio_request(bitbang->mdc, "mdc")) > + goto out_free_bitbang; > + > + if (gpio_request(bitbang->mdio, "mdio")) > + goto out_free_mdc; > + > + new_bus = alloc_mdio_bitbang(&bitbang->ctrl); > + if (!new_bus) > + goto out_free_gpio; > + > + new_bus->name = "GPIO Bitbanged MII", > + snprintf(new_bus->id, MII_BUS_ID_SIZE, "phy%i", pdev->id); > + > + new_bus->phy_mask = ~0; > + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL); > + if (!new_bus->irq) > + goto out_free_bus; The IRQ array is fixed size. You can add it to the mdio_gpio_info structure and then just set the pointer here so that only one kzalloc is needed. > + > + for (i = 0; i < PHY_MAX_ADDR; i++) > + new_bus->irq[i] = PHY_POLL; > + > + for (i = 0; i < pdata->nr_phys; i++) { > + unsigned int phy_addr = pdata->phys[i].addr; > + > + BUG_ON(phy_addr >= PHY_MAX_ADDR); Is it really worth bringing down the whole kernel when too many phys are specified in pdata? > + > + new_bus->phy_mask &= ~(1 << phy_addr); > + new_bus->irq[phy_addr] = pdata->phys[i].irq; > + } > + > + new_bus->parent = &pdev->dev; > + platform_set_drvdata(pdev, new_bus); > + > + ret = mdiobus_register(new_bus); > + if (ret) > + goto out_free_all; > + > + return 0; > + > +out_free_all: > + platform_set_drvdata(pdev, NULL); > + kfree(new_bus->irq); > +out_free_bus: > + free_mdio_bitbang(new_bus); > +out_free_gpio: > + gpio_free(bitbang->mdio); > +out_free_mdc: > + gpio_free(bitbang->mdc); > +out_free_bitbang: > + kfree(bitbang); > +out: > + return ret; Nit: labels in column 0 will confuse diff when it tries to put the function name in the diff hunk header. If you indent the labels by 1 space then future diffs will show the function name instead of the label name in diff hunk headers. > +} > + > +static int __devexit mdio_gpio_remove(struct platform_device *pdev) > +{ > + struct mii_bus *bus = platform_get_drvdata(pdev); > + struct mdio_gpio_info *bitbang = bus->priv; > + > + mdiobus_unregister(bus); > + kfree(bus->irq); > + free_mdio_bitbang(bus); > + platform_set_drvdata(pdev, NULL); > + gpio_free(bitbang->mdc); > + gpio_free(bitbang->mdio); > + kfree(bitbang); > + > + return 0; > +} > + > +static struct platform_driver mdio_gpio_driver = { > + .probe = mdio_gpio_probe, > + .remove = __devexit_p(mdio_gpio_remove), > + .driver = { > + .name = "mdio-gpio", > + .owner = THIS_MODULE, > + }, > +}; > + > +static int mdio_gpio_init(void) static int __init mdio_gpio_init(void) > +{ > + return platform_driver_register(&mdio_gpio_driver); > +} > + > +static void mdio_gpio_exit(void) static void __exit mdio_gpio_exit(void) > +{ > + platform_driver_unregister(&mdio_gpio_driver); > +} > + > +module_init(mdio_gpio_init); > +module_exit(mdio_gpio_exit); -- 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
On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote: > +config MDIO_GPIO > + tristate "Support for GPIO bitbanged MDIO buses" > + depends on MDIO_BITBANG && GENERIC_GPIO > + help > + Supports MDIO busses connected to GPIO. please add a line stating the name of the module if it is built as such > +static struct mdiobb_ops mdio_gpio_ops = { > + .owner = THIS_MODULE, > + .set_mdc = mdc, > + .set_mdio_dir = mdio_dir, > + .set_mdio_data = mdio, > + .get_mdio_data = mdio_read, > +}; shouldnt the function names match the gpio_ops ? things like "mdc" and "mdio" are a little obscure ... > +static int __devinit mdio_gpio_probe(struct platform_device *pdev) > +{ > + struct mii_bus *new_bus; > + struct mdio_gpio_info *bitbang; > + struct mdio_gpio_platform_data *pdata; > + int ret = -ENOMEM; > + int i; > + > + pdata = pdev->dev.platform_data; > + if (pdata == NULL) > + goto out; > + > + bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL); sizeof(*bitbang) tends to read better and be more resistant to bitrot > + if (gpio_request(bitbang->mdc, "mdc")) > + goto out_free_bitbang; > + > + if (gpio_request(bitbang->mdio, "mdio")) > + goto out_free_mdc; maybe include driver name and/or the platform id ? if you have multiple mdio-gpio's running at the same time, coordinating may get a little messy ... > + new_bus->name = "GPIO Bitbanged MII", platform id here too ? > +static int mdio_gpio_init(void) > +{ > + return platform_driver_register(&mdio_gpio_driver); > +} needs __init markings > +static void mdio_gpio_exit(void) > +{ > + platform_driver_unregister(&mdio_gpio_driver); > +} needs __exit markings -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
On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote:
> + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
oh, and this should be kcalloc()
-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
On Monday 27 October 2008, Grant Likely wrote: > On Mon, Oct 27, 2008 at 12:53:22PM +0200, Paulius Zaleckas wrote: > > Useful for machines where PHY control is connected to GPIO. > > This driver also supports interrupts from PHY. I get a kick out of seeing each new generic driver using the generic GPIO interface. I *should* have expected it, obviously. ;) With a few exceptions I'll second Grant's comments, and pick a few more nits. > > +#include <linux/kernel.h> > > +#include <linux/init.h> > > +#include <linux/platform_device.h> > > +#include <linux/gpio.h> > > +#include <linux/mdio-bitbang.h> > > +#include <linux/mdio-gpio.h> > > Missing: > > MODULE_AUTHOR() > MODULE_LICENSE() > MODULE_DESCRIPTION() ... which many of us like to see at the *end* of the driver, with other module housekeeping (driver registration), instead of duplicating the header contents we just saw. > > +static int __devinit mdio_gpio_probe(struct platform_device *pdev) There are a few cases where platform drivers can't use __init and platform_driver_probe(), instead of __devinit paired with platform_driver_register(). Does this need to be one of them? That is, are these platform devices going to be hotplugged? (Usually because they are driver model children of other devices which get hotplugged.) > > +out: > > + return ret; > > Nit: labels in column 0 will confuse diff when it tries to put the > function name in the diff hunk header. If you indent the labels by 1 > space then future diffs will show the function name instead of the label > name in diff hunk headers. ... but please don't change drivers to work around cosmitic diff bugs ... > > +static int __devexit mdio_gpio_remove(struct platform_device *pdev) As above: if these devices are really hotpluggable, so be it. But that's the exception for platform_device nodes, not the rule, so I'd normally use __exit here (and __exit_p in the driver structure, later) to shrink the runtime code footprint. > > +static void mdio_gpio_exit(void) > > static void __exit mdio_gpio_exit(void) > > > +{ > > + platform_driver_unregister(&mdio_gpio_driver); > > +} > > + > > +module_init(mdio_gpio_init); > > +module_exit(mdio_gpio_exit); Also, snug the module_init()/module_exit() up against the functions they affect, just as with EXPORT_SYMBOL(). -- 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
Mike Frysinger wrote: > On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote: >> + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL); > > oh, and this should be kcalloc() Why? kcalloc() fills allocated memory with zeros and in this case it has to be filled with -1... this is done by further for() routine. I don't see the point to fill it with zeros before... > -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
On Tue, Oct 28, 2008 at 03:37, Paulius Zaleckas wrote: > Mike Frysinger wrote: >> On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote: >>> + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL); >> >> oh, and this should be kcalloc() > > Why? kcalloc() fills allocated memory with zeros and in this case it has > to be filled with -1... this is done by further for() routine. I don't > see the point to fill it with zeros before... i thought it was a call to kzalloc(). kcalloc() is better for arrays, but if you dont need the memory zero-ed first, it's a hard sell of proper style vs wasted performance. -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
Grant Likely wrote: > On Mon, Oct 27, 2008 at 12:53:22PM +0200, Paulius Zaleckas wrote: >> Useful for machines where PHY control is connected to GPIO. >> This driver also supports interrupts from PHY. >> >> Changes since v1: >> - fixed releasing of gpio (thanks to Sascha Hauer) >> - fixed compiling due to bus->dev to bus->parent change >> >> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt> >> --- > > Well structured driver. Comments below. Thanks for review. >> diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c >> new file mode 100644 >> index 0000000..585897b >> --- /dev/null >> +++ b/drivers/net/phy/mdio-gpio.c [...] > >> +static int __devinit mdio_gpio_probe(struct platform_device *pdev) >> +{ >> + struct mii_bus *new_bus; >> + struct mdio_gpio_info *bitbang; >> + struct mdio_gpio_platform_data *pdata; >> + int ret = -ENOMEM; >> + int i; >> + >> + pdata = pdev->dev.platform_data; >> + if (pdata == NULL) >> + goto out; >> + >> + bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL); >> + if (!bitbang) >> + goto out; >> + >> + bitbang->ctrl.ops = &mdio_gpio_ops; >> + bitbang->mdc = pdata->mdc; >> + bitbang->mdio = pdata->mdio; >> + >> + if (gpio_request(bitbang->mdc, "mdc")) >> + goto out_free_bitbang; >> + >> + if (gpio_request(bitbang->mdio, "mdio")) >> + goto out_free_mdc; >> + >> + new_bus = alloc_mdio_bitbang(&bitbang->ctrl); >> + if (!new_bus) >> + goto out_free_gpio; >> + >> + new_bus->name = "GPIO Bitbanged MII", >> + snprintf(new_bus->id, MII_BUS_ID_SIZE, "phy%i", pdev->id); >> + >> + new_bus->phy_mask = ~0; >> + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL); >> + if (!new_bus->irq) >> + goto out_free_bus; > > The IRQ array is fixed size. You can add it to the mdio_gpio_info > structure and then just set the pointer here so that only one kzalloc > is needed. It can be put in mdio_gpio_info, but please note that mdio_gpio_info is allocated with kzalloc() and irq with kmalloc(), because there is no need to fill this array with zeros(see below). >> + >> + for (i = 0; i < PHY_MAX_ADDR; i++) >> + new_bus->irq[i] = PHY_POLL; >> + >> + for (i = 0; i < pdata->nr_phys; i++) { >> + unsigned int phy_addr = pdata->phys[i].addr; >> + >> + BUG_ON(phy_addr >= PHY_MAX_ADDR); > > Is it really worth bringing down the whole kernel when too many phys are > specified in pdata? > >> + >> + new_bus->phy_mask &= ~(1 << phy_addr); >> + new_bus->irq[phy_addr] = pdata->phys[i].irq; >> + } >> + >> + new_bus->parent = &pdev->dev; >> + platform_set_drvdata(pdev, new_bus); >> + >> + ret = mdiobus_register(new_bus); >> + if (ret) >> + goto out_free_all; >> + >> + return 0; -- 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
Thanks for review. Comments below. Mike Frysinger wrote: > On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote: [...] >> +static int __devinit mdio_gpio_probe(struct platform_device *pdev) >> +{ >> + struct mii_bus *new_bus; >> + struct mdio_gpio_info *bitbang; >> + struct mdio_gpio_platform_data *pdata; >> + int ret = -ENOMEM; >> + int i; >> + >> + pdata = pdev->dev.platform_data; >> + if (pdata == NULL) >> + goto out; >> + >> + bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL); > > sizeof(*bitbang) tends to read better and be more resistant to bitrot > >> + if (gpio_request(bitbang->mdc, "mdc")) >> + goto out_free_bitbang; >> + >> + if (gpio_request(bitbang->mdio, "mdio")) >> + goto out_free_mdc; > > maybe include driver name and/or the platform id ? if you have > multiple mdio-gpio's running at the same time, coordinating may get a > little messy ... Well... this is mostly for debugging only... I don't like the idea to add additional char[..] variable and use sprintf... IMO this would be just a bloat... >> + new_bus->name = "GPIO Bitbanged MII", > > platform id here too ? If you take a look one line below you would see that bus ID is formed using platform id. Does this really need to be duplicated also in the name? >> +static int mdio_gpio_init(void) >> +{ >> + return platform_driver_register(&mdio_gpio_driver); >> +} > > needs __init markings > >> +static void mdio_gpio_exit(void) >> +{ >> + platform_driver_unregister(&mdio_gpio_driver); >> +} > > needs __exit markings > -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
David Brownell wrote: > On Monday 27 October 2008, Grant Likely wrote: >> On Mon, Oct 27, 2008 at 12:53:22PM +0200, Paulius Zaleckas wrote: >>> Useful for machines where PHY control is connected to GPIO. >>> This driver also supports interrupts from PHY. > > I get a kick out of seeing each new generic driver using > the generic GPIO interface. I *should* have expected it, > obviously. ;) > > With a few exceptions I'll second Grant's comments, and > pick a few more nits. Thanks for review! > >>> +#include <linux/kernel.h> >>> +#include <linux/init.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/gpio.h> >>> +#include <linux/mdio-bitbang.h> >>> +#include <linux/mdio-gpio.h> >> Missing: >> >> MODULE_AUTHOR() >> MODULE_LICENSE() >> MODULE_DESCRIPTION() > > ... which many of us like to see at the *end* of the driver, > with other module housekeeping (driver registration), instead > of duplicating the header contents we just saw. > > >>> +static int __devinit mdio_gpio_probe(struct platform_device *pdev) > > There are a few cases where platform drivers can't use __init > and platform_driver_probe(), instead of __devinit paired with > platform_driver_register(). Does this need to be one of them? > > That is, are these platform devices going to be hotplugged? > (Usually because they are driver model children of other devices > which get hotplugged.) I think there is possibility that this driver will be hotplugged... I agree that all devices that are explicitly on the SoC itself have to use __init and platform_driver_probe(), but it is not the case for this one... I will add MODULE_ALIAS for udev. > >>> +out: >>> + return ret; >> Nit: labels in column 0 will confuse diff when it tries to put the >> function name in the diff hunk header. If you indent the labels by 1 >> space then future diffs will show the function name instead of the label >> name in diff hunk headers. > > ... but please don't change drivers to work around cosmitic diff bugs ... > > >>> +static int __devexit mdio_gpio_remove(struct platform_device *pdev) > > As above: if these devices are really hotpluggable, so be it. > But that's the exception for platform_device nodes, not the rule, > so I'd normally use __exit here (and __exit_p in the driver > structure, later) to shrink the runtime code footprint. -- 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
On Tue, Oct 28, 2008 at 04:30, Paulius Zaleckas wrote: > Mike Frysinger wrote: >> On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote: >>> + if (gpio_request(bitbang->mdc, "mdc")) >>> + goto out_free_bitbang; >>> + >>> + if (gpio_request(bitbang->mdio, "mdio")) >>> + goto out_free_mdc; >> >> maybe include driver name and/or the platform id ? if you have >> multiple mdio-gpio's running at the same time, coordinating may get a >> little messy ... > > Well... this is mostly for debugging only... I don't like the idea > to add additional char[..] variable and use sprintf... > IMO this would be just a bloat... if realistically you'd only run one instance of this driver on a platform, then it'll probably be fine >>> + new_bus->name = "GPIO Bitbanged MII", >> >> platform id here too ? > > If you take a look one line below you would see that bus ID is formed > using platform id. Does this really need to be duplicated also in the > name? i guess if you can divine the differences from a populated /sys directory, it should be ok as well -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
On Tue, Oct 28, 2008 at 1:46 AM, Paulius Zaleckas <paulius.zaleckas@teltonika.lt> wrote: > Grant Likely wrote: >> The IRQ array is fixed size. You can add it to the mdio_gpio_info >> structure and then just set the pointer here so that only one kzalloc >> is needed. > > It can be put in mdio_gpio_info, but please note that mdio_gpio_info is > allocated with kzalloc() and irq with kmalloc(), because there is no need > to fill this array with zeros(see below). Adding an additional 32 words to be zeroed in the mdio_gpio_info kzalloc is considerably cheaper than doing an additional kmalloc. Plus, once the array is zeroed it is then in the cache and so the filling it with -1 also becomes cheaper. g.
On Tue, Oct 28, 2008 at 07:08:06AM -0600, Grant Likely wrote: > On Tue, Oct 28, 2008 at 1:46 AM, Paulius Zaleckas > <paulius.zaleckas@teltonika.lt> wrote: > > Grant Likely wrote: > >> The IRQ array is fixed size. You can add it to the mdio_gpio_info > >> structure and then just set the pointer here so that only one kzalloc > >> is needed. > > > > It can be put in mdio_gpio_info, but please note that mdio_gpio_info is > > allocated with kzalloc() and irq with kmalloc(), because there is no need > > to fill this array with zeros(see below). > > Adding an additional 32 words to be zeroed in the mdio_gpio_info > kzalloc is considerably cheaper than doing an additional kmalloc. > Plus, once the array is zeroed it is then in the cache and so the > filling it with -1 also becomes cheaper. Actually no, it doesn't become cheaper. You're making the assumption that cache lines are allocated when memory is written to. This isn't the case with the vast majority of ARM CPUs. That means zeroing allocated memory and then filling it with -1 makes it twice as expensive as just filling it with -1. -- 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
On Tue, Oct 28, 2008 at 9:17 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Oct 28, 2008 at 07:08:06AM -0600, Grant Likely wrote: >> On Tue, Oct 28, 2008 at 1:46 AM, Paulius Zaleckas >> <paulius.zaleckas@teltonika.lt> wrote: >> > Grant Likely wrote: >> >> The IRQ array is fixed size. You can add it to the mdio_gpio_info >> >> structure and then just set the pointer here so that only one kzalloc >> >> is needed. >> > >> > It can be put in mdio_gpio_info, but please note that mdio_gpio_info is >> > allocated with kzalloc() and irq with kmalloc(), because there is no need >> > to fill this array with zeros(see below). >> >> Adding an additional 32 words to be zeroed in the mdio_gpio_info >> kzalloc is considerably cheaper than doing an additional kmalloc. >> Plus, once the array is zeroed it is then in the cache and so the >> filling it with -1 also becomes cheaper. > > Actually no, it doesn't become cheaper. You're making the assumption > that cache lines are allocated when memory is written to. This isn't > the case with the vast majority of ARM CPUs. Okay, I wasn't aware of that on ARM. However it is still true that increasing the size of the kzalloc is cheaper than doing 2 allocs. g.
On Tue, Oct 28, 2008 at 09:30:10AM -0600, Grant Likely wrote: > On Tue, Oct 28, 2008 at 9:17 AM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Tue, Oct 28, 2008 at 07:08:06AM -0600, Grant Likely wrote: > >> On Tue, Oct 28, 2008 at 1:46 AM, Paulius Zaleckas > >> <paulius.zaleckas@teltonika.lt> wrote: > >> > Grant Likely wrote: > >> >> The IRQ array is fixed size. You can add it to the mdio_gpio_info > >> >> structure and then just set the pointer here so that only one kzalloc > >> >> is needed. > >> > > >> > It can be put in mdio_gpio_info, but please note that mdio_gpio_info is > >> > allocated with kzalloc() and irq with kmalloc(), because there is no need > >> > to fill this array with zeros(see below). > >> > >> Adding an additional 32 words to be zeroed in the mdio_gpio_info > >> kzalloc is considerably cheaper than doing an additional kmalloc. > >> Plus, once the array is zeroed it is then in the cache and so the > >> filling it with -1 also becomes cheaper. > > > > Actually no, it doesn't become cheaper. You're making the assumption > > that cache lines are allocated when memory is written to. This isn't > > the case with the vast majority of ARM CPUs. > > Okay, I wasn't aware of that on ARM. > > However it is still true that increasing the size of the kzalloc is > cheaper than doing 2 allocs. Yes, that 'irq' array might as well be part of the same structure. -- 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 --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index d55932a..b6a0350 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -84,6 +84,12 @@ config MDIO_BITBANG If in doubt, say N. +config MDIO_GPIO + tristate "Support for GPIO bitbanged MDIO buses" + depends on MDIO_BITBANG && GENERIC_GPIO + help + Supports MDIO busses connected to GPIO. + config MDIO_OF_GPIO tristate "Support for GPIO lib-based bitbanged MDIO buses" depends on MDIO_BITBANG && OF_GPIO diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index eee329f..8629b09 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -15,4 +15,5 @@ obj-$(CONFIG_ICPLUS_PHY) += icplus.o obj-$(CONFIG_REALTEK_PHY) += realtek.o obj-$(CONFIG_FIXED_PHY) += fixed.o obj-$(CONFIG_MDIO_BITBANG) += mdio-bitbang.o +obj-$(CONFIG_MDIO_GPIO) += mdio-gpio.o obj-$(CONFIG_MDIO_OF_GPIO) += mdio-ofgpio.o diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c new file mode 100644 index 0000000..585897b --- /dev/null +++ b/drivers/net/phy/mdio-gpio.c @@ -0,0 +1,187 @@ +/* + * GPIO based MDIO bitbang driver. + * + * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt> + * + * Based on mdio-ofgpio.c: + * + * Copyright (c) 2008 CSE Semaphore Belgium. + * by Laurent Pinchart <laurentp@cse-semaphore.com> + * + * Copyright (c) 2003 Intracom S.A. + * by Pantelis Antoniou <panto@intracom.gr> + * + * 2005 (c) MontaVista Software, Inc. + * Vitaly Bordug <vbordug@ru.mvista.com> + * + * 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. + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/gpio.h> +#include <linux/mdio-bitbang.h> +#include <linux/mdio-gpio.h> + +struct mdio_gpio_info { + struct mdiobb_ctrl ctrl; + unsigned int mdc, mdio; +}; + +static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir) +{ + struct mdio_gpio_info *bitbang = + container_of(ctrl, struct mdio_gpio_info, ctrl); + + if (dir) + gpio_direction_output(bitbang->mdio, 1); + else + gpio_direction_input(bitbang->mdio); +} + +static int mdio_read(struct mdiobb_ctrl *ctrl) +{ + struct mdio_gpio_info *bitbang = + container_of(ctrl, struct mdio_gpio_info, ctrl); + + return gpio_get_value(bitbang->mdio); +} + +static void mdio(struct mdiobb_ctrl *ctrl, int what) +{ + struct mdio_gpio_info *bitbang = + container_of(ctrl, struct mdio_gpio_info, ctrl); + + gpio_set_value(bitbang->mdio, what); +} + +static void mdc(struct mdiobb_ctrl *ctrl, int what) +{ + struct mdio_gpio_info *bitbang = + container_of(ctrl, struct mdio_gpio_info, ctrl); + + gpio_set_value(bitbang->mdc, what); +} + +static struct mdiobb_ops mdio_gpio_ops = { + .owner = THIS_MODULE, + .set_mdc = mdc, + .set_mdio_dir = mdio_dir, + .set_mdio_data = mdio, + .get_mdio_data = mdio_read, +}; + +static int __devinit mdio_gpio_probe(struct platform_device *pdev) +{ + struct mii_bus *new_bus; + struct mdio_gpio_info *bitbang; + struct mdio_gpio_platform_data *pdata; + int ret = -ENOMEM; + int i; + + pdata = pdev->dev.platform_data; + if (pdata == NULL) + goto out; + + bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL); + if (!bitbang) + goto out; + + bitbang->ctrl.ops = &mdio_gpio_ops; + bitbang->mdc = pdata->mdc; + bitbang->mdio = pdata->mdio; + + if (gpio_request(bitbang->mdc, "mdc")) + goto out_free_bitbang; + + if (gpio_request(bitbang->mdio, "mdio")) + goto out_free_mdc; + + new_bus = alloc_mdio_bitbang(&bitbang->ctrl); + if (!new_bus) + goto out_free_gpio; + + new_bus->name = "GPIO Bitbanged MII", + snprintf(new_bus->id, MII_BUS_ID_SIZE, "phy%i", pdev->id); + + new_bus->phy_mask = ~0; + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL); + if (!new_bus->irq) + goto out_free_bus; + + for (i = 0; i < PHY_MAX_ADDR; i++) + new_bus->irq[i] = PHY_POLL; + + for (i = 0; i < pdata->nr_phys; i++) { + unsigned int phy_addr = pdata->phys[i].addr; + + BUG_ON(phy_addr >= PHY_MAX_ADDR); + + new_bus->phy_mask &= ~(1 << phy_addr); + new_bus->irq[phy_addr] = pdata->phys[i].irq; + } + + new_bus->parent = &pdev->dev; + platform_set_drvdata(pdev, new_bus); + + ret = mdiobus_register(new_bus); + if (ret) + goto out_free_all; + + return 0; + +out_free_all: + platform_set_drvdata(pdev, NULL); + kfree(new_bus->irq); +out_free_bus: + free_mdio_bitbang(new_bus); +out_free_gpio: + gpio_free(bitbang->mdio); +out_free_mdc: + gpio_free(bitbang->mdc); +out_free_bitbang: + kfree(bitbang); +out: + return ret; +} + +static int __devexit mdio_gpio_remove(struct platform_device *pdev) +{ + struct mii_bus *bus = platform_get_drvdata(pdev); + struct mdio_gpio_info *bitbang = bus->priv; + + mdiobus_unregister(bus); + kfree(bus->irq); + free_mdio_bitbang(bus); + platform_set_drvdata(pdev, NULL); + gpio_free(bitbang->mdc); + gpio_free(bitbang->mdio); + kfree(bitbang); + + return 0; +} + +static struct platform_driver mdio_gpio_driver = { + .probe = mdio_gpio_probe, + .remove = __devexit_p(mdio_gpio_remove), + .driver = { + .name = "mdio-gpio", + .owner = THIS_MODULE, + }, +}; + +static int mdio_gpio_init(void) +{ + return platform_driver_register(&mdio_gpio_driver); +} + +static void mdio_gpio_exit(void) +{ + platform_driver_unregister(&mdio_gpio_driver); +} + +module_init(mdio_gpio_init); +module_exit(mdio_gpio_exit); diff --git a/include/linux/mdio-gpio.h b/include/linux/mdio-gpio.h new file mode 100644 index 0000000..82d5fa4 --- /dev/null +++ b/include/linux/mdio-gpio.h @@ -0,0 +1,40 @@ +/* + * MDIO-GPIO bus platform data structures + * + * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#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 */
Useful for machines where PHY control is connected to GPIO. This driver also supports interrupts from PHY. Changes since v1: - fixed releasing of gpio (thanks to Sascha Hauer) - fixed compiling due to bus->dev to bus->parent change Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt> --- drivers/net/phy/Kconfig | 6 + drivers/net/phy/Makefile | 1 drivers/net/phy/mdio-gpio.c | 187 +++++++++++++++++++++++++++++++++++++++++++ include/linux/mdio-gpio.h | 40 +++++++++ 4 files changed, 234 insertions(+), 0 deletions(-) create mode 100644 drivers/net/phy/mdio-gpio.c 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