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

[Hans de Goede <hdegoede@redhat.com>]

Hi Jacek,

On 4/16/23 17:17, Jacek Anaszewski wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On 4/13/23 17:18, 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>
>> 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
>>
>> 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"
> 
> 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.

Regards,

Hans





> 
>> +
>> +    ^
>> +    |
>> +    Max-|     ---
>> +    |    /   \
>> +    |   /     \
>> +    |  /       \     /
>> +    | /         \   /
>> +    Min-|-           ---
>> +    |
>> +    0------2------4--> time (sec)
> v);
>