Re: [PATCH v5 09/11] power: bq24257: Allow input current limit sysfs access

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Wed, Sep 23, 2015 at 09:11:46AM -0500, Andreas Dannenberg wrote:
> On Wed, Sep 23, 2015 at 02:29:06AM +0200, Sebastian Reichel wrote:
> > On Tue, Sep 22, 2015 at 05:10:45PM -0500, Andreas Dannenberg wrote:
> > > On Tue, Sep 22, 2015 at 09:16:49PM +0200, Sebastian Reichel wrote:
> > > > On Fri, Sep 18, 2015 at 04:39:57PM -0500, Andreas Dannenberg wrote:
> > > > > This patch allows reading (and writing, if the D+/D- USB signal-based
> > > > > charger type detection is disabled) of the input current limit through
> > > > > the power supply's input_current_limit sysfs property. This allows
> > > > > userspace to see what charger was detected and to re-configure the
> > > > > maximum current drawn from the external supply at runtime based on
> > > > > system-level knowledge or user input.
> > > > 
> > > > Maybe also support writing into input_current_limit in auto mode.
> > > > Just disable auto detection until "auto" is written into sysfs node.
> > > 
> > > Auto-detection was enabled by default in the original driver so I think
> > > that should be left intact. I added the ability to manually override
> > > this via DT with a fixed value, and then configure said fixed value
> > > through sysfs at runtime.
> > > 
> > > I'm not 100% clear on the usecase of runtime enabling/disabling auto so
> > > I'd rather leave the implementation as-is. Either auto mode is enabled
> > > or not -- and this is directly tied to the DT setting. But if someone
> > > has a strong usecase for this I can certainly add it.
> > 
> > For some usb power supplies auto-detection doesn't work very well,
> > resulting in a 100mA default fallback. Users knowing their hardware
> > could force charging with the correct input current limitation.
> 
> Ok. So how should we best go about extending the usage of the
> 'input_current_limit' sysfs node for this charger? You mentioned
> writing 'auto' into it should enable the auto-detection mode. I suppose
> writing a fixed current value will disable it. But how to indicate to
> the user when reading 'input_current_limit' whether auto mode is enabled
> or not (I think this is something we should do).

Right, I haven't thought of this.

> Can we return a mixed number/string like this?
> 
> # Example: charger auto detection mode is disabled, and input current
> # limit is configured as 500mA.
> 
> $ cat input_current_limit
> 500000
> 
> # Example: charger auto detection mode is enabled, and a charger
> # supporting 1A was detected (note the mixed number/string thats
> # returned)
> 
> $ cat input_current_limit
> 1000000 (auto)
> 
> Would that work?

No. sysfs nodes should only contain one value per file.

> Or should we introduce a new sysfs property?

So maybe keep this patch as is (disallow setting the limit in auto
mode) and create another file for setting (and getting) the mode.

Also it might be a good idea to return to safe defaults when the
charger is disconnected (-> reset to DT specified values).

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]