Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1695465 > unrolled thread

[PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management Baolin Wang <baolin.wang@linaro.org> - 2017-07-25 10:10 +0200
    Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current  limit management Lee Jones <lee.jones@linaro.org> - 2017-07-25 11:20 +0200
    Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current  limit management Charles Keepax <ckeepax@opensource.cirrus.com> - 2017-07-25 11:40 +0200
    Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current  limit management Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-25 12:00 +0200
      Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current  limit management Baolin Wang <baolin.wang@linaro.org> - 2017-07-26 05:10 +0200
        Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current  limit management Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-26 14:10 +0200
          Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current  limit management Baolin Wang <baolin.wang@linaro.org> - 2017-07-27 05:00 +0200

#1695465 — [PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management

Integrate with the newly added USB charger interface to limit the current
we draw from the USB input based on the input device configuration
identified by the USB stack, allowing us to charge more quickly from high
current inputs without drawing more current than specified from others.

Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 Documentation/devicetree/bindings/mfd/wm831x.txt |    1 +
 drivers/power/supply/wm831x_power.c              |   58 ++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/wm831x.txt b/Documentation/devicetree/bindings/mfd/wm831x.txt
index 9f8b743..4e3bc07 100644
--- a/Documentation/devicetree/bindings/mfd/wm831x.txt
+++ b/Documentation/devicetree/bindings/mfd/wm831x.txt
@@ -31,6 +31,7 @@ Required properties:
     ../interrupt-controller/interrupts.txt
 
 Optional sub-nodes:
+  - usb-phy : Contains a phandle to the USB PHY.
   - regulators : Contains sub-nodes for each of the regulators supplied by
     the device. The regulators are bound using their names listed below:
 
diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c
index 7082301..d3948ab 100644
--- a/drivers/power/supply/wm831x_power.c
+++ b/drivers/power/supply/wm831x_power.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
+#include <linux/usb/phy.h>
 
 #include <linux/mfd/wm831x/core.h>
 #include <linux/mfd/wm831x/auxadc.h>
@@ -31,6 +32,8 @@ struct wm831x_power {
 	char usb_name[20];
 	char battery_name[20];
 	bool have_battery;
+	struct usb_phy *usb_phy;
+	struct notifier_block usb_notify;
 };
 
 static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
@@ -125,6 +128,43 @@ static int wm831x_usb_get_prop(struct power_supply *psy,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 };
 
+/* In milliamps */
+static const unsigned int wm831x_usb_limits[] = {
+	0,
+	2,
+	100,
+	500,
+	900,
+	1500,
+	1800,
+	550,
+};
+
+static int wm831x_usb_limit_change(struct notifier_block *nb,
+				   unsigned long limit, void *data)
+{
+	struct wm831x_power *wm831x_power = container_of(nb,
+							 struct wm831x_power,
+							 usb_notify);
+	unsigned int i, best;
+
+	/* Find the highest supported limit */
+	best = 0;
+	for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
+		if (limit >= wm831x_usb_limits[i] &&
+		    wm831x_usb_limits[best] < wm831x_usb_limits[i])
+			best = i;
+	}
+
+	dev_dbg(wm831x_power->wm831x->dev,
+		"Limiting USB current to %umA", wm831x_usb_limits[best]);
+
+	wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
+		        WM831X_USB_ILIM_MASK, best);
+
+	return 0;
+}
+
 /*********************************************************************
  *		Battery properties
  *********************************************************************/
@@ -607,6 +647,19 @@ static int wm831x_power_probe(struct platform_device *pdev)
 		}
 	}
 
+	power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
+						     "usb-phy", 0);
+	if (!IS_ERR(power->usb_phy)) {
+		power->usb_notify.notifier_call = wm831x_usb_limit_change;
+		ret = usb_register_notifier(power->usb_phy,
+					    &power->usb_notify);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to register notifier: %d\n",
+				ret);
+			goto err_bat_irq;
+		}
+	}
+
 	return ret;
 
 err_bat_irq:
@@ -637,6 +690,11 @@ static int wm831x_power_remove(struct platform_device *pdev)
 	struct wm831x *wm831x = wm831x_power->wm831x;
 	int irq, i;
 
