linux-kernel - Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Jul 2017 11:24:48 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Pavel Machek <pavel@....cz>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Lee Jones <lee.jones@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Rob Herring <robh+dt@...nel.org>,
        Richard Purdie <rpurdie@...ys.net>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Heiko Stuebner <heiko@...ech.de>, linux-pwm@...r.kernel.org,
        linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        groeck@...omium.org, linux-rockchip@...ts.infradead.org,
        huang lin <hl@...k-chips.com>
Subject: Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us
 property

On Thu, Jul 06, 2017 at 10:17:18AM +0100, Daniel Thompson wrote:
> On 06/07/17 10:12, Pavel Machek wrote:
> > On Thu 2017-07-06 10:01:32, Thierry Reding wrote:
> > > On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
> > > > From: huang lin <hl@...k-chips.com>
> > > > 
> > > > Some panels (i.e. N116BGE-L41), in their power sequence specifications,
> > > > request a delay between set the PWM signal and enable the backlight and
> > > > between clear the PWM signal and disable the backlight. Add support for
> > > > the new pwm-delay-us property to meet the timing.
> > > > 
> > > > Note that this patch inverts current sequence. Before this patch the
> > > > enable signal was set before the PWM signal and vice-versa on power off.
> > > > 
> > > > I assumed that this sequence was wrong, at least it is on different panel
> > > > datasheets that I checked, so I inverted the sequence to follow:
> > > > 
> > > >    On power on, set the PWM signal, wait, and set the LED_EN signal.
> > > >    On power off, clear the LED_EN signal, wait, and stop the PWM signal.
> > > 
> > > I think this should be two separate patches to make it easier to revert
> > > the inverted sequence should it prove to regress on other panels.
> > 
> > Don't make this overly complex. This is trivial. No need to split it
> > into more patches.
> 
> Agree. IMHO getting the code that reads the (optional) new parameter correct
> is the best way to manage risk of regression since in most cases the delay
> will be skipped anyway.

The potential regression that I'm referring to would be caused by
inversing the sequence (GPIO enable -> PWM enable). That's completely
unrelated to the delays introduced by this patch. Many boards use this
driver and they've been running with the old sequence for many years.
Granted, it's fairly unlikely to regress, but it's still a possibility.

Given that both changes are logically separate, I think separate patches
are totally appropriate. I also don't think that this would overly
complicate things.

Thierry

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