[PATCH v1 1/6] gpiolib: acpi: Assign polarity when call acpi_populate_gpio_lookup()

[PATCH v1 1/6] gpiolib: acpi: Assign polarity when call acpi_populate_gpio_lookup()

[Andy Shevchenko]

There is no need, since we preserve firmware settings, to override
polarity for GpioInt() resources.

While Documentation/gpio-properties.txt refers to any from GpioIo() /
GpioInt() resources, the active_low flag has been introduced to fill the
gap only for GpioIo() which lacks of that information.

Moreover, in case of GpioInt() existed solution was broken anyway, it
overrides only in one direction, i.e. from 0 to 1, otherwise it would be
still 1 as defined in the resource macro.

So, move the assignment to a right place and forbid to (semi-)override
polarity for GpioInt() type of resources.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index eb4528c87c0b..2a85d27eb028 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -531,8 +531,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 			lookup->info.triggering = agpio->triggering;
 		} else {
 			lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio);
+			lookup->info.polarity = lookup->active_low;
 		}
-
 	}
 
 	return 1;
@@ -557,11 +557,8 @@ static int acpi_gpio_resource_lookup(struct acpi_gpio_lookup *lookup,
 	if (!lookup->desc)
 		return -ENOENT;
 
-	if (info) {
+	if (info)
 		*info = lookup->info;
-		if (lookup->active_low)
-			info->polarity = lookup->active_low;
-	}
 	return 0;
 }
 
-- 
2.14.2

[PATCH v1 2/6] gpiolib: acpi: Don't contaminate return parameter in case of error

[Andy Shevchenko]

If error occurs, leave lookup parameter untouched.

There is no functional change, since all current callers just bail out
in case of error without using the assigned pieces.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 2a85d27eb028..f640445099ab 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -585,7 +585,6 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 	 * The property was found and resolved, so need to lookup the GPIO based
 	 * on returned args.
 	 */
-	lookup->adev = args.adev;
 	if (args.nargs != 3)
 		return -EPROTO;
 
@@ -593,6 +592,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 	lookup->pin_index = args.args[1];
 	lookup->active_low = !!args.args[2];
 
+	lookup->adev = args.adev;
 	return 0;
 }
 
-- 
2.14.2

[PATCH v1 3/6] gpiolib: acpi: Move adev member to struct acpi_gpio_info

[Andy Shevchenko]

The further improvements are based on this change since
struct acpi_gpio_lookup is not available in some cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 10 +++++-----
 drivers/gpio/gpiolib.h      |  2 ++
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index f640445099ab..7d0664a3d1b9 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -494,7 +494,6 @@ struct acpi_gpio_lookup {
 	int index;
 	int pin_index;
 	bool active_low;
-	struct acpi_device *adev;
 	struct gpio_desc *desc;
 	int n;
 };
@@ -541,12 +540,13 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 static int acpi_gpio_resource_lookup(struct acpi_gpio_lookup *lookup,
 				     struct acpi_gpio_info *info)
 {
+	struct acpi_device *adev = lookup->info.adev;
 	struct list_head res_list;
 	int ret;
 
 	INIT_LIST_HEAD(&res_list);
 
-	ret = acpi_dev_get_resources(lookup->adev, &res_list,
+	ret = acpi_dev_get_resources(adev, &res_list,
 				     acpi_populate_gpio_lookup,
 				     lookup);
 	if (ret < 0)
@@ -592,7 +592,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 	lookup->pin_index = args.args[1];
 	lookup->active_low = !!args.args[2];
 
-	lookup->adev = args.adev;
+	lookup->info.adev = args.adev;
 	return 0;
 }
 
@@ -640,11 +640,11 @@ static struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
 			return ERR_PTR(ret);
 
 		dev_dbg(&adev->dev, "GPIO: _DSD returned %s %d %d %u\n",
-			dev_name(&lookup.adev->dev), lookup.index,
+			dev_name(&lookup.info.adev->dev), lookup.index,
 			lookup.pin_index, lookup.active_low);
 	} else {
 		dev_dbg(&adev->dev, "GPIO: looking up %d in _CRS\n", index);
-		lookup.adev = adev;
+		lookup.info.adev = adev;
 	}
 
 	ret = acpi_gpio_resource_lookup(&lookup, info);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index af48322839c3..e8d0bfa57288 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -75,12 +75,14 @@ struct gpio_device {
 
 /**
  * struct acpi_gpio_info - ACPI GPIO specific information
+ * @adev: reference to ACPI device which consumes GPIO resource
  * @flags: GPIO initialization flags
  * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
  * @polarity: interrupt polarity as provided by ACPI
  * @triggering: triggering type as provided by ACPI
  */
 struct acpi_gpio_info {
+	struct acpi_device *adev;
 	enum gpiod_flags flags;
 	bool gpioint;
 	int polarity;
-- 
2.14.2

[PATCH v1 4/6] gpiolib: acpi: Consolidate debug output in acpi_gpio_update_gpiod_flags()

[Andy Shevchenko]

We have the duplicated debug strings printed whenever
acpi_gpio_update_gpiod_flags() fails. Instead of doing this by callers,
move the debug output inside function.

In one case convert almost useless pr_debug() to dev_dbg() where
actual consumer of GPIO resource is disclosed.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 23 ++++++++++++++++-------
 drivers/gpio/gpiolib.c      |  4 +---
 drivers/gpio/gpiolib.h      |  4 ++--
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 7d0664a3d1b9..88518afa26bd 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -461,8 +461,8 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
 	}
 }
 
