Re: [linux-usb-devel] [PATCH 11/12] drivers: PMC MSP71xx USB driver

[Alan Stern <stern@rowland.harvard.edu>]

On Tue, 5 Jun 2007, Marc St-Jean wrote:

> @@ -2749,12 +2749,33 @@ static void hub_events(void)
>                       }
>                       
>                       if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
> -                             dev_err (hub_dev,
> -                                     "over-current change on port %d\n",
> -                                     i);
> +                             /* clear OCC bit */
>                               clear_port_feature(hdev, i,
>                                       USB_PORT_FEAT_C_OVER_CURRENT);
> +
> +                             /*
> +                              * This step is required to toggle the
> +                              * PP bit to 0 and 1 (by hub_power_on)
> +                              * in order the CSC bit to be transitioned
> +                              * properly for device hotplug.
> +                              */
> +                             /* clear PP bit */
> +                             clear_port_feature(hdev, i,
> +                                             USB_PORT_FEAT_POWER);
> +
> +                             /* resume power */
>                               hub_power_on(hub);
> +
> +                             udelay(100);
> +
> +                             /* read OCA bit */
> +                             if (portstatus &
> +                                 (1 << USB_PORT_FEAT_OVER_CURRENT)) {
> +                                     /* declare overcurrent */
> +                                     dev_err(hub_dev,
> +                                             "over-current change "
> +                                             "on port %d\n", i);
> +                             }
>                       }

Quite apart from all the issues David mentioned, you shouldn't change 
the way errors are reported.  The dev_err() statement should always be 
executed when there is an overcurrent change; it shouldn't depend on 
whether the overcurrent feature is set at the moment.

Remember, the message reports an overcurrent _change_, not an 
overcurrent _state_.

Alan Stern