hwmon (pmbus): cffps: Add led class device for power supply fault led

PATCH Superseded Eddie James 2 participants 3 messages Jan 9, 2018 – Jan 10, 2018 Hardware monitoring

Share & tools

Eddie James

author

2018-01-09 22:05 UTC #0 lore raw

PATCH 1/1 hwmon (pmbus): cffps: Add led class device for power supply fault led

This power supply device doesn't correctly manage it's own fault led. Add an led class device and register it so that userspace can manage power supply fault led as necessary.

Signed-off-by Eddie James <[email protected]>

---
 drivers/hwmon/pmbus/ibm-cffps.c | 90 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 8 deletions(-)

drivers/hwmon/pmbus/ibm-cffps.c +82 -8

index 2d6f4f4..1e95dea 100644
@@ -13,6 +13,7 @@
 #include <linux/fs.h>
 #include <linux/i2c.h>
 #include <linux/jiffies.h>
+#include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/pmbus.h>

@@ -25,6 +26,7 @@
 #define CFFPS_CCIN_CMD				0xBD
 #define CFFPS_FW_CMD_START			0xFA
 #define CFFPS_FW_NUM_BYTES			4
+#define CFFPS_SYS_CONFIG_CMD			0xDA
 
 #define CFFPS_INPUT_HISTORY_CMD			0xD6
 #define CFFPS_INPUT_HISTORY_SIZE		100

@@ -39,6 +41,11 @@
 #define CFFPS_MFR_VAUX_FAULT			BIT(6)
 #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
 
+#define CFFPS_LED_BLINK				BIT(0)
+#define CFFPS_LED_ON				BIT(1)
+#define CFFPS_LED_OFF				BIT(2)
+#define CFFPS_BLINK_RATE_MS			250
+
 enum {
 	CFFPS_DEBUGFS_INPUT_HISTORY = 0,
 	CFFPS_DEBUGFS_FRU,

@@ -63,6 +70,9 @@ struct ibm_cffps {
 	struct ibm_cffps_input_history input_history;
 
 	int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];
+
+	u8 led_state;
+	struct led_classdev led;
 };
 
 #define to_psu(x, y) container_of((x), struct ibm_cffps, debugfs_entries[(y)])

@@ -258,6 +268,51 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
 	return rc;
 }
 