-int
-acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
+static int
+__acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
 {
 	int ret = 0;
 
@@ -489,6 +489,19 @@ acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
 	return ret;
 }
 
+int
+acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, struct acpi_gpio_info *info)
+{
+	struct device *dev = &info->adev->dev;
+	int ret;
+
+	ret = __acpi_gpio_update_gpiod_flags(flags, info->flags);
+	if (ret)
+		dev_dbg(dev, "Override GPIO initialization flags\n");
+
+	return ret;
+}
+
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
@@ -661,7 +674,6 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
 	struct acpi_gpio_info info;
 	struct gpio_desc *desc;
 	char propname[32];
-	int err;
 	int i;
 
 	/* Try first from _DSD */
@@ -700,10 +712,7 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
 	if (info.polarity == GPIO_ACTIVE_LOW)
 		*lookupflags |= GPIO_ACTIVE_LOW;
 
-	err = acpi_gpio_update_gpiod_flags(dflags, info.flags);
-	if (err)
-		dev_dbg(dev, "Override GPIO initialization flags\n");
-
+	acpi_gpio_update_gpiod_flags(dflags, &info);
 	return desc;
 }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 029ad1a0a0d5..ab464f29d41a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3584,9 +3584,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 		desc = acpi_node_get_gpiod(fwnode, propname, index, &info);
 		if (!IS_ERR(desc)) {
 			active_low = info.polarity == GPIO_ACTIVE_LOW;
-			ret = acpi_gpio_update_gpiod_flags(&dflags, info.flags);
-			if (ret)
-				pr_debug("Override GPIO initialization flags\n");
+			acpi_gpio_update_gpiod_flags(&dflags, &info);
 		}
 	}
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index e8d0bfa57288..b23aeaf5ac9e 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -126,7 +126,7 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
 void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
 
 int acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags,
-				 enum gpiod_flags update);
+				 struct acpi_gpio_info *info);
 
 struct gpio_desc *acpi_find_gpio(struct device *dev,
 				 const char *con_id,
@@ -151,7 +151,7 @@ static inline void
 acpi_gpiochip_free_interrupts(struct gpio_chip *chip) { }
 
 static inline int
-acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
+acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, struct acpi_gpio_info *info)
 {
 	return 0;
 }
-- 
2.14.2

[PATCH v1 5/6] gpiolib: acpi: Add quirks field to struct acpi_gpio_mapping

[Andy Shevchenko]

Some broken ACPI tables might require quirks in the OS.
Introduce quirks field in struct acpi_gpio_mapping.

Propagate them to struct acpi_gpio_info for further use.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 10 ++++++++--
 drivers/gpio/gpiolib.h      |  2 ++
 include/linux/acpi.h        |  1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 88518afa26bd..711f64b9dd30 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -414,7 +414,8 @@ EXPORT_SYMBOL_GPL(devm_acpi_dev_remove_driver_gpios);
 
 static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
 				      const char *name, int index,
