Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] hwmon: (pmbus/ibm-cffps) Fixes for the LED subsystem
@ 2019-11-06 20:01 Eddie James
  2019-11-06 20:01 ` [PATCH 1/2] hwmon: (pmbus/ibm-cffps) Switch LEDs to blocking brightness call Eddie James
  2019-11-06 20:01 ` [PATCH 2/2] hwmon: (pmbus/ibm-cffps) Fix LED blink behavior Eddie James
  0 siblings, 2 replies; 5+ messages in thread
From: Eddie James @ 2019-11-06 20:01 UTC (permalink / raw)
  To: linux-hwmon; +Cc: linux-kernel, linux, jdelvare, Eddie James

This series fixes a "scheduling while atomic" bug in the CFFPS driver and some
incorrect behavior with the LED blink functionality.

Eddie James (2):
  hwmon: (pmbus/ibm-cffps) Switch LEDs to blocking brightness call
  hwmon: (pmbus/ibm-cffps) Fix LED blink behavior

 drivers/hwmon/pmbus/ibm-cffps.c | 37 ++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 12 deletions(-)

-- 
2.23.0


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

* [PATCH 1/2] hwmon: (pmbus/ibm-cffps) Switch LEDs to blocking brightness call
  2019-11-06 20:01 [PATCH 0/2] hwmon: (pmbus/ibm-cffps) Fixes for the LED subsystem Eddie James
@ 2019-11-06 20:01 ` Eddie James
  2019-11-06 22:35   ` Guenter Roeck
  2019-11-06 20:01 ` [PATCH 2/2] hwmon: (pmbus/ibm-cffps) Fix LED blink behavior Eddie James
  1 sibling, 1 reply; 5+ messages in thread
From: Eddie James @ 2019-11-06 20:01 UTC (permalink / raw)
  To: linux-hwmon; +Cc: linux-kernel, linux, jdelvare, Eddie James

Since i2c_smbus functions can sleep, the brightness setting function
for this driver must be the blocking version to avoid scheduling while
atomic.

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

diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
index d61547ed0da0..c6a00e83aac6 100644
--- a/drivers/hwmon/pmbus/ibm-cffps.c
+++ b/drivers/hwmon/pmbus/ibm-cffps.c
@@ -297,8 +297,8 @@ 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)
+static int 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);
@@ -316,9 +316,11 @@ static void ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
 	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
 				       psu->led_state);
 	if (rc < 0)
-		return;
+		return rc;
 
 	led_cdev->brightness = brightness;
+
+	return 0;
 }
 
 static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
@@ -356,7 +358,7 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
 		 client->addr);
 	psu->led.name = psu->led_name;
 	psu->led.max_brightness = LED_FULL;
-	psu->led.brightness_set = ibm_cffps_led_brightness_set;
+	psu->led.brightness_set_blocking = ibm_cffps_led_brightness_set;
 	psu->led.blink_set = ibm_cffps_led_blink_set;
 
 	rc = devm_led_classdev_register(dev, &psu->led);
-- 
2.23.0


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

* [PATCH 2/2] hwmon: (pmbus/ibm-cffps) Fix LED blink behavior
  2019-11-06 20:01 [PATCH 0/2] hwmon: (pmbus/ibm-cffps) Fixes for the LED subsystem Eddie James
  2019-11-06 20:01 ` [PATCH 1/2] hwmon: (pmbus/ibm-cffps) Switch LEDs to blocking brightness call Eddie James
@ 2019-11-06 20:01 ` Eddie James
  2019-11-06 22:38   ` Guenter Roeck
  1 sibling, 1 reply; 5+ messages in thread
From: Eddie James @ 2019-11-06 20:01 UTC (permalink / raw)
  To: linux-hwmon; +Cc: linux-kernel, linux, jdelvare, Eddie James

The LED blink_set function incorrectly did not tell the PSU LED to blink
if brightness was LED_OFF. Fix this, and also correct the LED_OFF
command data, which should give control of the LED back to the PSU
firmware. Also prevent I2C failures from getting the driver LED state
out of sync and add some dev_dbg statements.

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

diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
index c6a00e83aac6..d359b76bcb36 100644
--- a/drivers/hwmon/pmbus/ibm-cffps.c
+++ b/drivers/hwmon/pmbus/ibm-cffps.c
@@ -44,9 +44,13 @@
 #define CFFPS_MFR_VAUX_FAULT			BIT(6)
 #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
 
+/*
+ * LED off state actually relinquishes LED control to PSU firmware, so it can
+ * turn on the LED for faults.
+ */
+#define CFFPS_LED_OFF				0
 #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 {
@@ -301,23 +305,31 @@ static int ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
 					enum led_brightness brightness)
 {
 	int rc;
+	u8 next_led_state;
 	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
 
 	if (brightness == LED_OFF) {
-		psu->led_state = CFFPS_LED_OFF;
+		next_led_state = CFFPS_LED_OFF;
 	} else {
 		brightness = LED_FULL;
+
 		if (psu->led_state != CFFPS_LED_BLINK)
-			psu->led_state = CFFPS_LED_ON;
+			next_led_state = CFFPS_LED_ON;
+		else
+			next_led_state = CFFPS_LED_BLINK;
 	}
 
+	dev_dbg(&psu->client->dev, "LED brightness set: %d. Command: %d.\n",
+		brightness, next_led_state);
+
 	pmbus_set_page(psu->client, 0);
 
 	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
-				       psu->led_state);
+				       next_led_state);
 	if (rc < 0)
 		return rc;
 
+	psu->led_state = next_led_state;
 	led_cdev->brightness = brightness;
 
 	return 0;
@@ -330,10 +342,7 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
 	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;
+	dev_dbg(&psu->client->dev, "LED blink set.\n");
 
 	pmbus_set_page(psu->client, 0);
 
@@ -342,6 +351,8 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
 	if (rc < 0)
 		return rc;
 
+	psu->led_state = CFFPS_LED_BLINK;
+	led_cdev->brightness = LED_FULL;
 	*delay_on = CFFPS_BLINK_RATE_MS;
 	*delay_off = CFFPS_BLINK_RATE_MS;
 
-- 
2.23.0


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

* Re: [PATCH 1/2] hwmon: (pmbus/ibm-cffps) Switch LEDs to blocking brightness call
  2019-11-06 20:01 ` [PATCH 1/2] hwmon: (pmbus/ibm-cffps) Switch LEDs to blocking brightness call Eddie James
@ 2019-11-06 22:35   ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-11-06 22:35 UTC (permalink / raw)
  To: Eddie James; +Cc: linux-hwmon, linux-kernel, jdelvare

On Wed, Nov 06, 2019 at 02:01:05PM -0600, Eddie James wrote:
> Since i2c_smbus functions can sleep, the brightness setting function
> for this driver must be the blocking version to avoid scheduling while
> atomic.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/ibm-cffps.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
> index d61547ed0da0..c6a00e83aac6 100644
> --- a/drivers/hwmon/pmbus/ibm-cffps.c
> +++ b/drivers/hwmon/pmbus/ibm-cffps.c
> @@ -297,8 +297,8 @@ 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)
> +static int 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);
> @@ -316,9 +316,11 @@ static void ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
>  	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
>  				       psu->led_state);
>  	if (rc < 0)
> -		return;
> +		return rc;
>  
>  	led_cdev->brightness = brightness;
> +
> +	return 0;
>  }
>  
>  static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
> @@ -356,7 +358,7 @@ static void ibm_cffps_create_led_class(struct ibm_cffps *psu)
>  		 client->addr);
>  	psu->led.name = psu->led_name;
>  	psu->led.max_brightness = LED_FULL;
> -	psu->led.brightness_set = ibm_cffps_led_brightness_set;
> +	psu->led.brightness_set_blocking = ibm_cffps_led_brightness_set;
>  	psu->led.blink_set = ibm_cffps_led_blink_set;
>  
>  	rc = devm_led_classdev_register(dev, &psu->led);

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

* Re: [PATCH 2/2] hwmon: (pmbus/ibm-cffps) Fix LED blink behavior
  2019-11-06 20:01 ` [PATCH 2/2] hwmon: (pmbus/ibm-cffps) Fix LED blink behavior Eddie James
