Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <30e7417e-43f3-5858-6663-9280f78e3af1@redhat.com>
Date:   Fri, 18 Mar 2022 09:59:40 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion
 macros
Content-Language: en-US
To:     Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Sebastian Reichel <sre@kernel.org>, linux-pm@vger.kernel.org
Cc:     Purism Kernel Team <kernel@puri.sm>, Rob Herring <robh@kernel.org>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
References: <20220318001048.20922-1-sebastian.krzyszkowiak@puri.sm>
 <20220318001048.20922-2-sebastian.krzyszkowiak@puri.sm>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20220318001048.20922-2-sebastian.krzyszkowiak@puri.sm>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 3/18/22 01:10, Sebastian Krzyszkowiak wrote:
> Instead of sprinkling the code with magic numbers, put the unit
> definitions used by the gauge into a set of macros. Macros are
> used instead of simple defines in order to not require floating
> point operations for divisions.
> 
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index ab031bbfbe78..c019d6c52363 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -51,6 +51,15 @@
>  
>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>  
> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
> +#define MAX17042_CURRENT_RSENSE(x)	(x * MAX17042_CURRENT_LSB) /* µV */
> +#define MAX17042_CAPACITY_LSB		5000000ll /* µVh */
> +#define MAX17042_CAPACITY_RSENSE(x)	(x * MAX17042_CAPACITY_LSB) /* µVh */
> +#define MAX17042_TIME(x)		(x * 5625 / 1000) /* s */
> +#define MAX17042_VOLTAGE(x)		(x * 625 / 8) /* µV */
> +#define MAX17042_RESISTANCE(x)		(x / 4096) /* Ω */
> +#define MAX17042_TEMPERATURE(x)		(x / 256) /* °C */
> +
>  struct max17042_chip {
>  	struct i2c_client *client;
>  	struct regmap *regmap;
> @@ -103,8 +112,7 @@ static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
>  
>  	*temp = sign_extend32(data, 15);
>  	/* The value is converted into deci-centigrade scale */
> -	/* Units of LSB = 1 / 256 degree Celsius */
> -	*temp = *temp * 10 / 256;
> +	*temp = MAX17042_TEMPERATURE(*temp * 10);

Shouldn't the "* 10"  be part of the macro?

Otherwise this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


>  	return 0;
>  }
>  
> @@ -161,7 +169,7 @@ static int max17042_get_status(struct max17042_chip *chip, int *status)
>  		return ret;
>  
>  	avg_current = sign_extend32(data, 15);
> -	avg_current *= 1562500 / chip->pdata->r_sns;
> +	avg_current *= MAX17042_CURRENT_LSB / chip->pdata->r_sns;
>  
>  	if (avg_current > 0)
>  		*status = POWER_SUPPLY_STATUS_CHARGING;
> @@ -181,7 +189,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
>  		goto health_error;
>  
>  	/* bits [0-3] unused */
> -	vavg = val * 625 / 8;
> +	vavg = MAX17042_VOLTAGE(val);
>  	/* Convert to millivolts */
>  	vavg /= 1000;
>  
> @@ -190,7 +198,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
>  		goto health_error;
>  
>  	/* bits [0-3] unused */
> -	vbatt = val * 625 / 8;
> +	vbatt = MAX17042_VOLTAGE(val);
>  	/* Convert to millivolts */
>  	vbatt /= 1000;
>  
> @@ -297,21 +305,21 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = data * 625 / 8;
> +		val->intval = MAX17042_VOLTAGE(data);
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
>  		ret = regmap_read(map, MAX17042_AvgVCELL, &data);
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = data * 625 / 8;
> +		val->intval = MAX17042_VOLTAGE(data);
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
>  		ret = regmap_read(map, MAX17042_OCVInternal, &data);
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = data * 625 / 8;
> +		val->intval = MAX17042_VOLTAGE(data);
>  		break;
>  	case POWER_SUPPLY_PROP_CAPACITY:
>  		if (chip->pdata->enable_current_sense)
> @@ -328,7 +336,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = data * 5000000ll;
> +		data64 = MAX17042_CAPACITY_RSENSE(data);
>  		do_div(data64, chip->pdata->r_sns);
>  		val->intval = data64;
>  		break;
> @@ -337,7 +345,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = data * 5000000ll;
> +		data64 = MAX17042_CAPACITY_RSENSE(data);
>  		do_div(data64, chip->pdata->r_sns);
>  		val->intval = data64;
>  		break;
> @@ -346,7 +354,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = data * 5000000ll;
> +		data64 = MAX17042_CAPACITY_RSENSE(data);
>  		do_div(data64, chip->pdata->r_sns);
>  		val->intval = data64;
>  		break;
> @@ -355,7 +363,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = sign_extend64(data, 15) * 5000000ll;
> +		data64 = MAX17042_CAPACITY_RSENSE(sign_extend64(data, 15));
>  		val->intval = div_s64(data64, chip->pdata->r_sns);
>  		break;
>  	case POWER_SUPPLY_PROP_TEMP:
> @@ -397,7 +405,7 @@ static int max17042_get_property(struct power_supply *psy,
>  			if (ret < 0)
>  				return ret;
>  
> -			data64 = sign_extend64(data, 15) * 1562500ll;
> +			data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
>  			val->intval = div_s64(data64, chip->pdata->r_sns);
>  		} else {
>  			return -EINVAL;
> @@ -409,7 +417,7 @@ static int max17042_get_property(struct power_supply *psy,
>  			if (ret < 0)
>  				return ret;
>  
> -			data64 = sign_extend64(data, 15) * 1562500ll;
> +			data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
>  			val->intval = div_s64(data64, chip->pdata->r_sns);
>  		} else {
>  			return -EINVAL;
> @@ -420,7 +428,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		data64 = data * 1562500ll;
> +		data64 = MAX17042_CURRENT_RSENSE(data);
>  		val->intval = div_s64(data64, chip->pdata->r_sns);
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
> @@ -428,7 +436,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = data * 5625 / 1000;
> +		val->intval = MAX17042_TIME(data);
>  		break;
>  	default:
>  		return -EINVAL;