[PATCH 0/4] pwm_backlight: Error fixes and update for DT support

[PATCH 0/4] pwm_backlight: Error fixes and update for DT support

[Peter Ujfalusi]

Hi,

This small series will:
- Fix the case when the kernel is booted in non DT mode and the board file try
to use levels array to specify the brightness levels.
- Validate the dft_brightness in all cases not only in DT mode.
- Add support in case of DT boot to be able to use the whole range of PWM.

Currently boards in legacy mode uses the 'max_brightness' value to specify the
maximum steps the PWM can be configured. When these boards move to DT support
they can no longer do this, they have to use the levels array. If we want to
have the same range in both DT and non DT case we would need to add all the
numbers from 0 to max (which can be 256) to the 'brightness-levels' array.
With the new property we can simply allow the whole range to be configured.

Regards,
Peter
---
Peter Ujfalusi (4):
  pwm_backlight: Fix PWM levels support in non DT case
  pwm_backlight: Validate dft_brightness in main probe function
  pwm_backlight: Refactor the DT parsing code
  pwm_backlight: Add support for the whole range of the PWM in DT mode

 .../bindings/video/backlight/pwm-backlight.txt     | 12 +++++-
 drivers/video/backlight/pwm_bl.c                   | 49 ++++++++++++----------
 2 files changed, 38 insertions(+), 23 deletions(-)

-- 
1.8.1.1

[PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case

[Peter Ujfalusi]

It is expected that board files would have:
static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };

static struct platform_pwm_backlight_data bl_data = {
	.levels = bl_levels,
	.max_brightness = ARRAY_SIZE(bl_levels),
	.dft_brightness = 4,
	.pwm_period_ns = 7812500,
};

In this case the max_brightness would be out of range in the levels array.
Decrement the received max_brightness in every case (DT or non DT) when the
levels has been provided.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/video/backlight/pwm_bl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f0d6854 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -142,7 +142,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		}
 
 		data->dft_brightness = value;
-		data->max_brightness--;
 	}
 
 	/*
@@ -202,6 +201,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	}
 
 	if (data->levels) {
+		data->max_brightness--;
 		max = data->levels[data->max_brightness];
 		pb->levels = data->levels;
 	} else
-- 
1.8.1.1

[PATCH 2/4] pwm_backlight: Validate dft_brightness in main probe function

[Peter Ujfalusi]

Move the dft_brightness validity check from the DT parsing function to the
main probe. In this way we can validate it in case we are booting with or
without DT.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/video/backlight/pwm_bl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index f0d6854..2a81c24 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -135,12 +135,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		if (ret < 0)
 			return ret;
 
-		if (value >= data->max_brightness) {
-			dev_warn(dev, "invalid default brightness level: %u, using %u\n",
-				 value, data->max_brightness - 1);
-			value = data->max_brightness - 1;
-		}
-
 		data->dft_brightness = value;
 	}
 
@@ -249,6 +243,12 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	if (data->dft_brightness > data->max_brightness) {
+		dev_warn(&pdev->dev,
+			 "invalid default brightness level: %u, using %u\n",
+			 data->dft_brightness, data->max_brightness);
+		data->dft_brightness = data->max_brightness;
+	}
 	bl->props.brightness = data->dft_brightness;
 	backlight_update_status(bl);
 
-- 
1.8.1.1

[PATCH 3/4] pwm_backlight: Refactor the DT parsing code

[Peter Ujfalusi]

In preparation to support the whole range of the PWM device in a similar way
it is possible when we boot in legacy mode (non DT mode).

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/video/backlight/pwm_bl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 2a81c24..df2d115 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -100,7 +100,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
 {
 	struct device_node *node = dev->of_node;
 	struct property *prop;
-	int length;
+	int num_levels = 0;
 	u32 value;
 	int ret;
 
@@ -110,26 +110,27 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	memset(data, 0, sizeof(*data));
 
 	/* determine the number of brightness levels */
-	prop = of_find_property(node, "brightness-levels", &length);
+	prop = of_find_property(node, "brightness-levels", &num_levels);
 	if (!prop)
 		return -EINVAL;
 
-	data->max_brightness = length / sizeof(u32);
+	num_levels /= sizeof(u32);
 
 	/* read brightness levels from DT property */
-	if (data->max_brightness > 0) {
-		size_t size = sizeof(*data->levels) * data->max_brightness;
+	if (num_levels > 0) {
+		size_t size = sizeof(*data->levels) * num_levels;
 
 		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
 		if (!data->levels)
 			return -ENOMEM;
 
 		ret = of_property_read_u32_array(node, "brightness-levels",
-						 data->levels,
-						 data->max_brightness);
+						 data->levels, num_levels);
 		if (ret < 0)
 			return ret;
 
+		data->max_brightness = num_levels;
+
 		ret = of_property_read_u32(node, "default-brightness-level",
 					   &value);
 		if (ret < 0)
-- 
1.8.1.1

[PATCH 4/4] pwm_backlight: Add support for the whole range of the PWM in DT mode

[Peter Ujfalusi]

When booting with DT make it possible to use the whole range of the PWM when
controlling the backlight in a same way it is possible when the kernel is
booted in non DT mode.
A new property "max-brightness-level" can be used to specify the maximum
value the PWM can handle (time slots).
DTS files can use either the "brightness-levels" or the "max-brightness-level"
to configure the PWM.
In case both of these properties exist the driver will prefer the
"brightness-levels" over the "max-brightness-level".

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 .../bindings/video/backlight/pwm-backlight.txt     | 12 +++++++++--
 drivers/video/backlight/pwm_bl.c                   | 24 ++++++++++++++--------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..517924b 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -3,13 +3,21 @@ pwm-backlight bindings
 Required properties:
   - compatible: "pwm-backlight"
   - pwms: OF device-tree PWM specification (see PWM binding[0])
+
+  Brightness range can be configured with either "brightness-levels" or with
+  "max-brightness-level".
   - 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
       from these values. 0 means a 0% duty cycle (darkest/off), while the
       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)
+  - max-brightness-level: The maximum brightness level the PWM supports. When
+      the brightness is specified using this property the whole range from 0 to
+      "max-brightness-level" will be available to configure.
+  - default-brightness-level: the default brightness level. With
+      "brightness-levels" it is an index into the array defined by the
+      "brightness-levels" property. When it is used with "max-brightness-level"
+      it is the value in the range from 0 to "max-brightness-level"
 
 Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index df2d115..c0e4bc7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -111,10 +111,17 @@ static int pwm_backlight_parse_dt(struct device *dev,
 
 	/* determine the number of brightness levels */
 	prop = of_find_property(node, "brightness-levels", &num_levels);
-	if (!prop)
-		return -EINVAL;
+	if (!prop) {
+		/* Levels not provided, look for the maximum property */
+		ret = of_property_read_u32(node, "max-brightness-level",
+					   &value);
+		if (ret < 0)
+			return ret;
 
-	num_levels /= sizeof(u32);
+		data->max_brightness = value;
+	} else {
+		num_levels /= sizeof(u32);
+	}
 
 	/* read brightness levels from DT property */
 	if (num_levels > 0) {
@@ -130,14 +137,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
 			return ret;
 
 		data->max_brightness = num_levels;
+	}
 
-		ret = of_property_read_u32(node, "default-brightness-level",
-					   &value);
-		if (ret < 0)
-			return ret;
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (ret < 0)
+		return ret;
 
-		data->dft_brightness = value;
-	}
+	data->dft_brightness = value;
 
 	/*
 	 * TODO: Most users of this driver use a number of GPIOs to control
-- 
1.8.1.1

Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
> It is expected that board files would have:
> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
> 
> static struct platform_pwm_backlight_data bl_data = {
> 	.levels = bl_levels,
> 	.max_brightness = ARRAY_SIZE(bl_levels),
> 	.dft_brightness = 4,
> 	.pwm_period_ns = 7812500,
> };
> 
> In this case the max_brightness would be out of range in the levels array.
> Decrement the received max_brightness in every case (DT or non DT) when the
> levels has been provided.

What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
instead?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case

[Peter Ujfalusi]

On 01/28/2013 10:01 PM, Thierry Reding wrote:
> On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
>> It is expected that board files would have:
>> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
>>
>> static struct platform_pwm_backlight_data bl_data = {
>> 	.levels = bl_levels,
>> 	.max_brightness = ARRAY_SIZE(bl_levels),
>> 	.dft_brightness = 4,
>> 	.pwm_period_ns = 7812500,
>> };
>>
>> In this case the max_brightness would be out of range in the levels array.
>> Decrement the received max_brightness in every case (DT or non DT) when the
>> levels has been provided.
> 
> What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
> instead?

There is nothing wrong with that either but IMHO it is more natural for board
files to use just ARRAY_SIZE(bl_levels). In this way the handling of
data->max_brightness among non DT and DT booted kernel is more uniform in the
driver itself.
Right now all board files are using only the .max_brightness to specify the
maximum value, I could not find any users of .levels in the kernel.

-- 
Péter

Re: [PATCH 4/4] pwm_backlight: Add support for the whole range of the PWM in DT mode

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

On Tue, Jan 22, 2013 at 02:39:56PM +0100, Peter Ujfalusi wrote:
> When booting with DT make it possible to use the whole range of the PWM when
> controlling the backlight in a same way it is possible when the kernel is
> booted in non DT mode.
> A new property "max-brightness-level" can be used to specify the maximum
> value the PWM can handle (time slots).
> DTS files can use either the "brightness-levels" or the "max-brightness-level"
> to configure the PWM.
> In case both of these properties exist the driver will prefer the
> "brightness-levels" over the "max-brightness-level".
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

I don't think this is a good idea. The brightness-levels property was
specifically introduced in order to have a more reasonable interface to
specify brightness levels.

As such, all uses of the non-DT max_brightness to be deprecated. It is
only kept for backwards compatibility with non-DT boards.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]

On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote:
> On 01/28/2013 10:01 PM, Thierry Reding wrote:
> > On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
> >> It is expected that board files would have:
> >> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
> >>
> >> static struct platform_pwm_backlight_data bl_data = {
> >> 	.levels = bl_levels,
> >> 	.max_brightness = ARRAY_SIZE(bl_levels),
> >> 	.dft_brightness = 4,
> >> 	.pwm_period_ns = 7812500,
> >> };
> >>
> >> In this case the max_brightness would be out of range in the levels array.
> >> Decrement the received max_brightness in every case (DT or non DT) when the
> >> levels has been provided.
> > 
> > What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
> > instead?
> 
> There is nothing wrong with that either but IMHO it is more natural for board
> files to use just ARRAY_SIZE(bl_levels). In this way the handling of
> data->max_brightness among non DT and DT booted kernel is more uniform in the
> driver itself.

The .max_brightness field is defined to be the maximum value that you
can set the brightness to. If you use .levels and set .max_brightness to
ARRAY_SIZE() then that's no longer true because the maximum value for
the brightness will actually be ARRAY_SIZE() - 1.

The reason why in the DT case we decrement .max_brightness is that it is
used as a temporary variable to keep the number of levels, which
corresponds to ARRAY_SIZE() in your example, and adjust it later on to
match the definition. That's an implementation detail.

Platform data content should be encoded properly without knowledge of
the internal implementation.

> Right now all board files are using only the .max_brightness to specify the
> maximum value, I could not find any users of .levels in the kernel.

Yes. But this is the legacy mode which should be considered deprecated.
The reason why the concept of brightness-levels was introduced back when
the DT bindings were created was that pretty much no hardware in
existence actually works that way. This was a topic of discussion at the
time when I first proposed these changes, see for example:

	http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case

[Peter Ujfalusi]

On 01/29/2013 11:17 AM, Thierry Reding wrote:
> On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote:
>> On 01/28/2013 10:01 PM, Thierry Reding wrote:
>>> On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
>>>> It is expected that board files would have:
>>>> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
>>>>
>>>> static struct platform_pwm_backlight_data bl_data = {
>>>> 	.levels = bl_levels,
>>>> 	.max_brightness = ARRAY_SIZE(bl_levels),
>>>> 	.dft_brightness = 4,
>>>> 	.pwm_period_ns = 7812500,
>>>> };
>>>>
>>>> In this case the max_brightness would be out of range in the levels array.
>>>> Decrement the received max_brightness in every case (DT or non DT) when the
>>>> levels has been provided.
>>>
>>> What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
>>> instead?
>>
>> There is nothing wrong with that either but IMHO it is more natural for board
>> files to use just ARRAY_SIZE(bl_levels). In this way the handling of
>> data->max_brightness among non DT and DT booted kernel is more uniform in the
>> driver itself.
> 
> The .max_brightness field is defined to be the maximum value that you
> can set the brightness to. If you use .levels and set .max_brightness to
> ARRAY_SIZE() then that's no longer true because the maximum value for
> the brightness will actually be ARRAY_SIZE() - 1.

Yes, in conjunction with .levels it would be better to have .nr_levels instead
reusing the max_brightness.

> The reason why in the DT case we decrement .max_brightness is that it is
> used as a temporary variable to keep the number of levels, which
> corresponds to ARRAY_SIZE() in your example, and adjust it later on to
> match the definition. That's an implementation detail.
> 
> Platform data content should be encoded properly without knowledge of
> the internal implementation.
> 
>> Right now all board files are using only the .max_brightness to specify the
>> maximum value, I could not find any users of .levels in the kernel.
> 
> Yes. But this is the legacy mode which should be considered deprecated.
> The reason why the concept of brightness-levels was introduced back when
> the DT bindings were created was that pretty much no hardware in
> existence actually works that way. This was a topic of discussion at the
> time when I first proposed these changes, see for example:
> 
> 	http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html

Right. Now I know the background.
However I do not agree with the conclusion. Probably it is fine in some cases
to limit the number of configurable duty cycles to have only distinct steps.
To not go too far, on my laptop I have:
# cat /sys/class/backlight/acpi_video0/max_brightness
15
# cat /sys/class/backlight/intel_backlight/max_brightness
93

In this case I would be more happier if the user space would use the
intel_backlight than the acpi_video0. I'm fine if user space decides to allow
me only 15 distinct steps on the intel_backlight but I would expect it to do
so in a way when I change the brightness in the UI it would step down or up to
the next user configurable level. Right now it uses the acpi_video0 to control
the levels which means that I have (ugly) jumps in the backlight every time I
adjust it.

In my phone and tablet all transitions between backlight levels are smooth.
This is not a case in my laptop (with exception when the backlight is set to
auto, FW controlled).

The perceived brightness change depends on the surrounding environment, you
can not say that in high level you would need less steps or you need to have
less steps in low brightness. If you in a dark room the low changes can be
observed, while the same change when you are outside in a sunny day would not
reflect the same.

Sure we could do the ramps in driver, but what are the parameters? how many
step between the two level? What is the time between the steps?

If you are about to make a product you will end up specifying all the possible
steps to provide the best user experience. So if the PWM have 127 duty cycle
you will have levels from 0, 1, 2, 3, ...., 125, 126, 127.

-- 
Péter

Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5076 bytes --]

On Tue, Jan 29, 2013 at 01:10:19PM +0100, Peter Ujfalusi wrote:
> On 01/29/2013 11:17 AM, Thierry Reding wrote:
> > On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote:
> >> On 01/28/2013 10:01 PM, Thierry Reding wrote:
> >>> On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
> >>>> It is expected that board files would have:
> >>>> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
> >>>>
> >>>> static struct platform_pwm_backlight_data bl_data = {
> >>>> 	.levels = bl_levels,
> >>>> 	.max_brightness = ARRAY_SIZE(bl_levels),
> >>>> 	.dft_brightness = 4,
> >>>> 	.pwm_period_ns = 7812500,
> >>>> };
> >>>>
> >>>> In this case the max_brightness would be out of range in the levels array.
> >>>> Decrement the received max_brightness in every case (DT or non DT) when the
> >>>> levels has been provided.
> >>>
> >>> What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
> >>> instead?
> >>
> >> There is nothing wrong with that either but IMHO it is more natural for board
> >> files to use just ARRAY_SIZE(bl_levels). In this way the handling of
> >> data->max_brightness among non DT and DT booted kernel is more uniform in the
> >> driver itself.
> > 
> > The .max_brightness field is defined to be the maximum value that you
> > can set the brightness to. If you use .levels and set .max_brightness to
> > ARRAY_SIZE() then that's no longer true because the maximum value for
> > the brightness will actually be ARRAY_SIZE() - 1.
> 
> Yes, in conjunction with .levels it would be better to have .nr_levels instead
> reusing the max_brightness.
> 
> > The reason why in the DT case we decrement .max_brightness is that it is
> > used as a temporary variable to keep the number of levels, which
> > corresponds to ARRAY_SIZE() in your example, and adjust it later on to
> > match the definition. That's an implementation detail.
> > 
> > Platform data content should be encoded properly without knowledge of
> > the internal implementation.
> > 
> >> Right now all board files are using only the .max_brightness to specify the
> >> maximum value, I could not find any users of .levels in the kernel.
> > 
> > Yes. But this is the legacy mode which should be considered deprecated.
> > The reason why the concept of brightness-levels was introduced back when
> > the DT bindings were created was that pretty much no hardware in
> > existence actually works that way. This was a topic of discussion at the
> > time when I first proposed these changes, see for example:
> > 
> > 	http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html
> 
> Right. Now I know the background.
> However I do not agree with the conclusion. Probably it is fine in some cases
> to limit the number of configurable duty cycles to have only distinct steps.
> To not go too far, on my laptop I have:
> # cat /sys/class/backlight/acpi_video0/max_brightness
> 15
> # cat /sys/class/backlight/intel_backlight/max_brightness
> 93

FWIW, I have hardware with an Intel chipset that has max_brightness
13666. That doesn't mean all of these are necessarily valid or useful.

> In this case I would be more happier if the user space would use the
> intel_backlight than the acpi_video0. I'm fine if user space decides to allow
> me only 15 distinct steps on the intel_backlight but I would expect it to do
> so in a way when I change the brightness in the UI it would step down or up to
> the next user configurable level. Right now it uses the acpi_video0 to control
> the levels which means that I have (ugly) jumps in the backlight every time I
> adjust it.
> 
> In my phone and tablet all transitions between backlight levels are smooth.
> This is not a case in my laptop (with exception when the backlight is set to
> auto, FW controlled).
> 
> The perceived brightness change depends on the surrounding environment, you
> can not say that in high level you would need less steps or you need to have
> less steps in low brightness. If you in a dark room the low changes can be
> observed, while the same change when you are outside in a sunny day would not
> reflect the same.
> 
> Sure we could do the ramps in driver, but what are the parameters? how many
> step between the two level? What is the time between the steps?
> 
> If you are about to make a product you will end up specifying all the possible
> steps to provide the best user experience. So if the PWM have 127 duty cycle
> you will have levels from 0, 1, 2, 3, ...., 125, 126, 127.

That's not true. The duty-cycle merely defines a percentage of how long
the PWM signal will be high. You can choose an arbitrary number of
subdivisions.

Since the brightness of a display isn't linearly proportional to the
duty cycle of the PWM driving it, representing that brightness by a
linear range is misleading. Furthermore some panels have regions where
the backlight isn't lit at all or where changes in the PWM duty-cycle
don't make any difference.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case

[Peter Ujfalusi]

On 01/29/2013 01:30 PM, Thierry Reding wrote:
>> Right. Now I know the background.
>> However I do not agree with the conclusion. Probably it is fine in some cases
>> to limit the number of configurable duty cycles to have only distinct steps.
>> To not go too far, on my laptop I have:
>> # cat /sys/class/backlight/acpi_video0/max_brightness
>> 15
>> # cat /sys/class/backlight/intel_backlight/max_brightness
>> 93
> 
> FWIW, I have hardware with an Intel chipset that has max_brightness
> 13666. That doesn't mean all of these are necessarily valid or useful.

For sure this range is overkill, but if you reduce it to let's say 15 would it
be better? I don't think. It is up to the userspace to decide how to use it.
User should have full control over the HW in hand. In this particular case I
would expect userspace to give you around 20 steps to change brightness and
when you change it would step between those so you will have nice, smooth
changes and not big jumps.

> That's not true. The duty-cycle merely defines a percentage of how long
> the PWM signal will be high. You can choose an arbitrary number of
> subdivisions.

Sure all HW has limitation. The HW I have either have 127 or 255 time slots.
Probably the best thing way to deal with the backlight is to have a range of 0
- 100% Nothing else.
We should have an API to PWMs so user drivers could get the duty cycle from
the HW drivers. In this way backlight would only expose percentage and the
backlight driver would get the number of time slots from the PWM core to
calculate the actual value for the selected percentage.

> Since the brightness of a display isn't linearly proportional to the
> duty cycle of the PWM driving it, representing that brightness by a
> linear range is misleading. Furthermore some panels have regions where
> the backlight isn't lit at all or where changes in the PWM duty-cycle
> don't make any difference.

and all of this also depend on the surrounding light conditions as well. If
the same device is used in low light condition you care about the lower light
ranges more compared to when the same device is used in bright environment. In
these case the user space has to be adopted to the conditions and one should
not need to recompile the kernel/dtb just because the device is used in
different environment.

-- 
Péter

Re: [PATCH 2/4] pwm_backlight: Validate dft_brightness in main probe function

[Peter Ujfalusi]

Hi Thierry,

On 01/22/2013 02:39 PM, Peter Ujfalusi wrote:
> Move the dft_brightness validity check from the DT parsing function to the
> main probe. In this way we can validate it in case we are booting with or
> without DT.

Based on the discussion the rest of the series is going to be dropped, but
what about this single patch?
I think it is still addresses a valid issue, which is that in non DT boot we
do not check if the dft_brightness is within valid range.

I can resend this patch alone since it is not going to apply on mainline as it is.

-- 
Péter

> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/video/backlight/pwm_bl.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index f0d6854..2a81c24 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -135,12 +135,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		if (value >= data->max_brightness) {
> -			dev_warn(dev, "invalid default brightness level: %u, using %u\n",
> -				 value, data->max_brightness - 1);
> -			value = data->max_brightness - 1;
> -		}
> -
>  		data->dft_brightness = value;
>  	}
>  
> @@ -249,6 +243,12 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  		goto err_alloc;
>  	}
>  
> +	if (data->dft_brightness > data->max_brightness) {
> +		dev_warn(&pdev->dev,
> +			 "invalid default brightness level: %u, using %u\n",
> +			 data->dft_brightness, data->max_brightness);
> +		data->dft_brightness = data->max_brightness;
> +	}
>  	bl->props.brightness = data->dft_brightness;
>  	backlight_update_status(bl);
>  
> 

Re: [PATCH 2/4] pwm_backlight: Validate dft_brightness in main probe function

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Thu, Jan 31, 2013 at 01:38:18PM +0100, Peter Ujfalusi wrote:
> Hi Thierry,
> 
> On 01/22/2013 02:39 PM, Peter Ujfalusi wrote:
> > Move the dft_brightness validity check from the DT parsing function to the
> > main probe. In this way we can validate it in case we are booting with or
> > without DT.
> 
> Based on the discussion the rest of the series is going to be dropped, but
> what about this single patch?
> I think it is still addresses a valid issue, which is that in non DT boot we
> do not check if the dft_brightness is within valid range.
> 
> I can resend this patch alone since it is not going to apply on mainline as it is.

No that's okay. I've gone ahead and applied in manually.

Thanks,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]