Re: [PATCH v8 1/2] leds: core: Introduce LED pattern trigger

[Pavel Machek]

Hi!

> >>> No, we are not back to full circle.
> >>>
> >>> Or at least we should not be.
> >>>
> >>> Yes, hw_pattern can have some limitation pattern does not, but if you
> >>> take values from hw_pattern file and put them into pattern file, you
> >>> should get the same pattern (with more power being consumed). And that
> >>> property is kind of important for me, because it should keep the ABI
> >>> reasonable.
> >>
> >> If you looked at what we agreed on with Baolin, you'd realize
> >> that this property is in no way preserved.
> >> This is what the whole story is about - we're introducing hw_pattern
> >> because of difficulties in describing breathing pattern by a series
> >> of [brightness delta_t] tuples.
> >>
> >> And Bjorn presented another example. I'm inclined to leave the
> >> definition of hw_pattern semantics to the LED class drivers,
> >> and allow them to create related sysfs files.
> > 
> > Please lets not do that.
> > 
> > We already have drivers that do that and it is complete
> > nightmare. Some take binary code for the tiny CPU driving the LED.
> > 
> > What exactly is the problem? [brightness delta_t] can describe
> 
> You wrote:
> 
> <quote>
> Yes, hw_pattern can have some limitation pattern does not, but if you
> take values from hw_pattern file and put them into pattern file, you
> should get the same pattern (with more power being consumed).
> </quote>
> 
> The problem is that we decided to introduce hw_pattern to
> to take away from drivers a responsibility for translating
> a series of tuples, approximating the brightness curve,
> to the values that can be written to the hardware registers.
> 
> Because this is what would need to be done to check if hw can support
> given series of tuples and activate it. Actually with this approach
> we wouldn't need hw_pattern at all, since pattern alone would do.
> But implementation thereof is what we could call a nightmare.
> 
> What follows, your claim from the quotation is inaccurate:
> values from hw_pattern written to the pattern file will not
> produce the same pattern, at least in case of what was proposed
> in [0] for drivers/leds/leds-sc27xx-bltc.c.

That sounds easy, see below.

> > anything single LED can do in finite time. You are right, that
> > [brightness delta_t] sequence may get rather long, and it may be hard
> > to turn that sequence into parameters. Are there any _interesting_
> > sequences hardware can do but [brightness delta_t] can not store
> > reasonably?
> 
> Please propose the implementation of pattern_set for
> drivers/leds/leds-sc27xx-bltc.c breathing pattern, that will
> setup breathing mode basing on the values from tuples.
> 
> Use Baolin's patch [0] for a reference of what hardware expects.
> 
> [0] https://lore.kernel.org/patchwork/patch/984246/

Yep, so we change documentation to require

0 rise_duration brightness high_duration brightness fall_duration 0 low_duration"

...and we are done; at least as long as user writes expected pattern
to the file.

I'd actually like to see this at begining of function:
    if (pattern[0].brightness != 0)
        return -EINVAL;
    if (pattern[2].brightness != 0)
        return -EINVAL;
    if (pattern[3].brightness != 0)
        return -EINVAL;
    if (pattern[1].brightness != pattern[2].brightness)
        return -EINVAL;

..so if user writes something unexpected, he gets the error back.

What am I missing?

Thanks,
									Pavel    
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature