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

[Lee Jones <lee@kernel.org>]

Jacek,

On Thu, 20 Apr 2023, Hans de Goede wrote:

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

Is this what you were after?

> Link: https://lore.kernel.org/all/6beed61c-1fc6-6525-e873-a8978f5fbffb@gmail.com/
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2
> - Improve/extend Documentation/leds/leds-cht-wcove.rst a bit
> ---
>  Documentation/leds/index.rst          |  1 +
>  Documentation/leds/leds-cht-wcove.rst | 38 ++++++++++++++++++++++++
>  drivers/leds/leds-cht-wcove.c         | 42 ++++++++++++++++++++++++---
>  3 files changed, 77 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/leds/leds-cht-wcove.rst
> 
> 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..5ec7cb60c4aa
> --- /dev/null
> +++ b/Documentation/leds/leds-cht-wcove.rst
> @@ -0,0 +1,38 @@
> +.. 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.
> +
> +The set 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.
> diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
> index 166c6f010492..23e97b08f6ea 100644
> --- a/drivers/leds/leds-cht-wcove.c
> +++ b/drivers/leds/leds-cht-wcove.c
> @@ -218,9 +218,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;
> @@ -247,7 +248,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);
>  
> @@ -266,6 +267,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)
>  {
> @@ -322,6 +354,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);
> -- 
> 2.39.2
> 

-- 
Lee Jones [李琼斯]