Re: [PATCH] phylib: add mdio-gpio bus driver (v2)

[Paulius Zaleckas <paulius.zaleckas@teltonika.lt>]

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.