Re: [hwmon-next v1] hwmon: (pmbus/tps53679) Add support for historical registers for TPS53688

[Guenter Roeck <linux@roeck-us.net>]

On Mon, Feb 24, 2020 at 10:13:18PM +0000, Vadim Pasternak wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Monday, February 24, 2020 4:51 PM
> > To: Vadim Pasternak <vadimp@mellanox.com>
> > Cc: linux-hwmon@vger.kernel.org
> > Subject: Re: [hwmon-next v1] hwmon: (pmbus/tps53679) Add support for
> > historical registers for TPS53688
> > 
> > On 2/24/20 5:13 AM, Vadim Pasternak wrote:
> > > TPS53688 supports historical registers. Patch allows exposing the next
> > > attributes for the historical registers in 'sysfs':
> > > - curr{x}_reset_history;
> > > - in{x}_reset_history;
> > > - power{x}_reset_history;
> > > - temp{x}_reset_history;
> > > - curr{x}_highest;
> > > - in{x}_highest;
> > > - power{x}_input_highest;
> > > - temp{x}_highest;
> > > - curr{x}_lowest;
> > > - in{x}_lowest;
> > > - power{x}_input_lowest;
> > > - temp{x}_lowest.
> > >
> > > This patch is required patch:
> > > "hwmon: (pmbus/core) Add callback for register to data conversion".
> > >
> > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
> > > ---
> > >   drivers/hwmon/pmbus/tps53679.c | 244
> > ++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 243 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hwmon/pmbus/tps53679.c
> > > b/drivers/hwmon/pmbus/tps53679.c index 157c99ffb52b..ae5ce144e5fe
> > > 100644
> > > --- a/drivers/hwmon/pmbus/tps53679.c
> > > +++ b/drivers/hwmon/pmbus/tps53679.c
> > > @@ -34,6 +34,227 @@ enum chips {
> > >
> > >   #define TPS53681_MFR_SPECIFIC_20	0xe4	/* Number of phases, per page
> > */
> > >
> > > +/* Registers for highest and lowest historical values records */
> > > +#define TPS53688_VOUT_PEAK		0xd1
> > > +#define TPS53688_IOUT_PEAK		0xd2
> > > +#define TPS53688_TEMP_PEAK		0xd3
> > > +#define TPS53688_VIN_PEAK		0xd5
> > > +#define TPS53688_IIN_PEAK		0xd6
> > > +#define TPS53688_PIN_PEAK		0xd7
> > > +#define TPS53688_POUT_PEAK		0xd8
> > > +#define TPS53688_HISTORY_REG_BUF_LEN	5
> > > +#define TPS53688_HISTORY_REG_MIN_OFFSET	3
> > > +#define TPS53688_HISTORY_REG_MAX_OFFSET	1
> > > +
> > > +const static u8 tps53688_reset_logging[] = { 0x04, 0x00, 0x01, 0x00,
> > > +0x00 }; const static u8 tps53688_resume_logging[] = { 0x04, 0x20,
> > > +0x00, 0x00, 0x00 };
> > > +
> > Passing the length as 1st field seems wrong.
> > 
> > > +static int tps53688_reg2data(u16 reg, int data, long *val) {
> > > +	s16 exponent;
> > > +	s32 mantissa;
> > > +
> > > +	if (data == 0)
> > > +		return data;
> > > +
> > > +	switch (reg) {
> > > +	case PMBUS_VIRT_READ_VOUT_MIN:
> > > +	case PMBUS_VIRT_READ_VOUT_MAX:
> > > +		/* Convert to LINEAR11. */
> > > +		exponent = ((s16)data) >> 11;
> > > +		mantissa = ((s16)((data & GENMASK(10, 0)) << 5)) >> 5;
> > > +		*val = mantissa * 1000L;
> > > +		if (exponent >= 0)
> > > +			*val <<= exponent;
> > > +		else
> > > +			*val >>= -exponent;
> > > +		return 0;
> > > +	default:
> > > +		return -ENODATA;
> > > +	}
> > > +}
> > > +
> > As with the xpde driver, I would suggest to implement the conversion in the
> > read_word_data function.
> > 
> > > +static int
> > > +tps53688_get_history(struct i2c_client *client, int reg, int page, int unused,
> > > +		     int offset)
> > 
> > What is the point of passing "unused" to this function ?
> > 
> > > +{
> > > +	u8 buf[TPS53688_HISTORY_REG_BUF_LEN];
> > > +	int ret;
> > > +
> > > +	ret = pmbus_set_page(client, page, 0);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = i2c_smbus_read_i2c_block_data(client, reg,
> > > +					    TPS53688_HISTORY_REG_BUF_LEN,
> > > +					    buf);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	else if (ret != TPS53688_HISTORY_REG_BUF_LEN)
> > > +		return -EIO;
> > 
> > I am a bit confused here. Are you sure this returns (and is supposed to return)
> > 5 bytes of data, not 4, and that the data offsets are 1 and 3, not 0 and 2 ?
> > i2c_smbus_read_i2c_block_data() returns the length, and only copies the data
> > into the buffer, not the length field.
> > 
> > > +
> > > +	return *(u16 *)(buf + offset);
> > 
> > This implies host byte order. I don't think it will work on big endian systems.
> > Besides, it won't work on systems which can not directly read from odd
> > addresses (if the odd offsets are indeed correct).
> > 
> > > +}
> > > +
> > > +static int
> > > +tps53688_reset_history(struct i2c_client *client, int reg) {
> > > +	int ret;
> > > +
> > > +	ret = i2c_smbus_write_i2c_block_data(client, reg,
> > > +					     TPS53688_HISTORY_REG_BUF_LEN,
> > > +					     tps53688_reset_logging);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = i2c_smbus_write_i2c_block_data(client, reg,
> > > +					     TPS53688_HISTORY_REG_BUF_LEN,
> > > +					     tps53688_resume_logging);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > Are you sure that a single write of 00 00 01 00 wouldn't do it ?
> > Is it indeed necessary to resume logging after resetting it ?
> > 
> 
> Yes.
> I used initially '0x00, 0x01, 0x00' and it didn't work for me.
> Then I managed to have a call with TI and after some debug they said
> I should resume after reset, so I added '0x02 0x00, 0x00, 0x00'.
> 
> Datasheet says:
> Issue a write transaction with the following values to control peak logging functions.
> Logging is not automatically started or stopped by TPS536xx.
> 0000 0001h Pause minimum value logging
> 0000 0002h Pause maximum value logging
> 0000 0004h Pause minimum and maximum value logging
> 0000 0008h Resume minimum value logging (if paused)
> 0000 0010h Resume maximum value logging (if paused)
> 0000 0020h Resume minimum and maximum value logging (if paused)
> 0000 0040h Reset the minimum value logging
> 0000 0080h Reset the maximum value logging
> 0000 0100h Reset both minimum and maximum value logging
> Other Invalid/Unsupported data.
> 
> So it's not active by default and should be resumed for starting logging.
> 
> Because of that I also added tps53688_reset_history_all() in probe, because
> otherwise after boot these registers have some abnormal values.
> But anyway I'll drop this routine following your comment below.
> Also considering that driver can be re-probed and in this case history after
> the first reset/resume could be important.
> 
Thanks for the explanation.

That means though that you'll need to call something like
tps53688_start_logging_all() in the probe function, or am I missing
something ? Otherwise the user would have to explicitly reset the
history to start logging it, which would not be desirable either.

Thanks,
Guenter