| Message ID | 20200428172322.2.Iacb3c8152c3cf9015a91308678155a578b0cc050@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: Document proper return value for gpio drivers | expand |
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); /**
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
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
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
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
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
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);
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); /**
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(-)