diff mbox series

[3/5] leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs API

Message ID 20230413151808.20900-4-hdegoede@redhat.com
State Superseded
Headers show
Series leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver | expand

Commit Message

Hans de Goede April 13, 2023, 3:18 p.m. UTC 
The hw-blinking of the LED controller in the Whiskey Cove PMIC can also
be used for a hw-breathing effect.

As discussed during review of v2 of the submission of the new
leds-cht-wcove driver, the LED subsystem already supports breathing mode
on several other LED controllers using the hw_pattern interface.

Implement a pattern_set callback to implement breathing mode modelled
after the breathing mode supported by the SC27xx breathing light and
Crane EL15203000 LED drivers. The Whiskey Cove PMIC's breathing mode
is closer to the EL15203000 one then to the SC27xx one since it does
not support staying high / low for a specific time, it only supports
rise and fall times.

As such the supported hw_pattern and the documentation for this is almost
a 1:1 copy of the pattern/docs for the EL15203000 breathing mode.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Link: https://lore.kernel.org/all/6beed61c-1fc6-6525-e873-a8978f5fbffb@gmail.com/
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/leds/index.rst          |  1 +
 Documentation/leds/leds-cht-wcove.rst | 29 ++++++++++++++++++
 drivers/leds/leds-cht-wcove.c         | 42 ++++++++++++++++++++++++---
 3 files changed, 68 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/leds/leds-cht-wcove.rst

Comments

Jacek Anaszewski April 17, 2023, 8 p.m. UTC | #1 
Hi Hans,

On 4/16/23 22:31, Hans de Goede wrote:
> Hi Jacek,
> 
> On 4/16/23 17:17, Jacek Anaszewski wrote:
>> Hi Hans,
>>
[...]
>>> +===========================================================
>>> +Kernel driver for Intel Cherry Trail Whiskey Cove PMIC LEDs
>>> +===========================================================
>>> +
>>> +/sys/class/leds/<led>/hw_pattern
>>> +--------------------------------
>>> +
>>> +Specify a hardware pattern for the Whiskey Cove PMIC LEDs.
>>> +
>>> +The only supported pattern is hardware breathing mode::
>>> +
>>> +    "0 2000 1 2000"
>>
>> Why 1? What is the peek brightness in this mode?
> 
> 255, but the pattern only controls the timing. For max brightness
> the last set brightness is used and the max brightness can be changed
> while breathing by writing the brightness attribute.
> 
> This is just like how blinking works in the LED subsystem,
> for both sw and hw blinking the brightness can also be changed
> while blinking. Breathing on this hw really is just a variant
> mode of blinking.

Thanks for the explanation. So it would be nice to have it
also mentioned explicitly here.
Hans de Goede April 19, 2023, 1:05 p.m. UTC | #2 
Hi,

On 4/17/23 22:00, Jacek Anaszewski wrote:
> Hi Hans,
> 
> On 4/16/23 22:31, Hans de Goede wrote:
>> Hi Jacek,
>>
>> On 4/16/23 17:17, Jacek Anaszewski wrote:
>>> Hi Hans,
>>>
> [...]
>>>> +===========================================================
>>>> +Kernel driver for Intel Cherry Trail Whiskey Cove PMIC LEDs
>>>> +===========================================================
>>>> +
>>>> +/sys/class/leds/<led>/hw_pattern
>>>> +--------------------------------
>>>> +
>>>> +Specify a hardware pattern for the Whiskey Cove PMIC LEDs.
>>>> +
>>>> +The only supported pattern is hardware breathing mode::
>>>> +
>>>> +    "0 2000 1 2000"
>>>
>>> Why 1? What is the peek brightness in this mode?
>>
>> 255, but the pattern only controls the timing. For max brightness
>> the last set brightness is used and the max brightness can be changed
>> while breathing by writing the brightness attribute.
>>
>> This is just like how blinking works in the LED subsystem,
>> for both sw and hw blinking the brightness can also be changed
>> while blinking. Breathing on this hw really is just a variant
>> mode of blinking.
> 
> Thanks for the explanation. So it would be nice to have it
> also mentioned explicitly here.

Ack, I'll add something like the above explanation to the
docs for v2.

Regards,

Hans
diff mbox series

Patch

diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
index b9ca081fac71..c92612271e25 100644
--- a/Documentation/leds/index.rst
+++ b/Documentation/leds/index.rst
@@ -17,6 +17,7 @@  LEDs
    uleds
 
    leds-blinkm
+   leds-cht-wcove
    leds-el15203000
    leds-lm3556
    leds-lp3944