+	if (!IS_ERR(wm831x_power->usb_phy)) {
+		usb_unregister_notifier(wm831x_power->usb_phy,
+					&wm831x_power->usb_notify);
+	}
+
 	for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
 		irq = wm831x_irq(wm831x, 
 				 platform_get_irq_byname(pdev,
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1695533 — Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management

On Tue, 25 Jul 2017, Baolin Wang wrote:

> Integrate with the newly added USB charger interface to limit the current
> we draw from the USB input based on the input device configuration
> identified by the USB stack, allowing us to charge more quickly from high
> current inputs without drawing more current than specified from others.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/wm831x.txt |    1 +

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1695545 — Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management

On Tue, Jul 25, 2017 at 04:00:01PM +0800, Baolin Wang wrote:
> Integrate with the newly added USB charger interface to limit the current
> we draw from the USB input based on the input device configuration
> identified by the USB stack, allowing us to charge more quickly from high
> current inputs without drawing more current than specified from others.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Thanks,
Charles

[toc] | [prev] | [next] | [standalone]

#1695556 — Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Tue, Jul 25, 2017 at 04:00:01PM +0800, Baolin Wang wrote:
> Integrate with the newly added USB charger interface to limit the current
> we draw from the USB input based on the input device configuration
> identified by the USB stack, allowing us to charge more quickly from high
> current inputs without drawing more current than specified from others.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/wm831x.txt |    1 +
>  drivers/power/supply/wm831x_power.c              |   58 ++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/wm831x.txt b/Documentation/devicetree/bindings/mfd/wm831x.txt
> index 9f8b743..4e3bc07 100644
> --- a/Documentation/devicetree/bindings/mfd/wm831x.txt
> +++ b/Documentation/devicetree/bindings/mfd/wm831x.txt
> @@ -31,6 +31,7 @@ Required properties:
>      ../interrupt-controller/interrupts.txt
>  
>  Optional sub-nodes:
> +  - usb-phy : Contains a phandle to the USB PHY.
>    - regulators : Contains sub-nodes for each of the regulators supplied by
>      the device. The regulators are bound using their names listed below:
>  
> diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c
> index 7082301..d3948ab 100644
> --- a/drivers/power/supply/wm831x_power.c
> +++ b/drivers/power/supply/wm831x_power.c
> @@ -13,6 +13,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/slab.h>
> +#include <linux/usb/phy.h>
>  
>  #include <linux/mfd/wm831x/core.h>
>  #include <linux/mfd/wm831x/auxadc.h>
> @@ -31,6 +32,8 @@ struct wm831x_power {
>  	char usb_name[20];
>  	char battery_name[20];
>  	bool have_battery;
> +	struct usb_phy *usb_phy;
> +	struct notifier_block usb_notify;
>  };
>  
>  static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
> @@ -125,6 +128,43 @@ static int wm831x_usb_get_prop(struct power_supply *psy,
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  };
>  
> +/* In milliamps */
> +static const unsigned int wm831x_usb_limits[] = {
> +	0,
> +	2,
> +	100,
> +	500,
> +	900,
> +	1500,
> +	1800,
> +	550,
> +};
> +
> +static int wm831x_usb_limit_change(struct notifier_block *nb,
> +				   unsigned long limit, void *data)
> +{
> +	struct wm831x_power *wm831x_power = container_of(nb,
> +							 struct wm831x_power,
> +							 usb_notify);
> +	unsigned int i, best;
> +
> +	/* Find the highest supported limit */
> +	best = 0;
> +	for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
> +		if (limit >= wm831x_usb_limits[i] &&
> +		    wm831x_usb_limits[best] < wm831x_usb_limits[i])
> +			best = i;
> +	}
> +
> +	dev_dbg(wm831x_power->wm831x->dev,
> +		"Limiting USB current to %umA", wm831x_usb_limits[best]);
> +
> +	wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
> +		        WM831X_USB_ILIM_MASK, best);
> +
> +	return 0;
> +}
> +
>  /*********************************************************************
>   *		Battery properties
>   *********************************************************************/
> @@ -607,6 +647,19 @@ static int wm831x_power_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> +						     "usb-phy", 0);
> +	if (!IS_ERR(power->usb_phy)) {
> +		power->usb_notify.notifier_call = wm831x_usb_limit_change;
> +		ret = usb_register_notifier(power->usb_phy,
> +					    &power->usb_notify);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to register notifier: %d\n",
> +				ret);
> +			goto err_bat_irq;
> +		}
> +	}

