| Message ID | 1358591420-7790-3-git-send-email-acourbot@nvidia.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
> 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
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
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
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
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"); + +
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