Introduce usb charger framework to deal with the usb gadget power negotation

-----Original Message-----
From: [email protected] [mailto:linux-usb- [email protected]] On Behalf Of Baolin Wang
Sent: Thursday, April 07, 2016 7:49 PM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; linux- [email protected]
Subject: [PATCH v10 1/4] gadget: Introduce the usb charger framework

This patch introduces the usb charger driver based on usb gadget that makes an enhancement to a power driver. It works well in practice but that requires a system with suitable hardware.

The basic conception of the usb charger is that, when one usb charger is added or removed by reporting from the usb gadget state change or the extcon device state change, the usb charger will report to power user to set the current limitation.

The usb charger will register notifiees on the usb gadget or the extcon device to get notified the usb charger state. It also supplies the notification mechanism to userspace When the usb charger state is changed.

Power user will register a notifiee on the usb charger to get notified by status changes from the usb charger. It will report to power user to set the current limitation when detecting the usb charger is added or removed from extcon device state or usb gadget state.

This patch doesn't yet integrate with the gadget code, so some functions which rely on the 'gadget' are not completed, that will be implemented in the following patches.

Signed-off-by Baolin Wang <[email protected]>

---
 drivers/usb/gadget/Kconfig       |    7 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  729
++++++++++++++++++++++++++++++++++++++
 include/linux/usb/charger.h      |  173 +++++++++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 941 insertions(+)
 create mode 100644 drivers/usb/gadget/udc/charger.c  create mode 100644

include/linux/usb/charger.h create mode 100644
include/uapi/linux/usb/charger.h

drivers/usb/gadget/udc/charger.c new file +18 -0

+
+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type manually
+on your
+ * platform.
+ * You should call it at the right gadget state to avoid affecting
+gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+	enum usb_charger_type type;
+
+	if (WARN(!uchger->charger_detect,
+		 "charger detection method should not be NULL"))
+		return -EINVAL;

drivers/usb/gadget/udc/charger.c new file +50 -0

+
+/*
+ * usb_charger_get_type_by_others() - Get the usb charger type by the
+callback
+ * which is implemented by users.
+ * @uchger - the usb charger device.
+ *
+ * Note: This function is just used for getting the charger type, not
+for
+ * detecting charger type which might affect the DP/DM line when gadget
+is on
+ * enumeration state.
+ */
+static enum usb_charger_type
+usb_charger_get_type_by_others(struct usb_charger *uchger) {
+	if (uchger->type != UNKNOWN_TYPE)
+		return uchger->type;
+
+	if (uchger->psy) {
+		union power_supply_propval val;
+
+		power_supply_get_property(uchger->psy,
+					  POWER_SUPPLY_PROP_CHARGE_TYPE,
+					  &val);
+		switch (val.intval) {
+		case POWER_SUPPLY_TYPE_USB:
+			uchger->type = SDP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_DCP:
+			uchger->type = DCP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_CDP:
+			uchger->type = CDP_TYPE;
+			break;
+		case POWER_SUPPLY_TYPE_USB_ACA:
+			uchger->type = ACA_TYPE;
+			break;
+		default:
+			uchger->type = UNKNOWN_TYPE;
+			break;
+		}
+	} else if (uchger->get_charger_type) {
+		uchger->type = uchger->get_charger_type(uchger);
+	} else {
+		uchger->type = UNKNOWN_TYPE;
+	}
+
+	return uchger->type;
+}
+

Hi, Baolin

Quoted 24 lines

drivers/usb/gadget/udc/charger.c new file +24 -0

+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type manually
+on your
+ * platform.
+ * You should call it at the right gadget state to avoid affecting
+gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+     enum usb_charger_type type;
+
+     if (WARN(!uchger->charger_detect,
+              "charger detection method should not be NULL"))
+             return -EINVAL;
+
+     type = uchger->charger_detect(uchger);
+
+     mutex_lock(&uchger->lock);
+     uchger->type = type;
+     mutex_unlock(&uchger->lock);
+

drivers/usb/gadget/udc/charger.c line 217

Quoted 3 lines

drivers/usb/gadget/udc/charger.c new file +3 -0

+     return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);

I still recommend have a central place to call usb_charger_detect_type() to cover real charger detection instead of leaving this question open to diff users. This can be done after vbus-on is detected and before do gadget connect. I don't think this will make your framework complicated.

Hi Felipe, what do you think of this?

Li Jun

Quoted 50 lines

