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

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]>

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

---
 drivers/usb/gadget/Kconfig       |    7 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/charger.h      |  191 ++++++++++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 1000 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/Kconfig +0 -0

index 2057add..89f4e9b 100644

drivers/usb/gadget/udc/charger.c +0 -0

+struct class *usb_charger_class;

+subsys_initcall(usb_charger_class_init);

Hi,

Quoted 44 lines · Baolin Wang

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]>

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

---
 drivers/usb/gadget/Kconfig       |    7 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/charger.h      |  191 ++++++++++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 1000 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/Kconfig +0 -0

index 2057add..89f4e9b 100644
@@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
      help
         It supports the serial gadget can be used as a console.
 

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 138

Quoted 2 lines

drivers/usb/gadget/Kconfig +2 -0

+config USB_CHARGER
+     bool "USB charger support"

you didn't build test all possibilities, did you?

I have a feeling this won't link if USB_GADGET=m. Can you test that?

Quoted 2 lines

drivers/usb/gadget/udc/charger.c +0 -0

[...]

Quoted 1 line

+struct class *usb_charger_class;

We already have a UDC class, do we really, really need another class here?

Quoted 2 lines

+subsys_initcall(usb_charger_class_init);

this should always work as module_init(). Please make sure that's the case.

--
balbi

Quoted 44 lines

Quoted 44 lines

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]>

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

---
 drivers/usb/gadget/Kconfig       |    7 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/charger.h      |  191 ++++++++++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 1000 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/Kconfig +0 -0

index 2057add..89f4e9b 100644
@@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
      help
         It supports the serial gadget can be used as a console.
 

Quoted 6 lines

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 138

Quoted 2 lines

drivers/usb/gadget/Kconfig +2 -0

+config USB_CHARGER
+     bool "USB charger support"

you didn't build test all possibilities, did you?

I have a feeling this won't link if USB_GADGET=m. Can you test that?

OK.

Quoted 3 lines

Quoted 2 lines

drivers/usb/gadget/udc/charger.c +0 -0

[...]

Quoted 4 lines

Quoted 1 line

+struct class *usb_charger_class;

We already have a UDC class, do we really, really need another class here?

We want to manage the usb charger devices by this class and one usb charger device is not equal with one usb device which managed by UDC

class, so can we use UDC class to manage charger devices? By the way, you also suggested to use the 'class' things instead of 'bus' in previous mail.

Quoted 4 lines

Quoted 1 line

+subsys_initcall(usb_charger_class_init);

this should always work as module_init(). Please make sure that's the case.

we should make sure the charger class has been allocated before user try to add a new gadget to the udc class. So it should be issued before 'module_init()' (many usb drivers are at module_init level).

Hi,

Quoted 58 lines · Baolin Wang

Quoted 44 lines

Quoted 44 lines

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]>

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

---
 drivers/usb/gadget/Kconfig       |    7 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  770 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/charger.h      |  191 ++++++++++
 include/uapi/linux/usb/charger.h |   31 ++
 5 files changed, 1000 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/Kconfig +0 -0

index 2057add..89f4e9b 100644
@@ -134,6 +134,13 @@ config U_SERIAL_CONSOLE
      help
         It supports the serial gadget can be used as a console.
 

Quoted 6 lines

drivers/usb/gadget/Kconfig config U_SERIAL_CONSOLE · line 138

Quoted 2 lines

drivers/usb/gadget/Kconfig +2 -0

+config USB_CHARGER
+     bool "USB charger support"

you didn't build test all possibilities, did you?

I have a feeling this won't link if USB_GADGET=m. Can you test that?

OK.

Quoted 5 lines

Quoted 2 lines

drivers/usb/gadget/udc/charger.c +0 -0

[...]

Quoted 1 line

+struct class *usb_charger_class;

Quoted 5 lines

Quoted 2 lines

We already have a UDC class, do we really, really need another class here?

We want to manage the usb charger devices by this class and one usb charger device is not equal with one usb device which managed by UDC

Can you explain this statement? If charging is done via a USB peripheral port, why don't we have a 1:1 mapping between charger and UDC?

Quoted 4 lines

