linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hwmon: (pmbus/adm1275) Fix power sampling support
@ 2019-06-12 14:58 Guenter Roeck
  2019-06-13 10:11 ` Adamski, Krzysztof (Nokia - PL/Wroclaw)
  0 siblings, 1 reply; 2+ messages in thread
From: Guenter Roeck @ 2019-06-12 14:58 UTC (permalink / raw)
  To: Hardware Monitoring
  Cc: Jean Delvare, Guenter Roeck, Krzysztof Adamski, Alexander Sverdlin

Not every chip supported by this driver supports setting the number
of samples for power averaging. Also, the power monitoring register
is not always a 16-bit register, and the configuration bits used for
voltage sampling are different depending on the register width.
Some conditional code is needed to fix the problem.

On top of all that, the compiler complains about problems with
FIELD_GET and FIELD_PREP macros if the file is built with W=1.
Avoid using those macros to silence the warning.

Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/adm1275.c | 84 +++++++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index a477ce0474bb..b23c2dd95893 100644
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -71,9 +71,17 @@ 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
+#define ADM1275_VI_AVG_SHIFT		0
+#define ADM1275_VI_AVG_MASK		GENMASK(ADM1275_VI_AVG_SHIFT + 2, \
+						ADM1275_VI_AVG_SHIFT)
+#define ADM1275_SAMPLES_AVG_MAX		128
+
+#define ADM1278_PRW_AVG_SHIFT		11
+#define ADM1278_PWR_AVG_MASK		GENMASK(ADM1278_PRW_AVG_SHIFT + 2, \
+						ADM1278_PRW_AVG_SHIFT)
+#define ADM1278_VI_AVG_SHIFT		8
+#define ADM1278_VI_AVG_MASK		GENMASK(ADM1278_VI_AVG_SHIFT + 2, \
+						ADM1278_VI_AVG_SHIFT)
 
 struct adm1275_data {
 	int id;
@@ -86,6 +94,7 @@ struct adm1275_data {
 	bool have_pin_min;
 	bool have_pin_max;
 	bool have_temp_max;
+	bool have_power_sampling;
 	struct pmbus_driver_info info;
 };
 
@@ -161,28 +170,60 @@ static const struct coefficients adm1293_coefficients[] = {
 	[18] = { 7658, 0, -3 },		/* power, 21V, irange200 */
 };
 
-static inline int adm1275_read_pmon_config(struct i2c_client *client, u16 mask)
+static inline int adm1275_read_pmon_config(const struct adm1275_data *data,
+					   struct i2c_client *client,
+					   bool is_power)
 {
-	int ret;
-
-	ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
+	int shift, ret;
+	u16 mask;
+
+	/*
+	 * The PMON configuration register is a 16-bit register only on chips
+	 * supporting power average sampling. On other chips it is an 8-bit
+	 * register.
+	 */
+	if (data->have_power_sampling) {
+		ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
+		mask = is_power ? ADM1278_PWR_AVG_MASK : ADM1278_VI_AVG_MASK;
+		shift = is_power ? ADM1278_PRW_AVG_SHIFT : ADM1278_VI_AVG_SHIFT;
+	} else {
+		ret = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+		mask = ADM1275_VI_AVG_MASK;
+		shift = ADM1275_VI_AVG_SHIFT;
+	}
 	if (ret < 0)
 		return ret;
 
-	return FIELD_GET(mask, (u16)ret);
+	return (ret & mask) >> shift;
 }
 
-static inline int adm1275_write_pmon_config(struct i2c_client *client, u16 mask,
+static inline int adm1275_write_pmon_config(const struct adm1275_data *data,
+					    struct i2c_client *client,
+					    bool is_power,
 					    u16 word)
 {
-	int ret;
-
-	ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
+	int shift, ret;
+	u16 mask;
+
+	if (data->have_power_sampling) {
+		ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
+		mask = is_power ? ADM1278_PWR_AVG_MASK : ADM1278_VI_AVG_MASK;
+		shift = is_power ? ADM1278_PRW_AVG_SHIFT : ADM1278_VI_AVG_SHIFT;
+	} else {
+		ret = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+		mask = ADM1275_VI_AVG_MASK;
+		shift = ADM1275_VI_AVG_SHIFT;
+	}
 	if (ret < 0)
 		return ret;
 
-	word = FIELD_PREP(mask, word) | (ret & ~mask);
-	ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word);
+	word = (ret & ~mask) | ((word << shift) & mask);
+	if (data->have_power_sampling)
+		ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG,
+						word);
+	else
+		ret = i2c_smbus_write_byte_data(client, ADM1275_PMON_CONFIG,
+						word);
 
 	return ret;
 }
@@ -266,14 +307,16 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg)
 			return -ENXIO;
 		break;
 	case PMBUS_VIRT_POWER_SAMPLES:
-		ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK);
+		if (!data->have_power_sampling)
+			return -ENXIO;
+		ret = adm1275_read_pmon_config(data, client, true);
 		if (ret < 0)
 			break;
 		ret = BIT(ret);
 		break;
 	case PMBUS_VIRT_IN_SAMPLES:
 	case PMBUS_VIRT_CURR_SAMPLES:
-		ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK);
+		ret = adm1275_read_pmon_config(data, client, false);
 		if (ret < 0)
 			break;
 		ret = BIT(ret);
@@ -323,14 +366,16 @@ static int adm1275_write_word_data(struct i2c_client *client, int page, int reg,
 		ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0);
 		break;
 	case PMBUS_VIRT_POWER_SAMPLES:
+		if (!data->have_power_sampling)
+			return -ENXIO;
 		word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX);
-		ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK,
+		ret = adm1275_write_pmon_config(data, client, true,
 						ilog2(word));
 		break;
 	case PMBUS_VIRT_IN_SAMPLES:
 	case PMBUS_VIRT_CURR_SAMPLES:
 		word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX);
-		ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK,
+		ret = adm1275_write_pmon_config(data, client, false,
 						ilog2(word));
 		break;
 	default:
@@ -528,6 +573,7 @@ static int adm1275_probe(struct i2c_client *client,
 		data->have_vout = true;
 		data->have_pin_max = true;
 		data->have_temp_max = true;
+		data->have_power_sampling = true;
 
 		coefficients = adm1272_coefficients;
 		vindex = (config & ADM1275_VRANGE) ? 1 : 0;
@@ -613,6 +659,7 @@ static int adm1275_probe(struct i2c_client *client,
 		data->have_vout = true;
 		data->have_pin_max = true;
 		data->have_temp_max = true;
+		data->have_power_sampling = true;
 
 		coefficients = adm1278_coefficients;
 		vindex = 0;
@@ -648,6 +695,7 @@ static int adm1275_probe(struct i2c_client *client,
 		data->have_pin_min = true;
 		data->have_pin_max = true;
 		data->have_mfr_vaux_status = true;
+		data->have_power_sampling = true;
 
 		coefficients = adm1293_coefficients;
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] hwmon: (pmbus/adm1275) Fix power sampling support
  2019-06-12 14:58 [PATCH] hwmon: (pmbus/adm1275) Fix power sampling support Guenter Roeck
@ 2019-06-13 10:11 ` Adamski, Krzysztof (Nokia - PL/Wroclaw)
  0 siblings, 0 replies; 2+ messages in thread
