subject:"\[PATCH 09\/10\] pwm\-backlight\: Use an optional power supply"

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

2013-10-02 Thread Mark Brown

On Tue, Oct 01, 2013 at 11:31:27PM +0200, Thierry Reding wrote:
 On Tue, Oct 01, 2013 at 02:59:43PM -0600, Stephen Warren wrote:

  OK, hopefully it (the regulator core) complains about the missing DT
  property though; I assume you're using regulator_get() not
  regulator_get_optional(), since the supply really is not optional.

 It doesn't always. There's a pr_warn() in _regulator_get(), but that's
 only for non-DT (since for DT, has_full_constraints is set to true in
 regulator_init_complete()). Actually that would mean that the regulator
 core will complain as long as init isn't complete. But since, like many
 other drivers, pwm-backlight could use deferred probing it's likely to
 end up being probed after init.

So, part of the thing here is that nobody ever bothers actually creating
the supplies even when the device fails to load without them - everyone
is much more keen to complain about needing to do the work than actually
provide the hookup even though it's generally pretty quick and simple to
do so.

That said we probably should still moan, I'll go and do that.


signature.asc
Description: Digital signature

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

2013-10-01 Thread Stephen Warren

On 09/23/2013 03:41 PM, Thierry Reding wrote:
 Many backlights require a power supply to work properly. This commit
 uses a power-supply regulator, if available, to power up and power down
 the panel.

I think that all backlights require a power supply, albeit the supply
may not be SW-controllable. Hence, shouldn't the regulator be mandatory
in the binding, yet the driver be defensively coded such that if one
isn't specified, the driver continues to work?

 diff --git a/drivers/video/backlight/pwm_bl.c 
 b/drivers/video/backlight/pwm_bl.c

 @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device 
 *pdev)
   }
   }
  
 + pb-power_supply = devm_regulator_get_optional(pdev-dev, power);

