[PATCH 0/3] pmbus: ibm-cffps: Add support for version 2 of PSU

[PATCH 0/3] pmbus: ibm-cffps: Add support for version 2 of PSU

[Eddie James]

Version 2 of this PSU supports a second page of data and changes the
format of the FW version command. Use the devicetree binding (or the I2C
device ID) to determine which version the driver should use. Therefore add
the new compatible string to the devicetree documentation and change the
Swift system devicetree to use version 2.

Eddie James (3):
  dt-bindings: hwmon: Document ibm,cffps2 compatible string
  ARM: dts: aspeed: swift: Change power supplies to version 2
  pmbus: ibm-cffps: Add support for version 2 of the PSU

 .../devicetree/bindings/hwmon/ibm,cffps1.txt       |   8 +-
 arch/arm/boot/dts/aspeed-bmc-opp-swift.dts         |   4 +-
 drivers/hwmon/pmbus/ibm-cffps.c                    | 109 ++++++++++++++++-----
 3 files changed, 94 insertions(+), 27 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] dt-bindings: hwmon: Document ibm,cffps2 compatible string

[Eddie James]

Document the compatible string for version 2 of the IBM CFFPS PSU.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 Documentation/devicetree/bindings/hwmon/ibm,cffps1.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/ibm,cffps1.txt b/Documentation/devicetree/bindings/hwmon/ibm,cffps1.txt
index f68a0a6..1036f65 100644
--- a/Documentation/devicetree/bindings/hwmon/ibm,cffps1.txt
+++ b/Documentation/devicetree/bindings/hwmon/ibm,cffps1.txt
@@ -1,8 +1,10 @@
-Device-tree bindings for IBM Common Form Factor Power Supply Version 1
-----------------------------------------------------------------------
+Device-tree bindings for IBM Common Form Factor Power Supply Versions 1 and 2
+-----------------------------------------------------------------------------
 
 Required properties:
- - compatible = "ibm,cffps1";
+ - compatible				: Must be one of the following:
+						"ibm,cffps1"
+						"ibm,cffps2"
  - reg = < I2C bus address >;		: Address of the power supply on the
 					  I2C bus.
 
-- 
1.8.3.1

[PATCH 2/3] ARM: dts: aspeed: swift: Change power supplies to version 2

[Eddie James]

Swift power supplies are version 2 of the IBM CFFPS. Make it so in the
devicetree.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-swift.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts b/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
index f14f745..815b865 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
@@ -494,7 +494,7 @@
 	};
 
 	power-supply@68 {
-		compatible = "ibm,cffps1";
+		compatible = "ibm,cffps2";
 		reg = <0x68>;
 	};
 
@@ -504,7 +504,7 @@
 	};
 
 	power-supply@69 {
-		compatible = "ibm,cffps1";
+		compatible = "ibm,cffps2";
 		reg = <0x69>;
 	};
 
-- 
1.8.3.1

[PATCH 3/3] pmbus: ibm-cffps: Add support for version 2 of the PSU

[Eddie James]

Version 2 of the PSU supports a second page of data and changes the
format of the FW version. Use the devicetree binding to differentiate
between the version the driver should use.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/hwmon/pmbus/ibm-cffps.c | 109 ++++++++++++++++++++++++++++++++--------
 1 file changed, 87 insertions(+), 22 deletions(-)

diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
index ee2ee9e..ca26fbd 100644
--- a/drivers/hwmon/pmbus/ibm-cffps.c
+++ b/drivers/hwmon/pmbus/ibm-cffps.c
@@ -12,16 +12,20 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_device.h>
 #include <linux/pmbus.h>
 
 #include "pmbus.h"
 
+#define CFFPS_VERSIONS				2
+
 #define CFFPS_FRU_CMD				0x9A
 #define CFFPS_PN_CMD				0x9B
 #define CFFPS_SN_CMD				0x9E
 #define CFFPS_CCIN_CMD				0xBD