-				      struct acpi_reference_args *args)
+				      struct acpi_reference_args *args,
+				      unsigned int *quirks)
 {
 	const struct acpi_gpio_mapping *gm;
 
@@ -430,6 +431,8 @@ static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
 			args->args[1] = par->line_index;
 			args->args[2] = par->active_low;
 			args->nargs = 3;
+
+			*quirks = gm->quirks;
 			return true;
 		}
 
@@ -580,6 +583,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 				     struct acpi_gpio_lookup *lookup)
 {
 	struct acpi_reference_args args;
+	unsigned int quirks = 0;
 	int ret;
 
 	memset(&args, 0, sizeof(args));
@@ -591,7 +595,8 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 		if (!adev)
 			return ret;
 
-		if (!acpi_get_driver_gpio_data(adev, propname, index, &args))
+		if (!acpi_get_driver_gpio_data(adev, propname, index, &args,
+					       &quirks))
 			return ret;
 	}
 	/*
@@ -606,6 +611,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 	lookup->active_low = !!args.args[2];
 
 	lookup->info.adev = args.adev;
+	lookup->info.quirks = quirks;
 	return 0;
 }
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b23aeaf5ac9e..cd4622863fe1 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -80,6 +80,7 @@ struct gpio_device {
  * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
  * @polarity: interrupt polarity as provided by ACPI
  * @triggering: triggering type as provided by ACPI
+ * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
  */
 struct acpi_gpio_info {
 	struct acpi_device *adev;
@@ -87,6 +88,7 @@ struct acpi_gpio_info {
 	bool gpioint;
 	int polarity;
 	int triggering;
+	unsigned int quirks;
 };
 
 /* gpio suffixes used for ACPI and device tree lookup */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index dc1ebfeeb5ec..25fe77fccea0 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -978,6 +978,7 @@ struct acpi_gpio_mapping {
 	const char *name;
 	const struct acpi_gpio_params *data;
 	unsigned int size;
+	unsigned int quirks;
 };
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_GPIOLIB)
-- 
2.14.2

[PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Andy Shevchenko]

Allow to relax IoRestriction for certain cases.

One of the use case is incorrectly cooked ACPI table where interrupt pin is
defined with GpioIo() macro with IoRestrictionOutputOnly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 13 ++++++++++---
 include/linux/acpi.h        |  4 ++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 711f64b9dd30..430a1475212d 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -496,11 +496,18 @@ int
 acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, struct acpi_gpio_info *info)
 {
 	struct device *dev = &info->adev->dev;
+	enum gpiod_flags old = *flags;
 	int ret;
 
-	ret = __acpi_gpio_update_gpiod_flags(flags, info->flags);
-	if (ret)
-		dev_dbg(dev, "Override GPIO initialization flags\n");
+	ret = __acpi_gpio_update_gpiod_flags(&old, info->flags);
+	if (info->quirks & ACPI_GPIO_QUIRK_NO_IO_RESTRICTION) {
+		if (ret)
+			dev_warn(dev, FW_BUG "GPIO not in correct mode, fixing\n");
+	} else {
+		if (ret)
+			dev_dbg(dev, "Override GPIO initialization flags\n");
+		*flags = old;
+	}
 
 	return ret;
 }
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 25fe77fccea0..06b6eb775115 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -978,6 +978,10 @@ struct acpi_gpio_mapping {
 	const char *name;
 	const struct acpi_gpio_params *data;
 	unsigned int size;
+
+/* Ignore IoRestriction field */
+#define ACPI_GPIO_QUIRK_NO_IO_RESTRICTION	BIT(0)
+
 	unsigned int quirks;
 };
 
-- 
2.14.2

Re: [PATCH v1 1/6] gpiolib: acpi: Assign polarity when call acpi_populate_gpio_lookup()

[Mika Westerberg]

On Fri, Nov 10, 2017 at 03:40:28PM +0200, Andy Shevchenko wrote:
> There is no need, since we preserve firmware settings, to override
> polarity for GpioInt() resources.
> 
> While Documentation/gpio-properties.txt refers to any from GpioIo() /
> GpioInt() resources, the active_low flag has been introduced to fill the
> gap only for GpioIo() which lacks of that information.
> 
> Moreover, in case of GpioInt() existed solution was broken anyway, it
> overrides only in one direction, i.e. from 0 to 1, otherwise it would be
> still 1 as defined in the resource macro.
> 
> So, move the assignment to a right place and forbid to (semi-)override
> polarity for GpioInt() type of resources.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 2/6] gpiolib: acpi: Don't contaminate return parameter in case of error

[Mika Westerberg]

On Fri, Nov 10, 2017 at 03:40:29PM +0200, Andy Shevchenko wrote:
> If error occurs, leave lookup parameter untouched.
> 
> There is no functional change, since all current callers just bail out
> in case of error without using the assigned pieces.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 3/6] gpiolib: acpi: Move adev member to struct acpi_gpio_info

[Mika Westerberg]

On Fri, Nov 10, 2017 at 03:40:30PM +0200, Andy Shevchenko wrote:
> The further improvements are based on this change since
> struct acpi_gpio_lookup is not available in some cases.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 4/6] gpiolib: acpi: Consolidate debug output in acpi_gpio_update_gpiod_flags()

[Mika Westerberg]

On Fri, Nov 10, 2017 at 03:40:31PM +0200, Andy Shevchenko wrote:
> We have the duplicated debug strings printed whenever
> acpi_gpio_update_gpiod_flags() fails. Instead of doing this by callers,
> move the debug output inside function.
> 
> In one case convert almost useless pr_debug() to dev_dbg() where
> actual consumer of GPIO resource is disclosed.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 5/6] gpiolib: acpi: Add quirks field to struct acpi_gpio_mapping

[Mika Westerberg]

On Fri, Nov 10, 2017 at 03:40:32PM +0200, Andy Shevchenko wrote:
> Some broken ACPI tables might require quirks in the OS.
> Introduce quirks field in struct acpi_gpio_mapping.
> 
> Propagate them to struct acpi_gpio_info for further use.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Mika Westerberg]

On Fri, Nov 10, 2017 at 03:40:33PM +0200, Andy Shevchenko wrote:
> Allow to relax IoRestriction for certain cases.
> 
> One of the use case is incorrectly cooked ACPI table where interrupt pin is
> defined with GpioIo() macro with IoRestrictionOutputOnly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I think you should include user of this quirk in the patch series as
well. Otherwise it is pretty pointless to add random quirks without
real issues they are supposed to solve ;-)

Anyway looks good to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Andy Shevchenko]

On Mon, 2017-11-13 at 13:55 +0200, Mika Westerberg wrote:
> On Fri, Nov 10, 2017 at 03:40:33PM +0200, Andy Shevchenko wrote:
> > Allow to relax IoRestriction for certain cases.
> > 
> > One of the use case is incorrectly cooked ACPI table where interrupt
> > pin is
> > defined with GpioIo() macro with IoRestrictionOutputOnly.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I think you should include user of this quirk in the patch series as
> well. Otherwise it is pretty pointless to add random quirks without
> real issues they are supposed to solve ;-)

Something like below?

--- a/drivers/extcon/extcon-intel-int3496.c
+++ b/drivers/extcon/extcon-intel-int3496.c
@@ -50,7 +50,7 @@ static const struct acpi_gpio_params vbus_gpios = {
INT3496_GPIO_VBUS_EN, 0, fal
 static const struct acpi_gpio_params mux_gpios = {
INT3496_GPIO_USB_MUX, 0, false };
 
 static const struct acpi_gpio_mapping acpi_int3496_default_gpios[] = {
-       { "id-gpios", &id_gpios, 1 },
+       { "id-gpios", &id_gpios, 1, ACPI_GPIO_QUIRK_NO_IO_RESTRICTION },
        { "vbus-gpios", &vbus_gpios, 1 },
        { "mux-gpios", &mux_gpios, 1 },
        { },
@@ -112,9 +112,6 @@ static int int3496_probe(struct platform_device
*pdev)
                ret = PTR_ERR(data->gpio_usb_id);
                dev_err(dev, "can't request USB ID GPIO: %d\n", ret);
                return ret;
-       } else if (gpiod_get_direction(data->gpio_usb_id) !=
GPIOF_DIR_IN) {
-               dev_warn(dev, FW_BUG "USB ID GPIO not in input mode,
fixing\n");
-               gpiod_direction_input(data->gpio_usb_id);
        }

> 
> Anyway looks good to me,
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Mika Westerberg]

On Mon, Nov 13, 2017 at 03:10:48PM +0200, Andy Shevchenko wrote:
> On Mon, 2017-11-13 at 13:55 +0200, Mika Westerberg wrote:
> > On Fri, Nov 10, 2017 at 03:40:33PM +0200, Andy Shevchenko wrote:
> > > Allow to relax IoRestriction for certain cases.
> > > 
> > > One of the use case is incorrectly cooked ACPI table where interrupt
> > > pin is
> > > defined with GpioIo() macro with IoRestrictionOutputOnly.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > I think you should include user of this quirk in the patch series as
> > well. Otherwise it is pretty pointless to add random quirks without
> > real issues they are supposed to solve ;-)
> 
> Something like below?