drivers/usb/gadget/udc/charger.c new file +50 -0

+
+/*
+ * usb_charger_get_type_by_others() - Get the usb charger type by the
+callback
+ * which is implemented by users.
+ * @uchger - the usb charger device.
+ *
+ * Note: This function is just used for getting the charger type, not
+for
+ * detecting charger type which might affect the DP/DM line when gadget
+is on
+ * enumeration state.
+ */
+static enum usb_charger_type
+usb_charger_get_type_by_others(struct usb_charger *uchger) {
+     if (uchger->type != UNKNOWN_TYPE)
+             return uchger->type;
+
+     if (uchger->psy) {
+             union power_supply_propval val;
+
+             power_supply_get_property(uchger->psy,
+                                       POWER_SUPPLY_PROP_CHARGE_TYPE,
+                                       &val);
+             switch (val.intval) {
+             case POWER_SUPPLY_TYPE_USB:
+                     uchger->type = SDP_TYPE;
+                     break;
+             case POWER_SUPPLY_TYPE_USB_DCP:
+                     uchger->type = DCP_TYPE;
+                     break;
+             case POWER_SUPPLY_TYPE_USB_CDP:
+                     uchger->type = CDP_TYPE;
+                     break;
+             case POWER_SUPPLY_TYPE_USB_ACA:
+                     uchger->type = ACA_TYPE;
+                     break;
+             default:
+                     uchger->type = UNKNOWN_TYPE;
+                     break;
+             }
+     } else if (uchger->get_charger_type) {
+             uchger->type = uchger->get_charger_type(uchger);
+     } else {
+             uchger->type = UNKNOWN_TYPE;
+     }
+
+     return uchger->type;
+}
+

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, April 08, 2016 5:56 PM
To: Jun Li <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; device- [email protected]; [email protected]
Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Hi Jun,

Quoted 26 lines · Jun Li

Hi, Baolin

Quoted 24 lines

drivers/usb/gadget/udc/charger.c new file +24 -0

+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type
+manually on your
+ * platform.
+ * You should call it at the right gadget state to avoid affecting
+gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+     enum usb_charger_type type;
+
+     if (WARN(!uchger->charger_detect,
+              "charger detection method should not be NULL"))
+             return -EINVAL;
+
+     type = uchger->charger_detect(uchger);
+
+     mutex_lock(&uchger->lock);
+     uchger->type = type;
+     mutex_unlock(&uchger->lock);
+

Quoted 8 lines

drivers/usb/gadget/udc/charger.c line 217

Quoted 3 lines

drivers/usb/gadget/udc/charger.c new file +3 -0

+     return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);

I still recommend have a central place to call usb_charger_detect_type() to cover real charger detection instead of leaving this question open to diff users. This can be done after vbus-on is detected and before do gadget connect. I don't think this

will make your framework complicated.

This API is used for detecting the charger type manually (software charger detection), so if user's udc driver is needed to do this, then they can issue it at their right place to make it more flexible. But let us see other people's suggestion. Thanks.

Quoted 48 lines

Hi Felipe, what do you think of this?

Li Jun

Quoted 42 lines

drivers/usb/gadget/udc/charger.c new file +42 -0

+
+/*
+ * usb_charger_get_type_by_others() - Get the usb charger type by
+the callback
+ * which is implemented by users.
+ * @uchger - the usb charger device.
+ *
+ * Note: This function is just used for getting the charger type,
+not for
+ * detecting charger type which might affect the DP/DM line when
+gadget is on
+ * enumeration state.
+ */
+static enum usb_charger_type
+usb_charger_get_type_by_others(struct usb_charger *uchger) {
+     if (uchger->type != UNKNOWN_TYPE)
+             return uchger->type;
+
+     if (uchger->psy) {
+             union power_supply_propval val;
+
+             power_supply_get_property(uchger->psy,
+                                       POWER_SUPPLY_PROP_CHARGE_TYPE,
+                                       &val);
+             switch (val.intval) {
+             case POWER_SUPPLY_TYPE_USB:
+                     uchger->type = SDP_TYPE;
+                     break;
+             case POWER_SUPPLY_TYPE_USB_DCP:
+                     uchger->type = DCP_TYPE;
+                     break;
+             case POWER_SUPPLY_TYPE_USB_CDP:
+                     uchger->type = CDP_TYPE;
+                     break;
+             case POWER_SUPPLY_TYPE_USB_ACA:
+                     uchger->type = ACA_TYPE;
+                     break;
+             default:
+                     uchger->type = UNKNOWN_TYPE;
+                     break;
+             }
+     } else if (uchger->get_charger_type) {

Quoted 8 lines

drivers/usb/gadget/udc/charger.c line 265

Quoted 8 lines

drivers/usb/gadget/udc/charger.c new file +8 -0

+             uchger->type = uchger->get_charger_type(uchger);
+     } else {
+             uchger->type = UNKNOWN_TYPE;
+     }
+
+     return uchger->type;
+}
+

--
Baolin.wang
Best Regards

Quoted 34 lines

Quoted 24 lines

Quoted 24 lines

drivers/usb/gadget/udc/charger.c new file +24 -0

+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type
+manually on your
+ * platform.
+ * You should call it at the right gadget state to avoid affecting
+gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+     enum usb_charger_type type;
+
+     if (WARN(!uchger->charger_detect,
+              "charger detection method should not be NULL"))
+             return -EINVAL;
+
+     type = uchger->charger_detect(uchger);
+
+     mutex_lock(&uchger->lock);
+     uchger->type = type;
+     mutex_unlock(&uchger->lock);
+

Quoted 8 lines

drivers/usb/gadget/udc/charger.c line 217

Quoted 3 lines

drivers/usb/gadget/udc/charger.c new file +3 -0

+     return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);

I still recommend have a central place to call usb_charger_detect_type() to cover real charger detection instead of leaving this question open to diff users. This can be done after vbus-on is detected and before do gadget connect. I don't think this

will make your framework complicated.

Quoted 4 lines

This API is used for detecting the charger type manually (software charger detection), so if user's udc driver is needed to do this, then they can issue it at their right place to make it more flexible. But let us see other people's suggestion. Thanks.

Ok, actually this API can be used for what ever charger detection type, user can implement any method(manual detection, directly read result from some HW...what ever) in uchger->charger_detect(), target is

to have the charger type and set uchger->type, then you no need to add the comments/description to limit it usage. Also I do see there is possible central place to do this.

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, April 08, 2016 7:00 PM
To: Jun Li <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; device- [email protected]; [email protected]
Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Hi Jun,

Quoted 40 lines

Quoted 39 lines

Quoted 25 lines

Quoted 25 lines

drivers/usb/gadget/udc/charger.c new file +25 -0

+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type
+manually on your
+ * platform.
+ * You should call it at the right gadget state to avoid
+affecting gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+     enum usb_charger_type type;
+
+     if (WARN(!uchger->charger_detect,
+              "charger detection method should not be NULL"))
+             return -EINVAL;
+
+     type = uchger->charger_detect(uchger);
+
+     mutex_lock(&uchger->lock);
+     uchger->type = type;
+     mutex_unlock(&uchger->lock);
+
+     return 0;

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);

I still recommend have a central place to call usb_charger_detect_type() to cover real charger detection instead of leaving this question open to diff users. This can be done after vbus-on is detected and before do gadget connect. I don't think this

will make your framework complicated.

This API is used for detecting the charger type manually (software charger detection), so if user's udc driver is needed to do this, then they can issue it at their right place to make it more flexible. But let us see other people's suggestion. Thanks.

Quoted 4 lines

Ok, actually this API can be used for what ever charger detection type, user can implement any method(manual detection, directly read result from some HW...what ever) in uchger->charger_detect(), target is

But reading result from some HW dose not means *detect*, actually is *get*. We can not mix them together with the different semantics.

Quoted 3 lines

to have the charger type and set uchger->type, then you no need to add the comments/description to limit it usage. Also I do see there is

possible central place to do this.

Quoted 1 line

--
Baolin.wang
Best Regards

Quoted 62 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, April 08, 2016 7:00 PM
To: Jun Li <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; device- [email protected]; [email protected]
Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Hi Jun,

Quoted 44 lines

Quoted 33 lines

Quoted 25 lines

Quoted 25 lines

drivers/usb/gadget/udc/charger.c new file +25 -0

+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type
+manually on your
+ * platform.
+ * You should call it at the right gadget state to avoid
+affecting gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+     enum usb_charger_type type;
+
+     if (WARN(!uchger->charger_detect,
+              "charger detection method should not be NULL"))
+             return -EINVAL;
+
+     type = uchger->charger_detect(uchger);
+
+     mutex_lock(&uchger->lock);
+     uchger->type = type;
+     mutex_unlock(&uchger->lock);
+
+     return 0;

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);

