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

On Tue, Jan 22, 2013 at 12:24:34PM +0900, Alex Courbot wrote:

On Tuesday 22 January 2013 01:46:33 Stephen Warren wrote:

quoted

quoted

 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.

quoted

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.

quoted

quoted

+		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?

quoted

+		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.

quoted

quoted

+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?

quoted

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