diff mbox series

[v2] gpiolib-acpi: Preserve non direction flags when updating gpiod_flags

Message ID 20181231205521.6678-1-hdegoede@redhat.com
State New
Headers show
Series [v2] gpiolib-acpi: Preserve non direction flags when updating gpiod_flags | expand

Commit Message

Hans de Goede Dec. 31, 2018, 8:55 p.m. UTC 
__acpi_gpio_update_gpiod_flags purpose is to make the gpiod_flags used
when requesting a GPIO match the restrictions from the ACPI resource,
as stored in acpi_gpio_info.flags.

But acpi_gpio_info.flags only contains direction info, and the
requester may have passed in special non-direction flags like
GPIOD_FLAGS_BIT_NONEXCLUSIVE, which we currently clobber.

This commit modifies __acpi_gpio_update_gpiod_flags to preserve these
special flags, so that a requested of an ACPI GPIO can e.g. pass
GPIOD_FLAGS_BIT_NONEXCLUSIV and have it work as intended.

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Fold 2 statements into 1 as suggested by Andy
---
 drivers/gpio/gpiolib-acpi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Jan. 7, 2019, 3:29 p.m. UTC | #1 
On Mon, Dec 31, 2018 at 09:55:21PM +0100, Hans de Goede wrote:
> __acpi_gpio_update_gpiod_flags purpose is to make the gpiod_flags used
> when requesting a GPIO match the restrictions from the ACPI resource,
> as stored in acpi_gpio_info.flags.
> 
> But acpi_gpio_info.flags only contains direction info, and the
> requester may have passed in special non-direction flags like
> GPIOD_FLAGS_BIT_NONEXCLUSIVE, which we currently clobber.
> 
> This commit modifies __acpi_gpio_update_gpiod_flags to preserve these
> special flags, so that a requested of an ACPI GPIO can e.g. pass
> GPIOD_FLAGS_BIT_NONEXCLUSIV and have it work as intended.
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Fold 2 statements into 1 as suggested by Andy
> ---
>  drivers/gpio/gpiolib-acpi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 091a3784720d..800fb0901283 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -469,6 +469,9 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
>  static int
>  __acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
>  {
> +	const enum gpiod_flags mask =
> +		GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT |
> +		GPIOD_FLAGS_BIT_DIR_VAL;

I'm just wondering if we can introduce in consumer.h something like

#define GPIOD_FLAGS_BIT_DIR_MASK	GENMASK(2, 0)

and use it here.

>  	int ret = 0;
>  
>  	/*
> @@ -489,7 +492,7 @@ __acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
>  		if (((*flags & GPIOD_FLAGS_BIT_DIR_SET) && (diff & GPIOD_FLAGS_BIT_DIR_OUT)) ||
>  		    ((*flags & GPIOD_FLAGS_BIT_DIR_OUT) && (diff & GPIOD_FLAGS_BIT_DIR_VAL)))
>  			ret = -EINVAL;
> -		*flags = update;
> +		*flags = (*flags & ~mask) | (update & mask);
>  	}
>  	return ret;
>  }
> -- 
> 2.19.1
>
Hans de Goede Jan. 7, 2019, 3:48 p.m. UTC | #2 
Hi,

On 07-01-19 16:29, Andy Shevchenko wrote:
> On Mon, Dec 31, 2018 at 09:55:21PM +0100, Hans de Goede wrote:
>> __acpi_gpio_update_gpiod_flags purpose is to make the gpiod_flags used
>> when requesting a GPIO match the restrictions from the ACPI resource,
>> as stored in acpi_gpio_info.flags.
>>
>> But acpi_gpio_info.flags only contains direction info, and the
>> requester may have passed in special non-direction flags like
>> GPIOD_FLAGS_BIT_NONEXCLUSIVE, which we currently clobber.
>>
>> This commit modifies __acpi_gpio_update_gpiod_flags to preserve these
>> special flags, so that a requested of an ACPI GPIO can e.g. pass
>> GPIOD_FLAGS_BIT_NONEXCLUSIV and have it work as intended.
>>
>> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Fold 2 statements into 1 as suggested by Andy
>> ---
>>   drivers/gpio/gpiolib-acpi.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
>> index 091a3784720d..800fb0901283 100644
>> --- a/drivers/gpio/gpiolib-acpi.c
>> +++ b/drivers/gpio/gpiolib-acpi.c
>> @@ -469,6 +469,9 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
>>   static int
>>   __acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
>>   {
>> +	const enum gpiod_flags mask =
>> +		GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT |
>> +		GPIOD_FLAGS_BIT_DIR_VAL;
> 
> I'm just wondering if we can introduce in consumer.h something like
> 
> #define GPIOD_FLAGS_BIT_DIR_MASK	GENMASK(2, 0)
> 
> and use it here.

That seems like more of a question for Linus then for me ...

My 2 cents: I'm not sure if there are other users needing this. Also we
will still need the mask helper const to keep things within 80 chars
(or split things into 2 lines again).

Note the ASoC changes which need this to work properly have landed in -next:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=0d3e91da0750835cfd5c16487ffb3cdd752ea53a

See the chunk which applies starting at line 323 for a longish
comment explaining the firmware brokenness which causes the
need to use the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag with an ACPI gpio.

Given this and that people seem to be ok with this gpiolib-acpi patch
as is, can we maybe merge v2 and then do a possible conversion to
GPIOD_FLAGS_BIT_DIR_MASK later?

Regards,

Hans




> 
>>   	int ret = 0;
>>   
>>   	/*
>> @@ -489,7 +492,7 @@ __acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
>>   		if (((*flags & GPIOD_FLAGS_BIT_DIR_SET) && (diff & GPIOD_FLAGS_BIT_DIR_OUT)) ||
>>   		    ((*flags & GPIOD_FLAGS_BIT_DIR_OUT) && (diff & GPIOD_FLAGS_BIT_DIR_VAL)))
>>   			ret = -EINVAL;
>> -		*flags = update;
>> +		*flags = (*flags & ~mask) | (update & mask);
>>   	}
>>   	return ret;
>>   }
>> -- 
>> 2.19.1
>>
>
Andy Shevchenko Jan. 7, 2019, 4:12 p.m. UTC | #3 
On Mon, Jan 07, 2019 at 04:48:56PM +0100, Hans de Goede wrote:

> Given this and that people seem to be ok with this gpiolib-acpi patch
> as is, can we maybe merge v2 and then do a possible conversion to
> GPIOD_FLAGS_BIT_DIR_MASK later?

Yes, it was just a side observation which we may consider later.
Linus Walleij Jan. 10, 2019, 3:23 p.m. UTC | #4 
On Mon, Dec 31, 2018 at 9:55 PM Hans de Goede <hdegoede@redhat.com> wrote:

> __acpi_gpio_update_gpiod_flags purpose is to make the gpiod_flags used
> when requesting a GPIO match the restrictions from the ACPI resource,
> as stored in acpi_gpio_info.flags.
>
> But acpi_gpio_info.flags only contains direction info, and the
> requester may have passed in special non-direction flags like
> GPIOD_FLAGS_BIT_NONEXCLUSIVE, which we currently clobber.
>
> This commit modifies __acpi_gpio_update_gpiod_flags to preserve these
> special flags, so that a requested of an ACPI GPIO can e.g. pass
> GPIOD_FLAGS_BIT_NONEXCLUSIV and have it work as intended.
>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Patch applied for next for now.

Is this a fix that need to go into the -rcN?

In that case, is there an appropriate Fixes: tag?

Yours,
Linus Walleij
Hans de Goede Jan. 10, 2019, 3:25 p.m. UTC | #5 
Hi,

On 10-01-19 16:23, Linus Walleij wrote:
> On Mon, Dec 31, 2018 at 9:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> __acpi_gpio_update_gpiod_flags purpose is to make the gpiod_flags used
>> when requesting a GPIO match the restrictions from the ACPI resource,
>> as stored in acpi_gpio_info.flags.
>>
>> But acpi_gpio_info.flags only contains direction info, and the
>> requester may have passed in special non-direction flags like
>> GPIOD_FLAGS_BIT_NONEXCLUSIVE, which we currently clobber.
>>
>> This commit modifies __acpi_gpio_update_gpiod_flags to preserve these
>> special flags, so that a requested of an ACPI GPIO can e.g. pass
>> GPIOD_FLAGS_BIT_NONEXCLUSIV and have it work as intended.
>>
>> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Patch applied for next for now.
> 
> Is this a fix that need to go into the -rcN?

Not this is necessary for some GPIO bit-banging added to the
ASoC Intel-SST <-> ES8316 codec machine driver to actually work,
but that itself is in -next too, so having this in -next is fine.

Thanks,

Hans
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 091a3784720d..800fb0901283 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -469,6 +469,9 @@  acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
 static int
 __acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
 {
+	const enum gpiod_flags mask =
+		GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT |
+		GPIOD_FLAGS_BIT_DIR_VAL;
 	int ret = 0;
 
 	/*
@@ -489,7 +492,7 @@  __acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
 		if (((*flags & GPIOD_FLAGS_BIT_DIR_SET) && (diff & GPIOD_FLAGS_BIT_DIR_OUT)) ||
 		    ((*flags & GPIOD_FLAGS_BIT_DIR_OUT) && (diff & GPIOD_FLAGS_BIT_DIR_VAL)))
 			ret = -EINVAL;
-		*flags = update;
+		*flags = (*flags & ~mask) | (update & mask);
 	}
 	return ret;
 }