I still recommend have a central place to call usb_charger_detect_type() to cover real charger detection instead of leaving this question open to diff users. This can be done after vbus-on is detected and before do gadget connect. I don't think this

Quoted 6 lines

will make your framework complicated.

This API is used for detecting the charger type manually (software charger detection), so if user's udc driver is needed to do this, then they can issue it at their right place to make it more flexible. But let us see other people's suggestion. Thanks.

Ok, actually this API can be used for what ever charger detection type, user can implement any method(manual detection, directly read result from some HW...what ever) in uchger->charger_detect(), target is

Quoted 2 lines

But reading result from some HW dose not means *detect*, actually is *get*. We can not mix them together with the different semantics.

It doesn’t matter here, you already define the _get_ API for just read the charger type from the saved value(uchger->type), so we can think the API to make uchger->type from UNKNOW to ONE KNOWN type is *detect*, because we don't need 2 separate API one for *get* type from HW and another one for *detect* from HW, only one API involve HW access is enough.

- Detect: (write to and) read from HW to get the charger type, save to uchger->type;
- Get: read the charger type from uchger->type.

Quoted 9 lines

Quoted 3 lines

to have the charger type and set uchger->type, then you no need to add the comments/description to limit it usage. Also I do see there is

possible central place to do this.

Quoted 1 line

--
Baolin.wang
Best Regards

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, April 08, 2016 7:51 PM
To: Jun Li <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; device- [email protected]; [email protected]
Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Quoted 59 lines · Jun Li

Quoted 59 lines

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Friday, April 08, 2016 7:00 PM
To: Jun Li <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; device- [email protected]; [email protected]
Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Hi Jun,

Quoted 40 lines

Quoted 39 lines

Quoted 25 lines

Quoted 25 lines

(quoted diff) +25 -0

+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type
+manually on your
+ * platform.
+ * You should call it at the right gadget state to avoid
+affecting gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+     enum usb_charger_type type;
+
+     if (WARN(!uchger->charger_detect,
+              "charger detection method should not be NULL"))
+             return -EINVAL;
+
+     type = uchger->charger_detect(uchger);
+
+     mutex_lock(&uchger->lock);
+     uchger->type = type;
+     mutex_unlock(&uchger->lock);
+
+     return 0;

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);

I still recommend have a central place to call usb_charger_detect_type() to cover real charger detection instead of leaving this question open to diff users. This can be done after vbus-on is detected and before do gadget connect. I don't think this

will make your framework complicated.

This API is used for detecting the charger type manually (software charger detection), so if user's udc driver is needed to do this, then they can issue it at their right place to make it more flexible. But let us see other people's suggestion. Thanks.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Ok, actually this API can be used for what ever charger detection type, user can implement any method(manual detection, directly read result from some HW...what ever) in uchger->charger_detect(), target is

But reading result from some HW dose not means *detect*, actually is

*get*.

Quoted 7 lines

Quoted 1 line

We can not mix them together with the different semantics.

It doesn’t matter here, you already define the _get_ API for just read the charger type from the saved value(uchger->type), so we can think the API to make uchger->type from UNKNOW to ONE KNOWN type is *detect*, because we don't need 2 separate API one for *get* type from HW and another one for *detect* from HW, only one API involve HW access

is enough.

But another issue is some users may need to get the charger type from power supply by "power_supply_get_property()" function, we need to integrate with the power supply things in the usb charger framework, not user to implement that.

Quoted 16 lines

- Detect: (write to and) read from HW to get the charger type, save to uchger->type;
- Get: read the charger type from uchger->type.

Quoted 10 lines

Quoted 4 lines

to have the charger type and set uchger->type, then you no need to add the comments/description to limit it usage. Also I do see there is

possible central place to do this.

Quoted 1 line

--
Baolin.wang
Best Regards

--
Baolin.wang
Best Regards

Quoted 54 lines

Quoted 40 lines

Quoted 40 lines

Quoted 40 lines

Quoted 39 lines

Quoted 25 lines

Quoted 25 lines

(quoted diff) +25 -0

+/*
+ * usb_charger_detect_type() - detect the charger type manually.
+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger type
+manually on your
+ * platform.
+ * You should call it at the right gadget state to avoid
+affecting gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+     enum usb_charger_type type;
+
+     if (WARN(!uchger->charger_detect,
+              "charger detection method should not be NULL"))
+             return -EINVAL;
+
+     type = uchger->charger_detect(uchger);
+
+     mutex_lock(&uchger->lock);
+     uchger->type = type;
+     mutex_unlock(&uchger->lock);
+
+     return 0;

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);

I still recommend have a central place to call usb_charger_detect_type() to cover real charger detection instead of leaving this question open to diff users. This can be done after vbus-on is detected and before do gadget connect. I don't think this

will make your framework complicated.

This API is used for detecting the charger type manually (software charger detection), so if user's udc driver is needed to do this, then they can issue it at their right place to make it more flexible. But let us see other people's suggestion. Thanks.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Ok, actually this API can be used for what ever charger detection type, user can implement any method(manual detection, directly read result from some HW...what ever) in uchger->charger_detect(), target is

But reading result from some HW dose not means *detect*, actually is

*get*.

Quoted 7 lines

Quoted 1 line

We can not mix them together with the different semantics.

It doesn’t matter here, you already define the _get_ API for just read the charger type from the saved value(uchger->type), so we can think the API to make uchger->type from UNKNOW to ONE KNOWN type is *detect*, because we don't need 2 separate API one for *get* type from HW and another one for *detect* from HW, only one API involve HW access

Quoted 6 lines

is enough.

But another issue is some users may need to get the charger type from power supply by "power_supply_get_property()" function, we need to integrate with the power supply things in the usb charger framework, not user to implement that.

Why this user(your case) can't put the charger type get by "power_supply_get_property()" in its uchger->charger_detect(), any special reason prevent you doing it? I am not sure if this case is very common, even if it is, you also can put it in usb_charger_detect_type()

usb_charger_detect_type()

{
        If can get from power_supply_get_property()
                ...
        else if uchger->charger_detect
                uchger->charger_detect();
        ...
}

I don't know if most design of usb charger detection now days is as easy as your PMIC for software(HW does the whole detection process and prevent the vbus generation until detection has completed), anyway if your framework can guarantee the detection process finished before gadget connect, then each user don't have to worry about the HW detection process details and seek a proper place to do it.

Li Jun

Quoted 22 lines

Quoted 16 lines

- Detect: (write to and) read from HW to get the charger type, save to uchger->type;
- Get: read the charger type from uchger->type.

Quoted 10 lines

Quoted 4 lines

to have the charger type and set uchger->type, then you no need to add the comments/description to limit it usage. Also I do see there is

possible central place to do this.

Quoted 1 line

--
Baolin.wang
Best Regards

--
Baolin.wang
Best Regards

-----Original Message-----
From: Baolin Wang [mailto:[email protected]]
Sent: Monday, April 11, 2016 7:15 PM
To: Jun Li <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; device- [email protected]; [email protected]
Subject: Re: [PATCH v10 1/4] gadget: Introduce the usb charger framework

Hi Jun,

Sorry for late reply.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+/*
+ * usb_charger_detect_type() - detect the charger type

manually.

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

Quoted 15 lines

+ * @uchger - usb charger device
+ *
+ * Note: You should ensure you need to detect the charger

(quoted diff) +12 -0

+type manually on your
+ * platform.
+ * You should call it at the right gadget state to avoid
+affecting gadget
+ * enumeration.
+ */
+int usb_charger_detect_type(struct usb_charger *uchger) {
+     enum usb_charger_type type;
+
+     if (WARN(!uchger->charger_detect,
+              "charger detection method should not be NULL"))
+             return -EINVAL;

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+     type = uchger->charger_detect(uchger);
+
+     mutex_lock(&uchger->lock);
+     uchger->type = type;
+     mutex_unlock(&uchger->lock);
+
+     return 0;
+}
+EXPORT_SYMBOL_GPL(usb_charger_detect_type);

I still recommend have a central place to call usb_charger_detect_type() to cover real charger detection instead of leaving this question open to diff users. This can be done after vbus-on is detected and before do gadget connect. I don't think this

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

will make your framework complicated.

This API is used for detecting the charger type manually (software charger detection), so if user's udc driver is needed to do this, then they can issue it at their right place to make

it more flexible.

Quoted 31 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

But let us see other people's suggestion. Thanks.

Ok, actually this API can be used for what ever charger detection type, user can implement any method(manual detection, directly read result from some HW...what ever) in uchger->charger_detect(), target is

But reading result from some HW dose not means *detect*, actually is

*get*.

Quoted 8 lines

Quoted 1 line

We can not mix them together with the different semantics.

