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

login
register
mail settings
Submitter Alexandre Courbot
Date Jan. 19, 2013, 10:30 a.m.
Message ID <1358591420-7790-3-git-send-email-acourbot@nvidia.com>
Download mbox | patch
Permalink /patch/213818/
State Changes Requested, archived
Headers show

Comments

Alexandre Courbot - Jan. 19, 2013, 10:30 a.m.
Add a PWM-backlight subdriver for Tegra boards, with support for
Ventana.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 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 +++++++++++++++++++++++++++++++++
 5 files changed, 186 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
Mark Zhang - Jan. 21, 2013, 7:35 a.m.
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
[...]
>  
> +	backlight {
> +		compatible = "pwm-backlight-ventana";
> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> +		default-brightness-level = <12>;
> +
> +		pwms = <&pwm 2 5000000>;

After read the codes of tegra pwm driver & pwm framework, I got to know
the meaning of this property. So I think we need to add a doc(e.g:
Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
explain this, because this may be different between different pwm drivers.

> +		pwm-names = "backlight";
> +
> +		power-supply = <&vdd_bl_reg>;
> +		panel-supply = <&vdd_pnl_reg>;
> +		bl-gpio = <&gpio 28 0>;
> +		bl-panel = <&gpio 10 0>;
> +	};
> +
[...]
> diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c
> new file mode 100644
> index 0000000..8f2195b
> --- /dev/null
> +++ b/drivers/video/backlight/pwm_bl_tegra.c

So according to the filename, I think we can put all tegra boards codes
here, right? Just like what you do for Ventana, if I wanna add support
for cardhu, I can define similar functions -- let's say "init_cardhu",
"exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?

But I think if we do in this way, the file will become very long soon.
And there are a lot of redundant codes in it. So do you have any
suggestions?

Mark
> @@ -0,0 +1,159 @@
> +/*
> + * pwm-backlight subdriver for Tegra.
> + *
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
[...]
> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot - Jan. 21, 2013, 8:24 a.m.
On Monday 21 January 2013 15:35:58 Mark Zhang wrote:
> > +	backlight {
> > +		compatible = "pwm-backlight-ventana";
> > +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 
208
> > 224 240 255>; +		default-brightness-level = <12>;
> > +
> > +		pwms = <&pwm 2 5000000>;
> 
> After read the codes of tegra pwm driver & pwm framework, I got to know
> the meaning of this property. So I think we need to add a doc(e.g:
> Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
> to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
> explain this, because this may be different between different pwm drivers.

The bindings are in Documentation/devicetree/bindings/video/backlight/pwm-
backlight.txt . But you are right that the power supplies and GPIO will 
require a description of their own - I omitted it for this version because I 
am not sure what the driver should be called.

The panel used on Ventana is a Chunghwa CLAA101WA01A, maybe that's the name we 
should use for the compatible string instead (and rename the driver 
accordingly).

> So according to the filename, I think we can put all tegra boards codes
> here, right? Just like what you do for Ventana, if I wanna add support
> for cardhu, I can define similar functions -- let's say "init_cardhu",
> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?

That was my initial intention, yes.

> But I think if we do in this way, the file will become very long soon.
> And there are a lot of redundant codes in it. So do you have any
> suggestions?

If we decide to make a "Tegra" driver, then I don't think the size of the file 
is a big issues, as long as one can easily navigate into it. It will make 
sense to do this since Tegra kernels should include support for all the 
boards.

If we go and name the drivers after their actual panel names, we should 
definitely put them into separate files. The Tegra configuration could then 
include them all by default to make sure all boards are supported.

Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Zhang - Jan. 21, 2013, 8:35 a.m.
On 01/21/2013 04:24 PM, Alex Courbot wrote:
> On Monday 21 January 2013 15:35:58 Mark Zhang wrote:
>>> +	backlight {
>>> +		compatible = "pwm-backlight-ventana";
>>> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 
> 208
>>> 224 240 255>; +		default-brightness-level = <12>;
>>> +
>>> +		pwms = <&pwm 2 5000000>;
>>
>> After read the codes of tegra pwm driver & pwm framework, I got to know
>> the meaning of this property. So I think we need to add a doc(e.g:
>> Documentation/devicetree/bindings/video/backlight/nvidia,tegra20-bl.txt)
>> to explain this, "Documentation/devicetree/bindings/pwm/pwm.txt" doesn't
>> explain this, because this may be different between different pwm drivers.
> 
> The bindings are in Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt . But you are right that the power supplies and GPIO will 
> require a description of their own - I omitted it for this version because I 
> am not sure what the driver should be called.
> 

The description of this property in pwm-backlight.txt is:

"pwms: OF device-tree PWM specification (see PWM binding[0])
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt"

So you can't get any useful infos from that. That's why I propose to add
a tegra specific doc in
"Documentation/devicetree/bindings/video/backlight" directory.

> The panel used on Ventana is a Chunghwa CLAA101WA01A, maybe that's the name we 
> should use for the compatible string instead (and rename the driver 
> accordingly).
> 
>> So according to the filename, I think we can put all tegra boards codes
>> here, right? Just like what you do for Ventana, if I wanna add support
>> for cardhu, I can define similar functions -- let's say "init_cardhu",
>> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
> 
> That was my initial intention, yes.
> 
>> But I think if we do in this way, the file will become very long soon.
>> And there are a lot of redundant codes in it. So do you have any
>> suggestions?
> 
> If we decide to make a "Tegra" driver, then I don't think the size of the file 
> is a big issues, as long as one can easily navigate into it. It will make 
> sense to do this since Tegra kernels should include support for all the 
> boards.
> 
> If we go and name the drivers after their actual panel names, we should 
> definitely put them into separate files. The Tegra configuration could then 
> include them all by default to make sure all boards are supported.

I don't think use panel name instead of board name is a good idea.
Developers may not be familiar with panel names. So if we use panel
name, we have to search and read a lot of manual to find out what the
panel is.

I'd rather putting all stuffs in pwm_bl_tegra.c than separating them.

Mark
> 
> Alex.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Dietrich - Jan. 21, 2013, 8:52 a.m.
Hi,

> > diff --git a/drivers/video/backlight/pwm_bl_tegra.c
> > b/drivers/video/backlight/pwm_bl_tegra.c new file mode 100644
> > index 0000000..8f2195b
> > --- /dev/null
> > +++ b/drivers/video/backlight/pwm_bl_tegra.c
> 
> So according to the filename, I think we can put all tegra boards codes
> here, right? Just like what you do for Ventana, if I wanna add support
> for cardhu, I can define similar functions -- let's say "init_cardhu",
> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
> 
> But I think if we do in this way, the file will become very long soon.
> And there are a lot of redundant codes in it. So do you have any
> suggestions?

I think we (for PAZ00) will just reuse the ventana code which is sufficient 
for us. But adding "pwm-backlight-ventana" to our DTS may look a bit strange. 
On the other hand, I guess that's why the property is called "compatible".

Marc

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Zhang - Jan. 21, 2013, 8:55 a.m.
On 01/21/2013 04:52 PM, Marc Dietrich wrote:
> Hi,
> 
>>> diff --git a/drivers/video/backlight/pwm_bl_tegra.c
>>> b/drivers/video/backlight/pwm_bl_tegra.c new file mode 100644
>>> index 0000000..8f2195b
>>> --- /dev/null
>>> +++ b/drivers/video/backlight/pwm_bl_tegra.c
>>
>> So according to the filename, I think we can put all tegra boards codes
>> here, right? Just like what you do for Ventana, if I wanna add support
>> for cardhu, I can define similar functions -- let's say "init_cardhu",
>> "exit_cardhu", "notify_cardhu" and "notify_after_cardhu", right?
>>
>> But I think if we do in this way, the file will become very long soon.
>> And there are a lot of redundant codes in it. So do you have any
>> suggestions?
> 
> I think we (for PAZ00) will just reuse the ventana code which is sufficient 
> for us. But adding "pwm-backlight-ventana" to our DTS may look a bit strange. 
> On the other hand, I guess that's why the property is called "compatible".
> 

Ah, yeah, that looks strange. :)
Okay, so I know why Alex wants to use panel name while not board name...

> Marc
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - Jan. 21, 2013, 5:46 p.m.
On 01/19/2013 03:30 AM, Alexandre Courbot wrote:
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  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.

> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts

> +	backlight {
> +		compatible = "pwm-backlight-ventana";

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?

There needs to be binding documentation.

> +		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> +		default-brightness-level = <12>;
> +
> +		pwms = <&pwm 2 5000000>;
> +		pwm-names = "backlight";
> +
> +		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?

> diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c

> +static void exit_ventana(struct device *dev)
> +{
> +	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
> +
> +	devm_gpio_free(dev, data->panel_gpio);
> +	devm_gpio_free(dev, data->bl_gpio);
> +	devm_regulator_put(data->vdd_panel);
> +	devm_regulator_put(data->vdd_power);
> +	devm_kfree(dev, data);
> +}

There shouldn't be a need to explicitly free devm-allocated objects in
almost all cases; that's the whole point of the devm APIs.

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

> +static int __init pwm_backlight_tegra_init(void)
> +{
> +	pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
> +	return 0;
> +}
> +
> +static void __exit pwm_backlight_tegra_exit(void)
> +{
> +	pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
> +}
> +
> +module_init(pwm_backlight_tegra_init);
> +module_exit(pwm_backlight_tegra_exit);

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.

> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +

Some extra blank lines there.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot - Jan. 22, 2013, 3:24 a.m.
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding - Jan. 22, 2013, 7:06 a.m.
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
Stephen Warren - Jan. 22, 2013, 4:30 p.m.
On 01/21/2013 08:24 PM, 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

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

Nothing there (i.e. in the body of any of those functions) seems
remotely specific to Ventana or even Ventana's panel; presumably it
would work for any built-in LCD panel?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot - Jan. 23, 2013, 9:45 a.m.
> 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";
> 		...
> 	};

This would still prevent any power control from the backlight driver. I.e. if 
someone sets the brightness to 0 through sysfs, we cannot power the backlight 
off as pwm-backlight cannot control more than the PWM without platform 
callbacks. Backlight could only be powered off as a result of a fb blank event.

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

So all the power control of both the panel and backlight would be performed 
from this device's driver. How would it plug into tegra-drm? I would see 
tegra_panel as a new member of the tegra_output structure, with one callback 
invoked from tegra_encoder_dpms(). Does that look sane?

> After that you can wire it up with host1x using something like:
> 
> 	host1x {
> 		dc@54200000 {
> 			rgb {
> 				status = "okay";
> 
> 				nvidia,panel = <&panel>;
> 			};
> 		};
> 	};

Indeed. So if we do that, the DRM DPMS functions would take care of the 
panel/backlight powering and the backlight driver will control the PWM after 
this, through the FB notifier. This is a little bit different from the "official" 
power sequence, but I just tested controlling the PWM at the very end of the 
sequence and it works just as well. If you think this looks better I don't 
mind doing it that way, it is actually a good excuse for me to dive into the 
DRM code.

Anyway, this will only be a temporary solution, CDF is the only way to do this 
right.

Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Leela Krishna Amudala - Jan. 23, 2013, 10:15 a.m.
Hello Alex,

On Sat, Jan 19, 2013 at 4:00 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> Add a PWM-backlight subdriver for Tegra boards, with support for
> Ventana.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  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
> +++++++++++++++++++++++++++++++++
>  5 files changed, 186 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
>
> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts
> b/arch/arm/boot/dts/tegra20-ventana.dts
> index adc4754..a77b529 100644
> --- a/arch/arm/boot/dts/tegra20-ventana.dts
> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> @@ -516,6 +516,20 @@
>                 bus-width = <8>;
>         };
>
> +       backlight {
> +               compatible = "pwm-backlight-ventana";
> +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160
> 176 192 208 224 240 255>;
> +               default-brightness-level = <12>;
> +
> +               pwms = <&pwm 2 5000000>;
> +               pwm-names = "backlight";
> +
> +               power-supply = <&vdd_bl_reg>;
> +               panel-supply = <&vdd_pnl_reg>;
> +               bl-gpio = <&gpio 28 0>;
> +               bl-panel = <&gpio 10 0>;
> +       };
> +
>         regulators {
>                 compatible = "simple-bus";
>                 #address-cells = <1>;
> @@ -549,7 +563,7 @@
>                         enable-active-high;
>                 };
>
> -               regulator@3 {
> +               vdd_pnl_reg: regulator@3 {
>                         compatible = "regulator-fixed";
>                         reg = <3>;
>                         regulator-name = "vdd_pnl";
> @@ -559,7 +573,7 @@
>                         enable-active-high;
>                 };
>
> -               regulator@4 {
> +               vdd_bl_reg: regulator@4 {
>                         compatible = "regulator-fixed";
>                         reg = <4>;
>                         regulator-name = "vdd_bl";
> diff --git a/arch/arm/configs/tegra_defconfig
> b/arch/arm/configs/tegra_defconfig
> index a7827fd..1c46602 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -150,6 +150,7 @@ CONFIG_BACKLIGHT_LCD_SUPPORT=y
>  CONFIG_BACKLIGHT_CLASS_DEVICE=y
>  # CONFIG_BACKLIGHT_GENERIC is not set
>  CONFIG_BACKLIGHT_PWM=y
> +CONFIG_BACKLIGHT_PWM_TEGRA=y
>  CONFIG_FRAMEBUFFER_CONSOLE=y
>  CONFIG_LOGO=y
>  CONFIG_SOUND=y
> diff --git a/drivers/video/backlight/Kconfig
> b/drivers/video/backlight/Kconfig
> index 765a945..377a409 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -244,6 +244,13 @@ config BACKLIGHT_PWM
>           If you have a LCD backlight adjustable by PWM, say Y to enable
>           this driver.
>
> +config BACKLIGHT_PWM_TEGRA
> +       bool "PWM Backlight Driver for Tegra boards"
> +       depends on BACKLIGHT_PWM && ARCH_TEGRA
> +       help
> +         Support backlight power sequencing for Tegra boards.
> +         Supported boards: Ventana.
> +
>  config BACKLIGHT_DA903X
>         tristate "Backlight Driver for DA9030/DA9034 using WLED"
>         depends on PMIC_DA903X
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index b65a797..1a4a9a3 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -217,6 +217,9 @@ static int pwm_backlight_parse_dt(struct device *dev,
>
>  static struct of_device_id pwm_backlight_of_match[] = {
>         { .compatible = "pwm-backlight" },
> +#ifdef CONFIG_BACKLIGHT_PWM_TEGRA
> +       { .compatible = "pwm-backlight-ventana" },
> +#endif
>         { }
>  };
>
> diff --git a/drivers/video/backlight/pwm_bl_tegra.c
> b/drivers/video/backlight/pwm_bl_tegra.c
> new file mode 100644
> index 0000000..8f2195b
> --- /dev/null
> +++ b/drivers/video/backlight/pwm_bl_tegra.c
> @@ -0,0 +1,159 @@
> +/*
> + * pwm-backlight subdriver for Tegra.
> + *
> + * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/pwm_backlight.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/list.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +
> +struct ventana_bl_data {
> +       struct regulator *vdd_power;
> +       struct regulator *vdd_panel;
> +       int bl_gpio;
> +       int panel_gpio;
> +       bool is_on;
> +};
> +
> +static int init_ventana(struct device *dev)
> +{
> +       struct ventana_bl_data *data;
> +       int ret;
> +
> +       data = devm_kzalloc(dev, sizeof(struct ventana_bl_data),
> GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->vdd_power = devm_regulator_get(dev, "power");
> +       if (IS_ERR(data->vdd_power)) {
> +               dev_err(dev, "cannot get power regulator!\n");
> +               return PTR_ERR(data->vdd_power);
> +       }
> +
> +       data->vdd_panel = devm_regulator_get(dev, "panel");
> +       if (IS_ERR(data->vdd_panel)) {
> +               dev_err(dev, "cannot get panel regulator!\n");
> +               return PTR_ERR(data->vdd_panel);
> +       }
> +
> +       ret = of_get_named_gpio(dev->of_node, "bl-gpio", 0);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot find backlight GPIO!\n");
> +               return ret;
> +       }
> +       data->bl_gpio = ret;
> +
> +       ret = of_get_named_gpio(dev->of_node, "bl-panel", 0);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot find panel GPIO!\n");
> +               return ret;
> +       }
> +       data->panel_gpio = ret;
> +
> +       ret = devm_gpio_request_one(dev, data->bl_gpio,
> +                                   GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
> +                                   "backlight");
> +       if (ret < 0) {
> +               dev_err(dev, "cannot request backlight GPIO!\n");
> +               return ret;
> +       }
> +
> +       ret = devm_gpio_request_one(dev, data->panel_gpio,
> +                                   GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
> +                                   "panel");
> +       if (ret < 0) {
> +               dev_err(dev, "cannot request panel GPIO!\n");
> +               return ret;
> +       }
> +
> +       pwm_backlight_set_subdriver_data(dev, data);

Here you are passing ventana_bl_data pointer as input and in the
pwm_backlight_get_subdriver_data() function you are assigning the
received driver data to backlight_device pointer. As both are two
different structures with different structure fields in it. There can
be a chance for a crash.

Please correct me if I'm wrong.

Best Wishes,
Leela Krishna Amudala.

> +
> +       return 0;
> +}
> +
> +static void exit_ventana(struct device *dev)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +
> +       devm_gpio_free(dev, data->panel_gpio);
> +       devm_gpio_free(dev, data->bl_gpio);
> +       devm_regulator_put(data->vdd_panel);
> +       devm_regulator_put(data->vdd_power);
> +       devm_kfree(dev, data);
> +}
> +
> +static int notify_ventana(struct device *dev, int brightness)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +       if (brightness && !data->is_on) {
> +               regulator_enable(data->vdd_panel);
> +               gpio_set_value(data->panel_gpio, 1);
> +               usleep_range(200000, 200000);
> +               regulator_enable(data->vdd_power);
> +               usleep_range(10000, 10000);
> +       } else if (!brightness && data->is_on) {
> +               gpio_set_value(data->bl_gpio, 0);
> +       }
> +
> +       return brightness;
> +}
> +
> +static void notify_after_ventana(struct device *dev, int brightness)
> +{
> +       struct ventana_bl_data *data =
> pwm_backlight_get_subdriver_data(dev);
> +       if (brightness && !data->is_on) {
> +               gpio_set_value(data->bl_gpio, 1);
> +               data->is_on = true;
> +       } else if (!brightness && data->is_on) {
> +               usleep_range(10000, 10000);
> +               regulator_disable(data->vdd_power);
> +               usleep_range(200000, 200000);
> +               gpio_set_value(data->panel_gpio, 0);
> +               regulator_disable(data->vdd_panel);
> +               data->is_on = false;
> +       }
> +}
> +
> +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,
> +};
> +
> +static int __init pwm_backlight_tegra_init(void)
> +{
> +       pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
> +       return 0;
> +}
> +
> +static void __exit pwm_backlight_tegra_exit(void)
> +{
> +       pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
> +}
> +
> +module_init(pwm_backlight_tegra_init);
> +module_exit(pwm_backlight_tegra_exit);
> +
> +MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-tegra-backlight");
> +
> +
> --
> 1.8.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot - Jan. 23, 2013, 10:29 a.m.
On Wednesday 23 January 2013 18:15:30 Leela Krishna Amudala wrote:
> > +       pwm_backlight_set_subdriver_data(dev, data);
> 
> Here you are passing ventana_bl_data pointer as input and in the
> pwm_backlight_get_subdriver_data() function you are assigning the
> received driver data to backlight_device pointer. As both are two
> different structures with different structure fields in it. There can
> be a chance for a crash.

That's because the following happens later in pwm_backlight_probe():

	pb->subdriver_data = dev_get_drvdata(&pdev->dev);
	...
	bl = backlight_device_register(dev_name(&pdev->dev), &pdev->dev, pb,
				       &pwm_backlight_ops, &props);
	...
	platform_set_drvdata(pdev, bl);

So from then on the result of dev_get_drvdata() is indeed an instance of 
backlight_device from which we can retrieve the subdriver data. I'm not really 
proud of this. But fortunately it seems like we are going to do things 
differently.

Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot - Jan. 24, 2013, 6:10 a.m.
On Wednesday 23 January 2013 17:45:39 Alex Courbot wrote:
> > 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";
> > 		...
> > 	
> > 	};
> 
> This would still prevent any power control from the backlight driver. I.e.
> if someone sets the brightness to 0 through sysfs, we cannot power the
> backlight off as pwm-backlight cannot control more than the PWM without
> platform callbacks. Backlight could only be powered off as a result of a fb
> blank event.

In order to solve this, how about adding a backlight notifier call chain to 
broadcast backlight events in a fashion similar to what is done in 
fbmem/fbcon? Then backlight_update_status() could send events like 
BACKLIGHT_EARLY_EVENT_UPDATE and BACKLIGHT_EVENT_UPDATE to which panel drivers 
could subscribe in order to power the backlight up and down as needed.

Then as the backlight is mentioned in the panel's DT node,

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

the panel's driver could listen to backlight-related events and do its stuff 
transparently, without changing anything to the backlight drivers. This would 
be a good way to replace pwm-backlight's callbacks for platforms that use the 
DT, and would also be applicable to any backlight class device.

Generally speaking, having a mean to monitor backlights state in the kernel 
does not seem too crazy, especially since we already have a way to notify user 
space through backlight_generate_event().

Richard, does that sound ok to you?

Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index adc4754..a77b529 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -516,6 +516,20 @@ 
 		bus-width = <8>;
 	};
 
+	backlight {
+		compatible = "pwm-backlight-ventana";
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <12>;
+
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+
+		power-supply = <&vdd_bl_reg>;
+		panel-supply = <&vdd_pnl_reg>;
+		bl-gpio = <&gpio 28 0>;
+		bl-panel = <&gpio 10 0>;
+	};
+
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -549,7 +563,7 @@ 
 			enable-active-high;
 		};
 
-		regulator@3 {
+		vdd_pnl_reg: regulator@3 {
 			compatible = "regulator-fixed";
 			reg = <3>;
 			regulator-name = "vdd_pnl";
@@ -559,7 +573,7 @@ 
 			enable-active-high;
 		};
 
-		regulator@4 {
+		vdd_bl_reg: regulator@4 {
 			compatible = "regulator-fixed";
 			reg = <4>;
 			regulator-name = "vdd_bl";
diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index a7827fd..1c46602 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -150,6 +150,7 @@  CONFIG_BACKLIGHT_LCD_SUPPORT=y
 CONFIG_BACKLIGHT_CLASS_DEVICE=y
 # CONFIG_BACKLIGHT_GENERIC is not set
 CONFIG_BACKLIGHT_PWM=y
+CONFIG_BACKLIGHT_PWM_TEGRA=y
 CONFIG_FRAMEBUFFER_CONSOLE=y
 CONFIG_LOGO=y
 CONFIG_SOUND=y
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 765a945..377a409 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -244,6 +244,13 @@  config BACKLIGHT_PWM
 	  If you have a LCD backlight adjustable by PWM, say Y to enable
 	  this driver.
 
+config BACKLIGHT_PWM_TEGRA
+	bool "PWM Backlight Driver for Tegra boards"
+	depends on BACKLIGHT_PWM && ARCH_TEGRA
+	help
+	  Support backlight power sequencing for Tegra boards.
+	  Supported boards: Ventana.
+
 config BACKLIGHT_DA903X
 	tristate "Backlight Driver for DA9030/DA9034 using WLED"
 	depends on PMIC_DA903X
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b65a797..1a4a9a3 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -217,6 +217,9 @@  static int pwm_backlight_parse_dt(struct device *dev,
 
 static struct of_device_id pwm_backlight_of_match[] = {
 	{ .compatible = "pwm-backlight" },
+#ifdef CONFIG_BACKLIGHT_PWM_TEGRA
+	{ .compatible = "pwm-backlight-ventana" },
+#endif
 	{ }
 };
 
diff --git a/drivers/video/backlight/pwm_bl_tegra.c b/drivers/video/backlight/pwm_bl_tegra.c
new file mode 100644
index 0000000..8f2195b
--- /dev/null
+++ b/drivers/video/backlight/pwm_bl_tegra.c
@@ -0,0 +1,159 @@ 
+/*
+ * pwm-backlight subdriver for Tegra.
+ *
+ * Copyright (c) 2013 NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/pwm_backlight.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/list.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+
+struct ventana_bl_data {
+	struct regulator *vdd_power;
+	struct regulator *vdd_panel;
+	int bl_gpio;
+	int panel_gpio;
+	bool is_on;
+};
+
+static int init_ventana(struct device *dev)
+{
+	struct ventana_bl_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(struct ventana_bl_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->vdd_power = devm_regulator_get(dev, "power");
+	if (IS_ERR(data->vdd_power)) {
+		dev_err(dev, "cannot get power regulator!\n");
+		return PTR_ERR(data->vdd_power);
+	}
+
+	data->vdd_panel = devm_regulator_get(dev, "panel");
+	if (IS_ERR(data->vdd_panel)) {
+		dev_err(dev, "cannot get panel regulator!\n");
+		return PTR_ERR(data->vdd_panel);
+	}
+
+	ret = of_get_named_gpio(dev->of_node, "bl-gpio", 0);
+	if (ret < 0) {
+		dev_err(dev, "cannot find backlight GPIO!\n");
+		return ret;
+	}
+	data->bl_gpio = ret;
+
+	ret = of_get_named_gpio(dev->of_node, "bl-panel", 0);
+	if (ret < 0) {
+		dev_err(dev, "cannot find panel GPIO!\n");
+		return ret;
+	}
+	data->panel_gpio = ret;
+
+	ret = devm_gpio_request_one(dev, data->bl_gpio,
+				    GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
+				    "backlight");
+	if (ret < 0) {
+		dev_err(dev, "cannot request backlight GPIO!\n");
+		return ret;
+	}
+
+	ret = devm_gpio_request_one(dev, data->panel_gpio,
+				    GPIOF_DIR_OUT | GPIOF_OUT_INIT_LOW,
+				    "panel");
+	if (ret < 0) {
+		dev_err(dev, "cannot request panel GPIO!\n");
+		return ret;
+	}
+
+	pwm_backlight_set_subdriver_data(dev, data);
+
+	return 0;
+}
+
+static void exit_ventana(struct device *dev)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+
+	devm_gpio_free(dev, data->panel_gpio);
+	devm_gpio_free(dev, data->bl_gpio);
+	devm_regulator_put(data->vdd_panel);
+	devm_regulator_put(data->vdd_power);
+	devm_kfree(dev, data);
+}
+
+static int notify_ventana(struct device *dev, int brightness)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+	if (brightness && !data->is_on) {
+		regulator_enable(data->vdd_panel);
+		gpio_set_value(data->panel_gpio, 1);
+		usleep_range(200000, 200000);
+		regulator_enable(data->vdd_power);
+		usleep_range(10000, 10000);
+	} else if (!brightness && data->is_on) {
+		gpio_set_value(data->bl_gpio, 0);
+	}
+
+	return brightness;
+}
+
+static void notify_after_ventana(struct device *dev, int brightness)
+{
+	struct ventana_bl_data *data = pwm_backlight_get_subdriver_data(dev);
+	if (brightness && !data->is_on) {
+		gpio_set_value(data->bl_gpio, 1);
+		data->is_on = true;
+	} else if (!brightness && data->is_on) {
+		usleep_range(10000, 10000);
+		regulator_disable(data->vdd_power);
+		usleep_range(200000, 200000);
+		gpio_set_value(data->panel_gpio, 0);
+		regulator_disable(data->vdd_panel);
+		data->is_on = false;
+	}
+}
+
+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,
+};
+
+static int __init pwm_backlight_tegra_init(void)
+{
+	pwm_backlight_add_subdriver(&pwm_backlight_ventana_subdriver);
+	return 0;
+}
+
+static void __exit pwm_backlight_tegra_exit(void)
+{
+	pwm_backlight_remove_subdriver(&pwm_backlight_ventana_subdriver);
+}
+
+module_init(pwm_backlight_tegra_init);
+module_exit(pwm_backlight_tegra_exit);
+
+MODULE_DESCRIPTION("Backlight Driver for Tegra boards");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-tegra-backlight");
+
+