class, so can we use UDC class to manage charger devices? By the way, you also suggested to use the 'class' things instead of 'bus' in previous mail.

true, I did say that. It seems clearer, however, that we don't need this virtual device here.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

+subsys_initcall(usb_charger_class_init);

this should always work as module_init(). Please make sure that's the case.

we should make sure the charger class has been allocated before user try to add a new gadget to the udc class. So it should be issued before 'module_init()' (many usb drivers are at module_init level).

-EPROBE_DEFER?

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Hi,

Quoted 3 lines · Baolin Wang

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

Quoted 6 lines

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's

minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 4 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

--
balbi

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

Hi,

Quoted 3 lines · Baolin Wang

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

manage in what way? It seems to me that they don't need to be real devices, just a handle as part of struct usb_gadget, no?

Quoted 8 lines

Quoted 6 lines · Baolin Wang

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

Quoted 2 lines

manage in what way? It seems to me that they don't need to be real devices, just a handle as part of struct usb_gadget, no?

Although charger device is not one real hardware device, we also use one 'struct device' to describe it in charger.c file. So we should manage the 'struct device' with one proper way.

Hi,

Quoted 8 lines · Baolin Wang

Quoted 8 lines

Quoted 6 lines · Baolin Wang

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

Quoted 6 lines

Quoted 2 lines

manage in what way? It seems to me that they don't need to be real devices, just a handle as part of struct usb_gadget, no?

Although charger device is not one real hardware device, we also use one 'struct device' to describe it in charger.c file. So we should manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

Quoted 12 lines

Quoted 11 lines · Baolin Wang

Quoted 10 lines

Quoted 6 lines · Baolin Wang

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

manage in what way? It seems to me that they don't need to be real devices, just a handle as part of struct usb_gadget, no?

Quoted 5 lines

Quoted 3 lines

Although charger device is not one real hardware device, we also use one 'struct device' to describe it in charger.c file. So we should manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

We can get/put usb charger and mange usb charger attributes with the device model if we use a struct device.

Hi,

Quoted 18 lines · Baolin Wang

Quoted 17 lines

Quoted 8 lines · Baolin Wang

Quoted 8 lines

Quoted 6 lines · Baolin Wang

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

Quoted 6 lines

Quoted 2 lines

manage in what way? It seems to me that they don't need to be real devices, just a handle as part of struct usb_gadget, no?

Although charger device is not one real hardware device, we also use one 'struct device' to describe it in charger.c file. So we should manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

Quoted 2 lines

We can get/put usb charger and mange usb charger attributes with the device model if we use a struct device.

We already have that as part of struct usb_udc. Why don't you just create a subdirectory called charger which will hold all your charger-related attributes. That directory will only be created if a valid ->charger pointer exists.

USB Charging is always tied to a peripheral side controller, anyway.

--
balbi

Quoted 19 lines

Quoted 18 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

manage in what way? It seems to me that they don't need to be real devices, just a handle as part of struct usb_gadget, no?

Although charger device is not one real hardware device, we also use one 'struct device' to describe it in charger.c file. So we should manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

We can get/put usb charger and mange usb charger attributes with the device model if we use a struct device.

Quoted 4 lines

We already have that as part of struct usb_udc. Why don't you just create a subdirectory called charger which will hold all your charger-related attributes. That directory will only be created if a valid ->charger pointer exists.

That means we can remove all the device and class things in charger.c file, right? OK, I try to do that. Thanks.

Quoted 24 lines

Quoted 23 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

manage in what way? It seems to me that they don't need to be real devices, just a handle as part of struct usb_gadget, no?

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Although charger device is not one real hardware device, we also use one 'struct device' to describe it in charger.c file. So we should manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

We can get/put usb charger and mange usb charger attributes with the device model if we use a struct device.

We already have that as part of struct usb_udc. Why don't you just create a subdirectory called charger which will hold all your charger-related attributes. That directory will only be created if a valid ->charger pointer exists.

Quoted 2 lines

That means we can remove all the device and class things in charger.c file, right? OK, I try to do that. Thanks.

right. Keep your charger.c file, because to conditionally compile and

link that to udc-core.ko, but remove all the class initialization and all of that extra code.

Signature · 1 line

balbi

Hi,

Quoted 24 lines · Baolin Wang

Quoted 23 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Can't you just tie a charger to a UDC and avoid the charger class completely?

Yeah, I also hope so. But we really want something to manage the charger devices, do you have any good suggestion to avoid the 'class' but also can manage the charger devices?

manage in what way? It seems to me that they don't need to be real devices, just a handle as part of struct usb_gadget, no?

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Although charger device is not one real hardware device, we also use one 'struct device' to describe it in charger.c file. So we should manage the 'struct device' with one proper way.

that's fine, but why do you think they need a struct device to start with?

We can get/put usb charger and mange usb charger attributes with the device model if we use a struct device.

We already have that as part of struct usb_udc. Why don't you just create a subdirectory called charger which will hold all your charger-related attributes. That directory will only be created if a valid ->charger pointer exists.

Quoted 2 lines

That means we can remove all the device and class things in charger.c file, right? OK, I try to do that. Thanks.

right. Keep your charger.c file, because to conditionally compile and link that to udc-core.ko, but remove all the class initialization and all of that extra code.

Hi,

Quoted 9 lines · Baolin Wang

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

Quoted 22 lines

Quoted 9 lines · Baolin Wang

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

Quoted 2 lines

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

We will access the 'gadget->charger->type' member when issuing usb_gadget_set_cur_limit_by_type(), so I think I should leave the check here in next new version.

Hi,

Quoted 22 lines · Baolin Wang

Quoted 22 lines

Quoted 9 lines · Baolin Wang

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

Quoted 6 lines

Quoted 2 lines

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

We will access the 'gadget->charger->type' member when issuing usb_gadget_set_cur_limit_by_type(), so I think I should leave the check here in next new version.

Here's what I mean:

int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)

{
        struct usb_charger *charger;
        enum usb_charger_type type;

        if (!gadget->charger)
                return 0;

        charger = gadget->charger;
        type = charger->type;

        return __usb_charger_set_cur_limit(charger, type, mA);
}

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)

{
        usb_charger_set_cur_limit(gadget,  mA);

        if (!gadget->ops->vbus_draw)
                return -EOPNOTSUPP;
        return gadget->ops->vbus_draw(gadget, mA);
}

--
balbi

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

Quoted 6 lines

Quoted 2 lines

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

We will access the 'gadget->charger->type' member when issuing usb_gadget_set_cur_limit_by_type(), so I think I should leave the check here in next new version.

Here's what I mean:

int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)

Quoted 12 lines

{
        struct usb_charger *charger;
        enum usb_charger_type type;

        if (!gadget->charger)
                return 0;

        charger = gadget->charger;
        type = charger->type;

        return __usb_charger_set_cur_limit(charger, type, mA);
}

But that means we need to export both 'usb_charger_set_cur_limit()' function and '__usb_charger_set_cur_limit()' function in charger.c file. Cause some user may want to set the current limitation by one charger type parameter (may be not from charger->type), like by issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do you think about this situation? Thanks.

Hi,

Quoted 44 lines · Baolin Wang

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

Quoted 6 lines

Quoted 2 lines

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

We will access the 'gadget->charger->type' member when issuing usb_gadget_set_cur_limit_by_type(), so I think I should leave the check here in next new version.

Here's what I mean:

int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)

Quoted 12 lines

{
        struct usb_charger *charger;
        enum usb_charger_type type;

        if (!gadget->charger)
                return 0;

        charger = gadget->charger;
        type = charger->type;

        return __usb_charger_set_cur_limit(charger, type, mA);
}

Quoted 6 lines

But that means we need to export both 'usb_charger_set_cur_limit()' function and '__usb_charger_set_cur_limit()' function in charger.c file. Cause some user may want to set the current limitation by one charger type parameter (may be not from charger->type), like by issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do you think about this situation? Thanks.

if we have that requirement, that's totally fine. Just rename __usb_charger_set_cur_limit() back to _usb_charger_set_cur_limit_by_type() and expose both. But set_cur_limit_by_type can assume its arguments are valid at all times.

Quoted 51 lines

Quoted 50 lines

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

Quoted 6 lines

Quoted 2 lines

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