diff --git a/Documentation/leds/leds-cht-wcove.rst b/Documentation/leds/leds-cht-wcove.rst
new file mode 100644
index 000000000000..fa79d8fd7ef8
--- /dev/null
+++ b/Documentation/leds/leds-cht-wcove.rst
@@ -0,0 +1,29 @@ 
+.. SPDX-License-Identifier: GPL-2.0
+
+===========================================================
+Kernel driver for Intel Cherry Trail Whiskey Cove PMIC LEDs
+===========================================================
+
+/sys/class/leds/<led>/hw_pattern
+--------------------------------
+
+Specify a hardware pattern for the Whiskey Cove PMIC LEDs.
+
+The only supported pattern is hardware breathing mode::
+
+    "0 2000 1 2000"
+
+	^
+	|
+    Max-|     ---
+	|    /   \
+	|   /     \
+	|  /       \     /
+	| /         \   /
+    Min-|-           ---
+	|
+	0------2------4--> time (sec)
+
+The rise and fall times must be the same value.
+Supported values are 2000, 1000, 500 and 250 for
+breathing frequencies of 1/4, 1/2, 1 and 2 Hz.
diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
index 2d968ddd18c5..2b6c6f39d406 100644
--- a/drivers/leds/leds-cht-wcove.c
+++ b/drivers/leds/leds-cht-wcove.c
@@ -214,9 +214,10 @@  static int cht_wc_leds_find_freq(unsigned long period)
 		return -1;
 }
 
-static int cht_wc_leds_blink_set(struct led_classdev *cdev,
-				 unsigned long *delay_on,
-				 unsigned long *delay_off)
+static int cht_wc_leds_set_effect(struct led_classdev *cdev,
+				  unsigned long *delay_on,
+				  unsigned long *delay_off,
+				  u8 effect)
 {
 	struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
 	unsigned int ctrl;
@@ -242,7 +243,7 @@  static int cht_wc_leds_blink_set(struct led_classdev *cdev,
 	}
 
 	ret = regmap_update_bits(led->regmap, led->regs->fsm,
-				 CHT_WC_LED_EFF_MASK, CHT_WC_LED_EFF_BLINKING);
+				 CHT_WC_LED_EFF_MASK, effect);
 	if (ret < 0)
 		dev_err(cdev->dev, "Failed to update LED FSM reg: %d\n", ret);
 
@@ -261,6 +262,37 @@  static int cht_wc_leds_blink_set(struct led_classdev *cdev,
 	return ret;
 }
 
+static int cht_wc_leds_blink_set(struct led_classdev *cdev,
+				 unsigned long *delay_on,
+				 unsigned long *delay_off)
+{
+	return cht_wc_leds_set_effect(cdev, delay_on, delay_off, CHT_WC_LED_EFF_BLINKING);
+}
+
+static int cht_wc_leds_pattern_set(struct led_classdev *cdev,
+				   struct led_pattern *pattern,
+				   u32 len, int repeat)
+{
+	unsigned long delay_off, delay_on;
+
+	if (repeat > 0 || len != 2 ||
+	    pattern[0].brightness != LED_OFF || pattern[1].brightness != LED_ON ||
+	    pattern[0].delta_t != pattern[1].delta_t ||
+	    (pattern[0].delta_t != 250 && pattern[0].delta_t != 500 &&
+	     pattern[0].delta_t != 1000 && pattern[0].delta_t != 2000))
+		return -EINVAL;
+
+	delay_off = pattern[0].delta_t;
+	delay_on  = pattern[1].delta_t;
+
+	return cht_wc_leds_set_effect(cdev, &delay_on, &delay_off, CHT_WC_LED_EFF_BREATHING);
+}
+
+static int cht_wc_leds_pattern_clear(struct led_classdev *cdev)
+{
+	return cht_wc_leds_brightness_set(cdev, LED_OFF);
+}
+
 static int cht_wc_led_save_regs(struct cht_wc_led *led,
 				struct cht_wc_led_saved_regs *saved_regs)
 {
@@ -317,6 +349,8 @@  static int cht_wc_leds_probe(struct platform_device *pdev)
 		led->cdev.brightness_set_blocking = cht_wc_leds_brightness_set;
 		led->cdev.brightness_get = cht_wc_leds_brightness_get;
 		led->cdev.blink_set = cht_wc_leds_blink_set;
+		led->cdev.pattern_set = cht_wc_leds_pattern_set;
+		led->cdev.pattern_clear = cht_wc_leds_pattern_clear;
 		led->cdev.max_brightness = 255;
 
 		ret = led_classdev_register(&pdev->dev, &led->cdev);