Re: [PATCH] i2c: pca954x: Fix reset GPIO property name

Re: [PATCH] i2c: pca954x: Fix reset GPIO property name

[Alexandre Courbot]

On 06/03/2014 09:22 AM, Laurent Pinchart wrote:
> The DT bindings for the pca954x document that the reset GPIO is
> specified through the "reset-gpios" property. However, the driver
> erroneously uses a property name of "reset-gpio".
>
> The GPIO DT bindings documentation mentions that
>
> "GPIO properties should be named "[<name>-]gpios". The exact meaning of
> each gpios property must be documented in the device tree binding for
> each device."
>
> The correct reset GPIO property name is thus "reset-gpios". Fix the
> driver accordingly.

The plural form should be preferred indeed.

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>   drivers/i2c/muxes/i2c-mux-pca954x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> While commit dd34c37aa3e81715a1ed8da61fa438072428e188
>
> Author: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Date:   Wed Apr 23 17:28:09 2014 +0200
>
>      gpio: of: Allow -gpio suffix for property names
>
>      Many bindings use the -gpio suffix in property names. Support this in
>      addition to the -gpios suffix when requesting GPIOs using the new
>      descriptor-based API.
>
> adds support for the singular form of the property, the GPIO DT bindings
> document the plural form only. I've thus decided to fix the driver instead of
> the bindings, even though it could be argued that the singular form makes more
> sense in this case. I've CC'ed the people involved with the above commit for
> comments on that, and have no issue fixing the bindings instead of the driver
> if the singular form is preferred.
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 550bd36..73491ca 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -205,7 +205,7 @@ static int pca954x_probe(struct i2c_client *client,
>   		int gpio;
>
>   		/* Get the mux out of reset if a reset GPIO is specified. */
> -		gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, &flags);
> +		gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);

Won't this break DT compatibility for potential users of this driver? (I 
cannot find any in mainline though)

Considering that this GPIO is only acquired and set once and for all 
during probe (a very simple use-case), how about switching this to the 
gpiod interface instead? That way, you can take advantage of Thierry's 
patch and the plural form will work while also maintaining the singular 
form for backward compatibility.

Alex.

Re: [PATCH] i2c: pca954x: Fix reset GPIO property name

[Laurent Pinchart]

Hi Alexandre,

On Tuesday 03 June 2014 10:38:53 Alexandre Courbot wrote:
> On 06/03/2014 09:22 AM, Laurent Pinchart wrote:
> > The DT bindings for the pca954x document that the reset GPIO is
> > specified through the "reset-gpios" property. However, the driver
> > erroneously uses a property name of "reset-gpio".
> > 
> > The GPIO DT bindings documentation mentions that
> > 
> > "GPIO properties should be named "[<name>-]gpios". The exact meaning of
> > each gpios property must be documented in the device tree binding for
> > each device."
> > 
> > The correct reset GPIO property name is thus "reset-gpios". Fix the
> > driver accordingly.
> 
> The plural form should be preferred indeed.
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > ---
> > 
> >   drivers/i2c/muxes/i2c-mux-pca954x.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > While commit dd34c37aa3e81715a1ed8da61fa438072428e188
> > 
> > Author: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Date:   Wed Apr 23 17:28:09 2014 +0200
> > 
> >      gpio: of: Allow -gpio suffix for property names
> >      
> >      Many bindings use the -gpio suffix in property names. Support this in
> >      addition to the -gpios suffix when requesting GPIOs using the new
> >      descriptor-based API.
> > 
> > adds support for the singular form of the property, the GPIO DT bindings
> > document the plural form only. I've thus decided to fix the driver instead
> > of the bindings, even though it could be argued that the singular form
> > makes more sense in this case. I've CC'ed the people involved with the
> > above commit for comments on that, and have no issue fixing the bindings
> > instead of the driver if the singular form is preferred.
> > 
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > b/drivers/i2c/muxes/i2c-mux-pca954x.c index 550bd36..73491ca 100644
> > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > @@ -205,7 +205,7 @@ static int pca954x_probe(struct i2c_client *client,
> > 
> >   		int gpio;
> >   		
> >   		/* Get the mux out of reset if a reset GPIO is specified. */
> > 
> > -		gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, &flags);
> > +		gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);
> 
> Won't this break DT compatibility for potential users of this driver? (I
> cannot find any in mainline though)

