[PATCH 1/2] gpio: Document proper return value for gpio drivers

[PATCH 1/2] gpio: Document proper return value for gpio drivers

[Douglas Anderson]

The legacy defines GPIOF_DIR_XXX are only for consumers. Document the
proper ones.  Also: don't use "_XXX" since that's harder to find with
"git grep".  Just list both of the values.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 include/linux/gpio/driver.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index b8fc92c177eb..7b5f5681b7e4 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -267,9 +267,9 @@ struct gpio_irq_chip {
  * @free: optional hook for chip-specific deactivation, such as
  *	disabling module power and clock; may sleep
  * @get_direction: returns direction for signal "offset", 0=out, 1=in,
- *	(same as GPIOF_DIR_XXX), or negative error.
- *	It is recommended to always implement this function, even on
- *	input-only or output-only gpio chips.
+ *	(same as GPIO_LINE_DIRECTION_OUT / GPIO_LINE_DIRECTION_IN),
+ *	or negative error. It is recommended to always implement this
+ *	function, even on input-only or output-only gpio chips.
  * @direction_input: configures signal "offset" as input, or returns error
  *	This can be omitted on input-only or output-only gpio chips.
  * @direction_output: configures signal "offset" as output, or returns error
-- 
2.26.2.303.gf8c07b1a785-goog

[PATCH 2/2] gpio: Make "offset" and "unsigned int", not just "unsigned"

[Douglas Anderson]

When I copied the function prototypes from the GPIO header file into
my own driver, checkpatch yelled at me saying that I shouldn't use use
"unsigned" but instead should say "unsigned int".  Let's make the
header file use "unsigned int" so others who copy like I did won't get
yelled at.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 include/linux/gpio/driver.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 7b5f5681b7e4..8c41ae41b6bb 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -349,30 +349,30 @@ struct gpio_chip {
 	struct module		*owner;
 
 	int			(*request)(struct gpio_chip *gc,
-						unsigned offset);
+						unsigned int offset);
 	void			(*free)(struct gpio_chip *gc,
-						unsigned offset);
+						unsigned int offset);
 	int			(*get_direction)(struct gpio_chip *gc,
-						unsigned offset);
+						unsigned int offset);
 	int			(*direction_input)(struct gpio_chip *gc,
-						unsigned offset);
+						unsigned int offset);
 	int			(*direction_output)(struct gpio_chip *gc,
-						unsigned offset, int value);
+						unsigned int offset, int value);
 	int			(*get)(struct gpio_chip *gc,
-						unsigned offset);
+						unsigned int offset);
 	int			(*get_multiple)(struct gpio_chip *gc,
 						unsigned long *mask,
 						unsigned long *bits);
 	void			(*set)(struct gpio_chip *gc,
-						unsigned offset, int value);
+						unsigned int offset, int value);
 	void			(*set_multiple)(struct gpio_chip *gc,
 						unsigned long *mask,
 						unsigned long *bits);
 	int			(*set_config)(struct gpio_chip *gc,
-					      unsigned offset,
+					      unsigned int offset,
 					      unsigned long config);
 	int			(*to_irq)(struct gpio_chip *gc,
-						unsigned offset);
+						unsigned int offset);
 
 	void			(*dbg_show)(struct seq_file *s,
 						struct gpio_chip *gc);
@@ -459,7 +459,7 @@ struct gpio_chip {
 };
 
 extern const char *gpiochip_is_requested(struct gpio_chip *gc,
-			unsigned offset);
+			unsigned int offset);
 
 /* add/remove chips */
 extern int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
@@ -657,9 +657,9 @@ static inline int gpiochip_irqchip_add_nested(struct gpio_chip *gc,
 }
 #endif /* CONFIG_LOCKDEP */
 
