[PATCH 1/3] hwmon: (ltc2945): wrap regmap into an ltc2945_state struct

[PATCH 1/3] hwmon: (ltc2945): wrap regmap into an ltc2945_state struct

[Alexandru Ardelean]

The intent is to add pass the value of the sense resistor in the driver.
This change wraps a 'struct ltc2945_state', and moves the regmap reference
on that object.

Then we can add the value of the sense resistor, or other information that
would be useful for the driver.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/hwmon/ltc2945.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
index ba9c868a8641..1cea710df668 100644
--- a/drivers/hwmon/ltc2945.c
+++ b/drivers/hwmon/ltc2945.c
@@ -58,6 +58,14 @@
 #define CONTROL_MULT_SELECT	(1 << 0)
 #define CONTROL_TEST_MODE	(1 << 4)
 
+/**
+ * struct ltc2945_state - driver instance specific data
+ * @regmap		regmap object to access device registers
+ */
+struct ltc2945_state {
+	struct regmap		*regmap;
+};
+
 static inline bool is_power_reg(u8 reg)
 {
 	return reg < LTC2945_SENSE_H;
@@ -66,7 +74,8 @@ static inline bool is_power_reg(u8 reg)
 /* Return the value from the given register in uW, mV, or mA */
 static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
 {
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct ltc2945_state *st = dev_get_drvdata(dev);
+	struct regmap *regmap = st->regmap;
 	unsigned int control;
 	u8 buf[3];
 	long long val;
@@ -148,7 +157,8 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
 static int ltc2945_val_to_reg(struct device *dev, u8 reg,
 			      unsigned long val)
 {
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct ltc2945_state *st = dev_get_drvdata(dev);
+	struct regmap *regmap = st->regmap;
 	unsigned int control;
 	int ret;
 
@@ -234,7 +244,8 @@ static ssize_t ltc2945_value_store(struct device *dev,
 				   const char *buf, size_t count)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct ltc2945_state *st = dev_get_drvdata(dev);
+	struct regmap *regmap = st->regmap;
 	u8 reg = attr->index;
 	unsigned long val;
 	u8 regbuf[3];
@@ -269,7 +280,8 @@ static ssize_t ltc2945_history_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct ltc2945_state *st = dev_get_drvdata(dev);
+	struct regmap *regmap = st->regmap;
 	u8 reg = attr->index;
 	int num_regs = is_power_reg(reg) ? 3 : 2;
 	u8 buf_min[3] = { 0xff, 0xff, 0xff };
@@ -321,7 +333,8 @@ static ssize_t ltc2945_bool_show(struct device *dev,
 				 struct device_attribute *da, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct ltc2945_state *st = dev_get_drvdata(dev);
+	struct regmap *regmap = st->regmap;
 	unsigned int fault;
 	int ret;
 
@@ -448,15 +461,22 @@ static const struct regmap_config ltc2945_regmap_config = {
 static int ltc2945_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
+	struct ltc2945_state *st;
 	struct device *hwmon_dev;
 	struct regmap *regmap;
 
+	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
 	regmap = devm_regmap_init_i2c(client, &ltc2945_regmap_config);
 	if (IS_ERR(regmap)) {
 		dev_err(dev, "failed to allocate register map\n");
 		return PTR_ERR(regmap);
 	}
 
+	st->regmap = regmap;
+
 	/* Clear faults */
 	regmap_write(regmap, LTC2945_FAULT, 0x00);
 
-- 
2.27.0

[PATCH 2/3] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()

[Alexandru Ardelean]

In order to account for any potential overflows that could occur.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/hwmon/ltc2945.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
index 1cea710df668..75d997d31e01 100644
--- a/drivers/hwmon/ltc2945.c
+++ b/drivers/hwmon/ltc2945.c
@@ -155,7 +155,7 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
 }
 
 static int ltc2945_val_to_reg(struct device *dev, u8 reg,
-			      unsigned long val)
+			      unsigned long long val)
 {
 	struct ltc2945_state *st = dev_get_drvdata(dev);
 	struct regmap *regmap = st->regmap;
@@ -181,14 +181,14 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
 			return ret;
 		if (control & CONTROL_MULT_SELECT) {
 			/* 25 mV * 25 uV = 0.625 uV resolution. */
-			val = DIV_ROUND_CLOSEST(val, 625);
+			val = DIV_ROUND_CLOSEST_ULL(val, 625);
 		} else {
 			/*
 			 * 0.5 mV * 25 uV = 0.0125 uV resolution.
 			 * Divide first to avoid overflow;
 			 * accept loss of accuracy.
 			 */
-			val = DIV_ROUND_CLOSEST(val, 25) * 2;
+			val = DIV_ROUND_CLOSEST_ULL(val, 25) * 2;
 		}
 		break;
 	case LTC2945_VIN_H:
@@ -197,7 +197,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
 	case LTC2945_MAX_VIN_THRES_H:
 	case LTC2945_MIN_VIN_THRES_H:
 		/* 25 mV resolution. */
-		val /= 25;
+		val = div_u64(val, 25);
 		break;
 	case LTC2945_ADIN_H:
 	case LTC2945_MAX_ADIN_H:
@@ -219,7 +219,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
 		 * dividing the reported current by the sense resistor value
 		 * in mOhm.
 		 */
-		val = DIV_ROUND_CLOSEST(val, 25);
+		val = DIV_ROUND_CLOSEST_ULL(val, 25);
 		break;
 	default:
 		return -EINVAL;
@@ -247,13 +247,13 @@ static ssize_t ltc2945_value_store(struct device *dev,
 	struct ltc2945_state *st = dev_get_drvdata(dev);
 	struct regmap *regmap = st->regmap;
 	u8 reg = attr->index;
-	unsigned long val;
+	unsigned long long val;
 	u8 regbuf[3];
 	int num_regs;
 	int regval;
 	int ret;
 
-	ret = kstrtoul(buf, 10, &val);
+	ret = kstrtoull(buf, 10, &val);
 	if (ret)
 		return ret;
 
-- 
2.27.0

[PATCH 3/3] hwmon: (ltc2945): add support for sense resistor

[Alexandru Ardelean]

The sense resistor is a parameter of the board. It should be configured in
the driver via a device-tree / ACPI property, so that the proper current
measurements can be done in the driver.

It shouldn't be necessary that userspace need to know about the value of
the resistor. It makes things a bit harder to make the application code
portable from one board to another.

This change implements support for the sense resistor to be configured from
DT/ACPI and used in current calculations.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/hwmon/ltc2945.c | 48 ++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
index 75d997d31e01..500401a82c49 100644
--- a/drivers/hwmon/ltc2945.c
+++ b/drivers/hwmon/ltc2945.c
@@ -61,9 +61,11 @@
 /**
  * struct ltc2945_state - driver instance specific data
  * @regmap		regmap object to access device registers
+ * @r_sense_uohm	current sense resistor value
  */
 struct ltc2945_state {
 	struct regmap		*regmap;
+	u32			r_sense_uohm;
 };
 
 static inline bool is_power_reg(u8 reg)
@@ -101,9 +103,8 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
 	case LTC2945_MAX_POWER_THRES_H:
 	case LTC2945_MIN_POWER_THRES_H:
 		/*
-		 * Convert to uW by assuming current is measured with
-		 * an 1mOhm sense resistor, similar to current
-		 * measurements.
+		 * Convert to uW by and scale it with the configured
+		 * sense resistor, similar to current measurements.
 		 * Control register bit 0 selects if voltage at SENSE+/VDD
 		 * or voltage at ADIN is used to measure power.
 		 */
@@ -112,10 +113,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
 			return ret;
 		if (control & CONTROL_MULT_SELECT) {
 			/* 25 mV * 25 uV = 0.625 uV resolution. */
-			val *= 625LL;
+			val = DIV_ROUND_CLOSEST_ULL(val * 625LL * 1000, st->r_sense_uohm);
 		} else {
 			/* 0.5 mV * 25 uV = 0.0125 uV resolution. */
-			val = (val * 25LL) >> 1;
+			val = DIV_ROUND_CLOSEST_ULL(val * 25LL * 1000, st->r_sense_uohm) >> 1;
 		}
 		break;
 	case LTC2945_VIN_H:
@@ -140,13 +141,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
 	case LTC2945_MAX_SENSE_THRES_H:
 	case LTC2945_MIN_SENSE_THRES_H:
 		/*
-		 * 25 uV resolution. Convert to current as measured with
-		 * an 1 mOhm sense resistor, in mA. If a different sense
-		 * resistor is installed, calculate the actual current by
-		 * dividing the reported current by the sense resistor value
-		 * in mOhm.
+		 * 25 uV resolution. Convert to current and scale it
+		 * with the value of the sense resistor.
 		 */
-		val *= 25;
+		val = DIV_ROUND_CLOSEST_ULL(val * 25 * 1000, st->r_sense_uohm);
 		break;
 	default:
 		return -EINVAL;
@@ -169,9 +167,8 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
 	case LTC2945_MAX_POWER_THRES_H:
 	case LTC2945_MIN_POWER_THRES_H:
 		/*
-		 * Convert to register value by assuming current is measured
-		 * with an 1mOhm sense resistor, similar to current
-		 * measurements.
+		 * Convert to register value, scale it with the configured sense
+		 * resistor value, similar to current measurements.
 		 * Control register bit 0 selects if voltage at SENSE+/VDD
 		 * or voltage at ADIN is used to measure power, which in turn
 		 * determines register calculations.
@@ -181,14 +178,10 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
 			return ret;
 		if (control & CONTROL_MULT_SELECT) {
 			/* 25 mV * 25 uV = 0.625 uV resolution. */
-			val = DIV_ROUND_CLOSEST_ULL(val, 625);
+			val = DIV_ROUND_CLOSEST_ULL(val * 1000, 625 * st->r_sense_uohm);
 		} else {
-			/*
-			 * 0.5 mV * 25 uV = 0.0125 uV resolution.
-			 * Divide first to avoid overflow;
-			 * accept loss of accuracy.
-			 */
-			val = DIV_ROUND_CLOSEST_ULL(val, 25) * 2;
+			/* 0.5 mV * 25 uV = 0.0125 uV resolution. */
+			val = DIV_ROUND_CLOSEST_ULL(val * 2 * 1000, 25 * st->r_sense_uohm);
 		}
 		break;
 	case LTC2945_VIN_H:
@@ -213,13 +206,10 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
 	case LTC2945_MAX_SENSE_THRES_H:
 	case LTC2945_MIN_SENSE_THRES_H:
 		/*
-		 * 25 uV resolution. Convert to current as measured with
-		 * an 1 mOhm sense resistor, in mA. If a different sense
-		 * resistor is installed, calculate the actual current by
-		 * dividing the reported current by the sense resistor value
-		 * in mOhm.
+		 * 25 uV resolution. Convert to current and scale it
+		 * with the value of the sense resistor, in mA.
 		 */
-		val = DIV_ROUND_CLOSEST_ULL(val, 25);
+		val = DIV_ROUND_CLOSEST_ULL(val * 1000, 25 * st->r_sense_uohm);
 		break;
 	default:
 		return -EINVAL;
@@ -475,6 +465,10 @@ static int ltc2945_probe(struct i2c_client *client)
 		return PTR_ERR(regmap);
 	}
 
+	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
+				     &st->r_sense_uohm))
+		st->r_sense_uohm = 1000;
+
 	st->regmap = regmap;
 
 	/* Clear faults */
-- 
2.27.0

Re: [PATCH 2/3] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()

[Guenter Roeck]

On Fri, Nov 06, 2020 at 12:18:24PM +0200, Alexandru Ardelean wrote:
> In order to account for any potential overflows that could occur.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/hwmon/ltc2945.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> index 1cea710df668..75d997d31e01 100644
> --- a/drivers/hwmon/ltc2945.c
> +++ b/drivers/hwmon/ltc2945.c
> @@ -155,7 +155,7 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
>  }
>  
>  static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> -			      unsigned long val)
> +			      unsigned long long val)
>  {
>  	struct ltc2945_state *st = dev_get_drvdata(dev);
>  	struct regmap *regmap = st->regmap;
> @@ -181,14 +181,14 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>  			return ret;
>  		if (control & CONTROL_MULT_SELECT) {
>  			/* 25 mV * 25 uV = 0.625 uV resolution. */
> -			val = DIV_ROUND_CLOSEST(val, 625);
> +			val = DIV_ROUND_CLOSEST_ULL(val, 625);
>  		} else {
>  			/*
>  			 * 0.5 mV * 25 uV = 0.0125 uV resolution.
>  			 * Divide first to avoid overflow;
>  			 * accept loss of accuracy.
>  			 */
> -			val = DIV_ROUND_CLOSEST(val, 25) * 2;
> +			val = DIV_ROUND_CLOSEST_ULL(val, 25) * 2;
>  		}
>  		break;
>  	case LTC2945_VIN_H:
> @@ -197,7 +197,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>  	case LTC2945_MAX_VIN_THRES_H:
>  	case LTC2945_MIN_VIN_THRES_H:
>  		/* 25 mV resolution. */
> -		val /= 25;
> +		val = div_u64(val, 25);
>  		break;
>  	case LTC2945_ADIN_H:
>  	case LTC2945_MAX_ADIN_H:
> @@ -219,7 +219,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>  		 * dividing the reported current by the sense resistor value
>  		 * in mOhm.
>  		 */
> -		val = DIV_ROUND_CLOSEST(val, 25);
> +		val = DIV_ROUND_CLOSEST_ULL(val, 25);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -247,13 +247,13 @@ static ssize_t ltc2945_value_store(struct device *dev,
>  	struct ltc2945_state *st = dev_get_drvdata(dev);
>  	struct regmap *regmap = st->regmap;
>  	u8 reg = attr->index;
> -	unsigned long val;
> +	unsigned long long val;
>  	u8 regbuf[3];
>  	int num_regs;
>  	int regval;
>  	int ret;
>  
> -	ret = kstrtoul(buf, 10, &val);
> +	ret = kstrtoull(buf, 10, &val);

This part of the change is unnecessary. Just keep 'val' as-is.
ltc2945_val_to_reg() can still accept ull.

Guenter

>  	if (ret)
>  		return ret;
>  
> -- 
> 2.27.0
> 

Re: [PATCH 3/3] hwmon: (ltc2945): add support for sense resistor

[Guenter Roeck]

On Fri, Nov 06, 2020 at 12:18:25PM +0200, Alexandru Ardelean wrote:
> The sense resistor is a parameter of the board. It should be configured in
> the driver via a device-tree / ACPI property, so that the proper current
> measurements can be done in the driver.
> 
> It shouldn't be necessary that userspace need to know about the value of
> the resistor. It makes things a bit harder to make the application code
> portable from one board to another.
> 
> This change implements support for the sense resistor to be configured from
> DT/ACPI and used in current calculations.
> 

This will require a matching deevicetree document.

> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/hwmon/ltc2945.c | 48 ++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> index 75d997d31e01..500401a82c49 100644
> --- a/drivers/hwmon/ltc2945.c
> +++ b/drivers/hwmon/ltc2945.c
> @@ -61,9 +61,11 @@
>  /**
>   * struct ltc2945_state - driver instance specific data
>   * @regmap		regmap object to access device registers
> + * @r_sense_uohm	current sense resistor value
>   */
>  struct ltc2945_state {
>  	struct regmap		*regmap;
> +	u32			r_sense_uohm;
>  };
>  
>  static inline bool is_power_reg(u8 reg)
> @@ -101,9 +103,8 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
>  	case LTC2945_MAX_POWER_THRES_H:
>  	case LTC2945_MIN_POWER_THRES_H:
>  		/*
> -		 * Convert to uW by assuming current is measured with
> -		 * an 1mOhm sense resistor, similar to current
> -		 * measurements.
> +		 * Convert to uW by and scale it with the configured
> +		 * sense resistor, similar to current measurements.
>  		 * Control register bit 0 selects if voltage at SENSE+/VDD
>  		 * or voltage at ADIN is used to measure power.
>  		 */
> @@ -112,10 +113,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
>  			return ret;
>  		if (control & CONTROL_MULT_SELECT) {
>  			/* 25 mV * 25 uV = 0.625 uV resolution. */
> -			val *= 625LL;
> +			val = DIV_ROUND_CLOSEST_ULL(val * 625LL * 1000, st->r_sense_uohm);
>  		} else {
>  			/* 0.5 mV * 25 uV = 0.0125 uV resolution. */
> -			val = (val * 25LL) >> 1;
> +			val = DIV_ROUND_CLOSEST_ULL(val * 25LL * 1000, st->r_sense_uohm) >> 1;
>  		}
>  		break;
>  	case LTC2945_VIN_H:
> @@ -140,13 +141,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
>  	case LTC2945_MAX_SENSE_THRES_H:
>  	case LTC2945_MIN_SENSE_THRES_H:
>  		/*
> -		 * 25 uV resolution. Convert to current as measured with
> -		 * an 1 mOhm sense resistor, in mA. If a different sense
> -		 * resistor is installed, calculate the actual current by
> -		 * dividing the reported current by the sense resistor value
> -		 * in mOhm.
> +		 * 25 uV resolution. Convert to current and scale it
> +		 * with the value of the sense resistor.
>  		 */
> -		val *= 25;
> +		val = DIV_ROUND_CLOSEST_ULL(val * 25 * 1000, st->r_sense_uohm);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -169,9 +167,8 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>  	case LTC2945_MAX_POWER_THRES_H:
>  	case LTC2945_MIN_POWER_THRES_H:
>  		/*
> -		 * Convert to register value by assuming current is measured
> -		 * with an 1mOhm sense resistor, similar to current
> -		 * measurements.
> +		 * Convert to register value, scale it with the configured sense
> +		 * resistor value, similar to current measurements.
>  		 * Control register bit 0 selects if voltage at SENSE+/VDD
>  		 * or voltage at ADIN is used to measure power, which in turn
>  		 * determines register calculations.
> @@ -181,14 +178,10 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>  			return ret;
>  		if (control & CONTROL_MULT_SELECT) {
>  			/* 25 mV * 25 uV = 0.625 uV resolution. */
> -			val = DIV_ROUND_CLOSEST_ULL(val, 625);
> +			val = DIV_ROUND_CLOSEST_ULL(val * 1000, 625 * st->r_sense_uohm);
>  		} else {
> -			/*
> -			 * 0.5 mV * 25 uV = 0.0125 uV resolution.
> -			 * Divide first to avoid overflow;
> -			 * accept loss of accuracy.
> -			 */
> -			val = DIV_ROUND_CLOSEST_ULL(val, 25) * 2;
> +			/* 0.5 mV * 25 uV = 0.0125 uV resolution. */
> +			val = DIV_ROUND_CLOSEST_ULL(val * 2 * 1000, 25 * st->r_sense_uohm);
>  		}
>  		break;
>  	case LTC2945_VIN_H:
> @@ -213,13 +206,10 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>  	case LTC2945_MAX_SENSE_THRES_H:
>  	case LTC2945_MIN_SENSE_THRES_H:
>  		/*
> -		 * 25 uV resolution. Convert to current as measured with
> -		 * an 1 mOhm sense resistor, in mA. If a different sense
> -		 * resistor is installed, calculate the actual current by
> -		 * dividing the reported current by the sense resistor value
> -		 * in mOhm.
> +		 * 25 uV resolution. Convert to current and scale it
> +		 * with the value of the sense resistor, in mA.
>  		 */
> -		val = DIV_ROUND_CLOSEST_ULL(val, 25);
> +		val = DIV_ROUND_CLOSEST_ULL(val * 1000, 25 * st->r_sense_uohm);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -475,6 +465,10 @@ static int ltc2945_probe(struct i2c_client *client)
>  		return PTR_ERR(regmap);
>  	}
>  
> +	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> +				     &st->r_sense_uohm))
> +		st->r_sense_uohm = 1000;
> +