No error handling for power->usb_phy? I think you should bail out
for all errors except for "not defined in DT". Especially I would
expect probe defer handling in case the power supply driver is
loaded before the phy driver.

>  	return ret;
>  
>  err_bat_irq:
> @@ -637,6 +690,11 @@ static int wm831x_power_remove(struct platform_device *pdev)
>  	struct wm831x *wm831x = wm831x_power->wm831x;
>  	int irq, i;
>  
> +	if (!IS_ERR(wm831x_power->usb_phy)) {
> +		usb_unregister_notifier(wm831x_power->usb_phy,
> +					&wm831x_power->usb_notify);
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
>  		irq = wm831x_irq(wm831x, 
>  				 platform_get_irq_byname(pdev,

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1696749 — Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management

Hi,

On 25 July 2017 at 17:59, Sebastian Reichel
<sebastian.reichel@collabora.co.uk> wrote:
> Hi,
>
> On Tue, Jul 25, 2017 at 04:00:01PM +0800, Baolin Wang wrote:
>> Integrate with the newly added USB charger interface to limit the current
>> we draw from the USB input based on the input device configuration
>> identified by the USB stack, allowing us to charge more quickly from high
>> current inputs without drawing more current than specified from others.
>>
>> Signed-off-by: Mark Brown <broonie@kernel.org>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/mfd/wm831x.txt |    1 +
>>  drivers/power/supply/wm831x_power.c              |   58 ++++++++++++++++++++++
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/wm831x.txt b/Documentation/devicetree/bindings/mfd/wm831x.txt
>> index 9f8b743..4e3bc07 100644
>> --- a/Documentation/devicetree/bindings/mfd/wm831x.txt
>> +++ b/Documentation/devicetree/bindings/mfd/wm831x.txt
>> @@ -31,6 +31,7 @@ Required properties:
>>      ../interrupt-controller/interrupts.txt
>>
>>  Optional sub-nodes:
>> +  - usb-phy : Contains a phandle to the USB PHY.
>>    - regulators : Contains sub-nodes for each of the regulators supplied by
>>      the device. The regulators are bound using their names listed below:
>>
>> diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c
>> index 7082301..d3948ab 100644
>> --- a/drivers/power/supply/wm831x_power.c
>> +++ b/drivers/power/supply/wm831x_power.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/power_supply.h>
>>  #include <linux/slab.h>
>> +#include <linux/usb/phy.h>
>>
>>  #include <linux/mfd/wm831x/core.h>
>>  #include <linux/mfd/wm831x/auxadc.h>
>> @@ -31,6 +32,8 @@ struct wm831x_power {
>>       char usb_name[20];
>>       char battery_name[20];
>>       bool have_battery;
>> +     struct usb_phy *usb_phy;
>> +     struct notifier_block usb_notify;
>>  };
>>
>>  static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
>> @@ -125,6 +128,43 @@ static int wm831x_usb_get_prop(struct power_supply *psy,
>>       POWER_SUPPLY_PROP_VOLTAGE_NOW,
>>  };
>>
>> +/* In milliamps */
>> +static const unsigned int wm831x_usb_limits[] = {
>> +     0,
>> +     2,
>> +     100,
>> +     500,
>> +     900,
>> +     1500,
>> +     1800,
>> +     550,
>> +};
>> +
>> +static int wm831x_usb_limit_change(struct notifier_block *nb,
>> +                                unsigned long limit, void *data)
>> +{
>> +     struct wm831x_power *wm831x_power = container_of(nb,
>> +                                                      struct wm831x_power,
>> +                                                      usb_notify);
>> +     unsigned int i, best;
>> +
>> +     /* Find the highest supported limit */
>> +     best = 0;
>> +     for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
>> +             if (limit >= wm831x_usb_limits[i] &&
>> +                 wm831x_usb_limits[best] < wm831x_usb_limits[i])
>> +                     best = i;
>> +     }
>> +
>> +     dev_dbg(wm831x_power->wm831x->dev,
>> +             "Limiting USB current to %umA", wm831x_usb_limits[best]);
>> +
>> +     wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
>> +                     WM831X_USB_ILIM_MASK, best);
>> +
>> +     return 0;
>> +}
>> +
>>  /*********************************************************************
>>   *           Battery properties
>>   *********************************************************************/
>> @@ -607,6 +647,19 @@ static int wm831x_power_probe(struct platform_device *pdev)
>>               }
>>       }
>>
>> +     power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
>> +                                                  "usb-phy", 0);
>> +     if (!IS_ERR(power->usb_phy)) {
>> +             power->usb_notify.notifier_call = wm831x_usb_limit_change;
>> +             ret = usb_register_notifier(power->usb_phy,
>> +                                         &power->usb_notify);
>> +             if (ret) {
>> +                     dev_err(&pdev->dev, "Failed to register notifier: %d\n",
>> +                             ret);
>> +                     goto err_bat_irq;
>> +             }
>> +     }
>
> No error handling for power->usb_phy? I think you should bail out
> for all errors except for "not defined in DT". Especially I would
> expect probe defer handling in case the power supply driver is
> loaded before the phy driver.

