Message-ID: <20170330074309.GA28533@amd>
Date:   Thu, 30 Mar 2017 09:43:09 +0200
From:   Pavel Machek <pavel@....cz>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Rob Herring <robh@...nel.org>, Richard Purdie <rpurdie@...ys.net>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG

Hi!

> > > There is a binding for ti,lp55xx, but there's nothing I can reuse from
> > > that binding...because it's completely different hardware.
> > 
> > Agreed, if you drop the pattern stuff from the binding, at least for now. 
> 
> I do not have a strong preference to expose these knobs in devicetree
> and I do fear that finding some common "pattern" bindings that suits
> everyone will be very difficult.
> 
> So I'll drop them from the binding for now.

Ok.

> > If you want driver merged quickly, I believe the best way would be to
> > leave out pattern support for now. We can merge the basic driver
> > easily to 4.12.
> > 
> 
> I'm not that much in a hurry and would rather see that we resolve any
> outstanding issues with the implementation of the pattern handling.

Ok, good.

> But regardless of this we still have the problem that the typical
> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
> RGB-LED. So we would have to create some sort of in-driver-wrapper
> around any three instances exposing them as a single LED to the user.

Yes, I believe we should do the wrapping. In N900 case, 

> I rather expose the individual channels and make sure that when we
> trigger a blink operation or enable a pattern (i.e. the two operations
> that do require synchronization) we will perform that synchronization
> under the hood.

First, we need a way to tell userspace which LEDs are synchronized,
because otherwise it will be confusing.

Second, there are more issues than just patterns with the RGB
LED. Most important is ability to set particular colors. You want to
set the RGB LED to "white", but that does not mean you can set
red=green=blue=1.0. You want color to look the same on LCD and on the
LED, which means coefficients for white and some kind of function for
brightness-to-PWM conversion.

> > Incremental patches sound like a good idea, yes.
> > 
> > I'd say that testing with actual RGB LED is not a requirement... as
> > long as we design reasonable interface where the synchronizaction will
> > be easy.
> > 
> 
> As this relates to the board layout (which LPG-channels are hooked to a
> RGB) I think it makes sense to expose a mechanism in devicetree to
> indicate which channels should have their pattern/blink synchronized.
> 
> We should be able to extend the LUT (the hardware that actually
> implements the pattern-walker logic) with a DT-property like:
> 
>   qcom,synchronize-group-0 = <1, 2, 3>;
>   qcom,synchronize-group-1 = <5, 6, 7>;
> 
> And whenever we configure a pattern involving one of the affected LEDs
> from a group we start all of them.

Yes we need some kind of grouping.

Additional complexity in the N900 case... groups can actually be
configured at run time. Original Maemo used that ability to group 6
keyboard backlight leds, and then run pattern on them. OTOH... I don't
think we _need_ to support that functionality.

Best regards,
									Pavel

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

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)

Powered by blists - more mailing lists