Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Mark Brown
2016-10-28T17:04:02Z

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
> On 28 October 2016 at 06:00, NeilBrown <neilb@suse.com> wrote:

> > 1/ I think we agreed that it doesn't make sense for there to be
> > two chargers registered in a system.

> Yes, until now...

> > However usb_charger_register() still allows that, and assigns
> > and arbitrary name to each based on discovery order.
> > This *cannot* make sense.

> Fine, I can change that to allow only one charger to register.

Yeah, it's a reasonable change. I'm not sure the prior discussion was
100% conclusive on the issue (I remember there being some debate about
leaving things there to avoid any need for future refactoring to touch
the interface).

> > 2/ Why do you have usb_charger_set_current()??
> > No code ever calls it.
> > This updates the min and max current which are defined in a
> > standard. It never makes sense to change the min and max
> > for a particular cable type.

> Mark, do we have some scenarios which want to change the current
> limitation? If not, okay, I agree with you to remove this function.

I'm not aware of any, we can always add it back if the need arises.

> > Related: I don't like charger_type_show(). I don't think
> > the usb-charger should export that information to user-space because
> > extcon already does that, and duplication is confusing and pointless.

> I think we should combine all charger related information into one
> place for user. Moreover if we don't get charger type from extcon, we
> should also need one place to export the charger type.

I had also thought there was some software negotation as well as the
physical charger in cases where the device is plugged into an active
host? I could be wrong.

> > 5/ There is no convincing example usage of this framework.
> > wm8931x_power.c just scratches the surface.
> > If it is so good, it should be easy to convert a lot of other
> > drivers over to it. If you did that it would be much easier
> > to see how it works and what the strengths/weaknesses were.

> Jun have send out one patchset[1] based on my patchset, and he tested
> mypatchset. Thanks for your comments.
> [1]http://www.spinics.net/lists/linux-usb/msg139809.html

I think it's a good idea to pick up Jun's patches into your patch set,
that way Jun doesn't need to rebase and it might help with review of
your patches too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]