Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Alexandre Courbot]

On Mon, Nov 17, 2014 at 6:09 PM, Uwe Kleine-KÃnig
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> Hello,
>
> On Wed, Feb 13, 2013 at 04:03:00PM +0900, Alexandre Courbot wrote:
>> From: Alexandre Courbot <acourbot@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/