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

On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote:

On 01/28/2013 10:01 PM, Thierry Reding wrote:

quoted

On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:

quoted

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