Make sense. So I think I need to change like below:

power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
if (!IS_ERR(power->usb_phy)) {
        power->usb_notify.notifier_call = wm831x_usb_limit_change;
        ret = usb_register_notifier(power->usb_phy, &power->usb_notify);
        if (ret) {
                dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
                goto err_bat_irq;
        }
} else if (PTR_ERR(power->usb_phy) != -ENODEV &&
PTR_ERR(power->usb_phy) != -EINVAL) {
       ret = PTR_ERR(power->usb_phy);
       dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
       goto err_bat_irq;
}

The return value -EINVAL means the platform device does not have a
device node and -ENODEV  means we did not set the 'usb-phy' phandle,
other errors we should bail out including -PROBE_DEFER. Is it OK for
you?

>
>>       return ret;
>>
>>  err_bat_irq:
>> @@ -637,6 +690,11 @@ static int wm831x_power_remove(struct platform_device *pdev)
>>       struct wm831x *wm831x = wm831x_power->wm831x;
>>       int irq, i;
>>
>> +     if (!IS_ERR(wm831x_power->usb_phy)) {
>> +             usb_unregister_notifier(wm831x_power->usb_phy,
>> +                                     &wm831x_power->usb_notify);
>> +     }
>> +
>>       for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
>>               irq = wm831x_irq(wm831x,
>>                                platform_get_irq_byname(pdev,
>
> -- Sebastian



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1697034 — Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Wed, Jul 26, 2017 at 11:05:25AM +0800, Baolin Wang wrote:
> On 25 July 2017 at 17:59, Sebastian Reichel
> <sebastian.reichel@collabora.co.uk> wrote:
> > On Tue, Jul 25, 2017 at 04:00:01PM +0800, Baolin Wang wrote:
> >> Integrate with the newly added USB charger interface to limit the current
> >> we draw from the USB input based on the input device configuration
> >> identified by the USB stack, allowing us to charge more quickly from high
> >> current inputs without drawing more current than specified from others.
> >>
> >> Signed-off-by: Mark Brown <broonie@kernel.org>
> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> >> ---
> >>  Documentation/devicetree/bindings/mfd/wm831x.txt |    1 +
> >>  drivers/power/supply/wm831x_power.c              |   58 ++++++++++++++++++++++
> >>  2 files changed, 59 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/wm831x.txt b/Documentation/devicetree/bindings/mfd/wm831x.txt
> >> index 9f8b743..4e3bc07 100644
> >> --- a/Documentation/devicetree/bindings/mfd/wm831x.txt
> >> +++ b/Documentation/devicetree/bindings/mfd/wm831x.txt
> >> @@ -31,6 +31,7 @@ Required properties:
> >>      ../interrupt-controller/interrupts.txt
> >>
> >>  Optional sub-nodes:
> >> +  - usb-phy : Contains a phandle to the USB PHY.
> >>    - regulators : Contains sub-nodes for each of the regulators supplied by
> >>      the device. The regulators are bound using their names listed below:
> >>
> >> diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c
> >> index 7082301..d3948ab 100644
> >> --- a/drivers/power/supply/wm831x_power.c
> >> +++ b/drivers/power/supply/wm831x_power.c
> >> @@ -13,6 +13,7 @@
> >>  #include <linux/platform_device.h>
> >>  #include <linux/power_supply.h>
> >>  #include <linux/slab.h>
> >> +#include <linux/usb/phy.h>
> >>
> >>  #include <linux/mfd/wm831x/core.h>
> >>  #include <linux/mfd/wm831x/auxadc.h>
> >> @@ -31,6 +32,8 @@ struct wm831x_power {
> >>       char usb_name[20];
> >>       char battery_name[20];
> >>       bool have_battery;
> >> +     struct usb_phy *usb_phy;
> >> +     struct notifier_block usb_notify;
> >>  };
> >>
> >>  static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
> >> @@ -125,6 +128,43 @@ static int wm831x_usb_get_prop(struct power_supply *psy,
> >>       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> >>  };
> >>
> >> +/* In milliamps */
> >> +static const unsigned int wm831x_usb_limits[] = {
> >> +     0,
> >> +     2,
> >> +     100,
> >> +     500,
> >> +     900,
> >> +     1500,
> >> +     1800,
> >> +     550,
> >> +};
> >> +
> >> +static int wm831x_usb_limit_change(struct notifier_block *nb,
> >> +                                unsigned long limit, void *data)
> >> +{
> >> +     struct wm831x_power *wm831x_power = container_of(nb,
> >> +                                                      struct wm831x_power,
> >> +                                                      usb_notify);
> >> +     unsigned int i, best;
> >> +
> >> +     /* Find the highest supported limit */
> >> +     best = 0;
> >> +     for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
> >> +             if (limit >= wm831x_usb_limits[i] &&
> >> +                 wm831x_usb_limits[best] < wm831x_usb_limits[i])
> >> +                     best = i;
> >> +     }
> >> +
> >> +     dev_dbg(wm831x_power->wm831x->dev,
> >> +             "Limiting USB current to %umA", wm831x_usb_limits[best]);
> >> +
> >> +     wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
> >> +                     WM831X_USB_ILIM_MASK, best);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >>  /*********************************************************************
> >>   *           Battery properties
> >>   *********************************************************************/
> >> @@ -607,6 +647,19 @@ static int wm831x_power_probe(struct platform_device *pdev)
> >>               }
> >>       }
> >>
> >> +     power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
> >> +                                                  "usb-phy", 0);
> >> +     if (!IS_ERR(power->usb_phy)) {
> >> +             power->usb_notify.notifier_call = wm831x_usb_limit_change;
> >> +             ret = usb_register_notifier(power->usb_phy,
> >> +                                         &power->usb_notify);
> >> +             if (ret) {
> >> +                     dev_err(&pdev->dev, "Failed to register notifier: %d\n",
> >> +                             ret);
> >> +                     goto err_bat_irq;
> >> +             }
> >> +     }
> >
> > No error handling for power->usb_phy? I think you should bail out
> > for all errors except for "not defined in DT". Especially I would
> > expect probe defer handling in case the power supply driver is
> > loaded before the phy driver.
> 
> Make sense. So I think I need to change like below:
> 
> power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> if (!IS_ERR(power->usb_phy)) {
>         power->usb_notify.notifier_call = wm831x_usb_limit_change;
>         ret = usb_register_notifier(power->usb_phy, &power->usb_notify);
>         if (ret) {
>                 dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
>                 goto err_bat_irq;
>         }
> } else if (PTR_ERR(power->usb_phy) != -ENODEV &&
> PTR_ERR(power->usb_phy) != -EINVAL) {
>        ret = PTR_ERR(power->usb_phy);
>        dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
>        goto err_bat_irq;
> }
> 
> The return value -EINVAL means the platform device does not have a
> device node and -ENODEV  means we did not set the 'usb-phy' phandle,
> other errors we should bail out including -PROBE_DEFER. Is it OK for
> you?

