Re: [PATCH v10 2/2] leds: sc27xx: Add pattern_set/clear interfaces for LED controller

[Baolin Wang]

Hi Jacek,

On 7 September 2018 at 04:16, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> wrote:
> Hi Baolin,
>
> Thank you for the patch. I really appreciate your effort.
>
> I see one more thing I forgot to mention in the previous
> review. Please take a look at my comment to pattern_set().
>
> On 09/06/2018 04:37 AM, Baolin Wang wrote:
>> This patch implements the 'pattern_set'and 'pattern_clear'
>> interfaces to support SC27XX LED breathing mode.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>> ---
>> Changes from v9:
>>  - Optimize the ABI documentation file.
>>  - Update the brightness value in hardware pattern mode.
>>
>> Changes from v8:
>>  - Optimize the ABI documentation file.
>>
>> Changes from v7:
>>  - Add its own ABI documentation file.
>>
>> Changes from v6:
>>  - None.
>>
>> Changes from v5:
>>  - None.
>>
>> Changes from v4:
>>  - None.
>>
>> Changes from v3:
>>  - None.
>>
>> Changes from v2:
>>  - None.
>>
>> Changes from v1:
>>  - Remove pattern_get interface.
>> ---
>>  .../ABI/testing/sysfs-class-led-driver-sc27xx      |   22 +++++
>>  drivers/leds/leds-sc27xx-bltc.c                    |  103 ++++++++++++++++++++
>>  2 files changed, 125 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> new file mode 100644
>> index 0000000..d8056d5
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> @@ -0,0 +1,22 @@
>> +What:                /sys/class/leds/<led>/hw_pattern
>> +Date:                September 2018
>> +KernelVersion:       4.20
>> +Description:
>> +             Specify a hardware pattern for the SC27XX LED. For the SC27XX
>> +             LED controller, it only supports 4 stages to make a single
>> +             hardware pattern, which is used to configure the rise time,
>> +             high time, fall time and low time for the breathing mode.
>> +
>> +             For the breathing mode, the SC27XX LED only expects one brightness
>> +             for the high stage. To be compatible with the hardware pattern
>> +             format, we should set brightness as 0 for rise stage, fall
>> +             stage and low stage.
>> +
>> +             Min stage duration: 125 ms
>> +             Max stage duration: 31875 ms
>> +
>> +             Since the stage duration step is 125 ms, the duration must be
>> +             a multiplier of 125, like 125ms, 250ms, 375ms, 500ms ... 31875ms.
>> +
>> +             Thus the format of the hardware pattern values should be:
>> +             "0 rise_duration brightness high_duration 0 fall_duration 0 low_duration".
>> diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
>> index 9d9b7aa..558a325 100644
>> --- a/drivers/leds/leds-sc27xx-bltc.c
>> +++ b/drivers/leds/leds-sc27xx-bltc.c
>> @@ -32,8 +32,15 @@
>>  #define SC27XX_DUTY_MASK     GENMASK(15, 0)
>>  #define SC27XX_MOD_MASK              GENMASK(7, 0)
>>
>> +#define SC27XX_CURVE_SHIFT   8
>> +#define SC27XX_CURVE_L_MASK  GENMASK(7, 0)
>> +#define SC27XX_CURVE_H_MASK  GENMASK(15, 8)
>> +
>>  #define SC27XX_LEDS_OFFSET   0x10
>>  #define SC27XX_LEDS_MAX              3
>> +#define SC27XX_LEDS_PATTERN_CNT      4
>> +/* Stage duration step, in milliseconds */
>> +#define SC27XX_LEDS_STEP     125
>>
>>  struct sc27xx_led {
>>       char name[LED_MAX_NAME_SIZE];
>> @@ -122,6 +129,98 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum led_brightness value)
>>       return err;
>>  }
>>
>> +static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
>> +{
>> +     struct sc27xx_led *leds = to_sc27xx_led(ldev);
>> +     struct regmap *regmap = leds->priv->regmap;
>> +     u32 base = sc27xx_led_get_offset(leds);
>> +     u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
>> +     u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
>> +     int err;
>> +
>> +     mutex_lock(&leds->priv->lock);
>> +
>> +     /* Reset the rise, high, fall and low time to zero. */
>> +     regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
>> +     regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
>> +
>> +     err = regmap_update_bits(regmap, ctrl_base,
>> +                     (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
>> +
>> +     ldev->brightness = LED_OFF;
>> +
>> +     mutex_unlock(&leds->priv->lock);
>> +
>> +     return err;
>> +}
>> +
>> +static int sc27xx_led_pattern_set(struct led_classdev *ldev,
>> +                               struct led_pattern *pattern,
>> +                               int len, u32 repeat)
>> +{
>> +     struct sc27xx_led *leds = to_sc27xx_led(ldev);
>> +     u32 base = sc27xx_led_get_offset(leds);
>> +     u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
>> +     u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
>> +     struct regmap *regmap = leds->priv->regmap;
>> +     int err;
>> +
>> +     /*
>> +      * Must contain 4 tuples to configure the rise time, high time, fall
>> +      * time and low time to enable the breathing mode.
>> +      */
>> +     if (len != SC27XX_LEDS_PATTERN_CNT)
>> +             return -EINVAL;
>> +
>> +     mutex_lock(&leds->priv->lock);
>
> Please add below macros at the top of the file and the function
> shown. It will allow to nicely align the value provided by
> the user to the nearest delta_t step. We have similar thing
> in led_class_flash.c (led_clamp_align()), that was adopted from
> v4l2-ctrls.c.
>
> #define SC27XX_DELTA_T_MIN SC27XX_LEDS_STEP
> #define SC27XX_DELTA_T_MAX (SC27XX_LEDS_STEP * 255)
>
> static void sc27xx_led_clamp_align_delta_t(u32 *delta_t)
> {
>         u32 v, offset, t = *delta_t;
>
>         v = t + SC27XX_LEDS_STEP / 2;
>         v = clamp(v, SC27XX_DELTA_T_MIN, SC27XX_DELTA_T_MAX);
>         offset = v - SC27XX_DELTA_T_MIN;
>         offset = SC27XX_LEDS_STEP * (offset / SC27XX_LEDS_STEP);
>         *delta_t = SC27XX_DELTA_T_MIN + offset;
> }
>
> Then call the function for each delta_t before writing it to the device:
>
> sc27xx_led_clamp_align_delta_t(&pattern[0].delta_t);
> sc27xx_led_clamp_align_delta_t(&pattern[1].delta_t);
> sc27xx_led_clamp_align_delta_t(&pattern[2].delta_t);
> sc27xx_led_clamp_align_delta_t(&pattern[3].delta_t);

That's a good point. Will add in next version.

>
> Also, regarding PATCH v8 1/2, please change the types of properties
> in struct led_pattern as follows:
>
> +struct led_pattern {
> +       u32 delta_t;
> +       enum led_brightness brightness;
> +};

Sure. Thanks for your comments.

-- 
Baolin Wang
Best Regards