We will access the 'gadget->charger->type' member when issuing usb_gadget_set_cur_limit_by_type(), so I think I should leave the check here in next new version.

Here's what I mean:

int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)

Quoted 12 lines

{
        struct usb_charger *charger;
        enum usb_charger_type type;

        if (!gadget->charger)
                return 0;

        charger = gadget->charger;
        type = charger->type;

        return __usb_charger_set_cur_limit(charger, type, mA);
}

But that means we need to export both 'usb_charger_set_cur_limit()' function and '__usb_charger_set_cur_limit()' function in charger.c file. Cause some user may want to set the current limitation by one charger type parameter (may be not from charger->type), like by issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do you think about this situation? Thanks.

Quoted 4 lines

if we have that requirement, that's totally fine. Just rename __usb_charger_set_cur_limit() back to _usb_charger_set_cur_limit_by_type() and expose both. But set_cur_limit_by_type can assume its arguments are valid at all times.

Make sense. I'll fix this issue in v14 version. Thanks.

Hi,

Quoted 56 lines · Baolin Wang

Quoted 55 lines

Quoted 44 lines

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

Quoted 6 lines

Quoted 2 lines

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

We will access the 'gadget->charger->type' member when issuing usb_gadget_set_cur_limit_by_type(), so I think I should leave the check here in next new version.

Here's what I mean:

int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)

Quoted 12 lines

{
        struct usb_charger *charger;
        enum usb_charger_type type;

        if (!gadget->charger)
                return 0;

        charger = gadget->charger;
        type = charger->type;

        return __usb_charger_set_cur_limit(charger, type, mA);
}

Quoted 6 lines

But that means we need to export both 'usb_charger_set_cur_limit()' function and '__usb_charger_set_cur_limit()' function in charger.c file. Cause some user may want to set the current limitation by one charger type parameter (may be not from charger->type), like by issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do you think about this situation? Thanks.

if we have that requirement, that's totally fine. Just rename __usb_charger_set_cur_limit() back to _usb_charger_set_cur_limit_by_type() and expose both. But set_cur_limit_by_type can assume its arguments are valid at all times.

Quoted 1 line

Make sense. I'll fix this issue in v14 version. Thanks.

there's one thing bothering me though:

gadget->charger is supposed to be "current detected charger" right? If we have a single port tied to a single charger, in which case would we *ever* need to change current limit of any charger type other than "current charger" ?

IOW, why do we need _set_cur_limit_by_type() exported at all?

--
balbi

Quoted 56 lines

Quoted 56 lines

Quoted 55 lines

Quoted 44 lines

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

For supporting the usb charger, it adds the usb_charger_init() and usb_charger_exit() functions for usb charger initialization and exit.

It will report to the usb charger when the gadget state is changed, then the usb charger can do the power things.

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

Reviewed-by Li Jun <[email protected]>

Tested-by Li Jun <[email protected]>

Before anything, I must say that I really liked this patch. It's minimaly invasive to udc core and does all the necessary changes. If it wasn't for the extra charger class, this would've been perfect.

Can't you just tie a charger to a UDC and avoid the charger class completely?

Quoted 3 lines

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) {
+ if (gadget->charger)

Quoted 6 lines

Quoted 2 lines

I guess you could do this check inside
usb_gadget_set_cur_limit_by_type() itself.

We will access the 'gadget->charger->type' member when issuing usb_gadget_set_cur_limit_by_type(), so I think I should leave the check here in next new version.

Here's what I mean:

int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int mA)

Quoted 12 lines

{
        struct usb_charger *charger;
        enum usb_charger_type type;

        if (!gadget->charger)
                return 0;

        charger = gadget->charger;
        type = charger->type;

        return __usb_charger_set_cur_limit(charger, type, mA);
}

Quoted 6 lines

But that means we need to export both 'usb_charger_set_cur_limit()' function and '__usb_charger_set_cur_limit()' function in charger.c file. Cause some user may want to set the current limitation by one charger type parameter (may be not from charger->type), like by issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do you think about this situation? Thanks.

if we have that requirement, that's totally fine. Just rename __usb_charger_set_cur_limit() back to _usb_charger_set_cur_limit_by_type() and expose both. But set_cur_limit_by_type can assume its arguments are valid at all times.

