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:
pgpA6s0CVFZEY.pgp
Description: PGP signature
