From: Guenter Roeck <linux@roeck-us.net>
To: Vadim Pasternak <vadimp@mellanox.com>
Cc: linux-hwmon@vger.kernel.org
Subject: Re: [hwmon-next v3] hwmon: (pmbus/tps53679) Add support for historical registers for TPS53688
Date: Tue, 25 Feb 2020 15:22:34 -0800 [thread overview]
Message-ID: <20200225232234.GA7751@roeck-us.net> (raw)
In-Reply-To: <20200225222911.30274-1-vadimp@mellanox.com>
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@mellanox.com>
> 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
prev parent reply other threads:[~2020-02-25 23:22 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-25 22:29 [hwmon-next v3] hwmon: (pmbus/tps53679) Add support for historical registers for TPS53688 Vadim Pasternak
2020-02-25 23:22 ` Guenter Roeck [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200225232234.GA7751@roeck-us.net \
--to=linux@roeck-us.net \
--cc=linux-hwmon@vger.kernel.org \
--cc=vadimp@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.