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-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
