[PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Mike Dunn]

Currently the driver assumes that the values specified in the
brightness-levels device tree property increase as they are parsed from
left to right.  But boards that invert the signal between the PWM output
and the backlight will need to specify decreasing brightness-levels.
This patch removes the assumption that the last element of the array is
the maximum value, and instead searches the array for the maximum value
and uses that in the duty cycle calculation.

Signed-off-by: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
---
Changelog:
v2: 
- commit message reworded; correct line wrap used
- 'max_level' variable renamed to 'scale'
- loop counter variable type changed to unsigned int
- value held in scale changed from array index to actual maximum level
- blank lines added around loop for readability

 drivers/video/backlight/pwm_bl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1fea627..5e99b88 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -27,6 +27,7 @@ struct pwm_bl_data {
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
+	unsigned int		scale;
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -57,7 +58,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 
 		if (pb->levels) {
 			duty_cycle = pb->levels[brightness];
-			max = pb->levels[max];
+			max = pb->scale;
 		} else {
 			duty_cycle = brightness;
 		}
@@ -195,7 +196,14 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	}
 
 	if (data->levels) {
-		max = data->levels[data->max_brightness];
+		unsigned int i;
+
+		for (i = 0; i <= data->max_brightness; i++) {
+			if (data->levels[i] > pb->scale)
+				pb->scale = data->levels[i];
+		}
+
+		max = pb->scale;
 		pb->levels = data->levels;
 	} else
 		max = data->max_brightness;
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Tomi Valkeinen]

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

On 22/09/13 19:59, Mike Dunn wrote:
> Currently the driver assumes that the values specified in the
> brightness-levels device tree property increase as they are parsed from
> left to right.  But boards that invert the signal between the PWM output
> and the backlight will need to specify decreasing brightness-levels.
> This patch removes the assumption that the last element of the array is
> the maximum value, and instead searches the array for the maximum value
> and uses that in the duty cycle calculation.
> 
> Signed-off-by: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
> ---
> Changelog:
> v2: 
> - commit message reworded; correct line wrap used
> - 'max_level' variable renamed to 'scale'
> - loop counter variable type changed to unsigned int
> - value held in scale changed from array index to actual maximum level
> - blank lines added around loop for readability

As you said in a previous mail, the code is rather confusing. And, at
least to me, "scale" sounds an odd name there, especially as it's then
assigned to "max" local var. But if you and Thierry think this version
is good, I'll take it.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Thierry Reding]

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

On Thu, Sep 26, 2013 at 01:03:06PM +0300, Tomi Valkeinen wrote:
[...]
> But if you and Thierry think this version is good, I'll take it.

That sounds like you want to take it through the fbdev tree. Jingoo is
listed (along with Richard, but he hasn't been responsive to email for
years) as maintainer for the backlight subsystem. Furthermore back at
the time when I began working on the PWM subsystem, the backlight sub-
system was pretty much orphaned, and pwm-backlight was by far the
biggest user of the PWM subsystem. I adopted the driver at the time
because it needed to be updated for PWM subsystem changes.

What's the plan going forward? Given the coupling between the PWM
subsystem and the pwm-backlight driver it might be useful to keep
maintaining it as part of the PWM subsystem. On the other hand, there's
some coupling between the driver and the backlight subsystem too.

I have a couple of patches queued up for 3.13 that rework parts of the
driver, so it'd be good to know how you guys want to handle this.

Thierry

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

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Tomi Valkeinen]

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

On 26/09/13 14:51, Thierry Reding wrote:
> On Thu, Sep 26, 2013 at 01:03:06PM +0300, Tomi Valkeinen wrote:
> [...]
>> But if you and Thierry think this version is good, I'll take it.
> 
> That sounds like you want to take it through the fbdev tree. Jingoo is
> listed (along with Richard, but he hasn't been responsive to email for
> years) as maintainer for the backlight subsystem. Furthermore back at

Ah, so they are. I just thought it falls under fbdev, as it's under
drivers/video/ =).

I don't have any particular "want" to take it through fbdev tree. But I
can take it.

> the time when I began working on the PWM subsystem, the backlight sub-
> system was pretty much orphaned, and pwm-backlight was by far the
> biggest user of the PWM subsystem. I adopted the driver at the time
> because it needed to be updated for PWM subsystem changes.
> 
> What's the plan going forward? Given the coupling between the PWM
> subsystem and the pwm-backlight driver it might be useful to keep
> maintaining it as part of the PWM subsystem. On the other hand, there's
> some coupling between the driver and the backlight subsystem too.

And backlight is coupled with fbdev... Which is something I don't like.

> I have a couple of patches queued up for 3.13 that rework parts of the
> driver, so it'd be good to know how you guys want to handle this.

