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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
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;