... so I think that should be devm_regulator_get(), since the regulator
isn't really optional.

 + if (IS_ERR(pb-power_supply)) {
 + if (PTR_ERR(pb-power_supply) != -ENODEV) {
 + ret = PTR_ERR(pb-power_supply);
 + goto err_gpio;
 + }
 +
 + pb-power_supply = NULL;

If devm_regulator_get_optional() returns an error value or a valid
value, then I don't think that this driver should transmute error values
into NULL; NULL might be a perfectly valid regulator value. Related, I
think the if (pb-power_supply) tests should be replaced with if
(!IS_ERR(pb-power_supply)) instead.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

2013-10-01 Thread Thierry Reding

On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote:
 On 09/23/2013 03:41 PM, Thierry Reding wrote:
  Many backlights require a power supply to work properly. This commit
  uses a power-supply regulator, if available, to power up and power down
  the panel.
 
 I think that all backlights require a power supply, albeit the supply
 may not be SW-controllable. Hence, shouldn't the regulator be mandatory
 in the binding, yet the driver be defensively coded such that if one
 isn't specified, the driver continues to work?

That has already changed in my local version of this patch.

  diff --git a/drivers/video/backlight/pwm_bl.c 
  b/drivers/video/backlight/pwm_bl.c
 
  @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device 
  *pdev)
  }
  }
   
  +   pb-power_supply = devm_regulator_get_optional(pdev-dev, power);
 
 ... so I think that should be devm_regulator_get(), since the regulator
 isn't really optional.
 
  +   if (IS_ERR(pb-power_supply)) {
  +   if (PTR_ERR(pb-power_supply) != -ENODEV) {
  +   ret = PTR_ERR(pb-power_supply);
  +   goto err_gpio;
  +   }
  +
  +   pb-power_supply = NULL;
 
 If devm_regulator_get_optional() returns an error value or a valid
 value, then I don't think that this driver should transmute error values
 into NULL; NULL might be a perfectly valid regulator value. Related, I
 think the if (pb-power_supply) tests should be replaced with if
 (!IS_ERR(pb-power_supply)) instead.

All of that is already done in my local tree. This actually turns out to
work rather smoothly with the new support for optional regulators. The
regulator core will give you a dummy regulator (assuming it's there
physically but hasn't been wired up in software) that's always on, so
the driver doesn't even have to special case it anymore.

Thierry


pgpMjIbcA3a4c.pgp
Description: PGP signature

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

2013-10-01 Thread Stephen Warren

On 10/01/2013 02:53 PM, Thierry Reding wrote:
 On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote:
 On 09/23/2013 03:41 PM, Thierry Reding wrote:
 Many backlights require a power supply to work properly. This
 commit uses a power-supply regulator, if available, to power up
 and power down the panel.
 
 I think that all backlights require a power supply, albeit the
 supply may not be SW-controllable. Hence, shouldn't the regulator
 be mandatory in the binding, yet the driver be defensively coded
 such that if one isn't specified, the driver continues to work?
 
 That has already changed in my local version of this patch.
 
 diff --git a/drivers/video/backlight/pwm_bl.c
 b/drivers/video/backlight/pwm_bl.c
 
 @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct
 platform_device *pdev) } }
 
 +   pb-power_supply = devm_regulator_get_optional(pdev-dev,
 power);
 
 ... so I think that should be devm_regulator_get(), since the
 regulator isn't really optional.
 
 +   if (IS_ERR(pb-power_supply)) { +   if
 (PTR_ERR(pb-power_supply) != -ENODEV) { +  ret =
 PTR_ERR(pb-power_supply); +goto err_gpio; +
 } + +
 pb-power_supply = NULL;
 
 If devm_regulator_get_optional() returns an error value or a
 valid value, then I don't think that this driver should transmute
 error values into NULL; NULL might be a perfectly valid regulator
 value. Related, I think the if (pb-power_supply) tests should be
 replaced with if (!IS_ERR(pb-power_supply)) instead.
 
 All of that is already done in my local tree. This actually turns
 out to work rather smoothly with the new support for optional
 regulators. The regulator core will give you a dummy regulator
 (assuming it's there physically but hasn't been wired up in
 software) that's always on, so the driver doesn't even have to
 special case it anymore.

OK, hopefully it (the regulator core) complains about the missing DT
property though; I assume you're using regulator_get() not
regulator_get_optional(), since the supply really is not optional.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

2013-10-01 Thread Thierry Reding

On Tue, Oct 01, 2013 at 02:59:43PM -0600, Stephen Warren wrote:
 On 10/01/2013 02:53 PM, Thierry Reding wrote:
  On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote:
  On 09/23/2013 03:41 PM, Thierry Reding wrote:
  Many backlights require a power supply to work properly. This
  commit uses a power-supply regulator, if available, to power up
  and power down the panel.
  
  I think that all backlights require a power supply, albeit the
  supply may not be SW-controllable. Hence, shouldn't the regulator
  be mandatory in the binding, yet the driver be defensively coded
  such that if one isn't specified, the driver continues to work?
  
  That has already changed in my local version of this patch.
  
  diff --git a/drivers/video/backlight/pwm_bl.c
  b/drivers/video/backlight/pwm_bl.c
  
  @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct
  platform_device *pdev) } }
  
  + pb-power_supply = devm_regulator_get_optional(pdev-dev,
  power);
  
  ... so I think that should be devm_regulator_get(), since the
  regulator isn't really optional.
  
  + if (IS_ERR(pb-power_supply)) { +   if
  (PTR_ERR(pb-power_supply) != -ENODEV) { +ret =
  PTR_ERR(pb-power_supply); +  goto err_gpio; +
  } + +
  pb-power_supply = NULL;
  
  If devm_regulator_get_optional() returns an error value or a
  valid value, then I don't think that this driver should transmute
  error values into NULL; NULL might be a perfectly valid regulator
  value. Related, I think the if (pb-power_supply) tests should be
  replaced with if (!IS_ERR(pb-power_supply)) instead.
  
  All of that is already done in my local tree. This actually turns
  out to work rather smoothly with the new support for optional
  regulators. The regulator core will give you a dummy regulator
  (assuming it's there physically but hasn't been wired up in
  software) that's always on, so the driver doesn't even have to
  special case it anymore.
 
 OK, hopefully it (the regulator core) complains about the missing DT
 property though; I assume you're using regulator_get() not
 regulator_get_optional(), since the supply really is not optional.

It doesn't always. There's a pr_warn() in _regulator_get(), but that's
only for non-DT (since for DT, has_full_constraints is set to true in
regulator_init_complete()). Actually that would mean that the regulator
core will complain as long as init isn't complete. But since, like many
other drivers, pwm-backlight could use deferred probing it's likely to
end up being probed after init.

Cc'ing Mark Brown.

Thierry


pgpqgdHYE7QEt.pgp
Description: PGP signature

[PATCH 09/10] pwm-backlight: Use an optional power supply

2013-09-23 Thread Thierry Reding

Many backlights require a power supply to work properly. This commit
uses a power-supply regulator, if available, to power up and power down
the panel.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 .../bindings/video/backlight/pwm-backlight.txt  |  2 ++
 drivers/video/backlight/pwm_bl.c| 21 +
 2 files changed, 23 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 052eb03..3898f26 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -15,6 +15,7 @@ Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
pwms property (see PWM binding[0])
   - enable-gpios: a list of GPIOs to enable and disable the backlight
+  - power-supply: regulator for supply voltage
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
@@ -28,4 +29,5 @@ Example:
default-brightness-level = 6;
 
enable-gpios = gpio 58 0;
+   power-supply = vdd_bl_reg;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 506810c..a2b3876 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -21,6 +21,7 @@
 #include linux/err.h
 #include linux/pwm.h
 #include linux/pwm_backlight.h
+#include linux/regulator/consumer.h
 #include linux/slab.h
 
 struct pwm_bl_data {
@@ -29,6 +30,7 @@ struct pwm_bl_data {
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
+   struct regulator*power_supply;
int enable_gpio;
unsigned long   enable_gpio_flags;
int (*notify)(struct device *,
@@ -56,6 +58,12 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, 
int brightness,
 
pwm_config(pb-pwm, duty_cycle, pb-period);
 
+   if (pb-power_supply) {
+   err = regulator_enable(pb-power_supply);
+   if (err  0)
+   dev_err(pb-dev, failed to enable power supply\n);
+   }
+
if (gpio_is_valid(pb-enable_gpio)) {
if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
gpio_set_value(pb-enable_gpio, 0);
@@ -76,6 +84,9 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
else
gpio_set_value(pb-enable_gpio, 0);
}
+
+   if (pb-power_supply)
+   regulator_disable(pb-power_supply);
 }
 
 static int pwm_backlight_update_status(struct backlight_device *bl)
@@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device 
*pdev)
}
}
 
+   pb-power_supply = devm_regulator_get_optional(pdev-dev, power);
+   if (IS_ERR(pb-power_supply)) {
+   if (PTR_ERR(pb-power_supply) != -ENODEV) {
+   ret = PTR_ERR(pb-power_supply);
+   goto err_gpio;
+   }
+
+   pb-power_supply = NULL;
+   }
+
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);
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

[PATCH 09/10] pwm-backlight: Use an optional power supply

6 matches

Site Navigation

Mail list logo

Footer information