David Brownell wrote:
quoted text > 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!
quoted text >
>>> +#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.
quoted text >
>>> +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
