On 6/6/22 05:14, Slawomir Stepien wrote:
On cze 05, 2022 23:50, Guenter Roeck wrote:
On 6/5/22 23:30, Slawomir Stepien wrote:
On cze 05, 2022 11:03, Guenter Roeck wrote:
On Wed, May 25, 2022 at 09:36:54AM +0200, Slawomir Stepien wrote:
From: Slawomir Stepien <slawomir.stepien@xxxxxxxxx>
The ADT7461 supports offset register for both remote channels it has.
ADT7481
Oops. I will fix that in new version.
Both registers have the same bit width (resolution).
In the code, this device has LM90_HAVE_TEMP3 and LM90_HAVE_OFFSET flags,
but the support of second remote channel's offset is missing. Add that
implementation.
Signed-off-by: Slawomir Stepien <slawomir.stepien@xxxxxxxxx>
---
drivers/hwmon/lm90.c | 37 ++++++++++++++++++++++++++++++++-----
1 file changed, 32 insertions(+), 5 deletions(-)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 02b211a4e571..d226f1dea2ba 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -153,6 +153,8 @@ enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
#define LM90_REG_REMOTE_TEMPL 0x10
#define LM90_REG_REMOTE_OFFSH 0x11
#define LM90_REG_REMOTE_OFFSL 0x12
+#define LM90_REG_REMOTE2_OFFSH 0x34
+#define LM90_REG_REMOTE2_OFFSL 0x35
I don't think those are needed.
In lm90_temp_write() (unlike in lm90_update_limits()) the remote channel is *not* set. I find
... unless lm90_set_temp() is used to write the values. If I recall correctly
I didn't do that because selecting the remote channel seemed unnecessary.
I think that modifying lm90_set_temp() to support offsets is a bit messy:
1. The offset on all supported devices is always on two bytes. Unlike the temperature, where
sometimes it is just on one (but if more than one byte, then we set reg_remote_ext). With this also
'regs' in lm90_set_temp() will be back as 2 dimensional array OR additional high and low indexes for
REMOTE_OFFSET and REMOTE2_OFFSET should be added (that will also caused bits glueing on write/read).
2. For offset the calls lm90_temp_from/to_reg should have 0 as flags (1st argument) - that would be
an additional if in lm90_set_temp() OR clear&restore of the flags before&after the call..
Maybe, Guenter you will be happy with something like this (new functions):
static int lm90_get_temp_offset(struct lm90_data *data, int index)
{
int res = lm90_temp_get_resolution(data, index);
return lm90_temp_from_reg(0, data->temp[index], res);
}
static int lm90_set_temp_offset(struct lm90_data *data, int index, int channel, long val)
{
int err;
static const u8 regs[][2] = {
[REMOTE_OFFSET] = {LM90_REG_REMOTE_OFFSH, LM90_REG_REMOTE_OFFSL},
[REMOTE2_OFFSET] = {LM90_REG_REMOTE_OFFSH, LM90_REG_REMOTE_OFFSL},
};
That array is unnecessary.
u8 regh = regs[index][0];
u8 regl = regs[index][1];
regh = regs[0];
regl = regs[1];
has the same result, meaning you can just use LM90_REG_REMOTE_OFFSH
and LM90_REG_REMOTE_OFFSL directly.
... and then you can just use the direct registers and add a comment stating
that those only work for ADT7481 and not for chips which require channel select.
val = lm90_temp_to_reg(0, val, lm90_temp_get_resolution(data, index));
if (channel > 1)
lm90_select_remote_channel(data, true);
err = lm90_write16(data->client, regh, regl, val);
if (channel > 1)
lm90_select_remote_channel(data, false);
if (err)
return err;
data->temp[index] = val;
return 0;
}
And new channel->index translator:
static const s8 lm90_temp_offset_index[MAX_CHANNELS] = {
-1, REMOTE_OFFSET, REMOTE2_OFFSET
};
Having that, we can use that functions in hwmon's read/write attrs but also while paring the
device-tree channel nodes.
Maybe I missed something and using lm90_set_temp() will not be messy?
What do you think?
Using a new (simplified) function to write the registers sounds good.
Go for it.
Thanks,
Guenter
