Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751172AbdGMHWU (ORCPT ); Thu, 13 Jul 2017 03:22:20 -0400 Received: from mail-it0-f67.google.com ([209.85.214.67]:35467 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750954AbdGMHWS (ORCPT ); Thu, 13 Jul 2017 03:22:18 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170630112109.13785-1-enric.balletbo@collabora.com> From: Enric Balletbo Serra Date: Thu, 13 Jul 2017 09:22:15 +0200 Message-ID: Subject: Re: [PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property To: Rob Herring Cc: Enric Balletbo i Serra , Thierry Reding , Lee Jones , Daniel Thompson , Jingoo Han , Bartlomiej Zolnierkiewicz , Pavel Machek , Richard Purdie , Jacek Anaszewski , Heiko Stuebner , Linux PWM List , "linux-fbdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Guenter Roeck , "open list:ARM/Rockchip SoC..." , huang lin Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3926 Lines: 89 Rob, 2017-07-06 20:23 GMT+02:00 Enric Balletbo Serra : > Hi Rob, > > 2017-07-06 19:07 GMT+02:00 Rob Herring : >> On Fri, Jun 30, 2017 at 6:21 AM, Enric Balletbo i Serra >> wrote: >>> From: huang lin >>> >>> Add a pwm-delay-us property to specify the delay between setting an >>> initial (non-zero) PWM value and enabling the backlight, and also the >>> delay between disabling the backlight and setting PWM value to 0. >>> >>> Signed-off-by: huang lin >>> Signed-off-by: Enric Balletbo i Serra >>> --- >>> Changes since v1: >>> - As suggested by Daniel Thompson >>> - Do not assume power-on delay and power-off delay will be the same >>> >>> v1: https://lkml.org/lkml/2017/6/28/219 >>> >>> Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>> index 764db86..49b037e 100644 >>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>> @@ -17,6 +17,11 @@ Optional properties: >>> "pwms" property (see PWM binding[0]) >>> - enable-gpios: contains a single GPIO specifier for the GPIO which enables >>> and disables the backlight (see GPIO binding[1]) >>> + - pwm-delay-us: delay between setting an initial (non-zero) PWM value and >>> + enabling the backlight, and also the delay between disabling >>> + the backlight and setting PWM value to 0. >>> + The 1st cell is the pre-delay in micro seconds. >>> + The 2nd cell is the post-delay in micro seconds. >> >> pre and post imply a time before and after a certain event, but these >> are for 2 different events. These are more like an enable/on delay and >> disable/off delay which probably should be separate properties. What >> happens when we need the opposite sequence or a different sequence? >> Maybe some panel requires the PWM to be 0 until some time after >> enabling. >> A second proposal, what do you think? - post-pwm-on-delay-us: Delay in us after setting an initial (non-zero) PWM and enabling the backlight using GPIO. - pwm-off-delay-us: Delay in us after disabling the backlight using a GPIO and setting PWM value to 0. Thanks, Enric > > Maybe, Only I can say that the panels I checked always first enable > the PWM and then set the ENABLE signal, but of course I didn't check > all the panels. > > Would be more acceptable have enable-delay-us and disable-delay-us proprieties? > >> I don't understand why you even need a post delay. The PWM can be set >> to 0 while enabled, right? So if the PWM is set to 0 while enabled and >> then disable the backlight, then there's no delay. Plus this doesn't >> make much sense to me electrically either. The PWM duty cycle is going >> to be completely async to the enable line change. The PWM state could >> go from 1 to 0 at any point in time relative to the enable line >> change. >> > > To be honest I'm also not sure why is required the post delay, some > panels specify a 0 but others specifies a minimum value between you > off the panel and disable the PWM. The only reason I added the post > delay is because the different datasheets specifies it, I don't have a > use case that the post delay is used to fix something. > > Thanks, > Enric > >> These issues are the problem with generic bindings. Adding 1 property >> is no big deal, but then what happens with the next variation. These >> timing constraints need to be able to be implied by the panel's >> compatible. >> >> Rob