[PATCH linux dev-4.10 v3 0/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[PATCH linux dev-4.10 v3 0/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Eddie James]

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

This series adds a hwmon pmbus driver for the IBM power supply. The core
monitoring functionality is provided by pmbus. The driver also exports some
sysfs entries for raw status register access.

The driver should be probed automatically, so I added it to the witherspoon
device-tree.

Edward A. James (5):
  drivers/hwmon/pmbus: Add IBM power supply hwmon driver
  drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes
  Documentation: hwmon: Add IBM power supply documentation
  Documentation: ABI: Add IBM power supply sysfs documentation
  ARM: dts: aspeed: witherspoon: Add IBM power supply to I2C bus

 Documentation/ABI/testing/sysfs-driver-ibmps     |  49 +++++
 Documentation/hwmon/ibmps                        |  53 +++++
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts |  10 +
 drivers/hwmon/pmbus/Kconfig                      |  10 +
 drivers/hwmon/pmbus/Makefile                     |   1 +
 drivers/hwmon/pmbus/ibmps.c                      | 242 +++++++++++++++++++++++
 6 files changed, 365 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-ibmps
 create mode 100644 Documentation/hwmon/ibmps
 create mode 100644 drivers/hwmon/pmbus/ibmps.c

-- 
1.8.3.1

[PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Eddie James]

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

Add the driver to monitor power supplies with hwmon over pmbus.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/Kconfig  |  10 +++
 drivers/hwmon/pmbus/Makefile |   1 +
 drivers/hwmon/pmbus/ibmps.c  | 164 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 drivers/hwmon/pmbus/ibmps.c

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 04f6baa..079a087 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -37,6 +37,16 @@ config SENSORS_ADM1275
 	  This driver can also be built as a module. If so, the module will
 	  be called adm1275.
 
+config SENSORS_IBMPS
+	tristate "IBM Power Supply"
+	default n
+	help
+	  If you say yes here you get hardware monitoring support for IBM
+	  power supply.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ibmps.
+
 config SENSORS_IR35221
 	tristate "Infineon IR35221"
 	default n
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 663801c..76d77e0 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -5,6 +5,7 @@
 obj-$(CONFIG_PMBUS)		+= pmbus_core.o
 obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
 obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
+obj-$(CONFIG_SENSORS_IBMPS)	+= ibmps.o
 obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
 obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
 obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
new file mode 100644
index 0000000..1928dd9
--- /dev/null
+++ b/drivers/hwmon/pmbus/ibmps.c
@@ -0,0 +1,164 @@
+/*
+ * Copyright 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+
+#include "pmbus.h"
+
+#define IBMPS_MFR_FAN_FAULT			BIT(0)
+#define IBMPS_MFR_THERMAL_FAULT			BIT(1)
+#define IBMPS_MFR_OV_FAULT			BIT(2)
+#define IBMPS_MFR_UV_FAULT			BIT(3)
+#define IBMPS_MFR_PS_KILL			BIT(4)
+#define IBMPS_MFR_OC_FAULT			BIT(5)
+#define IBMPS_MFR_VAUX_FAULT			BIT(6)
+#define IBMPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
+
+static int ibmps_read_word_data(struct i2c_client *client, int page, int reg);
+
+static int ibmps_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+	int rc, mfr;
+
+	switch (reg) {
+	case PMBUS_STATUS_BYTE:
+	case PMBUS_STATUS_WORD:
+		rc = ibmps_read_word_data(client, page, PMBUS_STATUS_WORD);
+		break;
+	case PMBUS_STATUS_VOUT:
+	case PMBUS_STATUS_IOUT:
+	case PMBUS_STATUS_TEMPERATURE:
+	case PMBUS_STATUS_FAN_12:
+		rc = pmbus_read_byte_data(client, page, reg);
+		if (rc < 0)
+			return rc;
+
+		mfr = pmbus_read_byte_data(client, page,
+					   PMBUS_STATUS_MFR_SPECIFIC);
+		if (mfr < 0)
+			return rc;
+
+		if (reg == PMBUS_STATUS_FAN_12) {
+			if (mfr & IBMPS_MFR_FAN_FAULT)
+				rc |= PB_FAN_FAN1_FAULT;
+		} else if (reg == PMBUS_STATUS_TEMPERATURE) {
+			if (mfr & IBMPS_MFR_THERMAL_FAULT)
+				rc |= PB_TEMP_OT_FAULT;
+		} else if (reg == PMBUS_STATUS_VOUT) {
+			if (mfr & (IBMPS_MFR_OV_FAULT | IBMPS_MFR_VAUX_FAULT))
+				rc |= PB_VOLTAGE_OV_FAULT;
+			if (mfr & IBMPS_MFR_UV_FAULT)
+				rc |= PB_VOLTAGE_UV_FAULT;
+		} else if (reg == PMBUS_STATUS_IOUT) {
+			if (mfr & IBMPS_MFR_OC_FAULT)
+				rc |= PB_IOUT_OC_FAULT;
+			if (mfr & IBMPS_MFR_CURRENT_SHARE_WARNING)
+				rc |= PB_CURRENT_SHARE_FAULT;
+		}
+		break;
+	default:
+		if (reg >= PMBUS_VIRT_BASE)
+			return -ENXIO;
+
+		rc = pmbus_read_byte_data(client, page, reg);
+		break;
+	}
+
+	return rc;
+}
+
+static int ibmps_read_word_data(struct i2c_client *client, int page, int reg)
+{
+	int rc, mfr;
+
+	switch (reg) {
+	case PMBUS_STATUS_BYTE:
+	case PMBUS_STATUS_WORD:
+		rc = pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
+		if (rc < 0)
+			return rc;
+
+		mfr = pmbus_read_byte_data(client, page,
+					   PMBUS_STATUS_MFR_SPECIFIC);
+		if (mfr < 0)
+			return rc;
+
+		if (mfr & IBMPS_MFR_PS_KILL)
+			rc |= PB_STATUS_OFF;
+
+		if (mfr)
+			rc |= PB_STATUS_WORD_MFR;
+		break;
+	default:
+		if (reg >= PMBUS_VIRT_BASE)
+			return -ENXIO;
+
+		rc = pmbus_read_word_data(client, page, reg);
+		if (rc < 0)
+			rc = ibmps_read_byte_data(client, page, reg);
+		break;
+	}
+
+	return rc;
+}
+
+static struct pmbus_driver_info ibmps_info = {
+	.pages = 1,
+	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+		PMBUS_HAVE_PIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP |
+		PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
+		PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
+		PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
+	.read_byte_data = ibmps_read_byte_data,
+	.read_word_data = ibmps_read_word_data,
+};
+
+static int ibmps_probe(struct i2c_client *client,
+		       const struct i2c_device_id *id)
+{
+	return pmbus_do_probe(client, id, &ibmps_info);
+}
+
+static int ibmps_remove(struct i2c_client *client)
+{
+	return pmbus_do_remove(client);
+}
+
+enum chips { witherspoon };
+
+static const struct i2c_device_id ibmps_id[] = {
+	{ "witherspoon", witherspoon },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ibmps_id);
+
+static const struct of_device_id ibmps_of_match[] = {
+	{ .compatible = "ibm,ibmps" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);
+
+static struct i2c_driver ibmps_driver = {
+	.driver = {
+		.name = "ibmps",
+		.of_match_table = ibmps_of_match,
+	},
+	.probe = ibmps_probe,
+	.remove = ibmps_remove,
+	.id_table = ibmps_id,
+};
+
+module_i2c_driver(ibmps_driver);
+
+MODULE_AUTHOR("Eddie James");
+MODULE_DESCRIPTION("PMBus driver for IBM power supply");
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

Re: [PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 8948 bytes --]

On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
> > From: "Edward A. James" <eajames@us.ibm.com>
> 
> Add the driver to monitor power supplies with hwmon over pmbus.
> 
> > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/hwmon/pmbus/Kconfig  |  10 +++
>  drivers/hwmon/pmbus/Makefile |   1 +
>  drivers/hwmon/pmbus/ibmps.c  | 164 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 175 insertions(+)
>  create mode 100644 drivers/hwmon/pmbus/ibmps.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 04f6baa..079a087 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -37,6 +37,16 @@ config SENSORS_ADM1275
> >  	  This driver can also be built as a module. If so, the module will
> >  	  be called adm1275.
>  
> +config SENSORS_IBMPS
> > +	tristate "IBM Power Supply"
> > +	default n
> > +	help
> > +	  If you say yes here you get hardware monitoring support for IBM
> > +	  power supply.
> +
> > +	  This driver can also be built as a module. If so, the module will
> > +	  be called ibmps.
> +
>  config SENSORS_IR35221
> >  	tristate "Infineon IR35221"
> >  	default n
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 663801c..76d77e0 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -5,6 +5,7 @@
> >  obj-$(CONFIG_PMBUS)		+= pmbus_core.o
> >  obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
> >  obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
> > +obj-$(CONFIG_SENSORS_IBMPS)	+= ibmps.o
> >  obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
> >  obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
> >  obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
> diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
> new file mode 100644
> index 0000000..1928dd9
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ibmps.c
> @@ -0,0 +1,164 @@
> +/*
> + * Copyright 2017 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +
> +#include "pmbus.h"
> +

I think a comment is needed here to point out these bits are for
STATUS_MFR_SPECIFIC.

> +#define IBMPS_MFR_FAN_FAULT			BIT(0)
> > +#define IBMPS_MFR_THERMAL_FAULT			BIT(1)
> > +#define IBMPS_MFR_OV_FAULT			BIT(2)
> > +#define IBMPS_MFR_UV_FAULT			BIT(3)
> > +#define IBMPS_MFR_PS_KILL			BIT(4)
> > +#define IBMPS_MFR_OC_FAULT			BIT(5)
> > +#define IBMPS_MFR_VAUX_FAULT			BIT(6)
> > +#define IBMPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
> +
> +static int ibmps_read_word_data(struct i2c_client *client, int page, int reg);
> +

This makes me curious.

> +static int ibmps_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> > +	int rc, mfr;
> +
> > +	switch (reg) {
> > +	case PMBUS_STATUS_BYTE:
> > +	case PMBUS_STATUS_WORD:
> +		rc = ibmps_read_word_data(client, page, PMBUS_STATUS_WORD);

Feels wrong to return "junk" in the result for a PMBUS_STATUS_BYTE
query.

> +		break;
> > +	case PMBUS_STATUS_VOUT:
> > +	case PMBUS_STATUS_IOUT:
> > +	case PMBUS_STATUS_TEMPERATURE:
> > +	case PMBUS_STATUS_FAN_12:
> > +		rc = pmbus_read_byte_data(client, page, reg);
> > +		if (rc < 0)
> > +			return rc;
> +
> > +		mfr = pmbus_read_byte_data(client, page,
> > +					   PMBUS_STATUS_MFR_SPECIFIC);
> > +		if (mfr < 0)
> > +			return rc;
> +
> > +		if (reg == PMBUS_STATUS_FAN_12) {
> > +			if (mfr & IBMPS_MFR_FAN_FAULT)
> > +				rc |= PB_FAN_FAN1_FAULT;
> > +		} else if (reg == PMBUS_STATUS_TEMPERATURE) {
> > +			if (mfr & IBMPS_MFR_THERMAL_FAULT)
> > +				rc |= PB_TEMP_OT_FAULT;
> > +		} else if (reg == PMBUS_STATUS_VOUT) {
> > +			if (mfr & (IBMPS_MFR_OV_FAULT | IBMPS_MFR_VAUX_FAULT))
> > +				rc |= PB_VOLTAGE_OV_FAULT;
> > +			if (mfr & IBMPS_MFR_UV_FAULT)
> > +				rc |= PB_VOLTAGE_UV_FAULT;
> > +		} else if (reg == PMBUS_STATUS_IOUT) {
> > +			if (mfr & IBMPS_MFR_OC_FAULT)
> > +				rc |= PB_IOUT_OC_FAULT;
> > +			if (mfr & IBMPS_MFR_CURRENT_SHARE_WARNING)
> +				rc |= PB_CURRENT_SHARE_FAULT;

> +		}

I wonder why it was done this way and not simply via the spec'ed bits?
Maybe some don't fit, but it seems there's a pretty direct mapping for
the most part.

Do the spec'ed status bits ever get set? As in, is there ever some
ambiguity about how the bits got set?

> > +		break;
> > +	default:
> > +		if (reg >= PMBUS_VIRT_BASE)
> > +			return -ENXIO;
> +
> +		rc = pmbus_read_byte_data(client, page, reg);

The alternative here is -ENODATA; not sure which is better though.

> +		break;
> > +	}
> +
> > +	return rc;
> +}
> +
> +static int ibmps_read_word_data(struct i2c_client *client, int page, int reg)
> +{
> > +	int rc, mfr;
> +
> > +	switch (reg) {
> > +	case PMBUS_STATUS_BYTE:
> > +	case PMBUS_STATUS_WORD:
> > +		rc = pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
> > +		if (rc < 0)
> > +			return rc;
> +
> > +		mfr = pmbus_read_byte_data(client, page,
> > +					   PMBUS_STATUS_MFR_SPECIFIC);
> > +		if (mfr < 0)
> > +			return rc;
> +
> > +		if (mfr & IBMPS_MFR_PS_KILL)
> > +			rc |= PB_STATUS_OFF;
> +
> > +		if (mfr)
> +			rc |= PB_STATUS_WORD_MFR;

This isn't set automatically by the device?

> +		break;
> > +	default:
> > +		if (reg >= PMBUS_VIRT_BASE)
> > +			return -ENXIO;
> +
> > +		rc = pmbus_read_word_data(client, page, reg);
> > +		if (rc < 0)
> +			rc = ibmps_read_byte_data(client, page, reg);

Is this really the way we should go about accesses?

> +		break;
> > +	}
> +
> > +	return rc;
> +}
> +
> +static struct pmbus_driver_info ibmps_info = {
> > +	.pages = 1,
> > +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > +		PMBUS_HAVE_PIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP |
> > +		PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
> > +		PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
> > +		PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
> > +	.read_byte_data = ibmps_read_byte_data,
> > +	.read_word_data = ibmps_read_word_data,
> +};
> +
> +static int ibmps_probe(struct i2c_client *client,
> > +		       const struct i2c_device_id *id)
> +{
> +	return pmbus_do_probe(client, id, &ibmps_info);

Is there anything we need to check before probe, e.g.
MFR_ID/MFR_MODEL/MFR_REVISION? Are there options that might need to be
configured (and exposed via devicetree bindings)?

I note we support fans: Is FAN_CONFIG_1_2 configured as needed? Or do
we need to enable the fan, configure PWM vs RPM and the tach pulses
value? If it is pre-configured, is this register always going to be
pre-configured? Or do we need a property like my proposed 'use-stored-
presence' for the MAX31785?

Have you had a look at my proposed bindings for the max31785? I think
it would be worth your input, as we have another case of fans here, and
it will help us shape a better picture of whether my generic properties
 and node structure are appropriate.

> +}
> +
> +static int ibmps_remove(struct i2c_client *client)
> +{
> > +	return pmbus_do_remove(client);
> +}
> +
> +enum chips { witherspoon };

This doesn't look necessary.

> +
> +static const struct i2c_device_id ibmps_id[] = {
> > +	{ "witherspoon", witherspoon },
> > +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ibmps_id);
> +
> +static const struct of_device_id ibmps_of_match[] = {
> > +	{ .compatible = "ibm,ibmps" },
> > +	{}
> +};
> +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);

p8_i2c_occ_of_match isn't right. Do we need both device tables?
"witherspoon" as a device ID also seems unusual. What's going on there?

> +
> +static struct i2c_driver ibmps_driver = {
> > +	.driver = {
> > +		.name = "ibmps",
> > +		.of_match_table = ibmps_of_match,
> > +	},
> > +	.probe = ibmps_probe,
> > +	.remove = ibmps_remove,
> > +	.id_table = ibmps_id,
> +};
> +
> +module_i2c_driver(ibmps_driver);
> +
> +MODULE_AUTHOR("Eddie James");
> +MODULE_DESCRIPTION("PMBus driver for IBM power supply");

So do we anticipate only ever having one power supply design? The name
feels a bit generic. In the device id table above you suggest it's
called 'witherspoon'. Is the design specific to Witherspoon? If so this
fact should be included in the driver name and description.

If there are other power supplies that are similar, then when we add
support we can rename the driver, and add any necessary module aliases,
and have a common device id table listing all of the supported devices.

Cheers,

Andrew

> +MODULE_LICENSE("GPL");

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Eddie James]

On 08/03/2017 07:34 PM, Andrew Jeffery wrote:
> On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
>>> From: "Edward A. James" <eajames@us.ibm.com>
>> Add the driver to monitor power supplies with hwmon over pmbus.
>>
>>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/hwmon/pmbus/Kconfig  |  10 +++
>>   drivers/hwmon/pmbus/Makefile |   1 +
>>   drivers/hwmon/pmbus/ibmps.c  | 164 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 175 insertions(+)
>>   create mode 100644 drivers/hwmon/pmbus/ibmps.c
>>
>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>> index 04f6baa..079a087 100644
>> --- a/drivers/hwmon/pmbus/Kconfig
>> +++ b/drivers/hwmon/pmbus/Kconfig
>> @@ -37,6 +37,16 @@ config SENSORS_ADM1275
>>>   	  This driver can also be built as a module. If so, the module will
>>>   	  be called adm1275.
>>   
>> +config SENSORS_IBMPS
>>> +	tristate "IBM Power Supply"
>>> +	default n
>>> +	help
>>> +	  If you say yes here you get hardware monitoring support for IBM
>>> +	  power supply.
>> +
>>> +	  This driver can also be built as a module. If so, the module will
>>> +	  be called ibmps.
>> +
>>   config SENSORS_IR35221
>>>   	tristate "Infineon IR35221"
>>>   	default n
>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
>> index 663801c..76d77e0 100644
>> --- a/drivers/hwmon/pmbus/Makefile
>> +++ b/drivers/hwmon/pmbus/Makefile
>> @@ -5,6 +5,7 @@
>>>   obj-$(CONFIG_PMBUS)		+= pmbus_core.o
>>>   obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
>>>   obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
>>> +obj-$(CONFIG_SENSORS_IBMPS)	+= ibmps.o
>>>   obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
>>>   obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
>>>   obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
>> diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
>> new file mode 100644
>> index 0000000..1928dd9
>> --- /dev/null
>> +++ b/drivers/hwmon/pmbus/ibmps.c
>> @@ -0,0 +1,164 @@
>> +/*
>> + * Copyright 2017 IBM Corp.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/module.h>
>> +
>> +#include "pmbus.h"
>> +
> I think a comment is needed here to point out these bits are for
> STATUS_MFR_SPECIFIC.
>
>> +#define IBMPS_MFR_FAN_FAULT			BIT(0)
>>> +#define IBMPS_MFR_THERMAL_FAULT			BIT(1)
>>> +#define IBMPS_MFR_OV_FAULT			BIT(2)
>>> +#define IBMPS_MFR_UV_FAULT			BIT(3)
>>> +#define IBMPS_MFR_PS_KILL			BIT(4)
>>> +#define IBMPS_MFR_OC_FAULT			BIT(5)
>>> +#define IBMPS_MFR_VAUX_FAULT			BIT(6)
>>> +#define IBMPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
>> +
>> +static int ibmps_read_word_data(struct i2c_client *client, int page, int reg);
>> +
> This makes me curious.
>
>> +static int ibmps_read_byte_data(struct i2c_client *client, int page, int reg)
>> +{
>>> +	int rc, mfr;
>> +
>>> +	switch (reg) {
>>> +	case PMBUS_STATUS_BYTE:
>>> +	case PMBUS_STATUS_WORD:
>> +		rc = ibmps_read_word_data(client, page, PMBUS_STATUS_WORD);
> Feels wrong to return "junk" in the result for a PMBUS_STATUS_BYTE
> query.

Good point, I'll mask the byte one.

>
>> +		break;
>>> +	case PMBUS_STATUS_VOUT:
>>> +	case PMBUS_STATUS_IOUT:
>>> +	case PMBUS_STATUS_TEMPERATURE:
>>> +	case PMBUS_STATUS_FAN_12:
>>> +		rc = pmbus_read_byte_data(client, page, reg);
>>> +		if (rc < 0)
>>> +			return rc;
>> +
>>> +		mfr = pmbus_read_byte_data(client, page,
>>> +					   PMBUS_STATUS_MFR_SPECIFIC);
>>> +		if (mfr < 0)
>>> +			return rc;
>> +
>>> +		if (reg == PMBUS_STATUS_FAN_12) {
>>> +			if (mfr & IBMPS_MFR_FAN_FAULT)
>>> +				rc |= PB_FAN_FAN1_FAULT;
>>> +		} else if (reg == PMBUS_STATUS_TEMPERATURE) {
>>> +			if (mfr & IBMPS_MFR_THERMAL_FAULT)
>>> +				rc |= PB_TEMP_OT_FAULT;
>>> +		} else if (reg == PMBUS_STATUS_VOUT) {
>>> +			if (mfr & (IBMPS_MFR_OV_FAULT | IBMPS_MFR_VAUX_FAULT))
>>> +				rc |= PB_VOLTAGE_OV_FAULT;
>>> +			if (mfr & IBMPS_MFR_UV_FAULT)
>>> +				rc |= PB_VOLTAGE_UV_FAULT;
>>> +		} else if (reg == PMBUS_STATUS_IOUT) {
>>> +			if (mfr & IBMPS_MFR_OC_FAULT)
>>> +				rc |= PB_IOUT_OC_FAULT;
>>> +			if (mfr & IBMPS_MFR_CURRENT_SHARE_WARNING)
>> +				rc |= PB_CURRENT_SHARE_FAULT;
>> +		}
> I wonder why it was done this way and not simply via the spec'ed bits?
> Maybe some don't fit, but it seems there's a pretty direct mapping for
> the most part.
>
> Do the spec'ed status bits ever get set? As in, is there ever some
> ambiguity about how the bits got set?

Yes I believe some are set by the "normal" status bits, but I'm not sure 
the coverage is complete, so we definitely need to add the MFR register 
bits as well.

>
>>> +		break;
>>> +	default:
>>> +		if (reg >= PMBUS_VIRT_BASE)
>>> +			return -ENXIO;
>> +
>> +		rc = pmbus_read_byte_data(client, page, reg);
> The alternative here is -ENODATA; not sure which is better though.
>
>> +		break;
>>> +	}
>> +
>>> +	return rc;
>> +}
>> +
>> +static int ibmps_read_word_data(struct i2c_client *client, int page, int reg)
>> +{
>>> +	int rc, mfr;
>> +
>>> +	switch (reg) {
>>> +	case PMBUS_STATUS_BYTE:
>>> +	case PMBUS_STATUS_WORD:
>>> +		rc = pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
>>> +		if (rc < 0)
>>> +			return rc;
>> +
>>> +		mfr = pmbus_read_byte_data(client, page,
>>> +					   PMBUS_STATUS_MFR_SPECIFIC);
>>> +		if (mfr < 0)
>>> +			return rc;
>> +
>>> +		if (mfr & IBMPS_MFR_PS_KILL)
>>> +			rc |= PB_STATUS_OFF;
>> +
>>> +		if (mfr)
>> +			rc |= PB_STATUS_WORD_MFR;
> This isn't set automatically by the device?

I'll have to check on that.

>
>> +		break;
>>> +	default:
>>> +		if (reg >= PMBUS_VIRT_BASE)
>>> +			return -ENXIO;
>> +
>>> +		rc = pmbus_read_word_data(client, page, reg);
>>> +		if (rc < 0)
>> +			rc = ibmps_read_byte_data(client, page, reg);
> Is this really the way we should go about accesses?

The power supply seems to mix up byte and word accesses. For example, 
the actual data regs are words, but the status_* are bytes (except 
status_word, obviously). This was basically the only way I could get 
everything working.

>
>> +		break;
>>> +	}
>> +
>>> +	return rc;
>> +}
>> +
>> +static struct pmbus_driver_info ibmps_info = {
>>> +	.pages = 1,
>>> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
>>> +		PMBUS_HAVE_PIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP |
>>> +		PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
>>> +		PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
>>> +		PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
>>> +	.read_byte_data = ibmps_read_byte_data,
>>> +	.read_word_data = ibmps_read_word_data,
>> +};
>> +
>> +static int ibmps_probe(struct i2c_client *client,
>>> +		       const struct i2c_device_id *id)
>> +{
>> +	return pmbus_do_probe(client, id, &ibmps_info);
> Is there anything we need to check before probe, e.g.
> MFR_ID/MFR_MODEL/MFR_REVISION? Are there options that might need to be
> configured (and exposed via devicetree bindings)?

Nothing in the spec that I see.

>
> I note we support fans: Is FAN_CONFIG_1_2 configured as needed? Or do
> we need to enable the fan, configure PWM vs RPM and the tach pulses
> value? If it is pre-configured, is this register always going to be
> pre-configured? Or do we need a property like my proposed 'use-stored-
> presence' for the MAX31785?

I believe they are all configured internally. FAN_CONFIG_1_2 is listed 
in the spec but doesn't state that anything needs to be setup.

>
> Have you had a look at my proposed bindings for the max31785? I think
> it would be worth your input, as we have another case of fans here, and
> it will help us shape a better picture of whether my generic properties
>   and node structure are appropriate.

Sure, I'll take a look.

>
>> +}
>> +
>> +static int ibmps_remove(struct i2c_client *client)
>> +{
>>> +	return pmbus_do_remove(client);
>> +}
>> +
>> +enum chips { witherspoon };
> This doesn't look necessary.

Agreed, it's somewhat of a pmbus convention, but maybe since there's 
just one I'll remove it.

>
>> +
>> +static const struct i2c_device_id ibmps_id[] = {
>>> +	{ "witherspoon", witherspoon },
>>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ibmps_id);
>> +
>> +static const struct of_device_id ibmps_of_match[] = {
>>> +	{ .compatible = "ibm,ibmps" },
>>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);
> p8_i2c_occ_of_match isn't right. Do we need both device tables?
> "witherspoon" as a device ID also seems unusual. What's going on there?

Uhh, not sure how that got in there... How does this compile?? I'll fix 
it. The power supply doesn't have any device id or anything, so I just 
went with witherspoon...

>
>> +
>> +static struct i2c_driver ibmps_driver = {
>>> +	.driver = {
>>> +		.name = "ibmps",
>>> +		.of_match_table = ibmps_of_match,
>>> +	},
>>> +	.probe = ibmps_probe,
>>> +	.remove = ibmps_remove,
>>> +	.id_table = ibmps_id,
>> +};
>> +
>> +module_i2c_driver(ibmps_driver);
>> +
>> +MODULE_AUTHOR("Eddie James");
>> +MODULE_DESCRIPTION("PMBus driver for IBM power supply");
> So do we anticipate only ever having one power supply design? The name
> feels a bit generic. In the device id table above you suggest it's
> called 'witherspoon'. Is the design specific to Witherspoon? If so this
> fact should be included in the driver name and description.

I was told it is specific to witherspoon. I just hate to make it so 
specific when I'm pretty sure there will be future similar power 
supplies (in fact there was another compatible one that's buggy, present 
on some older witherspoons). But that will work.

>
> If there are other power supplies that are similar, then when we add
> support we can rename the driver, and add any necessary module aliases,
> and have a common device id table listing all of the supported devices.
>
> Cheers,
>
> Andrew
>
>> +MODULE_LICENSE("GPL");

Re: [PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 13102 bytes --]

On Fri, 2017-08-04 at 10:03 -0500, Eddie James wrote:
> 
> On 08/03/2017 07:34 PM, Andrew Jeffery wrote:
> > On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
> > > > From: "Edward A. James" <eajames@us.ibm.com>
> > > 
> > > Add the driver to monitor power supplies with hwmon over pmbus.
> > > 
> > > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > > 
> > > ---
> > >   drivers/hwmon/pmbus/Kconfig  |  10 +++
> > >   drivers/hwmon/pmbus/Makefile |   1 +
> > >   drivers/hwmon/pmbus/ibmps.c  | 164 +++++++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 175 insertions(+)
> > >   create mode 100644 drivers/hwmon/pmbus/ibmps.c
> > > 
> > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > > index 04f6baa..079a087 100644
> > > --- a/drivers/hwmon/pmbus/Kconfig
> > > +++ b/drivers/hwmon/pmbus/Kconfig
> > > @@ -37,6 +37,16 @@ config SENSORS_ADM1275
> > > > > > > >   	  This driver can also be built as a module. If so, the module will
> > > >   	  be called adm1275.
> > > 
> > >   
> > > +config SENSORS_IBMPS
> > > > > > > > +	tristate "IBM Power Supply"
> > > > > > > > +	default n
> > > > > > > > +	help
> > > > > > > > +	  If you say yes here you get hardware monitoring support for IBM
> > > > +	  power supply.
> > > 
> > > +
> > > > > > > > +	  This driver can also be built as a module. If so, the module will
> > > > +	  be called ibmps.
> > > 
> > > +
> > >   config SENSORS_IR35221
> > > > > > > >   	tristate "Infineon IR35221"
> > > >   	default n
> > > 
> > > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > > index 663801c..76d77e0 100644
> > > --- a/drivers/hwmon/pmbus/Makefile
> > > +++ b/drivers/hwmon/pmbus/Makefile
> > > @@ -5,6 +5,7 @@
> > > > > > > >   obj-$(CONFIG_PMBUS)		+= pmbus_core.o
> > > > > > > >   obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
> > > > > > > >   obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
> > > > > > > > +obj-$(CONFIG_SENSORS_IBMPS)	+= ibmps.o
> > > > > > > >   obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
> > > > > > > >   obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
> > > >   obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
> > > 
> > > diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
> > > new file mode 100644
> > > index 0000000..1928dd9
> > > --- /dev/null
> > > +++ b/drivers/hwmon/pmbus/ibmps.c
> > > @@ -0,0 +1,164 @@
> > > +/*
> > > + * Copyright 2017 IBM Corp.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/jiffies.h>
> > > +#include <linux/module.h>
> > > +
> > > +#include "pmbus.h"
> > > +
> > 
> > I think a comment is needed here to point out these bits are for
> > STATUS_MFR_SPECIFIC.
> > 
> > > > > > +#define IBMPS_MFR_FAN_FAULT			BIT(0)
> > > > > > > > +#define IBMPS_MFR_THERMAL_FAULT			BIT(1)
> > > > > > > > +#define IBMPS_MFR_OV_FAULT			BIT(2)
> > > > > > > > +#define IBMPS_MFR_UV_FAULT			BIT(3)
> > > > > > > > +#define IBMPS_MFR_PS_KILL			BIT(4)
> > > > > > > > +#define IBMPS_MFR_OC_FAULT			BIT(5)
> > > > > > > > +#define IBMPS_MFR_VAUX_FAULT			BIT(6)
> > > > +#define IBMPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
> > > 
> > > +
> > > +static int ibmps_read_word_data(struct i2c_client *client, int page, int reg);
> > > +
> > 
> > This makes me curious.
> > 
> > > +static int ibmps_read_byte_data(struct i2c_client *client, int page, int reg)
> > > +{
> > > > +	int rc, mfr;
> > > 
> > > +
> > > > > > > > +	switch (reg) {
> > > > > > > > +	case PMBUS_STATUS_BYTE:
> > > > +	case PMBUS_STATUS_WORD:
> > > 
> > > +		rc = ibmps_read_word_data(client, page, PMBUS_STATUS_WORD);
> > 
> > Feels wrong to return "junk" in the result for a PMBUS_STATUS_BYTE
> > query.
> 
> Good point, I'll mask the byte one.
> 
> > 
> > > > > > +		break;
> > > > > > > > +	case PMBUS_STATUS_VOUT:
> > > > > > > > +	case PMBUS_STATUS_IOUT:
> > > > > > > > +	case PMBUS_STATUS_TEMPERATURE:
> > > > > > > > +	case PMBUS_STATUS_FAN_12:
> > > > > > > > +		rc = pmbus_read_byte_data(client, page, reg);
> > > > > > > > +		if (rc < 0)
> > > > +			return rc;
> > > 
> > > +
> > > > > > > > +		mfr = pmbus_read_byte_data(client, page,
> > > > > > > > +					   PMBUS_STATUS_MFR_SPECIFIC);
> > > > > > > > +		if (mfr < 0)
> > > > +			return rc;
> > > 
> > > +
> > > > > > > > +		if (reg == PMBUS_STATUS_FAN_12) {
> > > > > > > > +			if (mfr & IBMPS_MFR_FAN_FAULT)
> > > > > > > > +				rc |= PB_FAN_FAN1_FAULT;
> > > > > > > > +		} else if (reg == PMBUS_STATUS_TEMPERATURE) {
> > > > > > > > +			if (mfr & IBMPS_MFR_THERMAL_FAULT)
> > > > > > > > +				rc |= PB_TEMP_OT_FAULT;
> > > > > > > > +		} else if (reg == PMBUS_STATUS_VOUT) {
> > > > > > > > +			if (mfr & (IBMPS_MFR_OV_FAULT | IBMPS_MFR_VAUX_FAULT))
> > > > > > > > +				rc |= PB_VOLTAGE_OV_FAULT;
> > > > > > > > +			if (mfr & IBMPS_MFR_UV_FAULT)
> > > > > > > > +				rc |= PB_VOLTAGE_UV_FAULT;
> > > > > > > > +		} else if (reg == PMBUS_STATUS_IOUT) {
> > > > > > > > +			if (mfr & IBMPS_MFR_OC_FAULT)
> > > > > > > > +				rc |= PB_IOUT_OC_FAULT;
> > > > +			if (mfr & IBMPS_MFR_CURRENT_SHARE_WARNING)
> > > 
> > > > > > +				rc |= PB_CURRENT_SHARE_FAULT;
> > > +		}
> > 
> > I wonder why it was done this way and not simply via the spec'ed bits?
> > Maybe some don't fit, but it seems there's a pretty direct mapping for
> > the most part.
> > 
> > Do the spec'ed status bits ever get set? As in, is there ever some
> > ambiguity about how the bits got set?
> 
> Yes I believe some are set by the "normal" status bits, but I'm not sure 
> the coverage is complete, so we definitely need to add the MFR register 
> bits as well.

Okay, I'll have a look at the datasheet.

> 
> > 
> > > > > > > > +		break;
> > > > > > > > +	default:
> > > > > > > > +		if (reg >= PMBUS_VIRT_BASE)
> > > > +			return -ENXIO;
> > > 
> > > +
> > > +		rc = pmbus_read_byte_data(client, page, reg);
> > 
> > The alternative here is -ENODATA; not sure which is better though.
> > 
> > > > > > +		break;
> > > > +	}
> > > 
> > > +
> > > > +	return rc;
> > > 
> > > +}
> > > +
> > > +static int ibmps_read_word_data(struct i2c_client *client, int page, int reg)
> > > +{
> > > > +	int rc, mfr;
> > > 
> > > +
> > > > > > > > +	switch (reg) {
> > > > > > > > +	case PMBUS_STATUS_BYTE:
> > > > > > > > +	case PMBUS_STATUS_WORD:
> > > > > > > > +		rc = pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
> > > > > > > > +		if (rc < 0)
> > > > +			return rc;
> > > 
> > > +
> > > > > > > > +		mfr = pmbus_read_byte_data(client, page,
> > > > > > > > +					   PMBUS_STATUS_MFR_SPECIFIC);
> > > > > > > > +		if (mfr < 0)
> > > > +			return rc;
> > > 
> > > +
> > > > > > > > +		if (mfr & IBMPS_MFR_PS_KILL)
> > > > +			rc |= PB_STATUS_OFF;
> > > 
> > > +
> > > > +		if (mfr)
> > > 
> > > +			rc |= PB_STATUS_WORD_MFR;
> > 
> > This isn't set automatically by the device?
> 
> I'll have to check on that.
> 
> > 
> > > > > > +		break;
> > > > > > > > +	default:
> > > > > > > > +		if (reg >= PMBUS_VIRT_BASE)
> > > > +			return -ENXIO;
> > > 
> > > +
> > > > > > > > +		rc = pmbus_read_word_data(client, page, reg);
> > > > +		if (rc < 0)
> > > 
> > > +			rc = ibmps_read_byte_data(client, page, reg);
> > 
> > Is this really the way we should go about accesses?
> 
> The power supply seems to mix up byte and word accesses. For example, 
> the actual data regs are words, but the status_* are bytes (except 
> status_word, obviously). This was basically the only way I could get 
> everything working.

I don't understand - even if you read a word register with a byte-
accessor everything should still work (aside from not reading the
amount you're *meant* to).

So this still seems odd to me. Can you rephrase or expand on what
you've said?

> 
> > 
> > > > > > +		break;
> > > > +	}
> > > 
> > > +
> > > > +	return rc;
> > > 
> > > +}
> > > +
> > > +static struct pmbus_driver_info ibmps_info = {
> > > > > > > > +	.pages = 1,
> > > > > > > > +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > > > > > > > +		PMBUS_HAVE_PIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP |
> > > > > > > > +		PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
> > > > > > > > +		PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
> > > > > > > > +		PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
> > > > > > > > +	.read_byte_data = ibmps_read_byte_data,
> > > > +	.read_word_data = ibmps_read_word_data,
> > > 
> > > +};
> > > +
> > > +static int ibmps_probe(struct i2c_client *client,
> > > > +		       const struct i2c_device_id *id)
> > > 
> > > +{
> > > +	return pmbus_do_probe(client, id, &ibmps_info);
> > 
> > Is there anything we need to check before probe, e.g.
> > MFR_ID/MFR_MODEL/MFR_REVISION? Are there options that might need to be
> > configured (and exposed via devicetree bindings)?
> 
> Nothing in the spec that I see.

Okay.

> 
> > 
> > I note we support fans: Is FAN_CONFIG_1_2 configured as needed? Or do
> > we need to enable the fan, configure PWM vs RPM and the tach pulses
> > value? If it is pre-configured, is this register always going to be
> > pre-configured? Or do we need a property like my proposed 'use-stored-
> > presence' for the MAX31785?
> 
> I believe they are all configured internally. FAN_CONFIG_1_2 is listed 
> in the spec but doesn't state that anything needs to be setup.
> 
> > 
> > Have you had a look at my proposed bindings for the max31785? I think
> > it would be worth your input, as we have another case of fans here, and
> > it will help us shape a better picture of whether my generic properties
> >   and node structure are appropriate.
> 
> Sure, I'll take a look.
> 

Thanks.

> > 
> > > +}
> > > +
> > > +static int ibmps_remove(struct i2c_client *client)
> > > +{
> > > > +	return pmbus_do_remove(client);
> > > 
> > > +}
> > > +
> > > +enum chips { witherspoon };
> > 
> > This doesn't look necessary.
> 
> Agreed, it's somewhat of a pmbus convention, but maybe since there's 
> just one I'll remove it.

Right, at least IMO it's better to add it when we come to needing it.

> 
> > 
> > > +
> > > +static const struct i2c_device_id ibmps_id[] = {
> > > > > > > > +	{ "witherspoon", witherspoon },
> > > > +	{ }
> > > 
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, ibmps_id);
> > > +
> > > +static const struct of_device_id ibmps_of_match[] = {
> > > > > > > > +	{ .compatible = "ibm,ibmps" },
> > > > +	{}
> > > 
> > > +};
> > > +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);
> > 
> > p8_i2c_occ_of_match isn't right. Do we need both device tables?
> > "witherspoon" as a device ID also seems unusual. What's going on there?
> 
> Uhh, not sure how that got in there... How does this compile?? I'll fix 
> it. The power supply doesn't have any device id or anything, so I just 
> went with witherspoon...

My issue is "withersoon" identifies a class of machine, not a power
supply, even if the powersupply is specific to Witherspoon.

> 
> > 
> > > +
> > > +static struct i2c_driver ibmps_driver = {
> > > > > > > > +	.driver = {
> > > > > > > > +		.name = "ibmps",
> > > > > > > > +		.of_match_table = ibmps_of_match,
> > > > > > > > +	},
> > > > > > > > +	.probe = ibmps_probe,
> > > > > > > > +	.remove = ibmps_remove,
> > > > +	.id_table = ibmps_id,
> > > 
> > > +};
> > > +
> > > +module_i2c_driver(ibmps_driver);
> > > +
> > > +MODULE_AUTHOR("Eddie James");
> > > +MODULE_DESCRIPTION("PMBus driver for IBM power supply");
> > 
> > So do we anticipate only ever having one power supply design? The name
> > feels a bit generic. In the device id table above you suggest it's
> > called 'witherspoon'. Is the design specific to Witherspoon? If so this
> > fact should be included in the driver name and description.
> 
> I was told it is specific to witherspoon. I just hate to make it so 
> specific when I'm pretty sure there will be future similar power 
> supplies (in fact there was another compatible one that's buggy, present 
> on some older witherspoons). But that will work.

Right, but if we can't really claim support power supplies that don't
yet exist. I think we should scope ourselves to what we currently have.

Cheers,

Andrew

> 
> > 
> > If there are other power supplies that are similar, then when we add
> > support we can rename the driver, and add any necessary module aliases,
> > and have a common device id table listing all of the supported devices.
> > 
> > Cheers,
> > 
> > Andrew
> > 
> > > +MODULE_LICENSE("GPL");
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

On Mon, 2017-08-07 at 15:11 +0930, Andrew Jeffery wrote:
> > 
> > > 
> > > > +
> > > > +static const struct i2c_device_id ibmps_id[] = {
> > > > > > > > > +       { "witherspoon", witherspoon },
> > > > > +       { }
> > > > 
> > > > +};
> > > > +MODULE_DEVICE_TABLE(i2c, ibmps_id);
> > > > +
> > > > +static const struct of_device_id ibmps_of_match[] = {
> > > > > > > > > +       { .compatible = "ibm,ibmps" },
> > > > > +       {}
> > > > 
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);
> > > 
> > > p8_i2c_occ_of_match isn't right. Do we need both device tables?
> > > "witherspoon" as a device ID also seems unusual. What's going on there?
> > 
> > Uhh, not sure how that got in there... How does this compile?? I'll fix 
> > it. The power supply doesn't have any device id or anything, so I just 
> > went with witherspoon...
> 
> My issue is "withersoon" identifies a class of machine, not a power
> supply, even if the powersupply is specific to Witherspoon.

Further to this, I've just got hold of the datasheet. My reading of it
says the part number is "01KL054". Maybe the driver should include that
in its name and compatible.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Brandon Wyman]

On Mon, Aug 7, 2017 at 9:34 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Mon, 2017-08-07 at 15:11 +0930, Andrew Jeffery wrote:
>> >
>> > >
>> > > > +
>> > > > +static const struct i2c_device_id ibmps_id[] = {
>> > > > > > > > > +       { "witherspoon", witherspoon },
>> > > > > +       { }
>> > > >
>> > > > +};
>> > > > +MODULE_DEVICE_TABLE(i2c, ibmps_id);
>> > > > +
>> > > > +static const struct of_device_id ibmps_of_match[] = {
>> > > > > > > > > +       { .compatible = "ibm,ibmps" },
>> > > > > +       {}
>> > > >
>> > > > +};
>> > > > +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);
>> > >
>> > > p8_i2c_occ_of_match isn't right. Do we need both device tables?
>> > > "witherspoon" as a device ID also seems unusual. What's going on there?
>> >
>> > Uhh, not sure how that got in there... How does this compile?? I'll fix
>> > it. The power supply doesn't have any device id or anything, so I just
>> > went with witherspoon...
>>
>> My issue is "withersoon" identifies a class of machine, not a power
>> supply, even if the powersupply is specific to Witherspoon.
>
> Further to this, I've just got hold of the datasheet. My reading of it
> says the part number is "01KL054". Maybe the driver should include that
> in its name and compatible.
I don't know that this part number value always remains the same, for
the part used in a particular system. There was a similar 2000W PMBus
power supply that had one part number, and it stuck. However, a
previous 1720W power supply had at least four different part numbers
before it finally stuck on the final one.

That being said, there quite probably are some differences between
various part numbers, but most of what this device driver is doing
would still be compatible. I can see have 01KL054 in the compatible
list, but trying to shoe-horn it into the device driver name doesn't
feel like it makes sense to me.

>
> Andrew

Re: [PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

On Wed, 2017-08-09 at 17:43 -0500, Brandon Wyman wrote:
> > On Mon, Aug 7, 2017 at 9:34 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > On Mon, 2017-08-07 at 15:11 +0930, Andrew Jeffery wrote:
> > > > 
> > > > > 
> > > > > > +
> > > > > > +static const struct i2c_device_id ibmps_id[] = {
> > > > > > > > > > > +       { "witherspoon", witherspoon },
> > > > > > > 
> > > > > > > +       { }
> > > > > > 
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(i2c, ibmps_id);
> > > > > > +
> > > > > > +static const struct of_device_id ibmps_of_match[] = {
> > > > > > > > > > > +       { .compatible = "ibm,ibmps" },
> > > > > > > 
> > > > > > > +       {}
> > > > > > 
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);
> > > > > 
> > > > > p8_i2c_occ_of_match isn't right. Do we need both device tables?
> > > > > "witherspoon" as a device ID also seems unusual. What's going on there?
> > > > 
> > > > Uhh, not sure how that got in there... How does this compile?? I'll fix
> > > > it. The power supply doesn't have any device id or anything, so I just
> > > > went with witherspoon...
> > > 
> > > My issue is "withersoon" identifies a class of machine, not a power
> > > supply, even if the powersupply is specific to Witherspoon.
> > 
> > Further to this, I've just got hold of the datasheet. My reading of it
> > says the part number is "01KL054". Maybe the driver should include that
> > in its name and compatible.
> 
> I don't know that this part number value always remains the same, for
> the part used in a particular system. There was a similar 2000W PMBus
> power supply that had one part number, and it stuck. However, a
> previous 1720W power supply had at least four different part numbers
> before it finally stuck on the final one.
> 
> That being said, there quite probably are some differences between
> various part numbers, but most of what this device driver is doing
> would still be compatible. I can see have 01KL054 in the compatible
> list, but trying to shoe-horn it into the device driver name doesn't
> feel like it makes sense to me.

Right; without the context you've provided above I wouldn't consider it
shoe-horning. However, since having a diversity of part numbers is a
thing, I agree with your assessment. It should be part of a compatible,
but maybe we do need something generic for the driver name.

Andrew

> 
> > 
> > Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH linux dev-4.10 v3 2/5] drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes

[Eddie James]

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

Add sysfs entries to dump out PS registers and clear faults.

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

diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
index 1928dd9..f55d2bf 100644
--- a/drivers/hwmon/pmbus/ibmps.c
+++ b/drivers/hwmon/pmbus/ibmps.c
@@ -9,8 +9,10 @@
 
 #include <linux/device.h>
 #include <linux/i2c.h>
+#include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/module.h>
+#include <linux/sysfs.h>
 
 #include "pmbus.h"
 
@@ -122,14 +124,90 @@ static int ibmps_read_word_data(struct i2c_client *client, int page, int reg)
 	.read_word_data = ibmps_read_word_data,
 };
 
+static ssize_t ibmps_clear_faults(struct device *dev,
+				  struct device_attribute *dev_attr,
+				  const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	pmbus_clear_faults(client);
+
+	return count;
+}
+static DEVICE_ATTR(clear_faults, 0200, NULL, ibmps_clear_faults);
+
+static ssize_t ibmps_show_status_word(struct device *dev,
+				      struct device_attribute *dev_attr,
+				      char *buf)
+{
+	int rc;
+	struct i2c_client *client = to_i2c_client(dev);
+
+	rc = pmbus_read_word_data(client, 0, PMBUS_STATUS_WORD);
+	if (rc < 0)
+		return rc;
+
+	return snprintf(buf, PAGE_SIZE - 1, "%04x\n", rc);
+}
+static DEVICE_ATTR(status_word, 0444, ibmps_show_status_word, NULL);
+
+static ssize_t ibmps_show_status_byte(struct device *dev,
+				      struct device_attribute *dev_attr,
+				      char *buf)
+{
+	int rc;
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sensor_device_attribute *sattr = to_sensor_dev_attr(dev_attr);
+
+	rc = pmbus_read_byte_data(client, 0, PMBUS_STATUS_VOUT + sattr->index);
+	if (rc < 0)
+		return rc;
+
+	return snprintf(buf, PAGE_SIZE - 1, "%02x\n", rc);
+}
+
+static SENSOR_DEVICE_ATTR(status_vout, 0444, ibmps_show_status_byte, NULL, 0);
+static SENSOR_DEVICE_ATTR(status_iout, 0444, ibmps_show_status_byte, NULL, 1);
+static SENSOR_DEVICE_ATTR(status_input, 0444, ibmps_show_status_byte, NULL, 2);
+static SENSOR_DEVICE_ATTR(status_temp, 0444, ibmps_show_status_byte, NULL, 3);
+static SENSOR_DEVICE_ATTR(status_cml, 0444, ibmps_show_status_byte, NULL, 4);
+static SENSOR_DEVICE_ATTR(status_other, 0444, ibmps_show_status_byte, NULL, 5);
+static SENSOR_DEVICE_ATTR(status_mfr, 0444, ibmps_show_status_byte, NULL, 6);
+static SENSOR_DEVICE_ATTR(status_fan, 0444, ibmps_show_status_byte, NULL, 7);
+
+static struct attribute *ibmps_attributes[] = {
+	&dev_attr_clear_faults.attr,
+	&dev_attr_status_word.attr,
+	&sensor_dev_attr_status_vout.dev_attr.attr,
+	&sensor_dev_attr_status_iout.dev_attr.attr,
+	&sensor_dev_attr_status_input.dev_attr.attr,
+	&sensor_dev_attr_status_temp.dev_attr.attr,
+	&sensor_dev_attr_status_cml.dev_attr.attr,
+	&sensor_dev_attr_status_other.dev_attr.attr,
+	&sensor_dev_attr_status_mfr.dev_attr.attr,
+	&sensor_dev_attr_status_fan.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ibmps_attribute_group = {
+	.attrs = ibmps_attributes,
+};
+
 static int ibmps_probe(struct i2c_client *client,
 		       const struct i2c_device_id *id)
 {
+	int rc = sysfs_create_group(&client->dev.kobj,
+				    &ibmps_attribute_group);
+	if (rc)
+		return rc;
+
 	return pmbus_do_probe(client, id, &ibmps_info);
 }
 
 static int ibmps_remove(struct i2c_client *client)
 {
+	sysfs_remove_group(&client->dev.kobj, &ibmps_attribute_group);
+
 	return pmbus_do_remove(client);
 }
 
-- 
1.8.3.1

Re: [PATCH linux dev-4.10 v3 2/5] drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4664 bytes --]

On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
> > From: "Edward A. James" <eajames@us.ibm.com>
> 
> Add sysfs entries to dump out PS registers

Why are we working against what the pmbus subsystem already gives us?
Why not augment support in pmbus core to give us the information we
need in a manner acceptable to upstream?

Flipping to the other end of the spectrum: The device only has one
page. We can just use `i2cget -f -y ...` without risk of corruption.

>  and clear faults.

Can you describe the motivation for this? I feel like it would be worth
having this as a separate patch, and to the core rather than in an
arbitrary driver.

Cheers,

Andrew

> 
> > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/hwmon/pmbus/ibmps.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
> index 1928dd9..f55d2bf 100644
> --- a/drivers/hwmon/pmbus/ibmps.c
> +++ b/drivers/hwmon/pmbus/ibmps.c
> @@ -9,8 +9,10 @@
>  
>  #include <linux/device.h>
>  #include <linux/i2c.h>
> +#include <linux/hwmon-sysfs.h>
>  #include <linux/jiffies.h>
>  #include <linux/module.h>
> +#include <linux/sysfs.h>
>  
>  #include "pmbus.h"
>  
> @@ -122,14 +124,90 @@ static int ibmps_read_word_data(struct i2c_client *client, int page, int reg)
> >  	.read_word_data = ibmps_read_word_data,
>  };
>  
> +static ssize_t ibmps_clear_faults(struct device *dev,
> > +				  struct device_attribute *dev_attr,
> > +				  const char *buf, size_t count)
> +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> +
> > +	pmbus_clear_faults(client);
> +
> > +	return count;
> +}
> +static DEVICE_ATTR(clear_faults, 0200, NULL, ibmps_clear_faults);
> +
> +static ssize_t ibmps_show_status_word(struct device *dev,
> > +				      struct device_attribute *dev_attr,
> > +				      char *buf)
> +{
> > +	int rc;
> > +	struct i2c_client *client = to_i2c_client(dev);
> +
> > +	rc = pmbus_read_word_data(client, 0, PMBUS_STATUS_WORD);
> > +	if (rc < 0)
> > +		return rc;
> +
> > +	return snprintf(buf, PAGE_SIZE - 1, "%04x\n", rc);
> +}
> +static DEVICE_ATTR(status_word, 0444, ibmps_show_status_word, NULL);
> +
> +static ssize_t ibmps_show_status_byte(struct device *dev,
> > +				      struct device_attribute *dev_attr,
> > +				      char *buf)
> +{
> > +	int rc;
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(dev_attr);
> +
> > +	rc = pmbus_read_byte_data(client, 0, PMBUS_STATUS_VOUT + sattr->index);
> > +	if (rc < 0)
> > +		return rc;
> +
> > +	return snprintf(buf, PAGE_SIZE - 1, "%02x\n", rc);
> +}
> +
> +static SENSOR_DEVICE_ATTR(status_vout, 0444, ibmps_show_status_byte, NULL, 0);
> +static SENSOR_DEVICE_ATTR(status_iout, 0444, ibmps_show_status_byte, NULL, 1);
> +static SENSOR_DEVICE_ATTR(status_input, 0444, ibmps_show_status_byte, NULL, 2);
> +static SENSOR_DEVICE_ATTR(status_temp, 0444, ibmps_show_status_byte, NULL, 3);
> +static SENSOR_DEVICE_ATTR(status_cml, 0444, ibmps_show_status_byte, NULL, 4);
> +static SENSOR_DEVICE_ATTR(status_other, 0444, ibmps_show_status_byte, NULL, 5);
> +static SENSOR_DEVICE_ATTR(status_mfr, 0444, ibmps_show_status_byte, NULL, 6);
> +static SENSOR_DEVICE_ATTR(status_fan, 0444, ibmps_show_status_byte, NULL, 7);
> +
> +static struct attribute *ibmps_attributes[] = {
> > +	&dev_attr_clear_faults.attr,
> > +	&dev_attr_status_word.attr,
> > +	&sensor_dev_attr_status_vout.dev_attr.attr,
> > +	&sensor_dev_attr_status_iout.dev_attr.attr,
> > +	&sensor_dev_attr_status_input.dev_attr.attr,
> > +	&sensor_dev_attr_status_temp.dev_attr.attr,
> > +	&sensor_dev_attr_status_cml.dev_attr.attr,
> > +	&sensor_dev_attr_status_other.dev_attr.attr,
> > +	&sensor_dev_attr_status_mfr.dev_attr.attr,
> > +	&sensor_dev_attr_status_fan.dev_attr.attr,
> > +	NULL
> +};
> +
> +static const struct attribute_group ibmps_attribute_group = {
> > +	.attrs = ibmps_attributes,
> +};
> +
>  static int ibmps_probe(struct i2c_client *client,
> >  		       const struct i2c_device_id *id)
>  {
> > +	int rc = sysfs_create_group(&client->dev.kobj,
> > +				    &ibmps_attribute_group);
> > +	if (rc)
> > +		return rc;
> +
> >  	return pmbus_do_probe(client, id, &ibmps_info);
>  }
>  
>  static int ibmps_remove(struct i2c_client *client)
>  {
> > +	sysfs_remove_group(&client->dev.kobj, &ibmps_attribute_group);
> +
> >  	return pmbus_do_remove(client);
>  }
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 2/5] drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes

[Eddie James]

On 08/03/2017 07:33 PM, Andrew Jeffery wrote:
> On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
>>> From: "Edward A. James" <eajames@us.ibm.com>
>> Add sysfs entries to dump out PS registers
> Why are we working against what the pmbus subsystem already gives us?
> Why not augment support in pmbus core to give us the information we
> need in a manner acceptable to upstream?

Yea, that would be great. I feel like we're a little short on time for 
that, as we need this functionality basically now. But we can give it a 
shot.

>
> Flipping to the other end of the spectrum: The device only has one
> page. We can just use `i2cget -f -y ...` without risk of corruption.

Yea, I just get a lot of requests to have it all in the device driver. I 
guess we need to just push back and ask people to use i2c tools. But we 
will have to solve this for the UCD driver.

>
>>   and clear faults.
> Can you describe the motivation for this? I feel like it would be worth
> having this as a separate patch, and to the core rather than in an
> arbitrary driver.

Users need to be able to clear faults, that was just on my list of 
requirements :) They could probably use i2cset though.

>
> Cheers,
>
> Andrew
>
>>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/hwmon/pmbus/ibmps.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>
>> diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
>> index 1928dd9..f55d2bf 100644
>> --- a/drivers/hwmon/pmbus/ibmps.c
>> +++ b/drivers/hwmon/pmbus/ibmps.c
>> @@ -9,8 +9,10 @@
>>   
>>   #include <linux/device.h>
>>   #include <linux/i2c.h>
>> +#include <linux/hwmon-sysfs.h>
>>   #include <linux/jiffies.h>
>>   #include <linux/module.h>
>> +#include <linux/sysfs.h>
>>   
>>   #include "pmbus.h"
>>   
>> @@ -122,14 +124,90 @@ static int ibmps_read_word_data(struct i2c_client *client, int page, int reg)
>>>   	.read_word_data = ibmps_read_word_data,
>>   };
>>   
>> +static ssize_t ibmps_clear_faults(struct device *dev,
>>> +				  struct device_attribute *dev_attr,
>>> +				  const char *buf, size_t count)
>> +{
>>> +	struct i2c_client *client = to_i2c_client(dev);
>> +
>>> +	pmbus_clear_faults(client);
>> +
>>> +	return count;
>> +}
>> +static DEVICE_ATTR(clear_faults, 0200, NULL, ibmps_clear_faults);
>> +
>> +static ssize_t ibmps_show_status_word(struct device *dev,
>>> +				      struct device_attribute *dev_attr,
>>> +				      char *buf)
>> +{
>>> +	int rc;
>>> +	struct i2c_client *client = to_i2c_client(dev);
>> +
>>> +	rc = pmbus_read_word_data(client, 0, PMBUS_STATUS_WORD);
>>> +	if (rc < 0)
>>> +		return rc;
>> +
>>> +	return snprintf(buf, PAGE_SIZE - 1, "%04x\n", rc);
>> +}
>> +static DEVICE_ATTR(status_word, 0444, ibmps_show_status_word, NULL);
>> +
>> +static ssize_t ibmps_show_status_byte(struct device *dev,
>>> +				      struct device_attribute *dev_attr,
>>> +				      char *buf)
>> +{
>>> +	int rc;
>>> +	struct i2c_client *client = to_i2c_client(dev);
>>> +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(dev_attr);
>> +
>>> +	rc = pmbus_read_byte_data(client, 0, PMBUS_STATUS_VOUT + sattr->index);
>>> +	if (rc < 0)
>>> +		return rc;
>> +
>>> +	return snprintf(buf, PAGE_SIZE - 1, "%02x\n", rc);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(status_vout, 0444, ibmps_show_status_byte, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(status_iout, 0444, ibmps_show_status_byte, NULL, 1);
>> +static SENSOR_DEVICE_ATTR(status_input, 0444, ibmps_show_status_byte, NULL, 2);
>> +static SENSOR_DEVICE_ATTR(status_temp, 0444, ibmps_show_status_byte, NULL, 3);
>> +static SENSOR_DEVICE_ATTR(status_cml, 0444, ibmps_show_status_byte, NULL, 4);
>> +static SENSOR_DEVICE_ATTR(status_other, 0444, ibmps_show_status_byte, NULL, 5);
>> +static SENSOR_DEVICE_ATTR(status_mfr, 0444, ibmps_show_status_byte, NULL, 6);
>> +static SENSOR_DEVICE_ATTR(status_fan, 0444, ibmps_show_status_byte, NULL, 7);
>> +
>> +static struct attribute *ibmps_attributes[] = {
>>> +	&dev_attr_clear_faults.attr,
>>> +	&dev_attr_status_word.attr,
>>> +	&sensor_dev_attr_status_vout.dev_attr.attr,
>>> +	&sensor_dev_attr_status_iout.dev_attr.attr,
>>> +	&sensor_dev_attr_status_input.dev_attr.attr,
>>> +	&sensor_dev_attr_status_temp.dev_attr.attr,
>>> +	&sensor_dev_attr_status_cml.dev_attr.attr,
>>> +	&sensor_dev_attr_status_other.dev_attr.attr,
>>> +	&sensor_dev_attr_status_mfr.dev_attr.attr,
>>> +	&sensor_dev_attr_status_fan.dev_attr.attr,
>>> +	NULL
>> +};
>> +
>> +static const struct attribute_group ibmps_attribute_group = {
>>> +	.attrs = ibmps_attributes,
>> +};
>> +
>>   static int ibmps_probe(struct i2c_client *client,
>>>   		       const struct i2c_device_id *id)
>>   {
>>> +	int rc = sysfs_create_group(&client->dev.kobj,
>>> +				    &ibmps_attribute_group);
>>> +	if (rc)
>>> +		return rc;
>> +
>>>   	return pmbus_do_probe(client, id, &ibmps_info);
>>   }
>>   
>>   static int ibmps_remove(struct i2c_client *client)
>>   {
>>> +	sysfs_remove_group(&client->dev.kobj, &ibmps_attribute_group);
>> +
>>>   	return pmbus_do_remove(client);
>>   }
>>   

Re: [PATCH linux dev-4.10 v3 2/5] drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes

[Matt Spinler]

On 8/4/2017 10:16 AM, Eddie James wrote:
>
>
> On 08/03/2017 07:33 PM, Andrew Jeffery wrote:
>> On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
>>>> From: "Edward A. James" <eajames@us.ibm.com>
>>> Add sysfs entries to dump out PS registers
>> Why are we working against what the pmbus subsystem already gives us?
>> Why not augment support in pmbus core to give us the information we
>> need in a manner acceptable to upstream?
>
> Yea, that would be great. I feel like we're a little short on time for 
> that, as we need this functionality basically now. But we can give it 
> a shot.
>
>>
>> Flipping to the other end of the spectrum: The device only has one
>> page. We can just use `i2cget -f -y ...` without risk of corruption.
>
> Yea, I just get a lot of requests to have it all in the device driver. 
> I guess we need to just push back and ask people to use i2c tools. But 
> we will have to solve this for the UCD driver.
In general, IBM likes to capture a lot of registers on failures so we 
can actually figure out what went wrong after the fact, as we have 
customers who get mad when we can't give them root cause.  It's a bummer 
that is so hard to do here.  And if we had to use the i2cdev stuff for 
that, then why even use a device driver for any of this...


>
>>
>>>   and clear faults.
>> Can you describe the motivation for this? I feel like it would be worth
>> having this as a separate patch, and to the core rather than in an
>> arbitrary driver.
>
> Users need to be able to clear faults, that was just on my list of 
> requirements :) They could probably use i2cset though.

Yea, userspace needs this.  I'm kinda surprised there wasn't already a 
way to expose it.

>
>>
>> Cheers,
>>
>> Andrew
>>
>>>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>>> ---
>>>   drivers/hwmon/pmbus/ibmps.c | 78 
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 78 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
>>> index 1928dd9..f55d2bf 100644
>>> --- a/drivers/hwmon/pmbus/ibmps.c
>>> +++ b/drivers/hwmon/pmbus/ibmps.c
>>> @@ -9,8 +9,10 @@
>>>     #include <linux/device.h>
>>>   #include <linux/i2c.h>
>>> +#include <linux/hwmon-sysfs.h>
>>>   #include <linux/jiffies.h>
>>>   #include <linux/module.h>
>>> +#include <linux/sysfs.h>
>>>     #include "pmbus.h"
>>>   @@ -122,14 +124,90 @@ static int ibmps_read_word_data(struct 
>>> i2c_client *client, int page, int reg)
>>>>       .read_word_data = ibmps_read_word_data,
>>>   };
>>>   +static ssize_t ibmps_clear_faults(struct device *dev,
>>>> +                  struct device_attribute *dev_attr,
>>>> +                  const char *buf, size_t count)
>>> +{
>>>> +    struct i2c_client *client = to_i2c_client(dev);
>>> +
>>>> +    pmbus_clear_faults(client);
>>> +
>>>> +    return count;
>>> +}
>>> +static DEVICE_ATTR(clear_faults, 0200, NULL, ibmps_clear_faults);
>>> +
>>> +static ssize_t ibmps_show_status_word(struct device *dev,
>>>> +                      struct device_attribute *dev_attr,
>>>> +                      char *buf)
>>> +{
>>>> +    int rc;
>>>> +    struct i2c_client *client = to_i2c_client(dev);
>>> +
>>>> +    rc = pmbus_read_word_data(client, 0, PMBUS_STATUS_WORD);
>>>> +    if (rc < 0)
>>>> +        return rc;
>>> +
>>>> +    return snprintf(buf, PAGE_SIZE - 1, "%04x\n", rc);
>>> +}
>>> +static DEVICE_ATTR(status_word, 0444, ibmps_show_status_word, NULL);
>>> +
>>> +static ssize_t ibmps_show_status_byte(struct device *dev,
>>>> +                      struct device_attribute *dev_attr,
>>>> +                      char *buf)
>>> +{
>>>> +    int rc;
>>>> +    struct i2c_client *client = to_i2c_client(dev);
>>>> +    struct sensor_device_attribute *sattr = 
>>>> to_sensor_dev_attr(dev_attr);
>>> +
>>>> +    rc = pmbus_read_byte_data(client, 0, PMBUS_STATUS_VOUT + 
>>>> sattr->index);
>>>> +    if (rc < 0)
>>>> +        return rc;
>>> +
>>>> +    return snprintf(buf, PAGE_SIZE - 1, "%02x\n", rc);
>>> +}
>>> +
>>> +static SENSOR_DEVICE_ATTR(status_vout, 0444, 
>>> ibmps_show_status_byte, NULL, 0);
>>> +static SENSOR_DEVICE_ATTR(status_iout, 0444, 
>>> ibmps_show_status_byte, NULL, 1);
>>> +static SENSOR_DEVICE_ATTR(status_input, 0444, 
>>> ibmps_show_status_byte, NULL, 2);
>>> +static SENSOR_DEVICE_ATTR(status_temp, 0444, 
>>> ibmps_show_status_byte, NULL, 3);
>>> +static SENSOR_DEVICE_ATTR(status_cml, 0444, ibmps_show_status_byte, 
>>> NULL, 4);
>>> +static SENSOR_DEVICE_ATTR(status_other, 0444, 
>>> ibmps_show_status_byte, NULL, 5);
>>> +static SENSOR_DEVICE_ATTR(status_mfr, 0444, ibmps_show_status_byte, 
>>> NULL, 6);
>>> +static SENSOR_DEVICE_ATTR(status_fan, 0444, ibmps_show_status_byte, 
>>> NULL, 7);
>>> +
>>> +static struct attribute *ibmps_attributes[] = {
>>>> +    &dev_attr_clear_faults.attr,
>>>> +    &dev_attr_status_word.attr,
>>>> +    &sensor_dev_attr_status_vout.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_iout.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_input.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_temp.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_cml.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_other.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_mfr.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_fan.dev_attr.attr,
>>>> +    NULL
>>> +};
>>> +
>>> +static const struct attribute_group ibmps_attribute_group = {
>>>> +    .attrs = ibmps_attributes,
>>> +};
>>> +
>>>   static int ibmps_probe(struct i2c_client *client,
>>>>                  const struct i2c_device_id *id)
>>>   {
>>>> +    int rc = sysfs_create_group(&client->dev.kobj,
>>>> +                    &ibmps_attribute_group);
>>>> +    if (rc)
>>>> +        return rc;
>>> +
>>>>       return pmbus_do_probe(client, id, &ibmps_info);
>>>   }
>>>     static int ibmps_remove(struct i2c_client *client)
>>>   {
>>>> + sysfs_remove_group(&client->dev.kobj, &ibmps_attribute_group);
>>> +
>>>>       return pmbus_do_remove(client);
>>>   }
>

Re: [PATCH linux dev-4.10 v3 2/5] drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 9101 bytes --]

On Fri, 2017-08-04 at 11:17 -0500, Matt Spinler wrote:
> 
> On 8/4/2017 10:16 AM, Eddie James wrote:
> > 
> > 
> > On 08/03/2017 07:33 PM, Andrew Jeffery wrote:
> > > On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
> > > > > From: "Edward A. James" <eajames@us.ibm.com>
> > > > 
> > > > Add sysfs entries to dump out PS registers
> > > 
> > > Why are we working against what the pmbus subsystem already gives us?
> > > Why not augment support in pmbus core to give us the information we
> > > need in a manner acceptable to upstream?
> > 
> > Yea, that would be great. I feel like we're a little short on time for 
> > that, as we need this functionality basically now. But we can give it 
> > a shot.
> > 
> > > 
> > > Flipping to the other end of the spectrum: The device only has one
> > > page. We can just use `i2cget -f -y ...` without risk of corruption.
> > 
> > Yea, I just get a lot of requests to have it all in the device driver. 
> > I guess we need to just push back and ask people to use i2c tools. But 
> > we will have to solve this for the UCD driver.
> 
> In general, IBM likes to capture a lot of registers on failures so we 
> can actually figure out what went wrong after the fact, as we have 
> customers who get mad when we can't give them root cause.

I appreciate that.

>   It's a bummer 
> that is so hard to do here.  And if we had to use the i2cdev stuff for 
> that, then why even use a device driver for any of this...

Well, because:

1. There are existing ABIs for exposing a lot (but not all) of these
properties, and hwmon/pmbus devices should have drivers that conform to
these ABIs
2. Using the existing ABIs allows existing applications (e.g.
`sensors`) to display interesting information without knowledge of the
hardware (PMBus spec is pretty loose in terms of what you can do, what
you support, and how you go about it)

The problem I have with the proposal is it emulates what we can
equivalently scrape with `i2cget -f` by documenting a new kernel ABI
that has no additional semantics. This isn't going to fly with
upstream, and isn't terribly useful for us given the single page this
device supports.

The spanner in the works is the UCD driver where we will want to do the
same thing, but where the device has multiple pages. The `i2cget -f`
approach falls down there, and does seem it might require something
like what's proposed here if time is a factor. Given that there's the
question of consistency in the approach (i.e. providing the sysfs
interface for the UCD device but requiring the use of `i2cget -f` here
seems broken).

I'm a bit torn really.

Has anyone looked at how much effort it would really be to split out
the bits?

> 
> 
> > 
> > > 
> > > >   and clear faults.
> > > 
> > > Can you describe the motivation for this? I feel like it would be worth
> > > having this as a separate patch, and to the core rather than in an
> > > arbitrary driver.
> > 
> > Users need to be able to clear faults, that was just on my list of 
> > requirements :) They could probably use i2cset though.
> 
> Yea, userspace needs this.  I'm kinda surprised there wasn't already a 
> way to expose it.

Right; it seems the kernel's pmbus subsystem has mainly been concerned
with monitoring and not controlling the associated device. Clearing
faults would be a departure from that, as are my core changes to
support the MAX31785. I think there's a good chance that something like
what's proposed is acceptable, just no-one had implemented it.

Andrew

> 
> > 
> > > 
> > > Cheers,
> > > 
> > > Andrew
> > > 
> > > > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > > > 
> > > > ---
> > > >   drivers/hwmon/pmbus/ibmps.c | 78 
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 78 insertions(+)
> > > > 
> > > > diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
> > > > index 1928dd9..f55d2bf 100644
> > > > --- a/drivers/hwmon/pmbus/ibmps.c
> > > > +++ b/drivers/hwmon/pmbus/ibmps.c
> > > > @@ -9,8 +9,10 @@
> > > >     #include <linux/device.h>
> > > >   #include <linux/i2c.h>
> > > > +#include <linux/hwmon-sysfs.h>
> > > >   #include <linux/jiffies.h>
> > > >   #include <linux/module.h>
> > > > +#include <linux/sysfs.h>
> > > >     #include "pmbus.h"
> > > >   @@ -122,14 +124,90 @@ static int ibmps_read_word_data(struct 
> > > > i2c_client *client, int page, int reg)
> > > > >       .read_word_data = ibmps_read_word_data,
> > > > 
> > > >   };
> > > >   +static ssize_t ibmps_clear_faults(struct device *dev,
> > > > > +                  struct device_attribute *dev_attr,
> > > > > +                  const char *buf, size_t count)
> > > > 
> > > > +{
> > > > > +    struct i2c_client *client = to_i2c_client(dev);
> > > > 
> > > > +
> > > > > +    pmbus_clear_faults(client);
> > > > 
> > > > +
> > > > > +    return count;
> > > > 
> > > > +}
> > > > +static DEVICE_ATTR(clear_faults, 0200, NULL, ibmps_clear_faults);
> > > > +
> > > > +static ssize_t ibmps_show_status_word(struct device *dev,
> > > > > +                      struct device_attribute *dev_attr,
> > > > > +                      char *buf)
> > > > 
> > > > +{
> > > > > +    int rc;
> > > > > +    struct i2c_client *client = to_i2c_client(dev);
> > > > 
> > > > +
> > > > > +    rc = pmbus_read_word_data(client, 0, PMBUS_STATUS_WORD);
> > > > > +    if (rc < 0)
> > > > > +        return rc;
> > > > 
> > > > +
> > > > > +    return snprintf(buf, PAGE_SIZE - 1, "%04x\n", rc);
> > > > 
> > > > +}
> > > > +static DEVICE_ATTR(status_word, 0444, ibmps_show_status_word, NULL);
> > > > +
> > > > +static ssize_t ibmps_show_status_byte(struct device *dev,
> > > > > +                      struct device_attribute *dev_attr,
> > > > > +                      char *buf)
> > > > 
> > > > +{
> > > > > +    int rc;
> > > > > +    struct i2c_client *client = to_i2c_client(dev);
> > > > > +    struct sensor_device_attribute *sattr = 
> > > > > to_sensor_dev_attr(dev_attr);
> > > > 
> > > > +
> > > > > +    rc = pmbus_read_byte_data(client, 0, PMBUS_STATUS_VOUT + 
> > > > > sattr->index);
> > > > > +    if (rc < 0)
> > > > > +        return rc;
> > > > 
> > > > +
> > > > > +    return snprintf(buf, PAGE_SIZE - 1, "%02x\n", rc);
> > > > 
> > > > +}
> > > > +
> > > > +static SENSOR_DEVICE_ATTR(status_vout, 0444, 
> > > > ibmps_show_status_byte, NULL, 0);
> > > > +static SENSOR_DEVICE_ATTR(status_iout, 0444, 
> > > > ibmps_show_status_byte, NULL, 1);
> > > > +static SENSOR_DEVICE_ATTR(status_input, 0444, 
> > > > ibmps_show_status_byte, NULL, 2);
> > > > +static SENSOR_DEVICE_ATTR(status_temp, 0444, 
> > > > ibmps_show_status_byte, NULL, 3);
> > > > +static SENSOR_DEVICE_ATTR(status_cml, 0444, ibmps_show_status_byte, 
> > > > NULL, 4);
> > > > +static SENSOR_DEVICE_ATTR(status_other, 0444, 
> > > > ibmps_show_status_byte, NULL, 5);
> > > > +static SENSOR_DEVICE_ATTR(status_mfr, 0444, ibmps_show_status_byte, 
> > > > NULL, 6);
> > > > +static SENSOR_DEVICE_ATTR(status_fan, 0444, ibmps_show_status_byte, 
> > > > NULL, 7);
> > > > +
> > > > +static struct attribute *ibmps_attributes[] = {
> > > > > +    &dev_attr_clear_faults.attr,
> > > > > +    &dev_attr_status_word.attr,
> > > > > +    &sensor_dev_attr_status_vout.dev_attr.attr,
> > > > > +    &sensor_dev_attr_status_iout.dev_attr.attr,
> > > > > +    &sensor_dev_attr_status_input.dev_attr.attr,
> > > > > +    &sensor_dev_attr_status_temp.dev_attr.attr,
> > > > > +    &sensor_dev_attr_status_cml.dev_attr.attr,
> > > > > +    &sensor_dev_attr_status_other.dev_attr.attr,
> > > > > +    &sensor_dev_attr_status_mfr.dev_attr.attr,
> > > > > +    &sensor_dev_attr_status_fan.dev_attr.attr,
> > > > > +    NULL
> > > > 
> > > > +};
> > > > +
> > > > +static const struct attribute_group ibmps_attribute_group = {
> > > > > +    .attrs = ibmps_attributes,
> > > > 
> > > > +};
> > > > +
> > > >   static int ibmps_probe(struct i2c_client *client,
> > > > >                  const struct i2c_device_id *id)
> > > > 
> > > >   {
> > > > > +    int rc = sysfs_create_group(&client->dev.kobj,
> > > > > +                    &ibmps_attribute_group);
> > > > > +    if (rc)
> > > > > +        return rc;
> > > > 
> > > > +
> > > > >       return pmbus_do_probe(client, id, &ibmps_info);
> > > > 
> > > >   }
> > > >     static int ibmps_remove(struct i2c_client *client)
> > > >   {
> > > > > + sysfs_remove_group(&client->dev.kobj, &ibmps_attribute_group);
> > > > 
> > > > +
> > > > >       return pmbus_do_remove(client);
> > > > 
> > > >   }
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH linux dev-4.10 v3 3/5] Documentation: hwmon: Add IBM power supply documentation

[Eddie James]

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

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 Documentation/hwmon/ibmps | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/hwmon/ibmps

diff --git a/Documentation/hwmon/ibmps b/Documentation/hwmon/ibmps
new file mode 100644
index 0000000..7f13fd4
--- /dev/null
+++ b/Documentation/hwmon/ibmps
@@ -0,0 +1,53 @@
+Kernel driver ibmps
+====================
+
+Supported chips:
+  * IBM Witherspoon power supply
+
+Author: Eddie James <eajames@us.ibm.com>
+
+Description
+-----------
+
+This driver supports the IBM power supply. This driver is a client to the core
+PMBus driver.
+
+Usage Notes
+-----------
+
+This driver should auto-detect devices. In the event that it does not, you will
+have to instantiate the devices explicitly. Please see
+Documentation/i2c/instantiating-devices for details.
+
+Sysfs entries
+-------------
+
+The following attributes are supported:
+
+curr1_alarm		Output current over-current fault.
+curr1_input		Measured output current in mA.
+curr1_label		"iout1"
+
+fan1_alarm		Fan 1 warning.
+fan1_fault		Fan 1 fault.
+fan1_input		Fan 1 speed in RPM.
+fan2_alarm		Fan 2 warning.
+fan2_fault		Fan 2 fault.
+fan2_input		Fan 2 speed in RPM.
+
+in1_alarm		Input voltage under-voltage fault.
+in1_input		Measured input voltage in mV.
+in1_label		"vin"
+in2_alarm		Output voltage over-voltage fault.
+in2_input		Measured output voltage in mV.
+in2_label		"vout1"
+
+power1_input		Measured input power in uW.
+power1_label		"pin"
+
+temp1_alarm		PSU inlet ambient temperature over-temperature fault.
+temp1_input		Measured PSU inlet ambient temp in millidegrees C.
+temp2_alarm		Secondary rectifier temp over-temperature fault.
+temp2_input		Measured secondary rectifier temp in millidegrees C.
+temp3_alarm		ORing FET temperature over-temperature fault.
+temp3_input		Measured ORing FET temperature in millidegrees C.
-- 
1.8.3.1

Re: [PATCH linux dev-4.10 v3 3/5] Documentation: hwmon: Add IBM power supply documentation

[Brandon Wyman]

On Wed, Aug 2, 2017 at 3:25 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  Documentation/hwmon/ibmps | 53 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/hwmon/ibmps
>
> diff --git a/Documentation/hwmon/ibmps b/Documentation/hwmon/ibmps
> new file mode 100644
> index 0000000..7f13fd4
> --- /dev/null
> +++ b/Documentation/hwmon/ibmps
> @@ -0,0 +1,53 @@
> +Kernel driver ibmps
> +====================
> +
> +Supported chips:
> +  * IBM Witherspoon power supply
> +
> +Author: Eddie James <eajames@us.ibm.com>
> +
> +Description
> +-----------
> +
> +This driver supports the IBM power supply. This driver is a client to the core
> +PMBus driver.
> +
> +Usage Notes
> +-----------
> +
> +This driver should auto-detect devices. In the event that it does not, you will
> +have to instantiate the devices explicitly. Please see
> +Documentation/i2c/instantiating-devices for details.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported:
> +
> +curr1_alarm            Output current over-current fault.
> +curr1_input            Measured output current in mA.
> +curr1_label            "iout1"
> +
> +fan1_alarm             Fan 1 warning.
> +fan1_fault             Fan 1 fault.
> +fan1_input             Fan 1 speed in RPM.
> +fan2_alarm             Fan 2 warning.
> +fan2_fault             Fan 2 fault.
> +fan2_input             Fan 2 speed in RPM.
> +
> +in1_alarm              Input voltage under-voltage fault.

Is this the normal/regular file to put this under-voltage fault in? I
noticed that the UCD9200 device driver appears to be the only one
looking for that right now, and it has it updating in1_lcrit_alarm:
ucd9200:in1_lcrit_alarm         Voltage critical low alarm. From
VIN_UV_FAULT status.

> +in1_input              Measured input voltage in mV.
> +in1_label              "vin"
> +in2_alarm              Output voltage over-voltage fault.
> +in2_input              Measured output voltage in mV.
> +in2_label              "vout1"
> +
> +power1_input           Measured input power in uW.
> +power1_label           "pin"
> +
> +temp1_alarm            PSU inlet ambient temperature over-temperature fault.
> +temp1_input            Measured PSU inlet ambient temp in millidegrees C.
> +temp2_alarm            Secondary rectifier temp over-temperature fault.
> +temp2_input            Measured secondary rectifier temp in millidegrees C.
> +temp3_alarm            ORing FET temperature over-temperature fault.
> +temp3_input            Measured ORing FET temperature in millidegrees C.
> --
> 1.8.3.1
>

Re: [PATCH linux dev-4.10 v3 3/5] Documentation: hwmon: Add IBM power supply documentation

[Eddie James]

On 08/02/2017 05:11 PM, Brandon Wyman wrote:
> On Wed, Aug 2, 2017 at 3:25 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   Documentation/hwmon/ibmps | 53 +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>   create mode 100644 Documentation/hwmon/ibmps
>>
>> diff --git a/Documentation/hwmon/ibmps b/Documentation/hwmon/ibmps
>> new file mode 100644
>> index 0000000..7f13fd4
>> --- /dev/null
>> +++ b/Documentation/hwmon/ibmps
>> @@ -0,0 +1,53 @@
>> +Kernel driver ibmps
>> +====================
>> +
>> +Supported chips:
>> +  * IBM Witherspoon power supply
>> +
>> +Author: Eddie James <eajames@us.ibm.com>
>> +
>> +Description
>> +-----------
>> +
>> +This driver supports the IBM power supply. This driver is a client to the core
>> +PMBus driver.
>> +
>> +Usage Notes
>> +-----------
>> +
>> +This driver should auto-detect devices. In the event that it does not, you will
>> +have to instantiate the devices explicitly. Please see
>> +Documentation/i2c/instantiating-devices for details.
>> +
>> +Sysfs entries
>> +-------------
>> +
>> +The following attributes are supported:
>> +
>> +curr1_alarm            Output current over-current fault.
>> +curr1_input            Measured output current in mA.
>> +curr1_label            "iout1"
>> +
>> +fan1_alarm             Fan 1 warning.
>> +fan1_fault             Fan 1 fault.
>> +fan1_input             Fan 1 speed in RPM.
>> +fan2_alarm             Fan 2 warning.
>> +fan2_fault             Fan 2 fault.
>> +fan2_input             Fan 2 speed in RPM.
>> +
>> +in1_alarm              Input voltage under-voltage fault.
> Is this the normal/regular file to put this under-voltage fault in? I
> noticed that the UCD9200 device driver appears to be the only one
> looking for that right now, and it has it updating in1_lcrit_alarm:
> ucd9200:in1_lcrit_alarm         Voltage critical low alarm. From
> VIN_UV_FAULT status.

Since this is "vin" this will provide the status of VIN_UV_FAULT. 
Similarly, VOUT_OV_FAULT will be provided by the in2_alarm ("vout"). The 
power supply doesn't provide limit ranges for it's values, so in order 
to get lcrit_alarm or similar I'd have to "trick" pmbus into thinking 
limits are being provided by the PS. This seems more simple.

Thanks,
Eddie

>
>> +in1_input              Measured input voltage in mV.
>> +in1_label              "vin"
>> +in2_alarm              Output voltage over-voltage fault.
>> +in2_input              Measured output voltage in mV.
>> +in2_label              "vout1"
>> +
>> +power1_input           Measured input power in uW.
>> +power1_label           "pin"
>> +
>> +temp1_alarm            PSU inlet ambient temperature over-temperature fault.
>> +temp1_input            Measured PSU inlet ambient temp in millidegrees C.
>> +temp2_alarm            Secondary rectifier temp over-temperature fault.
>> +temp2_input            Measured secondary rectifier temp in millidegrees C.
>> +temp3_alarm            ORing FET temperature over-temperature fault.
>> +temp3_input            Measured ORing FET temperature in millidegrees C.
>> --
>> 1.8.3.1
>>

Re: [PATCH linux dev-4.10 v3 3/5] Documentation: hwmon: Add IBM power supply documentation

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4709 bytes --]

On Wed, 2017-08-02 at 21:23 -0500, Eddie James wrote:
> 
> On 08/02/2017 05:11 PM, Brandon Wyman wrote:
> > On Wed, Aug 2, 2017 at 3:25 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> > > > > > From: "Edward A. James" <eajames@us.ibm.com>
> > > 
> > > > > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > > ---
> > >   Documentation/hwmon/ibmps | 53 +++++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 53 insertions(+)
> > >   create mode 100644 Documentation/hwmon/ibmps
> > > 
> > > diff --git a/Documentation/hwmon/ibmps b/Documentation/hwmon/ibmps
> > > new file mode 100644
> > > index 0000000..7f13fd4
> > > --- /dev/null
> > > +++ b/Documentation/hwmon/ibmps
> > > @@ -0,0 +1,53 @@
> > > +Kernel driver ibmps
> > > +====================
> > > +
> > > +Supported chips:
> > > +  * IBM Witherspoon power supply
> > > +
> > > > > > +Author: Eddie James <eajames@us.ibm.com>
> > > +
> > > +Description
> > > +-----------
> > > +
> > > +This driver supports the IBM power supply. This driver is a client to the core
> > > +PMBus driver.
> > > +
> > > +Usage Notes
> > > +-----------
> > > +
> > > +This driver should auto-detect devices. In the event that it does not, you will
> > > +have to instantiate the devices explicitly. Please see
> > > +Documentation/i2c/instantiating-devices for details.
> > > +
> > > +Sysfs entries
> > > +-------------
> > > +
> > > +The following attributes are supported:
> > > +
> > > +curr1_alarm            Output current over-current fault.
> > > +curr1_input            Measured output current in mA.
> > > +curr1_label            "iout1"
> > > +
> > > +fan1_alarm             Fan 1 warning.
> > > +fan1_fault             Fan 1 fault.
> > > +fan1_input             Fan 1 speed in RPM.
> > > +fan2_alarm             Fan 2 warning.
> > > +fan2_fault             Fan 2 fault.
> > > +fan2_input             Fan 2 speed in RPM.
> > > +
> > > +in1_alarm              Input voltage under-voltage fault.
> > 
> > Is this the normal/regular file to put this under-voltage fault in? I
> > noticed that the UCD9200 device driver appears to be the only one
> > looking for that right now, and it has it updating in1_lcrit_alarm:
> > ucd9200:in1_lcrit_alarm         Voltage critical low alarm. From
> > VIN_UV_FAULT status.
> 
> Since this is "vin" this will provide the status of VIN_UV_FAULT. 
> Similarly, VOUT_OV_FAULT will be provided by the in2_alarm ("vout"). The 
> power supply doesn't provide limit ranges for it's values, so in order 
> to get lcrit_alarm or similar I'd have to "trick" pmbus into thinking 
> limits are being provided by the PS. This seems more simple.

I'm not across the context, but tricking pmbus is straight forward:
Implement the {read,write}_{byte,word}_data callbacks, and provide an
implementation of the register whose value you want to to fake out.

However, from Documentation/hwmon/sysfs-interface:


    Each channel or limit may have an associated alarm file, containing a
    boolean value. 1 means than an alarm condition exists, 0 means no alarm.

    Usually a given chip will either use channel-related alarms, or
    limit-related alarms, not both. The driver should just reflect the hardware.

Sounds like what Eddie's describing maybe fits the channel-related
case? Again, I haven't read though the whole series so take that with a
grain of salt.

Andrew

> 
> Thanks,
> Eddie
> 
> > 
> > > +in1_input              Measured input voltage in mV.
> > > +in1_label              "vin"
> > > +in2_alarm              Output voltage over-voltage fault.
> > > +in2_input              Measured output voltage in mV.
> > > +in2_label              "vout1"
> > > +
> > > +power1_input           Measured input power in uW.
> > > +power1_label           "pin"
> > > +
> > > +temp1_alarm            PSU inlet ambient temperature over-temperature fault.
> > > +temp1_input            Measured PSU inlet ambient temp in millidegrees C.
> > > +temp2_alarm            Secondary rectifier temp over-temperature fault.
> > > +temp2_input            Measured secondary rectifier temp in millidegrees C.
> > > +temp3_alarm            ORing FET temperature over-temperature fault.
> > > +temp3_input            Measured ORing FET temperature in millidegrees C.
> > > --
> > > 1.8.3.1
> > > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH linux dev-4.10 v3 4/5] Documentation: ABI: Add IBM power supply sysfs documentation

[Eddie James]

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

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 Documentation/ABI/testing/sysfs-driver-ibmps | 49 ++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-ibmps

diff --git a/Documentation/ABI/testing/sysfs-driver-ibmps b/Documentation/ABI/testing/sysfs-driver-ibmps
new file mode 100644
index 0000000..1645685
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-ibmps
@@ -0,0 +1,49 @@
+What:		/sys/bus/i2c/devices/<dev>/clear_faults
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Clears pmbus faults for this power supply.
+
+What:		/sys/bus/i2c/devices/<dev>/status_word
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_WORD register.
+
+What:		/sys/bus/i2c/devices/<dev>/status_vout
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_VOUT register.
+
+What:		/sys/bus/i2c/devices/<dev>/status_iout
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_IOUT register.
+
+What:		/sys/bus/i2c/devices/<dev>/status_input
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_INPUT register.
+
+What:		/sys/bus/i2c/devices/<dev>/status_temp
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_TEMPERATURE register.
+
+What:		/sys/bus/i2c/devices/<dev>/status_cml
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_CML register.
+
+What:		/sys/bus/i2c/devices/<dev>/status_other
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_OTHER register.
+
+What:		/sys/bus/i2c/devices/<dev>/status_mfr
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_MFR_SPECIFIC register.
+
+What:		/sys/bus/i2c/devices/<dev>/status_fan
+KernelVersion:	4.13
+Contact:	eajames@us.ibm.com
+Description:	Displays the STATUS_FAN register.
-- 
1.8.3.1

Re: [PATCH linux dev-4.10 v3 4/5] Documentation: ABI: Add IBM power supply sysfs documentation

[Matt Spinler]

On 8/2/2017 3:25 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>   Documentation/ABI/testing/sysfs-driver-ibmps | 49 ++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-driver-ibmps
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-ibmps b/Documentation/ABI/testing/sysfs-driver-ibmps
> new file mode 100644
> index 0000000..1645685
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-ibmps
> @@ -0,0 +1,49 @@
> +What:		/sys/bus/i2c/devices/<dev>/clear_faults
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Clears pmbus faults for this power supply.
> +
> +What:		/sys/bus/i2c/devices/<dev>/status_word
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_WORD register.

Hey Chris,  I think you did some research and found that upstream 
wouldn't allow capturing full registers like this?  Was that the case?  
Or does it not matter as long as it isn't in hwmon?

> +
> +What:		/sys/bus/i2c/devices/<dev>/status_vout
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_VOUT register.
> +
> +What:		/sys/bus/i2c/devices/<dev>/status_iout
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_IOUT register.
> +
> +What:		/sys/bus/i2c/devices/<dev>/status_input
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_INPUT register.
> +
> +What:		/sys/bus/i2c/devices/<dev>/status_temp
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_TEMPERATURE register.
> +
> +What:		/sys/bus/i2c/devices/<dev>/status_cml
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_CML register.
> +
> +What:		/sys/bus/i2c/devices/<dev>/status_other
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_OTHER register.
> +
> +What:		/sys/bus/i2c/devices/<dev>/status_mfr
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_MFR_SPECIFIC register.
> +
> +What:		/sys/bus/i2c/devices/<dev>/status_fan
> +KernelVersion:	4.13
> +Contact:	eajames@us.ibm.com
> +Description:	Displays the STATUS_FAN register.

Re: [PATCH linux dev-4.10 v3 4/5] Documentation: ABI: Add IBM power supply sysfs documentation

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3880 bytes --]

On Thu, 2017-08-03 at 10:42 -0500, Matt Spinler wrote:
> 
> On 8/2/2017 3:25 PM, Eddie James wrote:
> > > > From: "Edward A. James" <eajames@us.ibm.com>
> > 
> > > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > ---
> >   Documentation/ABI/testing/sysfs-driver-ibmps | 49 ++++++++++++++++++++++++++++
> >   1 file changed, 49 insertions(+)
> >   create mode 100644 Documentation/ABI/testing/sysfs-driver-ibmps
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-ibmps b/Documentation/ABI/testing/sysfs-driver-ibmps
> > new file mode 100644
> > index 0000000..1645685
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-ibmps
> > @@ -0,0 +1,49 @@
> > > > +What:		/sys/bus/i2c/devices/<dev>/clear_faults
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > > > +Description:	Clears pmbus faults for this power supply.
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_word
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > +Description:	Displays the STATUS_WORD register.
> 
> Hey Chris,  I think you did some research and found that upstream 
> wouldn't allow capturing full registers like this?  Was that the case?  
> Or does it not matter as long as it isn't in hwmon?

This is a PMBus device, so it should be implemented in terms of the
PMBus subsystem. I expect this approach will be shot down by the
hwmon/pmbus maintainer.

Given the STATUS_BYTE/STATUS_WORD registers are the root of a fault
tree, another approach is to ignore the tree and focus on the leaves,
which is the PMBus subsystem's approach:

https://github.com/openbmc/linux/blob/dev-4.10/drivers/hwmon/pmbus/pmbus_core.c#L1245

This continues down to line 1700.

Now, for several of the bits STATUS_WORD is the leaf node. Using
STATUS_WORD as a reference, as STATUS_BYTE is STATUS_WORD's lower byte,
STATUS_WORD is a leaf for the following bits:

* Bit 0: None of the above
* Bit 6: Unit is off
* Bit 7: Unit was busy
* Bit 8: Unknown fault or warning
* Bit 11: Power Good negated

Currently none of these bits are exposed by pmbus core, so that would
need to be rectified, but in-line with the approach used above. I
recommend getting in touch with Guenter on his preferred approach.

> 
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_vout
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > > > +Description:	Displays the STATUS_VOUT register.
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_iout
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > > > +Description:	Displays the STATUS_IOUT register.
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_input
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > > > +Description:	Displays the STATUS_INPUT register.
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_temp
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > > > +Description:	Displays the STATUS_TEMPERATURE register.
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_cml
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > > > +Description:	Displays the STATUS_CML register.
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_other
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > > > +Description:	Displays the STATUS_OTHER register.
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_mfr
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > > > +Description:	Displays the STATUS_MFR_SPECIFIC register.
> > +
> > > > +What:		/sys/bus/i2c/devices/<dev>/status_fan
> > > > +KernelVersion:	4.13
> > +Contact:	eajames@us.ibm.com
> > +Description:	Displays the STATUS_FAN register.

See my comment above. This will get a prickly response if sent
upstream.

Andrew

> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH linux dev-4.10 v3 5/5] ARM: dts: aspeed: witherspoon: Add IBM power supply to I2C bus

[Eddie James]

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

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index c0f992e..77658e0 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -321,6 +321,16 @@
 			type = <PCA955X_TYPE_LED>;
 		};
 	};
+
+	power-supply@68 {
+		compatible = "ibm,ibmps";
+		reg = <0x68>;
+	};
+
+	power-supply@69 {
+		compatible = "ibm,ibmps";
+		reg = <0x69>;
+	};
 };
 
 &i2c4 {
-- 
1.8.3.1

Re: [PATCH linux dev-4.10 v3 0/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Eddie James]

On 08/02/2017 03:25 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> This series adds a hwmon pmbus driver for the IBM power supply. The core
> monitoring functionality is provided by pmbus. The driver also exports some
> sysfs entries for raw status register access.
>
> The driver should be probed automatically, so I added it to the witherspoon
> device-tree.

Forgot to mention; only change since v2 was to rename the driver to ibmps.

>
> Edward A. James (5):
>    drivers/hwmon/pmbus: Add IBM power supply hwmon driver
>    drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes
>    Documentation: hwmon: Add IBM power supply documentation
>    Documentation: ABI: Add IBM power supply sysfs documentation
>    ARM: dts: aspeed: witherspoon: Add IBM power supply to I2C bus
>
>   Documentation/ABI/testing/sysfs-driver-ibmps     |  49 +++++
>   Documentation/hwmon/ibmps                        |  53 +++++
>   arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts |  10 +
>   drivers/hwmon/pmbus/Kconfig                      |  10 +
>   drivers/hwmon/pmbus/Makefile                     |   1 +
>   drivers/hwmon/pmbus/ibmps.c                      | 242 +++++++++++++++++++++++
>   6 files changed, 365 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-driver-ibmps
>   create mode 100644 Documentation/hwmon/ibmps
>   create mode 100644 drivers/hwmon/pmbus/ibmps.c
>