[PATCH v9 6/6] davinci: USB1.1 support for Omapl138-Hawkboard

[Nori, Sekhar]

On Fri, Dec 03, 2010 at 06:14:39, Victor Rodriguez wrote:
> On Thu, Dec 2, 2010 at 12:49 AM, Nori, Sekhar <nsekhar at ti.com> wrote:
> > On Thu, Dec 02, 2010 at 01:02:29, vm.rod25 at gmail.com wrote:
> >> +static int hawk_usb_ocic_notify(da8xx_ocic_handler_t handler)
> >> +{
> >> +     int irq         = gpio_to_irq(DA850_USB1_OC_PIN);
> >> +     int error       = 0;
> >> +
> >> +     if (handler != NULL) {
> >> +             hawk_usb_ocic_handler = handler;
> >> +
> >> +             error = request_irq(irq, omapl138_hawk_usb_ocic_irq,
> >> +                                     IRQF_DISABLED | IRQF_TRIGGER_RISING |
> >> +                                     IRQF_TRIGGER_FALLING,
> >> +                                     "OHCI over-current indicator", NULL);
> >> +             if (error)
> >> +                     pr_err(KERN_ERR "%s: could not request IRQ to watch "
> >> +                             "over-current indicator changes\n", __func__);
> >
> > pr_err adds a KERN_ERR already.
>
> Changed to this
>
> static int hawk_usb_ocic_notify(da8xx_ocic_handler_t handler)
> {
>       int irq         = gpio_to_irq(DA850_USB1_OC_PIN);
>       int error       = 0;
>
>       if (handler != NULL) {
>               hawk_usb_ocic_handler = handler;
>
>               error = request_irq(irq, omapl138_hawk_usb_ocic_irq,
>                                       IRQF_DISABLED | IRQF_TRIGGER_RISING |
>                                       IRQF_TRIGGER_FALLING,
>                                       "OHCI over-current indicator", NULL);
>               if (error)
>                       pr_err("%s: could not request IRQ to watch "
>                               "over-current indicator changes\n", __func__);

Looks good. Thanks. The same error is present elsewhere in the
patch and other patches too. Please fix those too.

>
> USB part
>
> Changed to this
>
>       if (ret < 0) {
>               pr_err("%s: failed to request GPIO for USB 1.1 port "
>                       "power control: %d\n", __func__, ret);
>               gpio_free(DA850_USB1_VBUS_PIN);
>               return;
>       }
>
>       ret = gpio_request_one(DA850_USB1_OC_PIN,
>                       GPIOF_DIR_IN, "USB1 OC");
>       if (ret < 0) {
>               pr_err("%s: failed to request GPIO for USB 1.1 port "
>                       "over-current indicator: %d\n", __func__, ret);
>               gpio_free(DA850_USB1_OC_PIN);
>               return;
>       }
>
> Patch 4/6
>
>       ret = gpio_request_one(DA850_HAWK_MMCSD_CD_PIN,
>                       GPIOF_DIR_IN, "MMC CD");
>       if (ret < 0) {
>               pr_warning("%s: can not open GPIO %d\n",
>                       __func__, DA850_HAWK_MMCSD_CD_PIN);
>               gpio_free(DA850_HAWK_MMCSD_CD_PIN);
>               return;
>       }
>
>       ret = gpio_request_one(DA850_HAWK_MMCSD_WP_PIN,
>                       GPIOF_DIR_IN, "MMC WP");
>       if (ret < 0) {
>               pr_warning("%s: can not open GPIO %d\n",
>                       __func__, DA850_HAWK_MMCSD_WP_PIN);
>               gpio_free(DA850_HAWK_MMCSD_WP_PIN);
>               return;
>       }
>
>
> Is this ok ? or should I free the GPIO on the next section ? Same with USB

Not sure what you mean by "next section"? goto based error recovery is
more commonly used and probably more preferred as it is easier to extend.

You can look at function da850_evm_ui_expander_setup() of
arch/arm/mach-davinci/board-da850-evm.c for an example.

Thanks,
Sekhar