On Wed, Feb 26, 2020 at 12:29:11AM +0200, 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> > v2->v3: > Fix added by Vadim: > - Fix offsets for read buffer. > v1->v2: > Comments pointed out by Guenter: > - Drop tps53688_reg2data(); > - Replace i2c_smbus_read_i2c_block_data() and > i2c_smbus_write_i2c_block_data() with i2c_smbus_read_block_data() > and i2c_smbus_write_block_data(); > - Drop 'unused' parameter in tps53688_get_history(); > - Fix host byte order in tps53688_get_history(); > - Rename tps53688_reset_history_all() to > tps53688_start_logging_all(); > --- > drivers/hwmon/pmbus/tps53679.c | 242 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 241 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/pmbus/tps53679.c b/drivers/hwmon/pmbus/tps53679.c > index 157c99ffb52b..dc2a32307f2a 100644 > --- a/drivers/hwmon/pmbus/tps53679.c > +++ b/drivers/hwmon/pmbus/tps53679.c > @@ -34,6 +34,226 @@ 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_MIN_OFFSET 2 > +#define TPS53688_HISTORY_REG_MAX_OFFSET 0 > + > +const static u8 tps53688_reset_logging[] = { 0x00, 0x01, 0x00, 0x00 }; > +const static u8 tps53688_resume_logging[] = { 0x20, 0x00, 0x00, 0x00 }; > + > +static int > +tps53688_get_history(struct i2c_client *client, int reg, int page, int offset) > +{ > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client); > + u8 buf[I2C_SMBUS_BLOCK_MAX]; > + s16 exponent; > + s32 mantissa; > + long val; > + int ret; > + > + ret = pmbus_set_page(client, page, 0); > + if (ret < 0) > + return ret; > + > + ret = i2c_smbus_read_block_data(client, reg, buf); > + if (ret < 0) > + return ret; Check if ret matches the expected data length ? > + > + ret = le16_to_cpu(*(u16 *)(buf + offset)); > + if (reg == TPS53688_VOUT_PEAK) { > + /* Convert register value to LINEAR11 data. */ > + exponent = ((s16)ret) >> 11; > + mantissa = ((s16)((ret & GENMASK(10, 0)) << 5)) >> 5; > + val = mantissa * 1000L; > + if (exponent >= 0) > + val <<= exponent; > + else > + val >>= -exponent; > + > + /* Convert data to VID register. */ > + switch (info->vrm_version[page]) { > + case vr13: > + if (val >= 500) > + return 1 + DIV_ROUND_CLOSEST(val - 500, 10); > + return 0; > + case vr12: > + if (val >= 250) > + return 1 + DIV_ROUND_CLOSEST(val - 250, 5); > + return 0; > + default: > + return -EINVAL; > + } > + } Makes me wonder - should we add a utility function such as "linear11_to_vrm(u16 linear11, enum vrm_version mode)" to the pmbus core ? > + > + return ret; > +} > + > +static int > +tps53688_reset_history(struct i2c_client *client, int reg) > +{ > + int ret; > + > + ret = i2c_smbus_write_block_data(client, reg, > + ARRAY_SIZE(tps53688_reset_logging), > + tps53688_reset_logging); > + if (ret < 0) > + return ret; > + > + ret = i2c_smbus_write_block_data(client, reg, > + ARRAY_SIZE(tps53688_resume_logging), > + tps53688_resume_logging); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int tps53688_start_logging_all(struct i2c_client *client, int page) That doesn't just start logging, it also resets it. > +{ > + 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, 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 +408,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 +428,15 @@ 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; > + reset_history = true; > break; > case tps53681: > info->pages = TPS53679_PAGE_NUM; > @@ -220,7 +448,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_start_logging_all(client, i); We really now have a mismatch with the function name here. Do you really feel that strongly about resetting the history when the driver is loaded ? If so, then just name the function tps53688_reset_logging_all() and add an explanation why you are resetting the history here instead of just starting it. > + if (ret) > + return ret; This doesn't undo the probe. It is also a bit racy - the history will briefly be reported differently immediately after the sysfs attributes are created (ie after pmbus_do_probe() but before the history is reset). Guenter