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

[Thierry Reding]

On Tue, Jan 22, 2013 at 12:24:34PM +0900, Alex Courbot wrote:
> 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?

I'm confused. Why would you want to call into pwm_bl directly? If we're
going to split this up into separate platform devices, why not look up a
given backlight device and use the backlight API on that? The pieces of
the puzzle are all there: you can use of_find_backlight_by_node() to
obtain a backlight device from a device tree node, so I'd expect the DT
to look something like this:

	backlight: backlight {
		compatible = "pwm-backlight";
		...
	};

	panel: panel {
		compatible = "...";
		...
		backlight = <&backlight>;
		...
	};

After that you can wire it up with host1x using something like:

	host1x {
		dc@54200000 {
			rgb {
				status = "okay";

				nvidia,panel = <&panel>;
			};
		};
	};

Maybe with such a binding, we should move the various display-timings
properties to the panel node as well and have an API to retrieve them
for use by tegra-drm.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature