| Message ID | 1340976167-27298-1-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/29/2012 07:22 AM, Alexandre Courbot wrote: > Add support for an optional power regulator and enable/disable GPIO. > This scheme is commonly used in embedded systems. > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > - dev_dbg(&pdev->dev, "got pwm for backlight\n"); > - That seems like an unrelated change? > @@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev) > if (data->pwm_period_ns > 0) > pwm_set_period(pb->pwm, data->pwm_period_ns); > > + > + pb->power_reg = devm_regulator_get(&pdev->dev, "power"); There's an extra blank line there. > + if (IS_ERR(pb->power_reg)) > + return PTR_ERR(pb->power_reg); > + > + pb->enable_gpio = -EINVAL; > + if (data->use_enable_gpio) { > + ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio, > + GPIOF_OUT_INIT_HIGH, "backlight_enable"); > + if (ret) > + dev_warn(&pdev->dev, > + "error %d requesting control gpio\n", ret); Shouldn't that be a hard error? If the user specified that some GPIO be used, and the GPIO could not be requested, shouldn't the driver fail to initialize? > + else > + pb->enable_gpio = data->enable_gpio; Aside from that, this looks good to me. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/30/2012 01:04 AM, Stephen Warren wrote: >> - dev_dbg(&pdev->dev, "got pwm for backlight\n"); >> - > > That seems like an unrelated change? Dammit, I hoped that would have gone unnoticed. ;) >> + pb->enable_gpio = -EINVAL; >> + if (data->use_enable_gpio) { >> + ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio, >> + GPIOF_OUT_INIT_HIGH, "backlight_enable"); >> + if (ret) >> + dev_warn(&pdev->dev, >> + "error %d requesting control gpio\n", ret); > > Shouldn't that be a hard error? If the user specified that some GPIO be > used, and the GPIO could not be requested, shouldn't the driver fail to > initialize? Yes, you are right. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 29, 2012 at 10:22:47PM +0900, Alexandre Courbot wrote: > Add support for an optional power regulator and enable/disable GPIO. > This scheme is commonly used in embedded systems. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> I've added some comments in addition to those by Stephen. [...] > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 057389d..821e03e 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c [...] > @@ -141,11 +178,14 @@ 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. > - */ > + ret = of_get_named_gpio(node, "enable-gpios", 0); > + if (ret >= 0) { > + data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0); Can't you just reuse the value of ret here? [...] > @@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev) > if (data->pwm_period_ns > 0) > pwm_set_period(pb->pwm, data->pwm_period_ns); > > + > + pb->power_reg = devm_regulator_get(&pdev->dev, "power"); > + if (IS_ERR(pb->power_reg)) > + return PTR_ERR(pb->power_reg); > + > + pb->enable_gpio = -EINVAL; Perhaps initialize this to -1? Assigning standard error codes to a GPIO doesn't make much sense. [...] > diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h > index 56f4a86..5ae2cd0 100644 > --- a/include/linux/pwm_backlight.h > +++ b/include/linux/pwm_backlight.h > @@ -18,6 +18,11 @@ struct platform_pwm_backlight_data { > void (*notify_after)(struct device *dev, int brightness); > void (*exit)(struct device *dev); > int (*check_fb)(struct device *dev, struct fb_info *info); > + /* optional GPIO that enables/disables the backlight */ > + int enable_gpio; > + /* 0 (default initialization value) is a valid GPIO number. Make use of > + * control gpio explicit to avoid bad surprises. */ > + bool use_enable_gpio; It's a shame we have to add workarounds like this... Also the canonical form to write multi-line comments would be: /* * 0 (default ... * ... surprises. */ Thierry
On 07/01/2012 03:37 AM, Thierry Reding wrote:>> + ret = of_get_named_gpio(node, "enable-gpios", 0); >> + if (ret >= 0) { >> + data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0); > > Can't you just reuse the value of ret here? Yes, definitely. >> + pb->enable_gpio = -EINVAL; > > Perhaps initialize this to -1? Assigning standard error codes to a GPIO > doesn't make much sense. Documentation/gpio.txt states the following: "If you want to initialize a structure with an invalid GPIO number, use some negative number (perhaps "-EINVAL"); that will never be valid." gpio_is_valid() seems to be happy with any negative value, but -EINVAL seems to be a convention here. >> + /* optional GPIO that enables/disables the backlight */ >> + int enable_gpio; >> + /* 0 (default initialization value) is a valid GPIO number. Make use of >> + * control gpio explicit to avoid bad surprises. */ >> + bool use_enable_gpio; > > It's a shame we have to add workarounds like this... Yeah, I hate that too. :/ I see nothing better to do unfortunately. Other remarks from Stephen made me realize that this patch has two major flaws: 1) The GPIO and regulator are fixed, optional entites ; this should cover most cases but is not very flexible. 2) Some (most?) backlight specify timings between turning power on/enabling PWM/enabling backlight. Even the order of things may be different. This patch totally ignores that. So instead of having fixed "power-supply" and "enable-gpio" properties, how about having properties describing the power-on and power-off sequences which odd cells alternate between phandles to regulators/gpios/pwm and delays in microseconds before continuing the sequence. For example: power-on = <&pwm 2 5000000 10000 &backlight_reg 0 &gpio 28 0>; power-off = <&gpio 28 0 0 &backlight_reg 10000 &pwm 2 0>; Here the power-on sequence would translate to, power on the second PWM with a duty-cycle of 5ms, wait 10ms, then enable the backlight regulator and GPIO 28 without delay. Power-off is the exact opposite. The nice thing with this scheme is that you can reorder the sequence at will and support the weirdest setups. What I don't know (device tree newbie here!) is: 1) Is it legal to refer the same phandle several times in the same node? 2) Is it ok to mix phandles of different types with integer values? The DT above compiled, but can you e.g. resolve a regulator phandle in the middle of a property? 3) Can you "guess" the type of a phandle before parsing it? Here the first phandle is a GPIO, but it could as well be the regulator. Do we have means to know that in the driver code? Sorry for the long explanation, but I really wonder if doing this is possible at all. If it is, then I think that's the way to do for backlight initialization. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 02, 2012 at 12:35:12PM +0900, Alexandre Courbot wrote: > On 07/01/2012 03:37 AM, Thierry Reding wrote:>> + ret = > of_get_named_gpio(node, "enable-gpios", 0); > >> + if (ret >= 0) { > >> + data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0); > > > > Can't you just reuse the value of ret here? > > Yes, definitely. > > >> + pb->enable_gpio = -EINVAL; > > > > Perhaps initialize this to -1? Assigning standard error codes to a GPIO > > doesn't make much sense. > > Documentation/gpio.txt states the following: > > "If you want to initialize a structure with an invalid GPIO number, use > some negative number (perhaps "-EINVAL"); that will never be valid." > > gpio_is_valid() seems to be happy with any negative value, but > -EINVAL seems to be a convention here. I would have thought something like -1 would be good enough to represent an invalid GPIO, but if there's already a convention then we should follow it. > >> + /* optional GPIO that enables/disables the backlight */ > >> + int enable_gpio; > >> + /* 0 (default initialization value) is a valid GPIO number. > Make use of > >> + * control gpio explicit to avoid bad surprises. */ > >> + bool use_enable_gpio; > > > > It's a shame we have to add workarounds like this... > > Yeah, I hate that too. :/ I see nothing better to do unfortunately. > > Other remarks from Stephen made me realize that this patch has two > major flaws: > > 1) The GPIO and regulator are fixed, optional entites ; this should > cover most cases but is not very flexible. > 2) Some (most?) backlight specify timings between turning power > on/enabling PWM/enabling backlight. Even the order of things may be > different. This patch totally ignores that. > > So instead of having fixed "power-supply" and "enable-gpio" > properties, how about having properties describing the power-on and > power-off sequences which odd cells alternate between phandles to > regulators/gpios/pwm and delays in microseconds before continuing > the sequence. For example: > > power-on = <&pwm 2 5000000 > 10000 > &backlight_reg > 0 > &gpio 28 0>; > power-off = <&gpio 28 0 > 0 > &backlight_reg > 10000 > &pwm 2 0>; > > Here the power-on sequence would translate to, power on the second > PWM with a duty-cycle of 5ms, wait 10ms, then enable the backlight > regulator and GPIO 28 without delay. Power-off is the exact > opposite. The nice thing with this scheme is that you can reorder > the sequence at will and support the weirdest setups. I generally like the idea. I'm Cc'ing the devicetree-discuss mailing list, let's see what people there think about it. I've also added Mitch Bradley who already helped a lot with the initial binding. > What I don't know (device tree newbie here!) is: > 1) Is it legal to refer the same phandle several times in the same node? > 2) Is it ok to mix phandles of different types with integer values? > The DT above compiled, but can you e.g. resolve a regulator phandle > in the middle of a property? I believe these shouldn't be a problem. > 3) Can you "guess" the type of a phandle before parsing it? Here the > first phandle is a GPIO, but it could as well be the regulator. Do > we have means to know that in the driver code? That isn't possible. But you could for instance use a cell to specify the type of the following phandle. > Sorry for the long explanation, but I really wonder if doing this is > possible at all. If it is, then I think that's the way to do for > backlight initialization. OTOH we basically end up describing a code sequence in the DT. But as you mention above sometimes the hardware requires specific timing parameters so this might actually be a kind of valid sequence to describe in the DT. Thierry
On 07/02/2012 03:46 PM, Thierry Reding wrote: >> So instead of having fixed "power-supply" and "enable-gpio" >> properties, how about having properties describing the power-on and >> power-off sequences which odd cells alternate between phandles to >> regulators/gpios/pwm and delays in microseconds before continuing >> the sequence. For example: >> >> power-on = <&pwm 2 5000000 >> 10000 >> &backlight_reg >> 0 >> &gpio 28 0>; >> power-off = <&gpio 28 0 >> 0 >> &backlight_reg >> 10000 >> &pwm 2 0>; >> >> Here the power-on sequence would translate to, power on the second >> PWM with a duty-cycle of 5ms, wait 10ms, then enable the backlight >> regulator and GPIO 28 without delay. Power-off is the exact >> opposite. The nice thing with this scheme is that you can reorder >> the sequence at will and support the weirdest setups. > > I generally like the idea. I'm Cc'ing the devicetree-discuss mailing > list, let's see what people there think about it. I've also added Mitch > Bradley who already helped a lot with the initial binding. Cool, thanks. I am aware that this idea probably needs to be refined, but ideally we would end up with something as simple as this example. >> What I don't know (device tree newbie here!) is: >> 1) Is it legal to refer the same phandle several times in the same node? >> 2) Is it ok to mix phandles of different types with integer values? >> The DT above compiled, but can you e.g. resolve a regulator phandle >> in the middle of a property? > > I believe these shouldn't be a problem. Nice, I'll try to see how I can parse it then. >> 3) Can you "guess" the type of a phandle before parsing it? Here the >> first phandle is a GPIO, but it could as well be the regulator. Do >> we have means to know that in the driver code? > > That isn't possible. But you could for instance use a cell to specify > the type of the following phandle. That sounds reasonnable, although we would also need to bind values to phandle types. That would make the DT a little bit more cryptic, although nothing too bad I think. >> Sorry for the long explanation, but I really wonder if doing this is >> possible at all. If it is, then I think that's the way to do for >> backlight initialization. > > OTOH we basically end up describing a code sequence in the DT. But as > you mention above sometimes the hardware requires specific timing > parameters so this might actually be a kind of valid sequence to > describe in the DT. Yes, the power on/power off sequences are part of the board/panel specification - they are a hardware description and thus fits within the purpose of the device tree IMHO. One could argue that initialization sequences are usually coded inside drivers, but that only works when the driver has a single initialization sequence to handle. With pwm-backlight we try to handle something much more general, and so far these sequences were performed by board-specific functions. All in all, adding timings & ordering is not so different from the original patch which accepted one optional regulator and GPIO. Also, if someone knows of anything else besides PWM/GPIO/regulators that may need to be controlled by a backlight power sequence, please let me know. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alexandre, On Fri, Jun 29, 2012 at 10:22:47PM +0900, Alexandre Courbot wrote: > @@ -221,8 +263,6 @@ static int pwm_backlight_probe(struct platform_device *pdev) > } > } > > - dev_dbg(&pdev->dev, "got pwm for backlight\n"); > - > /* > * The DT case will set the pwm_period_ns field to 0 and store the > * period, parsed from the DT, in the PWM device. For the non-DT case, > @@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev) > if (data->pwm_period_ns > 0) > pwm_set_period(pb->pwm, data->pwm_period_ns); > > + > + pb->power_reg = devm_regulator_get(&pdev->dev, "power"); > + if (IS_ERR(pb->power_reg)) > + return PTR_ERR(pb->power_reg); This looses several resources allocated earlier, like the enable gpio and the pwm. This is really bad here since I have no regulator specified and devm_regulator_get returns with -EPROBE_DEFER. Next time the core tries to probe the driver the driver bails out because the gpio and the pwm is already requested. Sascha
Hi Sascha, On 07/04/2012 07:48 PM, Sascha Hauer wrote:>> + >> + pb->power_reg = devm_regulator_get(&pdev->dev, "power"); >> + if (IS_ERR(pb->power_reg)) >> + return PTR_ERR(pb->power_reg); > > This looses several resources allocated earlier, like the enable gpio > and the pwm. This is really bad here since I have no regulator specified > and devm_regulator_get returns with -EPROBE_DEFER. Next time the core > tries to probe the driver the driver bails out because the gpio and the > pwm is already requested. That's very bad indeed. I assumed that the kernel would free devm-allocated resources after probe returned -EPROBE_DEFER, so that probe could reallocate them the next time it is called. Apparently that was wrong. Do you know what would the right approach be in this case? Does the kernel preserve the device structure and its associated data between the two calls to probe? If so, I could just check whether the private data has already been constructed to know which state we are in and continue from there. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 04, 2012 at 09:26:58PM +0900, Alex Courbot wrote: > On 07/04/2012 07:48 PM, Sascha Hauer wrote:>> + > > This looses several resources allocated earlier, like the enable gpio > > and the pwm. This is really bad here since I have no regulator specified > > and devm_regulator_get returns with -EPROBE_DEFER. Next time the core > > tries to probe the driver the driver bails out because the gpio and the > > pwm is already requested. > That's very bad indeed. I assumed that the kernel would free > devm-allocated resources after probe returned -EPROBE_DEFER, so that > probe could reallocate them the next time it is called. Apparently > that was wrong. Do you know what would the right approach be in this > case? Does the kernel preserve the device structure and its > associated data between the two calls to probe? If so, I could just > check whether the private data has already been constructed to know > which state we are in and continue from there. I'd really expect the devm_ functions to behave as you expect - if they don't currently we should probably fix them so that they do otherwise we're going to be in for a lot of surprises and probe deferral becomes a lot more cumbersome to use. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 04, 2012 at 09:26:58PM +0900, Alex Courbot wrote: > Hi Sascha, > > On 07/04/2012 07:48 PM, Sascha Hauer wrote:>> + > >> + pb->power_reg = devm_regulator_get(&pdev->dev, "power"); > >> + if (IS_ERR(pb->power_reg)) > >> + return PTR_ERR(pb->power_reg); > > > > This looses several resources allocated earlier, like the enable gpio > > and the pwm. This is really bad here since I have no regulator specified > > and devm_regulator_get returns with -EPROBE_DEFER. Next time the core > > tries to probe the driver the driver bails out because the gpio and the > > pwm is already requested. > > That's very bad indeed. I assumed that the kernel would free > devm-allocated resources after probe returned -EPROBE_DEFER, It indeed does free devm allocated resources, but neither the gpio nor the pwm are devm allocated. Also please be aware that using a regulator in the pwm backlight will instantly break all existing users. That's hardly your fault though. Sascha
On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote: >> That's very bad indeed. I assumed that the kernel would free >> devm-allocated resources after probe returned -EPROBE_DEFER, > > It indeed does free devm allocated resources, but neither the gpio nor > the pwm are devm allocated. As far as I can tell the gpio is allocated through devm as well: > + ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio, > + GPIOF_OUT_INIT_HIGH, "backlight_enable"); Thus if it is not reclaimed with probe returns with -EPROBE_DEFER, then I guess something is going wrong elsewhere. You are right that the PWM should be freed by the driver thought. > Also please be aware that using a regulator in the pwm backlight will > instantly break all existing users. That's hardly your fault though. Sorry, I don't see why. Could you elaborate on this? Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 05, 2012 at 12:14:39AM +0900, Alex Courbot wrote: > On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote: > >Also please be aware that using a regulator in the pwm backlight will > >instantly break all existing users. That's hardly your fault though. > Sorry, I don't see why. Could you elaborate on this? All existing machines will start failing during probe as they won't be able to find the regulator - you should ideally make sure everyone in mainline gets an appropriate regulator set up. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 05, 2012 at 12:14:39AM +0900, Alex Courbot wrote: > On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote: > >>That's very bad indeed. I assumed that the kernel would free > >>devm-allocated resources after probe returned -EPROBE_DEFER, > > > >It indeed does free devm allocated resources, but neither the gpio nor > >the pwm are devm allocated. > > As far as I can tell the gpio is allocated through devm as well: > > > + ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio, > > + GPIOF_OUT_INIT_HIGH, "backlight_enable"); You're right. For the GPIO it's ok the way it is. Sascha
On 07/05/2012 12:24 AM, Mark Brown wrote: > On Thu, Jul 05, 2012 at 12:14:39AM +0900, Alex Courbot wrote: >> On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote: > >>> Also please be aware that using a regulator in the pwm backlight will >>> instantly break all existing users. That's hardly your fault though. > >> Sorry, I don't see why. Could you elaborate on this? > > All existing machines will start failing during probe as they won't be > able to find the regulator - you should ideally make sure everyone in > mainline gets an appropriate regulator set up. Oh, that is a mistake of mine then. Driver probe should continue if no regulator is declared (but should fail if some other error occured). I want to maintain backward compatibility with current users of the driver, so regulator/gpio specification should be optional. Thanks for all the feedback people - I will come back with a new version that addresses the points highlighted and also allows power on/off sequences to be specified in the device tree. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 05, 2012 at 11:36:48AM +0900, Alex Courbot wrote: > On 07/05/2012 12:24 AM, Mark Brown wrote: > >On Thu, Jul 05, 2012 at 12:14:39AM +0900, Alex Courbot wrote: > >>On Wed 04 Jul 2012 10:00:56 PM JST, Sascha Hauer wrote: > > > >>>Also please be aware that using a regulator in the pwm backlight will > >>>instantly break all existing users. That's hardly your fault though. > > > >>Sorry, I don't see why. Could you elaborate on this? > > > >All existing machines will start failing during probe as they won't be > >able to find the regulator - you should ideally make sure everyone in > >mainline gets an appropriate regulator set up. > > Oh, that is a mistake of mine then. Driver probe should continue if > no regulator is declared (but should fail if some other error > occured). I want to maintain backward compatibility with current > users of the driver, so regulator/gpio specification should be > optional. I think the only way doing this is to add a flag to platform_data. I don't know if that's accepted though. Sascha
On 07/05/2012 03:20 PM, Sascha Hauer wrote: >> Oh, that is a mistake of mine then. Driver probe should continue if >> no regulator is declared (but should fail if some other error >> occured). I want to maintain backward compatibility with current >> users of the driver, so regulator/gpio specification should be >> optional. > > I think the only way doing this is to add a flag to platform_data. I > don't know if that's accepted though. I thought about just checking if devm_get_regulator returned -ENODEV and happily continue if that was the case, assuming no regulator was declared. But anyway with the power sequences specification this problem becomes null, since regulators will have to be explicitly declared anyway. I might be flamed for putting a parser and interpreter into a backlight driver, but I'll take my chances. :) Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 05, 2012 at 03:25:44PM +0900, Alex Courbot wrote: > On 07/05/2012 03:20 PM, Sascha Hauer wrote: > >>Oh, that is a mistake of mine then. Driver probe should continue if > >>no regulator is declared (but should fail if some other error > >>occured). I want to maintain backward compatibility with current > >>users of the driver, so regulator/gpio specification should be > >>optional. > > > >I think the only way doing this is to add a flag to platform_data. I > >don't know if that's accepted though. > > I thought about just checking if devm_get_regulator returned -ENODEV > and happily continue if that was the case, assuming no regulator was > declared. And that's the problem. The get_regulator won't return -ENODEV. It will return -EPROBE_DEFER which tells you nothing about whether a regulator will ever be available or not. Having a flag in platform data would be fine with me, but I know other people think differently. BTW in devicetree this flag implicitely exists with the power-supply property. The regulator core could look if a power-supply property is given and - if it is given, a regulator is mandatory and the core either returns the regulator or -EPROBE_DEFER if it cannot find one. - If it is not given, there is no regulator and the core could either return a special error code or a dummy regulator. Right now the regulator core will just return -EPROBE_DEFER in both cases. This could easily be changed in the regulator core. Sascha
On 07/05/2012 03:47 PM, Sascha Hauer wrote: >> I thought about just checking if devm_get_regulator returned -ENODEV >> and happily continue if that was the case, assuming no regulator was >> declared. > > And that's the problem. The get_regulator won't return -ENODEV. It will > return -EPROBE_DEFER which tells you nothing about whether a regulator > will ever be available or not. > > Having a flag in platform data would be fine with me, but I know other > people think differently. > > BTW in devicetree this flag implicitely exists with the power-supply > property. One could actually question whether the whole regulator/gpio thing should be supported at all with platform data. The platform interface can use the function hooks in order to implement whatever behavior it wants when the light needs to be powered on and off. The reason for introducing optional regulator/gpio parameters is because the DT cannot use these. Since I have no plan to remove these function hooks, making the regulator/gpio option available in platform data might be redundant. Any thought about this? > Right now the regulator core will just return -EPROBE_DEFER in both > cases. This could easily be changed in the regulator core. Could this be because the regulator core cannot make the difference between a not-yet-available regulator and a missing one? Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 05, 2012 at 04:43:27PM +0900, Alex Courbot wrote: > On 07/05/2012 03:47 PM, Sascha Hauer wrote: > >>I thought about just checking if devm_get_regulator returned -ENODEV > >>and happily continue if that was the case, assuming no regulator was > >>declared. > > > >And that's the problem. The get_regulator won't return -ENODEV. It will > >return -EPROBE_DEFER which tells you nothing about whether a regulator > >will ever be available or not. > > > >Having a flag in platform data would be fine with me, but I know other > >people think differently. > > > >BTW in devicetree this flag implicitely exists with the power-supply > >property. > > One could actually question whether the whole regulator/gpio thing > should be supported at all with platform data. The platform > interface can use the function hooks in order to implement whatever > behavior it wants when the light needs to be powered on and off. The > reason for introducing optional regulator/gpio parameters is because > the DT cannot use these. Since I have no plan to remove these > function hooks, making the regulator/gpio option available in > platform data might be redundant. Any thought about this? I agree. Non-DT platforms have always used the callbacks to execute this kind of code. As you've said before there are situations where it isn't just about setting a GPIO or enabling a regulator but it also requires a specific timing. Representing this in the platform data would become tedious. So I think for the DT case you can parse the power-on and power-off sequences directly and execute code based on it, while in non-DT cases the init and exit callbacks should be used instead. I think it even makes sense to reuse the platform data's init and exit functions in the DT case and implement the parser/interpreter within those. > >Right now the regulator core will just return -EPROBE_DEFER in both > >cases. This could easily be changed in the regulator core. > > Could this be because the regulator core cannot make the difference > between a not-yet-available regulator and a missing one? I case where the regulator comes from a DT it should assume that it will become available at some point, so -EPROBE_DEFER is correct. However if the DT doesn't even contain the power-supply property, then EPROBE_DEFER will never work because there's no regulator to become available. Thierry
On Thu, Jul 05, 2012 at 04:43:27PM +0900, Alex Courbot wrote: > On 07/05/2012 03:47 PM, Sascha Hauer wrote: > >>I thought about just checking if devm_get_regulator returned -ENODEV > >>and happily continue if that was the case, assuming no regulator was > >>declared. > > > >And that's the problem. The get_regulator won't return -ENODEV. It will > >return -EPROBE_DEFER which tells you nothing about whether a regulator > >will ever be available or not. > > > >Having a flag in platform data would be fine with me, but I know other > >people think differently. > > > >BTW in devicetree this flag implicitely exists with the power-supply > >property. > > One could actually question whether the whole regulator/gpio thing > should be supported at all with platform data. The platform > interface can use the function hooks in order to implement whatever > behavior it wants when the light needs to be powered on and off. The > reason for introducing optional regulator/gpio parameters is because > the DT cannot use these. Since I have no plan to remove these > function hooks, making the regulator/gpio option available in > platform data might be redundant. Any thought about this? sounds good. > > >Right now the regulator core will just return -EPROBE_DEFER in both > >cases. This could easily be changed in the regulator core. > > Could this be because the regulator core cannot make the difference > between a not-yet-available regulator and a missing one? It could. In regulator_dev_lookup we have: if (node) { ... } else { /* * If we couldn't even get the node then it's * not just that the device didn't register * yet, there's no node and we'll never * succeed. */ *ret = -ENODEV; } So here the regulator core knows that there is no regulator and never will be. All that needs to be done is to make _regulator_get look at that value. There may be some side effects if we just return ERR_PTR(-ENODEV) when regulator_dev_lookup returns -ENODEV. Maybe Mark has some comments to this. Sascha
On 07/05/2012 04:57 PM, Thierry Reding wrote: > I agree. Non-DT platforms have always used the callbacks to execute this > kind of code. As you've said before there are situations where it isn't > just about setting a GPIO or enabling a regulator but it also requires a > specific timing. Representing this in the platform data would become > tedious. That will settle the whole issue then. > So I think for the DT case you can parse the power-on and power-off > sequences directly and execute code based on it, while in non-DT cases > the init and exit callbacks should be used instead. I think it even > makes sense to reuse the platform data's init and exit functions in the > DT case and implement the parser/interpreter within those. It totally makes sense indeed. > I case where the regulator comes from a DT it should assume that it will > become available at some point, so -EPROBE_DEFER is correct. However if > the DT doesn't even contain the power-supply property, then EPROBE_DEFER > will never work because there's no regulator to become available. Indeed. And as Sascha mentionned this could easily be fixed. Guess I can also submit a patch for that while I am at it. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 05, 2012 at 03:25:44PM +0900, Alex Courbot wrote: > On 07/05/2012 03:20 PM, Sascha Hauer wrote: > >I think the only way doing this is to add a flag to platform_data. I > >don't know if that's accepted though. > I thought about just checking if devm_get_regulator returned -ENODEV > and happily continue if that was the case, assuming no regulator was > declared. No, that's really not a good idea - as I keep saying if we really want to go down that line we should remove all error checking instead, it's the end result.
On Thu, Jul 05, 2012 at 04:43:27PM +0900, Alex Courbot wrote: > One could actually question whether the whole regulator/gpio thing > should be supported at all with platform data. The platform > interface can use the function hooks in order to implement whatever > behavior it wants when the light needs to be powered on and off. The > reason for introducing optional regulator/gpio parameters is because > the DT cannot use these. Since I have no plan to remove these > function hooks, making the regulator/gpio option available in > platform data might be redundant. Any thought about this? Well, no - it's also done because even if you're not using device tree (as on most of the architectures we support...) it's not good to have to cut'n'paste code everywhere. This means that we want to be able to provide things like GPIOs and regulators via data which means we have exactly the same situation as we do with device tree. > >Right now the regulator core will just return -EPROBE_DEFER in both > >cases. This could easily be changed in the regulator core. > Could this be because the regulator core cannot make the difference > between a not-yet-available regulator and a missing one? Yes.
On Thu, Jul 05, 2012 at 10:02:34AM +0200, Sascha Hauer wrote: > So here the regulator core knows that there is no regulator and never > will be. All that needs to be done is to make _regulator_get look at > that value. > There may be some side effects if we just return ERR_PTR(-ENODEV) when > regulator_dev_lookup returns -ENODEV. Maybe Mark has some comments to > this. I'm concerned about how this is going to interact with all the plans people have for dynamically loading device tree fragments during enumeration.
On 07/05/2012 02:12 AM, Alex Courbot wrote: > On 07/05/2012 04:57 PM, Thierry Reding wrote: >> I agree. Non-DT platforms have always used the callbacks to execute this >> kind of code. As you've said before there are situations where it isn't >> just about setting a GPIO or enabling a regulator but it also requires a >> specific timing. Representing this in the platform data would become >> tedious. > > That will settle the whole issue then. > >> So I think for the DT case you can parse the power-on and power-off >> sequences directly and execute code based on it, while in non-DT cases >> the init and exit callbacks should be used instead. I think it even >> makes sense to reuse the platform data's init and exit functions in the >> DT case and implement the parser/interpreter within those. > > It totally makes sense indeed. I don't agree here. It'd be best if non-DT and DT cases worked as similarly as possible. Relying on callbacks in one case and data-parsed-from-DT in the other isn't consistent with that. After all, in the DT case, you parse some data out of the DT and into some data structure. In the non-DT case, you can have that data structure passed in directly using platform data. Now, there's certainly a need to continue to support callbacks for backwards compatibility, at the very least temporarily before all clients are converted to the new model, but requiring different models rather than simply allowing it seems like a bad idea to me. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-owner@vger.kernel.org] On Behalf Of Stephen > Warren > Sent: Friday, July 06, 2012 1:04 AM > To: Alex Courbot > Cc: Thierry Reding; Sascha Hauer; Mark Brown; linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-fbdev@vger.kernel.org > Subject: Re: [PATCH] pwm-backlight: add regulator and GPIO support > > On 07/05/2012 02:12 AM, Alex Courbot wrote: > > On 07/05/2012 04:57 PM, Thierry Reding wrote: > >> I agree. Non-DT platforms have always used the callbacks to execute this > >> kind of code. As you've said before there are situations where it isn't > >> just about setting a GPIO or enabling a regulator but it also requires a > >> specific timing. Representing this in the platform data would become > >> tedious. > > > > That will settle the whole issue then. > > > >> So I think for the DT case you can parse the power-on and power-off > >> sequences directly and execute code based on it, while in non-DT cases > >> the init and exit callbacks should be used instead. I think it even > >> makes sense to reuse the platform data's init and exit functions in the > >> DT case and implement the parser/interpreter within those. > > > > It totally makes sense indeed. > > I don't agree here. It'd be best if non-DT and DT cases worked as > similarly as possible. Relying on callbacks in one case and > data-parsed-from-DT in the other isn't consistent with that. After all, > in the DT case, you parse some data out of the DT and into some data > structure. In the non-DT case, you can have that data structure passed > in directly using platform data. Now, there's certainly a need to > continue to support callbacks for backwards compatibility, at the very > least temporarily before all clients are converted to the new model, but > requiring different models rather than simply allowing it seems like a > bad idea to me. Hi Alex Courbot, I couldn't agree with Stephen Warren more. Could you support DT and non-DT case for backwards compatibility? Best regards, Jingoo Han > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2012 02:19 PM, Jingoo Han wrote: > I couldn't agree with Stephen Warren more. > Could you support DT and non-DT case for backwards compatibility? Both cases are handled in the new version I just sent. I hope all other concerns have also been addressed properly. If I forgot something please ping me. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight b/Documentation/devicetree/bindings/video/backlight/pwm-backlight index 1e4fc72..85cbb7b 100644 --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight @@ -14,6 +14,8 @@ Required properties: Optional properties: - pwm-names: a list of names for the PWM devices specified in the "pwms" property (see PWM binding[0]) + - power-supply: a reference to a regulator used to control the backlight power + - enable-gpios: a reference to a GPIO used to enable/disable the backlight [0]: Documentation/devicetree/bindings/pwm/pwm.txt @@ -25,4 +27,6 @@ Example: brightness-levels = <0 4 8 16 32 64 128 255>; default-brightness-level = <6>; + power-supply = <&backlight_reg>; + enable-gpios = <&gpio 6 0>; }; diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 057389d..821e03e 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -20,6 +20,8 @@ #include <linux/pwm.h> #include <linux/pwm_backlight.h> #include <linux/slab.h> +#include <linux/of_gpio.h> +#include <linux/regulator/consumer.h> struct pwm_bl_data { struct pwm_device *pwm; @@ -27,6 +29,9 @@ struct pwm_bl_data { unsigned int period; unsigned int lth_brightness; unsigned int *levels; + bool enabled; + struct regulator *power_reg; + int enable_gpio; int (*notify)(struct device *, int brightness); void (*notify_after)(struct device *, @@ -35,6 +40,40 @@ struct pwm_bl_data { void (*exit)(struct device *); }; +static void pwm_backlight_off(struct pwm_bl_data *pb) +{ + if (!pb->enabled) + return; + + if (gpio_is_valid(pb->enable_gpio)) + gpio_set_value_cansleep(pb->enable_gpio, 0); + + if (pb->power_reg) + regulator_disable(pb->power_reg); + + pwm_config(pb->pwm, 0, pb->period); + pwm_disable(pb->pwm); + + pb->enabled = false; +} + +static void pwm_backlight_on(struct pwm_bl_data *pb, int brightness) +{ + pwm_config(pb->pwm, brightness, pb->period); + pwm_enable(pb->pwm); + + if (pb->enabled) + return; + + if (pb->power_reg) + regulator_enable(pb->power_reg); + + if (gpio_is_valid(pb->enable_gpio)) + gpio_set_value_cansleep(pb->enable_gpio, 1); + + pb->enabled = true; +} + static int pwm_backlight_update_status(struct backlight_device *bl) { struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); @@ -51,8 +90,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) brightness = pb->notify(pb->dev, brightness); if (brightness == 0) { - pwm_config(pb->pwm, 0, pb->period); - pwm_disable(pb->pwm); + pwm_backlight_off(pb); } else { if (pb->levels) { brightness = pb->levels[brightness]; @@ -61,8 +99,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) brightness = pb->lth_brightness + (brightness * (pb->period - pb->lth_brightness) / max); - pwm_config(pb->pwm, brightness, pb->period); - pwm_enable(pb->pwm); + pwm_backlight_on(pb, brightness); } if (pb->notify_after) @@ -141,11 +178,14 @@ 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. - */ + ret = of_get_named_gpio(node, "enable-gpios", 0); + if (ret >= 0) { + data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0); + data->use_enable_gpio = true; + } else if (ret != -ENOENT) { + /* GPIO is optional, so ENOENT is not an error here */ + return ret; + } return 0; } @@ -176,7 +216,9 @@ static int pwm_backlight_probe(struct platform_device *pdev) if (!data) { ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); - if (ret < 0) { + if (ret == -EPROBE_DEFER) { + return ret; + } else if (ret < 0) { dev_err(&pdev->dev, "failed to find platform data\n"); return ret; } @@ -221,8 +263,6 @@ static int pwm_backlight_probe(struct platform_device *pdev) } } - dev_dbg(&pdev->dev, "got pwm for backlight\n"); - /* * The DT case will set the pwm_period_ns field to 0 and store the * period, parsed from the DT, in the PWM device. For the non-DT case, @@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev) if (data->pwm_period_ns > 0) pwm_set_period(pb->pwm, data->pwm_period_ns); + + pb->power_reg = devm_regulator_get(&pdev->dev, "power"); + if (IS_ERR(pb->power_reg)) + return PTR_ERR(pb->power_reg); + + pb->enable_gpio = -EINVAL; + if (data->use_enable_gpio) { + ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio, + GPIOF_OUT_INIT_HIGH, "backlight_enable"); + if (ret) + dev_warn(&pdev->dev, + "error %d requesting control gpio\n", ret); + else + pb->enable_gpio = data->enable_gpio; + } + pb->period = pwm_get_period(pb->pwm); pb->lth_brightness = data->lth_brightness * (pb->period / max); @@ -265,8 +321,7 @@ static int pwm_backlight_remove(struct platform_device *pdev) struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev); backlight_device_unregister(bl); - pwm_config(pb->pwm, 0, pb->period); - pwm_disable(pb->pwm); + pwm_backlight_off(pb); pwm_put(pb->pwm); if (pb->exit) pb->exit(&pdev->dev); @@ -281,8 +336,7 @@ static int pwm_backlight_suspend(struct device *dev) if (pb->notify) pb->notify(pb->dev, 0); - pwm_config(pb->pwm, 0, pb->period); - pwm_disable(pb->pwm); + pwm_backlight_off(pb); if (pb->notify_after) pb->notify_after(pb->dev, 0); return 0; diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h index 56f4a86..5ae2cd0 100644 --- a/include/linux/pwm_backlight.h +++ b/include/linux/pwm_backlight.h @@ -18,6 +18,11 @@ struct platform_pwm_backlight_data { void (*notify_after)(struct device *dev, int brightness); void (*exit)(struct device *dev); int (*check_fb)(struct device *dev, struct fb_info *info); + /* optional GPIO that enables/disables the backlight */ + int enable_gpio; + /* 0 (default initialization value) is a valid GPIO number. Make use of + * control gpio explicit to avoid bad surprises. */ + bool use_enable_gpio; }; #endif
Add support for an optional power regulator and enable/disable GPIO. This scheme is commonly used in embedded systems. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- .../bindings/video/backlight/pwm-backlight | 4 + drivers/video/backlight/pwm_bl.c | 86 ++++++++++++++++++---- include/linux/pwm_backlight.h | 5 ++ 3 files changed, 79 insertions(+), 16 deletions(-)