On Tue 28 Aug 13:25 PDT 2018, Jacek Anaszewski wrote:
> 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.
>
We started this discussion with the suggestion that rather than
introducing a new Qualcomm specific sysfs interface for controlling the
pattern engine we should have a common one, but if I understand your
suggestion we should not have a common interface, just a common sysfs
file name?
Regards,
Bjorn
