> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Monday, February 24, 2020 5:11 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 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@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.
> >
> 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