Quoted 8 lines

Quoted 1 line

Make sense. I'll fix this issue in v14 version. Thanks.

there's one thing bothering me though:

gadget->charger is supposed to be "current detected charger" right? If we have a single port tied to a single charger, in which case would we *ever* need to change current limit of any charger type other than "current charger" ?

What I mean is user can set the current limit by charger type as what they want at initial stage. As we know the default current of SDP (not super speed) is 500 mA, but user can set the current limit of SDP as 450 mA if there are some special on their own platform by issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, 450)'.

drivers/power/wm831x_power.c static int wm831x_power_probe(struct platform_device *pdev) · line 652

Quoted 7 lines · Baolin Wang

drivers/power/wm831x_power.c +3 -0

@@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+		power->usb_charger =
+			usb_charger_find_by_name(wm831x_pdata->usb_gadget);

the fact that you rely on strings and pass them via pdata is an indication that you don't have enough description of the HW. Seems like we need to come up with a set of DT properties which tie a charger to a UDC.

I'm thinking a phandle would be enough?

Quoted 8 lines · Felipe Balbi

Quoted 7 lines · Baolin Wang

drivers/power/wm831x_power.c +3 -0

@@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
            }
    }
 
+   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+           power->usb_charger =
+                   usb_charger_find_by_name(wm831x_pdata->usb_gadget);

Quoted 6 lines

the fact that you rely on strings and pass them via pdata is an indication that you don't have enough description of the HW. Seems like we need to come up with a set of DT properties which tie a charger to a UDC.

I'm thinking a phandle would be enough?

The wm831x has no DT support currently.

Quoted 15 lines · Mark Brown

Quoted 13 lines · Felipe Balbi

drivers/power/wm831x_power.c static int wm831x_power_probe(struct platform_device *pdev) · line 652

Quoted 7 lines · Baolin Wang

drivers/power/wm831x_power.c +3 -0

@@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
            }
    }
 
+   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+           power->usb_charger =
+                   usb_charger_find_by_name(wm831x_pdata->usb_gadget);

the fact that you rely on strings and pass them via pdata is an indication that you don't have enough description of the HW. Seems like we need to come up with a set of DT properties which tie a charger to a UDC.

Quoted 3 lines

Quoted 1 line

I'm thinking a phandle would be enough?

The wm831x has no DT support currently.

Another hand I suppose the usb charger is one virtual device and does not need be described from DT.

Hi,

Quoted 18 lines · Baolin Wang

Quoted 17 lines · Mark Brown

Quoted 15 lines · Felipe Balbi

drivers/power/wm831x_power.c static int wm831x_power_probe(struct platform_device *pdev) · line 652

Quoted 7 lines · Baolin Wang

drivers/power/wm831x_power.c +3 -0

@@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
            }
    }
 
+   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+           power->usb_charger =
+                   usb_charger_find_by_name(wm831x_pdata->usb_gadget);

the fact that you rely on strings and pass them via pdata is an indication that you don't have enough description of the HW. Seems like we need to come up with a set of DT properties which tie a charger to a UDC.

I'm thinking a phandle would be enough?

Quoted 4 lines

Quoted 1 line

The wm831x has no DT support currently.

Another hand I suppose the usb charger is one virtual device and does not need be described from DT.

Right, I don't think that should be a device at all. But you can pass a phandle to the UDC controller and use that to get to struct usb_gadget from which you could reach ->charger.

[ Unknown signature status ]

Quoted 13 lines · Felipe Balbi

drivers/power/wm831x_power.c static int wm831x_power_probe(struct platform_device *pdev) · line 652

Quoted 7 lines · Baolin Wang

drivers/power/wm831x_power.c +3 -0

@@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+		power->usb_charger =
+			usb_charger_find_by_name(wm831x_pdata->usb_gadget);

the fact that you rely on strings and pass them via pdata is an indication that you don't have enough description of the HW. Seems like we need to come up with a set of DT properties which tie a charger to a UDC.

Quoted 1 line

I'm thinking a phandle would be enough?

The wm831x has no DT support currently.

Quoted 1 line · Mark Brown

The wm831x has no DT support currently.

okay, perhaps its time to add it.