Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755104AbcDGGL1 (ORCPT ); Thu, 7 Apr 2016 02:11:27 -0400 Received: from mail-yw0-f178.google.com ([209.85.161.178]:33209 "EHLO mail-yw0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754658AbcDGGLZ (ORCPT ); Thu, 7 Apr 2016 02:11:25 -0400 MIME-Version: 1.0 In-Reply-To: <87bn5m10fh.fsf@intel.com> References: <6c594cc66fd06b575b04cc8bb0fe0374d0501d4d.1459494744.git.baolin.wang@linaro.org> <20160406072513.GB21101@shlinux2.ap.freescale.net> <87vb3v2nm8.fsf@intel.com> <20160406074310.GC21101@shlinux2.ap.freescale.net> <87shyz2md5.fsf@intel.com> <20160406081006.GD21101@shlinux2.ap.freescale.net> <87k2kb2fwd.fsf@intel.com> <20160407023925.GA10672@shlinux2.ap.freescale.net> <87bn5m10fh.fsf@intel.com> Date: Thu, 7 Apr 2016 14:11:19 +0800 Message-ID: Subject: Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework From: Baolin Wang To: Felipe Balbi Cc: Peter Chen , Greg KH , Sebastian Reichel , Dmitry Eremin-Solenikov , David Woodhouse , Peter Chen , Alan Stern , r.baldyga@samsung.com, Yoshihiro Shimoda , Lee Jones , Mark Brown , Charles Keepax , patches@opensource.wolfsonmicro.com, Linux PM list , USB , device-mainlining@lists.linuxfoundation.org, LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3557 Lines: 99 On 7 April 2016 at 12:56, Felipe Balbi wrote: > > Hi, > > Peter Chen writes: >> On Wed, Apr 06, 2016 at 01:25:06PM +0300, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Peter Chen writes: >>> > On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote: >>> >> Peter Chen writes: >>> >> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote: >>> >> >> Peter Chen 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. > > Oh, now I get your comment and I totally agree. We already *know* the > detected charger type, there's no point in showing them all. > >> 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 >> +}; > > agreed, const though. OK. Thanks for Felipe, Peter and Jun's suggestion. I'll modify it in next version. > > -- > balbi -- Baolin.wang Best Regards