Yes, but I think the following is better, which avoids
spamming the log with probe defer messages. 

power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
ret = PTR_ERR_OR_ZERO(power->usb_phy);

switch (ret) {
case 0:
    power->usb_notify.notifier_call = wm831x_usb_limit_change;
    ret = usb_register_notifier(power->usb_phy, &power->usb_notify);
    if (ret) {
        dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
        goto err_bat_irq;
    }
    break;
case -EINVAL:
case -ENODEV:
    /* ignore missing usb-phy, it's optional */
    power->usb_phy = NULL;
    break;
default:
    dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
    /* fall-through */
case -EPROBE_DEFER:
    goto err_bat_irq;
    break;
}

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1697683 — Re: [PATCH v3 3/3] power: wm831x_power: Support USB charger current limit management

Hi,

On 26 July 2017 at 20:08, Sebastian Reichel
<sebastian.reichel@collabora.co.uk> wrote:
> Hi,
>
> On Wed, Jul 26, 2017 at 11:05:25AM +0800, Baolin Wang wrote:
>> On 25 July 2017 at 17:59, Sebastian Reichel
>> <sebastian.reichel@collabora.co.uk> wrote:
>> > On Tue, Jul 25, 2017 at 04:00:01PM +0800, Baolin Wang wrote:
>> >> Integrate with the newly added USB charger interface to limit the current
>> >> we draw from the USB input based on the input device configuration
>> >> identified by the USB stack, allowing us to charge more quickly from high
>> >> current inputs without drawing more current than specified from others.
>> >>
>> >> Signed-off-by: Mark Brown <broonie@kernel.org>
>> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> >> ---
>> >>  Documentation/devicetree/bindings/mfd/wm831x.txt |    1 +
>> >>  drivers/power/supply/wm831x_power.c              |   58 ++++++++++++++++++++++
>> >>  2 files changed, 59 insertions(+)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/mfd/wm831x.txt b/Documentation/devicetree/bindings/mfd/wm831x.txt
>> >> index 9f8b743..4e3bc07 100644
>> >> --- a/Documentation/devicetree/bindings/mfd/wm831x.txt
>> >> +++ b/Documentation/devicetree/bindings/mfd/wm831x.txt
>> >> @@ -31,6 +31,7 @@ Required properties:
>> >>      ../interrupt-controller/interrupts.txt
>> >>
>> >>  Optional sub-nodes:
>> >> +  - usb-phy : Contains a phandle to the USB PHY.
>> >>    - regulators : Contains sub-nodes for each of the regulators supplied by
>> >>      the device. The regulators are bound using their names listed below:
>> >>
>> >> diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c
>> >> index 7082301..d3948ab 100644
>> >> --- a/drivers/power/supply/wm831x_power.c
>> >> +++ b/drivers/power/supply/wm831x_power.c
>> >> @@ -13,6 +13,7 @@
>> >>  #include <linux/platform_device.h>
>> >>  #include <linux/power_supply.h>
>> >>  #include <linux/slab.h>
>> >> +#include <linux/usb/phy.h>
>> >>
>> >>  #include <linux/mfd/wm831x/core.h>
>> >>  #include <linux/mfd/wm831x/auxadc.h>
>> >> @@ -31,6 +32,8 @@ struct wm831x_power {
>> >>       char usb_name[20];
>> >>       char battery_name[20];
>> >>       bool have_battery;
>> >> +     struct usb_phy *usb_phy;
>> >> +     struct notifier_block usb_notify;
>> >>  };
>> >>
>> >>  static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
>> >> @@ -125,6 +128,43 @@ static int wm831x_usb_get_prop(struct power_supply *psy,
>> >>       POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> >>  };
>> >>
>> >> +/* In milliamps */
>> >> +static const unsigned int wm831x_usb_limits[] = {
>> >> +     0,
>> >> +     2,
>> >> +     100,
>> >> +     500,
>> >> +     900,
>> >> +     1500,
>> >> +     1800,
>> >> +     550,
>> >> +};
>> >> +
>> >> +static int wm831x_usb_limit_change(struct notifier_block *nb,
>> >> +                                unsigned long limit, void *data)
>> >> +{
>> >> +     struct wm831x_power *wm831x_power = container_of(nb,
>> >> +                                                      struct wm831x_power,
>> >> +                                                      usb_notify);
>> >> +     unsigned int i, best;
>> >> +
>> >> +     /* Find the highest supported limit */
>> >> +     best = 0;
>> >> +     for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
>> >> +             if (limit >= wm831x_usb_limits[i] &&
>> >> +                 wm831x_usb_limits[best] < wm831x_usb_limits[i])
>> >> +                     best = i;
>> >> +     }
>> >> +
>> >> +     dev_dbg(wm831x_power->wm831x->dev,
>> >> +             "Limiting USB current to %umA", wm831x_usb_limits[best]);
>> >> +
>> >> +     wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
>> >> +                     WM831X_USB_ILIM_MASK, best);
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >>  /*********************************************************************
>> >>   *           Battery properties
>> >>   *********************************************************************/
>> >> @@ -607,6 +647,19 @@ static int wm831x_power_probe(struct platform_device *pdev)
>> >>               }
>> >>       }
>> >>
>> >> +     power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev,
>> >> +                                                  "usb-phy", 0);
>> >> +     if (!IS_ERR(power->usb_phy)) {
>> >> +             power->usb_notify.notifier_call = wm831x_usb_limit_change;
>> >> +             ret = usb_register_notifier(power->usb_phy,
>> >> +                                         &power->usb_notify);
>> >> +             if (ret) {
>> >> +                     dev_err(&pdev->dev, "Failed to register notifier: %d\n",
>> >> +                             ret);
>> >> +                     goto err_bat_irq;
>> >> +             }
>> >> +     }
>> >
>> > No error handling for power->usb_phy? I think you should bail out
>> > for all errors except for "not defined in DT". Especially I would
>> > expect probe defer handling in case the power supply driver is
>> > loaded before the phy driver.
>>
>> Make sense. So I think I need to change like below:
>>
>> power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
>> if (!IS_ERR(power->usb_phy)) {
>>         power->usb_notify.notifier_call = wm831x_usb_limit_change;
>>         ret = usb_register_notifier(power->usb_phy, &power->usb_notify);
>>         if (ret) {
>>                 dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
>>                 goto err_bat_irq;
>>         }
>> } else if (PTR_ERR(power->usb_phy) != -ENODEV &&
>> PTR_ERR(power->usb_phy) != -EINVAL) {
>>        ret = PTR_ERR(power->usb_phy);
>>        dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
>>        goto err_bat_irq;
>> }
>>
>> The return value -EINVAL means the platform device does not have a
>> device node and -ENODEV  means we did not set the 'usb-phy' phandle,
>> other errors we should bail out including -PROBE_DEFER. Is it OK for
>> you?
>
> Yes, but I think the following is better, which avoids
> spamming the log with probe defer messages.
>
> power->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> ret = PTR_ERR_OR_ZERO(power->usb_phy);
>
> switch (ret) {
> case 0:
>     power->usb_notify.notifier_call = wm831x_usb_limit_change;
>     ret = usb_register_notifier(power->usb_phy, &power->usb_notify);
>     if (ret) {
>         dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret);
>         goto err_bat_irq;
>     }
>     break;
> case -EINVAL:
> case -ENODEV:
>     /* ignore missing usb-phy, it's optional */
>     power->usb_phy = NULL;
>     break;
> default:
>     dev_err(&pdev->dev, "Failed to find USB phy: %d\n", ret);
>     /* fall-through */
> case -EPROBE_DEFER:
>     goto err_bat_irq;
>     break;
> }

OK. Thanks.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web