* Re: Device tree binding for DVFS table
From: Rob Herring @ 2012-07-11 14:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4FFD77FE.8050206@nvidia.com>
On 07/11/2012 07:56 AM, Prashant Gaikwad wrote:
> Hi,
>
> I am working on DT binding for Tegra DVFS.
>
> For Tegra, DVFS node mainly consists of frequency and voltage pairs.
> Frequency in the pair may change for different process. E.g. for process
> 1 CPU clock frequency could be 900MHz at 1V while for process 2 it could
> be 1GHz at 1V.
> Tegra uses vendor specific ids to identify the correct frequency table.
>
> Following is the proposed binding for voltage and frequency tables used
> in DVFS. Looking for comments/suggestions to make it generic.
>
> ====DVFS table node=========> This node defines the voltage configuration for the DVFS which includes
> the regulator and voltage array.
>
> Required properties:
>
> reg_id : <regulator phandle>;
> voltage-table : <voltage array, values in mV>;
>
> #address-cells and #size-cells : To identify correct frequency table
> using process id (or some other vendor specific way).
>
> ====Frequency table node=======>
> This node defines frequency configuration for the device DVFS.
>
> Required properties:
>
> In device node need a reference to the DVFS table node.
>
> dvfs : the phandle for the DVFS table node
>
> frequency-table@n
> reg = <n>; dependent on address-cells and size-cells in DVFS
> table node.
> frequencies = <frequency array, values in MHz>;
You are storing u32 values, why limit the resolution to MHz.
Put the unit in the property names.
> }
>
> Example:
> -------------------------------------------------------------------------
>
> cpu-dvfs-table : dvfs-table {
This should be located with the node that the frequencies correspond to.
> compatible = "nvidia,tegra30-dvfs-table";
> reg_id = <&sm0>;
> #address-cells = <1>;
> #size-cells = <0>;
> voltage-array = <750 775 800 825 850 875 900 925 950 975
> 1000 1025 1050 1100 1125>;
The SOC is really characterized at all these voltages?
> };
>
> device {
> dvfs = <&cpu-dvfs-table>;
> frequency-table@102 {
> reg = <0x102>;
> frequencies = <314 314 314 456 456 456 608 608 608
> 760 817 817 912 1000>;
I don't see the point of repeating frequencies.
> };
> frequency-table@002 {
> reg = <0x002>;
> frequencies = <598 598 750 750 893 893 1000>;
> };
How do you determine the voltage for a frequency on table 2?
I'd expect a single property with freq/volt pairs or 2 properties for
freq and voltage where there is a 1:1 relationship (freq N uses voltage N).
Rob
> };
>
> Thanks & Regards,
> Prashant G
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: Device tree binding for DVFS table
From: Peter De Schrijver @ 2012-07-11 13:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20748722.319671342012092123.JavaMail.weblogic@epml17>
On Wed, Jul 11, 2012 at 03:08:14PM +0200, 함명주 wrote:
> > Hi,
> >
> > I am working on DT binding for Tegra DVFS.
> >
> > For Tegra, DVFS node mainly consists of frequency and voltage pairs.
> > Frequency in the pair may change for different process. E.g. for process
> > 1 CPU clock frequency could be 900MHz at 1V while for process 2 it could
> > be 1GHz at 1V.
> > Tegra uses vendor specific ids to identify the correct frequency table.
>
> Hello,
>
> It seems that in the example, the values in "voltage-array" and
> "frequencies" are switched.
>
> Anyway, what about SoCs that reads information from IEM (or any other module)
> to measure gate delays or some other value to set the appriorate voltage values
> for every possible frequency? I remember some of Exynos SoCs have been doing
> it; dynamically measure the characteristics at boot-up time and apply voltages
> accordingly; they couldn't identify it based on the chip-id or simply by reading
> a single register.
>
But in that case you would have a nominal voltage for each OPP which gets
adjusted at boottime or runtime depending on the exact silicon characteristics?
I would say the DT binding should then specify 1 table with the nominal values
and leave the dynamics to the driver.
Cheers,
Peter.
^ permalink raw reply
* Re: Device tree binding for DVFS table
From: Peter De Schrijver @ 2012-07-11 13:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20748722.319671342012092123.JavaMail.weblogic@epml17>
On Wed, Jul 11, 2012 at 03:08:14PM +0200, 함명주 wrote:
> > Hi,
> >
> > I am working on DT binding for Tegra DVFS.
> >
> > For Tegra, DVFS node mainly consists of frequency and voltage pairs.
> > Frequency in the pair may change for different process. E.g. for process
> > 1 CPU clock frequency could be 900MHz at 1V while for process 2 it could
> > be 1GHz at 1V.
> > Tegra uses vendor specific ids to identify the correct frequency table.
>
> Hello,
>
> It seems that in the example, the values in "voltage-array" and
> "frequencies" are switched.
>
> Anyway, what about SoCs that reads information from IEM (or any other module)
> to measure gate delays or some other value to set the appriorate voltage values
> for every possible frequency? I remember some of Exynos SoCs have been doing
> it; dynamically measure the characteristics at boot-up time and apply voltages
> accordingly; they couldn't identify it based on the chip-id or simply by reading
> a single register.
But in that case you would have a nominal voltage for each OPP which gets
adjusted at boottime or runtime depending on the exact silicon characteristics?
I would say the DT binding should then specify 1 table with the nominal values
and leave the dynamics to the driver.
Cheers,
Peter.
^ permalink raw reply
* Device tree binding for DVFS table
From: Prashant Gaikwad @ 2012-07-11 13:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20748722.319671342012092123.JavaMail.weblogic@epml17>
Hi,
I am working on DT binding for Tegra DVFS.
For Tegra, DVFS node mainly consists of frequency and voltage pairs.
Frequency in the pair may change for different process. E.g. for process
1 CPU clock frequency could be 900MHz at 1V while for process 2 it could
be 1GHz at 1V.
Tegra uses vendor specific ids to identify the correct frequency table.
Following is the proposed binding for voltage and frequency tables used
in DVFS. Looking for comments/suggestions to make it generic.
====DVFS table node=========This node defines the voltage configuration for the DVFS which includes
the regulator and voltage array.
Required properties:
reg_id : <regulator phandle>;
voltage-table : <voltage array, values in mV>;
#address-cells and #size-cells : To identify correct frequency table
using process id (or some other vendor specific way).
====Frequency table node=======
This node defines frequency configuration for the device DVFS.
Required properties:
In device node need a reference to the DVFS table node.
dvfs : the phandle for the DVFS table node
frequency-table@n
reg = <n>; dependent on address-cells and size-cells in DVFS
table node.
frequencies = <frequency array, values in MHz>;
}
Example:
-------------------------------------------------------------------------
cpu-dvfs-table : dvfs-table {
compatible = "nvidia,tegra30-dvfs-table";
reg_id = <&sm0>;
#address-cells = <1>;
#size-cells = <0>;
voltage-array = <750 775 800 825 850 875 900 925 950 975
1000 1025 1050 1100 1125>;
};
device {
dvfs = <&cpu-dvfs-table>;
frequency-table@102 {
reg = <0x102>;
frequencies = <314 314 314 456 456 456 608 608
608 760 817 817 912 1000>;
};
frequency-table@002 {
reg = <0x002>;
frequencies = <598 598 750 750 893 893 1000>;
};
};
Thanks & Regards,
Prashant G
^ permalink raw reply
* Re: Device tree binding for DVFS table
From: 함명주 @ 2012-07-11 13:08 UTC (permalink / raw)
To: linux-arm-kernel
PiBIaSwNCj4NCj4gSSBhbSB3b3JraW5nIG9uIERUIGJpbmRpbmcgZm9yIFRlZ3JhIERWRlMuDQo+
DQo+IEZvciBUZWdyYSwgRFZGUyBub2RlIG1haW5seSBjb25zaXN0cyBvZiBmcmVxdWVuY3kgYW5k
IHZvbHRhZ2UgcGFpcnMuIA0KPiBGcmVxdWVuY3kgaW4gdGhlIHBhaXIgbWF5IGNoYW5nZSBmb3Ig
ZGlmZmVyZW50IHByb2Nlc3MuIEUuZy4gZm9yIHByb2Nlc3MgDQo+IDEgQ1BVIGNsb2NrIGZyZXF1
ZW5jeSBjb3VsZCBiZSA5MDBNSHogYXQgMVYgd2hpbGUgZm9yIHByb2Nlc3MgMiBpdCBjb3VsZCAN
Cj4gYmUgMUdIeiBhdCAxVi4NCj4gVGVncmEgdXNlcyB2ZW5kb3Igc3BlY2lmaWMgaWRzIHRvIGlk
ZW50aWZ5IHRoZSBjb3JyZWN0IGZyZXF1ZW5jeSB0YWJsZS4NCg0KSGVsbG8sDQoNCkl0IHNlZW1z
IHRoYXQgaW4gdGhlIGV4YW1wbGUsIHRoZSB2YWx1ZXMgaW4gInZvbHRhZ2UtYXJyYXkiIGFuZA0K
ImZyZXF1ZW5jaWVzIiBhcmUgc3dpdGNoZWQuDQoNCkFueXdheSwgd2hhdCBhYm91dCBTb0NzIHRo
YXQgcmVhZHMgaW5mb3JtYXRpb24gZnJvbSBJRU0gKG9yIGFueSBvdGhlciBtb2R1bGUpDQogdG8g
bWVhc3VyZSBnYXRlIGRlbGF5cyBvciBzb21lIG90aGVyIHZhbHVlIHRvIHNldCB0aGUgYXBwcmlv
cmF0ZSB2b2x0YWdlIHZhbHVlcw0KIGZvciBldmVyeSBwb3NzaWJsZSBmcmVxdWVuY3k/IEkgcmVt
ZW1iZXIgc29tZSBvZiBFeHlub3MgU29DcyBoYXZlIGJlZW4gZG9pbmcNCiBpdDsgZHluYW1pY2Fs
bHkgbWVhc3VyZSB0aGUgY2hhcmFjdGVyaXN0aWNzIGF0IGJvb3QtdXAgdGltZSBhbmQgYXBwbHkg
dm9sdGFnZXMNCiBhY2NvcmRpbmdseTsgdGhleSBjb3VsZG4ndCBpZGVudGlmeSBpdCBiYXNlZCBv
biB0aGUgY2hpcC1pZCBvciBzaW1wbHkgYnkgcmVhZGluZw0KIGEgc2luZ2xlIHJlZ2lzdGVyLg0K
DQoNCkNoZWVycyENCk15dW5nSm9vLg0KDQo+IA0KPiBGb2xsb3dpbmcgaXMgdGhlIHByb3Bvc2Vk
IGJpbmRpbmcgZm9yIHZvbHRhZ2UgYW5kIGZyZXF1ZW5jeSB0YWJsZXMgdXNlZCANCj4gaW4gRFZG
Uy4gTG9va2luZyBmb3IgY29tbWVudHMvc3VnZ2VzdGlvbnMgdG8gbWFrZSBpdCBnZW5lcmljLg0K
DQo
^ permalink raw reply
* Re: [PATCH v2] of: Add videomode helper
From: Guennadi Liakhovetski @ 2012-07-11 8:34 UTC (permalink / raw)
To: Sascha Hauer
Cc: linux-fbdev, devicetree-discuss, dri-devel, Grant Likely,
Laurent Pinchart, kernel, Mitch Bradley, Anatolij Gustschin
In-Reply-To: <1341388595-30672-1-git-send-email-s.hauer@pengutronix.de>
Hi Sascha
On Wed, 4 Jul 2012, Sascha Hauer wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>
> changes since v1:
> - use hyphens instead of underscores for property names
>
> .../devicetree/bindings/video/displaymode | 40 ++++++++
> drivers/of/Kconfig | 5 +
> drivers/of/Makefile | 1 +
> drivers/of/of_videomode.c | 108 ++++++++++++++++++++
> include/linux/of_videomode.h | 19 ++++
> 5 files changed, 173 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/video/displaymode
> create mode 100644 drivers/of/of_videomode.c
> create mode 100644 include/linux/of_videomode.h
>
> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> new file mode 100644
> index 0000000..43cc17d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/displaymode
> @@ -0,0 +1,40 @@
> +videomode bindings
> +=========
> +
> +Required properties:
> + - xres, yres: Display resolution
> + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
> + in pixels
> + upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
> + lines
> + - clock: displayclock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
How about
+ - hsync-active: Hsync pulse polarity: 1 for high, 0 for low
and similar for vsync-active? Which would then become
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one used by the Linux Framebuffer framework described here in
> +Documentation/fb/framebuffer.txt. This representation has been chosen because it's
> +the only format which does not allow for inconsistent parameters.Unlike the Framebuffer
> +framework the devicetree has the clock in Hz instead of ps.
> +
> +Example:
> +
> + display@0 {
> + /* 1920x1080p24 */
> + clock = <52000000>;
> + xres = <1920>;
> + yres = <1080>;
> + left-margin = <25>;
> + right-margin = <25>;
> + hsync-len = <25>;
> + lower-margin = <2>;
> + upper-margin = <2>;
> + vsync-len = <2>;
> + hsync-active-high;
+ hsync-active = <1>;
> + };
> +
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH v2 11/11] MAINTAINERS: add fblog entry
From: David Herrmann @ 2012-07-09 18:38 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-serial, linux-kernel, florianschandinat, linux-fbdev,
gregkh, alan, bonbons
In-Reply-To: <CAMuHMdX_aC1hzKf1eRqoSsQ4oALBev8p77v7++aoB2KrkOUaEw@mail.gmail.com>
Hi Geert
On Mon, Jul 9, 2012 at 9:33 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sun, Jul 8, 2012 at 11:56 PM, David Herrmann
> <dh.herrmann@googlemail.com> wrote:
>> Add myself as maintainer for the fblog driver to the MAINTAINERS file.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
>> ---
>> MAINTAINERS | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ae8fe46..249b02a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2854,6 +2854,12 @@ F: drivers/video/
>> F: include/video/
>> F: include/linux/fb.h
>>
>> +FRAMEBUFFER LOG DRIVER
>> +M: David Herrmann <dh.herrmann@googlemail.com>
>> +L: linux-serial@vger.kernel.org
>
> Why linux-serial, and not linux-fbdev?
I thought fbcon was maintained on linux-serial as it is very related
to the vt/tty layer. If linux-fbdev is the better place, I can move
this. I CC'ed both lists, anyway.
I have actually no idea where it fits in best.
Thanks
David
^ permalink raw reply
* Re: [PATCH v2 09/11] fblog: register console driver
From: David Herrmann @ 2012-07-09 18:25 UTC (permalink / raw)
To: Joe Perches
Cc: linux-serial, linux-kernel, florianschandinat, linux-fbdev,
gregkh, alan, bonbons
In-Reply-To: <1341785361.13174.25.camel@joe2Laptop>
Hi Joe
On Mon, Jul 9, 2012 at 12:09 AM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2012-07-08 at 23:56 +0200, David Herrmann wrote:
>> We want to print the kernel log to all FBs so we need a console driver.
>> This registers the driver on startup and writes all messages to all
>> registered fblog instances.
>
> Hi David. Trivia only:
>
>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
> []
>> @@ -47,6 +71,107 @@ static struct fblog_fb *fblog_fbs[FB_MAX];
>> static bool active = 1;
>>
>> #define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
>> +#define FBLOG_STR(x) x, sizeof(x) - 1
>
> That's a rather ugly macro.
But handy ;) I've removed it and replaced the call below with a local
variable which works fine too and seems to be even shorter than with
this macro.
>> +static void fblog_buf_resize(struct fblog_buf *buf, size_t width,
>> + size_t height)
>> +{
>> + u16 **lines = NULL;
>> + size_t i, j, minw, minh;
>> +
>> + if (buf->height = height && buf->width = width)
>> + return;
>> +
>> + if (width && height) {
>> + lines = kzalloc(height * sizeof(char*), GFP_KERNEL);
>
> sizeof(char *) is a bit more kernel style like.
Thanks, both fixed.
David
^ permalink raw reply
* Re: [PATCH v2 05/11] fblog: register one fblog object per framebuffer
From: David Herrmann @ 2012-07-09 17:55 UTC (permalink / raw)
To: Joe Perches
Cc: linux-serial, linux-kernel, florianschandinat, linux-fbdev,
gregkh, alan, bonbons
In-Reply-To: <1341785628.13174.28.camel@joe2Laptop>
Hi Joe
On Mon, Jul 9, 2012 at 12:13 AM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2012-07-08 at 23:56 +0200, David Herrmann wrote:
>>
>
> Hi David. Trivial comments only:
>
>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
> []
>> @@ -23,15 +23,204 @@
>> * all fblog instances before running other graphics applications.
>> */
>
> Please add
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> before any #include and...
Thanks, I've added it.
>> static int __init fblog_init(void)
>> {
>> + int ret;
>> +
>> + ret = fb_register_client(&fblog_notifier);
>> + if (ret) {
>> + pr_err("fblog: cannot register framebuffer notifier");
>
> Missing newline, pr_fmt would add module name.
>
> pr_err("cannot register framebuffer notifier\n");
Fixed, too. Thanks!
David
^ permalink raw reply
* Re: [PATCH v2 04/11] fbdev: export get_fb_info()/put_fb_info()
From: David Herrmann @ 2012-07-09 17:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-serial, linux-kernel, florianschandinat, linux-fbdev,
gregkh, alan, bonbons
In-Reply-To: <CAMuHMdWsSqMjqDuE_qgNUjc1eCWZqLLo1GNxwLYZNih=YBFh0g@mail.gmail.com>
Hi Geert
On Mon, Jul 9, 2012 at 9:34 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sun, Jul 8, 2012 at 11:56 PM, David Herrmann
> <dh.herrmann@googlemail.com> wrote:
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -46,7 +46,7 @@ static DEFINE_MUTEX(registration_lock);
>> struct fb_info *registered_fb[FB_MAX] __read_mostly;
>> int num_registered_fb __read_mostly;
>>
>> -static struct fb_info *get_fb_info(unsigned int idx)
>> +struct fb_info *get_fb_info(unsigned int idx)
>> {
>> struct fb_info *fb_info;
>>
>> @@ -61,14 +61,16 @@ static struct fb_info *get_fb_info(unsigned int idx)
>>
>> return fb_info;
>> }
>> +EXPORT_SYMBOL(get_fb_info);
>
> EXPORT_SYMBOL_GPL?
Thanks, that seems more appropriate here. I've fixed it.
>> -static void put_fb_info(struct fb_info *fb_info)
>> +void put_fb_info(struct fb_info *fb_info)
>> {
>> if (!atomic_dec_and_test(&fb_info->count))
>> return;
>> if (fb_info->fbops->fb_destroy)
>> fb_info->fbops->fb_destroy(fb_info);
>> }
>> +EXPORT_SYMBOL(put_fb_info);
>
> EXPORT_SYMBOL_GPL?
Thanks
David
^ permalink raw reply
* Re: [PATCH 0/2] OMAPDSS: PM runtime fixes for 3.5-rc
From: Archit Taneja @ 2012-07-09 7:59 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: tomi.valkeinen, jaswinder.singh, jw, linux-fbdev, linux-omap
In-Reply-To: <4FFA0549.9020802@gmx.de>
Hi Florian,
On Monday 09 July 2012 03:40 AM, Florian Tobias Schandinat wrote:
> Hi Archit,
>
> On 07/04/2012 12:43 PM, Archit Taneja wrote:
>> Hi Florian,
>>
>> These are 2 fixes which Tomi had intended to push for the coming rcs. He is
>> currently on vacation and asked me to post the patches on his behalf.
>>
>> The first patch ensures that system suspend doesn't break on OMAP if DSS is
>> enabled. The second patch prevents some warning backtraces when the kernel is
>> not built with PM runtime.
>>
>> Could you please queue these for the upcoming rc?
>
> as you sent me those patches you should have added your Signed-off-by as
> well, I added it.
> Also the first patch of this series contained 2 coding style issues
> related to braces that checkpatch complained about and also differed
> from the coding style used in those files, I fixed those as well.
> I applied both patches.
>
Thanks for the fixes and the pull. I'll be more careful the next time.
Thanks,
Archit
^ permalink raw reply
* Re: [RFC][PATCH V2 2/3] pwm_backlight: use power sequences
From: Alex Courbot @ 2012-07-09 7:48 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Thierry Reding,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <1341814105-20690-3-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Sorry, I just noticed a mistake in this patch I made while merging
another one. The following also needs to be changed, otherwise the
power-on sequence will never be executed:
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
index 1a38953..4546d23 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -65,7 +98,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb->lth_brightness +
(duty_cycle * (pb->period - pb->lth_brightness) /
max);
pwm_config(pb->pwm, duty_cycle, pb->period);
- pwm_enable(pb->pwm);
+ pwm_backlight_on(bl);
}
Apologies for the inconvenience.
Alex.
^ permalink raw reply related
* Re: [PATCH v2 04/11] fbdev: export get_fb_info()/put_fb_info()
From: Geert Uytterhoeven @ 2012-07-09 7:34 UTC (permalink / raw)
To: David Herrmann
Cc: linux-serial, linux-kernel, florianschandinat, linux-fbdev,
gregkh, alan, bonbons
In-Reply-To: <1341784614-2797-5-git-send-email-dh.herrmann@googlemail.com>
On Sun, Jul 8, 2012 at 11:56 PM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -46,7 +46,7 @@ static DEFINE_MUTEX(registration_lock);
> struct fb_info *registered_fb[FB_MAX] __read_mostly;
> int num_registered_fb __read_mostly;
>
> -static struct fb_info *get_fb_info(unsigned int idx)
> +struct fb_info *get_fb_info(unsigned int idx)
> {
> struct fb_info *fb_info;
>
> @@ -61,14 +61,16 @@ static struct fb_info *get_fb_info(unsigned int idx)
>
> return fb_info;
> }
> +EXPORT_SYMBOL(get_fb_info);
EXPORT_SYMBOL_GPL?
> -static void put_fb_info(struct fb_info *fb_info)
> +void put_fb_info(struct fb_info *fb_info)
> {
> if (!atomic_dec_and_test(&fb_info->count))
> return;
> if (fb_info->fbops->fb_destroy)
> fb_info->fbops->fb_destroy(fb_info);
> }
> +EXPORT_SYMBOL(put_fb_info);
EXPORT_SYMBOL_GPL?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2 11/11] MAINTAINERS: add fblog entry
From: Geert Uytterhoeven @ 2012-07-09 7:33 UTC (permalink / raw)
To: David Herrmann
Cc: linux-serial, linux-kernel, florianschandinat, linux-fbdev,
gregkh, alan, bonbons
In-Reply-To: <1341784614-2797-12-git-send-email-dh.herrmann@googlemail.com>
On Sun, Jul 8, 2012 at 11:56 PM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> Add myself as maintainer for the fblog driver to the MAINTAINERS file.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> MAINTAINERS | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae8fe46..249b02a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2854,6 +2854,12 @@ F: drivers/video/
> F: include/video/
> F: include/linux/fb.h
>
> +FRAMEBUFFER LOG DRIVER
> +M: David Herrmann <dh.herrmann@googlemail.com>
> +L: linux-serial@vger.kernel.org
Why linux-serial, and not linux-fbdev?
> +S: Maintained
> +F: drivers/video/console/fblog.c
> +
> FREESCALE DMA DRIVER
> M: Li Yang <leoli@freescale.com>
> M: Zhang Wei <zw@zh-kernel.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCHv3] pwm_backlight: pass correct brightness to callback
From: Thierry Reding @ 2012-07-09 6:30 UTC (permalink / raw)
To: Alexandre Courbot; +Cc: linux-kernel, linux-fbdev
In-Reply-To: <1341813863-18822-1-git-send-email-acourbot@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]
On Mon, Jul 09, 2012 at 03:04:23PM +0900, Alexandre Courbot wrote:
> pwm_backlight_update_status calls the notify() and notify_after()
> callbacks before and after applying the new PWM settings. However, if
> brightness levels are used, the brightness value will be changed from
> the index into the levels array to the PWM duty cycle length before
> being passed to notify_after(), which results in inconsistent behavior.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/video/backlight/pwm_bl.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
Applied, with a minor stylistic fixup adding a blank line after the
duty_cycle variable declaration. Thanks.
Thierry
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 057389d..be48517 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -54,14 +54,17 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> pwm_config(pb->pwm, 0, pb->period);
> pwm_disable(pb->pwm);
> } else {
> + int duty_cycle;
> if (pb->levels) {
> - brightness = pb->levels[brightness];
> + duty_cycle = pb->levels[brightness];
> max = pb->levels[max];
> + } else {
> + duty_cycle = brightness;
> }
>
> - brightness = pb->lth_brightness +
> - (brightness * (pb->period - pb->lth_brightness) / max);
> - pwm_config(pb->pwm, brightness, pb->period);
> + duty_cycle = pb->lth_brightness +
> + (duty_cycle * (pb->period - pb->lth_brightness) / max);
> + pwm_config(pb->pwm, duty_cycle, pb->period);
> pwm_enable(pb->pwm);
> }
>
> --
> 1.7.11.1
>
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Alex Courbot @ 2012-07-09 6:12 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Thierry Reding', 'Sascha Hauer',
'Stephen Warren', 'Mark Brown',
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <00ae01cd5d92$70d1f9f0$5275edd0$%han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
On 07/09/2012 02:19 PM, Jingoo Han wrote:
> I couldn't agree with Stephen Warren more.
> Could you support DT and non-DT case for backwards compatibility?
Both cases are handled in the new version I just sent. I hope all other
concerns have also been addressed properly. If I forgot something please
ping me.
Alex.
^ permalink raw reply
* [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
From: Alexandre Courbot @ 2012-07-09 6:08 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
Alexandre Courbot
In-Reply-To: <1341814105-20690-1-git-send-email-acourbot@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
arch/arm/boot/dts/tegra20.dtsi | 2 +-
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..c67d9e1 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -317,6 +317,37 @@
bus-width = <8>;
};
+ backlight {
+ compatible = "pwm-backlight";
+ 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 = <&backlight_reg>;
+ enable-gpios = <&gpio 28 0>;
+
+ power-on-sequence = "REGULATOR", "power", <1>,
+ "DELAY", <10>,
+ "PWM", "backlight", <1>,
+ "GPIO", "enable", <1>;
+ power-off-sequence = "GPIO", "enable", <0>,
+ "PWM", "backlight", <0>,
+ "DELAY", <10>,
+ "REGULATOR", "power", <0>;
+ };
+
+ backlight_reg: fixedregulator@176 {
+ compatible = "regulator-fixed";
+ regulator-name = "backlight_regulator";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ gpio = <&gpio 176 0>;
+ startup-delay-us = <0>;
+ enable-active-high;
+ regulator-boot-off;
+ };
+
sound {
compatible = "nvidia,tegra-audio-wm8903-ventana",
"nvidia,tegra-audio-wm8903";
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 405d167..67a6cd9 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -123,7 +123,7 @@
status = "disabled";
};
- pwm {
+ pwm: pwm {
compatible = "nvidia,tegra20-pwm";
reg = <0x7000a000 0x100>;
#pwm-cells = <2>;
--
1.7.11.1
^ permalink raw reply related
* [RFC][PATCH V2 2/3] pwm_backlight: use power sequences
From: Alexandre Courbot @ 2012-07-09 6:08 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
Alexandre Courbot
In-Reply-To: <1341814105-20690-1-git-send-email-acourbot@nvidia.com>
Make use of the power sequences specified in the device tree or platform
data, if any.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
.../bindings/video/backlight/pwm-backlight.txt | 28 ++-
drivers/video/backlight/power_seq.c | 44 ++---
drivers/video/backlight/pwm_bl.c | 210 +++++++++++++++------
include/linux/pwm_backlight.h | 37 +++-
4 files changed, 239 insertions(+), 80 deletions(-)
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..86c9253 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -2,7 +2,10 @@ pwm-backlight bindings
Required properties:
- compatible: "pwm-backlight"
- - pwms: OF device-tree PWM specification (see PWM binding[0])
+ - pwms: OF device-tree PWM specification (see PWM binding[0]). Exactly one PWM
+ must be specified
+ - pwm-names: a list of names for the PWM devices specified in the
+ "pwms" property (see PWM binding[0])
- brightness-levels: Array of distinct brightness levels. Typically these
are in the range from 0 to 255, but any range starting at 0 will do.
The actual brightness level (PWM duty cycle) will be interpolated
@@ -10,10 +13,18 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the "brightness-levels" property)
+ - power-on-sequence: Power sequence that will bring the backlight on. This
+ sequence must reference the PWM specified in the pwms property by its
+ name. It can also reference extra GPIOs or regulators, and introduce
+ delays between sequence steps
+ - power-off-sequence: Power sequence that will bring the backlight off. This
+ sequence must reference the PWM specified in the pwms property by its
+ name. It can also reference extra GPIOs or regulators, and introduce
+ delays between sequence steps
Optional properties:
- - pwm-names: a list of names for the PWM devices specified in the
- "pwms" property (see PWM binding[0])
+ - *-supply: a reference to a regulator used within a power sequence
+ - *-gpios: a reference to a GPIO used within a power sequence.
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
@@ -22,7 +33,18 @@ Example:
backlight {
compatible = "pwm-backlight";
pwms = <&pwm 0 5000000>;
+ pwm-names = "backlight";
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+ power-supply = <&backlight_reg>;
+ enable-gpios = <&gpio 6 0>;
+ power-on-sequence = "REGULATOR", "power", <1>,
+ "DELAY", <10>,
+ "PWM", "backlight", <1>,
+ "GPIO", "enable", <1>;
+ power-off-sequence = "GPIO", "enable", <0>,
+ "PWM", "backlight", <0>,
+ "DELAY", <10>,
+ "REGULATOR", "power", <0>;
};
diff --git a/drivers/video/backlight/power_seq.c b/drivers/video/backlight/power_seq.c
index f54cb7d..f8737db 100644
--- a/drivers/video/backlight/power_seq.c
+++ b/drivers/video/backlight/power_seq.c
@@ -118,9 +118,9 @@ static int of_parse_power_seq_step(struct device *dev, struct property *prop,
tmp_buf[sizeof(tmp_buf) - 6] = 0;
strcat(tmp_buf, "-gpios");
ret = of_get_named_gpio(dev->of_node, tmp_buf, 0);
- if (ret >= 0)
+ if (ret >= 0) {
seq[cpt].value = ret;
- else {
+ } else {
if (ret != -EPROBE_DEFER)
dev_err(dev, "cannot get gpio \"%s\"\n",
seq[cpt].id);
@@ -218,26 +218,26 @@ power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
seq->type = pseq->type;
switch (pseq->type) {
- case POWER_SEQ_REGULATOR:
- case POWER_SEQ_GPIO:
- case POWER_SEQ_PWM:
- if (!(res = power_seq_find_resource(ress, pseq))) {
- /* create resource node */
- res = devm_kzalloc(dev, sizeof(*res),
- GFP_KERNEL);
- if (!res)
- return ERR_PTR(-ENOMEM);
- memcpy(&res->plat, pseq, sizeof(*pseq));
-
- list_add(&res->list, ress);
- }
- seq->resource = res;
- case POWER_SEQ_DELAY:
- seq->parameter = pseq->parameter;
- break;
- default:
- dev_err(dev, "invalid sequence step type!\n");
- return ERR_PTR(-EINVAL);
+ case POWER_SEQ_REGULATOR:
+ case POWER_SEQ_GPIO:
+ case POWER_SEQ_PWM:
+ if (!(res = power_seq_find_resource(ress, pseq))) {
+ /* create resource node */
+ res = devm_kzalloc(dev, sizeof(*res),
+ GFP_KERNEL);
+ if (!res)
+ return ERR_PTR(-ENOMEM);
+ memcpy(&res->plat, pseq, sizeof(*pseq));
+
+ list_add(&res->list, ress);
+ }
+ seq->resource = res;
+ case POWER_SEQ_DELAY:
+ seq->parameter = pseq->parameter;
+ break;
+ default:
+ dev_err(dev, "invalid sequence step type!\n");
+ return ERR_PTR(-EINVAL);
}
}
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1a38953..2936819 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -27,6 +27,12 @@ struct pwm_bl_data {
unsigned int period;
unsigned int lth_brightness;
unsigned int *levels;
+ bool enabled;
+ power_seq_resources resources;
+ power_seq *power_on_seq;
+ power_seq *power_off_seq;
+
+ /* Legacy callbacks */
int (*notify)(struct device *,
int brightness);
void (*notify_after)(struct device *,
@@ -35,6 +41,34 @@ struct pwm_bl_data {
void (*exit)(struct device *);
};
+static void pwm_backlight_on(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+ int ret;
+
+ if (pb->enabled)
+ return;
+
+ if ((ret = power_seq_run(pb->power_on_seq)) < 0)
+ dev_err(&bl->dev, "cannot run power on sequence\n");
+
+ pb->enabled = true;
+}
+
+static void pwm_backlight_off(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+ int ret;
+
+ if (!pb->enabled)
+ return;
+
+ if ((ret = power_seq_run(pb->power_off_seq)) < 0)
+ dev_err(&bl->dev, "cannot run power off sequence\n");
+
+ pb->enabled = false;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -51,8 +85,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
brightness = pb->notify(pb->dev, brightness);
if (brightness = 0) {
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_off(bl);
} else {
int duty_cycle;
if (pb->levels) {
@@ -144,12 +177,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
data->max_brightness--;
}
- /*
- * TODO: Most users of this driver use a number of GPIOs to control
- * backlight power. Support for specifying these needs to be
- * added.
- */
+ data->power_on_seq = of_parse_power_seq(dev, node, "power-on-sequence");
+ if (IS_ERR(data->power_on_seq))
+ return PTR_ERR(data->power_on_seq);
+ data->power_off_seq = of_parse_power_seq(dev, node,
+ "power-off-sequence");
+ if (IS_ERR(data->power_off_seq))
+ return PTR_ERR(data->power_off_seq);
+ data->use_power_sequences = true;
return 0;
}
@@ -167,37 +203,134 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
#endif
+/**
+ * Construct the power sequences corresponding to the legacy platform data.
+ */
+static int pwm_backlight_legacy_probe(struct platform_device *pdev,
+ struct pwm_bl_data *pb)
+{
+ struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
+ struct device *dev = &pdev->dev;
+ struct power_seq_resource *res;
+ struct power_seq_step *step;
+
+ pb->pwm = pwm_get(dev, NULL);
+ if (IS_ERR(pb->pwm)) {
+ dev_warn(dev, "unable to request PWM, trying legacy API\n");
+
+ pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
+ if (IS_ERR(pb->pwm)) {
+ dev_err(dev, "unable to request legacy PWM\n");
+ return PTR_ERR(pb->pwm);
+ }
+ pwm_set_period(pb->pwm, data->pwm_period_ns);
+ }
+
+ pb->notify = data->notify;
+ pb->notify_after = data->notify_after;
+ pb->check_fb = data->check_fb;
+ pb->exit = data->exit;
+ pb->dev = dev;
+
+ /* Now build the resources and sequences corresponding to this PWM */
+ res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+ if (!res) return -ENOMEM;
+ res->plat.type = POWER_SEQ_PWM;
+ res->plat.id = "pwm-backlight";
+ res->pwm = pb->pwm;
+ list_add(&res->list, &pb->resources);
+
+ /* allocate both power on and off sequences at the same time */
+ step = devm_kzalloc(dev, sizeof(*step) * 4, GFP_KERNEL);
+ if (!step) return -ENOMEM;
+ step->type = POWER_SEQ_PWM;
+ step->resource = res;
+ memcpy(&step[2], &step[0], sizeof(*step));
+ step[0].parameter = 1;
+ pb->power_on_seq = &step[0];
+ pb->power_off_seq = &step[2];
+
+ return 0;
+}
+
static int pwm_backlight_probe(struct platform_device *pdev)
{
struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
struct platform_pwm_backlight_data defdata;
+ struct power_seq_resource *res;
struct backlight_properties props;
struct backlight_device *bl;
struct pwm_bl_data *pb;
unsigned int max;
int ret;
+ pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
+ if (!pb) {
+ dev_err(&pdev->dev, "no memory for state\n");
+ return -ENOMEM;
+ }
+
+ INIT_LIST_HEAD(&pb->resources);
+
+ /* using new interface or device tree */
if (!data) {
+ /* build platform data from device tree */
ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
- if (ret < 0) {
+ if (ret = -EPROBE_DEFER) {
+ return ret;
+ } else if (ret < 0) {
dev_err(&pdev->dev, "failed to find platform data\n");
return ret;
}
-
data = &defdata;
}
- if (data->init) {
- ret = data->init(&pdev->dev);
+ if (!data->use_power_sequences) {
+ /* using legacy interface */
+ ret = pwm_backlight_legacy_probe(pdev, pb);
+ if (ret < 0)
+ return ret;
+ } else {
+ /* build sequences and allocate resources from platform data */
+ if (data->power_on_seq) {
+ pb->power_on_seq = power_seq_build(&pdev->dev, &pb->resources,
+ data->power_on_seq);
+ if (IS_ERR(pb->power_on_seq))
+ return PTR_ERR(pb->power_on_seq);
+ }
+ if (data->power_off_seq) {
+ pb->power_off_seq = power_seq_build(&pdev->dev, &pb->resources,
+ data->power_off_seq);
+ if (IS_ERR(pb->power_off_seq))
+ return PTR_ERR(pb->power_off_seq);
+ }
+ ret = power_seq_allocate_resources(&pdev->dev, &pb->resources);
if (ret < 0)
return ret;
+
+ /* we must have exactly one PWM for this driver */
+ list_for_each_entry(res, &pb->resources, list) {
+ if (res->plat.type != POWER_SEQ_PWM)
+ continue;
+ if (pb->pwm) {
+ dev_err(&pdev->dev, "cannot use more than one PWM\n");
+ return -EINVAL;
+ }
+ /* keep the pwm at hand */
+ pb->pwm = res->pwm;
+ }
}
- pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
- if (!pb) {
- dev_err(&pdev->dev, "no memory for state\n");
- ret = -ENOMEM;
- goto err_alloc;
+ /* from here we should have a PWM */
+ if (!pb->pwm) {
+ dev_err(&pdev->dev, "no PWM defined!\n");
+ return -EINVAL;
+ }
+
+ if (data->init) {
+ ret = data->init(&pdev->dev);
+ if (ret < 0)
+ goto err;
}
if (data->levels) {
@@ -207,34 +340,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
max = data->max_brightness;
}
- pb->notify = data->notify;
- pb->notify_after = data->notify_after;
- pb->check_fb = data->check_fb;
- pb->exit = data->exit;
- pb->dev = &pdev->dev;
-
- pb->pwm = pwm_get(&pdev->dev, NULL);
- if (IS_ERR(pb->pwm)) {
- dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
-
- pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
- if (IS_ERR(pb->pwm)) {
- dev_err(&pdev->dev, "unable to request legacy PWM\n");
- ret = PTR_ERR(pb->pwm);
- goto err_alloc;
- }
- }
-
- dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
- /*
- * The DT case will set the pwm_period_ns field to 0 and store the
- * period, parsed from the DT, in the PWM device. For the non-DT case,
- * set the period from platform data.
- */
- if (data->pwm_period_ns > 0)
- pwm_set_period(pb->pwm, data->pwm_period_ns);
-
pb->period = pwm_get_period(pb->pwm);
pb->lth_brightness = data->lth_brightness * (pb->period / max);
@@ -246,20 +351,20 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (IS_ERR(bl)) {
dev_err(&pdev->dev, "failed to register backlight\n");
ret = PTR_ERR(bl);
- goto err_bl;
+ goto err;
}
bl->props.brightness = data->dft_brightness;
backlight_update_status(bl);
platform_set_drvdata(pdev, bl);
+
return 0;
-err_bl:
- pwm_put(pb->pwm);
-err_alloc:
+err:
if (data->exit)
data->exit(&pdev->dev);
+ power_seq_free_resources(&pb->resources);
return ret;
}
@@ -269,9 +374,9 @@ static int pwm_backlight_remove(struct platform_device *pdev)
struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
backlight_device_unregister(bl);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
- pwm_put(pb->pwm);
+ pwm_backlight_off(bl);
+ power_seq_free_resources(&pb->resources);
+
if (pb->exit)
pb->exit(&pdev->dev);
return 0;
@@ -285,8 +390,7 @@ static int pwm_backlight_suspend(struct device *dev)
if (pb->notify)
pb->notify(pb->dev, 0);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_off(bl);
if (pb->notify_after)
pb->notify_after(pb->dev, 0);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..dda267e 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -5,14 +5,47 @@
#define __LINUX_PWM_BACKLIGHT_H
#include <linux/backlight.h>
+#include <linux/power_seq.h>
+/**
+ * Two ways of passing data to the driver can be used:
+ * 1) If not using device tree and use_power_sequences is not set, the legacy
+ * interface is used. power_on_sequence and power_off_sequences are ignored,
+ * and pwm_id and pwm_period_ns can be used to assign a PWM and period to
+ * the backlight. The callback functions will also be called by the driver
+ * at appropriate times.
+ * 2) If use_power_sequences is set, the power sequences should either be NULL
+ * of contain an array of platform_pwm_backlight_seq_step instances
+ * terminated by a full-zero'd one. The described sequences will then be used
+ * for powering the backlight on and off, and the callbacks will not be
+ * called. Instances of resources will be obtained using the get_* functions,
+ * giving id as a consumer name.
+ *
+ * If the device tree is used, the power sequences properties are parsed and
+ * converted to the corresponding power sequences of this structure, which is
+ * passed to the driver with use_power_sequences set to true. See the
+ * pwm-backlight bindings documentation file for more details.
+ */
struct platform_pwm_backlight_data {
- int pwm_id;
unsigned int max_brightness;
unsigned int dft_brightness;
unsigned int lth_brightness;
- unsigned int pwm_period_ns;
unsigned int *levels;
+ /* Set this to true otherwise the legacy interface will be used */
+ bool use_power_sequences;
+ /*
+ * New interface - arrays of steps terminated by a STOP instance, or
+ * NULL if unused.
+ */
+ struct platform_power_seq_step *power_on_seq;
+ struct platform_power_seq_step *power_off_seq;
+ /*
+ * Legacy interface - single PWM and callback methods to control
+ * the power sequence. pwm_id and pwm_period_ns need only be specified
+ * if get_pwm(dev, NULL) will return NULL.
+ */
+ int pwm_id;
+ unsigned int pwm_period_ns;
int (*init)(struct device *dev);
int (*notify)(struct device *dev, int brightness);
void (*notify_after)(struct device *dev, int brightness);
--
1.7.11.1
^ permalink raw reply related
* [RFC][PATCH V2 1/3] power sequences interpreter for device tree
From: Alexandre Courbot @ 2012-07-09 6:08 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
Alexandre Courbot
In-Reply-To: <1341814105-20690-1-git-send-email-acourbot@nvidia.com>
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree, we cannot rely of board-specific
hooks anymore, but still need a way to implement these sequences in a
portable manner. This patch introduces a simple interpreter that can
execute such power sequences encoded either as platform data or within
the device tree.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/video/backlight/Makefile | 2 +-
drivers/video/backlight/power_seq.c | 298 ++++++++++++++++++++++++++++++++++++
drivers/video/backlight/pwm_bl.c | 3 +-
include/linux/power_seq.h | 96 ++++++++++++
4 files changed, 397 insertions(+), 2 deletions(-)
create mode 100644 drivers/video/backlight/power_seq.c
create mode 100644 include/linux/power_seq.h
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index a2ac9cf..6bff124 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -28,7 +28,7 @@ obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o
obj-$(CONFIG_BACKLIGHT_PANDORA) += pandora_bl.o
obj-$(CONFIG_BACKLIGHT_PROGEAR) += progear_bl.o
obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
-obj-$(CONFIG_BACKLIGHT_PWM) += pwm_bl.o
+obj-$(CONFIG_BACKLIGHT_PWM) += pwm_bl.o power_seq.o
obj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o
obj-$(CONFIG_BACKLIGHT_DA9052) += da9052_bl.o
obj-$(CONFIG_BACKLIGHT_MAX8925) += max8925_bl.o
diff --git a/drivers/video/backlight/power_seq.c b/drivers/video/backlight/power_seq.c
new file mode 100644
index 0000000..f54cb7d
--- /dev/null
+++ b/drivers/video/backlight/power_seq.c
@@ -0,0 +1,298 @@
+#include <linux/err.h>
+#include <linux/of_gpio.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/power_seq.h>
+#include <linux/delay.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+
+#define PWM_SEQ_TYPE(type) [POWER_SEQ_ ## type] = #type
+static const char *pwm_seq_types[] = {
+ PWM_SEQ_TYPE(STOP),
+ PWM_SEQ_TYPE(DELAY),
+ PWM_SEQ_TYPE(REGULATOR),
+ PWM_SEQ_TYPE(PWM),
+ PWM_SEQ_TYPE(GPIO),
+};
+#undef PWM_SEQ_TYPE
+
+static bool power_seq_step_run(struct power_seq_step *step)
+{
+ switch (step->type) {
+ case POWER_SEQ_DELAY:
+ msleep(step->parameter);
+ break;
+ case POWER_SEQ_REGULATOR:
+ if (step->parameter)
+ regulator_enable(step->resource->regulator);
+ else
+ regulator_disable(step->resource->regulator);
+ break;
+ case POWER_SEQ_PWM:
+ if (step->parameter)
+ pwm_enable(step->resource->pwm);
+ else
+ pwm_disable(step->resource->pwm);
+ break;
+ case POWER_SEQ_GPIO:
+ gpio_set_value_cansleep(step->resource->gpio, step->parameter);
+ break;
+ /* should never happen since we verify the data when building it */
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+int power_seq_run(power_seq *seq)
+{
+ int err;
+
+ if (!seq) return 0;
+
+ while (seq->type != POWER_SEQ_STOP) {
+ if ((err = power_seq_step_run(seq++))) {
+ return err;
+ }
+ }
+
+ return 0;
+}
+
+static int of_parse_power_seq_step(struct device *dev, struct property *prop,
+ struct platform_power_seq_step *seq,
+ int max_steps)
+{
+ void *value = prop->value;
+ void *end = prop->value + prop->length;
+ int slen, smax, cpt = 0, i, ret;
+ char tmp_buf[32];
+
+ while (value < end && cpt < max_steps) {
+ smax = value - end;
+ slen = strnlen(value, end - value);
+
+ /* Unterminated string / not a string? */
+ if (slen >= end - value)
+ goto invalid_seq;
+
+ /* Find a matching sequence step type */
+ for (i = 0; i < POWER_SEQ_MAX; i++)
+ if (!strcmp(value, pwm_seq_types[i]))
+ break;
+
+ if (i >= POWER_SEQ_MAX)
+ goto unknown_step;
+
+ value += slen + 1;
+
+ seq[cpt].type = i;
+ switch (seq[cpt].type) {
+ case POWER_SEQ_DELAY:
+ /* integer parameter */
+ seq[cpt].parameter = be32_to_cpup(value);
+ value += sizeof(__be32);
+ break;
+ case POWER_SEQ_REGULATOR:
+ case POWER_SEQ_PWM:
+ case POWER_SEQ_GPIO:
+ /* consumer string */
+ slen = strnlen(value, end - value);
+ if (slen >= end - value)
+ goto invalid_seq;
+ seq[cpt].id = value;
+ value += slen + 1;
+
+ /* parameter */
+ seq[cpt].parameter = be32_to_cpup(value);
+ be32_to_cpup(value);
+ value += sizeof(__be32);
+
+ /* For GPIO we still need to resolve the phandle */
+ if (seq[cpt].type != POWER_SEQ_GPIO)
+ break;
+
+ strncpy(tmp_buf, seq[cpt].id, sizeof(tmp_buf) - 6);
+ tmp_buf[sizeof(tmp_buf) - 6] = 0;
+ strcat(tmp_buf, "-gpios");
+ ret = of_get_named_gpio(dev->of_node, tmp_buf, 0);
+ if (ret >= 0)
+ seq[cpt].value = ret;
+ else {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "cannot get gpio \"%s\"\n",
+ seq[cpt].id);
+ return ret;
+ }
+ default:
+ break;
+ }
+
+ cpt++;
+ }
+
+ if (cpt >= max_steps)
+ return -EOVERFLOW;
+
+ return 0;
+
+invalid_seq:
+ dev_err(dev, "invalid sequence \"%s\"\n", prop->name);
+ return -EINVAL;
+unknown_step:
+ dev_err(dev, "unknown step type \"%s\" in sequence \"%s\"\n",
+ (char *)value, prop->name);
+ return -EINVAL;
+}
+
+#define PWM_SEQ_MAX_LENGTH 16
+platform_power_seq *of_parse_power_seq(struct device *dev,
+ struct device_node *node, char *propname)
+{
+ platform_power_seq *seq = NULL;
+ struct property *prop;
+ int length;
+ int ret;
+
+ prop = of_find_property(node, propname, &length);
+ if (prop && length > 0) {
+ seq = devm_kzalloc(dev, sizeof(*seq) * PWM_SEQ_MAX_LENGTH,
+ GFP_KERNEL);
+ if (!seq)
+ return ERR_PTR(-ENOMEM);
+ /* keep one empty entry for the STOP step */
+ ret = of_parse_power_seq_step(dev, prop, seq,
+ PWM_SEQ_MAX_LENGTH - 1);
+ if (ret < 0)
+ return ERR_PTR(ret);
+ }
+
+ return seq;
+}
+
+static
+struct power_seq_resource * power_seq_find_resource(power_seq_resources *ress,
+ struct platform_power_seq_step *res)
+{
+ struct power_seq_resource *step;
+
+ list_for_each_entry(step, ress, list) {
+ if (step->plat.type != res->type) continue;
+ switch (res->type) {
+ case POWER_SEQ_DELAY:
+ case POWER_SEQ_GPIO:
+ if (step->plat.value = res->value)
+ return step;
+ break;
+ default:
+ if (!strcmp(step->plat.id, res->id))
+ return step;
+ break;
+ }
+ }
+
+ return NULL;
+}
+
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+ platform_power_seq *pseq)
+{
+ struct power_seq_step *seq = NULL, *ret;
+ struct power_seq_resource *res;
+ int cpt;
+
+ /* first pass to count the number of elements */
+ for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);
+
+ if (!cpt)
+ return seq;
+
+ /* 1 more for the STOP step */
+ ret = seq = devm_kzalloc(dev, sizeof(*seq) * (cpt + 1), GFP_KERNEL);
+ if (!seq)
+ return ERR_PTR(-ENOMEM);
+
+ for (; pseq->type != POWER_SEQ_STOP; pseq++, seq++) {
+ seq->type = pseq->type;
+
+ switch (pseq->type) {
+ case POWER_SEQ_REGULATOR:
+ case POWER_SEQ_GPIO:
+ case POWER_SEQ_PWM:
+ if (!(res = power_seq_find_resource(ress, pseq))) {
+ /* create resource node */
+ res = devm_kzalloc(dev, sizeof(*res),
+ GFP_KERNEL);
+ if (!res)
+ return ERR_PTR(-ENOMEM);
+ memcpy(&res->plat, pseq, sizeof(*pseq));
+
+ list_add(&res->list, ress);
+ }
+ seq->resource = res;
+ case POWER_SEQ_DELAY:
+ seq->parameter = pseq->parameter;
+ break;
+ default:
+ dev_err(dev, "invalid sequence step type!\n");
+ return ERR_PTR(-EINVAL);
+ }
+ }
+
+ return ret;
+}
+
+int power_seq_allocate_resources(struct device *dev, power_seq_resources *ress)
+{
+ struct power_seq_resource *res;
+ int err;
+
+ list_for_each_entry(res, ress, list) {
+ switch (res->plat.type) {
+ case POWER_SEQ_REGULATOR:
+ res->regulator = devm_regulator_get(dev, res->plat.id);
+ if (IS_ERR(res->regulator)) {
+ dev_err(dev, "cannot get regulator \"%s\"\n",
+ res->plat.id);
+ return PTR_ERR(res->regulator);
+ }
+ dev_dbg(dev, "got regulator %s\n", res->plat.id);
+ break;
+ case POWER_SEQ_PWM:
+ res->pwm = pwm_get(dev, res->plat.id);
+ if (IS_ERR(res->pwm)) {
+ dev_err(dev, "cannot get pwm \"%s\"\n",
+ res->plat.id);
+ return PTR_ERR(res->pwm);
+ }
+ dev_dbg(dev, "got PWM %s\n", res->plat.id);
+ break;
+ case POWER_SEQ_GPIO:
+ err = devm_gpio_request_one(dev, res->plat.value,
+ GPIOF_OUT_INIT_HIGH, "backlight_gpio");
+ if (err) {
+ dev_err(dev, "cannot get gpio %d\n",
+ res->plat.value);
+ return err;
+ }
+ res->gpio = res->plat.value;
+ dev_dbg(dev, "got GPIO %d\n", res->plat.value);
+ break;
+ default:
+ break;
+ };
+ }
+
+ return 0;
+}
+
+void power_seq_free_resources(power_seq_resources *ress) {
+ struct power_seq_resource *res;
+
+ list_for_each_entry(res, ress, list) {
+ if (res->plat.type = POWER_SEQ_PWM)
+ pwm_put(res->pwm);
+ }
+}
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index be48517..1a38953 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -203,8 +203,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (data->levels) {
max = data->levels[data->max_brightness];
pb->levels = data->levels;
- } else
+ } else {
max = data->max_brightness;
+ }
pb->notify = data->notify;
pb->notify_after = data->notify_after;
diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
new file mode 100644
index 0000000..7f76270
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,96 @@
+/*
+ * Simple interpreter for defining power sequences as platform data or device
+ * tree properties. Mainly for use with backlight drivers.
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/of.h>
+#include <linux/types.h>
+
+/**
+ * The different kinds of resources that can be controlled during the sequences.
+ */
+typedef enum {
+ POWER_SEQ_STOP = 0,
+ POWER_SEQ_DELAY,
+ POWER_SEQ_REGULATOR,
+ POWER_SEQ_PWM,
+ POWER_SEQ_GPIO,
+ POWER_SEQ_MAX,
+} seq_type;
+
+/**
+ * Describe something to do during the power-up/down sequence.
+ */
+struct platform_power_seq_step {
+ seq_type type;
+ /**
+ * Identify the resource. Steps of type DELAY use value, others name the
+ * consumer to use in id.
+ */
+ union {
+ const char *id;
+ int value;
+ };
+ /**
+ * A value of 0 disables the resource, while a non-zero enables it.
+ * For DELAY steps this contains the delay to wait in milliseconds.
+ */
+ int parameter;
+};
+typedef struct platform_power_seq_step platform_power_seq;
+
+struct power_seq_resource {
+ /* copied from platform data */
+ struct platform_power_seq_step plat;
+ /* resolved resource */
+ union {
+ struct regulator *regulator;
+ struct pwm_device *pwm;
+ int gpio;
+ };
+ /* used to maintain a list of resources used by the driver */
+ struct list_head list;
+};
+typedef struct list_head power_seq_resources;
+
+struct power_seq_step {
+ seq_type type;
+ int parameter;
+ struct power_seq_resource *resource;
+};
+typedef struct power_seq_step power_seq;
+
+/**
+ * Build a platform data sequence from a device tree node. Memory for the
+ * sequence is allocated using devm_kzalloc on dev.
+ */
+platform_power_seq *of_parse_power_seq(struct device *dev,
+ struct device_node *node,
+ char *propname);
+/**
+ * Build a runnable power sequence from platform data, and add the resources
+ * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
+ * on dev.
+ */
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+ platform_power_seq *pseq);
+/**
+ * Allocate all resources (regulators, PWMs, GPIOs) found by calls to
+ * power_seq_build() for use by dev. Return 0 in case of success, error code
+ * otherwise.
+ */
+int power_seq_allocate_resources(struct device *dev, power_seq_resources *ress);
+/**
+ * Free all the resources previously allocated by power_seq_allocate_resources.
+ */
+void power_seq_free_resources(power_seq_resources *ress);
+/**
+ * Run the given power sequence. Returns 0 on success, error code in case of
+ * failure.
+ */
+int power_seq_run(power_seq *seq);
+
+#endif
--
1.7.11.1
^ permalink raw reply related
* [RFC][PATCHv2 0/3] Power sequences interpreter for pwm_backlight
From: Alexandre Courbot @ 2012-07-09 6:08 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Alexandre Courbot
This is a RFC since this patch largely drifted beyond its original goal
of supporting one GPIO and one regulator for the pwm_backlight driver.
The issue to address is that backlight power sequences, which were
implemented using board-specific callbacks so far, could not be used with
the device tree. This series of patches adds a small power sequence
interpreter that allows to acquire and control regulators, GPIOs, and PWMs
during sequences defined in the device tree. It is easy to use,
low-footprint, and takes care of managing the resources that it acquires.
The implementation is working and should be complete, but documentation is
lacking. Also since the interpreter could be used by other drivers (which
ones?), it may make sense to have it in a better place than
drivers/video/backlight/.
The tegra device tree nodes are just here as an example usage.
Alexandre Courbot (3):
Power sequences interpreter for device tree
pwm-backlight: use power sequences
tegra: add pwm backlight device tree nodes
.../bindings/video/backlight/pwm-backlight.txt | 28 +-
arch/arm/boot/dts/tegra20-ventana.dts | 31 +++
arch/arm/boot/dts/tegra20.dtsi | 2 +-
drivers/video/backlight/Makefile | 2 +-
drivers/video/backlight/power_seq.c | 298 +++++++++++++++++++++
drivers/video/backlight/pwm_bl.c | 212 +++++++++++----
include/linux/power_seq.h | 96 +++++++
include/linux/pwm_backlight.h | 37 ++-
8 files changed, 645 insertions(+), 61 deletions(-)
create mode 100644 drivers/video/backlight/power_seq.c
create mode 100644 include/linux/power_seq.h
--
1.7.11.1
^ permalink raw reply
* [PATCHv3] pwm_backlight: pass correct brightness to callback
From: Alexandre Courbot @ 2012-07-09 6:04 UTC (permalink / raw)
To: Thierry Reding; +Cc: linux-kernel, linux-fbdev, Alexandre Courbot
In-Reply-To: <4FFA6BCA.2040101@nvidia.com>
pwm_backlight_update_status calls the notify() and notify_after()
callbacks before and after applying the new PWM settings. However, if
brightness levels are used, the brightness value will be changed from
the index into the levels array to the PWM duty cycle length before
being passed to notify_after(), which results in inconsistent behavior.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/video/backlight/pwm_bl.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 057389d..be48517 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -54,14 +54,17 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
pwm_config(pb->pwm, 0, pb->period);
pwm_disable(pb->pwm);
} else {
+ int duty_cycle;
if (pb->levels) {
- brightness = pb->levels[brightness];
+ duty_cycle = pb->levels[brightness];
max = pb->levels[max];
+ } else {
+ duty_cycle = brightness;
}
- brightness = pb->lth_brightness +
- (brightness * (pb->period - pb->lth_brightness) / max);
- pwm_config(pb->pwm, brightness, pb->period);
+ duty_cycle = pb->lth_brightness +
+ (duty_cycle * (pb->period - pb->lth_brightness) / max);
+ pwm_config(pb->pwm, duty_cycle, pb->period);
pwm_enable(pb->pwm);
}
--
1.7.11.1
^ permalink raw reply related
* [PATCHv2] pwm_backlight: pass correct brightness to callback
From: Alexandre Courbot @ 2012-07-09 5:36 UTC (permalink / raw)
To: Thierry Reding; +Cc: linux-kernel, linux-fbdev, Alexandre Courbot
In-Reply-To: <4FFA6BCA.2040101@nvidia.com>
pwm_backlight_update_status calls the notify() and notify_after()
callbacks before and after applying the new PWM settings. However, if
brightness levels are used, the brightness value will be changed from
the index into the levels array to the PWM duty cycle length before
being passed to notify_after(), which results in inconsistent behavior.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/video/backlight/pwm_bl.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 057389d..6d1d3ea 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -39,6 +39,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
int brightness = bl->props.brightness;
+ int pwm_brightness;
int max = bl->props.max_brightness;
if (bl->props.power != FB_BLANK_UNBLANK)
@@ -55,13 +56,15 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
pwm_disable(pb->pwm);
} else {
if (pb->levels) {
- brightness = pb->levels[brightness];
+ pwm_brightness = pb->levels[brightness];
max = pb->levels[max];
+ } else {
+ pwm_brightness = brightness;
}
- brightness = pb->lth_brightness +
- (brightness * (pb->period - pb->lth_brightness) / max);
- pwm_config(pb->pwm, brightness, pb->period);
+ pwm_brightness = pb->lth_brightness +
+ (pwm_brightness * (pb->period - pb->lth_brightness) / max);
+ pwm_config(pb->pwm, pwm_brightness, pb->period);
pwm_enable(pb->pwm);
}
--
1.7.11.1
^ permalink raw reply related
* Re: [PATCH] pwm_backlight: pass correct brightness to callback
From: Alex Courbot @ 2012-07-09 5:27 UTC (permalink / raw)
To: Thierry Reding; +Cc: linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org
In-Reply-To: <20120709051001.GA10108@avionic-0098.mockup.avionic-design.de>
On 07/09/2012 02:10 PM, Thierry Reding wrote:
> I had to actually read this patch a number of times and then realized I
> was completely missing the context. Looking at the whole function makes
> it more obvious what you mean.
>
> However I think it'd be much clearer if we just passed the value of
> bl->props.brightness into the callbacks, that way we can avoid the
> additional variable.
This won't work I'm afraid, as brightness can be modified prior to being
passed to the callback function:
if (bl->props.power != FB_BLANK_UNBLANK)
brightness = 0;
if (bl->props.fb_blank != FB_BLANK_UNBLANK)
brightness = 0;
if (pb->notify)
brightness = pb->notify(pb->dev, brightness);
Alex.
^ permalink raw reply
* RE: [PATCH] pwm-backlight: add regulator and GPIO support
From: Jingoo Han @ 2012-07-09 5:19 UTC (permalink / raw)
To: 'Alex Courbot'
Cc: 'Thierry Reding', 'Sascha Hauer',
'Stephen Warren', 'Mark Brown', linux-tegra,
linux-kernel, linux-fbdev
In-Reply-To: <4FF5BAEE.3020403@wwwdotorg.org>
> -----Original Message-----
> From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-owner@vger.kernel.org] On Behalf Of Stephen
> Warren
> Sent: Friday, July 06, 2012 1:04 AM
> To: Alex Courbot
> Cc: Thierry Reding; Sascha Hauer; Mark Brown; linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-fbdev@vger.kernel.org
> Subject: Re: [PATCH] pwm-backlight: add regulator and GPIO support
>
> On 07/05/2012 02:12 AM, Alex Courbot wrote:
> > On 07/05/2012 04:57 PM, Thierry Reding wrote:
> >> I agree. Non-DT platforms have always used the callbacks to execute this
> >> kind of code. As you've said before there are situations where it isn't
> >> just about setting a GPIO or enabling a regulator but it also requires a
> >> specific timing. Representing this in the platform data would become
> >> tedious.
> >
> > That will settle the whole issue then.
> >
> >> So I think for the DT case you can parse the power-on and power-off
> >> sequences directly and execute code based on it, while in non-DT cases
> >> the init and exit callbacks should be used instead. I think it even
> >> makes sense to reuse the platform data's init and exit functions in the
> >> DT case and implement the parser/interpreter within those.
> >
> > It totally makes sense indeed.
>
> I don't agree here. It'd be best if non-DT and DT cases worked as
> similarly as possible. Relying on callbacks in one case and
> data-parsed-from-DT in the other isn't consistent with that. After all,
> in the DT case, you parse some data out of the DT and into some data
> structure. In the non-DT case, you can have that data structure passed
> in directly using platform data. Now, there's certainly a need to
> continue to support callbacks for backwards compatibility, at the very
> least temporarily before all clients are converted to the new model, but
> requiring different models rather than simply allowing it seems like a
> bad idea to me.
Hi Alex Courbot,
I couldn't agree with Stephen Warren more.
Could you support DT and non-DT case for backwards compatibility?
Best regards,
Jingoo Han
>
>
> --
> 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
^ permalink raw reply
* RE: [PATCH] pwm_backlight: pass correct brightness to callback
From: Jingoo Han @ 2012-07-09 5:11 UTC (permalink / raw)
To: 'Alexandre Courbot', 'Thierry Reding'
Cc: linux-kernel, linux-fbdev, 'Jingoo Han'
In-Reply-To: <1341807700-7103-1-git-send-email-acourbot@nvidia.com>
On Monday, July 09, 2012 1:22 PM, Alexandre Courbot wrote:
> pwm_backlight_update_status calls two callbacks before and after
> applying the new PWM settings. However, the brightness scale is
> completely changed in between if brightness levels are used. This patch
> ensures that both callbacks are passed brightness values of the same
> meaning.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/video/backlight/pwm_bl.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 057389d..dd4d24d 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -39,6 +39,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> {
> struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> int brightness = bl->props.brightness;
> + int pwm_brightness;
> int max = bl->props.max_brightness;
>
> if (bl->props.power != FB_BLANK_UNBLANK)
> @@ -55,13 +56,14 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> pwm_disable(pb->pwm);
> } else {
> if (pb->levels) {
> - brightness = pb->levels[brightness];
> + pwm_brightness = pb->levels[brightness];
> max = pb->levels[max];
> - }
> + } else
> + pwm_brightness = brightness;
Hi Alexandre Courbot,
Please, use braces to keep the Coding Style.
Refer to Documentation/CodingStyle as follow:
169 This does not apply if only one branch of a conditional statement is a single
170 statement; in the latter case use braces in both branches:
171
172 if (condition) {
173 do_this();
174 do_that();
175 } else {
176 otherwise();
177 }
Best regards,
Jingoo Han
>
> - brightness = pb->lth_brightness +
> - (brightness * (pb->period - pb->lth_brightness) / max);
> - pwm_config(pb->pwm, brightness, pb->period);
> + pwm_brightness = pb->lth_brightness +
> + (pwm_brightness * (pb->period - pb->lth_brightness) / max);
> + pwm_config(pb->pwm, pwm_brightness, pb->period);
> pwm_enable(pb->pwm);
> }
>
> --
> 1.7.11.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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox