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 linux-embedded" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