+static void ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
+					 enum led_brightness brightness)
+{
+	int rc;
+	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
+
+	if (brightness == LED_OFF) {
+		psu->led_state = CFFPS_LED_OFF;
+	} else {
+		brightness = LED_FULL;
+		if (psu->led_state != CFFPS_LED_BLINK)
+			psu->led_state = CFFPS_LED_ON;
+	}
+
+	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
+				       psu->led_state);
+	if (rc < 0)
+		return;
+
+	led_cdev->brightness = brightness;
+}
+
+static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
+				   unsigned long *delay_on,
+				   unsigned long *delay_off)
+{
+	int rc;
+	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
+
+	psu->led_state = CFFPS_LED_BLINK;
+
+	if (led_cdev->brightness == LED_OFF)
+		return 0;
+
+	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
+				       CFFPS_LED_BLINK);
+	if (rc < 0)
+		return rc;
+
+	*delay_on = CFFPS_BLINK_RATE_MS;
+	*delay_off = CFFPS_BLINK_RATE_MS;
+
+	return 0;
+}
+
 static struct pmbus_driver_info ibm_cffps_info = {
 	.pages = 1,
 	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |

@@ -277,6 +332,8 @@ static int ibm_cffps_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
 	int i, rc;
+	char *led_name;
+	size_t name_length;
 	struct dentry *debugfs;
 	struct dentry *ibm_cffps_dir;
 	struct ibm_cffps *psu;

@@ -286,6 +343,31 @@ static int ibm_cffps_probe(struct i2c_client *client,
 	if (rc)
 		return rc;
 
+	/* client name, '-', 8 chars for addr, and null */
+	name_length = strlen(client->name) + 10;
+
+	/* Don't fail the probe if we can't create led devices */
+	psu = devm_kzalloc(&client->dev, sizeof(*psu) + name_length,
+			   GFP_KERNEL);
+	if (!psu)
+		return 0;
+
+	psu->client = client;
+	mutex_init(&psu->input_history.update_lock);
+	psu->input_history.last_update = jiffies - HZ;
+
+	led_name = (void *)(&psu[1]);
+	snprintf(led_name, name_length, "%s-%x", client->name, client->addr);
+	psu->led.name = led_name;
+	psu->led.max_brightness = LED_FULL;
+	psu->led.brightness_set = ibm_cffps_led_brightness_set;
+	psu->led.blink_set = ibm_cffps_led_blink_set;
+
+	rc = devm_led_classdev_register(&client->dev, &psu->led);
+	if (rc)
+		dev_info(&client->dev, "failed to register led class: %d\n",
+			 rc);
+
 	/* Don't fail the probe if we can't create debugfs */
 	debugfs = pmbus_get_debugfs_dir(client);
 	if (!debugfs)

@@ -295,14 +377,6 @@ static int ibm_cffps_probe(struct i2c_client *client,
 	if (!ibm_cffps_dir)
 		return 0;
 
-	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
-	if (!psu)
-		return 0;
-
-	psu->client = client;
-	mutex_init(&psu->input_history.update_lock);
-	psu->input_history.last_update = jiffies - HZ;
-
 	for (i = 0; i < CFFPS_DEBUGFS_NUM_ENTRIES; ++i)
 		psu->debugfs_entries[i] = i;

Signature · 1 line

1.8.3.1

Guenter Roeck

maintainer

2018-01-09 22:50 UTC #1 lore raw

Quoted 114 lines · Eddie James

This power supply device doesn't correctly manage it's own fault led. Add an led class device and register it so that userspace can manage power supply fault led as necessary.

Signed-off-by Eddie James <[email protected]>

---
 drivers/hwmon/pmbus/ibm-cffps.c | 90 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 8 deletions(-)

drivers/hwmon/pmbus/ibm-cffps.c +57 -0

index 2d6f4f4..1e95dea 100644
@@ -13,6 +13,7 @@
 #include <linux/fs.h>
 #include <linux/i2c.h>
 #include <linux/jiffies.h>
+#include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/pmbus.h>

@@ -25,6 +26,7 @@
 #define CFFPS_CCIN_CMD				0xBD
 #define CFFPS_FW_CMD_START			0xFA
 #define CFFPS_FW_NUM_BYTES			4
+#define CFFPS_SYS_CONFIG_CMD			0xDA
 
 #define CFFPS_INPUT_HISTORY_CMD			0xD6
 #define CFFPS_INPUT_HISTORY_SIZE		100

@@ -39,6 +41,11 @@
 #define CFFPS_MFR_VAUX_FAULT			BIT(6)
 #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
 
+#define CFFPS_LED_BLINK				BIT(0)
+#define CFFPS_LED_ON				BIT(1)
+#define CFFPS_LED_OFF				BIT(2)
+#define CFFPS_BLINK_RATE_MS			250
+
 enum {
 	CFFPS_DEBUGFS_INPUT_HISTORY = 0,
 	CFFPS_DEBUGFS_FRU,

@@ -63,6 +70,9 @@ struct ibm_cffps {
 	struct ibm_cffps_input_history input_history;
 
 	int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];
+
+	u8 led_state;
+	struct led_classdev led;
 };
 
 #define to_psu(x, y) container_of((x), struct ibm_cffps, debugfs_entries[(y)])

@@ -258,6 +268,51 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
 	return rc;
 }
 
+static void ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
+					 enum led_brightness brightness)
+{
+	int rc;
+	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
+
+	if (brightness == LED_OFF) {
+		psu->led_state = CFFPS_LED_OFF;
+	} else {
+		brightness = LED_FULL;
+		if (psu->led_state != CFFPS_LED_BLINK)
+			psu->led_state = CFFPS_LED_ON;
+	}
+
+	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
+				       psu->led_state);
+	if (rc < 0)
+		return;
+
+	led_cdev->brightness = brightness;
+}
+
+static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
+				   unsigned long *delay_on,
+				   unsigned long *delay_off)
+{
+	int rc;
+	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
+
+	psu->led_state = CFFPS_LED_BLINK;
+
+	if (led_cdev->brightness == LED_OFF)
+		return 0;
+
+	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
+				       CFFPS_LED_BLINK);
+	if (rc < 0)
+		return rc;
+
+	*delay_on = CFFPS_BLINK_RATE_MS;
+	*delay_off = CFFPS_BLINK_RATE_MS;
+
+	return 0;
+}
+
 static struct pmbus_driver_info ibm_cffps_info = {
 	.pages = 1,
 	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |

@@ -277,6 +332,8 @@ static int ibm_cffps_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
 	int i, rc;
+	char *led_name;
+	size_t name_length;
 	struct dentry *debugfs;
 	struct dentry *ibm_cffps_dir;
 	struct ibm_cffps *psu;

@@ -286,6 +343,31 @@ static int ibm_cffps_probe(struct i2c_client *client,
 	if (rc)

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_probe(struct i2c_client *client, · line 353

Quoted 10 lines

drivers/hwmon/pmbus/ibm-cffps.c +8 -0

 		return rc;
 
+	/* client name, '-', 8 chars for addr, and null */
+	name_length = strlen(client->name) + 10;
+
+	/* Don't fail the probe if we can't create led devices */
+	psu = devm_kzalloc(&client->dev, sizeof(*psu) + name_length,
+			   GFP_KERNEL);
+	if (!psu)
+		return 0;

... and no debugfs either ?

Quoted 5 lines

drivers/hwmon/pmbus/ibm-cffps.c +5 -0

+
+	psu->client = client;
+	mutex_init(&psu->input_history.update_lock);
+	psu->input_history.last_update = jiffies - HZ;
+

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_probe(struct i2c_client *client, · line 368

Quoted 10 lines

drivers/hwmon/pmbus/ibm-cffps.c +10 -0

+	led_name = (void *)(&psu[1]);
+	snprintf(led_name, name_length, "%s-%x", client->name, client->addr);
+	psu->led.name = led_name;
+	psu->led.max_brightness = LED_FULL;
+	psu->led.brightness_set = ibm_cffps_led_brightness_set;
+	psu->led.blink_set = ibm_cffps_led_blink_set;
+
+	rc = devm_led_classdev_register(&client->dev, &psu->led);
+	if (rc)
+		dev_info(&client->dev, "failed to register led class: %d\n",

dev_warn() might be more appropriate.

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_probe(struct i2c_client *client, · line 370

Quoted 2 lines

drivers/hwmon/pmbus/ibm-cffps.c +2 -0
+			 rc);
+

Please move the led initialization code into its own function.

Quoted 5 lines

drivers/hwmon/pmbus/ibm-cffps.c +0 -0

 	/* Don't fail the probe if we can't create debugfs */
 	debugfs = pmbus_get_debugfs_dir(client);
 	if (!debugfs)

@@ -295,14 +377,6 @@ static int ibm_cffps_probe(struct i2c_client *client,
 	if (!ibm_cffps_dir)

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_probe(struct i2c_client *client, · line 305

Quoted 10 lines

drivers/hwmon/pmbus/ibm-cffps.c +0 -8

 		return 0;
 
-	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
-	if (!psu)
-		return 0;
-
-	psu->client = client;
-	mutex_init(&psu->input_history.update_lock);
-	psu->input_history.last_update = jiffies - HZ;
-

unrelated ?

Quoted 6 lines

drivers/hwmon/pmbus/ibm-cffps.c +0 -0
 	for (i = 0; i < CFFPS_DEBUGFS_NUM_ENTRIES; ++i)
 		psu->debugfs_entries[i] = i;
 
Signature · 1 line
1.8.3.1

Eddie James

author

2018-01-10 16:36 UTC #2 lore raw

Quoted 117 lines · Guenter Roeck

Quoted 116 lines · Eddie James

This power supply device doesn't correctly manage it's own fault led. Add an led class device and register it so that userspace can manage power supply fault led as necessary.

Signed-off-by Eddie James <[email protected]>

---
  drivers/hwmon/pmbus/ibm-cffps.c | 90 +++++++++++++++++++++++++++++++++++++----
  1 file changed, 82 insertions(+), 8 deletions(-)

drivers/hwmon/pmbus/ibm-cffps.c +57 -0

index 2d6f4f4..1e95dea 100644
@@ -13,6 +13,7 @@
  #include <linux/fs.h>
  #include <linux/i2c.h>
  #include <linux/jiffies.h>
+#include <linux/leds.h>
  #include <linux/module.h>
  #include <linux/mutex.h>
  #include <linux/pmbus.h>

@@ -25,6 +26,7 @@
  #define CFFPS_CCIN_CMD				0xBD
  #define CFFPS_FW_CMD_START			0xFA
  #define CFFPS_FW_NUM_BYTES			4
+#define CFFPS_SYS_CONFIG_CMD			0xDA
 
  #define CFFPS_INPUT_HISTORY_CMD			0xD6
  #define CFFPS_INPUT_HISTORY_SIZE		100

@@ -39,6 +41,11 @@
  #define CFFPS_MFR_VAUX_FAULT			BIT(6)
  #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
 
+#define CFFPS_LED_BLINK				BIT(0)
+#define CFFPS_LED_ON				BIT(1)
+#define CFFPS_LED_OFF				BIT(2)
+#define CFFPS_BLINK_RATE_MS			250
+
  enum {
  	CFFPS_DEBUGFS_INPUT_HISTORY = 0,
  	CFFPS_DEBUGFS_FRU,

@@ -63,6 +70,9 @@ struct ibm_cffps {
  	struct ibm_cffps_input_history input_history;
 
  	int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];
+
+	u8 led_state;
+	struct led_classdev led;
  };
 
  #define to_psu(x, y) container_of((x), struct ibm_cffps, debugfs_entries[(y)])

@@ -258,6 +268,51 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
  	return rc;
  }
 
+static void ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
+					 enum led_brightness brightness)
+{
+	int rc;
+	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
+
+	if (brightness == LED_OFF) {
+		psu->led_state = CFFPS_LED_OFF;
+	} else {
+		brightness = LED_FULL;
+		if (psu->led_state != CFFPS_LED_BLINK)
+			psu->led_state = CFFPS_LED_ON;
+	}
+
+	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
+				       psu->led_state);
+	if (rc < 0)
+		return;
+
+	led_cdev->brightness = brightness;
+}
+
+static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
+				   unsigned long *delay_on,
+				   unsigned long *delay_off)
+{
+	int rc;
+	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
+
+	psu->led_state = CFFPS_LED_BLINK;
+
+	if (led_cdev->brightness == LED_OFF)
+		return 0;
+
+	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
+				       CFFPS_LED_BLINK);
+	if (rc < 0)
+		return rc;
+
+	*delay_on = CFFPS_BLINK_RATE_MS;
+	*delay_off = CFFPS_BLINK_RATE_MS;
+
+	return 0;
+}
+
  static struct pmbus_driver_info ibm_cffps_info = {
  	.pages = 1,
  	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |

@@ -277,6 +332,8 @@ static int ibm_cffps_probe(struct i2c_client *client,
  			   const struct i2c_device_id *id)
  {
  	int i, rc;
+	char *led_name;
+	size_t name_length;
  	struct dentry *debugfs;
  	struct dentry *ibm_cffps_dir;
  	struct ibm_cffps *psu;

@@ -286,6 +343,31 @@ static int ibm_cffps_probe(struct i2c_client *client,
  	if (rc)
  		return rc;

Quoted 9 lines

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_probe(struct i2c_client *client, · line 353
Quoted 8 lines
drivers/hwmon/pmbus/ibm-cffps.c +8 -0
+	/* client name, '-', 8 chars for addr, and null */
+	name_length = strlen(client->name) + 10;
+
+	/* Don't fail the probe if we can't create led devices */
+	psu = devm_kzalloc(&client->dev, sizeof(*psu) + name_length,
+			   GFP_KERNEL);
+	if (!psu)
+		return 0;
... and no debugfs either ?

Well we need that struct allocated for debugfs as well. I'll clarify the comment.

Quoted 17 lines

Quoted 6 lines

drivers/hwmon/pmbus/ibm-cffps.c +5 -0

+
+	psu->client = client;
+	mutex_init(&psu->input_history.update_lock);
+	psu->input_history.last_update = jiffies - HZ;
+

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_probe(struct i2c_client *client, · line 368

Quoted 10 lines

drivers/hwmon/pmbus/ibm-cffps.c +10 -0

+	led_name = (void *)(&psu[1]);
+	snprintf(led_name, name_length, "%s-%x", client->name, client->addr);
+	psu->led.name = led_name;
+	psu->led.max_brightness = LED_FULL;
+	psu->led.brightness_set = ibm_cffps_led_brightness_set;
+	psu->led.blink_set = ibm_cffps_led_blink_set;
+
+	rc = devm_led_classdev_register(&client->dev, &psu->led);
+	if (rc)
+		dev_info(&client->dev, "failed to register led class: %d\n",

dev_warn() might be more appropriate.

Sure.

Quoted 4 lines

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_probe(struct i2c_client *client, · line 370
Quoted 3 lines
drivers/hwmon/pmbus/ibm-cffps.c +2 -0
+			 rc);
+
Please move the led initialization code into its own function.

Sure.

Quoted 8 lines

drivers/hwmon/pmbus/ibm-cffps.c +0 -0

  	/* Don't fail the probe if we can't create debugfs */
  	debugfs = pmbus_get_debugfs_dir(client);
  	if (!debugfs)

@@ -295,14 +377,6 @@ static int ibm_cffps_probe(struct i2c_client *client,
  	if (!ibm_cffps_dir)
  		return 0;

Quoted 9 lines

drivers/hwmon/pmbus/ibm-cffps.c static int ibm_cffps_probe(struct i2c_client *client, · line 305
Quoted 8 lines
drivers/hwmon/pmbus/ibm-cffps.c +0 -8
-	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
-	if (!psu)
-		return 0;
-
-	psu->client = client;
-	mutex_init(&psu->input_history.update_lock);
-	psu->input_history.last_update = jiffies - HZ;
-
unrelated ?

Thought it made sense to move it with the allocation, but I can leave it where it is.

Thanks,
Eddie

Quoted 7 lines

Quoted 7 lines
drivers/hwmon/pmbus/ibm-cffps.c +0 -0
  	for (i = 0; i < CFFPS_DEBUGFS_NUM_ENTRIES; ++i)
  		psu->debugfs_entries[i] = i;
 
Signature · 1 line
1.8.3.1