linux-kernel - Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

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:	Tue, 22 Jan 2013 12:24:34 +0900
From:	Alex Courbot <acourbot@...dia.com>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	Thierry Reding <thierry.reding@...onic-design.de>,
	"linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Mark Zhang <markz@...dia.com>,
	"gnurou@...il.com" <gnurou@...il.com>
Subject: Re: [PATCH 2/3] tegra: pwm-backlight: add tegra pwm-bl driver

On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:
> >  arch/arm/boot/dts/tegra20-ventana.dts  |  18 +++-
> >  arch/arm/configs/tegra_defconfig       |   1 +
> >  drivers/video/backlight/Kconfig        |   7 ++
> >  drivers/video/backlight/pwm_bl.c       |   3 +
> >  drivers/video/backlight/pwm_bl_tegra.c | 159
> >  +++++++++++++++++++++++++++++++++
> This should be at least 3 separate patches: (1) Driver code (2) Ventana
> .dts file (3) Tegra defconfig.

Will do that.

> If this is Ventana-specific, this should have a vendor prefix; "nvidia,"
> would be appropriate.
> 
> But, why is this Ventana-specific; surely it's at most panel-specific,
> or perhaps even generic across any/most LCD panels?

Yes, we could use the panel model here instead. Not sure how many other panels 
follow the same powering sequence though.

Making it Ventana-specific would have allowed to group all Tegra board support 
into the same driver, and considering that probably not many devices use the 
same panels as we do this seemed to make sense at first.

> > +		power-supply = <&vdd_bl_reg>;
> 
> "power" doesn't seem like a good regulator name; power to what? Is this
> for the backlight, since I see there's a panel-supply below?
> 
> > +		panel-supply = <&vdd_pnl_reg>;
> > 
> > +		bl-gpio = <&gpio 28 0>;
> > +		bl-panel = <&gpio 10 0>;
> 
> GPIO names usually have "gpios" in their name, so I assume those should
> be bl-enable-gpios, panel-enable-gpios?

Indeed, even though there is only one gpio here. Maybe we could group them 
into a single property and retrieve them by index - that's what the DT GPIO 
APIs seem to be designed for initially.

> > +static struct pwm_backlight_subdriver pwm_backlight_ventana_subdriver = {
> > +	.name = "pwm-backlight-ventana",
> > +	.init = init_ventana,
> > +	.exit = exit_ventana,
> > +	.notify = notify_ventana,
> > +	.notify_after = notify_after_ventana,
> > +};
> 
> It seems like all of that code should be completely generic.

Sorry, I don't get your point here - could you elaborate?

> Rather than invent some new registration mechanism, if we need
> board-/panel-/...-specific drivers, it'd be better to make each of those
> specific drivers a full platform device in an of itself (i.e. regular
> Linux platform device/driver, have its own probe(), etc.), and have
> those specific drivers call into the base PWM backlight code, treating
> it like a utility API.

That's what would make the most sense indeed, but would require some extra 
changes in pwm-backlight and might go against Thierry's wish to keep it 
simple. On the other hand I totally agree this would be more elegant. Every 
pwm-backlight based driver would just need to invoke pwm_bl's probe/remove 
function from its own. Thierry, would that be an acceptable alternative to the 
sub-driver thing despite the slightly deeper changes this involves?

Thanks,
Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/