This series introduces a way to use pwm-backlight hooks with platforms
that use the device tree through a subdriver system. It also adds support
for the Tegra-based Ventana board, adding the last missing block to enable
its panel. Support for other Tegra board can thus be easily added.
I have something else in mind to properly support this (power
sequences), but this work relies on the GPIO subsystem redesign which will
take some time. The pwm-backlight subdrivers can do the job by the meantime.
There are a few design points that might need to be discussed:
1) Link order is important: subdrivers register themselves in their
module_init function, which must be called before pwm-backlight's probe.
This forbids linking subdrivers as separate modules from pwm-backlight.
2) The subdriver's data is temporarily passed through the backlight
device's driver data. This should not hurt, but maybe there is a better way
to do this.
3) Subdrivers must add themselves into pwm-backlight's own of_device_id
table. It would be cleaner to not have to list subdrivers into
pwm-backlight's main file, but I cannot think of a way to do otherwise.
Suggestions for the 3 points listed above are very welcome - in any case,
I hope to make this converge into something mergeable quickly.
Note that these patches are the last missing block to get a functional
panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
on Ventana is usable out-of-the-box. Yay.
Alexandre Courbot (3):
pwm-backlight: add subdriver mechanism
tegra: pwm-backlight: add tegra pwm-bl driver
tegra: ventana: of: add host1x device to DT
arch/arm/boot/dts/tegra20-ventana.dts | 29 +++++-
arch/arm/configs/tegra_defconfig | 1 +
drivers/video/backlight/Kconfig | 7 ++
drivers/video/backlight/Makefile | 4 +
drivers/video/backlight/pwm_bl.c | 70 ++++++++++++++-
drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
include/linux/pwm_backlight.h | 15 ++++
7 files changed, 281 insertions(+), 4 deletions(-)
create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
--
1.8.1.1
PWM-controlled backlights often need additional power control prior
to activating the PWM, typically switching regulators or GPIOs. This has
been done so far through hooks defined in board files, but this
mechanism cannot be used on platforms that rely on the device tree.
This patch introduces a "subdriver" mechanism to the pwm-backlight
driver that allows such hooks to be defined in optionally-compiled
sub-drivers. Every subdriver has its own device tree properties, which
sets the correct hooks to the pwm-backlight driver.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/video/backlight/Makefile | 4 +++
drivers/video/backlight/pwm_bl.c | 67 +++++++++++++++++++++++++++++++++++++++-
include/linux/pwm_backlight.h | 15 +++++++++
3 files changed, 85 insertions(+), 1 deletion(-)
@@ -29,6 +29,10 @@ obj-$(CONFIG_BACKLIGHT_LP855X) += lp855x_bl.oobj-$(CONFIG_BACKLIGHT_OMAP1)+=omap1_bl.oobj-$(CONFIG_BACKLIGHT_PANDORA)+=pandora_bl.oobj-$(CONFIG_BACKLIGHT_CARILLO_RANCH)+=cr_bllcd.o+# pwm-backlight subdrivers must be listed *before* pwm_bl.o.+# Link order is important as subdrivers must register themselves+# before pwm-backlight's probe function can be called.+obj-$(CONFIG_BACKLIGHT_PWM_TEGRA)+=pwm_bl_tegra.oobj-$(CONFIG_BACKLIGHT_PWM)+=pwm_bl.oobj-$(CONFIG_BACKLIGHT_DA903X)+=da903x_bl.oobj-$(CONFIG_BACKLIGHT_DA9052)+=da9052_bl.o
@@ -150,6 +200,17 @@ static int pwm_backlight_parse_dt(struct device *dev,*backlightpower.Supportforspecifyingtheseneedstobe*added.*/+mutex_lock(&pwm_backlight_subdrivers_mutex);+list_for_each_entry(subdriver,&pwm_backlight_subdrivers,list)+if(of_device_is_compatible(node,subdriver->name)){+data->init=subdriver->init;+data->exit=subdriver->exit;+data->notify=subdriver->notify;+data->notify_after=subdriver->notify_after;+data->check_fb=subdriver->check_fb;+break;+}+mutex_unlock(&pwm_backlight_subdrivers_mutex);return0;}
@@ -201,6 +262,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)gotoerr_alloc;}+/* if the init function set subdriver data, move it to correct place */+pb->subdriver_data=dev_get_drvdata(&pdev->dev);+if(data->levels){max=data->levels[data->max_brightness];pb->levels=data->levels;
@@ -249,10 +313,11 @@ static int pwm_backlight_probe(struct platform_device *pdev)gotoerr_alloc;}+platform_set_drvdata(pdev,bl);+bl->props.brightness=data->dft_brightness;backlight_update_status(bl);-platform_set_drvdata(pdev,bl);return0;err_alloc:
@@ -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
From: Mark Zhang <hidden> Date: 2013-01-20 03:38:10
Yeah, thanks Alex. :)
So this is a non power sequence version of backlight & panel enabling,
isn't it? I remember we talked about this several days ago and you
mentioned kernel guys want an ad-hoc version(power sequence logics
inside driver, not in DT) and I believe this is it, right?
I think finally I can enable Tegra30 cardhu's display after this patch
merged.
Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
This series introduces a way to use pwm-backlight hooks with platforms
that use the device tree through a subdriver system. It also adds support
for the Tegra-based Ventana board, adding the last missing block to enable
its panel. Support for other Tegra board can thus be easily added.
I have something else in mind to properly support this (power
sequences), but this work relies on the GPIO subsystem redesign which will
take some time. The pwm-backlight subdrivers can do the job by the meantime.
There are a few design points that might need to be discussed:
1) Link order is important: subdrivers register themselves in their
module_init function, which must be called before pwm-backlight's probe.
This forbids linking subdrivers as separate modules from pwm-backlight.
2) The subdriver's data is temporarily passed through the backlight
device's driver data. This should not hurt, but maybe there is a better way
to do this.
3) Subdrivers must add themselves into pwm-backlight's own of_device_id
table. It would be cleaner to not have to list subdrivers into
pwm-backlight's main file, but I cannot think of a way to do otherwise.
Suggestions for the 3 points listed above are very welcome - in any case,
I hope to make this converge into something mergeable quickly.
Note that these patches are the last missing block to get a functional
panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
on Ventana is usable out-of-the-box. Yay.
Alexandre Courbot (3):
pwm-backlight: add subdriver mechanism
tegra: pwm-backlight: add tegra pwm-bl driver
tegra: ventana: of: add host1x device to DT
arch/arm/boot/dts/tegra20-ventana.dts | 29 +++++-
arch/arm/configs/tegra_defconfig | 1 +
drivers/video/backlight/Kconfig | 7 ++
drivers/video/backlight/Makefile | 4 +
drivers/video/backlight/pwm_bl.c | 70 ++++++++++++++-
drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
include/linux/pwm_backlight.h | 15 ++++
7 files changed, 281 insertions(+), 4 deletions(-)
create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
On Sun, Jan 20, 2013 at 12:38 PM, Mark Zhang [off-list ref] wrote:
So this is a non power sequence version of backlight & panel enabling,
isn't it? I remember we talked about this several days ago and you
mentioned kernel guys want an ad-hoc version(power sequence logics
inside driver, not in DT) and I believe this is it, right?
Basically, yes - I still think power-seqs could be useful here
(especially after seeing the size of these sub-drivers if you want to
do error checking properly) and plan to give it another shot without
DT, but this will not happen soon since we need to do some GPIO
redesign before. You can see what's wrong in the init() function of
the subdriver: we call a device tree function to obtain the GPIO as
there is no get function.
I think finally I can enable Tegra30 cardhu's display after this patch
merged.
Yes, feel free to write a subdriver for Cardhu if you like - I'd like
to see all T20 and T30 boards supported by the time this gets merged.
Thanks,
Alex.
From: Mark Zhang <hidden> Date: 2013-01-20 05:51:59
On 01/20/2013 01:26 PM, Alexandre Courbot wrote:
On Sun, Jan 20, 2013 at 12:38 PM, Mark Zhang [off-list ref] wrote:
quoted
So this is a non power sequence version of backlight & panel enabling,
isn't it? I remember we talked about this several days ago and you
mentioned kernel guys want an ad-hoc version(power sequence logics
inside driver, not in DT) and I believe this is it, right?
Basically, yes - I still think power-seqs could be useful here
(especially after seeing the size of these sub-drivers if you want to
do error checking properly) and plan to give it another shot without
DT, but this will not happen soon since we need to do some GPIO
redesign before. You can see what's wrong in the init() function of
the subdriver: we call a device tree function to obtain the GPIO as
there is no get function.
Okay. I think I got the picture. I'll read the codes when I'm free and I
think I'll understand this better after that.
quoted
I think finally I can enable Tegra30 cardhu's display after this patch
merged.
Yes, feel free to write a subdriver for Cardhu if you like - I'd like
to see all T20 and T30 boards supported by the time this gets merged.
Yep, I can try to do that. I'll let you know if I have problems.
Mark
From: Mark Zhang <hidden> Date: 2013-01-21 02:09:18
Hi Alex,
This patch set applies failed on tot linux-next(0118). Here is the log:
markz@markz-hp6200:~/tegradrm/official-upstream-kernel$ git am
~/Desktop/*.eml
Applying: pwm-backlight: add subdriver mechanism
error: patch failed: drivers/video/backlight/pwm_bl.c:35
error: drivers/video/backlight/pwm_bl.c: patch does not apply
Patch failed at 0001 pwm-backlight: add subdriver mechanism
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Anyway, I'll try to apply this on 3.8-rc4.
Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
This series introduces a way to use pwm-backlight hooks with platforms
that use the device tree through a subdriver system. It also adds support
for the Tegra-based Ventana board, adding the last missing block to enable
its panel. Support for other Tegra board can thus be easily added.
I have something else in mind to properly support this (power
sequences), but this work relies on the GPIO subsystem redesign which will
take some time. The pwm-backlight subdrivers can do the job by the meantime.
There are a few design points that might need to be discussed:
1) Link order is important: subdrivers register themselves in their
module_init function, which must be called before pwm-backlight's probe.
This forbids linking subdrivers as separate modules from pwm-backlight.
2) The subdriver's data is temporarily passed through the backlight
device's driver data. This should not hurt, but maybe there is a better way
to do this.
3) Subdrivers must add themselves into pwm-backlight's own of_device_id
table. It would be cleaner to not have to list subdrivers into
pwm-backlight's main file, but I cannot think of a way to do otherwise.
Suggestions for the 3 points listed above are very welcome - in any case,
I hope to make this converge into something mergeable quickly.
Note that these patches are the last missing block to get a functional
panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
on Ventana is usable out-of-the-box. Yay.
Alexandre Courbot (3):
pwm-backlight: add subdriver mechanism
tegra: pwm-backlight: add tegra pwm-bl driver
tegra: ventana: of: add host1x device to DT
arch/arm/boot/dts/tegra20-ventana.dts | 29 +++++-
arch/arm/configs/tegra_defconfig | 1 +
drivers/video/backlight/Kconfig | 7 ++
drivers/video/backlight/Makefile | 4 +
drivers/video/backlight/pwm_bl.c | 70 ++++++++++++++-
drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
include/linux/pwm_backlight.h | 15 ++++
7 files changed, 281 insertions(+), 4 deletions(-)
create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
From: Mark Zhang <hidden> Date: 2013-01-21 02:59:44
Patch is applied OK on 3.8-rc4.
Hmmm.. But I think it's better to make the patch can be applied on
linux-next.
Mark
On 01/21/2013 10:09 AM, Mark Zhang wrote:
Hi Alex,
This patch set applies failed on tot linux-next(0118). Here is the log:
markz@markz-hp6200:~/tegradrm/official-upstream-kernel$ git am
~/Desktop/*.eml
Applying: pwm-backlight: add subdriver mechanism
error: patch failed: drivers/video/backlight/pwm_bl.c:35
error: drivers/video/backlight/pwm_bl.c: patch does not apply
Patch failed at 0001 pwm-backlight: add subdriver mechanism
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Anyway, I'll try to apply this on 3.8-rc4.
Mark
On 01/19/2013 06:30 PM, Alexandre Courbot wrote:
quoted
This series introduces a way to use pwm-backlight hooks with platforms
that use the device tree through a subdriver system. It also adds support
for the Tegra-based Ventana board, adding the last missing block to enable
its panel. Support for other Tegra board can thus be easily added.
I have something else in mind to properly support this (power
sequences), but this work relies on the GPIO subsystem redesign which will
take some time. The pwm-backlight subdrivers can do the job by the meantime.
There are a few design points that might need to be discussed:
1) Link order is important: subdrivers register themselves in their
module_init function, which must be called before pwm-backlight's probe.
This forbids linking subdrivers as separate modules from pwm-backlight.
2) The subdriver's data is temporarily passed through the backlight
device's driver data. This should not hurt, but maybe there is a better way
to do this.
3) Subdrivers must add themselves into pwm-backlight's own of_device_id
table. It would be cleaner to not have to list subdrivers into
pwm-backlight's main file, but I cannot think of a way to do otherwise.
Suggestions for the 3 points listed above are very welcome - in any case,
I hope to make this converge into something mergeable quickly.
Note that these patches are the last missing block to get a functional
panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
on Ventana is usable out-of-the-box. Yay.
Alexandre Courbot (3):
pwm-backlight: add subdriver mechanism
tegra: pwm-backlight: add tegra pwm-bl driver
tegra: ventana: of: add host1x device to DT
arch/arm/boot/dts/tegra20-ventana.dts | 29 +++++-
arch/arm/configs/tegra_defconfig | 1 +
drivers/video/backlight/Kconfig | 7 ++
drivers/video/backlight/Makefile | 4 +
drivers/video/backlight/pwm_bl.c | 70 ++++++++++++++-
drivers/video/backlight/pwm_bl_tegra.c | 159 +++++++++++++++++++++++++++++++++
include/linux/pwm_backlight.h | 15 ++++
7 files changed, 281 insertions(+), 4 deletions(-)
create mode 100644 drivers/video/backlight/pwm_bl_tegra.c
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.
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
quoted hunk
@@ -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");
+
+
On Sat, Jan 19, 2013 at 07:30:17PM +0900, Alexandre Courbot wrote:
This series introduces a way to use pwm-backlight hooks with platforms
that use the device tree through a subdriver system. It also adds support
for the Tegra-based Ventana board, adding the last missing block to enable
its panel. Support for other Tegra board can thus be easily added.
I have something else in mind to properly support this (power
sequences), but this work relies on the GPIO subsystem redesign which will
take some time. The pwm-backlight subdrivers can do the job by the meantime.
There are a few design points that might need to be discussed:
1) Link order is important: subdrivers register themselves in their
module_init function, which must be called before pwm-backlight's probe.
This forbids linking subdrivers as separate modules from pwm-backlight.
2) The subdriver's data is temporarily passed through the backlight
device's driver data. This should not hurt, but maybe there is a better way
to do this.
3) Subdrivers must add themselves into pwm-backlight's own of_device_id
table. It would be cleaner to not have to list subdrivers into
pwm-backlight's main file, but I cannot think of a way to do otherwise.
Suggestions for the 3 points listed above are very welcome - in any case,
I hope to make this converge into something mergeable quickly.
Note that these patches are the last missing block to get a functional
panel on Tegra boards. Using 3.8rc4 and these patches, the internal panel
on Ventana is usable out-of-the-box. Yay.
Hi Alexandre,
It's great to see you pick this up. I've been meaning to do this myself
but I just can't find the time right now. Generally I think the approach
you've chosen looks good, but I don't think doing it in pwm-backlight is
the right way.
Eventually this should all be covered by the CDF, but since that's not
ready yet we want something ad-hoc to get the hardware supported. As
such I would like to see this go into some sort of minimalistic, Tegra-
specific display/panel framework. I'd prefer to keep the pwm-backlight
driver as simple and generic as possible, that is, a driver for a PWM-
controlled backlight.
Another advantage of moving this into a sort of display framework is
that it may help in defining the requirements for a CDF and that moving
the code to the CDF should be easier once it is done.
Last but not least, abstracting away the panel allows other things such
as physical dimensions and display modes to be properly encapsulated. I
think that power-on/off timing requirements for panels also belong to
this set since they are usually specific to a given panel.
Maybe adding these drivers to tegra-drm for now would be a good option.
That way the corresponding glue can be added without a need for inter-
tree dependencies.
Thierry
From: Alex Courbot <acourbot@nvidia.com> Date: 2013-01-21 08:18:16
Hi Thierry,
On Monday 21 January 2013 15:49:28 Thierry Reding wrote:
Eventually this should all be covered by the CDF, but since that's not
ready yet we want something ad-hoc to get the hardware supported. As
such I would like to see this go into some sort of minimalistic, Tegra-
specific display/panel framework. I'd prefer to keep the pwm-backlight
driver as simple and generic as possible, that is, a driver for a PWM-
controlled backlight.
Another advantage of moving this into a sort of display framework is
that it may help in defining the requirements for a CDF and that moving
the code to the CDF should be easier once it is done.
Last but not least, abstracting away the panel allows other things such
as physical dimensions and display modes to be properly encapsulated. I
think that power-on/off timing requirements for panels also belong to
this set since they are usually specific to a given panel.
Maybe adding these drivers to tegra-drm for now would be a good option.
That way the corresponding glue can be added without a need for inter-
tree dependencies.
IIRC (because that was a while ago already) having a Tegra-only display
framework is exactly what we wanted to avoid in the first place. This series
does nothing but leverage the callbacks mechanism that already exists in pwm-
backlight and make it available to DT systems. If we start making a Tegra-
specific solution, then other architectures will have to reinvent the wheel
again. I really don't think we want to go that way.
These patches only makes slight changes to pwm_bl.c and do not extend its
capabilities. I agree that a suitable solution will require the CDF, but by
the meantime, let's go for the practical route instead of repeating the same
mistakes (i.e. architecture-specific frameworks) again.
There are certainly better ways to do this, but I'm not convinced at all that
a Tegra-only solution is one of them.
Alex.
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.
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).
quoted
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.
quoted
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
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
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...
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.
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");
+
+
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.
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?
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.
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.
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?
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.
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
On Mon, Jan 21, 2013 at 05:18:11PM +0900, Alex Courbot wrote:
Hi Thierry,
On Monday 21 January 2013 15:49:28 Thierry Reding wrote:
quoted
Eventually this should all be covered by the CDF, but since that's not
ready yet we want something ad-hoc to get the hardware supported. As
such I would like to see this go into some sort of minimalistic, Tegra-
specific display/panel framework. I'd prefer to keep the pwm-backlight
driver as simple and generic as possible, that is, a driver for a PWM-
controlled backlight.
Another advantage of moving this into a sort of display framework is
that it may help in defining the requirements for a CDF and that moving
the code to the CDF should be easier once it is done.
Last but not least, abstracting away the panel allows other things such
as physical dimensions and display modes to be properly encapsulated. I
think that power-on/off timing requirements for panels also belong to
this set since they are usually specific to a given panel.
Maybe adding these drivers to tegra-drm for now would be a good option.
That way the corresponding glue can be added without a need for inter-
tree dependencies.
IIRC (because that was a while ago already) having a Tegra-only display
framework is exactly what we wanted to avoid in the first place. This series
does nothing but leverage the callbacks mechanism that already exists in pwm-
backlight and make it available to DT systems. If we start making a Tegra-
specific solution, then other architectures will have to reinvent the wheel
again. I really don't think we want to go that way.
These patches only makes slight changes to pwm_bl.c and do not extend its
capabilities. I agree that a suitable solution will require the CDF, but by
the meantime, let's go for the practical route instead of repeating the same
mistakes (i.e. architecture-specific frameworks) again.
There are certainly better ways to do this, but I'm not convinced at all that
a Tegra-only solution is one of them.
Well, your proposal is a Tegra-only solution as well. Anything we come
up with now will be Tegra-only because it will eventually be integrated
with the CDF.
Trying to come up with something generic would be counter-productive.
CDF *is* the generic solution. All we would be doing is add a competing
framework.
Thierry
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?
From: Alex Courbot <acourbot@nvidia.com> Date: 2013-01-23 09:45:47
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.
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.
@@ -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
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
From: Alex Courbot <acourbot@nvidia.com> Date: 2013-01-23 10:29:20
On Wednesday 23 January 2013 18:15:30 Leela Krishna Amudala wrote:
quoted
+ 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.
From: Alex Courbot <acourbot@nvidia.com> Date: 2013-01-24 06:10:16
On Wednesday 23 January 2013 17:45:39 Alex Courbot wrote:
quoted
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,
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.