Well. I'm happy if somebody wants to maintain the backlight side. In
fact, I'd be happy if somebody would start restructuring it totally,
it's rather messy. The link with fbdev should be removed, and some
backlight drivers are actually panel drivers. However, perhaps Common
Display Framework is required until it can be fully cleaned.

So... For the time being, I'm fine with merging pwm-backlight via any
tree that works best. I'm presuming here that backlight framework and
fbdev (for the parts that are relevant for backlight) are not really
being changed, so there shouldn't be conflicts.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Thierry Reding]

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

On Thu, Sep 26, 2013 at 03:08:22PM +0300, Tomi Valkeinen wrote:
> On 26/09/13 14:51, Thierry Reding wrote:
> > On Thu, Sep 26, 2013 at 01:03:06PM +0300, Tomi Valkeinen wrote:
> > [...]
> >> But if you and Thierry think this version is good, I'll take it.
> > 
> > That sounds like you want to take it through the fbdev tree. Jingoo is
> > listed (along with Richard, but he hasn't been responsive to email for
> > years) as maintainer for the backlight subsystem. Furthermore back at
> 
> Ah, so they are. I just thought it falls under fbdev, as it's under
> drivers/video/ =).
> 
> I don't have any particular "want" to take it through fbdev tree. But I
> can take it.
> 
> > the time when I began working on the PWM subsystem, the backlight sub-
> > system was pretty much orphaned, and pwm-backlight was by far the
> > biggest user of the PWM subsystem. I adopted the driver at the time
> > because it needed to be updated for PWM subsystem changes.
> > 
> > What's the plan going forward? Given the coupling between the PWM
> > subsystem and the pwm-backlight driver it might be useful to keep
> > maintaining it as part of the PWM subsystem. On the other hand, there's
> > some coupling between the driver and the backlight subsystem too.
> 
> And backlight is coupled with fbdev... Which is something I don't like.
> 
> > I have a couple of patches queued up for 3.13 that rework parts of the
> > driver, so it'd be good to know how you guys want to handle this.
> 
> Well. I'm happy if somebody wants to maintain the backlight side. In
> fact, I'd be happy if somebody would start restructuring it totally,
> it's rather messy. The link with fbdev should be removed, and some
> backlight drivers are actually panel drivers. However, perhaps Common
> Display Framework is required until it can be fully cleaned.
> 
> So... For the time being, I'm fine with merging pwm-backlight via any
> tree that works best. I'm presuming here that backlight framework and
> fbdev (for the parts that are relevant for backlight) are not really
> being changed, so there shouldn't be conflicts.

In that case I'll just take it through the PWM tree as I've done for the
past year. I have some other changes planned for the PWM framework for
the near future that'll create dependencies between the PWM tree and the
pwm-backlight driver, so keeping them in one tree will make it easier to
merge them.

Longer term it probably makes sense, as you say, for someone to take
over the backlight subsystem completely and give it the love it could
really use. If Jingoo can do that, it'd be great. Perhaps it'd be a good
idea to remove Richard as maintainer since he's obviously no longer
responding to emails. Keeping him Cc'ed on all patches is just
pointless.

Thierry

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

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Jingoo Han]

On Thursday, September 26, 2013 10:00 PM, Thierry Reding wrote:
> On Thu, Sep 26, 2013 at 03:08:22PM +0300, Tomi Valkeinen wrote:
> > On 26/09/13 14:51, Thierry Reding wrote:
> > > On Thu, Sep 26, 2013 at 01:03:06PM +0300, Tomi Valkeinen wrote:
> > > [...]
> > >> But if you and Thierry think this version is good, I'll take it.
> > >
> > > That sounds like you want to take it through the fbdev tree. Jingoo is
> > > listed (along with Richard, but he hasn't been responsive to email for
> > > years) as maintainer for the backlight subsystem. Furthermore back at
> >
> > Ah, so they are. I just thought it falls under fbdev, as it's under
> > drivers/video/ =).
> >
> > I don't have any particular "want" to take it through fbdev tree. But I
> > can take it.
> >
> > > the time when I began working on the PWM subsystem, the backlight sub-
> > > system was pretty much orphaned, and pwm-backlight was by far the
> > > biggest user of the PWM subsystem. I adopted the driver at the time
> > > because it needed to be updated for PWM subsystem changes.
> > >
> > > What's the plan going forward? Given the coupling between the PWM
> > > subsystem and the pwm-backlight driver it might be useful to keep
> > > maintaining it as part of the PWM subsystem. On the other hand, there's
> > > some coupling between the driver and the backlight subsystem too.
> >
> > And backlight is coupled with fbdev... Which is something I don't like.
> >
> > > I have a couple of patches queued up for 3.13 that rework parts of the
> > > driver, so it'd be good to know how you guys want to handle this.
> >
> > Well. I'm happy if somebody wants to maintain the backlight side. In
> > fact, I'd be happy if somebody would start restructuring it totally,
> > it's rather messy. The link with fbdev should be removed, and some
> > backlight drivers are actually panel drivers. However, perhaps Common
> > Display Framework is required until it can be fully cleaned.
> >
> > So... For the time being, I'm fine with merging pwm-backlight via any
> > tree that works best. I'm presuming here that backlight framework and
> > fbdev (for the parts that are relevant for backlight) are not really
> > being changed, so there shouldn't be conflicts.
> 
> In that case I'll just take it through the PWM tree as I've done for the
> past year. I have some other changes planned for the PWM framework for
> the near future that'll create dependencies between the PWM tree and the
> pwm-backlight driver, so keeping them in one tree will make it easier to
> merge them.

