Re: [PATCH 3/4] drivers/hwmon/pmbus: Add generic alarm bit for iin and pin

Re: [PATCH 3/4] drivers/hwmon/pmbus: Add generic alarm bit for iin and pin

[Guenter Roeck]

On Wed, Aug 09, 2017 at 05:19:16PM -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Add PB_STATUS_INPUT as the generic alarm bit for iin and pin. We also
> need to redo the status register checking before setting up the boolean
> attribute, since it won't necessarily check STATUS_WORD if the device
> doesn't support it, which we need for this bit.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

For the entire series: subject should start with "hwmon: (pmbus)".

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 277d170..1a51f8f 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -1045,6 +1045,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
>  				      const struct pmbus_sensor_attr *attr)
>  {
>  	struct pmbus_sensor *base;
> +	bool upper = !!(attr->gbit & 0xff00);	/* need to check STATUS_WORD */
>  	int ret;
>  
>  	if (attr->label) {
> @@ -1065,10 +1066,13 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
>  		/*
>  		 * Add generic alarm attribute only if there are no individual
>  		 * alarm attributes, if there is a global alarm bit, and if
> -		 * the generic status register for this page is accessible.
> +		 * the generic status register (word or byte, depending on
> +		 * which global bit is set) for this page is accessible.
>  		 */
>  		if (!ret && attr->gbit &&
> -		    pmbus_check_status_register(client, page)) {
> +		    ((upper && pmbus_check_word_register(client, page,
> +							 PMBUS_STATUS_WORD)) ||
> +		     (!upper && pmbus_check_status_register(client, page)))) {

We already know if PMBUS_STATUS_WORD is supported or not, since
pmbus_check_status_register() will use it. Just store that knowledge
in a flag when discovered and make this something like

		if (!ret && attr->gbit &&
		    (!upper || (upper && data->has_word_status)) &&
		    pmbus_check_status_register(client, page)) {

>  			ret = pmbus_add_boolean(data, name, "alarm", index,
>  						NULL, NULL,
>  						PB_STATUS_BASE + page,
> @@ -1324,6 +1328,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
>  		.func = PMBUS_HAVE_IIN,
>  		.sfunc = PMBUS_HAVE_STATUS_INPUT,
>  		.sbase = PB_STATUS_INPUT_BASE,
> +		.gbit = PB_STATUS_INPUT,
>  		.limit = iin_limit_attrs,
>  		.nlimit = ARRAY_SIZE(iin_limit_attrs),
>  	}, {
> @@ -1408,6 +1413,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
>  		.func = PMBUS_HAVE_PIN,
>  		.sfunc = PMBUS_HAVE_STATUS_INPUT,
>  		.sbase = PB_STATUS_INPUT_BASE,
> +		.gbit = PB_STATUS_INPUT,
>  		.limit = pin_limit_attrs,
>  		.nlimit = ARRAY_SIZE(pin_limit_attrs),
>  	}, {
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] drivers/hwmon/pmbus: Core extensions for STATUS_WORD support and debugfs

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

This series adds some functionality to the pmbus core.

The first two patches provide support for the STATUS_WORD register. This allows
more default alarm attributes to be used, as the upper byte of the status
register is available. The third patch then uses the STATUS_INPUT bit of the
status register to setup boolean attributes for input voltage and input power
attributes.

The fourth patch provides support for raw reads of pmbus status registers
through the debugfs interface. These can be very useful for hardware
diagnostics, especially on multi-page pmbus devices, as user-space access of
the i2c space could corrupt the pmbus page accounting.

Since RFC series:
 * Just use u16 instead of complicated u8 method for STATUS_WORD.
 * Re-ordered the changes.
 * Added conditional for creating bool attr for higher byte STATUS_WORD bits.

Edward A. James (4):
  drivers/hwmon/pmbus: Switch status registers to 16 bit
  drivers/hwmon/pmbus: Access word data for STATUS_WORD
  drivers/hwmon/pmbus: Add generic alarm bit for iin and pin
  drivers/hwmon/pmbus: Add debugfs for status registers

 drivers/hwmon/pmbus/pmbus.c      |  24 +++-
 drivers/hwmon/pmbus/pmbus.h      |  11 ++
 drivers/hwmon/pmbus/pmbus_core.c | 251 +++++++++++++++++++++++++++++++++++----
 3 files changed, 263 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[PATCH 3/4] drivers/hwmon/pmbus: Add generic alarm bit for iin and pin

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Add PB_STATUS_INPUT as the generic alarm bit for iin and pin. We also
need to redo the status register checking before setting up the boolean
attribute, since it won't necessarily check STATUS_WORD if the device
doesn't support it, which we need for this bit.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 277d170..1a51f8f 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1045,6 +1045,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 				      const struct pmbus_sensor_attr *attr)
 {
 	struct pmbus_sensor *base;
+	bool upper = !!(attr->gbit & 0xff00);	/* need to check STATUS_WORD */
 	int ret;
 
 	if (attr->label) {
@@ -1065,10 +1066,13 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 		/*
 		 * Add generic alarm attribute only if there are no individual
 		 * alarm attributes, if there is a global alarm bit, and if
-		 * the generic status register for this page is accessible.
+		 * the generic status register (word or byte, depending on
+		 * which global bit is set) for this page is accessible.
 		 */
 		if (!ret && attr->gbit &&
-		    pmbus_check_status_register(client, page)) {
+		    ((upper && pmbus_check_word_register(client, page,
+							 PMBUS_STATUS_WORD)) ||
+		     (!upper && pmbus_check_status_register(client, page)))) {
 			ret = pmbus_add_boolean(data, name, "alarm", index,
 						NULL, NULL,
 						PB_STATUS_BASE + page,
@@ -1324,6 +1328,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
 		.func = PMBUS_HAVE_IIN,
 		.sfunc = PMBUS_HAVE_STATUS_INPUT,
 		.sbase = PB_STATUS_INPUT_BASE,
+		.gbit = PB_STATUS_INPUT,
 		.limit = iin_limit_attrs,
 		.nlimit = ARRAY_SIZE(iin_limit_attrs),
 	}, {
@@ -1408,6 +1413,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
 		.func = PMBUS_HAVE_PIN,
 		.sfunc = PMBUS_HAVE_STATUS_INPUT,
 		.sbase = PB_STATUS_INPUT_BASE,
+		.gbit = PB_STATUS_INPUT,
 		.limit = pin_limit_attrs,
 		.nlimit = ARRAY_SIZE(pin_limit_attrs),
 	}, {
-- 
1.8.3.1