[v2,2/3] gpiolib: gpiod_hog remove separate name argument
diff mbox

Message ID 1440920686-6892-3-git-send-email-mpa@pengutronix.de
State New
Headers show

Commit Message

Markus Pargmann Aug. 30, 2015, 7:44 a.m. UTC
The gpio name is now stored in the gpio descriptor, so we can simply use
that instead of an argument to the function.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib-of.c | 2 +-
 drivers/gpio/gpiolib.c    | 8 ++++----
 drivers/gpio/gpiolib.h    | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

Comments

Linus Walleij Sept. 21, 2015, 11:28 p.m. UTC | #1
On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> The gpio name is now stored in the gpio descriptor, so we can simply use
> that instead of an argument to the function.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Several problems with this patch:

- Does not apply to v4.3-rc1
- Fixed that but noted that it just alters the call to gpiod_hog()
  in of_gpiochip_scan_hogs(), there is a local const char *name that
  should be removed too.
- Looking at of_get_gpio_hog() it is unclear to me that .name
  is set in the gpio_desc as desired. Please check this.

  Crucial code looks like this:

    if (name && of_property_read_string(np, "line-name", name))
        *name = np->name;

  i.e. is the line-name really propagated to gpiod->name?
  Are you sure you have tested this with a DTS using line-name
  and verified that it propagates properly?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Markus Pargmann Sept. 24, 2015, 6:39 a.m. UTC | #2
Hi,

On Mon, Sep 21, 2015 at 04:28:48PM -0700, Linus Walleij wrote:
> On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > The gpio name is now stored in the gpio descriptor, so we can simply use
> > that instead of an argument to the function.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> Several problems with this patch:
> 
> - Does not apply to v4.3-rc1

Yes, a bit older already, will rebase it.

> - Fixed that but noted that it just alters the call to gpiod_hog()
>   in of_gpiochip_scan_hogs(), there is a local const char *name that
>   should be removed too.

The local const char *name is temporarily used in
of_gpiochip_scan_hogs() to get the name from DT and assign it to the
descriptor:
	desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
	...
	else
		desc->name = name;

> - Looking at of_get_gpio_hog() it is unclear to me that .name
>   is set in the gpio_desc as desired. Please check this.
> 
>   Crucial code looks like this:
> 
>     if (name && of_property_read_string(np, "line-name", name))
>         *name = np->name;

of_get_gpio_hog() only parses the properties of the DT. It does not
assign any properties to the descriptor itself. So I kept it this way
and added the code for assignment to of_gpiochip_scan_gpios.

> 
>   i.e. is the line-name really propagated to gpiod->name?
>   Are you sure you have tested this with a DTS using line-name
>   and verified that it propagates properly?

Yes this is tested and works for me. I am using the two series together
without problems.

Best Regards,

Markus
Linus Walleij Sept. 24, 2015, 5:52 p.m. UTC | #3
On Wed, Sep 23, 2015 at 11:39 PM, Markus Pargmann <mpa@pengutronix.de> wrote:

>> - Fixed that but noted that it just alters the call to gpiod_hog()
>>   in of_gpiochip_scan_hogs(), there is a local const char *name that
>>   should be removed too.
>
> The local const char *name is temporarily used in
> of_gpiochip_scan_hogs() to get the name from DT and assign it to the
> descriptor:
>         desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
>         ...
>         else
>                 desc->name = name;

OK the problem is that this series is dependent on the
named GPIOs series. I want this series to stand alone,
because this series is not as controversial, and I want to
merge these initvals first.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Markus Pargmann Sept. 27, 2015, 2:34 p.m. UTC | #4
On Thu, Sep 24, 2015 at 10:52:55AM -0700, Linus Walleij wrote:
> On Wed, Sep 23, 2015 at 11:39 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> >> - Fixed that but noted that it just alters the call to gpiod_hog()
> >>   in of_gpiochip_scan_hogs(), there is a local const char *name that
> >>   should be removed too.
> >
> > The local const char *name is temporarily used in
> > of_gpiochip_scan_hogs() to get the name from DT and assign it to the
> > descriptor:
> >         desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
> >         ...
> >         else
> >                 desc->name = name;
> 
> OK the problem is that this series is dependent on the
> named GPIOs series. I want this series to stand alone,
> because this series is not as controversial, and I want to
> merge these initvals first.

OK, I think I won't get this completely independent but it shouldn't be
a problem to get this before the controversial patches. Will do that.

Best Regards,

Markus

Patch
diff mbox

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 48a7579dd62d..f1fe5123da28 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -232,7 +232,7 @@  static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 				continue;
 			}
 
-			if (gpiod_hog(desc, name, lflags, dflags))
+			if (gpiod_hog(desc, lflags, dflags))
 				continue;
 		}
 	}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 872fdd3617c1..f1559fa72c36 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2181,15 +2181,15 @@  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 /**
  * gpiod_hog - Hog the specified GPIO desc given the provided flags
  * @desc:	gpio whose value will be assigned
- * @name:	gpio line name
  * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
  *		of_get_gpio_hog()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  */
-int gpiod_hog(struct gpio_desc *desc, const char *name,
-	      unsigned long lflags, enum gpiod_flags dflags)
+int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
+	      enum gpiod_flags dflags)
 {
 	int status;
+	const char *name = desc->name;
 
 	status = __gpiod_request(desc, name);
 	if (status) {
@@ -2211,7 +2211,7 @@  int gpiod_hog(struct gpio_desc *desc, const char *name,
 	set_bit(FLAG_IS_HOGGED, &desc->flags);
 
 	pr_info("GPIO line %d (%s) hogged as %s%s\n",
-		desc_to_gpio(desc), name,
+		desc_to_gpio(desc), desc->name,
 		(dflags&GPIOD_FLAGS_BIT_DIR_OUT) ? "output" : "input",
 		(dflags&GPIOD_FLAGS_BIT_DIR_OUT) ?
 		  (dflags&GPIOD_FLAGS_BIT_DIR_VAL) ? "/high" : "/low":"");
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 78e634d1c719..6c2aeff59f86 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -97,8 +97,8 @@  struct gpio_desc {
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
-int gpiod_hog(struct gpio_desc *desc, const char *name,
-		unsigned long lflags, enum gpiod_flags dflags);
+int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
+	      enum gpiod_flags dflags);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip