Hi Jacek,
On 7 September 2018 at 04:16, Jacek Anaszewski
<jacek.anaszew...@gmail.com> 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.w...@linaro.org>
>> ---
>> 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
