| Message ID | 1351699047-4487-1-git-send-email-panto@antoniou-consulting.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 31, 2012 at 05:57:27PM +0200, Pantelis Antoniou wrote: [...] > diff --git 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/pinctrl/consumer.h> > +#include <linux/err.h> linux/err.h is already included earlier. Thierry
On Wed, Oct 31, 2012 at 05:57:27PM +0200, Pantelis Antoniou wrote: > Enable pinctrl for pwm-backlight. > > Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> > --- > drivers/video/backlight/pwm_bl.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 0c91023..f3b6194 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/pinctrl/consumer.h> > +#include <linux/err.h> > > struct pwm_bl_data { > struct pwm_device *pwm; > @@ -180,9 +182,14 @@ static int pwm_backlight_probe(struct platform_device *pdev) > struct backlight_properties props; > struct backlight_device *bl; > struct pwm_bl_data *pb; > + struct pinctrl *pinctrl; > unsigned int max; > int ret; > > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > + if (IS_ERR(pinctrl)) > + dev_warn(&pdev->dev, "unable to select pin group\n"); > + I just saw this done in a similar way in some other driver and then remembered your patch. When I reviewed this I wasn't sure if a warning was good enough in this case. I've checked the kernel tree and it seems like a majority handled this as failure instead of a warning. I took a look at the pinctrl core and it seems like indeed if neither pinctrl is enabled or if there isn't actually a pinmux configuration for a device, then devm_pinctrl_get_select_default() will actually not return an error, so in those cases where an error is returned it should actually be handled as a fatal error. I'm Cc'ing Linus Walleij, maybe he knows what to do. Thierry > if (!data) { > ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); > if (ret < 0) { > -- > 1.7.12 > > >
On Fri, Nov 9, 2012 at 9:06 AM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Wed, Oct 31, 2012 at 05:57:27PM +0200, Pantelis Antoniou wrote: >> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); >> + if (IS_ERR(pinctrl)) >> + dev_warn(&pdev->dev, "unable to select pin group\n"); >> + > > I just saw this done in a similar way in some other driver and then > remembered your patch. When I reviewed this I wasn't sure if a warning > was good enough in this case. I've checked the kernel tree and it seems > like a majority handled this as failure instead of a warning. I took a > look at the pinctrl core and it seems like indeed if neither pinctrl is > enabled or if there isn't actually a pinmux configuration for a device, > then devm_pinctrl_get_select_default() will actually not return an > error, so in those cases where an error is returned it should actually > be handled as a fatal error. So it depends. One good reason to just error out and return the error code is if the error returned is -EPROBE_DEFER, right? Because then you're pinctrl driver is not up yet, and you need to bail out and be revisited later. On a related key we have this debate going on with some subsystem maintainers as to whether we should try do centralize boilerplate like this, the lates suggestion is: http://marc.info/?l=linux-kernel&m=135263661110528&w=2 The fun never ends... Yours, Linus Walleij -- 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 Linus, On Nov 11, 2012, at 7:26 PM, Linus Walleij wrote: > On Fri, Nov 9, 2012 at 9:06 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: >> On Wed, Oct 31, 2012 at 05:57:27PM +0200, Pantelis Antoniou wrote: > >>> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); >>> + if (IS_ERR(pinctrl)) >>> + dev_warn(&pdev->dev, "unable to select pin group\n"); >>> + >> >> I just saw this done in a similar way in some other driver and then >> remembered your patch. When I reviewed this I wasn't sure if a warning >> was good enough in this case. I've checked the kernel tree and it seems >> like a majority handled this as failure instead of a warning. I took a >> look at the pinctrl core and it seems like indeed if neither pinctrl is >> enabled or if there isn't actually a pinmux configuration for a device, >> then devm_pinctrl_get_select_default() will actually not return an >> error, so in those cases where an error is returned it should actually >> be handled as a fatal error. > > So it depends. > > One good reason to just error out and return the error code is > if the error returned is -EPROBE_DEFER, right? > > Because then you're pinctrl driver is not up yet, and you > need to bail out and be revisited later. > > On a related key we have this debate going on with some > subsystem maintainers as to whether we should try do > centralize boilerplate like this, the lates suggestion is: > http://marc.info/?l=linux-kernel&m=135263661110528&w=2 > Interesting... This is certainly much nicer than peppering all the devices with pinctrl calls. > The fun never ends... > Indeed. > Yours, > Linus Walleij Regards -- Pantelis -- 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/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 0c91023..f3b6194 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/pinctrl/consumer.h> +#include <linux/err.h> struct pwm_bl_data { struct pwm_device *pwm; @@ -180,9 +182,14 @@ static int pwm_backlight_probe(struct platform_device *pdev) struct backlight_properties props; struct backlight_device *bl; struct pwm_bl_data *pb; + struct pinctrl *pinctrl; unsigned int max; int ret; + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); + if (IS_ERR(pinctrl)) + dev_warn(&pdev->dev, "unable to select pin group\n"); + if (!data) { ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); if (ret < 0) {
Enable pinctrl for pwm-backlight. Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> --- drivers/video/backlight/pwm_bl.c | 7 +++++++ 1 file changed, 7 insertions(+)