Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 6146 bytes --]

Hi Thierry,

On Friday 01 November 2013 11:13:47 Thierry Reding wrote:
> On Fri, Nov 01, 2013 at 08:37:23AM +0900, Jingoo Han wrote:
> > On Wednesday, October 23, 2013 5:02 AM, Thierry Reding wrote:
> > > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe PLAGNIOL-
VILLARD wrote:
> > > > On 09:23 Tue 22 Oct     , Thierry Reding wrote:
> > > > > On Tue, Oct 22, 2013 at 06:58:33AM +0200, Jean-Christophe PLAGNIOL-
VILLARD wrote:
> > > > > > On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> > > > > > > Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> > > > > > > Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > > > Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > > > > > > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > > > > > > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > > > > > > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > > > > > Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > > > > > > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > > > > > > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > > > Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > > > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > > > > Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> > > > > > > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > > > > > > Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > > > Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > > > ---
> > > > > > > ChangeLog v3->v4:
> > > > > > > - The default-brightness property is now optional, it defaults
> > > > > > > to 1 if not set.> > > > > 
> > > > > > by default we set OFF not ON
> > > > > > 
> > > > > > do not actiate driver or properti by default you can not known to
> > > > > > consequence on the hw
> > > > > 
> > > > > Turning on a backlight by default is what pretty much every
> > > > > backlight driver does. I personally think that's the wrong default,
> > > > > I even tried to get some discussion started recently about how we
> > > > > could change this. However, given that this has been the case for
> > > > > possibly as long as the subsystem has existed, suddenly changing it
> > > > > might cause quite a few of our users to boot the new kernel and not
> > > > > see their display come up. As with any other ABI, this isn't
> > > > > something we can just change without a very good migration path.
> > > > 
> > > > I'm sorry but the blacklight descibe in DT have nothing to do with the
> > > > common pratice that the current driver have today
> > > 
> > > That's not at all what I said. What I said was that the majority of
> > > backlight drivers currently default to turning the backlight on when
> > > probed. Therefore I think it would be consistent if this driver did the
> > > same.
> > > 
> > > I also said that I don't think it's a very good default, but at the same
> > > time we can't just go and change the default behaviour at will because
> > > people may rely on it.
> > 
> > I agree with your opinion.
> > But, I can't decide how to change it.
> 
> One solution that Stephen proposed was to make all DT-based setups use a
> new default (off). An advantage of that is that such setups are still
> fairly actively being worked on, so we can probably get early adopters
> to cope with the new default.
> 
> Looking through some DTS files it seems that many use the pwm-backlight
> driver. So if we can get some concensus from all the users that changing
> the default would be okay, then I think we could reasonably change it.
> 
> Another solution perhaps would be to add a property to DT which encodes
> the default state of the backlight on boot. This used to be impossible
> because the general concensus was that DT should describe hardware only
> and not software policy. During the kernel summit this requirement was
> somewhat relaxed and it should now be okay to describe system
> configuration data, and I think this would be a good match. If the
> system is designed to keep the backlight off by default because some
> other component (display panel driver) is meant to turn it on later,
> that's system configuration data, right?
> 
> Perhaps a boolean property could be used:
> 
> 	backlight {
> 		compatible = "pwm-backlight";
> 
> 		...
> 
> 		backlight-default-off;
> 	};
> 
> > > > put on by default if wrong specially without the property define. Even
> > > > put it on by default it wrong as the bootloader may have set it
> > > > already for splash screen and to avoid glitch the drivers need to
> > > > detect this.
> > > 
> > > I agree that would be preferable, but I don't know of any way to detect
> > > what value the bootloader set a GPIO to. The GPIO API requires that you
> > > call gpio_direction_output(), and that requires a value parameter which
> > > will be used as the output level of the GPIO.
> > 
> > Jean-Christophe's point is right.
> > 
> > We may need to discuss 'the way to detect what value the bootloader
> > set a GPIO to'.
> 
> Since some GPIO hardware simply can't do it, I guess we could resolve to
> passing such information via DT. That obviously won't work for non-DT
> setups, but I guess that's something we could live with.

For what it's worth, the pcf857x GPIO controllers suffer from this problem, 
and the solution was to use a DT property to specify the initial GPIOs state. 
The driver can thus implement gpio_get_value() properly and the hardware 
limitation is hidden from the clients.

https://lkml.org/lkml/2013/9/21/105

> One possibility would be to supplement the backlight-default-off property
> with another property (backlight-boot-on) that can be passed on to the
> kernel from the bootloader to signal that it has turned the backlight on.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]