gpiolib: add __must_check attribute to the gpiochip_add function
Subsystems (2)
PATCH 3/3 gpiolib: add __must_check attribute to the gpiochip_add function
--- drivers/gpio/gpiolib.c | 2 +- include/asm-generic/gpio.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/gpio/gpiolib.c +1 -1
@@ -161,7 +161,7 @@ static int gpiochip_find_base(int ngpio)* because the chip->base is invalid or already associated with a * different chip. Otherwise it returns zero as a success code. */ -int gpiochip_add (struct gpio_chip *chip) +int __must_check gpiochip_add (struct gpio_chip *chip) {unsigned long flags;int status =0 ;
include/asm-generic/gpio.h +1 -1
@@ -77,7 +77,7 @@ extern const char *gpiochip_is_requested(struct gpio_chip *chip,extern int __must_check gpiochip_reserve (int start,int ngpio);/* add/remove chips */ -extern int gpiochip_add (struct gpio_chip *chip); +extern int __must_check gpiochip_add (struct gpio_chip *chip);extern int __must_check gpiochip_remove (struct gpio_chip *chip);
Signature · 1 line
1.5.2.2
Quoted 2 lines · Anton Vorontsov
Signed-off-by Anton Vorontsov <[email protected]>
I'm curious why you want to change this. When it fails, aKERN_ERR message is emitted ... if you're going to insist that callers handle this -- e.g. platform setup code might use a "WARN_ON(gpiochip_add(...) < 0)" -- why not take out that KERN_ERR mechanism too?
NAK unless it comes with a patch to update all callers to do that checking, so that this isn't just "add build warnings". But I'm not sure I like these attributes in cases like this.
- Dave
Quoted 34 lines
--- drivers/gpio/gpiolib.c | 2 +- include/asm-generic/gpio.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)drivers/gpio/gpiolib.c +1
-1@@ -161,7 +161,7 @@ static int gpiochip_find_base(int ngpio)* because the chip->base is invalid or already associated with a * different chip. Otherwise it returns zero as a success code. */ -int gpiochip_add (struct gpio_chip *chip) +int __must_check gpiochip_add (struct gpio_chip *chip) {unsigned long flags;int status =0 ;include/asm-generic/gpio.h +1
-1@@ -77,7 +77,7 @@ extern const char *gpiochip_is_requested(struct gpio_chip *chip,extern int __must_check gpiochip_reserve (int start,int ngpio);/* add/remove chips */ -extern int gpiochip_add (struct gpio_chip *chip); +extern int __must_check gpiochip_add (struct gpio_chip *chip);extern int __must_check gpiochip_remove (struct gpio_chip *chip);Signature · 1 line
1.5.2.2
Quoted 6 lines · David Brownell
Quoted 2 lines · Anton Vorontsov
Signed-off-by Anton Vorontsov <[email protected]>I'm curious why you want to change this. When it fails, a
KERN_ERRmessage is emitted ...
Ah, I missed that the gpiochip_add() issuing pr_err(), not pr_debug().
So, please just ignore that patch. Though, printing errors from the generic functions is confusing, you never know when your code or the function you call should print the error. And usually generic functions tend to be silent...
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Quoted 11 lines · Anton Vorontsov
Quoted 6 lines · David Brownell
Quoted 2 lines · Anton Vorontsov
Signed-off-by Anton Vorontsov <[email protected]>I'm curious why you want to change this. When it fails, a
KERN_ERRmessage is emitted ...Ah, I missed that the
gpiochip_add()issuingpr_err(), notpr_debug().So, please just ignore that patch.
OK ...
Quoted 4 lines
Though, printing errors from the
generic functions is confusing, you never know when your code or the function you call should print the error. And usually generic functions tend to be silent...
The logic in this case was that I don't really expect platform setup code to check this stuff any more than it checks IRQ setup. But, as the comment says, if platform setup code goofs, it can cause un-bootable systems. Ergo the pr_err().
It could be argued either way; that's just what I did, way back when I noticed the issue.
- Dave