Re: [PATCH] test_power: Add CURRENT and CHARGE_COUNTER properties

[Sebastian Reichel]

Hi,

On Tue, May 14, 2019 at 06:04:58PM +0800, Lecopzer Chen wrote:
> From: "Lecopzer Chen" <lecopzer.chen@xxxxxxxxxxxx>
> 
> Emulate battery current (variable) and battery CHARGE_COUNTER
> (same as battery_capacity) properties.
> 
> Signed-off-by: Lecopzer Chen <lecopzer.chen@xxxxxxxxxxxx>
> Cc: YJ Chiang <yj.chiang@xxxxxxxxxxxx>

Thanks for the patch and thanks for taking care of testing infrastructure.
I don't think it's a good idea to use CHARGE_CAPACITY/CHARGE_NOW also for
CHARGE_COUNTER. CHARGE_COUNTER may be negative, but capacity should not be
negative. Also the patch should be splitted (one for the current feature and
one for the charge counter feature). Last but not least I don't see any
conversion from mA to µA. The userspace sysfs ABI should always
return µA.

-- Sebastian

> ---
>  drivers/power/supply/test_power.c | 33 +++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> index 57246cdbd042..9f85060c84de 100644
> --- a/drivers/power/supply/test_power.c
> +++ b/drivers/power/supply/test_power.c
> @@ -36,6 +36,7 @@ static int battery_present		= 1; /* true */
>  static int battery_technology		= POWER_SUPPLY_TECHNOLOGY_LION;
>  static int battery_capacity		= 50;
>  static int battery_voltage		= 3300;
> +static int battery_current		= 1600;
>  
>  static bool module_initialized;
>  
> @@ -101,6 +102,7 @@ static int test_power_get_battery_property(struct power_supply *psy,
>  		break;
>  	case POWER_SUPPLY_PROP_CAPACITY:
>  	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>  		val->intval = battery_capacity;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> @@ -117,6 +119,10 @@ static int test_power_get_battery_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>  		val->intval = battery_voltage;
>  		break;
> +	case POWER_SUPPLY_PROP_CURRENT_AVG:
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		val->intval = battery_current;
> +		break;
>  	default:
>  		pr_info("%s: some properties deliberately report errors.\n",
>  			__func__);
> @@ -138,6 +144,7 @@ static enum power_supply_property test_power_battery_props[] = {
>  	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_CHARGE_FULL,
>  	POWER_SUPPLY_PROP_CHARGE_NOW,
> +	POWER_SUPPLY_PROP_CHARGE_COUNTER,
>  	POWER_SUPPLY_PROP_CAPACITY,
>  	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
>  	POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG,
> @@ -147,6 +154,8 @@ static enum power_supply_property test_power_battery_props[] = {
>  	POWER_SUPPLY_PROP_SERIAL_NUMBER,
>  	POWER_SUPPLY_PROP_TEMP,
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_AVG,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
>  };
>  
>  static char *test_power_ac_supplied_to[] = {
> @@ -450,6 +459,21 @@ static int param_set_battery_voltage(const char *key,
>  
>  #define param_get_battery_voltage param_get_int
>  
> +static int param_set_battery_current(const char *key,
> +					const struct kernel_param *kp)
> +{
> +	int tmp;
> +
> +	if (1 != sscanf(key, "%d", &tmp))
> +		return -EINVAL;
> +
> +	battery_current = tmp;
> +	signal_power_supply_changed(test_power_supplies[TEST_BATTERY]);
> +	return 0;
> +}
> +
> +#define param_get_battery_current param_get_int
> +
>  static const struct kernel_param_ops param_ops_ac_online = {
>  	.set = param_set_ac_online,
>  	.get = param_get_ac_online,
> @@ -490,6 +514,11 @@ static const struct kernel_param_ops param_ops_battery_voltage = {
>  	.get = param_get_battery_voltage,
>  };
>  
> +static const struct kernel_param_ops param_ops_battery_current = {
> +	.set = param_set_battery_current,
> +	.get = param_get_battery_current,
> +};
> +
>  #define param_check_ac_online(name, p) __param_check(name, p, void);
>  #define param_check_usb_online(name, p) __param_check(name, p, void);
>  #define param_check_battery_status(name, p) __param_check(name, p, void);
> @@ -498,6 +527,7 @@ static const struct kernel_param_ops param_ops_battery_voltage = {
>  #define param_check_battery_health(name, p) __param_check(name, p, void);
>  #define param_check_battery_capacity(name, p) __param_check(name, p, void);
>  #define param_check_battery_voltage(name, p) __param_check(name, p, void);
> +#define param_check_battery_current(name, p) __param_check(name, p, void);
>  
>  
>  module_param(ac_online, ac_online, 0644);
> @@ -528,6 +558,9 @@ MODULE_PARM_DESC(battery_capacity, "battery capacity (percentage)");
>  module_param(battery_voltage, battery_voltage, 0644);
>  MODULE_PARM_DESC(battery_voltage, "battery voltage (millivolts)");
>  
> +module_param(battery_current, battery_current, 0644);
> +MODULE_PARM_DESC(battery_current, "battery current (milliampere)");
> +
>  MODULE_DESCRIPTION("Power supply driver for testing");
>  MODULE_AUTHOR("Anton Vorontsov <cbouatmailru@xxxxxxxxx>");
>  MODULE_LICENSE("GPL");
> -- 
> 2.18.0
>

Attachment: signature.asc
Description: PGP signature