It doesn’t matter here, you already define the _get_ API for just read the charger type from the saved value(uchger->type), so we can think the API to make uchger->type from UNKNOW to ONE KNOWN type is *detect*, because we don't need 2 separate API one for *get* type from HW and another one for *detect* from HW, only one API involve HW access

Quoted 6 lines

is enough.

But another issue is some users may need to get the charger type from power supply by "power_supply_get_property()" function, we need to integrate with the power supply things in the usb charger framework, not user to implement that.

Why this user(your case) can't put the charger type get by "power_supply_get_property()" in its uchger->charger_detect(), any special reason prevent you doing it? I am not sure if this case is very common, even if it is, you also can put it in usb_charger_detect_type()

Quoted 8 lines

usb_charger_detect_type()

{
        If can get from power_supply_get_property()
                ...
        else if uchger->charger_detect
                uchger->charger_detect();
        ...
}

If user want to implement above method, they need to get the 'power_supply' structure to do this action, which maybe can not get in some contexts. So we need to integrate with the power supply things to avoid this situation. IIRC, Felipe suggested me to flesh out the charger getting method.

Hi Felipe, what do you think of Jun's suggestion? Thanks.

Quoted 31 lines

I don't know if most design of usb charger detection now days is as easy as your PMIC for software(HW does the whole detection process and prevent the vbus generation until detection has completed), anyway if your framework can guarantee the detection process finished before gadget connect, then each user don't have to worry about the HW detection process details and seek a proper place to do it.

Li Jun

Quoted 22 lines

Quoted 16 lines

- Detect: (write to and) read from HW to get the charger type, save to uchger->type;
- Get: read the charger type from uchger->type.

Quoted 10 lines

Quoted 4 lines

to have the charger type and set uchger->type, then you no need to add the comments/description to limit it usage. Also I do see there is

possible central place to do this.

Quoted 1 line

--
Baolin.wang
Best Regards

--
Baolin.wang
Best Regards

--
Baolin.wang
Best Regards

Quoted 21 lines

Quoted 12 lines

Quoted 5 lines

But another issue is some users may need to get the charger type from power supply by "power_supply_get_property()" function, we need to integrate with the power supply things in the usb charger framework, not user to implement that.

Why this user(your case) can't put the charger type get by "power_supply_get_property()" in its uchger->charger_detect(), any special reason prevent you doing it? I am not sure if this case is very common, even if it is, you also can put it in usb_charger_detect_type()

Quoted 8 lines

usb_charger_detect_type()

{
        If can get from power_supply_get_property()
                ...
        else if uchger->charger_detect
                uchger->charger_detect();
        ...
}

Quoted 6 lines

If user want to implement above method, they need to get the 'power_supply' structure to do this action, which maybe can not get in some contexts. So we need to integrate with the power supply things to avoid this situation. IIRC, Felipe suggested me to flesh out the charger getting method.

Okay, then I agree with you to let user do that with more flexibility, I tried to enable usb charger detection on one NXP i.mx platform based on this framework, works fine, thanks!

Currently the Linux kernel does not provide any standard integration of this feature that integrates the USB subsystem with the system power regulation provided by PMICs meaning that either vendors must add this in their kernels or USB gadget devices based on Linux (such as mobile phones) may not behave as they should. Thus provide a standard framework for doing this in kernel.

Now introduce one user with wm831x_power to support and test the usb charger, which is pending testing. Moreover there may be other potential users will use it in future.

Changes since v9:
- Remove some redundant sysfs attributes.
- Change the SDP charger default current if gadget is SS.
- Remove the 'get_charger_type' callback in gadget->ops.

Baolin Wang (4):
gadget: Introduce the usb charger framework
gadget: Support for the usb charger framework
gadget: Integrate with the usb gadget supporting for usb charger
power: wm831x_power: Support USB charger current limit management

 drivers/power/wm831x_power.c      |   69 ++++
 drivers/usb/gadget/Kconfig        |    7 +
 drivers/usb/gadget/udc/Makefile   |    1 +
 drivers/usb/gadget/udc/charger.c  |  766 +++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/udc-core.c |   11 +
 include/linux/mfd/wm831x/pdata.h  |    3 +
 include/linux/usb/charger.h       |  173 +++++++++
 include/linux/usb/gadget.h        |   13 +
 include/uapi/linux/usb/charger.h  |   31 ++
 9 files changed, 1074 insertions(+)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

--
1.7.9.5