From: Adamski, Krzysztof (Nokia - PL/Wroclaw) @ 2019-06-13 10:11 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Hardware Monitoring, Jean Delvare, Sverdlin, Alexander (Nokia - DE/Ulm)

On Wed, Jun 12, 2019 at 07:58:24AM -0700, Guenter Roeck wrote:
>Not every chip supported by this driver supports setting the number
>of samples for power averaging. Also, the power monitoring register
>is not always a 16-bit register, and the configuration bits used for
>voltage sampling are different depending on the register width.
>Some conditional code is needed to fix the problem.
>
>On top of all that, the compiler complains about problems with
>FIELD_GET and FIELD_PREP macros if the file is built with W=1.
>Avoid using those macros to silence the warning.
>
>Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>---
> drivers/hwmon/pmbus/adm1275.c | 84 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 66 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
>index a477ce0474bb..b23c2dd95893 100644
>--- a/drivers/hwmon/pmbus/adm1275.c
>+++ b/drivers/hwmon/pmbus/adm1275.c
>@@ -71,9 +71,17 @@ 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
>+#define ADM1275_VI_AVG_SHIFT		0
>+#define ADM1275_VI_AVG_MASK		GENMASK(ADM1275_VI_AVG_SHIFT + 2, \
>+						ADM1275_VI_AVG_SHIFT)
>+#define ADM1275_SAMPLES_AVG_MAX		128
>+
>+#define ADM1278_PRW_AVG_SHIFT		11
>+#define ADM1278_PWR_AVG_MASK		GENMASK(ADM1278_PRW_AVG_SHIFT + 2, \
>+						ADM1278_PRW_AVG_SHIFT)

s/PRW/PWR/

