diff mbox series

[1/4] Revert "gpio: set up initial state from .get_direction()"

Message ID 1509396602-1936-2-git-send-email-timur@codeaurora.org
State New
Headers show
Series pinctrl: qcom: add support for sparse GPIOs | expand

Commit Message

Timur Tabi Oct. 30, 2017, 8:49 p.m. UTC 
This reverts commit 72d3200061776264941be1b5a9bb8e926b3b30a5.

We cannot blindly query the direction of all GPIOs when the pins are
first registered.  The get_direction callback normally triggers a
read/write to hardware, but we shouldn't be touching the hardware for
an individual GPIO until after it's been properly requested.
---
 drivers/gpio/gpiolib.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

Comments

Andy Shevchenko Oct. 31, 2017, 9:08 a.m. UTC | #1 
On Mon, 2017-10-30 at 15:49 -0500, Timur Tabi wrote:
> This reverts commit 72d3200061776264941be1b5a9bb8e926b3b30a5.
> 
> We cannot blindly query the direction of all GPIOs when the pins are
> first registered.  The get_direction callback normally triggers a
> read/write to hardware, but we shouldn't be touching the hardware for
> an individual GPIO until after it's been properly requested.
> 

> +		/* REVISIT: most hardware initializes GPIOs as inputs
> (often
> +		 * with pullups enabled) so power usage is minimized.
> Linux
> +		 * code should set the gpio direction first thing;
> but until
> +		 * it does, and in case chip->get_direction is not
> set, we may
> +		 * expose the wrong direction in sysfs.
> +		 */

Can you preserve the style and indentation of the commit?
Does checkpatch warn you about style? (It's apparently not a net
subsystem)

> +		desc->flags = !chip->direction_input ? (1 <<
> FLAG_IS_OUT) : 0;
>  	}
>  
>  #ifdef CONFIG_PINCTRL
Timur Tabi Oct. 31, 2017, 12:28 p.m. UTC | #2 
On 10/31/17 4:08 AM, Andy Shevchenko wrote:
> Can you preserve the style and indentation of the commit?
> Does checkpatch warn you about style? (It's apparently not a net
> subsystem)

I ran checkpatch on it and it didn't complain.

Also, I don't think that when reverting a patch, I am also supposed to 
to fix cosmetic changes of the code that existed before the original patch.
Fabio Estevam Oct. 31, 2017, 12:33 p.m. UTC | #3 
On Tue, Oct 31, 2017 at 10:28 AM, Timur Tabi <timur@codeaurora.org> wrote:

> I ran checkpatch on it and it didn't complain.

In this case checkpatch should have given you an error due to the
missing Signed-off-by tag :-)
--
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
Andy Shevchenko Oct. 31, 2017, 12:34 p.m. UTC | #4 
On Tue, 2017-10-31 at 07:28 -0500, Timur Tabi wrote:
> On 10/31/17 4:08 AM, Andy Shevchenko wrote:
> > Can you preserve the style and indentation of the commit?
> > Does checkpatch warn you about style? (It's apparently not a net
> > subsystem)
> 
> I ran checkpatch on it and it didn't complain.

Hmm... Okay.

> Also, I don't think that when reverting a patch, I am also supposed
> to 
> to fix cosmetic changes of the code that existed before the original
> patch.

Point taken. Up to Linus how to proceed, it's indeed cosmetic non-code
change.
Timur Tabi Oct. 31, 2017, 12:36 p.m. UTC | #5 
On 10/31/17 7:33 AM, Fabio Estevam wrote:
> In this case checkpatch should have given you an error due to the
> missing Signed-off-by tag:-)

Indeed!  Something must be broken in my checkpatch invocation.  I did 
think that it was odd that I didn't get any complaints.  Thanks.
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eb80dac4e26a..60553af4c004 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1221,31 +1221,14 @@  int gpiochip_add_data(struct gpio_chip *chip, void *data)
 		struct gpio_desc *desc = &gdev->descs[i];
 
 		desc->gdev = gdev;
-		/*
-		 * REVISIT: most hardware initializes GPIOs as inputs
-		 * (often with pullups enabled) so power usage is
-		 * minimized. Linux code should set the gpio direction
-		 * first thing; but until it does, and in case
-		 * chip->get_direction is not set, we may expose the
-		 * wrong direction in sysfs.
-		 */
-
-		if (chip->get_direction) {
-			/*
-			 * If we have .get_direction, set up the initial
-			 * direction flag from the hardware.
-			 */
-			int dir = chip->get_direction(chip, i);
 
-			if (!dir)
-				set_bit(FLAG_IS_OUT, &desc->flags);
-		} else if (!chip->direction_input) {
-			/*
-			 * If the chip lacks the .direction_input callback
-			 * we logically assume all lines are outputs.
-			 */
-			set_bit(FLAG_IS_OUT, &desc->flags);
-		}
+		/* REVISIT: most hardware initializes GPIOs as inputs (often
+		 * with pullups enabled) so power usage is minimized. Linux
+		 * code should set the gpio direction first thing; but until
+		 * it does, and in case chip->get_direction is not set, we may
+		 * expose the wrong direction in sysfs.
+		 */
+		desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
 	}
 
 #ifdef CONFIG_PINCTRL