> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of G,
> Manjunath Kondaiah
> Sent: Thursday, May 06, 2010 8:45 AM
> To: Arce, Abraham; Tony Lindgren
> Cc: [email protected];
> [email protected]
> Subject: RE: [PATCH v1 2/3] OMAP4: Ethernet: KS8851 Board Support
>
>
>
> > -----Original Message-----
> > From: Arce, Abraham
> > Sent: Thursday, May 06, 2010 6:18 AM
> > To: Tony Lindgren
> > Cc: G, Manjunath Kondaiah; [email protected];
> > [email protected]
> > Subject: RE: [PATCH v1 2/3] OMAP4: Ethernet: KS8851 Board Support
> >
> > > > > > +
> > > > > > +static void omap_ethernet_init(void)
> > > > > > +{
> > > > > > + gpio_request(ETHERNET_KS8851_POWER_ENABLE, "ethernet");
> > > > > > + gpio_direction_output(ETHERNET_KS8851_POWER_ENABLE, 1);
> > > > > > + gpio_request(ETHERNET_KS8851_QUART, "quart");
> > > > > > + gpio_direction_output(ETHERNET_KS8851_QUART, 1);
> > > > > > + gpio_request(ETHERNET_KS8851_IRQ, "ks8851");
> > > > >
> > > > > Any reason for not checking return value of gpio_request()?
> > > > >
> > > >
> > > > Thought initially about them as dedicated lines for
> > ethernet avoiding
> > > > both reasons to check for a gpio's request:
> > > >
> > > > 1. invalid gpio
> > > > 2. already claimed gpio
> > >
> > > You still need to check for the result.
> > >
> >
> > Is the below approach ok? Using goto would make it better?
> >
> > int status;
> >
> > status = gpio_request(ETHERNET_KS8851_POWER_ENABLE,
> > "ethernet");
> > if (status < 0)
>
> You need to have warning message about this failure.
>
> > return status;
> >
> > gpio_request(ETHERNET_KS8851_QUART, "quart");
> > if (status < 0) {
>
> -Ditto-
>
> > gpio_free(ETHERNET_KS8851_POWER_ENABLE);
> > return status;
>
> Return to where? This function seems to be void, change signature
> of this API.
>
> > }
> >
> > gpio_request(ETHERNET_KS8851_IRQ, "ks8851");
> > if (status < 0) {
>
> Bug! Checking previous status.
>
> > gpio_free(ETHERNET_KS8851_POWER_ENABLE);
> > gpio_free(ETHERNET_KS8851_QUART);
> > return status;
> > }
> >
> > gpio_direction_output(ETHERNET_KS8851_POWER_ENABLE, 1);
> > gpio_direction_output(ETHERNET_KS8851_QUART, 1);
> > gpio_direction_input(ETHERNET_KS8851_IRQ);
>
> Goto will be better approach compared to above logic.
>
> if(gpio_request(x)
> goto err1;
>
> if(gpio_request(y)
> goto err2;
>
> if(gpio_request(z)
> goto err3;
> ...
> return 0;
>
> err3:
> free(x);
> err2:
> free(y);
> err1:
> free(z);
>
> return status;
Minor changes:
if (gpio_request(x))
return status;
if (gpio_request(y))
goto err1;
if (gpio_request(z))
goto err2;
...
return 0;
err2:
free(y);
err1:
free(x);
return status;
-Manjunath
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
