| Message ID | 1442612399-341-10-git-send-email-dannenberg@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, 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. -- Sebastian
On Tue, Sep 22, 2015 at 09:16:49PM +0200, Sebastian Reichel wrote: > Hi, > > 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. Regards, -- Andreas Dannenberg Texas Instruments Inc > > -- Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- Sebastian
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). 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? Or should we introduce a new sysfs property? -- Andreas Dannenberg Texas Instruments Inc -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Wed, Sep 23, 2015 at 05:02:28PM +0200, Sebastian Reichel wrote: > Hi, > > 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 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. Regards, -- Andreas Dannenberg Texas Instruments Inc -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Wed, Sep 23, 2015 at 08:53:59PM +0200, Sebastian Reichel wrote: > 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. Ok yes considering the default being the DT value what you originally wrote that makes more sense now. My mind was buried in the code :) > > 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. I don't think there is really a right or wrong here but I do see your point. It's a workable and safe solution. Will implement accordingly. Regards, -- Andreas Dannenberg Texas Instruments Inc -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c index 615624c..66867d5 100644 --- a/drivers/power/bq24257_charger.c +++ b/drivers/power/bq24257_charger.c @@ -269,6 +269,42 @@ enum bq24257_fault { FAULT_INPUT_LDO_LOW, }; +static int bq24257_get_input_current_limit(struct bq24257_device *bq, + union power_supply_propval *val) +{ + int ret; + + ret = bq24257_field_read(bq, F_IILIMIT); + if (ret < 0) + return ret; + + /* + * The "External ILIM" and "Production & Test" modes are not exposed + * through this driver and not being covered by the lookup table. + * Should such a mode have become active let's return an error rather + * than exceeding the bounds of the lookup table and returning + * garbage. + */ + if (ret >= BQ24257_IILIMIT_MAP_SIZE) + return -ENODATA; + + val->intval = bq24257_iilimit_map[ret]; + + return 0; +} + +static int bq24257_set_input_current_limit(struct bq24257_device *bq, + const union power_supply_propval *val) +{ + if (bq->iilimit_autoset_enable) + return -EPERM; + + return bq24257_field_write(bq, F_IILIMIT, + bq24257_find_idx(val->intval, + bq24257_iilimit_map, + BQ24257_IILIMIT_MAP_SIZE)); +} + static int bq24257_power_supply_get_property(struct power_supply *psy, enum power_supply_property psp, union power_supply_propval *val) @@ -353,6 +389,9 @@ static int bq24257_power_supply_get_property(struct power_supply *psy, val->intval = bq24257_iterm_map[bq->init_data.iterm]; break; + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: + return bq24257_get_input_current_limit(bq, val); + default: return -EINVAL; } @@ -360,6 +399,31 @@ static int bq24257_power_supply_get_property(struct power_supply *psy, return 0; } +static int bq24257_power_supply_set_property(struct power_supply *psy, + enum power_supply_property prop, + const union power_supply_propval *val) +{ + struct bq24257_device *bq = power_supply_get_drvdata(psy); + + switch (prop) { + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: + return bq24257_set_input_current_limit(bq, val); + default: + return -EINVAL; + } +} + +static int bq24257_power_supply_property_is_writeable(struct power_supply *psy, + enum power_supply_property psp) +{ + switch (psp) { + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: + return true; + default: + return false; + } +} + static int bq24257_get_chip_state(struct bq24257_device *bq, struct bq24257_state *state) { @@ -694,6 +758,7 @@ static enum power_supply_property bq24257_power_supply_props[] = { POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX, POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT, + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, }; static char *bq24257_charger_supplied_to[] = { @@ -706,6 +771,8 @@ static const struct power_supply_desc bq24257_power_supply_desc = { .properties = bq24257_power_supply_props, .num_properties = ARRAY_SIZE(bq24257_power_supply_props), .get_property = bq24257_power_supply_get_property, + .set_property = bq24257_power_supply_set_property, + .property_is_writeable = bq24257_power_supply_property_is_writeable, }; static ssize_t bq24257_show_ovp_voltage(struct device *dev,
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. Signed-off-by: Andreas Dannenberg <dannenberg@ti.com> --- drivers/power/bq24257_charger.c | 67 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)