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

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

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
>