Re: [PATCH 20/57] power: Adds support for legacy USB chargers

[Anton Vorontsov]

On Tue, Sep 25, 2012 at 10:12:17AM -0600, mathieu.poirier@xxxxxxxxxx wrote:
> From: Marcus Cooper <marcus.xm.cooper@xxxxxxxxxxxxxx>
> 
> A Legacy USB charger should be handled directly by the charger
> driver.
> 
> Signed-off-by: Marcus Cooper <marcus.xm.cooper@xxxxxxxxxxxxxx>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> Reviewed-by: Karl KOMIEROWSKI <karl.komierowski@xxxxxxxxxxxxxx>
> Reviewed-by: Jonas ABERG <jonas.aberg@xxxxxxxxxxxxxx>
> ---
>  drivers/power/ab8500_charger.c |   66 ++++++++++++++++++++++++++++++++++-----
>  1 files changed, 57 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c
> index 4155feb..7cd4165 100644
> --- a/drivers/power/ab8500_charger.c
> +++ b/drivers/power/ab8500_charger.c
> @@ -85,6 +85,9 @@
>  /* Step up/down delay in us */
>  #define STEP_UDELAY			1000
>  
> +/* Wait for enumeration before charging in ms */
> +#define WAIT_FOR_USB_ENUMERATION	(5 * 1000)
> +
>  #define CHARGER_STATUS_POLL 10 /* in ms */
>  
>  /* UsbLineStatus register - usb types */
> @@ -198,6 +201,7 @@ struct ab8500_charger_usb_state {
>   *			charger is enabled
>   * @vbat		Battery voltage
>   * @old_vbat		Previously measured battery voltage
> + * @usb_device_is_unrecognised	USB device is unrecognised by the hardware
>   * @autopower		Indicate if we should have automatic pwron after pwrloss
>   * @parent:		Pointer to the struct ab8500
>   * @gpadc:		Pointer to the struct gpadc
> @@ -216,6 +220,7 @@ struct ab8500_charger_usb_state {
>   * @check_usbchgnotok_work:	Work for checking USB charger not ok status
>   * @kick_wd_work:		Work for kicking the charger watchdog in case
>   *				of ABB rev 1.* due to the watchog logic bug
> + * @attach_work:		Work for checking the usb enumeration
>   * @ac_charger_attached_work:	Work for checking if AC charger is still
>   *				connected
>   * @usb_charger_attached_work:	Work for checking if USB charger is still
> @@ -239,6 +244,7 @@ struct ab8500_charger {
>  	bool vddadc_en_usb;
>  	int vbat;
>  	int old_vbat;
> +	bool usb_device_is_unrecognised;
>  	bool autopower;
>  	struct ab8500 *parent;
>  	struct ab8500_gpadc *gpadc;
> @@ -256,6 +262,7 @@ struct ab8500_charger {
>  	struct delayed_work check_hw_failure_work;
>  	struct delayed_work check_usbchgnotok_work;
>  	struct delayed_work kick_wd_work;
> +	struct delayed_work attach_work;
>  	struct delayed_work ac_charger_attached_work;
>  	struct delayed_work usb_charger_attached_work;

Quite a lot of these. I doubt that the driver really needs all this stuff
executed in parallel. I guess something is wrong with the driver design.

Maybe there should be just one delayed work: you can probably queue it
early from the irq handler, and do all the work there...

>  	struct work_struct ac_work;
> @@ -588,6 +595,8 @@ static int ab8500_charger_max_usb_curr(struct ab8500_charger *di,
>  {
>  	int ret = 0;
>  
> +	di->usb_device_is_unrecognised = false;
> +
>  	switch (link_status) {
>  	case USB_STAT_STD_HOST_NC:
>  	case USB_STAT_STD_HOST_C_NS:
> @@ -633,9 +642,15 @@ static int ab8500_charger_max_usb_curr(struct ab8500_charger *di,
>  		dev_dbg(di->dev, "USB Type - 0x%02x MaxCurr: %d", link_status,
>  				di->max_usb_in_curr);
>  		break;
> +	case USB_STAT_NOT_CONFIGURED:
> +		if (di->vbus_detected) {
> +			di->usb_device_is_unrecognised = true;
> +			dev_dbg(di->dev, "USB Type - Legacy charger.\n");
> +			di->max_usb_in_curr = USB_CH_IP_CUR_LVL_1P5;
> +			break;
> +		}
>  	case USB_STAT_HM_IDGND:
>  	case USB_STAT_NOT_VALID_LINK:
> -	case USB_STAT_NOT_CONFIGURED:
>  		dev_err(di->dev, "USB Type - Charging not allowed\n");
>  		di->max_usb_in_curr = USB_CH_IP_CUR_LVL_0P05;
>  		ret = -ENXIO;
> @@ -1899,6 +1914,30 @@ static void ab8500_charger_detect_usb_type_work(struct work_struct *work)
>  }
>  
>  /**
> + * ab8500_charger_usb_link_attach_work() - delayd work to detect USB type
> + * @work:	pointer to the work_struct structure
> + *
> + * Detect the type of USB plugged
> + */
> +static void ab8500_charger_usb_link_attach_work(struct work_struct *work)
> +{
> +	int ret;
> +

This empty line is not needed.

> +	struct ab8500_charger *di = container_of(work,
> +		struct ab8500_charger, attach_work.work);
> +
> +	/* Update maximum input current if USB enumeration is not detected */
> +	if (!di->usb.charger_online) {
> +		ret = ab8500_charger_set_vbus_in_curr(di, di->max_usb_in_curr);
> +		if (ret)
> +			return;
> +	}
> +
> +	ab8500_charger_set_usb_connected(di, true);
> +	ab8500_power_supply_changed(di, &di->usb_chg.psy);
> +}
> +
> +/**
>   * ab8500_charger_usb_link_status_work() - work to detect USB type
>   * @work:	pointer to the work_struct structure
>   *
> @@ -1928,14 +1967,20 @@ static void ab8500_charger_usb_link_status_work(struct work_struct *work)
>  		di->vbus_detected = 1;
>  		ret = ab8500_charger_read_usb_type(di);
>  		if (!ret) {
> -			/* Update maximum input current */
> -			ret = ab8500_charger_set_vbus_in_curr(di,
> -					di->max_usb_in_curr);
> -			if (ret)
> -				return;
> -
> -			ab8500_charger_set_usb_connected(di, true);
> -			ab8500_power_supply_changed(di, &di->usb_chg.psy);
> +			if (di->usb_device_is_unrecognised) {
> +				dev_dbg(di->dev,
> +					"Potential Legacy Charger device. "
> +					"Delay work for %d msec for USB enum "
> +					"to finish",
> +					WAIT_FOR_USB_ENUMERATION);
> +				queue_delayed_work(di->charger_wq,
> +					&di->attach_work,
> +					msecs_to_jiffies
> +						(WAIT_FOR_USB_ENUMERATION));

Very weird way to wrap lines. I'd prefer "(" to stay on the previous line.

I also think that it's possible to refactor this function to make it less
indented.

> +			} else {
> +				queue_delayed_work(di->charger_wq,
> +							&di->attach_work, 0);
> +			}
>  		} else if (ret == -ENXIO) {
>  			/* No valid charger type detected */
>  			ab8500_charger_set_usb_connected(di, false);
> @@ -2886,6 +2931,9 @@ static int __devinit ab8500_charger_probe(struct platform_device *pdev)
>  	INIT_DELAYED_WORK_DEFERRABLE(&di->kick_wd_work,
>  		ab8500_charger_kick_watchdog_work);
>  
> +	INIT_DELAYED_WORK(&di->attach_work,
> +		ab8500_charger_usb_link_attach_work);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/