On 08/25/2018 09:51 AM, Baolin Wang wrote: > On 25 August 2018 at 04:44, Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> wrote: >> On 08/24/2018 10:12 PM, Pavel Machek wrote: >>> On Fri 2018-08-24 21:49:50, Jacek Anaszewski wrote: >>>> Hi Pavel, >>>> >>>> On 08/24/2018 12:11 PM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>> I think that it would be more flexible if software pattern fallback >>>>>> was applied in case of pattern_set failure. Otherwise, it would >>>>>> lead to the situation where LED class devices that support hardware >>>>>> blinking couldn't be applied the same set of patterns as LED class >>>>>> devices that don't implement pattern_set. The latter will always have to >>>>>> resort to using software pattern engine which will accept far greater >>>>>> amount of pattern combinations. >>>>>> >>>>>> In this case we need to discuss on what basis the decision will be >>>>>> made on whether hardware or software engine will be used. >>>>>> >>>>>> Possible options coming to mind: >>>>>> - an interface will be provided to determine max difference between >>>>>> the settings supported by the hardware and the settings requested by >>>>>> the user, that will result in aligning user's setting to the hardware >>>>>> capabilities >>>>>> - the above alignment rate will be predefined instead >>>>>> - hardware engine will be used only if user requests supported settings >>>>>> on the whole span of the requested pattern >>>>>> - in each of the above cases it would be worth to think of the >>>>>> interface to show the scope of the settings supported by hardware >>>>> >>>>> I'd recommend keeping it simple. We use hardware engine if driver >>>>> author thinks pattern is "close enough". >>>> >>>> The thing is that in the ledtrig-pattern v5 implementation there >>>> is no option of using software fallback if pattern_set op >>>> is initialized: >>>> >>>> + if (led_cdev->pattern_set) { >>>> + return led_cdev->pattern_set(led_cdev, data->patterns, >>>> + data->npatterns, data->repeat); >>>> + } >>> >>> Yeah, that sounds wrong. (Sorry I did not pay enough attention). >>> >>> It pattern_set() returns special error code, it should just continue >>> and use software pattern fallback. >> >> And now we can get back to the issue I was concerned about in the >> email you replied to, i.e. what series of [brightness delta_t] tuples >> should be written to the pattern file to enable hardware breathing >> engine. > > OK. So now we've made a consensus to use the software pattern fallback > if failed to set hardware pattern. How about introduce one interface > to convert hardware patterns to software patterns if necessary? > > diff --git a/drivers/leds/trigger/ledtrig-pattern.c > b/drivers/leds/trigger/ledtrig-pattern.c > index 63b94a2..d46a641 100644 > --- a/drivers/leds/trigger/ledtrig-pattern.c > +++ b/drivers/leds/trigger/ledtrig-pattern.c > @@ -66,8 +66,15 @@ static int pattern_trig_start_pattern(struct > pattern_trig_data *data, > return 0; > > if (led_cdev->pattern_set) { > - return led_cdev->pattern_set(led_cdev, data->patterns, > - data->npatterns, data->repeat); > + ret = led_cdev->pattern_set(led_cdev, data->patterns, > + data->npatterns, data->repeat); > + if (!ret) > + return 0; > + > + dev_warn(led_cdev->dev, "Failed to set hardware pattern\n"); > + > + if (led_cdev->pattern_convert) > + led_cdev->pattern_convert(led_cdev, I can't see how it could help to assess if hw pattern engine can launch given pattern, and with what accuracy. Instead, I propose to add a means for defining whether the pattern to be set is intended for hardware pattern engine or for software fallback. It could be either separate sysfs file e.g. hw_pattern, or a modifier to the pattern written to the pattern file, e,g, echo "hw 100 2 200 3 100 2" > pattern hw format expected by given driver would have to be described in the per-driver ABI documentation. All patterns without "hw" prefix would enable software pattern engine. > data->patterns, &data->npatterns); > } > > data->curr = data->patterns; > @@ -106,8 +113,7 @@ static ssize_t repeat_store(struct device *dev, > struct device_attribute *attr, > if (err) > return err; > > - if (!led_cdev->pattern_set) > - del_timer_sync(&data->timer); > + del_timer_sync(&data->timer); > > mutex_lock(&data->lock); > > @@ -161,8 +167,7 @@ static ssize_t pattern_store(struct device *dev, > struct device_attribute *attr, > struct pattern_trig_data *data = led_cdev->trigger_data; > int ccount, cr, offset = 0, err = 0; > > - if (!led_cdev->pattern_set) > - del_timer_sync(&data->timer); > + del_timer_sync(&data->timer); > > mutex_lock(&data->lock); > > @@ -232,9 +237,8 @@ static void pattern_trig_deactivate(struct > led_classdev *led_cdev) > > if (led_cdev->pattern_clear) > led_cdev->pattern_clear(led_cdev); > - else > - del_timer_sync(&data->timer); > > + del_timer_sync(&data->timer); > led_set_brightness(led_cdev, LED_OFF); > kfree(data); > led_cdev->activated = false; > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 74fc2c6..04f3eaf 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -93,6 +93,8 @@ struct led_classdev { > struct led_pattern *pattern, int len, > unsigned int repeat); > int (*pattern_clear)(struct led_classdev *led_cdev); > + int (*pattern_convert)(struct led_classdev *led_cdev, > + struct led_pattern *pattern, int *len); > > struct device *dev; > const struct attribute_group **groups; > -- Best regards, Jacek Anaszewski