Yes, I think so.
I want you to take the patches for pwm-backlight through the PWM tree,
as you have done.

> 
> Longer term it probably makes sense, as you say, for someone to take
> over the backlight subsystem completely and give it the love it could
> really use. If Jingoo can do that, it'd be great. Perhaps it'd be a good
> idea to remove Richard as maintainer since he's obviously no longer
> responding to emails. Keeping him Cc'ed on all patches is just
> pointless.

OK, I will send the patch to remove Richard as maintainer.

I will make a git tree and mailing-list for the backlight subsystem later.
However, I am not certain when it will be done. :-(

Best regards,
Jingoo Han

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Jingoo Han]

On Thursday, September 26, 2013 9:08 PM, Tomi Valkeinen wrote:
> On 26/09/13 14:51, Thierry Reding wrote:
> > On Thu, Sep 26, 2013 at 01:03:06PM +0300, Tomi Valkeinen wrote:
> > [...]
> >> But if you and Thierry think this version is good, I'll take it.
> >
> > That sounds like you want to take it through the fbdev tree. Jingoo is
> > listed (along with Richard, but he hasn't been responsive to email for
> > years) as maintainer for the backlight subsystem. Furthermore back at
> 
> Ah, so they are. I just thought it falls under fbdev, as it's under
> drivers/video/ =).
> 
> I don't have any particular "want" to take it through fbdev tree. But I
> can take it.
> 
> > the time when I began working on the PWM subsystem, the backlight sub-
> > system was pretty much orphaned, and pwm-backlight was by far the
> > biggest user of the PWM subsystem. I adopted the driver at the time
> > because it needed to be updated for PWM subsystem changes.
> >
> > What's the plan going forward? Given the coupling between the PWM
> > subsystem and the pwm-backlight driver it might be useful to keep
> > maintaining it as part of the PWM subsystem. On the other hand, there's
> > some coupling between the driver and the backlight subsystem too.
> 
> And backlight is coupled with fbdev... Which is something I don't like.

+cc Laurent Pinchart,

Yes, right. 
The backlight should be de-coupled with fbdev.
I remember that Laurent Pinchart was doing this patch.

Laurent Pinchart,
Would you let us know your plan about this? :-)

> 
> > I have a couple of patches queued up for 3.13 that rework parts of the
> > driver, so it'd be good to know how you guys want to handle this.
> 
> Well. I'm happy if somebody wants to maintain the backlight side. In
> fact, I'd be happy if somebody would start restructuring it totally,
> it's rather messy. The link with fbdev should be removed, and some
> backlight drivers are actually panel drivers. However, perhaps Common
> Display Framework is required until it can be fully cleaned.

I think that some backlight drivers can be moved to 'Common Display Framework',
after 'Common Display Framework' is merged.
But, I am not sure, when it will be completed.

Best regards,
Jingoo Han

> 
> So... For the time being, I'm fine with merging pwm-backlight via any
> tree that works best. I'm presuming here that backlight framework and
> fbdev (for the parts that are relevant for backlight) are not really
> being changed, so there shouldn't be conflicts.
> 
>  Tomi
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Laurent Pinchart]

Hi Jingoo,

