Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework

[Peter Chen]

On Wed, Apr 06, 2016 at 01:25:06PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@xxxxxxxxx> writes:
> > On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
> >> Peter Chen <hzpeterchen@xxxxxxxxx> writes:
> >> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
> >> >> Peter Chen <hzpeterchen@xxxxxxxxx> writes:
> >> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> >> >> >  +
> >> >> >> +static struct attribute *usb_charger_attrs[] = {
> >> >> >> +	&dev_attr_sdp_current.attr,
> >> >> >> +	&dev_attr_dcp_current.attr,
> >> >> >> +	&dev_attr_cdp_current.attr,
> >> >> >> +	&dev_attr_aca_current.attr,
> >> >> >> +	&dev_attr_charger_type.attr,
> >> >> >> +	&dev_attr_charger_state.attr,
> >> >> >> +	NULL
> >> >> >> +};
> >> >> >
> >> >> > The user may only care about current limit, type and state, why they
> >> >> > need to care what type's current limit, it is the usb charger
> >> >> > framework handles, the framework judge the current according to
> >> >> > charger type and USB state (connect/configured/suspended).
> >> >> 
> >> >> it might be useful if we want to know that $this charger doesn't really
> >> >> give us as much current as it advertises.
> >> >> 
> >> >
> >> > As my understanding, the current limit is dynamic value, it should
> >> > report the value the charger supports now, eg, it connects SDP, but
> >> > the host is suspended now, then the value should be 2mA.
> >> 
> >> yes, and that's the limit. Now consider we connect to DCP or CDP and
> >> limit is 2000mA but we're charging at 1000mA ;-)
> >> 
> >
> > Does the user need to know the $this charger limit? Don't they only
> > care about the current charging value? I have a USB cable which can
> 
> Why not ? UI might want to change the color of the battery charging icon
> if we're charging @ 2000mA or @ 1000mA to give some visual feedback as
> to "how fast" battery is supposed to be charged.
> 
> > show charging current value, it changes from time to time, when it
> > connects to host pc, it shows 430mA; when it connects to dedicated
> > charger, it shows 1000mA.
> 
> good for you, now what does that have to do with $subject ?
> 

+static struct attribute *usb_charger_attrs[] = {
+	&dev_attr_sdp_current.attr,
+	&dev_attr_dcp_current.attr,
+	&dev_attr_cdp_current.attr,
+	&dev_attr_aca_current.attr,
+	&dev_attr_charger_type.attr,
+	&dev_attr_charger_state.attr,
+	NULL
+};

Ok, even the users are interesting in current limit, we still have no
necessary to know all kinds of chargers limit and current value, since
we already have charger type user interface, the framework can show
limit according to charger type.

I think below user interfaces are enough, who do you think?

+static struct attribute *usb_charger_attrs[] = {
+	&dev_attr_current.attr,
+	&dev_attr_current_limit.attr,
+	&dev_attr_charger_type.attr,
+	&dev_attr_charger_state.attr,
+	NULL
+};

-- 

Best Regards,
Peter Chen