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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4906D389.5050706@teltonika.lt>
Date:	Tue, 28 Oct 2008 10:55:37 +0200
From:	Paulius Zaleckas <paulius.zaleckas@...tonika.lt>
To:	David Brownell <david-b@...bell.net>
CC:	Grant Likely <grant.likely@...retlab.ca>, netdev@...r.kernel.org,
	linux-arm-kernel@...ts.arm.linux.org.uk,
	linux-embedded@...r.kernel.org
Subject: Re: [PATCH] phylib: add mdio-gpio bus driver (v2)

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html