Devicetree could set shunt-resistor-micro-ohms to 0, which would result
in divide by 0 errors.

Guenter

>  	st->regmap = regmap;
>  
>  	/* Clear faults */
> -- 
> 2.27.0
> 

Re: [PATCH 2/3] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()

[Alexandru Ardelean]

On Fri, Nov 6, 2020 at 3:14 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Nov 06, 2020 at 12:18:24PM +0200, Alexandru Ardelean wrote:
> > In order to account for any potential overflows that could occur.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/hwmon/ltc2945.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> > index 1cea710df668..75d997d31e01 100644
> > --- a/drivers/hwmon/ltc2945.c
> > +++ b/drivers/hwmon/ltc2945.c
> > @@ -155,7 +155,7 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
> >  }
> >
> >  static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> > -                           unsigned long val)
> > +                           unsigned long long val)
> >  {
> >       struct ltc2945_state *st = dev_get_drvdata(dev);
> >       struct regmap *regmap = st->regmap;
> > @@ -181,14 +181,14 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >                       return ret;
> >               if (control & CONTROL_MULT_SELECT) {
> >                       /* 25 mV * 25 uV = 0.625 uV resolution. */
> > -                     val = DIV_ROUND_CLOSEST(val, 625);
> > +                     val = DIV_ROUND_CLOSEST_ULL(val, 625);
> >               } else {
> >                       /*
> >                        * 0.5 mV * 25 uV = 0.0125 uV resolution.
> >                        * Divide first to avoid overflow;
> >                        * accept loss of accuracy.
> >                        */
> > -                     val = DIV_ROUND_CLOSEST(val, 25) * 2;
> > +                     val = DIV_ROUND_CLOSEST_ULL(val, 25) * 2;
> >               }
> >               break;
> >       case LTC2945_VIN_H:
> > @@ -197,7 +197,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >       case LTC2945_MAX_VIN_THRES_H:
> >       case LTC2945_MIN_VIN_THRES_H:
> >               /* 25 mV resolution. */
> > -             val /= 25;
> > +             val = div_u64(val, 25);
> >               break;
> >       case LTC2945_ADIN_H:
> >       case LTC2945_MAX_ADIN_H:
> > @@ -219,7 +219,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >                * dividing the reported current by the sense resistor value
> >                * in mOhm.
> >                */
> > -             val = DIV_ROUND_CLOSEST(val, 25);
> > +             val = DIV_ROUND_CLOSEST_ULL(val, 25);
> >               break;
> >       default:
> >               return -EINVAL;
> > @@ -247,13 +247,13 @@ static ssize_t ltc2945_value_store(struct device *dev,
> >       struct ltc2945_state *st = dev_get_drvdata(dev);
> >       struct regmap *regmap = st->regmap;
> >       u8 reg = attr->index;
> > -     unsigned long val;
> > +     unsigned long long val;
> >       u8 regbuf[3];
> >       int num_regs;
> >       int regval;
> >       int ret;
> >
> > -     ret = kstrtoul(buf, 10, &val);
> > +     ret = kstrtoull(buf, 10, &val);
>
> This part of the change is unnecessary. Just keep 'val' as-is.
> ltc2945_val_to_reg() can still accept ull.

Ack

>
> Guenter
>
> >       if (ret)
> >               return ret;
> >
> > --
> > 2.27.0
> >

Re: [PATCH 3/3] hwmon: (ltc2945): add support for sense resistor

[Alexandru Ardelean]

On Fri, Nov 6, 2020 at 3:17 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Nov 06, 2020 at 12:18:25PM +0200, Alexandru Ardelean wrote:
> > The sense resistor is a parameter of the board. It should be configured in
> > the driver via a device-tree / ACPI property, so that the proper current
> > measurements can be done in the driver.
> >
> > It shouldn't be necessary that userspace need to know about the value of
> > the resistor. It makes things a bit harder to make the application code
> > portable from one board to another.
> >
> > This change implements support for the sense resistor to be configured from
> > DT/ACPI and used in current calculations.
> >
>
> This will require a matching deevicetree document.

Ack
Will create a dt binding schema doc.
Are you fine with being added as maintainer in the DT doc?
Seeing as you are the original author of the driver.

>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/hwmon/ltc2945.c | 48 ++++++++++++++++++-----------------------
> >  1 file changed, 21 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> > index 75d997d31e01..500401a82c49 100644
> > --- a/drivers/hwmon/ltc2945.c
> > +++ b/drivers/hwmon/ltc2945.c
> > @@ -61,9 +61,11 @@
> >  /**
> >   * struct ltc2945_state - driver instance specific data
> >   * @regmap           regmap object to access device registers
> > + * @r_sense_uohm     current sense resistor value
> >   */
> >  struct ltc2945_state {
> >       struct regmap           *regmap;
> > +     u32                     r_sense_uohm;
> >  };
> >
> >  static inline bool is_power_reg(u8 reg)
> > @@ -101,9 +103,8 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
> >       case LTC2945_MAX_POWER_THRES_H:
> >       case LTC2945_MIN_POWER_THRES_H:
> >               /*
> > -              * Convert to uW by assuming current is measured with
> > -              * an 1mOhm sense resistor, similar to current
> > -              * measurements.
> > +              * Convert to uW by and scale it with the configured
> > +              * sense resistor, similar to current measurements.
> >                * Control register bit 0 selects if voltage at SENSE+/VDD
> >                * or voltage at ADIN is used to measure power.
> >                */
> > @@ -112,10 +113,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
> >                       return ret;
> >               if (control & CONTROL_MULT_SELECT) {
> >                       /* 25 mV * 25 uV = 0.625 uV resolution. */
> > -                     val *= 625LL;
> > +                     val = DIV_ROUND_CLOSEST_ULL(val * 625LL * 1000, st->r_sense_uohm);
> >               } else {
> >                       /* 0.5 mV * 25 uV = 0.0125 uV resolution. */
> > -                     val = (val * 25LL) >> 1;
> > +                     val = DIV_ROUND_CLOSEST_ULL(val * 25LL * 1000, st->r_sense_uohm) >> 1;
> >               }
> >               break;
> >       case LTC2945_VIN_H:
> > @@ -140,13 +141,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
> >       case LTC2945_MAX_SENSE_THRES_H:
> >       case LTC2945_MIN_SENSE_THRES_H:
> >               /*
> > -              * 25 uV resolution. Convert to current as measured with
> > -              * an 1 mOhm sense resistor, in mA. If a different sense
> > -              * resistor is installed, calculate the actual current by
> > -              * dividing the reported current by the sense resistor value
> > -              * in mOhm.
> > +              * 25 uV resolution. Convert to current and scale it
> > +              * with the value of the sense resistor.
> >                */
> > -             val *= 25;
> > +             val = DIV_ROUND_CLOSEST_ULL(val * 25 * 1000, st->r_sense_uohm);
> >               break;
> >       default:
> >               return -EINVAL;
> > @@ -169,9 +167,8 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >       case LTC2945_MAX_POWER_THRES_H:
> >       case LTC2945_MIN_POWER_THRES_H:
> >               /*
> > -              * Convert to register value by assuming current is measured
> > -              * with an 1mOhm sense resistor, similar to current
> > -              * measurements.
> > +              * Convert to register value, scale it with the configured sense
> > +              * resistor value, similar to current measurements.
> >                * Control register bit 0 selects if voltage at SENSE+/VDD
> >                * or voltage at ADIN is used to measure power, which in turn
> >                * determines register calculations.
> > @@ -181,14 +178,10 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >                       return ret;
> >               if (control & CONTROL_MULT_SELECT) {
> >                       /* 25 mV * 25 uV = 0.625 uV resolution. */
> > -                     val = DIV_ROUND_CLOSEST_ULL(val, 625);
> > +                     val = DIV_ROUND_CLOSEST_ULL(val * 1000, 625 * st->r_sense_uohm);
> >               } else {
> > -                     /*
> > -                      * 0.5 mV * 25 uV = 0.0125 uV resolution.
> > -                      * Divide first to avoid overflow;
> > -                      * accept loss of accuracy.
> > -                      */
> > -                     val = DIV_ROUND_CLOSEST_ULL(val, 25) * 2;
> > +                     /* 0.5 mV * 25 uV = 0.0125 uV resolution. */
> > +                     val = DIV_ROUND_CLOSEST_ULL(val * 2 * 1000, 25 * st->r_sense_uohm);
> >               }
> >               break;
> >       case LTC2945_VIN_H:
> > @@ -213,13 +206,10 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> >       case LTC2945_MAX_SENSE_THRES_H:
> >       case LTC2945_MIN_SENSE_THRES_H:
> >               /*
> > -              * 25 uV resolution. Convert to current as measured with
> > -              * an 1 mOhm sense resistor, in mA. If a different sense
> > -              * resistor is installed, calculate the actual current by
> > -              * dividing the reported current by the sense resistor value
> > -              * in mOhm.
> > +              * 25 uV resolution. Convert to current and scale it
> > +              * with the value of the sense resistor, in mA.
> >                */
> > -             val = DIV_ROUND_CLOSEST_ULL(val, 25);
> > +             val = DIV_ROUND_CLOSEST_ULL(val * 1000, 25 * st->r_sense_uohm);
> >               break;
> >       default:
> >               return -EINVAL;
> > @@ -475,6 +465,10 @@ static int ltc2945_probe(struct i2c_client *client)
> >               return PTR_ERR(regmap);
> >       }
> >
> > +     if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> > +                                  &st->r_sense_uohm))
> > +             st->r_sense_uohm = 1000;
> > +
>
> Devicetree could set shunt-resistor-micro-ohms to 0, which would result
> in divide by 0 errors.