-#define CFFPS_FW_CMD_START			0xFA
-#define CFFPS_FW_NUM_BYTES			4
+#define CFFPS_FW_CMD				0xFA
+#define CFFPS1_FW_NUM_BYTES			4
+#define CFFPS2_FW_NUM_WORDS			3
 #define CFFPS_SYS_CONFIG_CMD			0xDA
 
 #define CFFPS_INPUT_HISTORY_CMD			0xD6
@@ -61,6 +65,7 @@ struct ibm_cffps_input_history {
 };
 
 struct ibm_cffps {
+	int version;
 	struct i2c_client *client;
 
 	struct ibm_cffps_input_history input_history;
@@ -132,6 +137,8 @@ static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
 	struct ibm_cffps *psu = to_psu(idxp, idx);
 	char data[I2C_SMBUS_BLOCK_MAX] = { 0 };
 
+	pmbus_set_page(psu->client, 0);
+
 	switch (idx) {
 	case CFFPS_DEBUGFS_INPUT_HISTORY:
 		return ibm_cffps_read_input_history(psu, buf, count, ppos);
@@ -152,16 +159,36 @@ static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
 		rc = snprintf(data, 5, "%04X", rc);
 		goto done;
 	case CFFPS_DEBUGFS_FW:
-		for (i = 0; i < CFFPS_FW_NUM_BYTES; ++i) {
-			rc = i2c_smbus_read_byte_data(psu->client,
-						      CFFPS_FW_CMD_START + i);
-			if (rc < 0)
-				return rc;
+		switch (psu->version) {
+		case 1:
+			for (i = 0; i < CFFPS1_FW_NUM_BYTES; ++i) {
+				rc = i2c_smbus_read_byte_data(psu->client,
+							      CFFPS_FW_CMD +
+								i);
+				if (rc < 0)
+					return rc;
+
+				snprintf(&data[i * 2], 3, "%02X", rc);
+			}
 
-			snprintf(&data[i * 2], 3, "%02X", rc);
-		}
+			rc = i * 2;
+			break;
+		case 2:
+			for (i = 0; i < CFFPS2_FW_NUM_WORDS; ++i) {
+				rc = i2c_smbus_read_word_data(psu->client,
+							      CFFPS_FW_CMD +
+								i);
+				if (rc < 0)
+					return rc;
+
+				snprintf(&data[i * 4], 5, "%04X", rc);
+			}
 
-		rc = i * 2;
+			rc = i * 4;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
 		goto done;
 	default:
 		return -EINVAL;
@@ -279,6 +306,8 @@ static void ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
 			psu->led_state = CFFPS_LED_ON;
 	}
 
+	pmbus_set_page(psu->client, 0);
+
 	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
 				       psu->led_state);
 	if (rc < 0)
@@ -299,6 +328,8 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
 	if (led_cdev->brightness == LED_OFF)
 		return 0;
 
+	pmbus_set_page(psu->client, 0);
+
 	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
 				       CFFPS_LED_BLINK);
 	if (rc < 0)
@@ -328,15 +359,32 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
 		dev_warn(dev, "failed to register led class: %d\n", rc);
 }
 
-static struct pmbus_driver_info ibm_cffps_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 = ibm_cffps_read_byte_data,
-	.read_word_data = ibm_cffps_read_word_data,
+static struct pmbus_driver_info ibm_cffps_info[CFFPS_VERSIONS] = {
+	[0] = {
+		.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 = ibm_cffps_read_byte_data,
+		.read_word_data = ibm_cffps_read_word_data,
+	},
+	[1] = {
+		.pages = 2,
+		.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,
+		.func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+			PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+			PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT,
+		.read_byte_data = ibm_cffps_read_byte_data,
+		.read_word_data = ibm_cffps_read_word_data,
+	},
 };
 
 static struct pmbus_platform_data ibm_cffps_pdata = {
@@ -346,13 +394,21 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
 static int ibm_cffps_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
-	int i, rc;
+	int i, rc, vs;
 	struct dentry *debugfs;
 	struct dentry *ibm_cffps_dir;
 	struct ibm_cffps *psu;
+	const void *md = of_device_get_match_data(&client->dev);
+
+	if (md)
+		vs = (int)md;
+	else if (id)
+		vs = (int)id->driver_data;
+	else
+		vs = 1;
 
 	client->dev.platform_data = &ibm_cffps_pdata;
-	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
+	rc = pmbus_do_probe(client, id, &ibm_cffps_info[vs - 1]);
 	if (rc)
 		return rc;
 
@@ -364,6 +420,7 @@ static int ibm_cffps_probe(struct i2c_client *client,
 	if (!psu)
 		return 0;
 
+	psu->version = vs;
 	psu->client = client;
 	mutex_init(&psu->input_history.update_lock);
 	psu->input_history.last_update = jiffies - HZ;
@@ -406,12 +463,20 @@ static int ibm_cffps_probe(struct i2c_client *client,
 
 static const struct i2c_device_id ibm_cffps_id[] = {
 	{ "ibm_cffps1", 1 },
+	{ "ibm_cffps2", 2 },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, ibm_cffps_id);
 
 static const struct of_device_id ibm_cffps_of_match[] = {
-	{ .compatible = "ibm,cffps1" },
+	{
+		.compatible = "ibm,cffps1",
+		.data = (void *)1
+	},
+	{
+		.compatible = "ibm,cffps2",
+		.data = (void *)2
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, ibm_cffps_of_match);
-- 
1.8.3.1

Re: [PATCH 3/3] pmbus: ibm-cffps: Add support for version 2 of the PSU

[Guenter Roeck]

On Fri, Aug 30, 2019 at 11:09:45AM -0500, Eddie James wrote:
> Version 2 of the PSU supports a second page of data and changes the
> format of the FW version. Use the devicetree binding to differentiate
> between the version the driver should use.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/hwmon/pmbus/ibm-cffps.c | 109 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 87 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
> index ee2ee9e..ca26fbd 100644
> --- a/drivers/hwmon/pmbus/ibm-cffps.c
> +++ b/drivers/hwmon/pmbus/ibm-cffps.c
> @@ -12,16 +12,20 @@
>  #include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/of_device.h>
>  #include <linux/pmbus.h>
>  
>  #include "pmbus.h"
>  
> +#define CFFPS_VERSIONS				2
> +

Any chance you can use an enum for the versions ? Using version
numbers 1/2 combined with array indices 0/1 is confusing, error
prone, and seems unnecessary.

Thanks,
Guenter

>  #define CFFPS_FRU_CMD				0x9A
>  #define CFFPS_PN_CMD				0x9B
>  #define CFFPS_SN_CMD				0x9E
>  #define CFFPS_CCIN_CMD				0xBD
> -#define CFFPS_FW_CMD_START			0xFA
> -#define CFFPS_FW_NUM_BYTES			4
> +#define CFFPS_FW_CMD				0xFA
> +#define CFFPS1_FW_NUM_BYTES			4
> +#define CFFPS2_FW_NUM_WORDS			3
>  #define CFFPS_SYS_CONFIG_CMD			0xDA
>  
>  #define CFFPS_INPUT_HISTORY_CMD			0xD6
> @@ -61,6 +65,7 @@ struct ibm_cffps_input_history {
>  };
>  
>  struct ibm_cffps {
> +	int version;
>  	struct i2c_client *client;
>  
>  	struct ibm_cffps_input_history input_history;
> @@ -132,6 +137,8 @@ static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
>  	struct ibm_cffps *psu = to_psu(idxp, idx);
>  	char data[I2C_SMBUS_BLOCK_MAX] = { 0 };
>  
> +	pmbus_set_page(psu->client, 0);
> +
>  	switch (idx) {
>  	case CFFPS_DEBUGFS_INPUT_HISTORY:
>  		return ibm_cffps_read_input_history(psu, buf, count, ppos);
> @@ -152,16 +159,36 @@ static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
>  		rc = snprintf(data, 5, "%04X", rc);
>  		goto done;
>  	case CFFPS_DEBUGFS_FW:
> -		for (i = 0; i < CFFPS_FW_NUM_BYTES; ++i) {
> -			rc = i2c_smbus_read_byte_data(psu->client,
> -						      CFFPS_FW_CMD_START + i);
> -			if (rc < 0)
> -				return rc;
> +		switch (psu->version) {
> +		case 1:
> +			for (i = 0; i < CFFPS1_FW_NUM_BYTES; ++i) {
> +				rc = i2c_smbus_read_byte_data(psu->client,
> +							      CFFPS_FW_CMD +
> +								i);
> +				if (rc < 0)
> +					return rc;
> +
> +				snprintf(&data[i * 2], 3, "%02X", rc);
> +			}
>  
> -			snprintf(&data[i * 2], 3, "%02X", rc);
> -		}
> +			rc = i * 2;
> +			break;
> +		case 2:
> +			for (i = 0; i < CFFPS2_FW_NUM_WORDS; ++i) {
> +				rc = i2c_smbus_read_word_data(psu->client,
> +							      CFFPS_FW_CMD +
> +								i);
> +				if (rc < 0)
> +					return rc;
> +
> +				snprintf(&data[i * 4], 5, "%04X", rc);
> +			}
>  
> -		rc = i * 2;
> +			rc = i * 4;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
>  		goto done;
>  	default:
>  		return -EINVAL;
> @@ -279,6 +306,8 @@ static void ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
>  			psu->led_state = CFFPS_LED_ON;
>  	}
>  
> +	pmbus_set_page(psu->client, 0);
> +
>  	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
>  				       psu->led_state);
>  	if (rc < 0)
> @@ -299,6 +328,8 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
>  	if (led_cdev->brightness == LED_OFF)
>  		return 0;
>  
> +	pmbus_set_page(psu->client, 0);
> +
>  	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
>  				       CFFPS_LED_BLINK);
>  	if (rc < 0)
> @@ -328,15 +359,32 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
>  		dev_warn(dev, "failed to register led class: %d\n", rc);
>  }
>  
> -static struct pmbus_driver_info ibm_cffps_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 = ibm_cffps_read_byte_data,
> -	.read_word_data = ibm_cffps_read_word_data,
> +static struct pmbus_driver_info ibm_cffps_info[CFFPS_VERSIONS] = {
> +	[0] = {
> +		.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 = ibm_cffps_read_byte_data,
> +		.read_word_data = ibm_cffps_read_word_data,
> +	},
> +	[1] = {
> +		.pages = 2,
> +		.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,
> +		.func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> +			PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +			PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT,
> +		.read_byte_data = ibm_cffps_read_byte_data,
> +		.read_word_data = ibm_cffps_read_word_data,
> +	},
>  };
>  
>  static struct pmbus_platform_data ibm_cffps_pdata = {
> @@ -346,13 +394,21 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
>  static int ibm_cffps_probe(struct i2c_client *client,
>  			   const struct i2c_device_id *id)
>  {
> -	int i, rc;
> +	int i, rc, vs;
>  	struct dentry *debugfs;
>  	struct dentry *ibm_cffps_dir;
>  	struct ibm_cffps *psu;
> +	const void *md = of_device_get_match_data(&client->dev);
> +
> +	if (md)
> +		vs = (int)md;
> +	else if (id)
> +		vs = (int)id->driver_data;
> +	else
> +		vs = 1;
>  
>  	client->dev.platform_data = &ibm_cffps_pdata;
> -	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
> +	rc = pmbus_do_probe(client, id, &ibm_cffps_info[vs - 1]);
>  	if (rc)
>  		return rc;
>  
> @@ -364,6 +420,7 @@ static int ibm_cffps_probe(struct i2c_client *client,
>  	if (!psu)
>  		return 0;
>  
> +	psu->version = vs;
>  	psu->client = client;
>  	mutex_init(&psu->input_history.update_lock);
>  	psu->input_history.last_update = jiffies - HZ;
> @@ -406,12 +463,20 @@ static int ibm_cffps_probe(struct i2c_client *client,
>  
>  static const struct i2c_device_id ibm_cffps_id[] = {
>  	{ "ibm_cffps1", 1 },
> +	{ "ibm_cffps2", 2 },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, ibm_cffps_id);
>  
>  static const struct of_device_id ibm_cffps_of_match[] = {
> -	{ .compatible = "ibm,cffps1" },
> +	{
> +		.compatible = "ibm,cffps1",
> +		.data = (void *)1
> +	},
> +	{
> +		.compatible = "ibm,cffps2",
> +		.data = (void *)2
> +	},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, ibm_cffps_of_match);
> -- 
> 1.8.3.1
> 

Re: [PATCH 3/3] pmbus: ibm-cffps: Add support for version 2 of the PSU

[Eddie James]

On 8/30/19 12:36 PM, Guenter Roeck wrote:
> On Fri, Aug 30, 2019 at 11:09:45AM -0500, Eddie James wrote:
>> Version 2 of the PSU supports a second page of data and changes the
>> format of the FW version. Use the devicetree binding to differentiate
>> between the version the driver should use.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   drivers/hwmon/pmbus/ibm-cffps.c | 109 ++++++++++++++++++++++++++++++++--------
>>   1 file changed, 87 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
>> index ee2ee9e..ca26fbd 100644
>> --- a/drivers/hwmon/pmbus/ibm-cffps.c
>> +++ b/drivers/hwmon/pmbus/ibm-cffps.c
>> @@ -12,16 +12,20 @@
>>   #include <linux/leds.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>> +#include <linux/of_device.h>
>>   #include <linux/pmbus.h>
>>   
>>   #include "pmbus.h"
>>   
>> +#define CFFPS_VERSIONS				2
>> +
> Any chance you can use an enum for the versions ? Using version
> numbers 1/2 combined with array indices 0/1 is confusing, error
> prone, and seems unnecessary.


Sure, good idea.

Thanks,

Eddie


>
> Thanks,
> Guenter
>
>>   #define CFFPS_FRU_CMD				0x9A
>>   #define CFFPS_PN_CMD				0x9B
>>   #define CFFPS_SN_CMD				0x9E
>>   #define CFFPS_CCIN_CMD				0xBD
>> -#define CFFPS_FW_CMD_START			0xFA
>> -#define CFFPS_FW_NUM_BYTES			4
>> +#define CFFPS_FW_CMD				0xFA
>> +#define CFFPS1_FW_NUM_BYTES			4
>> +#define CFFPS2_FW_NUM_WORDS			3
>>   #define CFFPS_SYS_CONFIG_CMD			0xDA
>>   
>>   #define CFFPS_INPUT_HISTORY_CMD			0xD6
>> @@ -61,6 +65,7 @@ struct ibm_cffps_input_history {
>>   };
>>   
>>   struct ibm_cffps {
>> +	int version;
>>   	struct i2c_client *client;
>>   
>>   	struct ibm_cffps_input_history input_history;
>> @@ -132,6 +137,8 @@ static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
>>   	struct ibm_cffps *psu = to_psu(idxp, idx);
>>   	char data[I2C_SMBUS_BLOCK_MAX] = { 0 };
>>   
>> +	pmbus_set_page(psu->client, 0);
>> +
>>   	switch (idx) {
>>   	case CFFPS_DEBUGFS_INPUT_HISTORY:
>>   		return ibm_cffps_read_input_history(psu, buf, count, ppos);
>> @@ -152,16 +159,36 @@ static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
>>   		rc = snprintf(data, 5, "%04X", rc);
>>   		goto done;
>>   	case CFFPS_DEBUGFS_FW:
>> -		for (i = 0; i < CFFPS_FW_NUM_BYTES; ++i) {
>> -			rc = i2c_smbus_read_byte_data(psu->client,
>> -						      CFFPS_FW_CMD_START + i);
>> -			if (rc < 0)
>> -				return rc;
>> +		switch (psu->version) {
>> +		case 1:
>> +			for (i = 0; i < CFFPS1_FW_NUM_BYTES; ++i) {
>> +				rc = i2c_smbus_read_byte_data(psu->client,
>> +							      CFFPS_FW_CMD +
>> +								i);
>> +				if (rc < 0)
>> +					return rc;
>> +
>> +				snprintf(&data[i * 2], 3, "%02X", rc);
>> +			}
>>   
>> -			snprintf(&data[i * 2], 3, "%02X", rc);
>> -		}
>> +			rc = i * 2;
>> +			break;
>> +		case 2:
>> +			for (i = 0; i < CFFPS2_FW_NUM_WORDS; ++i) {
>> +				rc = i2c_smbus_read_word_data(psu->client,
>> +							      CFFPS_FW_CMD +
>> +								i);
>> +				if (rc < 0)
>> +					return rc;
>> +
>> +				snprintf(&data[i * 4], 5, "%04X", rc);
>> +			}
>>   
>> -		rc = i * 2;
>> +			rc = i * 4;
>> +			break;
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>>   		goto done;
>>   	default:
>>   		return -EINVAL;
>> @@ -279,6 +306,8 @@ static void ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
>>   			psu->led_state = CFFPS_LED_ON;
>>   	}
>>   
>> +	pmbus_set_page(psu->client, 0);
>> +
>>   	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
>>   				       psu->led_state);
>>   	if (rc < 0)
>> @@ -299,6 +328,8 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
>>   	if (led_cdev->brightness == LED_OFF)
>>   		return 0;
>>   
>> +	pmbus_set_page(psu->client, 0);
>> +
>>   	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
>>   				       CFFPS_LED_BLINK);
>>   	if (rc < 0)
>> @@ -328,15 +359,32 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
>>   		dev_warn(dev, "failed to register led class: %d\n", rc);
>>   }
>>   
>> -static struct pmbus_driver_info ibm_cffps_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 = ibm_cffps_read_byte_data,
>> -	.read_word_data = ibm_cffps_read_word_data,
>> +static struct pmbus_driver_info ibm_cffps_info[CFFPS_VERSIONS] = {
>> +	[0] = {
>> +		.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 = ibm_cffps_read_byte_data,
>> +		.read_word_data = ibm_cffps_read_word_data,
>> +	},
>> +	[1] = {
>> +		.pages = 2,
>> +		.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,
>> +		.func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
>> +			PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
>> +			PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT,
>> +		.read_byte_data = ibm_cffps_read_byte_data,
>> +		.read_word_data = ibm_cffps_read_word_data,
>> +	},
>>   };
>>   
>>   static struct pmbus_platform_data ibm_cffps_pdata = {
>> @@ -346,13 +394,21 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
>>   static int ibm_cffps_probe(struct i2c_client *client,
>>   			   const struct i2c_device_id *id)
>>   {
>> -	int i, rc;
>> +	int i, rc, vs;
>>   	struct dentry *debugfs;
>>   	struct dentry *ibm_cffps_dir;
>>   	struct ibm_cffps *psu;
>> +	const void *md = of_device_get_match_data(&client->dev);
>> +
>> +	if (md)
>> +		vs = (int)md;
>> +	else if (id)
>> +		vs = (int)id->driver_data;
>> +	else
>> +		vs = 1;
>>   
>>   	client->dev.platform_data = &ibm_cffps_pdata;
>> -	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
>> +	rc = pmbus_do_probe(client, id, &ibm_cffps_info[vs - 1]);
>>   	if (rc)
>>   		return rc;
>>   
>> @@ -364,6 +420,7 @@ static int ibm_cffps_probe(struct i2c_client *client,
>>   	if (!psu)
>>   		return 0;
>>   
>> +	psu->version = vs;
>>   	psu->client = client;
>>   	mutex_init(&psu->input_history.update_lock);
>>   	psu->input_history.last_update = jiffies - HZ;
>> @@ -406,12 +463,20 @@ static int ibm_cffps_probe(struct i2c_client *client,
>>   
>>   static const struct i2c_device_id ibm_cffps_id[] = {
>>   	{ "ibm_cffps1", 1 },
>> +	{ "ibm_cffps2", 2 },
>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(i2c, ibm_cffps_id);
>>   
>>   static const struct of_device_id ibm_cffps_of_match[] = {
>> -	{ .compatible = "ibm,cffps1" },
>> +	{
>> +		.compatible = "ibm,cffps1",
>> +		.data = (void *)1
>> +	},
>> +	{
>> +		.compatible = "ibm,cffps2",
>> +		.data = (void *)2
>> +	},
>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(of, ibm_cffps_of_match);
>> -- 
>> 1.8.3.1
>>

Re: [PATCH 3/3] pmbus: ibm-cffps: Add support for version 2 of the PSU

[kbuild test robot]

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

Hi Eddie,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eddie-James/pmbus-ibm-cffps-Add-support-for-version-2-of-PSU/20190901-151755
config: x86_64-randconfig-s0-09021304 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-11) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/hwmon/pmbus/ibm-cffps.c: In function 'ibm_cffps_probe':
>> drivers/hwmon/pmbus/ibm-cffps.c:404:8: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
      vs = (int)md;
           ^

vim +404 drivers/hwmon/pmbus/ibm-cffps.c

   393	
   394	static int ibm_cffps_probe(struct i2c_client *client,
   395				   const struct i2c_device_id *id)
   396	{
   397		int i, rc, vs;
   398		struct dentry *debugfs;
   399		struct dentry *ibm_cffps_dir;
   400		struct ibm_cffps *psu;
   401		const void *md = of_device_get_match_data(&client->dev);
   402	
   403		if (md)
 > 404			vs = (int)md;
   405		else if (id)
   406			vs = (int)id->driver_data;
   407		else
   408			vs = 1;
   409	
   410		client->dev.platform_data = &ibm_cffps_pdata;
   411		rc = pmbus_do_probe(client, id, &ibm_cffps_info[vs - 1]);
   412		if (rc)
   413			return rc;
   414	
   415		/*
   416		 * Don't fail the probe if there isn't enough memory for leds and
   417		 * debugfs.
   418		 */
   419		psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
   420		if (!psu)
   421			return 0;
   422	
   423		psu->version = vs;
   424		psu->client = client;
   425		mutex_init(&psu->input_history.update_lock);
   426		psu->input_history.last_update = jiffies - HZ;
   427	
   428		ibm_cffps_create_led_class(psu);
   429	
   430		/* Don't fail the probe if we can't create debugfs */
   431		debugfs = pmbus_get_debugfs_dir(client);
   432		if (!debugfs)
   433			return 0;
   434	
   435		ibm_cffps_dir = debugfs_create_dir(client->name, debugfs);
   436		if (!ibm_cffps_dir)
   437			return 0;
   438	
   439		for (i = 0; i < CFFPS_DEBUGFS_NUM_ENTRIES; ++i)
   440			psu->debugfs_entries[i] = i;
   441	
   442		debugfs_create_file("input_history", 0444, ibm_cffps_dir,
   443				    &psu->debugfs_entries[CFFPS_DEBUGFS_INPUT_HISTORY],
   444				    &ibm_cffps_fops);
   445		debugfs_create_file("fru", 0444, ibm_cffps_dir,
   446				    &psu->debugfs_entries[CFFPS_DEBUGFS_FRU],
   447				    &ibm_cffps_fops);
   448		debugfs_create_file("part_number", 0444, ibm_cffps_dir,
   449				    &psu->debugfs_entries[CFFPS_DEBUGFS_PN],
   450				    &ibm_cffps_fops);
   451		debugfs_create_file("serial_number", 0444, ibm_cffps_dir,
   452				    &psu->debugfs_entries[CFFPS_DEBUGFS_SN],
   453				    &ibm_cffps_fops);
   454		debugfs_create_file("ccin", 0444, ibm_cffps_dir,
   455				    &psu->debugfs_entries[CFFPS_DEBUGFS_CCIN],
   456				    &ibm_cffps_fops);
   457		debugfs_create_file("fw_version", 0444, ibm_cffps_dir,
   458				    &psu->debugfs_entries[CFFPS_DEBUGFS_FW],
   459				    &ibm_cffps_fops);
   460	
   461		return 0;
   462	}
   463	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30810 bytes --]