Message-ID: <20190529125314.GA30959@localhost.localdomain>
Date:   Wed, 29 May 2019 12:53:51 +0000
From:   "Adamski, Krzysztof (Nokia - PL/Wroclaw)" 
        <krzysztof.adamski@...ia.com>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     Jean Delvare <jdelvare@...e.com>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@...ia.com>
Subject: Re: [PATCH] adm1275: support PMBUS_VIRT_*_SAMPLES

On Wed, May 29, 2019 at 05:17:47AM -0700, Guenter Roeck wrote:
>On 5/29/19 12:11 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>On Tue, May 28, 2019 at 12:46:52PM -0700, Guenter Roeck wrote:
>>>On Fri, May 24, 2019 at 12:49:13PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>>>The device supports setting the number of samples for averaging the
>>>>measurements. There are two separate settings - PWR_AVG for averaging
>>>>PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of
>>>>PMON_CONFIG register. The values are stored as exponent of base 2 of the
>>>>actual number of samples that will be taken.
>>>>
>>>>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@...ia.com>
>>>>---
>>>>  drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 67 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
>>>>index f569372c9204..4efe1a9df563 100644
>>>>--- a/drivers/hwmon/pmbus/adm1275.c
>>>>+++ b/drivers/hwmon/pmbus/adm1275.c
>>>>@@ -23,6 +23,8 @@
>>>>  #include <linux/slab.h>
>>>>  #include <linux/i2c.h>
>>>>  #include <linux/bitops.h>
>>>>+#include <linux/bitfield.h>
>>>>+#include <linux/log2.h>
>>>>  #include "pmbus.h"
>>>>
>>>>  enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
>>>>@@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
>>>>  #define ADM1075_VAUX_OV_WARN		BIT(7)
>>>>  #define ADM1075_VAUX_UV_WARN		BIT(6)
>>>>
>>>>+#define ADM1275_PWR_AVG_MASK		GENMASK(13, 11)
>>>>+#define ADM1275_VI_AVG_MASK		GENMASK(10, 8)
>>>>+#define ADM1275_SAMPLES_AVG_MAX	128
>>>>+
>>>>  struct adm1275_data {
>>>>  	int id;
>>>>  	bool have_oc_fault;
>>>>@@ -90,6 +96,7 @@ struct adm1275_data {
>>>>  	bool have_pin_max;
>>>>  	bool have_temp_max;
>>>>  	struct pmbus_driver_info info;
>>>>+	struct mutex lock;
>>>>  };
>>>>
>>>>  #define to_adm1275_data(x)  container_of(x, struct adm1275_data, info)
>>>>@@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = {
>>>>  	[18] = { 7658, 0, -3 },		/* power, 21V, irange200 */
>>>>  };
>>>>
>>>>+static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask)
>>>
>>>Why is the mask passed through as u64 ?
>>
>>Good point. I used u64 as this is the type used by bitfield machinery
>>under the hood but I agree it doesn't make sense and is even confusing
>>to have this in the function prototype as we are using this to mask 16
>>bit word anyways. I will fix that in v2. I am gonna have to cast the ret
>>to u16 when passing to FIELD_GET() to make sure the __BF_FIELD_CHECK is
>>not complaining (since it is signed right now), though.
>>
>
>Not sure I understand what you are talking about. FIELD_GET() uses typeof().
>FIELD_GET() is used by other callers even with u8 and without any typecasts.
>Why would it be a problem here ?

So I basically agree with you but just wanted to note why there will be
additional cast needed in my code. The:
   return FIELD_GET(mask, ret);
will be changed to:
   return FIELD_GET(mask, (u16)ret);

And the reason for that is that the __BF_FIELD_CHECK does this check at
compile time (and breaks if this is true)
   (_mask) > (typeof(_reg))~0ull

In my case typeof(_reg) is int, so (typeof(_reg))~0ull = -1 which is
signed. _mask is unsigned. Depending on the type promotion, this might
or might not be true depending on the size of _mask. When _mask was u64,
it always worked. For _mask being u16, it will fail. For u32, this will
fail depending on if we are compiling for 32 or 64 bit architecture.

All this might be obvious to you but it wasn't to me, thus this note.

>>>
>>>>+{
>>>>+	int ret;
>>>>+
>>>>+	ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>>>>+	if (ret < 0)
>>>>+		return ret;
>>>>+
>>>>+	return FIELD_GET(mask, ret);
>>>>+}
>>>>+
>>>>+static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask,
>>>>+					    u16 word)
>>>>+{
>>>>+	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>>>>+	struct adm1275_data *data = to_adm1275_data(info);
>>>>+	int ret;
>>>>+
>>>>+	mutex_lock(&data->lock);
>>>
>>>Why is another lock on top of the lock provided by the pmbus core required ?
>>>
>>
>>Good point, I was considering if I should instead add mutex_lock on
>>update_lock in the pmbus_set_samples() function inside of pmbus_core.c
>>instead (as this function is missing it) but figured that not all
>>devices will need that (lm25066 didn't) so it might be a waste in most
>>cases. But this may be cleaner approach indeed.
>>
>>Is this what you mean or there is some other lock I missed?
>>
>pmbus_set_samples() should set the pmbus lock. That was missed when
>the function was added.

And by pmbus lock you mean the update_lock from the pmbus_data
structure? I didn't see any other lock but wanted to double check my
understanding.

>>>>+	ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>>>>+	if (ret < 0) {
>>>>+		mutex_unlock(&data->lock);
>>>>+		return ret;
>>>>+	}
>>>>+
>>>>+	word = FIELD_PREP(mask, word) | (ret & ~mask);
>>>>+	ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word);
>>>>+	mutex_unlock(&data->lock);
>>>>+
>>>>+	return ret;
>>>>+}
>>>>+
>>>>  static int adm1275_read_word_data(struct i2c_client *client, int page, int reg)
>>>>  {
>>>>  	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>>>>@@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg)
>>>>  		if (!data->have_temp_max)
>>>>  			return -ENXIO;
>>>>  		break;
>>>>+	case PMBUS_VIRT_POWER_SAMPLES:
>>>>+		ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK);
>>>>+		if (ret < 0)
>>>>+			break;
>>>>+		ret = 1 << ret;
>>>
>>>		ret = BIT(ret);
>>>
>>
>>I intentionally used the "raw" left shift to make it more obvious this
>>is pow2 arithmetic operation and an direct inverse to the ilog2() used
>>on write counterpart. This is also consistent with what I used in
>>lm25066.c driver not long time ago.
>>
>>I don't have strong preference but this is my reasoning. So do you still
>>think it is better to use BIT() macro instead?
>>
>
>I don't think that is a good rationale, but I'll let it go.
>

If you don't think so, I'll change it in v2. As I said, I don't have a
strong opinion on that.

Krzysztof

Powered by blists - more mailing lists