* [PATCH v2 1/4] hwmon: (ltc2945): wrap regmap into an ltc2945_state struct
2020-11-11 9:12 [PATCH v2 0/4] hwmon: (ltc2945): add support for sense resistor Alexandru Ardelean
@ 2020-11-11 9:12 ` Alexandru Ardelean
2020-11-11 9:12 ` [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg() Alexandru Ardelean
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Alexandru Ardelean @ 2020-11-11 9:12 UTC (permalink / raw)
To: linux-hwmon, devicetree, linux-kernel
Cc: robh+dt, linux, jdelvare, mark.thoren, ardeleanalex, 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, <c2945_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.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()
2020-11-11 9:12 [PATCH v2 0/4] hwmon: (ltc2945): add support for sense resistor Alexandru Ardelean
2020-11-11 9:12 ` [PATCH v2 1/4] hwmon: (ltc2945): wrap regmap into an ltc2945_state struct Alexandru Ardelean
@ 2020-11-11 9:12 ` Alexandru Ardelean
2020-11-11 14:54 ` Guenter Roeck
2020-11-13 7:25 ` kernel test robot
2020-11-11 9:12 ` [PATCH v2 3/4] hwmon: (ltc2945): add support for sense resistor Alexandru Ardelean
2020-11-11 9:12 ` [PATCH v2 4/4] dt-bindings: hwmon: ltc2945: add device tree doc for ltc2945 Alexandru Ardelean
3 siblings, 2 replies; 15+ messages in thread
From: Alexandru Ardelean @ 2020-11-11 9:12 UTC (permalink / raw)
To: linux-hwmon, devicetree, linux-kernel
Cc: robh+dt, linux, jdelvare, mark.thoren, ardeleanalex, 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 | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
index 1cea710df668..6d4569a25471 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,7 +247,7 @@ 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;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()
2020-11-11 9:12 ` [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg() Alexandru Ardelean
@ 2020-11-11 14:54 ` Guenter Roeck
2020-11-11 15:28 ` Alexandru Ardelean
2020-11-18 14:40 ` Alexandru Ardelean
2020-11-13 7:25 ` kernel test robot
1 sibling, 2 replies; 15+ messages in thread
From: Guenter Roeck @ 2020-11-11 14:54 UTC (permalink / raw)
To: Alexandru Ardelean, linux-hwmon, devicetree, linux-kernel
Cc: robh+dt, jdelvare, mark.thoren, ardeleanalex
On 11/11/20 1:12 AM, Alexandru Ardelean wrote:
> In order to account for any potential overflows that could occur.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Thinking about it, this can only really happen if the user provides
excessive values for limit attributes. Those are currently clamped
later, after the conversion. I think it would be better to modify
the code to apply a clamp _before_ the conversion as well instead
of trying to solve the overflow problem with unsigned long long.
Either case, can you send me a register dump for this chip ?
I'd like to write a module test script to actually check if there
are any over/underflows or other problems.
Thanks,
Guenter
> ---
> drivers/hwmon/ltc2945.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> index 1cea710df668..6d4569a25471 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,7 +247,7 @@ 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;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()
2020-11-11 14:54 ` Guenter Roeck
@ 2020-11-11 15:28 ` Alexandru Ardelean
2020-11-11 15:44 ` Guenter Roeck
2020-11-18 14:40 ` Alexandru Ardelean
1 sibling, 1 reply; 15+ messages in thread
From: Alexandru Ardelean @ 2020-11-11 15:28 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alexandru Ardelean, linux-hwmon, devicetree, LKML, Rob Herring,
jdelvare, Thoren, Mark
On Wed, Nov 11, 2020 at 4:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 11/11/20 1:12 AM, Alexandru Ardelean wrote:
> > In order to account for any potential overflows that could occur.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Thinking about it, this can only really happen if the user provides
> excessive values for limit attributes. Those are currently clamped
> later, after the conversion. I think it would be better to modify
> the code to apply a clamp _before_ the conversion as well instead
> of trying to solve the overflow problem with unsigned long long.
>
> Either case, can you send me a register dump for this chip ?
I asked Mark to help out on this.
Right now I don't have a board around my home-office.
I"m just pulling patches from our own tree to send upstream.
Is there a specific command you have in mind for this i2cdump?
Is the output of something like this fine:
# i2cdump -r 0x00-0x31 1 0x6f
?
We have a board that's being developed with this driver (and chip).
I think Mark will try to read values from the eval-board [since he has
one around].
Thanks
Alex
> I'd like to write a module test script to actually check if there
> are any over/underflows or other problems.
>
> Thanks,
> Guenter
>
> > ---
> > drivers/hwmon/ltc2945.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> > index 1cea710df668..6d4569a25471 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,7 +247,7 @@ 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;
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()
2020-11-11 15:28 ` Alexandru Ardelean
@ 2020-11-11 15:44 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2020-11-11 15:44 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: Alexandru Ardelean, linux-hwmon, devicetree, LKML, Rob Herring,
jdelvare, Thoren, Mark
On Wed, Nov 11, 2020 at 05:28:51PM +0200, Alexandru Ardelean wrote:
> On Wed, Nov 11, 2020 at 4:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 11/11/20 1:12 AM, Alexandru Ardelean wrote:
> > > In order to account for any potential overflows that could occur.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> >
> > Thinking about it, this can only really happen if the user provides
> > excessive values for limit attributes. Those are currently clamped
> > later, after the conversion. I think it would be better to modify
> > the code to apply a clamp _before_ the conversion as well instead
> > of trying to solve the overflow problem with unsigned long long.
> >
> > Either case, can you send me a register dump for this chip ?
>
> I asked Mark to help out on this.
> Right now I don't have a board around my home-office.
> I"m just pulling patches from our own tree to send upstream.
> Is there a specific command you have in mind for this i2cdump?
>
> Is the output of something like this fine:
> # i2cdump -r 0x00-0x31 1 0x6f
> ?
Yes, that should do, assuming the chip is on bus #1, address 0x6f.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()
2020-11-11 14:54 ` Guenter Roeck
2020-11-11 15:28 ` Alexandru Ardelean
@ 2020-11-18 14:40 ` Alexandru Ardelean
2020-11-18 15:19 ` Guenter Roeck
1 sibling, 1 reply; 15+ messages in thread
From: Alexandru Ardelean @ 2020-11-18 14:40 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alexandru Ardelean, linux-hwmon, devicetree, LKML, Rob Herring,
jdelvare, Thoren, Mark
On Wed, Nov 11, 2020 at 4:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 11/11/20 1:12 AM, Alexandru Ardelean wrote:
> > In order to account for any potential overflows that could occur.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Thinking about it, this can only really happen if the user provides
> excessive values for limit attributes. Those are currently clamped
> later, after the conversion. I think it would be better to modify
> the code to apply a clamp _before_ the conversion as well instead
> of trying to solve the overflow problem with unsigned long long.
Coming back to this, I think that using the shunt resistor value
(which is in micro-ohms), and multiplying with multiples of 1000, the
chances of overflow grow quite a lot.
I could be wrong, but that is how it looks when I try to do some math
with the shunt resistor in place.
Looking at the clamping code, it looks like the initial version is
quite simple & straightforward.
I mean when doing the math and getting values out of range, clamping
afterwards to 0xffffff for power values is quite handy.
And clamping everything else to 0xfff for voltage also looks pretty simple.
We can do some clamping before, but it looks like it's extra math that
is already done in the conversion code anyway.
Hopefully, I'm not missing something here :)
>
> Either case, can you send me a register dump for this chip ?
> I'd like to write a module test script to actually check if there
> are any over/underflows or other problems.
>
> Thanks,
> Guenter
>
> > ---
> > drivers/hwmon/ltc2945.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> > index 1cea710df668..6d4569a25471 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,7 +247,7 @@ 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;
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()
2020-11-18 14:40 ` Alexandru Ardelean
@ 2020-11-18 15:19 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2020-11-18 15:19 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: Alexandru Ardelean, linux-hwmon, devicetree, LKML, Rob Herring,
jdelvare, Thoren, Mark
On Wed, Nov 18, 2020 at 04:40:24PM +0200, Alexandru Ardelean wrote:
> On Wed, Nov 11, 2020 at 4:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 11/11/20 1:12 AM, Alexandru Ardelean wrote:
> > > In order to account for any potential overflows that could occur.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> >
> > Thinking about it, this can only really happen if the user provides
> > excessive values for limit attributes. Those are currently clamped
> > later, after the conversion. I think it would be better to modify
> > the code to apply a clamp _before_ the conversion as well instead
> > of trying to solve the overflow problem with unsigned long long.
>
> Coming back to this, I think that using the shunt resistor value
> (which is in micro-ohms), and multiplying with multiples of 1000, the
> chances of overflow grow quite a lot.
> I could be wrong, but that is how it looks when I try to do some math
> with the shunt resistor in place.
>
> Looking at the clamping code, it looks like the initial version is
> quite simple & straightforward.
> I mean when doing the math and getting values out of range, clamping
> afterwards to 0xffffff for power values is quite handy.
> And clamping everything else to 0xfff for voltage also looks pretty simple.
> We can do some clamping before, but it looks like it's extra math that
> is already done in the conversion code anyway.
>
> Hopefully, I'm not missing something here :)
>
Using ull operations when not necessary can also be quite expensive,
and I'd rather avoid it. I'd rather see an extra clamp than ull.
Guenter
> >
> > Either case, can you send me a register dump for this chip ?
> > I'd like to write a module test script to actually check if there
> > are any over/underflows or other problems.
> >
> > Thanks,
> > Guenter
> >
> > > ---
> > > drivers/hwmon/ltc2945.c | 12 ++++++------
> > > 1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> > > index 1cea710df668..6d4569a25471 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,7 +247,7 @@ 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;
> > >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()
2020-11-11 9:12 ` [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg() Alexandru Ardelean
@ 2020-11-13 7:25 ` kernel test robot
2020-11-13 7:25 ` kernel test robot
1 sibling, 0 replies; 15+ messages in thread
From: kernel test robot @ 2020-11-13 7:25 UTC (permalink / raw)
To: Alexandru Ardelean, linux-hwmon, devicetree, linux-kernel
Cc: kbuild-all, clang-built-linux, robh+dt, linux, jdelvare,
mark.thoren, ardeleanalex, Alexandru Ardelean
[-- Attachment #1: Type: text/plain, Size: 5196 bytes --]
Hi Alexandru,
I love your patch! Yet something to improve:
[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v5.10-rc3 next-20201112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Alexandru-Ardelean/hwmon-ltc2945-add-support-for-sense-resistor/20201111-171129
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: powerpc64-randconfig-r005-20201111 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 874b0a0b9db93f5d3350ffe6b5efda2d908415d0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/4e0e9315df2733ae5efe6095c5ab9b7675d07fb0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Alexandru-Ardelean/hwmon-ltc2945-add-support-for-sense-resistor/20201111-171129
git checkout 4e0e9315df2733ae5efe6095c5ab9b7675d07fb0
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/hwmon/ltc2945.c:256:26: error: incompatible pointer types passing 'unsigned long long *' to parameter of type 'unsigned long *' [-Werror,-Wincompatible-pointer-types]
ret = kstrtoul(buf, 10, &val);
^~~~
include/linux/kernel.h:351:90: note: passing argument to parameter 'res' here
static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
^
1 error generated.
vim +256 drivers/hwmon/ltc2945.c
6700ce035f8301 Guenter Roeck 2014-01-11 241
5614e26d84a99a Guenter Roeck 2018-12-06 242 static ssize_t ltc2945_value_store(struct device *dev,
6700ce035f8301 Guenter Roeck 2014-01-11 243 struct device_attribute *da,
6700ce035f8301 Guenter Roeck 2014-01-11 244 const char *buf, size_t count)
6700ce035f8301 Guenter Roeck 2014-01-11 245 {
6700ce035f8301 Guenter Roeck 2014-01-11 246 struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
c159257a60302f Alexandru Ardelean 2020-11-11 247 struct ltc2945_state *st = dev_get_drvdata(dev);
c159257a60302f Alexandru Ardelean 2020-11-11 248 struct regmap *regmap = st->regmap;
6700ce035f8301 Guenter Roeck 2014-01-11 249 u8 reg = attr->index;
4e0e9315df2733 Alexandru Ardelean 2020-11-11 250 unsigned long long val;
6700ce035f8301 Guenter Roeck 2014-01-11 251 u8 regbuf[3];
6700ce035f8301 Guenter Roeck 2014-01-11 252 int num_regs;
6700ce035f8301 Guenter Roeck 2014-01-11 253 int regval;
6700ce035f8301 Guenter Roeck 2014-01-11 254 int ret;
6700ce035f8301 Guenter Roeck 2014-01-11 255
6700ce035f8301 Guenter Roeck 2014-01-11 @256 ret = kstrtoul(buf, 10, &val);
6700ce035f8301 Guenter Roeck 2014-01-11 257 if (ret)
6700ce035f8301 Guenter Roeck 2014-01-11 258 return ret;
6700ce035f8301 Guenter Roeck 2014-01-11 259
6700ce035f8301 Guenter Roeck 2014-01-11 260 /* convert to register value, then clamp and write result */
6700ce035f8301 Guenter Roeck 2014-01-11 261 regval = ltc2945_val_to_reg(dev, reg, val);
6700ce035f8301 Guenter Roeck 2014-01-11 262 if (is_power_reg(reg)) {
6700ce035f8301 Guenter Roeck 2014-01-11 263 regval = clamp_val(regval, 0, 0xffffff);
6700ce035f8301 Guenter Roeck 2014-01-11 264 regbuf[0] = regval >> 16;
6700ce035f8301 Guenter Roeck 2014-01-11 265 regbuf[1] = (regval >> 8) & 0xff;
6700ce035f8301 Guenter Roeck 2014-01-11 266 regbuf[2] = regval;
6700ce035f8301 Guenter Roeck 2014-01-11 267 num_regs = 3;
6700ce035f8301 Guenter Roeck 2014-01-11 268 } else {
6700ce035f8301 Guenter Roeck 2014-01-11 269 regval = clamp_val(regval, 0, 0xfff) << 4;
6700ce035f8301 Guenter Roeck 2014-01-11 270 regbuf[0] = regval >> 8;
6700ce035f8301 Guenter Roeck 2014-01-11 271 regbuf[1] = regval & 0xff;
6700ce035f8301 Guenter Roeck 2014-01-11 272 num_regs = 2;
6700ce035f8301 Guenter Roeck 2014-01-11 273 }
6700ce035f8301 Guenter Roeck 2014-01-11 274 ret = regmap_bulk_write(regmap, reg, regbuf, num_regs);
6700ce035f8301 Guenter Roeck 2014-01-11 275 return ret < 0 ? ret : count;
6700ce035f8301 Guenter Roeck 2014-01-11 276 }
6700ce035f8301 Guenter Roeck 2014-01-11 277
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27065 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg()
@ 2020-11-13 7:25 ` kernel test robot
0 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2020-11-13 7:25 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5278 bytes --]
Hi Alexandru,
I love your patch! Yet something to improve:
[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v5.10-rc3 next-20201112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Alexandru-Ardelean/hwmon-ltc2945-add-support-for-sense-resistor/20201111-171129
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: powerpc64-randconfig-r005-20201111 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 874b0a0b9db93f5d3350ffe6b5efda2d908415d0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/4e0e9315df2733ae5efe6095c5ab9b7675d07fb0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Alexandru-Ardelean/hwmon-ltc2945-add-support-for-sense-resistor/20201111-171129
git checkout 4e0e9315df2733ae5efe6095c5ab9b7675d07fb0
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/hwmon/ltc2945.c:256:26: error: incompatible pointer types passing 'unsigned long long *' to parameter of type 'unsigned long *' [-Werror,-Wincompatible-pointer-types]
ret = kstrtoul(buf, 10, &val);
^~~~
include/linux/kernel.h:351:90: note: passing argument to parameter 'res' here
static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
^
1 error generated.
vim +256 drivers/hwmon/ltc2945.c
6700ce035f8301 Guenter Roeck 2014-01-11 241
5614e26d84a99a Guenter Roeck 2018-12-06 242 static ssize_t ltc2945_value_store(struct device *dev,
6700ce035f8301 Guenter Roeck 2014-01-11 243 struct device_attribute *da,
6700ce035f8301 Guenter Roeck 2014-01-11 244 const char *buf, size_t count)
6700ce035f8301 Guenter Roeck 2014-01-11 245 {
6700ce035f8301 Guenter Roeck 2014-01-11 246 struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
c159257a60302f Alexandru Ardelean 2020-11-11 247 struct ltc2945_state *st = dev_get_drvdata(dev);
c159257a60302f Alexandru Ardelean 2020-11-11 248 struct regmap *regmap = st->regmap;
6700ce035f8301 Guenter Roeck 2014-01-11 249 u8 reg = attr->index;
4e0e9315df2733 Alexandru Ardelean 2020-11-11 250 unsigned long long val;
6700ce035f8301 Guenter Roeck 2014-01-11 251 u8 regbuf[3];
6700ce035f8301 Guenter Roeck 2014-01-11 252 int num_regs;
6700ce035f8301 Guenter Roeck 2014-01-11 253 int regval;
6700ce035f8301 Guenter Roeck 2014-01-11 254 int ret;
6700ce035f8301 Guenter Roeck 2014-01-11 255
6700ce035f8301 Guenter Roeck 2014-01-11 @256 ret = kstrtoul(buf, 10, &val);
6700ce035f8301 Guenter Roeck 2014-01-11 257 if (ret)
6700ce035f8301 Guenter Roeck 2014-01-11 258 return ret;
6700ce035f8301 Guenter Roeck 2014-01-11 259
6700ce035f8301 Guenter Roeck 2014-01-11 260 /* convert to register value, then clamp and write result */
6700ce035f8301 Guenter Roeck 2014-01-11 261 regval = ltc2945_val_to_reg(dev, reg, val);
6700ce035f8301 Guenter Roeck 2014-01-11 262 if (is_power_reg(reg)) {
6700ce035f8301 Guenter Roeck 2014-01-11 263 regval = clamp_val(regval, 0, 0xffffff);
6700ce035f8301 Guenter Roeck 2014-01-11 264 regbuf[0] = regval >> 16;
6700ce035f8301 Guenter Roeck 2014-01-11 265 regbuf[1] = (regval >> 8) & 0xff;
6700ce035f8301 Guenter Roeck 2014-01-11 266 regbuf[2] = regval;
6700ce035f8301 Guenter Roeck 2014-01-11 267 num_regs = 3;
6700ce035f8301 Guenter Roeck 2014-01-11 268 } else {
6700ce035f8301 Guenter Roeck 2014-01-11 269 regval = clamp_val(regval, 0, 0xfff) << 4;
6700ce035f8301 Guenter Roeck 2014-01-11 270 regbuf[0] = regval >> 8;
6700ce035f8301 Guenter Roeck 2014-01-11 271 regbuf[1] = regval & 0xff;
6700ce035f8301 Guenter Roeck 2014-01-11 272 num_regs = 2;
6700ce035f8301 Guenter Roeck 2014-01-11 273 }
6700ce035f8301 Guenter Roeck 2014-01-11 274 ret = regmap_bulk_write(regmap, reg, regbuf, num_regs);
6700ce035f8301 Guenter Roeck 2014-01-11 275 return ret < 0 ? ret : count;
6700ce035f8301 Guenter Roeck 2014-01-11 276 }
6700ce035f8301 Guenter Roeck 2014-01-11 277
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 27065 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/4] hwmon: (ltc2945): add support for sense resistor
2020-11-11 9:12 [PATCH v2 0/4] hwmon: (ltc2945): add support for sense resistor Alexandru Ardelean
2020-11-11 9:12 ` [PATCH v2 1/4] hwmon: (ltc2945): wrap regmap into an ltc2945_state struct Alexandru Ardelean
2020-11-11 9:12 ` [PATCH v2 2/4] docs: hwmon: (ltc2945): change type of val to ULL in ltc2945_val_to_reg() Alexandru Ardelean
@ 2020-11-11 9:12 ` Alexandru Ardelean
2020-11-18 14:43 ` Alexandru Ardelean
2020-11-11 9:12 ` [PATCH v2 4/4] dt-bindings: hwmon: ltc2945: add device tree doc for ltc2945 Alexandru Ardelean
3 siblings, 1 reply; 15+ messages in thread
From: Alexandru Ardelean @ 2020-11-11 9:12 UTC (permalink / raw)
To: linux-hwmon, devicetree, linux-kernel
Cc: robh+dt, linux, jdelvare, mark.thoren, ardeleanalex, 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 | 53 ++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
index 6d4569a25471..909dd92a7a20 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,15 @@ 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;
+
+ if (st->r_sense_uohm == 0) {
+ dev_err(dev, "Zero value provided for sense resistor in DT");
+ return -EINVAL;
+ }
+
st->regmap = regmap;
/* Clear faults */
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] hwmon: (ltc2945): add support for sense resistor
2020-11-11 9:12 ` [PATCH v2 3/4] hwmon: (ltc2945): add support for sense resistor Alexandru Ardelean
@ 2020-11-18 14:43 ` Alexandru Ardelean
2020-11-18 15:21 ` Guenter Roeck
0 siblings, 1 reply; 15+ messages in thread
From: Alexandru Ardelean @ 2020-11-18 14:43 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-hwmon, devicetree, LKML, Rob Herring, Guenter Roeck,
jdelvare, Thoren, Mark
On Wed, Nov 11, 2020 at 11:08 AM Alexandru Ardelean
<alexandru.ardelean@analog.com> 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.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/hwmon/ltc2945.c | 53 ++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> index 6d4569a25471..909dd92a7a20 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);
I think that in this ltc2945_val_to_reg(), I should do the math the
other way around.
i.e. val = DIV_ROUND_CLOSEST_ULL(val * st->r_sense_uohm, 625000);
I got confused initially and did it wrong.
> } 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,15 @@ 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;
> +
> + if (st->r_sense_uohm == 0) {
> + dev_err(dev, "Zero value provided for sense resistor in DT");
> + return -EINVAL;
> + }
> +
> st->regmap = regmap;
>
> /* Clear faults */
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] hwmon: (ltc2945): add support for sense resistor
2020-11-18 14:43 ` Alexandru Ardelean
@ 2020-11-18 15:21 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2020-11-18 15:21 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: Alexandru Ardelean, linux-hwmon, devicetree, LKML, Rob Herring,
jdelvare, Thoren, Mark
On Wed, Nov 18, 2020 at 04:43:07PM +0200, Alexandru Ardelean wrote:
> On Wed, Nov 11, 2020 at 11:08 AM Alexandru Ardelean
> <alexandru.ardelean@analog.com> 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.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > drivers/hwmon/ltc2945.c | 53 ++++++++++++++++++++---------------------
> > 1 file changed, 26 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c
> > index 6d4569a25471..909dd92a7a20 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);
>
> I think that in this ltc2945_val_to_reg(), I should do the math the
> other way around.
> i.e. val = DIV_ROUND_CLOSEST_ULL(val * st->r_sense_uohm, 625000);
>
> I got confused initially and did it wrong.
>
Good catch.
Guenter
> > } 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,15 @@ 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;
> > +
> > + if (st->r_sense_uohm == 0) {
> > + dev_err(dev, "Zero value provided for sense resistor in DT");
> > + return -EINVAL;
> > + }
> > +
> > st->regmap = regmap;
> >
> > /* Clear faults */
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/4] dt-bindings: hwmon: ltc2945: add device tree doc for ltc2945
2020-11-11 9:12 [PATCH v2 0/4] hwmon: (ltc2945): add support for sense resistor Alexandru Ardelean
` (2 preceding siblings ...)
2020-11-11 9:12 ` [PATCH v2 3/4] hwmon: (ltc2945): add support for sense resistor Alexandru Ardelean
@ 2020-11-11 9:12 ` Alexandru Ardelean
2020-11-16 17:36 ` Rob Herring
3 siblings, 1 reply; 15+ messages in thread
From: Alexandru Ardelean @ 2020-11-11 9:12 UTC (permalink / raw)
To: linux-hwmon, devicetree, linux-kernel
Cc: robh+dt, linux, jdelvare, mark.thoren, ardeleanalex, Alexandru Ardelean
This change adds a device-tree binding documentation for the Linear
Technology (now Analog Devices) LTC2945 Wide Range I2C Power Monitor.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
.../bindings/hwmon/adi,ltc2945.yaml | 49 +++++++++++++++++++
1 file changed, 49 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..e49d7da09f74
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml
@@ -0,0 +1,49 @@
+# 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: Linear Technology LTC2945 Wide Range I2C Power Monitor
+
+maintainers:
+ - Guenter Roeck <linux@roeck-us.net>
+
+description: |
+ The LTC2945 is a rail-to-rail system monitor that measures current, voltage,
+ and power consumption.
+
+properties:
+ compatible:
+ enum:
+ - adi,ltc2945
+
+ reg:
+ maxItems: 1
+
+ shunt-resistor-micro-ohms:
+ description:
+ The value of curent sense resistor in microohms. If not provided,
+ a default value of 1000 microohms is used.
+ default: 1000
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ltc2945_i2c: ltc2945@6f {
+ compatible = "adi,ltc2945";
+ reg = <0x6f>;
+
+ shunt-resistor-micro-ohms = <20000>;
+ };
+ };
+...
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/4] dt-bindings: hwmon: ltc2945: add device tree doc for ltc2945
2020-11-11 9:12 ` [PATCH v2 4/4] dt-bindings: hwmon: ltc2945: add device tree doc for ltc2945 Alexandru Ardelean
@ 2020-11-16 17:36 ` Rob Herring
0 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2020-11-16 17:36 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-kernel, mark.thoren, devicetree, jdelvare, ardeleanalex,
linux, linux-hwmon, robh+dt
On Wed, 11 Nov 2020 11:12:59 +0200, Alexandru Ardelean wrote:
> This change adds a device-tree binding documentation for the Linear
> Technology (now Analog Devices) LTC2945 Wide Range I2C Power Monitor.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> .../bindings/hwmon/adi,ltc2945.yaml | 49 +++++++++++++++++++
> 1 file changed, 49 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc2945.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread