[PATCH] Patch for pwm backlight driver

[PATCH] Patch for pwm backlight driver

[Yang, Robert]

[-- Attachment #1.1: Type: text/plain, Size: 2584 bytes --]

Hi Thierry,



Attached is a patch which will fix a bug in the video/pwm backlight driver if multiple pwm chips exist. The defect could cause the backlight driver use the wrong pwm chip and fail finally and also lock up the unattended pwm. The patch will allow a deferral pwm backlight probing if the attended pwm driver is not loaded yet.



This patch is currently against a linux 3.16 kernel.

The technology in this patch is architecture-independent.

Please let me know any feedback you have on this patch or the approach used.

Thanks,


Signed-off-by: Robert Yang <ryang@hach.com>

Robert Yang | Sr. Software Engineer
P 970.6631377 ext 2626
Hach Company | www.hach.com<http://www.hach.com/> |ryang@hach.com<mailto:|email@hach.com>

Water analysis has to be right. You deserve complete solutions you can be fully confident in. Hach is your resource for expert answers, outstanding support, and reliable, easy-to-use products.

--- linux-linux-3.16/pwm_bl.c.orig              2014-08-26 14:26:02.210580989 -0600
+++ ./linux-linux-3.16/drivers/video/backlight/pwm_bl.c   2014-08-26 14:26:45.318582726 -0600
@@ -281,15 +281,9 @@ static int pwm_backlight_probe(struct pl
               pb->pwm = devm_pwm_get(&pdev->dev, NULL);
              if (IS_ERR(pb->pwm)) {
-                             dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
+                            dev_err(&pdev->dev, "unable to request PWM, trying a deferral binding later\n");
                             ret = PTR_ERR(pb->pwm);
-
-                             pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
-                             if (IS_ERR(pb->pwm)) {
-                                            dev_err(&pdev->dev, "unable to request legacy PWM\n");
-                                            ret = PTR_ERR(pb->pwm);
-                                            goto err_alloc;
-                             }
+                            goto err_alloc;
              }
               dev_dbg(&pdev->dev, "got pwm for backlight\n");

Please be advised that this email may contain confidential information. If you are not the intended recipient, please notify us by email by replying to the sender and delete this message. The sender disclaims that the content of this email constitutes an offer to enter into, or the acceptance of, any agreement; provided that the foregoing does not invalidate the binding effect of any digital or other electronic reproduction of a manual signature that is included in any attachment.

[-- Attachment #1.2: Type: text/html, Size: 9590 bytes --]

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 749 bytes --]

--- linux-linux-3.16/pwm_bl.c.orig	2014-08-26 14:26:02.210580989 -0600
+++ ./linux-linux-3.16/drivers/video/backlight/pwm_bl.c	2014-08-26 14:26:45.318582726 -0600
@@ -281,15 +281,9 @@ static int pwm_backlight_probe(struct pl
 
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pb->pwm)) {
-		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
+		dev_err(&pdev->dev, "unable to request PWM, trying a deferral binding later\n");
 		ret = PTR_ERR(pb->pwm);
-
-		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
-		if (IS_ERR(pb->pwm)) {
-			dev_err(&pdev->dev, "unable to request legacy PWM\n");
-			ret = PTR_ERR(pb->pwm);
-			goto err_alloc;
-		}
+		goto err_alloc;
 	}
 
 	dev_dbg(&pdev->dev, "got pwm for backlight\n");

Re: [PATCH] Patch for pwm backlight driver

[Thierry Reding]

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

Hi,

Thanks for the patch. A couple of general comments before I get to the
patch. When sending patches, please follow the process explained in
Documentation/SubmittingPatches. In addition, I suggest you use git to
make that process easier. Once you've committed a change and written up
a proper changelog you can use git format-patch to generate the patch
and send it out using git send-email. Also before sending it, you should
run the patch through scripts/checkpatch.pl and make sure there are no
warnings or errors. Then use scripts/get_maintainer.pl to generate a
list of people and lists to Cc. I'm on that list and so is the linux-pwm
mailing list, which is good, but you should additionally Cc the
backlight maintainers (Lee, Bryan and Jingoo). It's usually not
necessary to Cc Linus on a patch directly. It'll get reviewed and
applied to a subsystem tree which will then be sent to Linus during the
merge window along with any other patches applied to that tree.

The commit message should start with a short subject line that explains
very roughly what the commit does. It's also common to prefix it with a
short string that identifies it as belonging to a given subsystem or
driver. Looking at git log -- path/to/modified/file is often a good way
to determine the proper prefix. The subject line should be followed by a
more verbose description of the problem and how it is solved by the
patch.

Also, please wrap email at around 78 characters to increase readability.

A few more comments inline.

On Tue, Aug 26, 2014 at 09:14:07PM +0000, Yang, Robert wrote:
> Hi Thierry,

A commit message doesn't need this.

> Attached is a patch which will fix a bug in the video/pwm backlight
> driver if multiple pwm chips exist. The defect could cause the

Please use PWM in text, since it's an abbreviation.

> backlight driver use the wrong pwm chip and fail finally and also lock
> up the unattended pwm.

What does "lock up" mean?

> The patch will allow a deferral pwm backlight probing if the attended
> pwm driver is not loaded yet.

> This patch is currently against a linux 3.16 kernel.

It's often better to base patches off the latest release candidate,
which in this case is v3.17-rc1, or even the subsystem tree that feeds
into linux-next or linux-next itself. That avoids possible conflicts
with other people's patches. In this case I think 3.16 is fine since
there hasn't been any other work on this driver that I'm aware of.

> The technology in this patch is architecture-independent.

The majority of patches are architecture independent, so there's usually
no need to say this.

> Please let me know any feedback you have on this patch or the approach used.
> 
> Thanks,
> 
> 
> Signed-off-by: Robert Yang <ryang@hach.com>
> 
> Robert Yang | Sr. Software Engineer
> P 970.6631377 ext 2626
> Hach Company | www.hach.com<http://www.hach.com/> |ryang@hach.com<mailto:|email@hach.com>
> 
> Water analysis has to be right. You deserve complete solutions you can be fully confident in. Hach is your resource for expert answers, outstanding support, and reliable, easy-to-use products.

The above signature doesn't belong in a patch. Your commit message
should end after the Signed-off-by: line.

> --- linux-linux-3.16/pwm_bl.c.orig              2014-08-26 14:26:02.210580989 -0600
> +++ ./linux-linux-3.16/drivers/video/backlight/pwm_bl.c   2014-08-26 14:26:45.318582726 -0600
> @@ -281,15 +281,9 @@ static int pwm_backlight_probe(struct pl
>                pb->pwm = devm_pwm_get(&pdev->dev, NULL);
>               if (IS_ERR(pb->pwm)) {

The patch here seems whitespace damaged. This could be due to the email
system that you're using. Can you find out why that's happening and make
sure the email infrastructure doesn't do this? As it is, this patch
cannot be applied automatically and I (or someone else) would have to
manually make the changes, which is both tedious and error prone.

> -                             dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
> +                            dev_err(&pdev->dev, "unable to request PWM, trying a deferral binding later\n");
>                              ret = PTR_ERR(pb->pwm);

There are other errors for which it is legitimate to use the legacy code
path here. For the case that you're trying to fix there should probably
be a check here for -EPROBE_DEFER. Somewhat like this:

	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
	if (IS_ERR(pb->pwm)) {
		if (PTR_ERR(pb->pwm) == -EPROBE_DEFER)
			goto err_alloc;

		...
	}

Also there's no need for the "... trying deferral binding later" error
message in this case, since returning -EPROBE_DEFER will cause the
driver core to print out such a message already.

> -
> -                             pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
> -                             if (IS_ERR(pb->pwm)) {
> -                                            dev_err(&pdev->dev, "unable to request legacy PWM\n");
> -                                            ret = PTR_ERR(pb->pwm);
> -                                            goto err_alloc;
> -                             }

Like I said, this code path is still required to support old systems
that haven't been converted to use PWM lookup tables yet. There are only
very few of those left, but we cannot remove this code yet.

> +                            goto err_alloc;
>               }
>                dev_dbg(&pdev->dev, "got pwm for backlight\n");
> 
> Please be advised that this email may contain confidential information. If you are not the intended recipient, please notify us by email by replying to the sender and delete this message. The sender disclaims that the content of this email constitutes an offer to enter into, or the acceptance of, any agreement; provided that the foregoing does not invalidate the binding effect of any digital or other electronic reproduction of a manual signature that is included in any attachment.

This "disclaimer" is useless in email that goes to a public mailing list
so it should just be removed.

Thierry

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