| Message ID | 20200224131316.28238-1-vadimp@mellanox.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [hwmon-next,v1] hwmon: (pmbus/tps53679) Add support for historical registers for TPS53688 | expand |
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 ? > + 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[] = { >
On 2/24/20 6:50 AM, Guenter Roeck wrote: > 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. > Wait, you care calling i2c_smbus_read_i2c_block_data() instead of i2c_smbus_read_block_data(). Maybe that explains why you need to specify the length twice. This should really be i2c_smbus_read_block_data() (and, yes, the buffer needs to fit the maximum smbus block length). Same for write. Thanks, Guenter
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Monday, February 24, 2020 5:11 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 6:50 AM, Guenter Roeck wrote: > > 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. > > > Wait, you care calling i2c_smbus_read_i2c_block_data() instead of > i2c_smbus_read_block_data(). Maybe that explains why you need to specify the > length twice. This should really be i2c_smbus_read_block_data() (and, yes, the > buffer needs to fit the maximum smbus block length). > > Same for write. Hi Gunter, It seems it was some misunderstanding. When we discussed this feature I asked: >> Can I assume that most i2c controllers support "smbus_read_block", or >> it's better to read with >> i2c_smbus_read_i2c_block_data() with size 5? >> And you replied: > I use i2c_smbus_read_i2c_block_data(); that is what it is for. So, I thought that i2c_smbus_read_i2c_block_data will be more common case. I'll change it. > > Thanks, > Guenter
On Mon, Feb 24, 2020 at 03:44:04PM +0000, Vadim Pasternak wrote: > [ ... ] > > When we discussed this feature I asked: > > >> Can I assume that most i2c controllers support "smbus_read_block", or > >> it's better to read with > >> i2c_smbus_read_i2c_block_data() with size 5? > >> > And you replied: > > I use i2c_smbus_read_i2c_block_data(); that is what it is for. > Outch. I must have had a bad day when I wrote that. I don't think I ever used it; I always use i2c_smbus_read_block_data(). Sorry for the confusion. Guenter
> -----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. > > + 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[] = { > >
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
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Tuesday, February 25, 2020 12:24 AM > 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 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. Yes, this is was my intention to activate logging on probe and do not it explicitly through all '{x}_reset_history' attributes. So, keep it in probe and just rename to tps53688_start_logging_all()? > > Thanks, > Guenter
On Mon, Feb 24, 2020 at 10:29:28PM +0000, Vadim Pasternak wrote: > > > > -----Original Message----- > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > > Sent: Tuesday, February 25, 2020 12:24 AM > > 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 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. > > Yes, this is was my intention to activate logging on probe and do not > it explicitly through all '{x}_reset_history' attributes. > So, keep it in probe and just rename to tps53688_start_logging_all()? > As long as it doesn't also _reset_ the history, sure. Thanks, Guenter
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 }; + +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; + } +} + +static int +tps53688_get_history(struct i2c_client *client, int reg, int page, int unused, + int offset) +{ + 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; + + return *(u16 *)(buf + offset); +} + +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; + + 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; + } + } + + return ret; } static const struct i2c_device_id tps53679_id[] = {
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(-)