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
> 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.
> #define LM90_REG_REMOTE_HIGHH 0x07
> #define LM90_REG_REMOTE_HIGHL 0x13
> #define LM90_REG_REMOTE_LOWH 0x08
> @@ -669,6 +671,7 @@ enum lm90_temp_reg_index {
> REMOTE2_TEMP, /* max6695/96 only */
> REMOTE2_LOW, /* max6695/96 only */
> REMOTE2_HIGH, /* max6695/96 only */
> + REMOTE2_OFFSET,
>
> TEMP_REG_NUM
> };
> @@ -1024,6 +1027,14 @@ static int lm90_update_limits(struct device *dev)
> return val;
> data->temp[REMOTE2_HIGH] = val << 8;
>
> + if (data->flags & LM90_HAVE_OFFSET) {
> + val = lm90_read16(client, LM90_REG_REMOTE2_OFFSH,
> + LM90_REG_REMOTE2_OFFSL, false);
Why not use LM90_REG_REMOTE_OFFSH and LM90_REG_REMOTE_OFFSL ?
The remove channel is selected here, after all.
> + if (val < 0)
> + return val;
> + data->temp[REMOTE2_OFFSET] = val;
> + }
> +
> lm90_select_remote_channel(data, false);
> }
>
> @@ -1294,6 +1305,7 @@ static int lm90_temp_get_resolution(struct lm90_data *data, int index)
> return data->resolution;
> return 8;
> case REMOTE_OFFSET:
> + case REMOTE2_OFFSET:
> case REMOTE2_TEMP:
> return data->resolution;
> case LOCAL_TEMP:
> @@ -1515,8 +1527,13 @@ static int lm90_temp_read(struct device *dev, u32 attr, int channel, long *val)
> *val = lm90_get_temphyst(data, lm90_temp_emerg_index[channel], channel);
> break;
> case hwmon_temp_offset:
> - *val = lm90_temp_from_reg(0, data->temp[REMOTE_OFFSET],
> - lm90_temp_get_resolution(data, REMOTE_OFFSET));
> + /* Both offset registers have the same resolution */
> + int res = lm90_temp_get_resolution(data, REMOTE_OFFSET);
> +
> + if (channel == 1)
> + *val = lm90_temp_from_reg(0, data->temp[REMOTE_OFFSET], res);
> + else
> + *val = lm90_temp_from_reg(0, data->temp[REMOTE2_OFFSET], res);
I would prefer the use of lm90_temp_offset_index[] as implemented for the
other attributes.
> break;
> default:
> return -EOPNOTSUPP;
> @@ -1556,11 +1573,19 @@ static int lm90_temp_write(struct device *dev, u32 attr, int channel, long val)
> channel, val);
> break;
> case hwmon_temp_offset:
> + /* Both offset registers have the same resolution */
> val = lm90_temp_to_reg(0, val,
> lm90_temp_get_resolution(data, REMOTE_OFFSET));
> - data->temp[REMOTE_OFFSET] = val;
> - err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> - LM90_REG_REMOTE_OFFSL, val);
> +
> + if (channel == 1) {
> + data->temp[REMOTE_OFFSET] = val;
> + err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> + LM90_REG_REMOTE_OFFSL, val);
> + } else {
> + data->temp[REMOTE2_OFFSET] = val;
> + err = lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
> + LM90_REG_REMOTE2_OFFSL, val);
> + }
Same as above.
Thanks,
Guenter
> break;
> default:
> err = -EOPNOTSUPP;
> @@ -2733,6 +2758,8 @@ static int lm90_probe(struct i2c_client *client)
> }
> if (data->flags & LM90_HAVE_EMERGENCY_ALARM)
> data->channel_config[2] |= HWMON_T_EMERGENCY_ALARM;
> + if (data->flags & LM90_HAVE_OFFSET)
> + data->channel_config[2] |= HWMON_T_OFFSET;
> }
>
> data->faultqueue_mask = lm90_params[data->kind].faultqueue_mask;