Ack
Will do a check for this.

>
> Guenter
>
> >       st->regmap = regmap;
> >
> >       /* Clear faults */
> > --
> > 2.27.0
> >

Re: [PATCH 3/3] hwmon: (ltc2945): add support for sense resistor

[Guenter Roeck]

On 11/8/20 10:41 PM, Alexandru Ardelean wrote:
> On Fri, Nov 6, 2020 at 3:17 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Fri, Nov 06, 2020 at 12:18:25PM +0200, Alexandru Ardelean wrote:
>>> The sense resistor is a parameter of the board. It should be configured in
>>> the driver via a device-tree / ACPI property, so that the proper current
>>> measurements can be done in the driver.
>>>
>>> It shouldn't be necessary that userspace need to know about the value of
>>> the resistor. It makes things a bit harder to make the application code
>>> portable from one board to another.
>>>
>>> This change implements support for the sense resistor to be configured from
>>> DT/ACPI and used in current calculations.
>>>
>>
>> This will require a matching deevicetree document.
> 
> Ack
> Will create a dt binding schema doc.
> Are you fine with being added as maintainer in the DT doc?
> Seeing as you are the original author of the driver.
> 

Sure.

Guenter

>>
>>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>>> ---
>>>  drivers/hwmon/ltc2945.c | 48 ++++++++++++++++++-----------------------
>>>  1 file changed, 21 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
>>> index 75d997d31e01..500401a82c49 100644
>>> --- a/drivers/hwmon/ltc2945.c
>>> +++ b/drivers/hwmon/ltc2945.c
>>> @@ -61,9 +61,11 @@
>>>  /**
>>>   * struct ltc2945_state - driver instance specific data
>>>   * @regmap           regmap object to access device registers
>>> + * @r_sense_uohm     current sense resistor value
>>>   */
>>>  struct ltc2945_state {
>>>       struct regmap           *regmap;
>>> +     u32                     r_sense_uohm;
>>>  };
>>>
>>>  static inline bool is_power_reg(u8 reg)
>>> @@ -101,9 +103,8 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
>>>       case LTC2945_MAX_POWER_THRES_H:
>>>       case LTC2945_MIN_POWER_THRES_H:
>>>               /*
>>> -              * Convert to uW by assuming current is measured with
>>> -              * an 1mOhm sense resistor, similar to current
>>> -              * measurements.
>>> +              * Convert to uW by and scale it with the configured
>>> +              * sense resistor, similar to current measurements.
>>>                * Control register bit 0 selects if voltage at SENSE+/VDD
>>>                * or voltage at ADIN is used to measure power.
>>>                */
>>> @@ -112,10 +113,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
>>>                       return ret;
>>>               if (control & CONTROL_MULT_SELECT) {
>>>                       /* 25 mV * 25 uV = 0.625 uV resolution. */
>>> -                     val *= 625LL;
>>> +                     val = DIV_ROUND_CLOSEST_ULL(val * 625LL * 1000, st->r_sense_uohm);
>>>               } else {
>>>                       /* 0.5 mV * 25 uV = 0.0125 uV resolution. */
>>> -                     val = (val * 25LL) >> 1;
>>> +                     val = DIV_ROUND_CLOSEST_ULL(val * 25LL * 1000, st->r_sense_uohm) >> 1;
>>>               }
>>>               break;
>>>       case LTC2945_VIN_H:
>>> @@ -140,13 +141,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
>>>       case LTC2945_MAX_SENSE_THRES_H:
>>>       case LTC2945_MIN_SENSE_THRES_H:
>>>               /*
>>> -              * 25 uV resolution. Convert to current as measured with
>>> -              * an 1 mOhm sense resistor, in mA. If a different sense
>>> -              * resistor is installed, calculate the actual current by
>>> -              * dividing the reported current by the sense resistor value
>>> -              * in mOhm.
>>> +              * 25 uV resolution. Convert to current and scale it
>>> +              * with the value of the sense resistor.
>>>                */
>>> -             val *= 25;
>>> +             val = DIV_ROUND_CLOSEST_ULL(val * 25 * 1000, st->r_sense_uohm);
>>>               break;
>>>       default:
>>>               return -EINVAL;
>>> @@ -169,9 +167,8 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>>>       case LTC2945_MAX_POWER_THRES_H:
>>>       case LTC2945_MIN_POWER_THRES_H:
>>>               /*
>>> -              * Convert to register value by assuming current is measured
>>> -              * with an 1mOhm sense resistor, similar to current
>>> -              * measurements.
>>> +              * Convert to register value, scale it with the configured sense
>>> +              * resistor value, similar to current measurements.
>>>                * Control register bit 0 selects if voltage at SENSE+/VDD
>>>                * or voltage at ADIN is used to measure power, which in turn
>>>                * determines register calculations.
>>> @@ -181,14 +178,10 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>>>                       return ret;
>>>               if (control & CONTROL_MULT_SELECT) {
>>>                       /* 25 mV * 25 uV = 0.625 uV resolution. */
>>> -                     val = DIV_ROUND_CLOSEST_ULL(val, 625);
>>> +                     val = DIV_ROUND_CLOSEST_ULL(val * 1000, 625 * st->r_sense_uohm);
>>>               } else {
>>> -                     /*
>>> -                      * 0.5 mV * 25 uV = 0.0125 uV resolution.
>>> -                      * Divide first to avoid overflow;
>>> -                      * accept loss of accuracy.
>>> -                      */
>>> -                     val = DIV_ROUND_CLOSEST_ULL(val, 25) * 2;
>>> +                     /* 0.5 mV * 25 uV = 0.0125 uV resolution. */
>>> +                     val = DIV_ROUND_CLOSEST_ULL(val * 2 * 1000, 25 * st->r_sense_uohm);
>>>               }
>>>               break;
>>>       case LTC2945_VIN_H:
>>> @@ -213,13 +206,10 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
>>>       case LTC2945_MAX_SENSE_THRES_H:
>>>       case LTC2945_MIN_SENSE_THRES_H:
>>>               /*
>>> -              * 25 uV resolution. Convert to current as measured with
>>> -              * an 1 mOhm sense resistor, in mA. If a different sense
>>> -              * resistor is installed, calculate the actual current by
>>> -              * dividing the reported current by the sense resistor value
>>> -              * in mOhm.
>>> +              * 25 uV resolution. Convert to current and scale it
>>> +              * with the value of the sense resistor, in mA.
>>>                */
>>> -             val = DIV_ROUND_CLOSEST_ULL(val, 25);
>>> +             val = DIV_ROUND_CLOSEST_ULL(val * 1000, 25 * st->r_sense_uohm);
>>>               break;
>>>       default:
>>>               return -EINVAL;
>>> @@ -475,6 +465,10 @@ static int ltc2945_probe(struct i2c_client *client)
>>>               return PTR_ERR(regmap);
>>>       }
>>>
>>> +     if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
>>> +                                  &st->r_sense_uohm))
>>> +             st->r_sense_uohm = 1000;
>>> +
>>
>> Devicetree could set shunt-resistor-micro-ohms to 0, which would result
>> in divide by 0 errors.
> 
> Ack
> Will do a check for this.
> 
>>
>> Guenter
>>
>>>       st->regmap = regmap;
>>>
>>>       /* Clear faults */
>>> --
>>> 2.27.0
>>>