@ 2019-11-06 22:38   ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-11-06 22:38 UTC (permalink / raw)
  To: Eddie James; +Cc: linux-hwmon, linux-kernel, jdelvare

On Wed, Nov 06, 2019 at 02:01:06PM -0600, Eddie James wrote:
> The LED blink_set function incorrectly did not tell the PSU LED to blink
> if brightness was LED_OFF. Fix this, and also correct the LED_OFF
> command data, which should give control of the LED back to the PSU
> firmware. Also prevent I2C failures from getting the driver LED state
> out of sync and add some dev_dbg statements.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/ibm-cffps.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
> index c6a00e83aac6..d359b76bcb36 100644
> --- a/drivers/hwmon/pmbus/ibm-cffps.c
> +++ b/drivers/hwmon/pmbus/ibm-cffps.c
> @@ -44,9 +44,13 @@
>  #define CFFPS_MFR_VAUX_FAULT			BIT(6)
>  #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
>  
> +/*
> + * LED off state actually relinquishes LED control to PSU firmware, so it can
> + * turn on the LED for faults.
> + */
> +#define CFFPS_LED_OFF				0
>  #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 {
> @@ -301,23 +305,31 @@ static int ibm_cffps_led_brightness_set(struct led_classdev *led_cdev,
>  					enum led_brightness brightness)
>  {
>  	int rc;
> +	u8 next_led_state;
>  	struct ibm_cffps *psu = container_of(led_cdev, struct ibm_cffps, led);
>  
>  	if (brightness == LED_OFF) {
> -		psu->led_state = CFFPS_LED_OFF;
> +		next_led_state = CFFPS_LED_OFF;
>  	} else {
>  		brightness = LED_FULL;
> +
>  		if (psu->led_state != CFFPS_LED_BLINK)
> -			psu->led_state = CFFPS_LED_ON;
> +			next_led_state = CFFPS_LED_ON;
> +		else
> +			next_led_state = CFFPS_LED_BLINK;
>  	}
>  
> +	dev_dbg(&psu->client->dev, "LED brightness set: %d. Command: %d.\n",
> +		brightness, next_led_state);
> +
>  	pmbus_set_page(psu->client, 0);
>  
>  	rc = i2c_smbus_write_byte_data(psu->client, CFFPS_SYS_CONFIG_CMD,
> -				       psu->led_state);
> +				       next_led_state);
>  	if (rc < 0)
>  		return rc;
>  
> +	psu->led_state = next_led_state;
>  	led_cdev->brightness = brightness;
>  
>  	return 0;
> @@ -330,10 +342,7 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
>  	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;
> +	dev_dbg(&psu->client->dev, "LED blink set.\n");
>  
>  	pmbus_set_page(psu->client, 0);
>  
> @@ -342,6 +351,8 @@ static int ibm_cffps_led_blink_set(struct led_classdev *led_cdev,
>  	if (rc < 0)
>  		return rc;
>  
> +	psu->led_state = CFFPS_LED_BLINK;
> +	led_cdev->brightness = LED_FULL;
>  	*delay_on = CFFPS_BLINK_RATE_MS;
>  	*delay_off = CFFPS_BLINK_RATE_MS;
>  

^ 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-11-06 20:01 [PATCH 0/2] hwmon: (pmbus/ibm-cffps) Fixes for the LED subsystem Eddie James
2019-11-06 20:01 ` [PATCH 1/2] hwmon: (pmbus/ibm-cffps) Switch LEDs to blocking brightness call Eddie James
2019-11-06 22:35   ` Guenter Roeck
2019-11-06 20:01 ` [PATCH 2/2] hwmon: (pmbus/ibm-cffps) Fix LED blink behavior Eddie James
2019-11-06 22:38   ` 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
	public-inbox-index linux-hwmon

Example config snippet for mirrors

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.git