> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Monday, February 24, 2020 4:51 PM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> Cc: linux-hwmon@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxx>
> > ---
> > 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.
> > + return 0;
> > +}
> > +
> > +static int tps53688_reset_history_all(struct i2c_client *client, int
> > +page) {
> > + int ret;
> > +
> > + ret = pmbus_set_page(client, page, 0);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = tps53688_reset_history(client, TPS53688_TEMP_PEAK);
> > + ret = !ret ? tps53688_reset_history(client, TPS53688_VIN_PEAK) : ret;
> > + ret = !ret ? tps53688_reset_history(client, TPS53688_IIN_PEAK) : ret;
> > + ret = !ret ? tps53688_reset_history(client, TPS53688_PIN_PEAK) : ret;
> > + ret = !ret ? tps53688_reset_history(client, TPS53688_POUT_PEAK) : ret;
> > + ret = !ret ? tps53688_reset_history(client, TPS53688_VOUT_PEAK) : ret;
> > + ret = !ret ? tps53688_reset_history(client, TPS53688_IOUT_PEAK) :
> > +ret;
> > +
> > + return ret;
> > +}
> > +
> > +static int tps53688_read_word_data(struct i2c_client *client, int page,
> > + int unused, int reg)
> > +{
> > + int histreg, offset;
> > +
> > + switch (reg) {
> > + case PMBUS_VIRT_READ_TEMP_MIN:
> > + histreg = TPS53688_TEMP_PEAK;
> > + offset = TPS53688_HISTORY_REG_MIN_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_TEMP_MAX:
> > + histreg = TPS53688_TEMP_PEAK;
> > + offset = TPS53688_HISTORY_REG_MAX_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_VIN_MIN:
> > + histreg = TPS53688_VIN_PEAK;
> > + offset = TPS53688_HISTORY_REG_MIN_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_VIN_MAX:
> > + histreg = TPS53688_VIN_PEAK;
> > + offset = TPS53688_HISTORY_REG_MAX_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_IIN_MIN:
> > + histreg = TPS53688_IIN_PEAK;
> > + offset = TPS53688_HISTORY_REG_MIN_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_IIN_MAX:
> > + histreg = TPS53688_IIN_PEAK;
> > + offset = TPS53688_HISTORY_REG_MAX_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_PIN_MIN:
> > + histreg = TPS53688_PIN_PEAK;
> > + offset = TPS53688_HISTORY_REG_MIN_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_PIN_MAX:
> > + histreg = TPS53688_PIN_PEAK;
> > + offset = TPS53688_HISTORY_REG_MAX_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_POUT_MIN:
> > + histreg = TPS53688_POUT_PEAK;
> > + offset = TPS53688_HISTORY_REG_MIN_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_POUT_MAX:
> > + histreg = TPS53688_POUT_PEAK;
> > + offset = TPS53688_HISTORY_REG_MAX_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_VOUT_MIN:
> > + histreg = TPS53688_VOUT_PEAK;
> > + offset = TPS53688_HISTORY_REG_MIN_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_VOUT_MAX:
> > + histreg = TPS53688_VOUT_PEAK;
> > + offset = TPS53688_HISTORY_REG_MAX_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_IOUT_MIN:
> > + histreg = TPS53688_IOUT_PEAK;
> > + offset = TPS53688_HISTORY_REG_MIN_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_IOUT_MAX:
> > + histreg = TPS53688_IOUT_PEAK;
> > + offset = TPS53688_HISTORY_REG_MAX_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_TEMP2_MIN:
> > + histreg = TPS53688_TEMP_PEAK;
> > + offset = TPS53688_HISTORY_REG_MIN_OFFSET;
> > + break;
> > + case PMBUS_VIRT_READ_TEMP2_MAX:
> > + histreg = TPS53688_TEMP_PEAK;
> > + offset = TPS53688_HISTORY_REG_MAX_OFFSET;
> > + break;
> > + case PMBUS_VIRT_RESET_TEMP_HISTORY:
> > + case PMBUS_VIRT_RESET_TEMP2_HISTORY:
> > + case PMBUS_VIRT_RESET_VIN_HISTORY:
> > + case PMBUS_VIRT_RESET_IIN_HISTORY:
> > + case PMBUS_VIRT_RESET_PIN_HISTORY:
> > + case PMBUS_VIRT_RESET_POUT_HISTORY:
> > + case PMBUS_VIRT_RESET_VOUT_HISTORY:
> > + case PMBUS_VIRT_RESET_IOUT_HISTORY:
> > + return 0;
> > + default:
> > + return -ENODATA;
> > + }
> > +
> > + return tps53688_get_history(client, histreg, page, unused, offset);
> > +}
> > +
> > +static int tps53688_write_word_data(struct i2c_client *client, int unused1,
> > + int reg, u16 unused2)
> > +{
> > + int ret;
> > +
> > + switch (reg) {
> > + case PMBUS_VIRT_RESET_TEMP_HISTORY:
> > + case PMBUS_VIRT_RESET_TEMP2_HISTORY:
> > + ret = tps53688_reset_history(client, TPS53688_TEMP_PEAK);
> > + break;
> > + case PMBUS_VIRT_RESET_VIN_HISTORY:
> > + ret = tps53688_reset_history(client, TPS53688_VIN_PEAK);
> > + break;
> > + case PMBUS_VIRT_RESET_IIN_HISTORY:
> > + ret = tps53688_reset_history(client, TPS53688_IIN_PEAK);
> > + break;
> > + case PMBUS_VIRT_RESET_PIN_HISTORY:
> > + ret = tps53688_reset_history(client, TPS53688_PIN_PEAK);
> > + break;
> > + case PMBUS_VIRT_RESET_POUT_HISTORY:
> > + ret = tps53688_reset_history(client, TPS53688_POUT_PEAK);
> > + break;
> > + case PMBUS_VIRT_RESET_VOUT_HISTORY:
> > + ret = tps53688_reset_history(client, TPS53688_VOUT_PEAK);
> > + break;
> > + case PMBUS_VIRT_RESET_IOUT_HISTORY:
> > + ret = tps53688_reset_history(client, TPS53688_IOUT_PEAK);
> > + break;
> > + default:
> > + return -ENODATA;
> > + }
> > + return ret;
> > +}
> > +
> > static int tps53679_identify_mode(struct i2c_client *client,
> > struct pmbus_driver_info *info)
> > {
> > @@ -188,7 +409,9 @@ static int tps53679_probe(struct i2c_client *client,
> > {
> > struct device *dev = &client->dev;
> > struct pmbus_driver_info *info;
> > + bool reset_history = false;
> > enum chips chip_id;
> > + int i, ret;
> >
> > if (dev->of_node)
> > chip_id = (enum chips)of_device_get_match_data(dev);
> > @@ -206,9 +429,16 @@ static int tps53679_probe(struct i2c_client *client,
> > info->identify = tps53679_identify;
> > break;
> > case tps53679:
> > + info->pages = TPS53679_PAGE_NUM;
> > + info->identify = tps53679_identify;
> > + break;
> > case tps53688:
> > info->pages = TPS53679_PAGE_NUM;
> > info->identify = tps53679_identify;
> > + info->read_word_data = tps53688_read_word_data;
> > + info->write_word_data = tps53688_write_word_data;
> > + info->reg2data = tps53688_reg2data;
> > + reset_history = true;
> > break;
> > case tps53681:
> > info->pages = TPS53679_PAGE_NUM;
> > @@ -220,7 +450,19 @@ static int tps53679_probe(struct i2c_client *client,
> > return -ENODEV;
> > }
> >
> > - return pmbus_do_probe(client, id, info);
> > + ret = pmbus_do_probe(client, id, info);
> > + if (ret)
> > + return ret;
> > +
> > + if (reset_history) {
> > + for (i = 0; i < info->pages; i++) {
> > + ret = tps53688_reset_history_all(client, i);
> > + if (ret)
> > + return ret;
> > + }
> > + }
> > +
> Why ? Values recorded prior to loading the driver are just as important, as values
> recorded afterwards, or even more important.
>
> > + return ret;
> > }
> >
> > static const struct i2c_device_id tps53679_id[] = {
> >
