.../bindings/hwmon/adi,ltc2945.yaml | 50 +++++++++++ drivers/hwmon/ltc2945.c | 86 ++++++++++++++----- 2 files changed, 115 insertions(+), 21 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml
Added the ability to specify the value of the shunt resistor in the device tree instead of assuming it is 1 milliOhm. Would be good to backport as well Cormier, Jonathan (1): dt-bindings: hwmon: adi,ltc2945: Add binding John Pruitt (1): hwmon: ltc2945: Allow setting shunt resistor .../bindings/hwmon/adi,ltc2945.yaml | 50 +++++++++++ drivers/hwmon/ltc2945.c | 86 ++++++++++++++----- 2 files changed, 115 insertions(+), 21 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml -- 2.25.1
Added the ability to specify the value of the shunt resistor in the device tree instead of assuming it is 1 milliOhm. Would be good to backport as well Changes since v1: - Add devicetree match table - Add kerneldoc for the ltc2945_data struct - Cleanup excesive comments about the shunt resistor - Switch to device_property_read_u32() Cormier, Jonathan (1): dt-bindings: hwmon: adi,ltc2945: Add binding John Pruitt (1): hwmon: ltc2945: Allow setting shunt resistor .../bindings/hwmon/adi,ltc2945.yaml | 50 +++++++++++ drivers/hwmon/ltc2945.c | 86 ++++++++++++++----- 2 files changed, 115 insertions(+), 21 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml -- 2.25.1
Create initial binding for the LTC2945 I2C power monitor.
Also adds shunt-resistor-micro-ohms parameter
Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com>
---
.../bindings/hwmon/adi,ltc2945.yaml | 50 +++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml
new file mode 100644
index 000000000000..f90d40919ee6
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/adi,ltc2945.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices LTC2945 wide range i2c power monitor
+
+maintainers:
+ - Guenter Roeck <linux@roeck-us.net>
+
+description: |
+ Analog Devices LTC2945 wide range i2c power monitor over I2C.
+
+ https://www.analog.com/media/en/technical-documentation/data-sheets/LTC2945.pdf
+
+properties:
+ compatible:
+ enum:
+ - adi,ltc2945
+
+ reg:
+ maxItems: 1
+
+ shunt-resistor-micro-ohms:
+ description:
+ Shunt resistor value in micro-Ohms
+ default: 1000
+
+required:
+ - compatible
+ - reg
+
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ power-monitor@6e {
+ compatible = "adi,ltc2945";
+ reg = <0x6e>;
+ /* 10 milli-Ohm shunt resistor */
+ shunt-resistor-micro-ohms = <10000>;
+ };
+ };
+...
--
2.25.1On 20/12/2022 01:04, Cormier, Jonathan wrote: > Create initial binding for the LTC2945 I2C power monitor. > Also adds shunt-resistor-micro-ohms parameter The last sentence does not make sense. I propose to skip it. > > Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com> > --- > .../bindings/hwmon/adi,ltc2945.yaml | 50 +++++++++++++++++++ > 1 file changed, 50 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > new file mode 100644 > index 000000000000..f90d40919ee6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > @@ -0,0 +1,50 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/hwmon/adi,ltc2945.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices LTC2945 wide range i2c power monitor > + > +maintainers: > + - Guenter Roeck <linux@roeck-us.net> Maintainer of binding is person interested in the device, e.g. having the hardware or datasheet. Not the subsystem maintainer. Unless by coincidence this is the same person here? > + > +description: | > + Analog Devices LTC2945 wide range i2c power monitor over I2C. > + > + https://www.analog.com/media/en/technical-documentation/data-sheets/LTC2945.pdf > + > +properties: > + compatible: > + enum: > + - adi,ltc2945 > + > + reg: > + maxItems: 1 > + > + shunt-resistor-micro-ohms: > + description: > + Shunt resistor value in micro-Ohms > + default: 1000 > + > +required: > + - compatible > + - reg > + > + This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. > +additionalProperties: false > + Best regards, Krzysztof
On Tue, Dec 20, 2022 at 5:15 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 20/12/2022 01:04, Cormier, Jonathan wrote: > > Create initial binding for the LTC2945 I2C power monitor. > > Also adds shunt-resistor-micro-ohms parameter > > The last sentence does not make sense. I propose to skip it. Ok > > > > > Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com> > > --- > > .../bindings/hwmon/adi,ltc2945.yaml | 50 +++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > new file mode 100644 > > index 000000000000..f90d40919ee6 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > @@ -0,0 +1,50 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/hwmon/adi,ltc2945.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices LTC2945 wide range i2c power monitor > > + > > +maintainers: > > + - Guenter Roeck <linux@roeck-us.net> > > Maintainer of binding is person interested in the device, e.g. having > the hardware or datasheet. Not the subsystem maintainer. Unless by > coincidence this is the same person here? What do you do with a basic kernel driver which hasn't been touched since it was introduced except for various refactors? He seems to be the one who introduced it and most consistently made changes to it. > > > > + > > +description: | > > + Analog Devices LTC2945 wide range i2c power monitor over I2C. > > + > > + https://www.analog.com/media/en/technical-documentation/data-sheets/LTC2945.pdf > > + > > +properties: > > + compatible: > > + enum: > > + - adi,ltc2945 > > + > > + reg: > > + maxItems: 1 > > + > > + shunt-resistor-micro-ohms: > > + description: > > + Shunt resistor value in micro-Ohms > > + default: 1000 > > + > > +required: > > + - compatible > > + - reg > > + > > + > This is a friendly reminder during the review process. > > It seems my previous comments were not fully addressed. Maybe my > feedback got lost between the quotes, maybe you just forgot to apply it. > Please go back to the previous discussion and either implement all > requested changes or keep discussing them. My bad, > > Thank you. > > > +additionalProperties: false > > + > > Best regards, > Krzysztof > -- Jonathan Cormier Software Engineer Voice: 315.425.4045 x222 http://www.CriticalLink.com 6712 Brooklawn Parkway, Syracuse, NY 13211
On Tue, Dec 20, 2022 at 09:35:38AM -0500, Jon Cormier wrote: > On Tue, Dec 20, 2022 at 5:15 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 20/12/2022 01:04, Cormier, Jonathan wrote: > > > Create initial binding for the LTC2945 I2C power monitor. > > > Also adds shunt-resistor-micro-ohms parameter > > > > The last sentence does not make sense. I propose to skip it. > Ok > > > > > > > > Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com> > > > --- > > > .../bindings/hwmon/adi,ltc2945.yaml | 50 +++++++++++++++++++ > > > 1 file changed, 50 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > > new file mode 100644 > > > index 000000000000..f90d40919ee6 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > > @@ -0,0 +1,50 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/hwmon/adi,ltc2945.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Analog Devices LTC2945 wide range i2c power monitor > > > + > > > +maintainers: > > > + - Guenter Roeck <linux@roeck-us.net> > > > > Maintainer of binding is person interested in the device, e.g. having > > the hardware or datasheet. Not the subsystem maintainer. Unless by > > coincidence this is the same person here? > What do you do with a basic kernel driver which hasn't been touched > since it was introduced except for various refactors? He seems to be > the one who introduced it and most consistently made changes to it. FWIW, if I was not ok with being listed as maintainer I would have objected. Having said that, it is just as fine with me to list someone else. Guenter > > > > > > > + > > > +description: | > > > + Analog Devices LTC2945 wide range i2c power monitor over I2C. > > > + > > > + https://www.analog.com/media/en/technical-documentation/data-sheets/LTC2945.pdf > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - adi,ltc2945 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + shunt-resistor-micro-ohms: > > > + description: > > > + Shunt resistor value in micro-Ohms > > > + default: 1000 > > > + > > > +required: > > > + - compatible > > > + - reg > > > + > > > + > > This is a friendly reminder during the review process. > > > > It seems my previous comments were not fully addressed. Maybe my > > feedback got lost between the quotes, maybe you just forgot to apply it. > > Please go back to the previous discussion and either implement all > > requested changes or keep discussing them. > My bad, > > > > Thank you. > > > > > +additionalProperties: false > > > + > > > > Best regards, > > Krzysztof > > > > > -- > Jonathan Cormier > Software Engineer > > Voice: 315.425.4045 x222 > > > > http://www.CriticalLink.com > 6712 Brooklawn Parkway, Syracuse, NY 13211
Okay so for V3, I see only 2 changes. Please let me know if I've overlooked something else. - Remove "Also adds shunt-resistor-micro-ohms parameter" from commit description - Remove extra newline from binding doc Resending in plaintext... On Tue, Dec 20, 2022 at 9:46 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Tue, Dec 20, 2022 at 09:35:38AM -0500, Jon Cormier wrote: > > On Tue, Dec 20, 2022 at 5:15 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > On 20/12/2022 01:04, Cormier, Jonathan wrote: > > > > Create initial binding for the LTC2945 I2C power monitor. > > > > Also adds shunt-resistor-micro-ohms parameter > > > > > > The last sentence does not make sense. I propose to skip it. > > Ok > > > > > > > > > > > Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com> > > > > --- > > > > .../bindings/hwmon/adi,ltc2945.yaml | 50 +++++++++++++++++++ > > > > 1 file changed, 50 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > > > new file mode 100644 > > > > index 000000000000..f90d40919ee6 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml > > > > @@ -0,0 +1,50 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/hwmon/adi,ltc2945.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Analog Devices LTC2945 wide range i2c power monitor > > > > + > > > > +maintainers: > > > > + - Guenter Roeck <linux@roeck-us.net> > > > > > > Maintainer of binding is person interested in the device, e.g. having > > > the hardware or datasheet. Not the subsystem maintainer. Unless by > > > coincidence this is the same person here? > > What do you do with a basic kernel driver which hasn't been touched > > since it was introduced except for various refactors? He seems to be > > the one who introduced it and most consistently made changes to it. > > FWIW, if I was not ok with being listed as maintainer I would have objected. > > Having said that, it is just as fine with me to list someone else. > > Guenter > > > > > > > > > > > + > > > > +description: | > > > > + Analog Devices LTC2945 wide range i2c power monitor over I2C. > > > > + > > > > + https://www.analog.com/media/en/technical-documentation/data-sheets/LTC2945.pdf > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - adi,ltc2945 > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + shunt-resistor-micro-ohms: > > > > + description: > > > > + Shunt resistor value in micro-Ohms > > > > + default: 1000 > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + > > > > + > > > This is a friendly reminder during the review process. > > > > > > It seems my previous comments were not fully addressed. Maybe my > > > feedback got lost between the quotes, maybe you just forgot to apply it. > > > Please go back to the previous discussion and either implement all > > > requested changes or keep discussing them. > > My bad, > > > > > > Thank you. > > > > > > > +additionalProperties: false > > > > + > > > > > > Best regards, > > > Krzysztof > > > > > > > > > -- > > Jonathan Cormier > > Software Engineer > > > > Voice: 315.425.4045 x222 > > > > > > > > http://www.CriticalLink.com > > 6712 Brooklawn Parkway, Syracuse, NY 13211 -- Jonathan Cormier Software Engineer Voice: 315.425.4045 x222 http://www.CriticalLink.com 6712 Brooklawn Parkway, Syracuse, NY 13211
Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com>
---
drivers/hwmon/ltc2945.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
index 9adebb59f604..9af3e3821152 100644
--- a/drivers/hwmon/ltc2945.c
+++ b/drivers/hwmon/ltc2945.c
@@ -58,6 +58,12 @@
#define CONTROL_MULT_SELECT (1 << 0)
#define CONTROL_TEST_MODE (1 << 4)
+static const struct of_device_id __maybe_unused ltc2945_of_match[] = {
+ { .compatible = "adi,ltc2945" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ltc2945_of_match);
+
static inline bool is_power_reg(u8 reg)
{
return reg < LTC2945_SENSE_H;
@@ -475,8 +481,9 @@ MODULE_DEVICE_TABLE(i2c, ltc2945_id);
static struct i2c_driver ltc2945_driver = {
.driver = {
- .name = "ltc2945",
- },
+ .name = "ltc2945",
+ .of_match_table = of_match_ptr(ltc2945_of_match),
+ },
.probe_new = ltc2945_probe,
.id_table = ltc2945_id,
};
--
2.25.1On Mon, Dec 19, 2022 at 07:04:55PM -0500, Cormier, Jonathan wrote:
> Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com>
There should still be some description here.
> ---
> drivers/hwmon/ltc2945.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> index 9adebb59f604..9af3e3821152 100644
> --- a/drivers/hwmon/ltc2945.c
> +++ b/drivers/hwmon/ltc2945.c
> @@ -58,6 +58,12 @@
> #define CONTROL_MULT_SELECT (1 << 0)
> #define CONTROL_TEST_MODE (1 << 4)
>
> +static const struct of_device_id __maybe_unused ltc2945_of_match[] = {
> + { .compatible = "adi,ltc2945" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ltc2945_of_match);
> +
> static inline bool is_power_reg(u8 reg)
> {
> return reg < LTC2945_SENSE_H;
> @@ -475,8 +481,9 @@ MODULE_DEVICE_TABLE(i2c, ltc2945_id);
>
> static struct i2c_driver ltc2945_driver = {
> .driver = {
> - .name = "ltc2945",
> - },
> + .name = "ltc2945",
> + .of_match_table = of_match_ptr(ltc2945_of_match),
> + },
> .probe_new = ltc2945_probe,
> .id_table = ltc2945_id,
> };
> --
> 2.25.1
From: John Pruitt <jpruitt@criticallink.com>
Added the ability to specify the value of the shunt resistor in the
device tree instead of assuming it is 1 milliOhm. The value in the
device tree has the name shunt-resistor-micro-ohms and the
default value is 1000 micro-ohms in order to preserve the
current behavior.
Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com>
Signed-off-by: John Pruitt <jpruitt@criticallink.com>
---
drivers/hwmon/ltc2945.c | 80 +++++++++++++++++++++++++----------------
1 file changed, 50 insertions(+), 30 deletions(-)
diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
index 9af3e3821152..fc7d399b2c85 100644
--- a/drivers/hwmon/ltc2945.c
+++ b/drivers/hwmon/ltc2945.c
@@ -64,6 +64,16 @@ static const struct of_device_id __maybe_unused ltc2945_of_match[] = {
};
MODULE_DEVICE_TABLE(of, ltc2945_of_match);
+/**
+ * struct ltc2945_data - LTC2945 device data
+ * @regmap: regmap device
+ * @shunt_resistor: shunt resistor value in micro ohms (1000 by default)
+ */
+struct ltc2945_data {
+ struct regmap *regmap;
+ u32 shunt_resistor;
+};
+
static inline bool is_power_reg(u8 reg)
{
return reg < LTC2945_SENSE_H;
@@ -72,7 +82,9 @@ 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_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
+ u32 shunt_resistor = data->shunt_resistor;
unsigned int control;
u8 buf[3];
long long val;
@@ -98,9 +110,7 @@ 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
* Control register bit 0 selects if voltage at SENSE+/VDD
* or voltage at ADIN is used to measure power.
*/
@@ -114,6 +124,8 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
/* 0.5 mV * 25 uV = 0.0125 uV resolution. */
val = (val * 25LL) >> 1;
}
+ val *= 1000;
+ val = DIV_ROUND_CLOSEST_ULL(val, shunt_resistor);
break;
case LTC2945_VIN_H:
case LTC2945_MAX_VIN_H:
@@ -136,14 +148,9 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
case LTC2945_MIN_SENSE_H:
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.
- */
- val *= 25;
+ /* 25 uV resolution. Convert to mA. */
+ val *= 25 * 1000;
+ val = DIV_ROUND_CLOSEST_ULL(val, shunt_resistor);
break;
default:
return -EINVAL;
@@ -154,7 +161,9 @@ 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_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
+ u32 shunt_resistor = data->shunt_resistor;
unsigned int control;
int ret;
@@ -165,9 +174,6 @@ 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.
* 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.
@@ -177,14 +183,16 @@ 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 *= shunt_resistor;
+ val = DIV_ROUND_CLOSEST(val, 625 * 1000);
} 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 *= shunt_resistor;
+ val = DIV_ROUND_CLOSEST(val, 25 * 1000) * 2;
}
break;
case LTC2945_VIN_H:
@@ -208,14 +216,9 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
case LTC2945_MIN_SENSE_H:
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.
- */
- val = DIV_ROUND_CLOSEST(val, 25);
+ /* 25 uV resolution. Convert to mA. */
+ val *= shunt_resistor;
+ val = DIV_ROUND_CLOSEST(val, 25 * 1000);
break;
default:
return -EINVAL;
@@ -240,7 +243,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_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
u8 reg = attr->index;
unsigned long val;
u8 regbuf[3];
@@ -275,7 +279,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_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
u8 reg = attr->index;
int num_regs = is_power_reg(reg) ? 3 : 2;
u8 buf_min[3] = { 0xff, 0xff, 0xff };
@@ -327,7 +332,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_data *data = dev_get_drvdata(dev);
+ struct regmap *regmap = data->regmap;
unsigned int fault;
int ret;
@@ -456,6 +462,12 @@ static int ltc2945_probe(struct i2c_client *client)
struct device *dev = &client->dev;
struct device *hwmon_dev;
struct regmap *regmap;
+ struct ltc2945_data *data;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+ dev_set_drvdata(dev, data);
regmap = devm_regmap_init_i2c(client, <c2945_regmap_config);
if (IS_ERR(regmap)) {
@@ -463,11 +475,19 @@ static int ltc2945_probe(struct i2c_client *client)
return PTR_ERR(regmap);
}
+ data->regmap = regmap;
+ if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
+ &data->shunt_resistor))
+ data->shunt_resistor = 1000;
+
+ if (data->shunt_resistor == 0)
+ return -EINVAL;
+
/* Clear faults */
regmap_write(regmap, LTC2945_FAULT, 0x00);
hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
- regmap,
+ data,
ltc2945_groups);
return PTR_ERR_OR_ZERO(hwmon_dev);
}
--
2.25.1On Mon, Dec 19, 2022 at 07:04:56PM -0500, Cormier, Jonathan wrote:
> From: John Pruitt <jpruitt@criticallink.com>
>
> Added the ability to specify the value of the shunt resistor in the
a/Added/Add/
Per Documentation/process/submitting-patches.rst:
"
Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.
"
> device tree instead of assuming it is 1 milliOhm. The value in the
> device tree has the name shunt-resistor-micro-ohms and the
> default value is 1000 micro-ohms in order to preserve the
> current behavior.
>
> Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com>
> Signed-off-by: John Pruitt <jpruitt@criticallink.com>
> ---
> drivers/hwmon/ltc2945.c | 80 +++++++++++++++++++++++++----------------
> 1 file changed, 50 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> index 9af3e3821152..fc7d399b2c85 100644
> --- a/drivers/hwmon/ltc2945.c
> +++ b/drivers/hwmon/ltc2945.c
> @@ -64,6 +64,16 @@ static const struct of_device_id __maybe_unused ltc2945_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, ltc2945_of_match);
>
> +/**
> + * struct ltc2945_data - LTC2945 device data
> + * @regmap: regmap device
> + * @shunt_resistor: shunt resistor value in micro ohms (1000 by default)
> + */
> +struct ltc2945_data {
> + struct regmap *regmap;
> + u32 shunt_resistor;
> +};
> +
> static inline bool is_power_reg(u8 reg)
> {
> return reg < LTC2945_SENSE_H;
> @@ -72,7 +82,9 @@ 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_data *data = dev_get_drvdata(dev);
> + struct regmap *regmap = data->regmap;
> + u32 shunt_resistor = data->shunt_resistor;
> unsigned int control;
> u8 buf[3];
> long long val;
> @@ -98,9 +110,7 @@ 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
> * Control register bit 0 selects if voltage at SENSE+/VDD
> * or voltage at ADIN is used to measure power.
> */
> @@ -114,6 +124,8 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
> /* 0.5 mV * 25 uV = 0.0125 uV resolution. */
> val = (val * 25LL) >> 1;
> }
> + val *= 1000;
> + val = DIV_ROUND_CLOSEST_ULL(val, shunt_resistor);
> break;
> case LTC2945_VIN_H:
> case LTC2945_MAX_VIN_H:
> @@ -136,14 +148,9 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
> case LTC2945_MIN_SENSE_H:
> 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.
> - */
> - val *= 25;
> + /* 25 uV resolution. Convert to mA. */
> + val *= 25 * 1000;
> + val = DIV_ROUND_CLOSEST_ULL(val, shunt_resistor);
> break;
> default:
> return -EINVAL;
> @@ -154,7 +161,9 @@ 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_data *data = dev_get_drvdata(dev);
> + struct regmap *regmap = data->regmap;
> + u32 shunt_resistor = data->shunt_resistor;
> unsigned int control;
> int ret;
>
> @@ -165,9 +174,6 @@ 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.
> * 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.
> @@ -177,14 +183,16 @@ 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 *= shunt_resistor;
> + val = DIV_ROUND_CLOSEST(val, 625 * 1000);
Overflows are introduced and need to be be handled in this patch.
It doesn't make sense to introduce them and fix them in the next patch.
> } 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 *= shunt_resistor;
> + val = DIV_ROUND_CLOSEST(val, 25 * 1000) * 2;
> }
> break;
> case LTC2945_VIN_H:
> @@ -208,14 +216,9 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> case LTC2945_MIN_SENSE_H:
> 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.
> - */
> - val = DIV_ROUND_CLOSEST(val, 25);
> + /* 25 uV resolution. Convert to mA. */
> + val *= shunt_resistor;
> + val = DIV_ROUND_CLOSEST(val, 25 * 1000);
> break;
> default:
> return -EINVAL;
> @@ -240,7 +243,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_data *data = dev_get_drvdata(dev);
> + struct regmap *regmap = data->regmap;
> u8 reg = attr->index;
> unsigned long val;
> u8 regbuf[3];
> @@ -275,7 +279,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_data *data = dev_get_drvdata(dev);
> + struct regmap *regmap = data->regmap;
> u8 reg = attr->index;
> int num_regs = is_power_reg(reg) ? 3 : 2;
> u8 buf_min[3] = { 0xff, 0xff, 0xff };
> @@ -327,7 +332,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_data *data = dev_get_drvdata(dev);
> + struct regmap *regmap = data->regmap;
> unsigned int fault;
> int ret;
>
> @@ -456,6 +462,12 @@ static int ltc2945_probe(struct i2c_client *client)
> struct device *dev = &client->dev;
> struct device *hwmon_dev;
> struct regmap *regmap;
> + struct ltc2945_data *data;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + dev_set_drvdata(dev, data);
>
> regmap = devm_regmap_init_i2c(client, <c2945_regmap_config);
> if (IS_ERR(regmap)) {
> @@ -463,11 +475,19 @@ static int ltc2945_probe(struct i2c_client *client)
> return PTR_ERR(regmap);
> }
>
> + data->regmap = regmap;
> + if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> + &data->shunt_resistor))
> + data->shunt_resistor = 1000;
> +
> + if (data->shunt_resistor == 0)
> + return -EINVAL;
> +
> /* Clear faults */
> regmap_write(regmap, LTC2945_FAULT, 0x00);
>
> hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
> - regmap,
> + data,
> ltc2945_groups);
> return PTR_ERR_OR_ZERO(hwmon_dev);
> }
> --
> 2.25.1
From: John Pruitt <jpruitt@criticallink.com>
Use 64-bit values for intermediate calculations. Check for
overflows and return INT_MAX if overflows happened.
Signed-off-by: John Pruitt <jpruitt@criticallink.com>
Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com>
---
drivers/hwmon/ltc2945.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
index fc7d399b2c85..7239422fc6db 100644
--- a/drivers/hwmon/ltc2945.c
+++ b/drivers/hwmon/ltc2945.c
@@ -126,6 +126,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
}
val *= 1000;
val = DIV_ROUND_CLOSEST_ULL(val, shunt_resistor);
+ /* check for overflow, use MAX value if it happened */
+ if (val > INT_MAX)
+ val = INT_MAX;
+
break;
case LTC2945_VIN_H:
case LTC2945_MAX_VIN_H:
@@ -159,12 +163,14 @@ 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 val_32)
{
struct ltc2945_data *data = dev_get_drvdata(dev);
struct regmap *regmap = data->regmap;
u32 shunt_resistor = data->shunt_resistor;
unsigned int control;
+ /* use 64-bit val for intermediate calculations */
+ unsigned long long val = val_32;
int ret;
switch (reg) {
@@ -184,7 +190,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
if (control & CONTROL_MULT_SELECT) {
/* 25 mV * 25 uV = 0.625 uV resolution. */
val *= shunt_resistor;
- val = DIV_ROUND_CLOSEST(val, 625 * 1000);
+ val = DIV_ROUND_CLOSEST_ULL(val, 625LL * 1000LL);
} else {
/*
* 0.5 mV * 25 uV = 0.0125 uV resolution.
@@ -192,7 +198,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
* accept loss of accuracy.
*/
val *= shunt_resistor;
- val = DIV_ROUND_CLOSEST(val, 25 * 1000) * 2;
+ val = DIV_ROUND_CLOSEST_ULL(val, 25LL * 1000LL) * 2;
}
break;
case LTC2945_VIN_H:
@@ -201,7 +207,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_ROUND_CLOSEST_ULL(val, 25LL);
break;
case LTC2945_ADIN_H:
case LTC2945_MAX_ADIN_H:
@@ -218,11 +224,15 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
case LTC2945_MIN_SENSE_THRES_H:
/* 25 uV resolution. Convert to mA. */
val *= shunt_resistor;
- val = DIV_ROUND_CLOSEST(val, 25 * 1000);
+ val = DIV_ROUND_CLOSEST_ULL(val, 25LL * 1000LL);
break;
default:
return -EINVAL;
}
+ /* If val is too large, just return the max value */
+ if (val > INT_MAX)
+ return INT_MAX;
+
return val;
}
--
2.25.1On Mon, Dec 19, 2022 at 07:04:57PM -0500, Cormier, Jonathan wrote:
> From: John Pruitt <jpruitt@criticallink.com>
>
> Use 64-bit values for intermediate calculations. Check for
> overflows and return INT_MAX if overflows happened.
>
> Signed-off-by: John Pruitt <jpruitt@criticallink.com>
> Signed-off-by: "Cormier, Jonathan" <jcormier@criticallink.com>
The problems here are introduced with the previous patch
and thus would need a Fixes: tag. It just doesn't make sense
to submit that as separate patch.
> ---
> drivers/hwmon/ltc2945.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> index fc7d399b2c85..7239422fc6db 100644
> --- a/drivers/hwmon/ltc2945.c
> +++ b/drivers/hwmon/ltc2945.c
> @@ -126,6 +126,10 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg)
> }
> val *= 1000;
> val = DIV_ROUND_CLOSEST_ULL(val, shunt_resistor);
> + /* check for overflow, use MAX value if it happened */
> + if (val > INT_MAX)
> + val = INT_MAX;
> +
ltc2945_reg_to_val returns long long, and the calling code expects long long.
How would this ever overflow ?
> break;
> case LTC2945_VIN_H:
> case LTC2945_MAX_VIN_H:
> @@ -159,12 +163,14 @@ 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 val_32)
> {
> struct ltc2945_data *data = dev_get_drvdata(dev);
> struct regmap *regmap = data->regmap;
> u32 shunt_resistor = data->shunt_resistor;
> unsigned int control;
> + /* use 64-bit val for intermediate calculations */
> + unsigned long long val = val_32;
This is unnnecessary. The parameter can be unsigned long long,
making the conversion automatic.
> int ret;
>
> switch (reg) {
> @@ -184,7 +190,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> if (control & CONTROL_MULT_SELECT) {
> /* 25 mV * 25 uV = 0.625 uV resolution. */
> val *= shunt_resistor;
> - val = DIV_ROUND_CLOSEST(val, 625 * 1000);
> + val = DIV_ROUND_CLOSEST_ULL(val, 625LL * 1000LL);
> } else {
> /*
> * 0.5 mV * 25 uV = 0.0125 uV resolution.
> @@ -192,7 +198,7 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> * accept loss of accuracy.
> */
> val *= shunt_resistor;
> - val = DIV_ROUND_CLOSEST(val, 25 * 1000) * 2;
> + val = DIV_ROUND_CLOSEST_ULL(val, 25LL * 1000LL) * 2;
> }
> break;
> case LTC2945_VIN_H:
> @@ -201,7 +207,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_ROUND_CLOSEST_ULL(val, 25LL);
Unrelated change causing behavioral change. Not that I mind, but it is
still unrelated and would have to be a separate patch.
> break;
> case LTC2945_ADIN_H:
> case LTC2945_MAX_ADIN_H:
> @@ -218,11 +224,15 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg,
> case LTC2945_MIN_SENSE_THRES_H:
> /* 25 uV resolution. Convert to mA. */
> val *= shunt_resistor;
> - val = DIV_ROUND_CLOSEST(val, 25 * 1000);
> + val = DIV_ROUND_CLOSEST_ULL(val, 25LL * 1000LL);
> break;
> default:
> return -EINVAL;
> }
> + /* If val is too large, just return the max value */
> + if (val > INT_MAX)
> + return INT_MAX;
> +
While the return value is declared as int, the calling code expects
unsigned long. It would be better to adjust the return value and clamp
against ULONG_MAX.
> return val;
> }
>
> --
> 2.25.1
© 2016 - 2025 Red Hat, Inc.