-int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset);
-void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset);
-int gpiochip_generic_config(struct gpio_chip *gc, unsigned offset,
+int gpiochip_generic_request(struct gpio_chip *gc, unsigned int offset);
+void gpiochip_generic_free(struct gpio_chip *gc, unsigned int offset);
+int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
 			    unsigned long config);
 
 /**
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH 2/2] gpio: Make "offset" and "unsigned int", not just "unsigned"

[Joe Perches]

On Tue, 2020-04-28 at 17:23 -0700, Douglas Anderson wrote:
> When I copied the function prototypes from the GPIO header file into
> my own driver, checkpatch yelled at me saying that I shouldn't use use
> "unsigned" but instead should say "unsigned int".  Let's make the
> header file use "unsigned int" so others who copy like I did won't get
> yelled at.

There are a few other unsigned declarations in the file.
Maybe do all of them (and remove the unnecessary externs)?

trivial reformatting of the function pointer block too
---
 include/linux/gpio/driver.h | 79 ++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 47 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index b8fc92c..478fb0 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -348,40 +348,26 @@ struct gpio_chip {
 	struct device		*parent;
 	struct module		*owner;
 
-	int			(*request)(struct gpio_chip *gc,
-						unsigned offset);
-	void			(*free)(struct gpio_chip *gc,
-						unsigned offset);
-	int			(*get_direction)(struct gpio_chip *gc,
-						unsigned offset);
-	int			(*direction_input)(struct gpio_chip *gc,
-						unsigned offset);
-	int			(*direction_output)(struct gpio_chip *gc,
-						unsigned offset, int value);
-	int			(*get)(struct gpio_chip *gc,
-						unsigned offset);
-	int			(*get_multiple)(struct gpio_chip *gc,
-						unsigned long *mask,
-						unsigned long *bits);
-	void			(*set)(struct gpio_chip *gc,
-						unsigned offset, int value);
-	void			(*set_multiple)(struct gpio_chip *gc,
-						unsigned long *mask,
-						unsigned long *bits);
-	int			(*set_config)(struct gpio_chip *gc,
-					      unsigned offset,
-					      unsigned long config);
-	int			(*to_irq)(struct gpio_chip *gc,
-						unsigned offset);
-
-	void			(*dbg_show)(struct seq_file *s,
-						struct gpio_chip *gc);
-
-	int			(*init_valid_mask)(struct gpio_chip *gc,
-						   unsigned long *valid_mask,
-						   unsigned int ngpios);
-
-	int			(*add_pin_ranges)(struct gpio_chip *gc);
+	int	(*request)(struct gpio_chip *gc, unsigned int offset);
+	void	(*free)(struct gpio_chip *gc, unsigned int offset);
+	int	(*get_direction)(struct gpio_chip *gc, unsigned int offset);
+	int	(*direction_input)(struct gpio_chip *gc, unsigned int offset);
+	int	(*direction_output)(struct gpio_chip *gc,
+				    unsigned int offset, int value);
+	int	(*get)(struct gpio_chip *gc, unsigned int offset);
+	int	(*get_multiple)(struct gpio_chip *gc,
+				unsigned long *mask, unsigned long *bits);
+	void	(*set)(struct gpio_chip *gc, unsigned int offset, int value);
+	void	(*set_multiple)(struct gpio_chip *gc,
+				unsigned long *mask, unsigned long *bits);
+	int	(*set_config)(struct gpio_chip *gc,
+			      unsigned int offset, unsigned long config);
+	int	(*to_irq)(struct gpio_chip *gc, unsigned int offset);
+	void	(*dbg_show)(struct seq_file *s, struct gpio_chip *gc);
+	int	(*init_valid_mask)(struct gpio_chip *gc,
+				   unsigned long *valid_mask,
+				   unsigned int ngpios);
+	int	(*add_pin_ranges)(struct gpio_chip *gc);
 
 	int			base;
 	u16			ngpio;
@@ -458,13 +444,12 @@ struct gpio_chip {
 #endif /* CONFIG_OF_GPIO */
 };
 
-extern const char *gpiochip_is_requested(struct gpio_chip *gc,
-			unsigned offset);
+const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset);
 
 /* add/remove chips */
-extern int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
-				      struct lock_class_key *lock_key,
-				      struct lock_class_key *request_key);
+int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
+			       struct lock_class_key *lock_key,
+			       struct lock_class_key *request_key);
 
 /**
  * gpiochip_add_data() - register a gpio_chip
@@ -504,12 +489,12 @@ static inline int gpiochip_add(struct gpio_chip *gc)
 {
 	return gpiochip_add_data(gc, NULL);
 }
-extern void gpiochip_remove(struct gpio_chip *gc);
-extern int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
-				  void *data);
+void gpiochip_remove(struct gpio_chip *gc);
+int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
+			   void *data);
 
-extern struct gpio_chip *gpiochip_find(void *data,
-			      int (*match)(struct gpio_chip *gc, void *data));
+struct gpio_chip *gpiochip_find(void *data,
+				int (*match)(struct gpio_chip *gc, void *data));
 
 bool gpiochip_line_is_irq(struct gpio_chip *gc, unsigned int offset);
 int gpiochip_reqres_irq(struct gpio_chip *gc, unsigned int offset);
@@ -657,9 +642,9 @@ static inline int gpiochip_irqchip_add_nested(struct gpio_chip *gc,
 }
 #endif /* CONFIG_LOCKDEP */
 
-int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset);
-void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset);
-int gpiochip_generic_config(struct gpio_chip *gc, unsigned offset,
+int gpiochip_generic_request(struct gpio_chip *gc, unsigned int offset);
+void gpiochip_generic_free(struct gpio_chip *gc, unsigned int offset);
+int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
 			    unsigned long config);
 
 /**

Re: [PATCH 2/2] gpio: Make "offset" and "unsigned int", not just "unsigned"

[Doug Anderson]

Hi,

On Tue, Apr 28, 2020 at 5:38 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2020-04-28 at 17:23 -0700, Douglas Anderson wrote:
> > When I copied the function prototypes from the GPIO header file into
> > my own driver, checkpatch yelled at me saying that I shouldn't use use
> > "unsigned" but instead should say "unsigned int".  Let's make the
> > header file use "unsigned int" so others who copy like I did won't get
> > yelled at.
>
> There are a few other unsigned declarations in the file.

There are?  I swear I looked for them before I sent my patch and I
couldn't find them.  Then I looked again upon seeing your reply and I
still can't find them.  My eyes are bad, though.  Maybe you can give
me specifics?


> Maybe do all of them (and remove the unnecessary externs)?

You mean just remove the word "extern" everywhere in this file?  Sure,
I can if you want.


> trivial reformatting of the function pointer block too

Wow, I must be totally out of it.  Maybe it's the gin and tonic I just
had.  I don't understand this comment either.  Can you clarify?


-Doug

Re: [PATCH 2/2] gpio: Make "offset" and "unsigned int", not just "unsigned"

[Joe Perches]

On Tue, 2020-04-28 at 17:50 -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Apr 28, 2020 at 5:38 PM Joe Perches <joe@perches.com> wrote:
> > On Tue, 2020-04-28 at 17:23 -0700, Douglas Anderson wrote:
> > > When I copied the function prototypes from the GPIO header file into
> > > my own driver, checkpatch yelled at me saying that I shouldn't use use
> > > "unsigned" but instead should say "unsigned int".  Let's make the
> > > header file use "unsigned int" so others who copy like I did won't get
> > > yelled at.
> > 
> > There are a few other unsigned declarations in the file.
> 
> There are?  I swear I looked for them before I sent my patch and I
> couldn't find them.  Then I looked again upon seeing your reply and I
> still can't find them.  My eyes are bad, though.  Maybe you can give
> me specifics?

$ git grep -P -n '\bunsigned\s+(?!int|long)' include/linux/gpio/driver.h
include/linux/gpio/driver.h:352:                                                unsigned offset);
include/linux/gpio/driver.h:354:                                                unsigned offset);
include/linux/gpio/driver.h:356:                                                unsigned offset);
include/linux/gpio/driver.h:358:                                                unsigned offset);
include/linux/gpio/driver.h:360:                                                unsigned offset, int value);
include/linux/gpio/driver.h:362:                                                unsigned offset);
include/linux/gpio/driver.h:367:                                                unsigned offset, int value);
include/linux/gpio/driver.h:372:                                              unsigned offset,
include/linux/gpio/driver.h:375:                                                unsigned offset);
include/linux/gpio/driver.h:462:                        unsigned offset);
include/linux/gpio/driver.h:660:int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset);
include/linux/gpio/driver.h:661:void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset);
include/linux/gpio/driver.h:662:int gpiochip_generic_config(struct gpio_chip *gc, unsigned offset,

> > Maybe do all of them (and remove the unnecessary externs)?
> 
> You mean just remove the word "extern" everywhere in this file?  Sure,
> I can if you want.

Up to the actual maintainers I suppose.
There are only a few extern function declarations.
Most do not use extern.

> > trivial reformatting of the function pointer block too
> 
> Wow, I must be totally out of it.  Maybe it's the gin and tonic I just
> had.  I don't understand this comment either.  Can you clarify?

	int				(*foo)(...,
				               ...);

might be better with fewer tabs between return type and function pointer

	int		(*foo)(..., ...);

cheers, oe

Re: [PATCH 2/2] gpio: Make "offset" and "unsigned int", not just "unsigned"

[Doug Anderson]

Hi,

On Tue, Apr 28, 2020 at 5:57 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2020-04-28 at 17:50 -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 28, 2020 at 5:38 PM Joe Perches <joe@perches.com> wrote:
> > > On Tue, 2020-04-28 at 17:23 -0700, Douglas Anderson wrote:
> > > > When I copied the function prototypes from the GPIO header file into
> > > > my own driver, checkpatch yelled at me saying that I shouldn't use use
> > > > "unsigned" but instead should say "unsigned int".  Let's make the
> > > > header file use "unsigned int" so others who copy like I did won't get
> > > > yelled at.
> > >
> > > There are a few other unsigned declarations in the file.
> >
> > There are?  I swear I looked for them before I sent my patch and I
> > couldn't find them.  Then I looked again upon seeing your reply and I
> > still can't find them.  My eyes are bad, though.  Maybe you can give
> > me specifics?
>
> $ git grep -P -n '\bunsigned\s+(?!int|long)' include/linux/gpio/driver.h
> include/linux/gpio/driver.h:352:                                                unsigned offset);
> include/linux/gpio/driver.h:354:                                                unsigned offset);
> include/linux/gpio/driver.h:356:                                                unsigned offset);
> include/linux/gpio/driver.h:358:                                                unsigned offset);
> include/linux/gpio/driver.h:360:                                                unsigned offset, int value);
> include/linux/gpio/driver.h:362:                                                unsigned offset);
> include/linux/gpio/driver.h:367:                                                unsigned offset, int value);
> include/linux/gpio/driver.h:372:                                              unsigned offset,
> include/linux/gpio/driver.h:375:                                                unsigned offset);
> include/linux/gpio/driver.h:462:                        unsigned offset);
> include/linux/gpio/driver.h:660:int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset);
> include/linux/gpio/driver.h:661:void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset);
> include/linux/gpio/driver.h:662:int gpiochip_generic_config(struct gpio_chip *gc, unsigned offset,

...riiiiiggght.   ...and now I run your sed script _after_ my patch
and I get no hits.  ...so I'm still confused about what you want me to
do that's not already done in my patch.


> > > Maybe do all of them (and remove the unnecessary externs)?
> >
> > You mean just remove the word "extern" everywhere in this file?  Sure,
> > I can if you want.
>
> Up to the actual maintainers I suppose.
> There are only a few extern function declarations.
> Most do not use extern.

OK, maybe I'll wait for Linux W. or Bartosz to weigh in unless there
is some Linux policy against using "extern" in header files?


> > > trivial reformatting of the function pointer block too
> >
> > Wow, I must be totally out of it.  Maybe it's the gin and tonic I just
> > had.  I don't understand this comment either.  Can you clarify?
>
>         int                             (*foo)(...,
>                                                ...);
>
> might be better with fewer tabs between return type and function pointer
>
>         int             (*foo)(..., ...);

I'll wait for Linux W. or Bartosz to weigh in here, since it feels
more like a style decision.  Happy to add a patch for it, though.

-Doug

Re: [PATCH 2/2] gpio: Make "offset" and "unsigned int", not just "unsigned"

[Joe Perches]

On Tue, 2020-04-28 at 18:04 -0700, Doug Anderson wrote:
> Hi,

<slurring but replying with hi again...>

> On Tue, Apr 28, 2020 at 5:57 PM Joe Perches <joe@perches.com> wrote:
> > On Tue, 2020-04-28 at 17:50 -0700, Doug Anderson wrote:
> > > $ git grep -P -n '\bunsigned\s+(?!int|long)' include/linux/gpio/driver.h
> > include/linux/gpio/driver.h:352:                                                unsigned offset);
> > include/linux/gpio/driver.h:354:                                                unsigned offset);
> > include/linux/gpio/driver.h:356:                                                unsigned offset);
> > include/linux/gpio/driver.h:358:                                                unsigned offset);
> > include/linux/gpio/driver.h:360:                                                unsigned offset, int value);
> > include/linux/gpio/driver.h:362:                                                unsigned offset);
> > include/linux/gpio/driver.h:367:                                                unsigned offset, int value);
> > include/linux/gpio/driver.h:372:                                              unsigned offset,
> > include/linux/gpio/driver.h:375:                                                unsigned offset);
> > include/linux/gpio/driver.h:462:                        unsigned offset);
> > include/linux/gpio/driver.h:660:int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset);
> > include/linux/gpio/driver.h:661:void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset);
> > include/linux/gpio/driver.h:662:int gpiochip_generic_config(struct gpio_chip *gc, unsigned offset,
> 
> ...riiiiiggght.   ...and now I run your sed script _after_ my patch
> and I get no hits.  ...so I'm still confused about what you want me to
> do that's not already done in my patch.

So you did say it's the g&t.
It seems I only looked at the first diff block.

cheers, <hck>, Joe

Re: [PATCH 1/2] gpio: Document proper return value for gpio drivers

[Linus Walleij]

On Wed, Apr 29, 2020 at 2:24 AM Douglas Anderson <dianders@chromium.org> wrote:

> The legacy defines GPIOF_DIR_XXX are only for consumers. Document the
> proper ones.  Also: don't use "_XXX" since that's harder to find with
> "git grep".  Just list both of the values.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Patch applied! Sorry for the historical confusion.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: Make "offset" and "unsigned int", not just "unsigned"

[Linus Walleij]

On Wed, Apr 29, 2020 at 2:24 AM Douglas Anderson <dianders@chromium.org> wrote:

> When I copied the function prototypes from the GPIO header file into
> my own driver, checkpatch yelled at me saying that I shouldn't use use
> "unsigned" but instead should say "unsigned int".  Let's make the
> header file use "unsigned int" so others who copy like I did won't get
> yelled at.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Patch applied. Nice cleanup!

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: Make "offset" and "unsigned int", not just "unsigned"

[Andy Shevchenko]

On Wed, Apr 29, 2020 at 3:40 AM Joe Perches <joe@perches.com> wrote:
> On Tue, 2020-04-28 at 17:23 -0700, Douglas Anderson wrote:

...

> +       int     (*request)(struct gpio_chip *gc, unsigned int offset);
> +       void    (*free)(struct gpio_chip *gc, unsigned int offset);
> +       int     (*get_direction)(struct gpio_chip *gc, unsigned int offset);
> +       int     (*direction_input)(struct gpio_chip *gc, unsigned int offset);
> +       int     (*direction_output)(struct gpio_chip *gc,
> +                                   unsigned int offset, int value);

Here...

> +       int     (*get)(struct gpio_chip *gc, unsigned int offset);
> +       int     (*get_multiple)(struct gpio_chip *gc,
> +                               unsigned long *mask, unsigned long *bits);
> +       void    (*set)(struct gpio_chip *gc, unsigned int offset, int value);
> +       void    (*set_multiple)(struct gpio_chip *gc,
> +                               unsigned long *mask, unsigned long *bits);
> +       int     (*set_config)(struct gpio_chip *gc,
> +                             unsigned int offset, unsigned long config);

...and here perhaps offset to move to previous line despite checkpatch warnings.

> +       int     (*to_irq)(struct gpio_chip *gc, unsigned int offset);
> +       void    (*dbg_show)(struct seq_file *s, struct gpio_chip *gc);
> +       int     (*init_valid_mask)(struct gpio_chip *gc,
> +                                  unsigned long *valid_mask,
> +                                  unsigned int ngpios);
> +       int     (*add_pin_ranges)(struct gpio_chip *gc);

-- 
With Best Regards,
Andy Shevchenko