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

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Wed, Sep 23, 2015 at 01:32:58PM -0500, Andreas Dannenberg wrote:
> On Wed, Sep 23, 2015 at 05:02:28PM +0200, Sebastian Reichel wrote:
> > On Wed, Sep 23, 2015 at 09:11:46AM -0500, Andreas Dannenberg wrote:
> > > 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.
> 
> Thanks for the continued feedback. Will look into this and add a new
> property.
>  
> > Also it might be a good idea to return to safe defaults when the
> > charger is disconnected (-> reset to DT specified values).
> 
> It already does this when the charger type auto-detection is enabled.
> When configured for manually setting the input current limit the use
> case is a bit more complex and I do not recommend resetting the limit to
> 500mA. Let me explain why:
>
> 1) Using USB chargers is only one of the ways the bq2525x devices can be
> used in a system. Imagine a shipping product that comes with its own
> proprietary wall power (or built-in) supply that let's say cranks out
> 2A.  As a vendor you want to configure your system (meaning, the bq2425x
> via DT) to a fixed value of 2A. The user un-plugging and re-plugging the
> power supply should not arbitrarily change that pre-configured limit

I did not write, that it should reset to 500mA, but that it should
reset to DT specified values. So e.g. if the device is confiured to
be in auto mode in DT, then configured to use fixed 1000mA, then it
should return to auto mode on unplug. OTOH if DT specifies 500mA
fixed and user sets 1000mA, then it should return to 500mA fixes.
So re-plugging returns to pre-configured limits.

> 2) Even in case of USB chargers, userspace could detect the power
> un-plug event, and re-configure the limit to something else.  So I think
> it's really policy that should not be implemented in the Kernel driver.

I think it should be done exactly the opposite way. Userspace should
set the custom value again. My main motivation is, that DT values
should be safe for all attachable power supplies, while the user
supplied value may only be safe with the currently connected power
supply.

-- Sebastian

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