linux-kernel - Re: [PATCH v11 2/2] leds: Add driver for Qualcomm LPG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220203232000.btb2qt7t6rmmnayw@SoMainline.org>
Date:   Fri, 4 Feb 2022 00:20:00 +0100
From:   Marijn Suijten <marijn.suijten@...ainline.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Pavel Machek <pavel@....cz>,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-K?nig <u.kleine-koenig@...gutronix.de>,
        Lee Jones <lee.jones@...aro.org>,
        Satya Priya Kakitapalli <c_skakit@....qualcomm.com>,
        Luca Weiss <luca@...tu.xyz>, Rob Herring <robh+dt@...nel.org>,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v11 2/2] leds: Add driver for Qualcomm LPG

On 2022-02-02 16:08:29, Bjorn Andersson wrote:
> On Wed, Feb 2, 2022 at 2:04 PM Bjorn Andersson
> <bjorn.andersson@...aro.org> wrote:
> >
> > On Wed 02 Feb 03:18 PST 2022, Marijn Suijten wrote:
> >
> > > On 2022-01-28 16:54:29, Bjorn Andersson wrote:
> > > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > > > PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> > > > with their output being routed to various other components, such as
> > > > current sinks or GPIOs.
> > > >
> > > > Each LPG instance can operate on fixed parameters or based on a shared
> > > > lookup-table, altering the duty cycle over time. This provides the means
> > > > for hardware assisted transitions of LED brightness.
> > > >
> > > > A typical use case for the fixed parameter mode is to drive a PWM
> > > > backlight control signal, the driver therefor allows each LPG instance
> > > > to be exposed to the kernel either through the LED framework or the PWM
> > > > framework.
> > > >
> > > > A typical use case for the LED configuration is to drive RGB LEDs in
> > > > smartphones etc, for which the driver support multiple channels to be
> > > > ganged up to a MULTICOLOR LED. In this configuration the pattern
> > > > generators will be synchronized, to allow for multi-color patterns.
> > > >
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> > >
> > > Reviewed-by: Marijn Suijten <marijn.suijten@...ainline.org>
> > >
> > > There may still be some things to improve based on whether lo_pause
> > > works in non-ping-pong mode - to alleviate the requirement for the first
> > > delta_t to be at most 512ms - but this patch should not be delayed much
> > > longer and that's perhaps for a followup patch.  Same for my request for
> > > documentation and examples which at the same time serve as some form of
> > > tests to see if everything works as desired.
> > >
> >
> > I've been considering lopause to be the value before we start the
> > pattern, but I think you're right in that it denotes how long we should
> > hold the first value.
> >
> > So I think it might make sense in the predefined "<value> <delay> <value>
> > <delay>" scheme to use first <delay> as to calculate lo-pause. I think
> > it has to be calculated, because the first value will iiuc be held
> > for (lopause + 1) * delay ms.

As mentioned in v10 that seems like a great idea, as long as we can
carefully validate and communicate these numbers to the user; both
through documentation and kernel error messages when values are
ultimately rejected.

Again, perhaps it might be better to postpone this to a separate
patchset as to not block the use of LPG for backlights which is arguably
more important than some fancy phone notification led patterns.

> > > I also vaguely remember other (downstream) drivers to support more than
> > > 512ms per step by (drastically?) changing PWM period, but not sure how
> > > that worked again nor if it was reliable.
> > >
> >
> > Thinking about it again, while 512 is the 9th bit, we should be able to
> > represent [0..1023] with 9 bits...
> >
> 
> Sorry, my mind was elsewhere as I wrote that. [0..511] is what we got.

Yes, 9 bits in total and BIT(8) being the highest settable ^^

- Marijn