On Friday 27 September 2013 12:28:21 Jingoo Han wrote:
> On Thursday, September 26, 2013 9:08 PM, Tomi Valkeinen wrote:
> > On 26/09/13 14:51, Thierry Reding wrote:
> > > On Thu, Sep 26, 2013 at 01:03:06PM +0300, Tomi Valkeinen wrote:
> > > [...]
> > > 
> > >> But if you and Thierry think this version is good, I'll take it.
> > > 
> > > That sounds like you want to take it through the fbdev tree. Jingoo is
> > > listed (along with Richard, but he hasn't been responsive to email for
> > > years) as maintainer for the backlight subsystem. Furthermore back at
> > 
> > Ah, so they are. I just thought it falls under fbdev, as it's under
> > drivers/video/ =).
> > 
> > I don't have any particular "want" to take it through fbdev tree. But I
> > can take it.
> > 
> > > the time when I began working on the PWM subsystem, the backlight sub-
> > > system was pretty much orphaned, and pwm-backlight was by far the
> > > biggest user of the PWM subsystem. I adopted the driver at the time
> > > because it needed to be updated for PWM subsystem changes.
> > > 
> > > What's the plan going forward? Given the coupling between the PWM
> > > subsystem and the pwm-backlight driver it might be useful to keep
> > > maintaining it as part of the PWM subsystem. On the other hand, there's
> > > some coupling between the driver and the backlight subsystem too.
> > 
> > And backlight is coupled with fbdev... Which is something I don't like.
> 
> +cc Laurent Pinchart,
> 
> Yes, right.
> The backlight should be de-coupled with fbdev.
> I remember that Laurent Pinchart was doing this patch.
> 
> Laurent Pinchart,
> Would you let us know your plan about this? :-)

My plans include finishing CDF first :-) I thus don't know when I'll have time 
to tackle this task. Feel free to pick it but. If you do, I would appreciate 
if you could discuss your ideas with me.

> > > I have a couple of patches queued up for 3.13 that rework parts of the
> > > driver, so it'd be good to know how you guys want to handle this.
> > 
> > Well. I'm happy if somebody wants to maintain the backlight side. In
> > fact, I'd be happy if somebody would start restructuring it totally,
> > it's rather messy. The link with fbdev should be removed, and some
> > backlight drivers are actually panel drivers. However, perhaps Common
> > Display Framework is required until it can be fully cleaned.
> 
> I think that some backlight drivers can be moved to 'Common Display
> Framework', after 'Common Display Framework' is merged.
> But, I am not sure, when it will be completed.

Hardware backlight devices don't process video streams, so they don't really 
belong to CDF, at least to the CDF that we know today. However, a hardware 
panel device that integrates a backlight would be supported by a CDF driver, 
which would create a Linux backlight device.

> > So... For the time being, I'm fine with merging pwm-backlight via any
> > tree that works best. I'm presuming here that backlight framework and
> > fbdev (for the parts that are relevant for backlight) are not really
> > being changed, so there shouldn't be conflicts.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Thierry Reding]

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

On Sun, Sep 22, 2013 at 09:59:56AM -0700, Mike Dunn wrote:
> Currently the driver assumes that the values specified in the
> brightness-levels device tree property increase as they are parsed from
> left to right.  But boards that invert the signal between the PWM output
> and the backlight will need to specify decreasing brightness-levels.
> This patch removes the assumption that the last element of the array is
> the maximum value, and instead searches the array for the maximum value
> and uses that in the duty cycle calculation.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> Changelog:
> v2: 
> - commit message reworded; correct line wrap used
> - 'max_level' variable renamed to 'scale'
> - loop counter variable type changed to unsigned int
> - value held in scale changed from array index to actual maximum level
> - blank lines added around loop for readability
> 
>  drivers/video/backlight/pwm_bl.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Hey Mike,

I've pushed a slightly different version of this patch which gets rid of
the intermediate max variable and uses the new scale field exclusively
to pass the same information around. Could you look at the patch from my
for-next branch in the PWM tree and see whether that still works for the
specific hardware that you need this for?

Thierry

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

Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels

[Mike Dunn]

On 10/18/2013 12:46 AM, Thierry Reding wrote:
> On Sun, Sep 22, 2013 at 09:59:56AM -0700, Mike Dunn wrote:
>> Currently the driver assumes that the values specified in the
>> brightness-levels device tree property increase as they are parsed from
>> left to right.  But boards that invert the signal between the PWM output
>> and the backlight will need to specify decreasing brightness-levels.
>> This patch removes the assumption that the last element of the array is
>> the maximum value, and instead searches the array for the maximum value
>> and uses that in the duty cycle calculation.
>>
>> Signed-off-by: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
>> ---
>> Changelog:
>> v2: 
>> - commit message reworded; correct line wrap used
>> - 'max_level' variable renamed to 'scale'
>> - loop counter variable type changed to unsigned int
>> - value held in scale changed from array index to actual maximum level
>> - blank lines added around loop for readability
>>
>>  drivers/video/backlight/pwm_bl.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> Hey Mike,
> 
> I've pushed a slightly different version of this patch which gets rid of
> the intermediate max variable and uses the new scale field exclusively
> to pass the same information around. Could you look at the patch from my
> for-next branch in the PWM tree and see whether that still works for the
> specific hardware that you need this for?


Yes looks good.

I also tested the current HEAD of for-next on the Palm Treo 680, including the
enable-gpios DT node property.  I will have to do more work and investigation
before I will be able to try the power-supply property, though.

Thanks,
Mike

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html