Yes, but I would also add comment on top of the added line explaining
why this particular quirk is needed in this particular system.

> --- a/drivers/extcon/extcon-intel-int3496.c
> +++ b/drivers/extcon/extcon-intel-int3496.c
> @@ -50,7 +50,7 @@ static const struct acpi_gpio_params vbus_gpios = {
> INT3496_GPIO_VBUS_EN, 0, fal
>  static const struct acpi_gpio_params mux_gpios = {
> INT3496_GPIO_USB_MUX, 0, false };
>  
>  static const struct acpi_gpio_mapping acpi_int3496_default_gpios[] = {
> -       { "id-gpios", &id_gpios, 1 },
> +       { "id-gpios", &id_gpios, 1, ACPI_GPIO_QUIRK_NO_IO_RESTRICTION },
>         { "vbus-gpios", &vbus_gpios, 1 },
>         { "mux-gpios", &mux_gpios, 1 },
>         { },
> @@ -112,9 +112,6 @@ static int int3496_probe(struct platform_device
> *pdev)
>                 ret = PTR_ERR(data->gpio_usb_id);
>                 dev_err(dev, "can't request USB ID GPIO: %d\n", ret);
>                 return ret;
> -       } else if (gpiod_get_direction(data->gpio_usb_id) !=
> GPIOF_DIR_IN) {
> -               dev_warn(dev, FW_BUG "USB ID GPIO not in input mode,
> fixing\n");
> -               gpiod_direction_input(data->gpio_usb_id);
>         }
> 
> > 
> > Anyway looks good to me,
> > 
> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Thanks!
> 
> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

Re: [PATCH v1 1/6] gpiolib: acpi: Assign polarity when call acpi_populate_gpio_lookup()

[Rafael J. Wysocki]

On Friday, November 10, 2017 2:40:28 PM CET Andy Shevchenko wrote:
> There is no need, since we preserve firmware settings, to override
> polarity for GpioInt() resources.
> 
> While Documentation/gpio-properties.txt refers to any from GpioIo() /
> GpioInt() resources, the active_low flag has been introduced to fill the
> gap only for GpioIo() which lacks of that information.
> 
> Moreover, in case of GpioInt() existed solution was broken anyway, it
> overrides only in one direction, i.e. from 0 to 1, otherwise it would be
> still 1 as defined in the resource macro.
> 
> So, move the assignment to a right place and forbid to (semi-)override
> polarity for GpioInt() type of resources.

I'm assuming that this series is targeted at the GPIO subsystem.

Please let me know if that's not the case.

Thanks,
Rafael

Re: [PATCH v1 1/6] gpiolib: acpi: Assign polarity when call acpi_populate_gpio_lookup()

[Andy Shevchenko]

On Sat, 2017-11-18 at 15:55 +0100, Rafael J. Wysocki wrote:
> On Friday, November 10, 2017 2:40:28 PM CET Andy Shevchenko wrote:

> I'm assuming that this series is targeted at the GPIO subsystem.

That's correct.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/6] gpiolib: acpi: Assign polarity when call acpi_populate_gpio_lookup()

[Linus Walleij]

On Fri, Nov 10, 2017 at 2:40 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> There is no need, since we preserve firmware settings, to override
> polarity for GpioInt() resources.
>
> While Documentation/gpio-properties.txt refers to any from GpioIo() /
> GpioInt() resources, the active_low flag has been introduced to fill the
> gap only for GpioIo() which lacks of that information.
>
> Moreover, in case of GpioInt() existed solution was broken anyway, it
> overrides only in one direction, i.e. from 0 to 1, otherwise it would be
> still 1 as defined in the resource macro.
>
> So, move the assignment to a right place and forbid to (semi-)override
> polarity for GpioInt() type of resources.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v1 2/6] gpiolib: acpi: Don't contaminate return parameter in case of error

[Linus Walleij]

On Fri, Nov 10, 2017 at 2:40 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> If error occurs, leave lookup parameter untouched.
>
> There is no functional change, since all current callers just bail out
> in case of error without using the assigned pieces.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v1 3/6] gpiolib: acpi: Move adev member to struct acpi_gpio_info

[Linus Walleij]

On Fri, Nov 10, 2017 at 2:40 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The further improvements are based on this change since
> struct acpi_gpio_lookup is not available in some cases.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v1 4/6] gpiolib: acpi: Consolidate debug output in acpi_gpio_update_gpiod_flags()

[Linus Walleij]

On Fri, Nov 10, 2017 at 2:40 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> We have the duplicated debug strings printed whenever
> acpi_gpio_update_gpiod_flags() fails. Instead of doing this by callers,
> move the debug output inside function.
>
> In one case convert almost useless pr_debug() to dev_dbg() where
> actual consumer of GPIO resource is disclosed.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v1 5/6] gpiolib: acpi: Add quirks field to struct acpi_gpio_mapping

[Linus Walleij]

On Fri, Nov 10, 2017 at 2:40 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Some broken ACPI tables might require quirks in the OS.
> Introduce quirks field in struct acpi_gpio_mapping.
>
> Propagate them to struct acpi_gpio_info for further use.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Linus Walleij]

On Fri, Nov 10, 2017 at 2:40 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Allow to relax IoRestriction for certain cases.
>
> One of the use case is incorrectly cooked ACPI table where interrupt pin is
> defined with GpioIo() macro with IoRestrictionOutputOnly.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Linus Walleij]

On Mon, Nov 13, 2017 at 2:10 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2017-11-13 at 13:55 +0200, Mika Westerberg wrote:
>> On Fri, Nov 10, 2017 at 03:40:33PM +0200, Andy Shevchenko wrote:
>> > Allow to relax IoRestriction for certain cases.
>> >
>> > One of the use case is incorrectly cooked ACPI table where interrupt
>> > pin is
>> > defined with GpioIo() macro with IoRestrictionOutputOnly.
>> >
>> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
>> I think you should include user of this quirk in the patch series as
>> well. Otherwise it is pretty pointless to add random quirks without
>> real issues they are supposed to solve ;-)
>
> Something like below?

I bet this patch is already somewhere in my mailbox else just send
it separately. All these patches are applied.

Yours,
Linus Walleij

Re: [PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Andy Shevchenko]

On Wed, 2017-11-29 at 13:35 +0100, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:10 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, 2017-11-13 at 13:55 +0200, Mika Westerberg wrote:

> > > I think you should include user of this quirk in the patch series
> > > as
> > > well. Otherwise it is pretty pointless to add random quirks
> > > without
> > > real issues they are supposed to solve ;-)
> > 
> > Something like below?
> 
> I bet this patch is already somewhere in my mailbox else just send
> it separately. All these patches are applied.

I'm going to send them (there are two affected users) next week. It
would be nice if you can prepare immutable branch in case the subsystems
would like to merge patches via their respective trees.

Thanks for applying!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Linus Walleij]

On Wed, Nov 29, 2017 at 2:41 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> It
> would be nice if you can prepare immutable branch in case the subsystems
> would like to merge patches via their respective trees.

I have created a branch named "ib-gpio-acpi-quirks" in my tree
with the six patches from you, then merged that to my devel
branch.

You can point subsystem maintainers to that one.

Yours,
Linus Walleij

Re: [PATCH v1 6/6] gpiolib: acpi: Introduce NO_RESTRICTION quirk

[Andy Shevchenko]

On Thu, 2017-11-30 at 10:57 +0100, Linus Walleij wrote:
> On Wed, Nov 29, 2017 at 2:41 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > It
> > would be nice if you can prepare immutable branch in case the
> > subsystems
> > would like to merge patches via their respective trees.
> 
> I have created a branch named "ib-gpio-acpi-quirks" in my tree
> with the six patches from you, then merged that to my devel
> branch.
> 
> You can point subsystem maintainers to that one.
> 

Thanks!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy