* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Mark Zhang @ 2013-10-23 2:16 UTC (permalink / raw)
To: Thierry Reding
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20131022124905.GA24255@ulmo.nvidia.com>
On 10/22/2013 08:49 PM, Thierry Reding wrote:
> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
>> [...]
>>>>>
>>>>
>>>> Okay, I just want to set the "notify" function pointer in "struct
>>>> platform_pwm_backlight_data", because I want to tune the brightness
>>>> value before the pwm-bl sets the brightness to hardware. I don't know
>>>> how to do that, unless we define the platform data explicitly.
>>>
>>> Okay, my question should have been what you need the functions for and
>>> why you think you need them.
>>>
>>
>> If I understanding you correctly, I suppose I've said that: "because I
>> want to tune the brightness value before the pwm-bl sets the brightness
>> to hardware".
>
> Why do you want to tune the brightness value? What are you trying to
> achieve?
>
Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
color value to make the display looks bright so that we can reduce the
backlight brightness to save power. So everytime PRISM is triggered, we
call "backlight_update_status", then in the "notify" callback, we change
the brightness value which pwm-bl provides by considering the PRISM
compensations.
Mark
> Thierry
>
^ permalink raw reply
* Re: [PATCH 0/9] backlight: atmel-pwm-bl: fixes and clean ups
From: Nicolas Ferre @ 2013-10-23 7:57 UTC (permalink / raw)
To: Jingoo Han, 'Johan Hovold'
Cc: 'Andrew Morton', 'Tomi Valkeinen',
'Jean-Christophe Plagniol-Villard',
'Richard Purdie', linux-fbdev, linux-kernel
In-Reply-To: <001801cecf8d$f3834d90$da89e8b0$%han@samsung.com>
On 23/10/2013 02:19, Jingoo Han :
> On Wednesday, October 23, 2013 2:27 AM, Johan Hovold wrote:
>>
>> These patches fix a few issues and clean up the atmel-pwm-bl driver
>> somewhat.
>>
>> Johan
>>
>> Johan Hovold (9):
>> backlight: atmel-pwm-bl: fix reported brightness
>> backlight: atmel-pwm-bl: fix gpio polarity in remove
>> backlight: atmel-pwm-bl: fix module autoload
>> backlight: atmel-pwm-bl: clean up probe error handling
>> backlight: atmel-pwm-bl: clean up get_intensity
>> backlight: atmel-pwm-bl: remove unused include
>> backlight: atmel-pwm-bl: use gpio_is_valid
>> backlight: atmel-pwm-bl: refactor gpio_on handling
>> backlight: atmel-pwm-bl: use gpio_request_one
>
> ++cc Andrew Morton, Tomi Valkeinen, Jean-Christophe Plagniol-Villard
>
> Hi Johan Hovold,
>
> Currently, because there is no git tree for backlight,
> backlight patches have been merged to mm-tree by Andrew Morton.
> Please, add Andrew Morton to CC list.
>
> Also, there is another way.
> If Nicolas Ferre wants to merge these patches, the patches can be
> merged through ATMEL-SoC tree with my Acked-by.
Hi,
As it's a driver without interaction with AT91 code, maybe routing this
patch series through mm-tree is the way to go.
If you find any issue in the process, please tell me. I would be happy
to ease the process.
Bye,
>>
>> drivers/video/backlight/atmel-pwm-bl.c | 86 ++++++++++++++++------------------
>> 1 file changed, 40 insertions(+), 46 deletions(-)
>
>
--
Nicolas Ferre
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Thierry Reding @ 2013-10-23 8:00 UTC (permalink / raw)
To: Mark Zhang
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <52673178.8070805@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]
On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
> On 10/22/2013 08:49 PM, Thierry Reding wrote:
> > On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
> >> On 10/22/2013 03:24 PM, Thierry Reding wrote:
> >>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
> >> [...]
> >>>>>
> >>>>
> >>>> Okay, I just want to set the "notify" function pointer in "struct
> >>>> platform_pwm_backlight_data", because I want to tune the brightness
> >>>> value before the pwm-bl sets the brightness to hardware. I don't know
> >>>> how to do that, unless we define the platform data explicitly.
> >>>
> >>> Okay, my question should have been what you need the functions for and
> >>> why you think you need them.
> >>>
> >>
> >> If I understanding you correctly, I suppose I've said that: "because I
> >> want to tune the brightness value before the pwm-bl sets the brightness
> >> to hardware".
> >
> > Why do you want to tune the brightness value? What are you trying to
> > achieve?
> >
>
> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
> color value to make the display looks bright so that we can reduce the
> backlight brightness to save power. So everytime PRISM is triggered, we
> call "backlight_update_status", then in the "notify" callback, we change
> the brightness value which pwm-bl provides by considering the PRISM
> compensations.
If you automatically call backlight_update_status() everytime PRISM
gives you new data, can't you just pass the correct value in in the
first place so that you don't have to tweak it in the .notify()
callback?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCHv5][ 2/9] video: imxfb: Introduce regulator support.
From: Denis Carikli @ 2013-10-23 8:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382516974-13371-1-git-send-email-denis@eukrea.com>
This commit is based on the following commit by Fabio Estevam:
4344429 video: mxsfb: Introduce regulator support
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
drivers/video/imxfb.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 44ee678..4bf3837 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -28,6 +28,7 @@
#include <linux/cpufreq.h>
#include <linux/clk.h>
#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
#include <linux/dma-mapping.h>
#include <linux/io.h>
#include <linux/math64.h>
@@ -145,6 +146,7 @@ struct imxfb_info {
struct clk *clk_ipg;
struct clk *clk_ahb;
struct clk *clk_per;
+ struct regulator *reg_lcd;
enum imxfb_type devtype;
bool enabled;
@@ -563,12 +565,23 @@ static void imxfb_exit_backlight(struct imxfb_info *fbi)
static void imxfb_enable_controller(struct imxfb_info *fbi)
{
+ int ret;
if (fbi->enabled)
return;
pr_debug("Enabling LCD controller\n");
+ if (fbi->reg_lcd) {
+ ret = regulator_enable(fbi->reg_lcd);
+ if (ret) {
+ dev_err(&fbi->pdev->dev,
+ "lcd regulator enable failed with error: %d\n",
+ ret);
+ return;
+ }
+ }
+
writel(fbi->screen_dma, fbi->regs + LCDC_SSA);
/* panning offset 0 (0 pixel offset) */
@@ -597,6 +610,8 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
static void imxfb_disable_controller(struct imxfb_info *fbi)
{
+ int ret;
+
if (!fbi->enabled)
return;
@@ -613,6 +628,14 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
fbi->enabled = false;
writel(0, fbi->regs + LCDC_RMCR);
+
+ if (fbi->reg_lcd) {
+ ret = regulator_disable(fbi->reg_lcd);
+ if (ret)
+ dev_err(&fbi->pdev->dev,
+ "lcd regulator disable failed with error: %d\n",
+ ret);
+ }
}
static int imxfb_blank(int blank, struct fb_info *info)
@@ -1020,6 +1043,12 @@ static int imxfb_probe(struct platform_device *pdev)
goto failed_register;
}
+ fbi->reg_lcd = devm_regulator_get(&pdev->dev, "lcd");
+ if (IS_ERR(fbi->reg_lcd)) {
+ dev_info(&pdev->dev, "No lcd regulator used.\n");
+ fbi->reg_lcd = NULL;
+ }
+
imxfb_enable_controller(fbi);
fbi->pdev = pdev;
#ifdef PWMR_BACKLIGHT_AVAILABLE
--
1.7.9.5
^ permalink raw reply related
* [PATCHv5][ 3/9] video: imxfb: Also add pwmr for the device tree.
From: Denis Carikli @ 2013-10-23 8:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382516974-13371-1-git-send-email-denis@eukrea.com>
pwmr has to be set to get the imxfb backlight work,
though pwmr was only configurable trough the platform data.
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree@vger.kernel.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
.../devicetree/bindings/video/fsl,imx-fb.txt | 3 +++
drivers/video/imxfb.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
index 46da08d..ac457ae 100644
--- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
+++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
@@ -17,6 +17,9 @@ Required nodes:
Optional properties:
- fsl,dmacr: DMA Control Register value. This is optional. By default, the
register is not modified as recommended by the datasheet.
+- fsl,pwmr: LCDC PWM Contrast Control Register value. That property is
+ optional, but defining it is necessary to get the backlight working. If that
+ property is ommited, the register is zeroed.
- fsl,lscr1: LCDC Sharp Configuration Register value.
Example:
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 4bf3837..f09372d 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -833,6 +833,8 @@ static int imxfb_init_fbinfo(struct platform_device *pdev)
of_property_read_u32(np, "fsl,dmacr", &fbi->dmacr);
+ of_property_read_u32(np, "fsl,pwmr", &fbi->pwmr);
+
/* These two function pointers could be used by some specific
* platforms. */
fbi->lcd_power = NULL;
--
1.7.9.5
^ permalink raw reply related
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Mark Zhang @ 2013-10-23 8:49 UTC (permalink / raw)
To: Thierry Reding
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20131023080002.GB7404@ulmo.nvidia.com>
On 10/23/2013 04:00 PM, Thierry Reding wrote:
> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
>>>> [...]
>>>>>>>
>>>>>>
>>>>>> Okay, I just want to set the "notify" function pointer in "struct
>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
>>>>>> how to do that, unless we define the platform data explicitly.
>>>>>
>>>>> Okay, my question should have been what you need the functions for and
>>>>> why you think you need them.
>>>>>
>>>>
>>>> If I understanding you correctly, I suppose I've said that: "because I
>>>> want to tune the brightness value before the pwm-bl sets the brightness
>>>> to hardware".
>>>
>>> Why do you want to tune the brightness value? What are you trying to
>>> achieve?
>>>
>>
>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
>> color value to make the display looks bright so that we can reduce the
>> backlight brightness to save power. So everytime PRISM is triggered, we
>> call "backlight_update_status", then in the "notify" callback, we change
>> the brightness value which pwm-bl provides by considering the PRISM
>> compensations.
>
> If you automatically call backlight_update_status() everytime PRISM
> gives you new data, can't you just pass the correct value in in the
> first place so that you don't have to tweak it in the .notify()
> callback?
OK, how to do that? -- "pass the correct value in in the first place"?
Mark
>
> Thierry
>
^ permalink raw reply
* Re: [PATCH 1/9] backlight: atmel-pwm-bl: fix reported brightness
From: Johan Hovold @ 2013-10-23 8:51 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Johan Hovold', 'Richard Purdie',
'Nicolas Ferre', linux-fbdev, linux-kernel, stable
In-Reply-To: <001901cecf8e$21764db0$6462e910$%han@samsung.com>
On Wed, Oct 23, 2013 at 10:20:59AM +0900, Jingoo Han wrote:
> On Wednesday, October 23, 2013 2:27 AM, Johan Hovold wrote:
> >
> > The driver supports 16-bit brightness values, but the value returned
> > from get_brightness was truncated to eight bits.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > ---
> > drivers/video/backlight/atmel-pwm-bl.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> > index 66885fb..8aac273 100644
> > --- a/drivers/video/backlight/atmel-pwm-bl.c
> > +++ b/drivers/video/backlight/atmel-pwm-bl.c
> > @@ -70,7 +70,7 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> > static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> > {
> > struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> > - u8 intensity;
> > + u32 intensity;
> >
> > if (pwmbl->pdata->pwm_active_low) {
> > intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> > @@ -80,7 +80,7 @@ static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> > pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> > }
> >
> > - return intensity;
> > + return (u16)intensity;
>
> However, atmel_pwm_bl_get_intensity() should return 'int',
> instead of 'u16'.
Yes, but the cast to int is implicit. Perhaps
return (intensity & 0xffff);
(or just a comment) would make it more clear why the cast is there.
> Also, pwm_channel_readl() returns 'u32'.
Yes, (and only the 16 least-significant bits are used). That and the
fact that the platform-data limits are currently unsigned long (I was
considering fixing this later) was why I preferred keeping all register
value manipulation in u32 and do a single cast at the end.
> Then, how about the following?
>
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -70,17 +70,17 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> {
> struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> - u8 intensity;
> + u16 intensity;
>
> if (pwmbl->pdata->pwm_active_low) {
> - intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> + intensity = (u16) pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> pwmbl->pdata->pwm_duty_min;
This would actually introduce a new conversion warning (unless you add
parentheses as well) as pwm_duty_min is unsigned long. Same below.
> } else {
> - intensity = pwmbl->pdata->pwm_duty_max -
> + intensity = (u16) pwmbl->pdata->pwm_duty_max -
> pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> }
>
> - return intensity;
> + return (int)intensity;
> }
However, if you feel strongly about this, I'll respin the series (a later
patch would likely no longer apply), use u16 and add casts to the two
assignments.
Thanks,
Johan
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Thierry Reding @ 2013-10-23 9:09 UTC (permalink / raw)
To: Mark Zhang
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <52678D99.8090003@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]
On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
> On 10/23/2013 04:00 PM, Thierry Reding wrote:
> > On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
> >> On 10/22/2013 08:49 PM, Thierry Reding wrote:
> >>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
> >>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
> >>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
> >>>> [...]
> >>>>>>>
> >>>>>>
> >>>>>> Okay, I just want to set the "notify" function pointer in "struct
> >>>>>> platform_pwm_backlight_data", because I want to tune the brightness
> >>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
> >>>>>> how to do that, unless we define the platform data explicitly.
> >>>>>
> >>>>> Okay, my question should have been what you need the functions for and
> >>>>> why you think you need them.
> >>>>>
> >>>>
> >>>> If I understanding you correctly, I suppose I've said that: "because I
> >>>> want to tune the brightness value before the pwm-bl sets the brightness
> >>>> to hardware".
> >>>
> >>> Why do you want to tune the brightness value? What are you trying to
> >>> achieve?
> >>>
> >>
> >> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
> >> color value to make the display looks bright so that we can reduce the
> >> backlight brightness to save power. So everytime PRISM is triggered, we
> >> call "backlight_update_status", then in the "notify" callback, we change
> >> the brightness value which pwm-bl provides by considering the PRISM
> >> compensations.
> >
> > If you automatically call backlight_update_status() everytime PRISM
> > gives you new data, can't you just pass the correct value in in the
> > first place so that you don't have to tweak it in the .notify()
> > callback?
>
> OK, how to do that? -- "pass the correct value in in the first place"?
Well, if you call backlight_update_status(), then you can pass in a
brightness value, right? You usually do that by setting the backlight's
props.brightness field.
So when PRISM gives you new data, you could just read out the current
brightness, compute the new one based on the current one and the PRISM
data, set the props.brightness field to that value and then call
backlight_update_status().
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/9] backlight: atmel-pwm-bl: fix reported brightness
From: Jingoo Han @ 2013-10-23 9:35 UTC (permalink / raw)
To: 'Johan Hovold'
Cc: 'Richard Purdie', 'Nicolas Ferre', linux-fbdev,
linux-kernel, stable, 'Jingoo Han'
In-Reply-To: <20131023085128.GA11025@localhost>
On Wednesday, October 23, 2013 5:51 PM, Johan Hovold wrote:
> On Wed, Oct 23, 2013 at 10:20:59AM +0900, Jingoo Han wrote:
> > On Wednesday, October 23, 2013 2:27 AM, Johan Hovold wrote:
> > >
> > > The driver supports 16-bit brightness values, but the value returned
> > > from get_brightness was truncated to eight bits.
> > >
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > > ---
> > > drivers/video/backlight/atmel-pwm-bl.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> > > index 66885fb..8aac273 100644
> > > --- a/drivers/video/backlight/atmel-pwm-bl.c
> > > +++ b/drivers/video/backlight/atmel-pwm-bl.c
> > > @@ -70,7 +70,7 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> > > static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> > > {
> > > struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> > > - u8 intensity;
> > > + u32 intensity;
> > >
> > > if (pwmbl->pdata->pwm_active_low) {
> > > intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> > > @@ -80,7 +80,7 @@ static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> > > pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> > > }
> > >
> > > - return intensity;
> > > + return (u16)intensity;
> >
> > However, atmel_pwm_bl_get_intensity() should return 'int',
> > instead of 'u16'.
>
> Yes, but the cast to int is implicit. Perhaps
>
> return (intensity & 0xffff);
>
> (or just a comment) would make it more clear why the cast is there.
>
> > Also, pwm_channel_readl() returns 'u32'.
>
> Yes, (and only the 16 least-significant bits are used). That and the
> fact that the platform-data limits are currently unsigned long (I was
> considering fixing this later) was why I preferred keeping all register
> value manipulation in u32 and do a single cast at the end.
>
> > Then, how about the following?
> >
> > --- a/drivers/video/backlight/atmel-pwm-bl.c
> > +++ b/drivers/video/backlight/atmel-pwm-bl.c
> > @@ -70,17 +70,17 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> > static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> > {
> > struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> > - u8 intensity;
> > + u16 intensity;
> >
> > if (pwmbl->pdata->pwm_active_low) {
> > - intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> > + intensity = (u16) pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> > pwmbl->pdata->pwm_duty_min;
>
> This would actually introduce a new conversion warning (unless you add
> parentheses as well) as pwm_duty_min is unsigned long. Same below.
>
> > } else {
> > - intensity = pwmbl->pdata->pwm_duty_max -
> > + intensity = (u16) pwmbl->pdata->pwm_duty_max -
> > pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> > }
> >
> > - return intensity;
> > + return (int)intensity;
> > }
>
> However, if you feel strongly about this, I'll respin the series (a later
> patch would likely no longer apply), use u16 and add casts to the two
> assignments.
Thank you for your kind and detailed description. :-)
OK, I have no objection on your original patch.
Would you re-send these patches with my Acked-by with CC'ing Andrew Morton,
Tomi Valkeinen? Then, these patches can be merged through mm-tree.
Best regards,
Jingoo Han
^ permalink raw reply
* [PATCH RESEND 0/9] backlight: atmel-pwm-bl: fixes and clean ups
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold
These patches fix a few issues and clean up the atmel-pwm-bl driver
somewhat.
Resend with added ack from Jingo Han.
Johan
Johan Hovold (9):
backlight: atmel-pwm-bl: fix reported brightness
backlight: atmel-pwm-bl: fix gpio polarity in remove
backlight: atmel-pwm-bl: fix module autoload
backlight: atmel-pwm-bl: clean up probe error handling
backlight: atmel-pwm-bl: clean up get_intensity
backlight: atmel-pwm-bl: remove unused include
backlight: atmel-pwm-bl: use gpio_is_valid
backlight: atmel-pwm-bl: refactor gpio_on handling
backlight: atmel-pwm-bl: use gpio_request_one
drivers/video/backlight/atmel-pwm-bl.c | 86 ++++++++++++++++------------------
1 file changed, 40 insertions(+), 46 deletions(-)
--
1.8.4
^ permalink raw reply
* [PATCH 1/9] backlight: atmel-pwm-bl: fix reported brightness
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold, stable
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
The driver supports 16-bit brightness values, but the value returned
from get_brightness was truncated to eight bits.
Cc: stable@vger.kernel.org
Acked-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index 66885fb..8aac273 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -70,7 +70,7 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
{
struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
- u8 intensity;
+ u32 intensity;
if (pwmbl->pdata->pwm_active_low) {
intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
@@ -80,7 +80,7 @@ static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
}
- return intensity;
+ return (u16)intensity;
}
static int atmel_pwm_bl_init_pwm(struct atmel_pwm_bl *pwmbl)
--
1.8.4
^ permalink raw reply related
* [PATCH 2/9] backlight: atmel-pwm-bl: fix gpio polarity in remove
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold, stable
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
Make sure to honour gpio polarity also at remove so that the backlight
is actually disabled on boards with active-low enable pin.
Cc: stable@vger.kernel.org
Acked-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index 8aac273..3cb0094 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -205,8 +205,10 @@ static int atmel_pwm_bl_remove(struct platform_device *pdev)
{
struct atmel_pwm_bl *pwmbl = platform_get_drvdata(pdev);
- if (pwmbl->gpio_on != -1)
- gpio_set_value(pwmbl->gpio_on, 0);
+ if (pwmbl->gpio_on != -1) {
+ gpio_set_value(pwmbl->gpio_on,
+ 0 ^ pwmbl->pdata->on_active_low);
+ }
pwm_channel_disable(&pwmbl->pwmc);
pwm_channel_free(&pwmbl->pwmc);
--
1.8.4
^ permalink raw reply related
* [PATCH 3/9] backlight: atmel-pwm-bl: fix module autoload
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
Add missing module alias which is needed for module autoloading.
Acked-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index 3cb0094..cc5a5ed 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -229,3 +229,4 @@ module_platform_driver(atmel_pwm_bl_driver);
MODULE_AUTHOR("Hans-Christian egtvedt <hans-christian.egtvedt@atmel.com>");
MODULE_DESCRIPTION("Atmel PWM backlight driver");
MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:atmel-pwm-bl");
--
1.8.4
^ permalink raw reply related
* [PATCH 4/9] backlight: atmel-pwm-bl: clean up probe error handling
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
Clean up probe error handling by checking parameters before any
allocations and removing an obsolete error label. Also remove
unnecessary reset of private gpio number.
Acked-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 31 ++++++++++++-------------------
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index cc5a5ed..52a8134 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -126,40 +126,33 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
struct atmel_pwm_bl *pwmbl;
int retval;
+ pdata = dev_get_platdata(&pdev->dev);
+ if (!pdata)
+ return -ENODEV;
+
+ if (pdata->pwm_compare_max < pdata->pwm_duty_max ||
+ pdata->pwm_duty_min > pdata->pwm_duty_max ||
+ pdata->pwm_frequency = 0)
+ return -EINVAL;
+
pwmbl = devm_kzalloc(&pdev->dev, sizeof(struct atmel_pwm_bl),
GFP_KERNEL);
if (!pwmbl)
return -ENOMEM;
pwmbl->pdev = pdev;
-
- pdata = dev_get_platdata(&pdev->dev);
- if (!pdata) {
- retval = -ENODEV;
- goto err_free_mem;
- }
-
- if (pdata->pwm_compare_max < pdata->pwm_duty_max ||
- pdata->pwm_duty_min > pdata->pwm_duty_max ||
- pdata->pwm_frequency = 0) {
- retval = -EINVAL;
- goto err_free_mem;
- }
-
pwmbl->pdata = pdata;
pwmbl->gpio_on = pdata->gpio_on;
retval = pwm_channel_alloc(pdata->pwm_channel, &pwmbl->pwmc);
if (retval)
- goto err_free_mem;
+ return retval;
if (pwmbl->gpio_on != -1) {
retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
"gpio_atmel_pwm_bl");
- if (retval) {
- pwmbl->gpio_on = -1;
+ if (retval)
goto err_free_pwm;
- }
/* Turn display off by default. */
retval = gpio_direction_output(pwmbl->gpio_on,
@@ -197,7 +190,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
err_free_pwm:
pwm_channel_free(&pwmbl->pwmc);
-err_free_mem:
+
return retval;
}
--
1.8.4
^ permalink raw reply related
* [PATCH 5/9] backlight: atmel-pwm-bl: clean up get_intensity
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
Clean up get_intensity to increase readability.
Acked-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index 52a8134..504061c 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -70,15 +70,14 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
{
struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
+ u32 cdty;
u32 intensity;
- if (pwmbl->pdata->pwm_active_low) {
- intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
- pwmbl->pdata->pwm_duty_min;
- } else {
- intensity = pwmbl->pdata->pwm_duty_max -
- pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
- }
+ cdty = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
+ if (pwmbl->pdata->pwm_active_low)
+ intensity = cdty - pwmbl->pdata->pwm_duty_min;
+ else
+ intensity = pwmbl->pdata->pwm_duty_max - cdty;
return (u16)intensity;
}
--
1.8.4
^ permalink raw reply related
* [PATCH 6/9] backlight: atmel-pwm-bl: remove unused include
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
Remove unused include of clk.h.
Acked-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index 504061c..db68cab 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -12,7 +12,6 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/fb.h>
-#include <linux/clk.h>
#include <linux/gpio.h>
#include <linux/backlight.h>
#include <linux/atmel_pwm.h>
--
1.8.4
^ permalink raw reply related
* [PATCH 7/9] backlight: atmel-pwm-bl: use gpio_is_valid
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
Use gpio_is_valid rather than open coding the more restrictive != -1
test.
Acked-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index db68cab..ffc30d2 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -48,7 +48,7 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
pwm_duty = pwmbl->pdata->pwm_duty_min;
if (!intensity) {
- if (pwmbl->gpio_on != -1) {
+ if (gpio_is_valid(pwmbl->gpio_on)) {
gpio_set_value(pwmbl->gpio_on,
0 ^ pwmbl->pdata->on_active_low);
}
@@ -57,7 +57,7 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
} else {
pwm_channel_enable(&pwmbl->pwmc);
pwm_channel_writel(&pwmbl->pwmc, PWM_CUPD, pwm_duty);
- if (pwmbl->gpio_on != -1) {
+ if (gpio_is_valid(pwmbl->gpio_on)) {
gpio_set_value(pwmbl->gpio_on,
1 ^ pwmbl->pdata->on_active_low);
}
@@ -146,7 +146,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
if (retval)
return retval;
- if (pwmbl->gpio_on != -1) {
+ if (gpio_is_valid(pwmbl->gpio_on)) {
retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
"gpio_atmel_pwm_bl");
if (retval)
@@ -196,7 +196,7 @@ static int atmel_pwm_bl_remove(struct platform_device *pdev)
{
struct atmel_pwm_bl *pwmbl = platform_get_drvdata(pdev);
- if (pwmbl->gpio_on != -1) {
+ if (gpio_is_valid(pwmbl->gpio_on)) {
gpio_set_value(pwmbl->gpio_on,
0 ^ pwmbl->pdata->on_active_low);
}
--
1.8.4
^ permalink raw reply related
* [PATCH 8/9] backlight: atmel-pwm-bl: refactor gpio_on handling
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
Add helper function to control the gpio_on signal.
Acked-by: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index ffc30d2..1277e0c 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -26,6 +26,14 @@ struct atmel_pwm_bl {
int gpio_on;
};
+static void atmel_pwm_bl_set_gpio_on(struct atmel_pwm_bl *pwmbl, int on)
+{
+ if (!gpio_is_valid(pwmbl->gpio_on))
+ return;
+
+ gpio_set_value(pwmbl->gpio_on, on ^ pwmbl->pdata->on_active_low);
+}
+
static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
{
struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
@@ -48,19 +56,13 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
pwm_duty = pwmbl->pdata->pwm_duty_min;
if (!intensity) {
- if (gpio_is_valid(pwmbl->gpio_on)) {
- gpio_set_value(pwmbl->gpio_on,
- 0 ^ pwmbl->pdata->on_active_low);
- }
+ atmel_pwm_bl_set_gpio_on(pwmbl, 0);
pwm_channel_writel(&pwmbl->pwmc, PWM_CUPD, pwm_duty);
pwm_channel_disable(&pwmbl->pwmc);
} else {
pwm_channel_enable(&pwmbl->pwmc);
pwm_channel_writel(&pwmbl->pwmc, PWM_CUPD, pwm_duty);
- if (gpio_is_valid(pwmbl->gpio_on)) {
- gpio_set_value(pwmbl->gpio_on,
- 1 ^ pwmbl->pdata->on_active_low);
- }
+ atmel_pwm_bl_set_gpio_on(pwmbl, 1);
}
return 0;
@@ -196,10 +198,7 @@ static int atmel_pwm_bl_remove(struct platform_device *pdev)
{
struct atmel_pwm_bl *pwmbl = platform_get_drvdata(pdev);
- if (gpio_is_valid(pwmbl->gpio_on)) {
- gpio_set_value(pwmbl->gpio_on,
- 0 ^ pwmbl->pdata->on_active_low);
- }
+ atmel_pwm_bl_set_gpio_on(pwmbl, 0);
pwm_channel_disable(&pwmbl->pwmc);
pwm_channel_free(&pwmbl->pwmc);
--
1.8.4
^ permalink raw reply related
* [PATCH 9/9] backlight: atmel-pwm-bl: use gpio_request_one
From: Johan Hovold @ 2013-10-23 9:55 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han
Cc: Nicolas Ferre, Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Andrew Morton, linux-fbdev, linux-kernel, Johan Hovold
In-Reply-To: <1382522143-32072-1-git-send-email-jhovold@gmail.com>
Use devm_gpio_request_one rather than requesting and setting direction
in two calls.
Acked-by: Jingoo Han <jg1.han@samsung.com>:w
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/backlight/atmel-pwm-bl.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
index 1277e0c..5ea2517 100644
--- a/drivers/video/backlight/atmel-pwm-bl.c
+++ b/drivers/video/backlight/atmel-pwm-bl.c
@@ -124,6 +124,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
const struct atmel_pwm_bl_platform_data *pdata;
struct backlight_device *bldev;
struct atmel_pwm_bl *pwmbl;
+ int flags;
int retval;
pdata = dev_get_platdata(&pdev->dev);
@@ -149,14 +150,14 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
return retval;
if (gpio_is_valid(pwmbl->gpio_on)) {
- retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
- "gpio_atmel_pwm_bl");
- if (retval)
- goto err_free_pwm;
-
/* Turn display off by default. */
- retval = gpio_direction_output(pwmbl->gpio_on,
- 0 ^ pdata->on_active_low);
+ if (pdata->on_active_low)
+ flags = GPIOF_OUT_INIT_HIGH;
+ else
+ flags = GPIOF_OUT_INIT_LOW;
+
+ retval = devm_gpio_request_one(&pdev->dev, pwmbl->gpio_on,
+ flags, "gpio_atmel_pwm_bl");
if (retval)
goto err_free_pwm;
}
--
1.8.4
^ permalink raw reply related
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Mark Zhang @ 2013-10-23 10:31 UTC (permalink / raw)
To: Thierry Reding
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20131023090925.GC11954@ulmo.nvidia.com>
On 10/23/2013 05:09 PM, Thierry Reding wrote:
> On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
>> On 10/23/2013 04:00 PM, Thierry Reding wrote:
>>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
>>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
>>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
>>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
>>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
>>>>>> [...]
>>>>>>>>>
>>>>>>>>
>>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
>>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
>>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
>>>>>>>> how to do that, unless we define the platform data explicitly.
>>>>>>>
>>>>>>> Okay, my question should have been what you need the functions for and
>>>>>>> why you think you need them.
>>>>>>>
>>>>>>
>>>>>> If I understanding you correctly, I suppose I've said that: "because I
>>>>>> want to tune the brightness value before the pwm-bl sets the brightness
>>>>>> to hardware".
>>>>>
>>>>> Why do you want to tune the brightness value? What are you trying to
>>>>> achieve?
>>>>>
>>>>
>>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
>>>> color value to make the display looks bright so that we can reduce the
>>>> backlight brightness to save power. So everytime PRISM is triggered, we
>>>> call "backlight_update_status", then in the "notify" callback, we change
>>>> the brightness value which pwm-bl provides by considering the PRISM
>>>> compensations.
>>>
>>> If you automatically call backlight_update_status() everytime PRISM
>>> gives you new data, can't you just pass the correct value in in the
>>> first place so that you don't have to tweak it in the .notify()
>>> callback?
>>
>> OK, how to do that? -- "pass the correct value in in the first place"?
>
> Well, if you call backlight_update_status(), then you can pass in a
> brightness value, right? You usually do that by setting the backlight's
> props.brightness field.
>
> So when PRISM gives you new data, you could just read out the current
> brightness, compute the new one based on the current one and the PRISM
> data, set the props.brightness field to that value and then call
> backlight_update_status().
>
The param of "backlight_update_status" is "struct backlight_device *".
So you mean after I get a pointer of correct backlight device, just set
the brightness value I want to it's "props.brightness" directly?
I think the "struct backlight_device" should be opaque(although it's
definition is in include/linux/backlight.h, I know that), so it's better
not to touch it's member directly, that's why I wanna use that "notify"
callback.
Mark
> Thierry
>
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Mark Zhang @ 2013-10-23 10:36 UTC (permalink / raw)
To: Thierry Reding
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20131023090925.GC11954@ulmo.nvidia.com>
On 10/23/2013 05:09 PM, Thierry Reding wrote:
> On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
>> On 10/23/2013 04:00 PM, Thierry Reding wrote:
>>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
>>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
>>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
>>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
>>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
>>>>>> [...]
>>>>>>>>>
>>>>>>>>
>>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
>>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
>>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
>>>>>>>> how to do that, unless we define the platform data explicitly.
>>>>>>>
>>>>>>> Okay, my question should have been what you need the functions for and
>>>>>>> why you think you need them.
>>>>>>>
>>>>>>
>>>>>> If I understanding you correctly, I suppose I've said that: "because I
>>>>>> want to tune the brightness value before the pwm-bl sets the brightness
>>>>>> to hardware".
>>>>>
>>>>> Why do you want to tune the brightness value? What are you trying to
>>>>> achieve?
>>>>>
>>>>
>>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
>>>> color value to make the display looks bright so that we can reduce the
>>>> backlight brightness to save power. So everytime PRISM is triggered, we
>>>> call "backlight_update_status", then in the "notify" callback, we change
>>>> the brightness value which pwm-bl provides by considering the PRISM
>>>> compensations.
>>>
>>> If you automatically call backlight_update_status() everytime PRISM
>>> gives you new data, can't you just pass the correct value in in the
>>> first place so that you don't have to tweak it in the .notify()
>>> callback?
>>
>> OK, how to do that? -- "pass the correct value in in the first place"?
>
> Well, if you call backlight_update_status(), then you can pass in a
> brightness value, right? You usually do that by setting the backlight's
> props.brightness field.
>
> So when PRISM gives you new data, you could just read out the current
> brightness, compute the new one based on the current one and the PRISM
> data, set the props.brightness field to that value and then call
> backlight_update_status().
>
Okay, anyway, I got the idea. Actually it's simpler. :)
I'm just curious that if we can do that in this way, why we need
"notify" anymore?
Mark
> Thierry
>
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Mark Zhang @ 2013-10-23 10:46 UTC (permalink / raw)
To: Thierry Reding
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <5267A691.8050503@gmail.com>
On 10/23/2013 06:36 PM, Mark Zhang wrote:
> On 10/23/2013 05:09 PM, Thierry Reding wrote:
>> On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
>>> On 10/23/2013 04:00 PM, Thierry Reding wrote:
>>>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
>>>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
>>>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
>>>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
>>>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
>>>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
>>>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
>>>>>>>>> how to do that, unless we define the platform data explicitly.
>>>>>>>>
>>>>>>>> Okay, my question should have been what you need the functions for and
>>>>>>>> why you think you need them.
>>>>>>>>
>>>>>>>
>>>>>>> If I understanding you correctly, I suppose I've said that: "because I
>>>>>>> want to tune the brightness value before the pwm-bl sets the brightness
>>>>>>> to hardware".
>>>>>>
>>>>>> Why do you want to tune the brightness value? What are you trying to
>>>>>> achieve?
>>>>>>
>>>>>
>>>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
>>>>> color value to make the display looks bright so that we can reduce the
>>>>> backlight brightness to save power. So everytime PRISM is triggered, we
>>>>> call "backlight_update_status", then in the "notify" callback, we change
>>>>> the brightness value which pwm-bl provides by considering the PRISM
>>>>> compensations.
>>>>
>>>> If you automatically call backlight_update_status() everytime PRISM
>>>> gives you new data, can't you just pass the correct value in in the
>>>> first place so that you don't have to tweak it in the .notify()
>>>> callback?
>>>
>>> OK, how to do that? -- "pass the correct value in in the first place"?
>>
>> Well, if you call backlight_update_status(), then you can pass in a
>> brightness value, right? You usually do that by setting the backlight's
>> props.brightness field.
>>
>> So when PRISM gives you new data, you could just read out the current
>> brightness, compute the new one based on the current one and the PRISM
>> data, set the props.brightness field to that value and then call
>> backlight_update_status().
>>
>
> Okay, anyway, I got the idea. Actually it's simpler. :)
> I'm just curious that if we can do that in this way, why we need
> "notify" anymore?
Oh, by the way, how about "check_fb" fops? Is there some kind of
substitution as well? I mean, if I wanna set "check_fb" and also want to
define the backlight device via DT, is there a way to do that?
Mark
>
> Mark
>> Thierry
>>
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Thierry Reding @ 2013-10-23 10:51 UTC (permalink / raw)
To: Mark Zhang
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <5267A691.8050503@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2995 bytes --]
On Wed, Oct 23, 2013 at 06:36:01PM +0800, Mark Zhang wrote:
> On 10/23/2013 05:09 PM, Thierry Reding wrote:
> > On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
> >> On 10/23/2013 04:00 PM, Thierry Reding wrote:
> >>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
> >>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
> >>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
> >>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
> >>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
> >>>>>> [...]
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
> >>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
> >>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
> >>>>>>>> how to do that, unless we define the platform data explicitly.
> >>>>>>>
> >>>>>>> Okay, my question should have been what you need the functions for and
> >>>>>>> why you think you need them.
> >>>>>>>
> >>>>>>
> >>>>>> If I understanding you correctly, I suppose I've said that: "because I
> >>>>>> want to tune the brightness value before the pwm-bl sets the brightness
> >>>>>> to hardware".
> >>>>>
> >>>>> Why do you want to tune the brightness value? What are you trying to
> >>>>> achieve?
> >>>>>
> >>>>
> >>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
> >>>> color value to make the display looks bright so that we can reduce the
> >>>> backlight brightness to save power. So everytime PRISM is triggered, we
> >>>> call "backlight_update_status", then in the "notify" callback, we change
> >>>> the brightness value which pwm-bl provides by considering the PRISM
> >>>> compensations.
> >>>
> >>> If you automatically call backlight_update_status() everytime PRISM
> >>> gives you new data, can't you just pass the correct value in in the
> >>> first place so that you don't have to tweak it in the .notify()
> >>> callback?
> >>
> >> OK, how to do that? -- "pass the correct value in in the first place"?
> >
> > Well, if you call backlight_update_status(), then you can pass in a
> > brightness value, right? You usually do that by setting the backlight's
> > props.brightness field.
> >
> > So when PRISM gives you new data, you could just read out the current
> > brightness, compute the new one based on the current one and the PRISM
> > data, set the props.brightness field to that value and then call
> > backlight_update_status().
> >
>
> Okay, anyway, I got the idea. Actually it's simpler. :)
> I'm just curious that if we can do that in this way, why we need
> "notify" anymore?
We don't need it any longer for DT, you've just proven that. =)
However we still need to support board files, and the .notify() callback
is the only way for board files to use board specific means to enable or
disable the backlight depending on the brightness value.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Thierry Reding @ 2013-10-23 10:54 UTC (permalink / raw)
To: Mark Zhang
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <5267A582.9070504@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3356 bytes --]
On Wed, Oct 23, 2013 at 06:31:30PM +0800, Mark Zhang wrote:
> On 10/23/2013 05:09 PM, Thierry Reding wrote:
> > On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
> >> On 10/23/2013 04:00 PM, Thierry Reding wrote:
> >>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
> >>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
> >>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
> >>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
> >>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
> >>>>>> [...]
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
> >>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
> >>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
> >>>>>>>> how to do that, unless we define the platform data explicitly.
> >>>>>>>
> >>>>>>> Okay, my question should have been what you need the functions for and
> >>>>>>> why you think you need them.
> >>>>>>>
> >>>>>>
> >>>>>> If I understanding you correctly, I suppose I've said that: "because I
> >>>>>> want to tune the brightness value before the pwm-bl sets the brightness
> >>>>>> to hardware".
> >>>>>
> >>>>> Why do you want to tune the brightness value? What are you trying to
> >>>>> achieve?
> >>>>>
> >>>>
> >>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
> >>>> color value to make the display looks bright so that we can reduce the
> >>>> backlight brightness to save power. So everytime PRISM is triggered, we
> >>>> call "backlight_update_status", then in the "notify" callback, we change
> >>>> the brightness value which pwm-bl provides by considering the PRISM
> >>>> compensations.
> >>>
> >>> If you automatically call backlight_update_status() everytime PRISM
> >>> gives you new data, can't you just pass the correct value in in the
> >>> first place so that you don't have to tweak it in the .notify()
> >>> callback?
> >>
> >> OK, how to do that? -- "pass the correct value in in the first place"?
> >
> > Well, if you call backlight_update_status(), then you can pass in a
> > brightness value, right? You usually do that by setting the backlight's
> > props.brightness field.
> >
> > So when PRISM gives you new data, you could just read out the current
> > brightness, compute the new one based on the current one and the PRISM
> > data, set the props.brightness field to that value and then call
> > backlight_update_status().
> >
>
> The param of "backlight_update_status" is "struct backlight_device *".
> So you mean after I get a pointer of correct backlight device, just set
> the brightness value I want to it's "props.brightness" directly?
Yes.
> I think the "struct backlight_device" should be opaque(although it's
> definition is in include/linux/backlight.h, I know that), so it's better
> not to touch it's member directly, that's why I wanna use that "notify"
> callback.
Well, backlight_update_status() is the only way to change the brightness
of a backlight. If nobody ever calls backlight_update_status() then the
.notify() callback will never be called either.
By the way, what method do you use to control the backlight brightness?
Do you use the sysfs interface from userspace?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Thierry Reding @ 2013-10-23 10:58 UTC (permalink / raw)
To: Mark Zhang
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <5267A8F9.9050907@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]
On Wed, Oct 23, 2013 at 06:46:17PM +0800, Mark Zhang wrote:
> On 10/23/2013 06:36 PM, Mark Zhang wrote:
> > On 10/23/2013 05:09 PM, Thierry Reding wrote:
> >> On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
> >>> On 10/23/2013 04:00 PM, Thierry Reding wrote:
> >>>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
> >>>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
> >>>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
> >>>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
> >>>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
> >>>>>>> [...]
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
> >>>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
> >>>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
> >>>>>>>>> how to do that, unless we define the platform data explicitly.
> >>>>>>>>
> >>>>>>>> Okay, my question should have been what you need the functions for and
> >>>>>>>> why you think you need them.
> >>>>>>>>
> >>>>>>>
> >>>>>>> If I understanding you correctly, I suppose I've said that: "because I
> >>>>>>> want to tune the brightness value before the pwm-bl sets the brightness
> >>>>>>> to hardware".
> >>>>>>
> >>>>>> Why do you want to tune the brightness value? What are you trying to
> >>>>>> achieve?
> >>>>>>
> >>>>>
> >>>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
> >>>>> color value to make the display looks bright so that we can reduce the
> >>>>> backlight brightness to save power. So everytime PRISM is triggered, we
> >>>>> call "backlight_update_status", then in the "notify" callback, we change
> >>>>> the brightness value which pwm-bl provides by considering the PRISM
> >>>>> compensations.
> >>>>
> >>>> If you automatically call backlight_update_status() everytime PRISM
> >>>> gives you new data, can't you just pass the correct value in in the
> >>>> first place so that you don't have to tweak it in the .notify()
> >>>> callback?
> >>>
> >>> OK, how to do that? -- "pass the correct value in in the first place"?
> >>
> >> Well, if you call backlight_update_status(), then you can pass in a
> >> brightness value, right? You usually do that by setting the backlight's
> >> props.brightness field.
> >>
> >> So when PRISM gives you new data, you could just read out the current
> >> brightness, compute the new one based on the current one and the PRISM
> >> data, set the props.brightness field to that value and then call
> >> backlight_update_status().
> >>
> >
> > Okay, anyway, I got the idea. Actually it's simpler. :)
> > I'm just curious that if we can do that in this way, why we need
> > "notify" anymore?
>
> Oh, by the way, how about "check_fb" fops? Is there some kind of
> substitution as well? I mean, if I wanna set "check_fb" and also want to
> define the backlight device via DT, is there a way to do that?
No, there's no substitution for that. .check_fb() is used to verify that
a backlight device is associated with a framebuffer device. There are
better ways to check for that with DT, although nothing has been merged
into mainline yet.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ 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