Re: [PATCH 4/7] hwmon: (lm90) Add support for 2nd remote channel's offset register

[Slawomir Stepien <sst@poczta.fm>]

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@nokia.com>
> > 
> > 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@nokia.com>
> > ---
> >  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
setting it (the remote channel) in lm90_temp_write() a waste of xfers, if we can address the
registers directly. But if you prefer to have just one set of register and setting the remote
channel bit, then sure I can do it like that.

> >  #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.

Yes, in this case I can use the regs for remote channel 1. I will fix that in new version.

> > +			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.

OK, I will try to do it like that in new version.

> >  		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.

OK!

-- 
Slawomir Stepien