| Message ID | 1379972467-11243-9-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/23/2013 03:41 PM, Thierry Reding wrote: > Make use of the new enable_gpio field and allow it to be set from DT as > well. Now that all legacy users of platform data have been converted to > initialize this field to an invalid value, it is safe to use the field > from the driver. > diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt > Optional properties: > + - enable-gpios: a list of GPIOs to enable and disable the backlight That seems to imply that multiple GPIOs are legal. Was that intended? If not, I would suggest: - enable-gpios: contains a single GPIO specifier for the GPIO which enables/disables the backlight. See [1] for the format. > > [0]: Documentation/devicetree/bindings/pwm/pwm.txt + [1]: Documentation/devicetree/bindings/gpio/gpio.txt > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > @@ -51,12 +55,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness, > pb->lth_brightness; > > pwm_config(pb->pwm, duty_cycle, pb->period); > + > + if (gpio_is_valid(pb->enable_gpio)) { > + if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) > + gpio_set_value(pb->enable_gpio, 0); > + else > + gpio_set_value(pb->enable_gpio, 1); > + } Feel completely free to ignore this, but when the difference in two code-paths is solely in function parameters, I prefer to calculate the parameter inside the if statement, then call the function outside the conditional code, to highlight the common/different parts: int value; /* or an if statement for the next 1 line, if you don't like ?: */ value = (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) ? 0 : 1; gpio_set_value((pb->enable_gpio, value); > @@ -148,11 +168,10 @@ static int pwm_backlight_parse_dt(struct device *dev, > + data->enable_gpio = of_get_named_gpio_flags(node, "enable-gpios", 0, > + &flags); > + if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW)) > + data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW; This doesn't seem to handle deferred probe correctly; I would expect something more like: data->enable_gpio = of_get_named_gpio_flags(...); if (data->enable_gpio == -EPROBE_DEFERRED) return data->enable_gpio; if (gpio_is_valid(...)) ... return 0; I suppose it's possible that the value filters down to gpio_request_one() and this actually does work out OK, but that feels very accidental/implicit to me. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 01, 2013 at 12:39:36PM -0600, Stephen Warren wrote: > On 09/23/2013 03:41 PM, Thierry Reding wrote: > > Make use of the new enable_gpio field and allow it to be set from DT as > > well. Now that all legacy users of platform data have been converted to > > initialize this field to an invalid value, it is safe to use the field > > from the driver. > > > diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt > > > Optional properties: > > > + - enable-gpios: a list of GPIOs to enable and disable the backlight > > That seems to imply that multiple GPIOs are legal. Was that intended? If > not, I would suggest: > > - enable-gpios: contains a single GPIO specifier for the GPIO which > enables/disables the backlight. See [1] for the format. > > > > > [0]: Documentation/devicetree/bindings/pwm/pwm.txt > + [1]: Documentation/devicetree/bindings/gpio/gpio.txt Yes, that sounds better. Indeed only a single GPIO is supported. Adding more than one would require representing the timing between the two and that will take us back to where we left off with the power sequences. > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > @@ -51,12 +55,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness, > > pb->lth_brightness; > > > > pwm_config(pb->pwm, duty_cycle, pb->period); > > + > > + if (gpio_is_valid(pb->enable_gpio)) { > > + if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) > > + gpio_set_value(pb->enable_gpio, 0); > > + else > > + gpio_set_value(pb->enable_gpio, 1); > > + } > > Feel completely free to ignore this, but when the difference in two > code-paths is solely in function parameters, I prefer to calculate the > parameter inside the if statement, then call the function outside the > conditional code, to highlight the common/different parts: > > int value; > > /* or an if statement for the next 1 line, if you don't like ?: */ > value = (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) ? 0 : 1; > gpio_set_value((pb->enable_gpio, value); I actually tried that, but it looked cluttered, so I opted for the alternative. Not that this matters all that much, because beginning with 3.13 the GPIO subsystem should be able to handle the active low flag internally using the new gpiod_*() API. I plan on converting the driver to use that during the next cycle. > > @@ -148,11 +168,10 @@ static int pwm_backlight_parse_dt(struct device *dev, > > > + data->enable_gpio = of_get_named_gpio_flags(node, "enable-gpios", 0, > > + &flags); > > + if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW)) > > + data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW; > > This doesn't seem to handle deferred probe correctly; I would expect > something more like: > > data->enable_gpio = of_get_named_gpio_flags(...); > if (data->enable_gpio == -EPROBE_DEFERRED) > return data->enable_gpio; > if (gpio_is_valid(...)) > ... > return 0; > > I suppose it's possible that the value filters down to > gpio_request_one() and this actually does work out OK, but that feels > very accidental/implicit to me. Yes, I think it does indeed propagate to gpio_request_one(), but you're right, it should be caught explicitly. Thierry
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt index 1e4fc72..052eb03 100644 --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt @@ -14,6 +14,7 @@ Required properties: 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 [0]: Documentation/devicetree/bindings/pwm/pwm.txt @@ -25,4 +26,6 @@ Example: brightness-levels = <0 4 8 16 32 64 128 255>; default-brightness-level = <6>; + + enable-gpios = <&gpio 58 0>; }; diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 8a49b30..506810c 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -10,6 +10,8 @@ * published by the Free Software Foundation. */ +#include <linux/gpio.h> +#include <linux/of_gpio.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/init.h> @@ -27,6 +29,8 @@ struct pwm_bl_data { unsigned int period; unsigned int lth_brightness; unsigned int *levels; + int enable_gpio; + unsigned long enable_gpio_flags; int (*notify)(struct device *, int brightness); void (*notify_after)(struct device *, @@ -51,12 +55,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness, pb->lth_brightness; pwm_config(pb->pwm, duty_cycle, pb->period); + + if (gpio_is_valid(pb->enable_gpio)) { + if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) + gpio_set_value(pb->enable_gpio, 0); + else + gpio_set_value(pb->enable_gpio, 1); + } + pwm_enable(pb->pwm); } static void pwm_backlight_power_off(struct pwm_bl_data *pb) { pwm_disable(pb->pwm); + + if (gpio_is_valid(pb->enable_gpio)) { + if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) + gpio_set_value(pb->enable_gpio, 1); + else + gpio_set_value(pb->enable_gpio, 0); + } } static int pwm_backlight_update_status(struct backlight_device *bl) @@ -108,6 +127,7 @@ static int pwm_backlight_parse_dt(struct device *dev, struct platform_pwm_backlight_data *data) { struct device_node *node = dev->of_node; + enum of_gpio_flags flags; struct property *prop; int length; u32 value; @@ -148,11 +168,10 @@ static int pwm_backlight_parse_dt(struct device *dev, data->max_brightness--; } - /* - * TODO: Most users of this driver use a number of GPIOs to control - * backlight power. Support for specifying these needs to be - * added. - */ + data->enable_gpio = of_get_named_gpio_flags(node, "enable-gpios", 0, + &flags); + if (gpio_is_valid(data->enable_gpio) && (flags & OF_GPIO_ACTIVE_LOW)) + data->enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW; return 0; } @@ -210,12 +229,30 @@ static int pwm_backlight_probe(struct platform_device *pdev) } else max = data->max_brightness; + pb->enable_gpio = data->enable_gpio; + pb->enable_gpio_flags = data->enable_gpio_flags; pb->notify = data->notify; pb->notify_after = data->notify_after; pb->check_fb = data->check_fb; pb->exit = data->exit; pb->dev = &pdev->dev; + if (gpio_is_valid(pb->enable_gpio)) { + unsigned long flags; + + if (pb->enable_gpio_flags & PWM_BACKLIGHT_GPIO_ACTIVE_LOW) + flags = GPIOF_OUT_INIT_HIGH; + else + flags = GPIOF_OUT_INIT_LOW; + + ret = gpio_request_one(pb->enable_gpio, flags, "enable"); + if (ret < 0) { + dev_err(&pdev->dev, "failed to request GPIO#%d: %d\n", + pb->enable_gpio, ret); + goto err_alloc; + } + } + 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"); @@ -224,7 +261,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) 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_gpio; } } @@ -249,7 +286,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) if (IS_ERR(bl)) { dev_err(&pdev->dev, "failed to register backlight\n"); ret = PTR_ERR(bl); - goto err_alloc; + goto err_gpio; } if (data->dft_brightness > data->max_brightness) { @@ -265,6 +302,9 @@ static int pwm_backlight_probe(struct platform_device *pdev) platform_set_drvdata(pdev, bl); return 0; +err_gpio: + if (gpio_is_valid(pb->enable_gpio)) + gpio_free(pb->enable_gpio); err_alloc: if (data->exit) data->exit(&pdev->dev);
Make use of the new enable_gpio field and allow it to be set from DT as well. Now that all legacy users of platform data have been converted to initialize this field to an invalid value, it is safe to use the field from the driver. Signed-off-by: Thierry Reding <treding@nvidia.com> --- .../bindings/video/backlight/pwm-backlight.txt | 3 ++ drivers/video/backlight/pwm_bl.c | 54 +++++++++++++++++++--- 2 files changed, 50 insertions(+), 7 deletions(-)