Only if those users don't conform to the DT bindings documentation that 
mandates the plural form, and are thus broken already ;-)

> Considering that this GPIO is only acquired and set once and for all
> during probe (a very simple use-case), how about switching this to the
> gpiod interface instead? That way, you can take advantage of Thierry's
> patch and the plural form will work while also maintaining the singular
> form for backward compatibility.

Regardless of the singular vs. plural debate I think that's a good idea. I'll 
thus rework this patch.

-- 
Regards,

Laurent Pinchart

[PATCH v2] i2c: pca954x: Use the descriptor-based GPIO API

[Laurent Pinchart]

The ID-based GPIO API pushes handling of GPIO polarity to drivers.
Simplify the driver by switching to the descriptor-based GPIO API.

This also fixes a mismatch between the pca954x DT bindings that document
a "reset-gpios" property and the driver that requests a "reset-gpio"
property.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 550bd36..c2c443f 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -186,7 +186,7 @@ static int pca954x_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
 	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
-	struct device_node *np = client->dev.of_node;
+	struct gpio_desc *gpio;
 	int num, force, class;
 	struct pca954x *data;
 	int ret;
@@ -200,21 +200,10 @@ static int pca954x_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, data);
 
-	if (IS_ENABLED(CONFIG_OF) && np) {
-		enum of_gpio_flags flags;
-		int gpio;
-
-		/* Get the mux out of reset if a reset GPIO is specified. */
-		gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, &flags);
-		if (gpio_is_valid(gpio)) {
-			ret = devm_gpio_request_one(&client->dev, gpio,
-					flags & OF_GPIO_ACTIVE_LOW ?
-					GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
-					"pca954x reset");
-			if (ret < 0)
-				return ret;
-		}
-	}
+	/* Get the mux out of reset if a reset GPIO is specified. */
+	gpio = devm_gpiod_get(&client->dev, "reset");
+	if (!IS_ERR(gpio))
+		gpiod_direction_output(gpio, 0);
 
 	/* Write the mux register at addr to verify
 	 * that the mux is in fact present. This also
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] i2c: pca954x: Use the descriptor-based GPIO API

[Linus Walleij]

On Tue, Jun 3, 2014 at 1:15 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:

> The ID-based GPIO API pushes handling of GPIO polarity to drivers.
> Simplify the driver by switching to the descriptor-based GPIO API.
>
> This also fixes a mismatch between the pca954x DT bindings that document
> a "reset-gpios" property and the driver that requests a "reset-gpio"
> property.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij

Re: [PATCH v2] i2c: pca954x: Use the descriptor-based GPIO API

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

On Tue, Jun 03, 2014 at 01:15:26PM +0200, Laurent Pinchart wrote:
> The ID-based GPIO API pushes handling of GPIO polarity to drivers.
> Simplify the driver by switching to the descriptor-based GPIO API.
> 
> This also fixes a mismatch between the pca954x DT bindings that document
> a "reset-gpios" property and the driver that requests a "reset-gpio"
> property.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] i2c: pca954x: Use the descriptor-based GPIO API

[Alexandre Courbot]

On 06/03/2014 08:15 PM, Laurent Pinchart wrote:
> The ID-based GPIO API pushes handling of GPIO polarity to drivers.
> Simplify the driver by switching to the descriptor-based GPIO API.
>
> This also fixes a mismatch between the pca954x DT bindings that document
> a "reset-gpios" property and the driver that requests a "reset-gpio"
> property.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Reviewed-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------