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]

Date:	Tue, 28 Oct 2008 10:30:39 +0200
From:	Paulius Zaleckas <paulius.zaleckas@...tonika.lt>
To:	Mike Frysinger <vapier.adi@...il.com>
CC:	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)

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