From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support. Date: Tue, 22 Oct 2013 00:48:08 +0200 Message-ID: <1671561.1xYjjYivUO@avalon> References: <20131019104555.GI18477@ns203013.ovh.net> <1382346813-8449-1-git-send-email-denis@eukrea.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <1382346813-8449-1-git-send-email-denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Denis Carikli Cc: Jean-Christophe Plagniol-Villard , Sascha Hauer , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Eric =?ISO-8859-1?Q?B=E9nard?= , Richard Purdie , Jingoo Han , Rob Herring , Pawel Moll , Mark Rutland , Stephen Warren , Ian Campbell , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Lothar =?ISO-8859-1?Q?Wa=DFmann?= List-Id: devicetree@vger.kernel.org Hi Denis, Thanks for the patch. Please see below for a couple of small comment. On Monday 21 October 2013 11:13:33 Denis Carikli wrote: > Cc: Richard Purdie > Cc: Jingoo Han > Cc: Laurent Pinchart > Cc: Rob Herring > Cc: Pawel Moll > Cc: Mark Rutland > Cc: Stephen Warren > Cc: Ian Campbell > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Cc: Sascha Hauer > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org > Cc: Lothar Wa=DFmann > Cc: Jean-Christophe Plagniol-Villard > Cc: Eric B=E9nard > Signed-off-by: Denis Carikli > --- > ChangeLog v3->v4: > - The default-brightness property is now optional, it defaults to 1 i= f not > set. - def_value int becomes an u32. > - gbl->def_value was set to pdata->def_value in pdata mode to avoid a= n extra > check. > --- > .../bindings/video/backlight/gpio-backlight.txt | 20 ++++++ > drivers/video/backlight/gpio_backlight.c | 69 ++++++++++= +++++-- > 2 files changed, 82 insertions(+), 7 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt >=20 > diff --git > a/Documentation/devicetree/bindings/video/backlight/gpio-backlight.tx= t > b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.tx= t new > file mode 100644 > index 0000000..3474d4a > --- /dev/null > +++ b/Documentation/devicetree/bindings/video/backlight/gpio-backligh= t.txt > @@ -0,0 +1,20 @@ > +gpio-backlight bindings > + > +Required properties: > + - compatible: "gpio-backlight" > + - gpios: describes the gpio that is used for enabling/disabling th= e > backlight > + (see GPIO binding[0] for more details). > + > +Optional properties: > + - default-brightness-level: the default brightness level (can be 0= (off) > or > + 1(on) since GPIOs only support theses levels). I believe you should specify what the default value is when the propert= y isn't=20 available. > + > +[0]: Documentation/devicetree/bindings/gpio/gpio.txt > + > +Example: > + > + backlight { > + compatible =3D "gpio-backlight"; > + gpios =3D <&gpio3 4 0>; > + default-brightness-level =3D <0>; > + }; > diff --git a/drivers/video/backlight/gpio_backlight.c > b/drivers/video/backlight/gpio_backlight.c index 81fb127..248124d 100= 644 > --- a/drivers/video/backlight/gpio_backlight.c > +++ b/drivers/video/backlight/gpio_backlight.c > @@ -13,6 +13,8 @@ > #include > #include > #include > +#include > +#include > #include > #include > #include > @@ -23,6 +25,7 @@ struct gpio_backlight { >=20 > int gpio; > int active; > + u32 def_value; > }; >=20 > static int gpio_backlight_update_status(struct backlight_device *bl) > @@ -60,6 +63,41 @@ static const struct backlight_ops gpio_backlight_o= ps =3D { > .check_fb =3D gpio_backlight_check_fb, > }; >=20 > +static int gpio_backlight_probe_dt(struct platform_device *pdev, > + struct gpio_backlight *gbl) > +{ > + struct device_node *np =3D pdev->dev.of_node; > + enum of_gpio_flags gpio_flags; > + int ret; > + > + gbl->fbdev =3D NULL; > + gbl->gpio =3D of_get_gpio_flags(np, 0, &gpio_flags); > + > + gbl->active =3D (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1; I would move this line after the error check below. > + > + if (gbl->gpio =3D=3D -EPROBE_DEFER) { > + return ERR_PTR(-EPROBE_DEFER); Any reason not to retrun -EPROBE_DEFER directly ? > + } else if (gbl->gpio < 0) { > + dev_err(&pdev->dev, "Error: gpios is a required parameter.\n"); > + return gbl->gpio; > + } Maybe you would do something like if (gbl->gpio < 0) { if (gbl->gpio =3D=3D -EPROBE_DEFER) dev_err(&pdev->dev, "Error: gpios is a required parameter.\n"); return gbl->gpio; } > + > + ret =3D of_property_read_u32(np, "default-brightness-level", > + &gbl->def_value); > + if (ret < 0) { > + /* The property is optional. */ > + gbl->def_value =3D 1; > + } > + > + if (gbl->def_value > 1) { > + dev_warn(&pdev->dev, > + "Warning: Invalid default-brightness-level value. Its value can=20 be > either 0(off) or 1(on).\n"); I believe a less verbose message (without the second sentence) would ha= ve=20 done, but that's up to you. > + gbl->def_value =3D 1; > + } > + > + return 0; > +} > + > static int gpio_backlight_probe(struct platform_device *pdev) > { > struct gpio_backlight_platform_data *pdata =3D > @@ -67,10 +105,12 @@ static int gpio_backlight_probe(struct platform_= device > *pdev) struct backlight_properties props; > struct backlight_device *bl; > struct gpio_backlight *gbl; > + struct device_node *np =3D pdev->dev.of_node; > int ret; >=20 > - if (!pdata) { > - dev_err(&pdev->dev, "failed to find platform data\n"); > + if (!pdata && !np) { > + dev_err(&pdev->dev, > + "failed to find platform data or device tree node.\n"); > return -ENODEV; > } >=20 > @@ -79,14 +119,22 @@ static int gpio_backlight_probe(struct platform_= device > *pdev) return -ENOMEM; >=20 > gbl->dev =3D &pdev->dev; > - gbl->fbdev =3D pdata->fbdev; > - gbl->gpio =3D pdata->gpio; > - gbl->active =3D pdata->active_low ? 0 : 1; > + > + if (np) { > + ret =3D gpio_backlight_probe_dt(pdev, gbl); > + if (ret) > + return ret; > + } else { > + gbl->fbdev =3D pdata->fbdev; > + gbl->gpio =3D pdata->gpio; > + gbl->active =3D pdata->active_low ? 0 : 1; > + gbl->def_value =3D pdata->def_value; > + } >=20 > ret =3D devm_gpio_request_one(gbl->dev, gbl->gpio, GPIOF_DIR_OUT | > (gbl->active ? GPIOF_INIT_LOW >=20 > : GPIOF_INIT_HIGH), >=20 > - pdata->name); > + pdata ? pdata->name : "backlight"); > if (ret < 0) { > dev_err(&pdev->dev, "unable to request GPIO\n"); > return ret; > @@ -103,17 +151,24 @@ static int gpio_backlight_probe(struct platform= _device > *pdev) return PTR_ERR(bl); > } >=20 > - bl->props.brightness =3D pdata->def_value; > + bl->props.brightness =3D gbl->def_value; > + > backlight_update_status(bl); >=20 > platform_set_drvdata(pdev, bl); > return 0; > } >=20 > +static struct of_device_id gpio_backlight_of_match[] =3D { > + { .compatible =3D "gpio-backlight" }, > + { /* sentinel */ } > +}; > + > static struct platform_driver gpio_backlight_driver =3D { > .driver =3D { > .name =3D "gpio-backlight", > .owner =3D THIS_MODULE, > + .of_match_table =3D of_match_ptr(gpio_backlight_of_match), > }, > .probe =3D gpio_backlight_probe, > }; --=20 Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe devicetree" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html