>+#define ADM1278_VI_AVG_SHIFT		8
>+#define ADM1278_VI_AVG_MASK		GENMASK(ADM1278_VI_AVG_SHIFT + 2, \
>+						ADM1278_VI_AVG_SHIFT)
>
> struct adm1275_data {
> 	int id;
>@@ -86,6 +94,7 @@ struct adm1275_data {
> 	bool have_pin_min;
> 	bool have_pin_max;
> 	bool have_temp_max;
>+	bool have_power_sampling;
> 	struct pmbus_driver_info info;
> };
>
>@@ -161,28 +170,60 @@ static const struct coefficients adm1293_coefficients[] = {
> 	[18] = { 7658, 0, -3 },		/* power, 21V, irange200 */
> };
>
>-static inline int adm1275_read_pmon_config(struct i2c_client *client, u16 mask)
>+static inline int adm1275_read_pmon_config(const struct adm1275_data *data,

This function does not have to be inline anymore. It had to be inline
before because of FIELD_GET but this is not used anymore.

>+					   struct i2c_client *client,
>+					   bool is_power)
> {
>-	int ret;
>-
>-	ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+	int shift, ret;
>+	u16 mask;
>+
>+	/*
>+	 * The PMON configuration register is a 16-bit register only on chips
>+	 * supporting power average sampling. On other chips it is an 8-bit
>+	 * register.
>+	 */
>+	if (data->have_power_sampling) {
>+		ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+		mask = is_power ? ADM1278_PWR_AVG_MASK : ADM1278_VI_AVG_MASK;
>+		shift = is_power ? ADM1278_PRW_AVG_SHIFT : ADM1278_VI_AVG_SHIFT;
>+	} else {
>+		ret = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
>+		mask = ADM1275_VI_AVG_MASK;
>+		shift = ADM1275_VI_AVG_SHIFT;
>+	}
> 	if (ret < 0)
> 		return ret;
>
>-	return FIELD_GET(mask, (u16)ret);
>+	return (ret & mask) >> shift;
> }
>
>-static inline int adm1275_write_pmon_config(struct i2c_client *client, u16 mask,
>+static inline int adm1275_write_pmon_config(const struct adm1275_data *data,
>+					    struct i2c_client *client,
>+					    bool is_power,
> 					    u16 word)

Same comment about inline here.

> {
>-	int ret;
>-
>-	ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+	int shift, ret;
>+	u16 mask;
>+
>+	if (data->have_power_sampling) {
>+		ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+		mask = is_power ? ADM1278_PWR_AVG_MASK : ADM1278_VI_AVG_MASK;
>+		shift = is_power ? ADM1278_PRW_AVG_SHIFT : ADM1278_VI_AVG_SHIFT;
>+	} else {
>+		ret = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
>+		mask = ADM1275_VI_AVG_MASK;
>+		shift = ADM1275_VI_AVG_SHIFT;
>+	}
> 	if (ret < 0)
> 		return ret;
>
>-	word = FIELD_PREP(mask, word) | (ret & ~mask);
>-	ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word);
>+	word = (ret & ~mask) | ((word << shift) & mask);
>+	if (data->have_power_sampling)
>+		ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG,
>+						word);
>+	else
>+		ret = i2c_smbus_write_byte_data(client, ADM1275_PMON_CONFIG,
>+						word);
>
> 	return ret;
> }
>@@ -266,14 +307,16 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg)
> 			return -ENXIO;
> 		break;
> 	case PMBUS_VIRT_POWER_SAMPLES:
>-		ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK);
>+		if (!data->have_power_sampling)
>+			return -ENXIO;
>+		ret = adm1275_read_pmon_config(data, client, true);
> 		if (ret < 0)
> 			break;
> 		ret = BIT(ret);
> 		break;
> 	case PMBUS_VIRT_IN_SAMPLES:
> 	case PMBUS_VIRT_CURR_SAMPLES:
>-		ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK);
>+		ret = adm1275_read_pmon_config(data, client, false);
> 		if (ret < 0)
> 			break;
> 		ret = BIT(ret);
>@@ -323,14 +366,16 @@ static int adm1275_write_word_data(struct i2c_client *client, int page, int reg,
> 		ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0);
> 		break;
> 	case PMBUS_VIRT_POWER_SAMPLES:
>+		if (!data->have_power_sampling)
>+			return -ENXIO;
> 		word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX);
>-		ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK,
>+		ret = adm1275_write_pmon_config(data, client, true,
> 						ilog2(word));
> 		break;
> 	case PMBUS_VIRT_IN_SAMPLES:
> 	case PMBUS_VIRT_CURR_SAMPLES:
> 		word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX);
>-		ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK,
>+		ret = adm1275_write_pmon_config(data, client, false,
> 						ilog2(word));
> 		break;
> 	default:
>@@ -528,6 +573,7 @@ static int adm1275_probe(struct i2c_client *client,
> 		data->have_vout = true;
> 		data->have_pin_max = true;
> 		data->have_temp_max = true;
>+		data->have_power_sampling = true;
>
> 		coefficients = adm1272_coefficients;
> 		vindex = (config & ADM1275_VRANGE) ? 1 : 0;
>@@ -613,6 +659,7 @@ static int adm1275_probe(struct i2c_client *client,
> 		data->have_vout = true;
> 		data->have_pin_max = true;
> 		data->have_temp_max = true;
>+		data->have_power_sampling = true;
>
> 		coefficients = adm1278_coefficients;
> 		vindex = 0;
>@@ -648,6 +695,7 @@ static int adm1275_probe(struct i2c_client *client,
> 		data->have_pin_min = true;
> 		data->have_pin_max = true;
> 		data->have_mfr_vaux_status = true;
>+		data->have_power_sampling = true;
>
> 		coefficients = adm1293_coefficients;
>
>-- 
>2.7.4
>

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2019-06-13 15:37 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 14:58 [PATCH] hwmon: (pmbus/adm1275) Fix power sampling support Guenter Roeck
2019-06-13 10:11 ` Adamski, Krzysztof (Nokia - PL/Wroclaw)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).