Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
@ 2019-08-16 10:19 John Wang
  2019-08-16 16:01 ` Vijay Khemka
  2019-08-19  2:20 ` Joel Stanley
  0 siblings, 2 replies; 5+ messages in thread
From: John Wang @ 2019-08-16 10:19 UTC (permalink / raw)
  To: jdelvare, linux, corbet, linux-hwmon, linux-doc, linux-kernel,
	openbmc, duanzhijia01, mine260309, joel

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

This driver adds sysfs attributes for additional power supply data,
including vendor, model, part_number, serial number,
firmware revision, hardware revision, and psu mode(active/standby).

Signed-off-by: John Wang <wangzqbj@inspur.com>
---
v5:
    - Align sysfs attrs description in inspur-ipsps1.rst
      (Use tab instead of space to sperate names and values)
v4:
    - Remove the additional tabs in the Makefile
    - Rebased on 5.3-rc4, not 5.2
v3:
    - Sort kconfig/makefile entries alphabetically
    - Remove unnecessary initialization
    - Use ATTRIBUTE_GROUPS instead of expanding directly
    - Use memscan to avoid reimplementation
v2:
    - Fix typos in commit message
    - Invert Christmas tree
    - Configure device with sysfs attrs, not debugfs entries
    - Fix errno in fw_version_read, ENODATA to EPROTO
    - Change the print format of fw-version
    - Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
    - Document sysfs attributes
---
 Documentation/hwmon/inspur-ipsps1.rst |  79 +++++++++
 drivers/hwmon/pmbus/Kconfig           |   9 +
 drivers/hwmon/pmbus/Makefile          |   1 +
 drivers/hwmon/pmbus/inspur-ipsps.c    | 226 ++++++++++++++++++++++++++
 4 files changed, 315 insertions(+)
 create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
 create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c

diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
new file mode 100644
index 000000000000..2b871ae3448f
--- /dev/null
+++ b/Documentation/hwmon/inspur-ipsps1.rst
@@ -0,0 +1,79 @@
+Kernel driver inspur-ipsps1
+=======================
+
+Supported chips:
+
+  * Inspur Power System power supply unit
+
+Author: John Wang <wangzqbj@inspur.com>
+
+Description
+-----------
+
+This driver supports Inspur Power System power supplies. This driver
+is a client to the core PMBus driver.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices for
+details.
+
+Sysfs entries
+-------------
+
+The following attributes are supported:
+
+======================= ======================================================
+curr1_input		Measured input current
+curr1_label		"iin"
+curr1_max		Maximum current
+curr1_max_alarm		Current high alarm
+curr2_input		Measured output current in mA.
+curr2_label		"iout1"
+curr2_crit		Critical maximum current
+curr2_crit_alarm	Current critical high alarm
+curr2_max		Maximum current
+curr2_max_alarm		Current high alarm
+
+fan1_alarm		Fan 1 warning.
+fan1_fault		Fan 1 fault.
+fan1_input		Fan 1 speed in RPM.
+
+in1_alarm		Input voltage under-voltage alarm.
+in1_input		Measured input voltage in mV.
+in1_label		"vin"
+in2_input		Measured output voltage in mV.
+in2_label		"vout1"
+in2_lcrit		Critical minimum output voltage
+in2_lcrit_alarm		Output voltage critical low alarm
+in2_max			Maximum output voltage
+in2_max_alarm		Output voltage high alarm
+in2_min			Minimum output voltage
+in2_min_alarm		Output voltage low alarm
+
+power1_alarm		Input fault or alarm.
+power1_input		Measured input power in uW.
+power1_label		"pin"
+power1_max		Input power limit
+power2_max_alarm	Output power high alarm
+power2_max		Output power limit
+power2_input		Measured output power in uW.
+power2_label		"pout"
+
+temp[1-3]_input		Measured temperature
+temp[1-2]_max		Maximum temperature
+temp[1-3]_max_alarm	Temperature high alarm
+
+vendor			Manufacturer name
+model			Product model
+part_number		Product part number
+serial_number		Product serial number
+fw_version		Firmware version
+hw_version		Hardware version
+mode			Work mode. Can be set to active or
+			standby, when set to standby, PSU will
+			automatically switch between standby
+			and redundancy mode.
+======================= ======================================================
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index b6588483fae1..d62d69bb7e49 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
 	  This driver can also be built as a module. If so, the module will
 	  be called ibm-cffps.
 
+config SENSORS_INSPUR_IPSPS
+	tristate "INSPUR Power System Power Supply"
+	help
+	  If you say yes here you get hardware monitoring support for the INSPUR
+	  Power System power supply.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called inspur-ipsps.
+
 config SENSORS_IR35221
 	tristate "Infineon IR35221"
 	help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index c950ea9a5d00..03bacfcfd660 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS)		+= pmbus_core.o
 obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
 obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
 obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
+obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
 obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
 obj-$(CONFIG_SENSORS_IR38064)	+= ir38064.o
 obj-$(CONFIG_SENSORS_IRPS5401)	+= irps5401.o
diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
new file mode 100644
index 000000000000..fa981b881a60
--- /dev/null
+++ b/drivers/hwmon/pmbus/inspur-ipsps.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2019 Inspur Corp.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include <linux/hwmon-sysfs.h>
+
+#include "pmbus.h"
+
+#define IPSPS_REG_VENDOR_ID	0x99
+#define IPSPS_REG_MODEL		0x9A
+#define IPSPS_REG_FW_VERSION	0x9B
+#define IPSPS_REG_PN		0x9C
+#define IPSPS_REG_SN		0x9E
+#define IPSPS_REG_HW_VERSION	0xB0
+#define IPSPS_REG_MODE		0xFC
+
+#define MODE_ACTIVE		0x55
+#define MODE_STANDBY		0x0E
+#define MODE_REDUNDANCY		0x00
+
+#define MODE_ACTIVE_STRING		"active"
+#define MODE_STANDBY_STRING		"standby"
+#define MODE_REDUNDANCY_STRING		"redundancy"
+
+enum ipsps_index {
+	vendor,
+	model,
+	fw_version,
+	part_number,
+	serial_number,
+	hw_version,
+	mode,
+	num_regs,
+};
+
+static const u8 ipsps_regs[num_regs] = {
+	[vendor] = IPSPS_REG_VENDOR_ID,
+	[model] = IPSPS_REG_MODEL,
+	[fw_version] = IPSPS_REG_FW_VERSION,
+	[part_number] = IPSPS_REG_PN,
+	[serial_number] = IPSPS_REG_SN,
+	[hw_version] = IPSPS_REG_HW_VERSION,
+	[mode] = IPSPS_REG_MODE,
+};
+
+static ssize_t ipsps_string_show(struct device *dev,
+				 struct device_attribute *devattr,
+				 char *buf)
+{
+	u8 reg;
+	int rc;
+	char *p;
+	char data[I2C_SMBUS_BLOCK_MAX + 1];
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	reg = ipsps_regs[attr->index];
+	rc = i2c_smbus_read_block_data(client, reg, data);
+	if (rc < 0)
+		return rc;
+
+	/* filled with printable characters, ending with # */
+	p = memscan(data, '#', rc);
+	*p = '\0';
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", data);
+}
+
+static ssize_t ipsps_fw_version_show(struct device *dev,
+				     struct device_attribute *devattr,
+				     char *buf)
+{
+	u8 reg;
+	int rc;
+	u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	reg = ipsps_regs[attr->index];
+	rc = i2c_smbus_read_block_data(client, reg, data);
+	if (rc < 0)
+		return rc;
+
+	if (rc != 6)
+		return -EPROTO;
+
+	return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
+			data[1], data[2]/* < 100 */, data[3]/*< 10*/,
+			data[4], data[5]/* < 100 */);
+}
+
+static ssize_t ipsps_mode_show(struct device *dev,
+			       struct device_attribute *devattr, char *buf)
+{
+	u8 reg;
+	int rc;
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	reg = ipsps_regs[attr->index];
+	rc = i2c_smbus_read_byte_data(client, reg);
+	if (rc < 0)
+		return rc;
+
+	switch (rc) {
+	case MODE_ACTIVE:
+		return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
+				MODE_ACTIVE_STRING,
+				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+	case MODE_STANDBY:
+		return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
+				MODE_ACTIVE_STRING,
+				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+	case MODE_REDUNDANCY:
+		return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
+				MODE_ACTIVE_STRING,
+				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+	default:
+		return snprintf(buf, PAGE_SIZE, "unspecified\n");
+	}
+}
+
+static ssize_t ipsps_mode_store(struct device *dev,
+				struct device_attribute *devattr,
+				const char *buf, size_t count)
+{
+	u8 reg;
+	int rc;
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	reg = ipsps_regs[attr->index];
+	if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
+		rc = i2c_smbus_write_byte_data(client, reg,
+					       MODE_STANDBY);
+		if (rc < 0)
+			return rc;
+		return count;
+	} else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
+		rc = i2c_smbus_write_byte_data(client, reg,
+					       MODE_ACTIVE);
+		if (rc < 0)
+			return rc;
+		return count;
+	}
+
+	return -EINVAL;
+}
+
+static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
+static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
+static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
+static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
+static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
+static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
+static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
+
+static struct attribute *ipsps_attrs[] = {
+	&sensor_dev_attr_vendor.dev_attr.attr,
+	&sensor_dev_attr_model.dev_attr.attr,
+	&sensor_dev_attr_part_number.dev_attr.attr,
+	&sensor_dev_attr_serial_number.dev_attr.attr,
+	&sensor_dev_attr_hw_version.dev_attr.attr,
+	&sensor_dev_attr_fw_version.dev_attr.attr,
+	&sensor_dev_attr_mode.dev_attr.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(ipsps);
+
+static struct pmbus_driver_info ipsps_info = {
+	.pages = 1,
+	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+		PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | 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,
+	.groups = ipsps_groups,
+};
+
+static struct pmbus_platform_data ipsps_pdata = {
+	.flags = PMBUS_SKIP_STATUS_CHECK,
+};
+
+static int ipsps_probe(struct i2c_client *client,
+		       const struct i2c_device_id *id)
+{
+	client->dev.platform_data = &ipsps_pdata;
+	return pmbus_do_probe(client, id, &ipsps_info);
+}
+
+static const struct i2c_device_id ipsps_id[] = {
+	{ "inspur_ipsps1", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ipsps_id);
+
+static const struct of_device_id ipsps_of_match[] = {
+	{ .compatible = "inspur,ipsps1" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ipsps_of_match);
+
+static struct i2c_driver ipsps_driver = {
+	.driver = {
+		.name = "inspur-ipsps",
+		.of_match_table = ipsps_of_match,
+	},
+	.probe = ipsps_probe,
+	.remove = pmbus_do_remove,
+	.id_table = ipsps_id,
+};
+
+module_i2c_driver(ipsps_driver);
+
+MODULE_AUTHOR("John Wang");
+MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
+MODULE_LICENSE("GPL");
-- 
2.17.1


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

* Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
  2019-08-16 10:19 [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver John Wang
@ 2019-08-16 16:01 ` Vijay Khemka
  2019-08-16 16:12   ` Guenter Roeck
  2019-08-19  2:20 ` Joel Stanley
  1 sibling, 1 reply; 5+ messages in thread
From: Vijay Khemka @ 2019-08-16 16:01 UTC (permalink / raw)
  To: John Wang, jdelvare, linux, corbet, linux-hwmon, linux-doc,
	linux-kernel, openbmc, duanzhijia01, mine260309, joel



On 8/16/19, 3:20 AM, "openbmc on behalf of John Wang" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of wangzqbj@inspur.com> wrote:

    Add the driver to monitor Inspur Power System power supplies
    with hwmon over pmbus.
    
    This driver adds sysfs attributes for additional power supply data,
    including vendor, model, part_number, serial number,
    firmware revision, hardware revision, and psu mode(active/standby).
    
    Signed-off-by: John Wang <wangzqbj@inspur.com>
    ---
    v5:
        - Align sysfs attrs description in inspur-ipsps1.rst
          (Use tab instead of space to sperate names and values)
    v4:
        - Remove the additional tabs in the Makefile
        - Rebased on 5.3-rc4, not 5.2
    v3:
        - Sort kconfig/makefile entries alphabetically
        - Remove unnecessary initialization
        - Use ATTRIBUTE_GROUPS instead of expanding directly
        - Use memscan to avoid reimplementation
    v2:
        - Fix typos in commit message
        - Invert Christmas tree
        - Configure device with sysfs attrs, not debugfs entries
        - Fix errno in fw_version_read, ENODATA to EPROTO
        - Change the print format of fw-version
        - Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
        - Document sysfs attributes
    ---
     Documentation/hwmon/inspur-ipsps1.rst |  79 +++++++++
     drivers/hwmon/pmbus/Kconfig           |   9 +
     drivers/hwmon/pmbus/Makefile          |   1 +
     drivers/hwmon/pmbus/inspur-ipsps.c    | 226 ++++++++++++++++++++++++++
     4 files changed, 315 insertions(+)
     create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
     create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
    
    diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
    new file mode 100644
    index 000000000000..2b871ae3448f
    --- /dev/null
    +++ b/Documentation/hwmon/inspur-ipsps1.rst
    @@ -0,0 +1,79 @@
    +Kernel driver inspur-ipsps1
    +=======================
    +
    +Supported chips:
    +
    +  * Inspur Power System power supply unit
    +
    +Author: John Wang <wangzqbj@inspur.com>
    +
    +Description
    +-----------
    +
    +This driver supports Inspur Power System power supplies. This driver
    +is a client to the core PMBus driver.
    +
    +Usage Notes
    +-----------
    +
    +This driver does not auto-detect devices. You will have to instantiate the
    +devices explicitly. Please see Documentation/i2c/instantiating-devices for
    +details.
    +
    +Sysfs entries
    +-------------
    +
    +The following attributes are supported:
    +
    +======================= ======================================================
    +curr1_input		Measured input current
    +curr1_label		"iin"
    +curr1_max		Maximum current
    +curr1_max_alarm		Current high alarm
    +curr2_input		Measured output current in mA.
    +curr2_label		"iout1"
    +curr2_crit		Critical maximum current
    +curr2_crit_alarm	Current critical high alarm
    +curr2_max		Maximum current
    +curr2_max_alarm		Current high alarm
    +
    +fan1_alarm		Fan 1 warning.
    +fan1_fault		Fan 1 fault.
    +fan1_input		Fan 1 speed in RPM.
    +
    +in1_alarm		Input voltage under-voltage alarm.
    +in1_input		Measured input voltage in mV.
    +in1_label		"vin"
    +in2_input		Measured output voltage in mV.
    +in2_label		"vout1"
    +in2_lcrit		Critical minimum output voltage
    +in2_lcrit_alarm		Output voltage critical low alarm
    +in2_max			Maximum output voltage
    +in2_max_alarm		Output voltage high alarm
    +in2_min			Minimum output voltage
    +in2_min_alarm		Output voltage low alarm
    +
    +power1_alarm		Input fault or alarm.
    +power1_input		Measured input power in uW.
    +power1_label		"pin"
    +power1_max		Input power limit
    +power2_max_alarm	Output power high alarm
    +power2_max		Output power limit
    +power2_input		Measured output power in uW.
    +power2_label		"pout"
    +
    +temp[1-3]_input		Measured temperature
    +temp[1-2]_max		Maximum temperature
    +temp[1-3]_max_alarm	Temperature high alarm
    +
    +vendor			Manufacturer name
    +model			Product model
    +part_number		Product part number
    +serial_number		Product serial number
    +fw_version		Firmware version
    +hw_version		Hardware version
    +mode			Work mode. Can be set to active or
    +			standby, when set to standby, PSU will
    +			automatically switch between standby
    +			and redundancy mode.
I don't think it is aligned yet. Please use space only instead of tabs.

    +======================= ======================================================
    diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
    index b6588483fae1..d62d69bb7e49 100644
    --- a/drivers/hwmon/pmbus/Kconfig
    +++ b/drivers/hwmon/pmbus/Kconfig
    @@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
     	  This driver can also be built as a module. If so, the module will
     	  be called ibm-cffps.
     
    +config SENSORS_INSPUR_IPSPS
    +	tristate "INSPUR Power System Power Supply"
    +	help
    +	  If you say yes here you get hardware monitoring support for the INSPUR
    +	  Power System power supply.
    +
    +	  This driver can also be built as a module. If so, the module will
    +	  be called inspur-ipsps.
    +
     config SENSORS_IR35221
     	tristate "Infineon IR35221"
     	help
    diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
    index c950ea9a5d00..03bacfcfd660 100644
    --- a/drivers/hwmon/pmbus/Makefile
    +++ b/drivers/hwmon/pmbus/Makefile
    @@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS)		+= pmbus_core.o
     obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
     obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
     obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
    +obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
     obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
     obj-$(CONFIG_SENSORS_IR38064)	+= ir38064.o
     obj-$(CONFIG_SENSORS_IRPS5401)	+= irps5401.o
    diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
    new file mode 100644
    index 000000000000..fa981b881a60
    --- /dev/null
    +++ b/drivers/hwmon/pmbus/inspur-ipsps.c
    @@ -0,0 +1,226 @@
    +// SPDX-License-Identifier: GPL-2.0-or-later
    +/*
    + * Copyright 2019 Inspur Corp.
    + */
    +
    +#include <linux/debugfs.h>
    +#include <linux/device.h>
    +#include <linux/fs.h>
    +#include <linux/i2c.h>
    +#include <linux/module.h>
    +#include <linux/pmbus.h>
    +#include <linux/hwmon-sysfs.h>
    +
    +#include "pmbus.h"
    +
    +#define IPSPS_REG_VENDOR_ID	0x99
    +#define IPSPS_REG_MODEL		0x9A
    +#define IPSPS_REG_FW_VERSION	0x9B
    +#define IPSPS_REG_PN		0x9C
    +#define IPSPS_REG_SN		0x9E
    +#define IPSPS_REG_HW_VERSION	0xB0
    +#define IPSPS_REG_MODE		0xFC
    +
    +#define MODE_ACTIVE		0x55
    +#define MODE_STANDBY		0x0E
    +#define MODE_REDUNDANCY		0x00
    +
    +#define MODE_ACTIVE_STRING		"active"
    +#define MODE_STANDBY_STRING		"standby"
    +#define MODE_REDUNDANCY_STRING		"redundancy"
    +
    +enum ipsps_index {
    +	vendor,
    +	model,
    +	fw_version,
    +	part_number,
    +	serial_number,
    +	hw_version,
    +	mode,
    +	num_regs,
    +};
    +
    +static const u8 ipsps_regs[num_regs] = {
    +	[vendor] = IPSPS_REG_VENDOR_ID,
    +	[model] = IPSPS_REG_MODEL,
    +	[fw_version] = IPSPS_REG_FW_VERSION,
    +	[part_number] = IPSPS_REG_PN,
    +	[serial_number] = IPSPS_REG_SN,
    +	[hw_version] = IPSPS_REG_HW_VERSION,
    +	[mode] = IPSPS_REG_MODE,
    +};
    +
    +static ssize_t ipsps_string_show(struct device *dev,
    +				 struct device_attribute *devattr,
    +				 char *buf)
    +{
    +	u8 reg;
    +	int rc;
    +	char *p;
    +	char data[I2C_SMBUS_BLOCK_MAX + 1];
    +	struct i2c_client *client = to_i2c_client(dev->parent);
    +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
    +
    +	reg = ipsps_regs[attr->index];
    +	rc = i2c_smbus_read_block_data(client, reg, data);
    +	if (rc < 0)
    +		return rc;
    +
    +	/* filled with printable characters, ending with # */
    +	p = memscan(data, '#', rc);
    +	*p = '\0';
    +
    +	return snprintf(buf, PAGE_SIZE, "%s\n", data);
    +}
    +
    +static ssize_t ipsps_fw_version_show(struct device *dev,
    +				     struct device_attribute *devattr,
    +				     char *buf)
    +{
    +	u8 reg;
    +	int rc;
    +	u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
    +	struct i2c_client *client = to_i2c_client(dev->parent);
    +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
    +
    +	reg = ipsps_regs[attr->index];
    +	rc = i2c_smbus_read_block_data(client, reg, data);
    +	if (rc < 0)
    +		return rc;
    +
    +	if (rc != 6)
    +		return -EPROTO;
    +
    +	return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
    +			data[1], data[2]/* < 100 */, data[3]/*< 10*/,
    +			data[4], data[5]/* < 100 */);
    +}
    +
    +static ssize_t ipsps_mode_show(struct device *dev,
    +			       struct device_attribute *devattr, char *buf)
    +{
    +	u8 reg;
    +	int rc;
    +	struct i2c_client *client = to_i2c_client(dev->parent);
    +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
    +
    +	reg = ipsps_regs[attr->index];
    +	rc = i2c_smbus_read_byte_data(client, reg);
    +	if (rc < 0)
    +		return rc;
    +
    +	switch (rc) {
    +	case MODE_ACTIVE:
    +		return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
    +				MODE_ACTIVE_STRING,
    +				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
    +	case MODE_STANDBY:
    +		return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
    +				MODE_ACTIVE_STRING,
    +				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
    +	case MODE_REDUNDANCY:
    +		return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
    +				MODE_ACTIVE_STRING,
    +				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
    +	default:
    +		return snprintf(buf, PAGE_SIZE, "unspecified\n");
    +	}
    +}
    +
    +static ssize_t ipsps_mode_store(struct device *dev,
    +				struct device_attribute *devattr,
    +				const char *buf, size_t count)
    +{
    +	u8 reg;
    +	int rc;
    +	struct i2c_client *client = to_i2c_client(dev->parent);
    +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
    +
    +	reg = ipsps_regs[attr->index];
    +	if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
    +		rc = i2c_smbus_write_byte_data(client, reg,
    +					       MODE_STANDBY);
    +		if (rc < 0)
    +			return rc;
    +		return count;
    +	} else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
    +		rc = i2c_smbus_write_byte_data(client, reg,
    +					       MODE_ACTIVE);
    +		if (rc < 0)
    +			return rc;
    +		return count;
    +	}
    +
    +	return -EINVAL;
    +}
    +
    +static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
    +static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
    +static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
    +static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
    +static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
    +static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
    +static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
    +
    +static struct attribute *ipsps_attrs[] = {
    +	&sensor_dev_attr_vendor.dev_attr.attr,
    +	&sensor_dev_attr_model.dev_attr.attr,
    +	&sensor_dev_attr_part_number.dev_attr.attr,
    +	&sensor_dev_attr_serial_number.dev_attr.attr,
    +	&sensor_dev_attr_hw_version.dev_attr.attr,
    +	&sensor_dev_attr_fw_version.dev_attr.attr,
    +	&sensor_dev_attr_mode.dev_attr.attr,
    +	NULL,
    +};
    +
    +ATTRIBUTE_GROUPS(ipsps);
    +
    +static struct pmbus_driver_info ipsps_info = {
    +	.pages = 1,
    +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
    +		PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | 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,
    +	.groups = ipsps_groups,
    +};
    +
    +static struct pmbus_platform_data ipsps_pdata = {
    +	.flags = PMBUS_SKIP_STATUS_CHECK,
    +};
    +
    +static int ipsps_probe(struct i2c_client *client,
    +		       const struct i2c_device_id *id)
    +{
    +	client->dev.platform_data = &ipsps_pdata;
    +	return pmbus_do_probe(client, id, &ipsps_info);
    +}
    +
    +static const struct i2c_device_id ipsps_id[] = {
    +	{ "inspur_ipsps1", 0 },
    +	{}
    +};
    +MODULE_DEVICE_TABLE(i2c, ipsps_id);
    +
    +static const struct of_device_id ipsps_of_match[] = {
    +	{ .compatible = "inspur,ipsps1" },
    +	{}
    +};
    +MODULE_DEVICE_TABLE(of, ipsps_of_match);
    +
    +static struct i2c_driver ipsps_driver = {
    +	.driver = {
    +		.name = "inspur-ipsps",
    +		.of_match_table = ipsps_of_match,
    +	},
    +	.probe = ipsps_probe,
    +	.remove = pmbus_do_remove,
    +	.id_table = ipsps_id,
    +};
    +
    +module_i2c_driver(ipsps_driver);
    +
    +MODULE_AUTHOR("John Wang");
    +MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
    +MODULE_LICENSE("GPL");
    -- 
    2.17.1
    
    


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

* Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
  2019-08-16 16:01 ` Vijay Khemka
@ 2019-08-16 16:12   ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-08-16 16:12 UTC (permalink / raw)
  To: Vijay Khemka
  Cc: John Wang, jdelvare, corbet, linux-hwmon, linux-doc,
	linux-kernel, openbmc, duanzhijia01, mine260309, joel

On Fri, Aug 16, 2019 at 04:01:05PM +0000, Vijay Khemka wrote:
> 
> 
> On 8/16/19, 3:20 AM, "openbmc on behalf of John Wang" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of wangzqbj@inspur.com> wrote:
> 
>     Add the driver to monitor Inspur Power System power supplies
>     with hwmon over pmbus.
>     
>     This driver adds sysfs attributes for additional power supply data,
>     including vendor, model, part_number, serial number,
>     firmware revision, hardware revision, and psu mode(active/standby).
>     
>     Signed-off-by: John Wang <wangzqbj@inspur.com>
>     ---
>     v5:
>         - Align sysfs attrs description in inspur-ipsps1.rst
>           (Use tab instead of space to sperate names and values)
>     v4:
>         - Remove the additional tabs in the Makefile
>         - Rebased on 5.3-rc4, not 5.2
>     v3:
>         - Sort kconfig/makefile entries alphabetically
>         - Remove unnecessary initialization
>         - Use ATTRIBUTE_GROUPS instead of expanding directly
>         - Use memscan to avoid reimplementation
>     v2:
>         - Fix typos in commit message
>         - Invert Christmas tree
>         - Configure device with sysfs attrs, not debugfs entries
>         - Fix errno in fw_version_read, ENODATA to EPROTO
>         - Change the print format of fw-version
>         - Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
>         - Document sysfs attributes
>     ---
>      Documentation/hwmon/inspur-ipsps1.rst |  79 +++++++++
>      drivers/hwmon/pmbus/Kconfig           |   9 +
>      drivers/hwmon/pmbus/Makefile          |   1 +
>      drivers/hwmon/pmbus/inspur-ipsps.c    | 226 ++++++++++++++++++++++++++
>      4 files changed, 315 insertions(+)
>      create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
>      create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
>     
>     diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
>     new file mode 100644
>     index 000000000000..2b871ae3448f
>     --- /dev/null
>     +++ b/Documentation/hwmon/inspur-ipsps1.rst
>     @@ -0,0 +1,79 @@
>     +Kernel driver inspur-ipsps1
>     +=======================
>     +
>     +Supported chips:
>     +
>     +  * Inspur Power System power supply unit
>     +
>     +Author: John Wang <wangzqbj@inspur.com>
>     +
>     +Description
>     +-----------
>     +
>     +This driver supports Inspur Power System power supplies. This driver
>     +is a client to the core PMBus driver.
>     +
>     +Usage Notes
>     +-----------
>     +
>     +This driver does not auto-detect devices. You will have to instantiate the
>     +devices explicitly. Please see Documentation/i2c/instantiating-devices for
>     +details.
>     +
>     +Sysfs entries
>     +-------------
>     +
>     +The following attributes are supported:
>     +
>     +======================= ======================================================
>     +curr1_input		Measured input current
>     +curr1_label		"iin"
>     +curr1_max		Maximum current
>     +curr1_max_alarm		Current high alarm
>     +curr2_input		Measured output current in mA.
>     +curr2_label		"iout1"
>     +curr2_crit		Critical maximum current
>     +curr2_crit_alarm	Current critical high alarm
>     +curr2_max		Maximum current
>     +curr2_max_alarm		Current high alarm
>     +
>     +fan1_alarm		Fan 1 warning.
>     +fan1_fault		Fan 1 fault.
>     +fan1_input		Fan 1 speed in RPM.
>     +
>     +in1_alarm		Input voltage under-voltage alarm.
>     +in1_input		Measured input voltage in mV.
>     +in1_label		"vin"
>     +in2_input		Measured output voltage in mV.
>     +in2_label		"vout1"
>     +in2_lcrit		Critical minimum output voltage
>     +in2_lcrit_alarm		Output voltage critical low alarm
>     +in2_max			Maximum output voltage
>     +in2_max_alarm		Output voltage high alarm
>     +in2_min			Minimum output voltage
>     +in2_min_alarm		Output voltage low alarm
>     +
>     +power1_alarm		Input fault or alarm.
>     +power1_input		Measured input power in uW.
>     +power1_label		"pin"
>     +power1_max		Input power limit
>     +power2_max_alarm	Output power high alarm
>     +power2_max		Output power limit
>     +power2_input		Measured output power in uW.
>     +power2_label		"pout"
>     +
>     +temp[1-3]_input		Measured temperature
>     +temp[1-2]_max		Maximum temperature
>     +temp[1-3]_max_alarm	Temperature high alarm
>     +
>     +vendor			Manufacturer name
>     +model			Product model
>     +part_number		Product part number
>     +serial_number		Product serial number
>     +fw_version		Firmware version
>     +hw_version		Hardware version
>     +mode			Work mode. Can be set to active or
>     +			standby, when set to standby, PSU will
>     +			automatically switch between standby
>     +			and redundancy mode.
> I don't think it is aligned yet. Please use space only instead of tabs.
> 

First, it is aligned. It just appears unaligned in the patch because
'+' at the beginning of the lins shifts the output. We want the result
to be aligned, not the patch file.

Second, let's use tabs like every other hwmon documentation file. I don't
see the point of using spaces just to make the patch file appear aligned.

Unless there is serious feedback, I am going to apply this patch to
hwmon-next.

Guenter

>     +======================= ======================================================
>     diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>     index b6588483fae1..d62d69bb7e49 100644
>     --- a/drivers/hwmon/pmbus/Kconfig
>     +++ b/drivers/hwmon/pmbus/Kconfig
>     @@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
>      	  This driver can also be built as a module. If so, the module will
>      	  be called ibm-cffps.
>      
>     +config SENSORS_INSPUR_IPSPS
>     +	tristate "INSPUR Power System Power Supply"
>     +	help
>     +	  If you say yes here you get hardware monitoring support for the INSPUR
>     +	  Power System power supply.
>     +
>     +	  This driver can also be built as a module. If so, the module will
>     +	  be called inspur-ipsps.
>     +
>      config SENSORS_IR35221
>      	tristate "Infineon IR35221"
>      	help
>     diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
>     index c950ea9a5d00..03bacfcfd660 100644
>     --- a/drivers/hwmon/pmbus/Makefile
>     +++ b/drivers/hwmon/pmbus/Makefile
>     @@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS)		+= pmbus_core.o
>      obj-$(CONFIG_SENSORS_PMBUS)	+= pmbus.o
>      obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
>      obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
>     +obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
>      obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
>      obj-$(CONFIG_SENSORS_IR38064)	+= ir38064.o
>      obj-$(CONFIG_SENSORS_IRPS5401)	+= irps5401.o
>     diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
>     new file mode 100644
>     index 000000000000..fa981b881a60
>     --- /dev/null
>     +++ b/drivers/hwmon/pmbus/inspur-ipsps.c
>     @@ -0,0 +1,226 @@
>     +// SPDX-License-Identifier: GPL-2.0-or-later
>     +/*
>     + * Copyright 2019 Inspur Corp.
>     + */
>     +
>     +#include <linux/debugfs.h>
>     +#include <linux/device.h>
>     +#include <linux/fs.h>
>     +#include <linux/i2c.h>
>     +#include <linux/module.h>
>     +#include <linux/pmbus.h>
>     +#include <linux/hwmon-sysfs.h>
>     +
>     +#include "pmbus.h"
>     +
>     +#define IPSPS_REG_VENDOR_ID	0x99
>     +#define IPSPS_REG_MODEL		0x9A
>     +#define IPSPS_REG_FW_VERSION	0x9B
>     +#define IPSPS_REG_PN		0x9C
>     +#define IPSPS_REG_SN		0x9E
>     +#define IPSPS_REG_HW_VERSION	0xB0
>     +#define IPSPS_REG_MODE		0xFC
>     +
>     +#define MODE_ACTIVE		0x55
>     +#define MODE_STANDBY		0x0E
>     +#define MODE_REDUNDANCY		0x00
>     +
>     +#define MODE_ACTIVE_STRING		"active"
>     +#define MODE_STANDBY_STRING		"standby"
>     +#define MODE_REDUNDANCY_STRING		"redundancy"
>     +
>     +enum ipsps_index {
>     +	vendor,
>     +	model,
>     +	fw_version,
>     +	part_number,
>     +	serial_number,
>     +	hw_version,
>     +	mode,
>     +	num_regs,
>     +};
>     +
>     +static const u8 ipsps_regs[num_regs] = {
>     +	[vendor] = IPSPS_REG_VENDOR_ID,
>     +	[model] = IPSPS_REG_MODEL,
>     +	[fw_version] = IPSPS_REG_FW_VERSION,
>     +	[part_number] = IPSPS_REG_PN,
>     +	[serial_number] = IPSPS_REG_SN,
>     +	[hw_version] = IPSPS_REG_HW_VERSION,
>     +	[mode] = IPSPS_REG_MODE,
>     +};
>     +
>     +static ssize_t ipsps_string_show(struct device *dev,
>     +				 struct device_attribute *devattr,
>     +				 char *buf)
>     +{
>     +	u8 reg;
>     +	int rc;
>     +	char *p;
>     +	char data[I2C_SMBUS_BLOCK_MAX + 1];
>     +	struct i2c_client *client = to_i2c_client(dev->parent);
>     +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>     +
>     +	reg = ipsps_regs[attr->index];
>     +	rc = i2c_smbus_read_block_data(client, reg, data);
>     +	if (rc < 0)
>     +		return rc;
>     +
>     +	/* filled with printable characters, ending with # */
>     +	p = memscan(data, '#', rc);
>     +	*p = '\0';
>     +
>     +	return snprintf(buf, PAGE_SIZE, "%s\n", data);
>     +}
>     +
>     +static ssize_t ipsps_fw_version_show(struct device *dev,
>     +				     struct device_attribute *devattr,
>     +				     char *buf)
>     +{
>     +	u8 reg;
>     +	int rc;
>     +	u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
>     +	struct i2c_client *client = to_i2c_client(dev->parent);
>     +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>     +
>     +	reg = ipsps_regs[attr->index];
>     +	rc = i2c_smbus_read_block_data(client, reg, data);
>     +	if (rc < 0)
>     +		return rc;
>     +
>     +	if (rc != 6)
>     +		return -EPROTO;
>     +
>     +	return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
>     +			data[1], data[2]/* < 100 */, data[3]/*< 10*/,
>     +			data[4], data[5]/* < 100 */);
>     +}
>     +
>     +static ssize_t ipsps_mode_show(struct device *dev,
>     +			       struct device_attribute *devattr, char *buf)
>     +{
>     +	u8 reg;
>     +	int rc;
>     +	struct i2c_client *client = to_i2c_client(dev->parent);
>     +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>     +
>     +	reg = ipsps_regs[attr->index];
>     +	rc = i2c_smbus_read_byte_data(client, reg);
>     +	if (rc < 0)
>     +		return rc;
>     +
>     +	switch (rc) {
>     +	case MODE_ACTIVE:
>     +		return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
>     +				MODE_ACTIVE_STRING,
>     +				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
>     +	case MODE_STANDBY:
>     +		return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
>     +				MODE_ACTIVE_STRING,
>     +				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
>     +	case MODE_REDUNDANCY:
>     +		return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
>     +				MODE_ACTIVE_STRING,
>     +				MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
>     +	default:
>     +		return snprintf(buf, PAGE_SIZE, "unspecified\n");
>     +	}
>     +}
>     +
>     +static ssize_t ipsps_mode_store(struct device *dev,
>     +				struct device_attribute *devattr,
>     +				const char *buf, size_t count)
>     +{
>     +	u8 reg;
>     +	int rc;
>     +	struct i2c_client *client = to_i2c_client(dev->parent);
>     +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>     +
>     +	reg = ipsps_regs[attr->index];
>     +	if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
>     +		rc = i2c_smbus_write_byte_data(client, reg,
>     +					       MODE_STANDBY);
>     +		if (rc < 0)
>     +			return rc;
>     +		return count;
>     +	} else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
>     +		rc = i2c_smbus_write_byte_data(client, reg,
>     +					       MODE_ACTIVE);
>     +		if (rc < 0)
>     +			return rc;
>     +		return count;
>     +	}
>     +
>     +	return -EINVAL;
>     +}
>     +
>     +static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
>     +static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
>     +static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
>     +static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
>     +static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
>     +static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
>     +static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
>     +
>     +static struct attribute *ipsps_attrs[] = {
>     +	&sensor_dev_attr_vendor.dev_attr.attr,
>     +	&sensor_dev_attr_model.dev_attr.attr,
>     +	&sensor_dev_attr_part_number.dev_attr.attr,
>     +	&sensor_dev_attr_serial_number.dev_attr.attr,
>     +	&sensor_dev_attr_hw_version.dev_attr.attr,
>     +	&sensor_dev_attr_fw_version.dev_attr.attr,
>     +	&sensor_dev_attr_mode.dev_attr.attr,
>     +	NULL,
>     +};
>     +
>     +ATTRIBUTE_GROUPS(ipsps);
>     +
>     +static struct pmbus_driver_info ipsps_info = {
>     +	.pages = 1,
>     +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
>     +		PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | 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,
>     +	.groups = ipsps_groups,
>     +};
>     +
>     +static struct pmbus_platform_data ipsps_pdata = {
>     +	.flags = PMBUS_SKIP_STATUS_CHECK,
>     +};
>     +
>     +static int ipsps_probe(struct i2c_client *client,
>     +		       const struct i2c_device_id *id)
>     +{
>     +	client->dev.platform_data = &ipsps_pdata;
>     +	return pmbus_do_probe(client, id, &ipsps_info);
>     +}
>     +
>     +static const struct i2c_device_id ipsps_id[] = {
>     +	{ "inspur_ipsps1", 0 },
>     +	{}
>     +};
>     +MODULE_DEVICE_TABLE(i2c, ipsps_id);
>     +
>     +static const struct of_device_id ipsps_of_match[] = {
>     +	{ .compatible = "inspur,ipsps1" },
>     +	{}
>     +};
>     +MODULE_DEVICE_TABLE(of, ipsps_of_match);
>     +
>     +static struct i2c_driver ipsps_driver = {
>     +	.driver = {
>     +		.name = "inspur-ipsps",
>     +		.of_match_table = ipsps_of_match,
>     +	},
>     +	.probe = ipsps_probe,
>     +	.remove = pmbus_do_remove,
>     +	.id_table = ipsps_id,
>     +};
>     +
>     +module_i2c_driver(ipsps_driver);
>     +
>     +MODULE_AUTHOR("John Wang");
>     +MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
>     +MODULE_LICENSE("GPL");
>     -- 
>     2.17.1
>     
>     
> 

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

* Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
  2019-08-16 10:19 [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver John Wang
  2019-08-16 16:01 ` Vijay Khemka
@ 2019-08-19  2:20 ` Joel Stanley
  2019-08-19  2:56   ` Guenter Roeck
  1 sibling, 1 reply; 5+ messages in thread
From: Joel Stanley @ 2019-08-19  2:20 UTC (permalink / raw)
  To: John Wang
  Cc: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
	linux-doc, Linux Kernel Mailing List, OpenBMC Maillist,
	duanzhijia01, Lei YU

On Fri, 16 Aug 2019 at 10:19, John Wang <wangzqbj@inspur.com> wrote:
>
> Add the driver to monitor Inspur Power System power supplies
> with hwmon over pmbus.
>
> This driver adds sysfs attributes for additional power supply data,
> including vendor, model, part_number, serial number,
> firmware revision, hardware revision, and psu mode(active/standby).
>
> Signed-off-by: John Wang <wangzqbj@inspur.com>

> +static const struct i2c_device_id ipsps_id[] = {
> +       { "inspur_ipsps1", 0 },

Convention would be to use "ipsps" here, instead of "vendor_device"?
> +       {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ipsps_id);
> +
> +static const struct of_device_id ipsps_of_match[] = {
> +       { .compatible = "inspur,ipsps1" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, ipsps_of_match);

Do we need the of match table? I thought the match on the device name
from the i2c table would be enough. I will defer to Guenter here
though.

Assuming the device tables are okay:

Reviewed-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel

> +
> +static struct i2c_driver ipsps_driver = {
> +       .driver = {
> +               .name = "inspur-ipsps",
> +               .of_match_table = ipsps_of_match,
> +       },
> +       .probe = ipsps_probe,
> +       .remove = pmbus_do_remove,
> +       .id_table = ipsps_id,
> +};

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

* Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
  2019-08-19  2:20 ` Joel Stanley
@ 2019-08-19  2:56   ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-08-19  2:56 UTC (permalink / raw)
  To: Joel Stanley, John Wang
  Cc: Jean Delvare, Jonathan Corbet, linux-hwmon, linux-doc,
	Linux Kernel Mailing List, OpenBMC Maillist, duanzhijia01,
	Lei YU

On 8/18/19 7:20 PM, Joel Stanley wrote:
> On Fri, 16 Aug 2019 at 10:19, John Wang <wangzqbj@inspur.com> wrote:
>>
>> Add the driver to monitor Inspur Power System power supplies
>> with hwmon over pmbus.
>>
>> This driver adds sysfs attributes for additional power supply data,
>> including vendor, model, part_number, serial number,
>> firmware revision, hardware revision, and psu mode(active/standby).
>>
>> Signed-off-by: John Wang <wangzqbj@inspur.com>
> 
>> +static const struct i2c_device_id ipsps_id[] = {
>> +       { "inspur_ipsps1", 0 },
> 
> Convention would be to use "ipsps" here, instead of "vendor_device"?

ipsps1, but good catch.

>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ipsps_id);
>> +
>> +static const struct of_device_id ipsps_of_match[] = {
>> +       { .compatible = "inspur,ipsps1" },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ipsps_of_match);
> 
> Do we need the of match table? I thought the match on the device name
> from the i2c table would be enough. I will defer to Guenter here
> though.
> 

Strictly speaking it is unnecessary, but it is kind of customary to have it.
The automatic detection also only works if the i2c device ID would be "ipsps1",
without vendor ID embedded.

I would recomment to have both, but name the i2c device "ipsps1" as you suggested,
for consistency.

I'll also have to check if we need of_match_ptr below in the assignment of
of_match_table. Probably not, but it would save a few bytes if CONFIG_OF
is not enabled.

Thanks,
Guenter


> Assuming the device tables are okay:
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> Cheers,
> 
> Joel
> 
>> +
>> +static struct i2c_driver ipsps_driver = {
>> +       .driver = {
>> +               .name = "inspur-ipsps",
>> +               .of_match_table = ipsps_of_match,
>> +       },
>> +       .probe = ipsps_probe,
>> +       .remove = pmbus_do_remove,
>> +       .id_table = ipsps_id,
>> +};
> 


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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16 10:19 [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver John Wang
2019-08-16 16:01 ` Vijay Khemka
2019-08-16 16:12   ` Guenter Roeck
2019-08-19  2:20 ` Joel Stanley
2019-08-19  2:56   ` Guenter Roeck

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox