linux-kernel - Re: [PATCH 05/10] ARM: SAMSUNG: Initialize PWM backlight enable

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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 1 Oct 2013 23:23:06 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Rob Herring <rob.herring@...xeda.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Tony Lindgren <tony@...mide.com>,
	Eric Miao <eric.y.miao@...il.com>,
	Haojian Zhuang <haojian.zhuang@...il.com>,
	Ben Dooks <ben-linux@...ff.org>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Simon Horman <horms@...ge.net.au>,
	Magnus Damm <magnus.damm@...il.com>,
	Guan Xuetao <gxt@...c.pku.edu.cn>, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-omap@...r.kernel.org,
	openezx-devel@...ts.openezx.org, linux-samsung-soc@...r.kernel.org,
	linux-sh@...r.kernel.org, linux-pwm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/10] ARM: SAMSUNG: Initialize PWM backlight enable_gpio
 field

On Tue, Oct 01, 2013 at 02:58:22PM -0600, Stephen Warren wrote:
> On 10/01/2013 02:43 PM, Thierry Reding wrote:
> > On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote:
> >> On 09/23/2013 03:41 PM, Thierry Reding wrote:
> >>> The GPIO API defines 0 as being a valid GPIO number, so this
> >>> field needs to be initialized explicitly.
> >> 
> >>> static void __init smdkv210_map_io(void)
> >> 
> >>> @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata
> >>> samsung_dfl_bl_data __initdata = { .max_brightness = 255, 
> >>> .dft_brightness = 255, .pwm_period_ns  = 78770, +		.enable_gpio
> >>> = -1, .init           = samsung_bl_init, .exit           =
> >>> samsung_bl_exit, }, @@ -121,6 +122,10 @@ void __init
> >>> samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, 
> >>> samsung_bl_data->lth_brightness = bl_data->lth_brightness; if
> >>> (bl_data->pwm_period_ns) samsung_bl_data->pwm_period_ns =
> >>> bl_data->pwm_period_ns; +	if (bl_data->enable_gpio) +
> >>> samsung_bl_data->enable_gpio = bl_data->enable_gpio; +	if
> >>> (bl_data->enable_gpio_flags) +
> >>> samsung_bl_data->enable_gpio_flags =
> >>> bl_data->enable_gpio_flags;
> >> 
> >> Won't this cause the core pwm_bl driver to request/manipulate the
> >> GPIO, whereas this driver already does that inside the
> >> samsung_bl_init/exit callbacks? I think you either need to adjust
> >> those callbacks, or not set the new standard GPIO property in
> >> samsung_bl_data.
> > 
> > I don't think so. The samsung_bl_data is a copy of
> > samsung_dfl_bl_data augmented by board-specific settings. So in
> > fact copying these values here is essential to allow boards to
> > override the enable_gpio and flags fields. Currently no board sets
> > the enable_gpio to a valid GPIO so it's all still handled by the
> > callbacks only.
> 
> Oh yes, you're right. I was confusing the new enable_gpio field in
> pwm_bl's platform data with some other field in a custom data structure.
> 
> One minor point though:
> 
> >>> +	if (bl_data->enable_gpio) +		samsung_bl_data->enable_gpio =
> >>> bl_data->enable_gpio;
> 
> That assumes that enable_gpio==0 means "none", whereas you've gone to
> great pains in the rest of the series to allow 0 to be a valid GPIO
> ID. right now, the default value of samsung_bl_data->enable_gpio is
> -1, and if !bl_data->enable_gpio, that value won't be propagated across.

Right, that check should now be:

	if (bl_data->enable_gpio >= 0)

Well, it depends. It would be possible for the default to specify a
valid GPIO and for a board to override it with -1 (and provide a set of
corresponding callbacks). In that case the right thing to do here would
be not to check at all.

Then again, I don't think that will ever happen, because no fixed GPIO
will ever be a good default. So changing to >= 0 instead of != 0 should
work fine.

Again, starting with 3.13 this should become a lot easier to handle
since the GPIO subsystem will gain functionality to use a per-board
lookup table, similarly to what the regulator and PWM subsystems do.
Once that's in place I plan to make another pass over all users of the
pwm-backlight driver and replace the enable_gpio field with a GPIO
lookup table, so that the driver can uniformly request them using a
simple gpiod_get().

Thierry

Content of type "application/pgp-signature" skipped