Messages in this thread | | | From | Alexandre Courbot <> | Date | Wed, 19 Nov 2014 17:33:42 +0900 | Subject | Re: [PATCH 2/4] gpiolib: use const parameters when possible |
| |
On Mon, Nov 17, 2014 at 6:09 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello, > > On Wed, Feb 13, 2013 at 04:03:00PM +0900, Alexandre Courbot wrote: >> From: Alexandre Courbot <acourbot@nvidia.com> >> >> Constify descriptor parameter of gpiod_* functions for those that >> should obviously not modify it. This includes value or direction get, >> cansleep, and IRQ number query. >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > This patch is applied as def634338d3ffb32fbe9b0a2d70cc24ef909cd4f since > v3.9-rc2. > >> drivers/gpio/gpiolib.c | 29 ++++++++++++++++------------- >> 1 file changed, 16 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index 8a2cf9c..ad6df6b 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -207,7 +208,7 @@ static int gpiochip_find_base(int ngpio) >> } >> >> /* caller ensures gpio is valid and requested, chip->get_direction may sleep */ >> -static int gpiod_get_direction(struct gpio_desc *desc) >> +static int gpiod_get_direction(const struct gpio_desc *desc) >> { >> struct gpio_chip *chip; >> unsigned offset; >> @@ -223,11 +224,13 @@ static int gpiod_get_direction(struct gpio_desc *desc) >> if (status > 0) { >> /* GPIOF_DIR_IN, or other positive */ >> status = 1; >> - clear_bit(FLAG_IS_OUT, &desc->flags); >> + /* FLAG_IS_OUT is just a cache of the result of get_direction(), >> + * so it does not affect constness per se */ >> + clear_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags); > I think this is worse than not having gpiod_get_direction take a const > descriptor.
Yeah, this kind of sucks but ultimately gpiod_get_direction() is without side-effect to the caller and the const parameter helps highlighting that fact.
> I don't know an example for this particular case where this could break, > but usually casting away a const is bad and can break in various ways. > > Maybe even drop the public function gpiod_get_direction completely. > As of next-20141114 there are only two users of this function (apart > from drivers/gpio/gpiolib*) and both are wrong (as discussed in > http://thread.gmane.org/gmane.linux.serial/16808/focus=4783).
Or maybe gpiod_direction_*() should register the last set direction to be used by gpiod_get_direction() if the controller does not implement the get_direction op? On the other hand callers can also track the direction themselves (since they presumably set it), so this function is really only useful for users who want to read the *hardware* state of a GPIO. Which might still be a useful feature?
| |