[PATCH 0/3] [v10] pinctrl: qcom: add support for sparse GPIOs

[PATCH 0/3] [v10] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

A series of patches that add support for GPIO maps that have holes in
them.  That is, even though a client driver has N consecutive GPIOs,
some are just unavailable for whatever reason, and the hardware should
not be accessed for those GPIOs.

Patch 1 reverts an old patch that triggers a get_direction of every
pin upon init, without attempting to request the pins first.  The
direction is already being queried when the pin is requested.

Patch 2 adds support to pinctrl-msm for "unavailable" GPIOs.

Patch 3 extends that support to pinctrl-qdf2xxx.  A recent ACPI change
on QDF2400 platforms blocks access to most pins, so the driver can only
register a subset.

This version drops the availability check in gpiolib, because it's no
necessary.  Instead, just having pinctrl-msm return -EACCES is enough
to block all unavailable GPIOs.  Patch 1 removes the only instance where
an unrequested GPIO is being accessed.

v10:
  Use driver_stuct to obtain ACPI match table entry

Timur Tabi (3):
  [v2] Revert "gpio: set up initial state from .get_direction()"
  [v8] pinctrl: qcom: disable GPIO groups with no pins
  [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

 drivers/gpio/gpiolib.c                 |  31 ++------
 drivers/pinctrl/qcom/pinctrl-msm.c     |  28 +++++--
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 134 +++++++++++++++++++++++++--------
 3 files changed, 133 insertions(+), 60 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/3] [v2] Revert "gpio: set up initial state from .get_direction()"

[Timur Tabi]

This reverts commit 72d3200061776264941be1b5a9bb8e926b3b30a5.

We cannot blindly query the direction of all GPIOs when the pins are
first registered.  The get_direction callback normally triggers a
read/write to hardware, but we shouldn't be touching the hardware for
an individual GPIO until after it's been properly claimed.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/gpio/gpiolib.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 641a5eb552cb..168dd831551d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1207,31 +1207,14 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
 		struct gpio_desc *desc = &gdev->descs[i];
 
 		desc->gdev = gdev;
-		/*
-		 * REVISIT: most hardware initializes GPIOs as inputs
-		 * (often with pullups enabled) so power usage is
-		 * minimized. Linux code should set the gpio direction
-		 * first thing; but until it does, and in case
-		 * chip->get_direction is not set, we may expose the
-		 * wrong direction in sysfs.
-		 */
-
-		if (chip->get_direction) {
-			/*
-			 * If we have .get_direction, set up the initial
-			 * direction flag from the hardware.
-			 */
-			int dir = chip->get_direction(chip, i);
 
-			if (!dir)
-				set_bit(FLAG_IS_OUT, &desc->flags);
-		} else if (!chip->direction_input) {
-			/*
-			 * If the chip lacks the .direction_input callback
-			 * we logically assume all lines are outputs.
-			 */
-			set_bit(FLAG_IS_OUT, &desc->flags);
-		}
+		/* REVISIT: most hardware initializes GPIOs as inputs (often
+		 * with pullups enabled) so power usage is minimized. Linux
+		 * code should set the gpio direction first thing; but until
+		 * it does, and in case chip->get_direction is not set, we may
+		 * expose the wrong direction in sysfs.
+		 */
+		desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
 	}
 
 #ifdef CONFIG_PINCTRL
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/3] [v2] Revert "gpio: set up initial state from .get_direction()"

[Stephen Boyd]

On 12/13, Timur Tabi wrote:
> This reverts commit 72d3200061776264941be1b5a9bb8e926b3b30a5.
> 
> We cannot blindly query the direction of all GPIOs when the pins are
> first registered.  The get_direction callback normally triggers a
> read/write to hardware, but we shouldn't be touching the hardware for
> an individual GPIO until after it's been properly claimed.
> 
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] [v8] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

pinctrl-msm only accepts an array of GPIOs from 0 to n-1, and it expects
each group to support have only one pin (npins == 1).

We can support "sparse" GPIO maps if we allow for some groups to have zero
pins (npins == 0).  These pins are "hidden" from the rest of the driver
and gpiolib.

Access to unavailable GPIOs is blocked via a request callback.  If the
requested GPIO is unavailable, -EACCES is returned, which prevents
further access to that GPIO.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7a960590ecaa..d45b4c2b5af1 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -507,6 +507,11 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 	};
 
 	g = &pctrl->soc->groups[offset];
+
+	/* If the GPIO group has no pins, then don't show it. */
+	if (!g->npins)
+		return;
+
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
 	is_out = !!(ctl_reg & BIT(g->oe_bit));
@@ -516,7 +521,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 
 	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
 	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
-	seq_printf(s, " %s", pulls[pull]);
+	seq_printf(s, " %s\n", pulls[pull]);
 }
 
 static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -524,23 +529,36 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned gpio = chip->base;
 	unsigned i;
 
-	for (i = 0; i < chip->ngpio; i++, gpio++) {
+	for (i = 0; i < chip->ngpio; i++, gpio++)
 		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
-		seq_puts(s, "\n");
-	}
 }
 
 #else
 #define msm_gpio_dbg_show NULL
 #endif
 
+/*
+ * If the requested GPIO has no pins, then treat it as unavailable.
+ * Otherwise, call the standard request function.
+ */
+static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct msm_pingroup *g = &pctrl->soc->groups[offset];
+
+	if (!g->npins)
+		return -EACCES;
+
+	return gpiochip_generic_request(chip, offset);
+}
+
 static const struct gpio_chip msm_gpio_template = {
 	.direction_input  = msm_gpio_direction_input,
 	.direction_output = msm_gpio_direction_output,
 	.get_direction    = msm_gpio_get_direction,
 	.get              = msm_gpio_get,
 	.set              = msm_gpio_set,
-	.request          = gpiochip_generic_request,
+	.request          = msm_gpio_request,
 	.free             = gpiochip_generic_free,
 	.dbg_show         = msm_gpio_dbg_show,
 };
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/3] [v8] pinctrl: qcom: disable GPIO groups with no pins

[Stephen Boyd]

On 12/13, Timur Tabi wrote:
> pinctrl-msm only accepts an array of GPIOs from 0 to n-1, and it expects
> each group to support have only one pin (npins == 1).
> 
> We can support "sparse" GPIO maps if we allow for some groups to have zero
> pins (npins == 0).  These pins are "hidden" from the rest of the driver
> and gpiolib.
> 
> Access to unavailable GPIOs is blocked via a request callback.  If the
> requested GPIO is unavailable, -EACCES is returned, which prevents
> further access to that GPIO.
> 
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

Newer versions of the firmware for the Qualcomm Datacenter Technologies
QDF2400 restricts access to a subset of the GPIOs on the TLMM.  To
prevent older kernels from accidentally accessing the restricted GPIOs,
we change the ACPI HID for the TLMM block from QCOM8001 to QCOM8002,
and introduce a new property "gpios".  This property is an array of
specific GPIOs that are accessible.  When an older kernel boots on
newer (restricted) firmware, it will fail to probe.

To implement the sparse GPIO map, we register all of the GPIOs, but set
the pin count for the unavailable GPIOs to zero.  The pinctrl-msm
driver will block those unavailable GPIOs from being accessed.

To allow newer kernels to support older firmware, the driver retains
support for QCOM8001.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 134 +++++++++++++++++++++++++--------
 1 file changed, 103 insertions(+), 31 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
index bb3ce5c3e18b..deb08e08e86d 100644
--- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
+++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
@@ -38,68 +38,139 @@
 /* maximum size of each gpio name (enough room for "gpioXXX" + null) */
 #define NAME_SIZE	8
 
+enum {
+	QDF2XXX_V1,
+	QDF2XXX_V2,
+};
+
 static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 {
+	const struct acpi_device_id *id;
 	struct pinctrl_pin_desc *pins;
 	struct msm_pingroup *groups;
 	char (*names)[NAME_SIZE];
 	unsigned int i;
 	u32 num_gpios;
+	unsigned int avail_gpios; /* The number of GPIOs we support */
+	u16 *gpios; /* An array of supported GPIOs */
 	int ret;
 
 	/* Query the number of GPIOs from ACPI */
 	ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios);
 	if (ret < 0) {
-		dev_warn(&pdev->dev, "missing num-gpios property\n");
+		dev_err(&pdev->dev, "missing 'num-gpios' property\n");
 		return ret;
 	}
-
 	if (!num_gpios || num_gpios > MAX_GPIOS) {
-		dev_warn(&pdev->dev, "invalid num-gpios property\n");
+		dev_err(&pdev->dev, "invalid 'num-gpios' property\n");
 		return -ENODEV;
 	}
 
+	/*
+	 * The QCOM8001 HID contains only the number of GPIOs, and assumes
+	 * that all of them are available. avail_gpios is the same as num_gpios.
+	 *
+	 * The QCOM8002 HID introduces the 'gpios' DSD, which lists
+	 * specific GPIOs that the driver is allowed to access.
+	 *
+	 * The make the common code simpler, in both cases we create an
+	 * array of GPIOs that are accessible.  So for QCOM8001, that would
+	 * be all of the GPIOs.
+	 */
+	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
+
+	if (id->driver_data == QDF2XXX_V1) {
+		avail_gpios = num_gpios;
+
+		gpios = devm_kmalloc_array(&pdev->dev, avail_gpios,
+					   sizeof(gpios[0]), GFP_KERNEL);
+		if (!gpios)
+			return -ENOMEM;
+
+		for (i = 0; i < avail_gpios; i++)
+			gpios[i] = i;
+	} else {
+		/* The number of GPIOs in the approved list */
+		ret = device_property_read_u16_array(&pdev->dev, "gpios",
+						     NULL, 0);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "missing 'gpios' property\n");
+			return ret;
+		}
+		/*
+		 * The number of available GPIOs should be non-zero, and no
+		 * more than the total number of GPIOS.
+		 */
+		if (!ret || ret > num_gpios) {
+			dev_err(&pdev->dev, "invalid 'gpios' property\n");
+			return -ENODEV;
+		}
+		avail_gpios = ret;
+
+		gpios = devm_kmalloc_array(&pdev->dev, avail_gpios,
+					   sizeof(gpios[0]), GFP_KERNEL);
+		if (!gpios)
+			return -ENOMEM;
+
+		ret = device_property_read_u16_array(&pdev->dev, "gpios", gpios,
+						     avail_gpios);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "could not read list of GPIOs\n");
+			return ret;
+		}
+	}
+
 	pins = devm_kcalloc(&pdev->dev, num_gpios,
 		sizeof(struct pinctrl_pin_desc), GFP_KERNEL);
 	groups = devm_kcalloc(&pdev->dev, num_gpios,
 		sizeof(struct msm_pingroup), GFP_KERNEL);
-	names = devm_kcalloc(&pdev->dev, num_gpios, NAME_SIZE, GFP_KERNEL);
+	names = devm_kcalloc(&pdev->dev, avail_gpios, NAME_SIZE, GFP_KERNEL);
 
 	if (!pins || !groups || !names)
 		return -ENOMEM;
 
+	/*
+	 * Initialize the array.  GPIOs not listed in the 'gpios' array
+	 * still need a number, but nothing else.
+	 */
 	for (i = 0; i < num_gpios; i++) {
-		snprintf(names[i], NAME_SIZE, "gpio%u", i);
-
 		pins[i].number = i;
-		pins[i].name = names[i];
-
-		groups[i].npins = 1;
-		groups[i].name = names[i];
 		groups[i].pins = &pins[i].number;
+	}
 
-		groups[i].ctl_reg = 0x10000 * i;
-		groups[i].io_reg = 0x04 + 0x10000 * i;
-		groups[i].intr_cfg_reg = 0x08 + 0x10000 * i;
-		groups[i].intr_status_reg = 0x0c + 0x10000 * i;
-		groups[i].intr_target_reg = 0x08 + 0x10000 * i;
-
-		groups[i].mux_bit = 2;
-		groups[i].pull_bit = 0;
-		groups[i].drv_bit = 6;
-		groups[i].oe_bit = 9;
-		groups[i].in_bit = 0;
-		groups[i].out_bit = 1;
-		groups[i].intr_enable_bit = 0;
-		groups[i].intr_status_bit = 0;
-		groups[i].intr_target_bit = 5;
-		groups[i].intr_target_kpss_val = 1;
-		groups[i].intr_raw_status_bit = 4;
-		groups[i].intr_polarity_bit = 1;
-		groups[i].intr_detection_bit = 2;
-		groups[i].intr_detection_width = 2;
+	/* Populate the entries that are meant to be exposes as GPIOs. */
+	for (i = 0; i < avail_gpios; i++) {
+		unsigned int gpio = gpios[i];
+
+		groups[gpio].npins = 1;
+		snprintf(names[i], NAME_SIZE, "gpio%u", gpio);
+		pins[gpio].name = names[i];
+		groups[gpio].name = names[i];
+
+		groups[gpio].ctl_reg = 0x10000 * gpio;
+		groups[gpio].io_reg = 0x04 + 0x10000 * gpio;
+		groups[gpio].intr_cfg_reg = 0x08 + 0x10000 * gpio;
+		groups[gpio].intr_status_reg = 0x0c + 0x10000 * gpio;
+		groups[gpio].intr_target_reg = 0x08 + 0x10000 * gpio;
+
+		groups[gpio].mux_bit = 2;
+		groups[gpio].pull_bit = 0;
+		groups[gpio].drv_bit = 6;
+		groups[gpio].oe_bit = 9;
+		groups[gpio].in_bit = 0;
+		groups[gpio].out_bit = 1;
+		groups[gpio].intr_enable_bit = 0;
+		groups[gpio].intr_status_bit = 0;
+		groups[gpio].intr_target_bit = 5;
+		groups[gpio].intr_target_kpss_val = 1;
+		groups[gpio].intr_raw_status_bit = 4;
+		groups[gpio].intr_polarity_bit = 1;
+		groups[gpio].intr_detection_bit = 2;
+		groups[gpio].intr_detection_width = 2;
 	}
 
+	devm_kfree(&pdev->dev, gpios);
+
 	qdf2xxx_pinctrl.pins = pins;
 	qdf2xxx_pinctrl.groups = groups;
 	qdf2xxx_pinctrl.npins = num_gpios;
@@ -110,7 +181,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id qdf2xxx_acpi_ids[] = {
-	{"QCOM8001"},
+	{"QCOM8001", QDF2XXX_V1},
+	{"QCOM8002", QDF2XXX_V2},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Stephen Boyd]

On 12/13, Timur Tabi wrote:
> Newer versions of the firmware for the Qualcomm Datacenter Technologies
> QDF2400 restricts access to a subset of the GPIOs on the TLMM.  To
> prevent older kernels from accidentally accessing the restricted GPIOs,
> we change the ACPI HID for the TLMM block from QCOM8001 to QCOM8002,
> and introduce a new property "gpios".  This property is an array of
> specific GPIOs that are accessible.  When an older kernel boots on
> newer (restricted) firmware, it will fail to probe.
> 
> To implement the sparse GPIO map, we register all of the GPIOs, but set
> the pin count for the unavailable GPIOs to zero.  The pinctrl-msm
> driver will block those unavailable GPIOs from being accessed.
> 
> To allow newer kernels to support older firmware, the driver retains
> support for QCOM8001.
> 
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 134 +++++++++++++++++++++++++--------
>  1 file changed, 103 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
> index bb3ce5c3e18b..deb08e08e86d 100644
> --- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
> +++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
> @@ -38,68 +38,139 @@
>  /* maximum size of each gpio name (enough room for "gpioXXX" + null) */
>  #define NAME_SIZE	8
>  
> +enum {
> +	QDF2XXX_V1,
> +	QDF2XXX_V2,
> +};
> +
>  static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
>  {
> +	const struct acpi_device_id *id;
>  	struct pinctrl_pin_desc *pins;
>  	struct msm_pingroup *groups;
>  	char (*names)[NAME_SIZE];
>  	unsigned int i;
>  	u32 num_gpios;
> +	unsigned int avail_gpios; /* The number of GPIOs we support */
> +	u16 *gpios; /* An array of supported GPIOs */
>  	int ret;
>  
>  	/* Query the number of GPIOs from ACPI */
>  	ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios);
>  	if (ret < 0) {
> -		dev_warn(&pdev->dev, "missing num-gpios property\n");
> +		dev_err(&pdev->dev, "missing 'num-gpios' property\n");
>  		return ret;
>  	}
> -
>  	if (!num_gpios || num_gpios > MAX_GPIOS) {

Given that we have MAX_GPIOS, it would be better to declare a
bitmap of available gpios of that size on the stack and then
iterate through the bitmap and set bits for the available ones.
In the QCOM8001 case, that would be setting all bits up to
num_gpios, and in the QCOM8002 case it would be iterating through
the list of gpios from the DSD property and setting the bit for
that gpio number.  This avoids explicitly allocating a list of
numbers that is freed almost immediately. Instead we just stack
256 / sizeof(unsigned long) words and set bits.

Hopefully we could lift the same logic into the core pinctrl msm
driver for usage on non-ACPI systems.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/13/2017 05:01 PM, Stephen Boyd wrote:
> Given that we have MAX_GPIOS, it would be better to declare a
> bitmap of available gpios of that size on the stack and then
> iterate through the bitmap and set bits for the available ones.
> In the QCOM8001 case, that would be setting all bits up to
> num_gpios, and in the QCOM8002 case it would be iterating through
> the list of gpios from the DSD property and setting the bit for
> that gpio number.  This avoids explicitly allocating a list of
> numbers that is freed almost immediately. Instead we just stack
> 256 / sizeof(unsigned long) words and set bits.

I'm not sure I understand.  The only think I'm allocating temporarily is 
the 'gpios' array, which is an array of shorts.  Each element stores the 
gpio number.  It's not a bit array, so "256 / sizeof(unsigned long)" 
doesn't apply.  I need that array to read the DSD.  You can't iterate 
through an DSD property without reading it completely first.

> Hopefully we could lift the same logic into the core pinctrl msm
> driver for usage on non-ACPI systems.

There is no new memory allocation being done in pinctrl-msm, so I don't 
understand this either.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

Stephen, any follow-up to this?  I'd like to get these patches into 4.16 
if at all possible.  Thanks.

On 12/13/17 5:09 PM, Timur Tabi wrote:
> On 12/13/2017 05:01 PM, Stephen Boyd wrote:
>> Given that we have MAX_GPIOS, it would be better to declare a
>> bitmap of available gpios of that size on the stack and then
>> iterate through the bitmap and set bits for the available ones.
>> In the QCOM8001 case, that would be setting all bits up to
>> num_gpios, and in the QCOM8002 case it would be iterating through
>> the list of gpios from the DSD property and setting the bit for
>> that gpio number.  This avoids explicitly allocating a list of
>> numbers that is freed almost immediately. Instead we just stack
>> 256 / sizeof(unsigned long) words and set bits.
> 
> I'm not sure I understand.  The only think I'm allocating temporarily is 
> the 'gpios' array, which is an array of shorts.  Each element stores the 
> gpio number.  It's not a bit array, so "256 / sizeof(unsigned long)" 
> doesn't apply.  I need that array to read the DSD.  You can't iterate 
> through an DSD property without reading it completely first.
> 
>> Hopefully we could lift the same logic into the core pinctrl msm
>> driver for usage on non-ACPI systems.
> 
> There is no new memory allocation being done in pinctrl-msm, so I don't 
> understand this either.
> 


-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Stephen Boyd]

On 12/18, Timur Tabi wrote:
> Stephen, any follow-up to this?  I'd like to get these patches into
> 4.16 if at all possible.  Thanks.
> 

Ah I missed that the u16 array can't be iterated through. Any
chance the ACPI tables can be changed to list pin ranges, like
<33 3>, <90 2>, to indicate that pins 33, 34, 35 and pins 90, 91
are available? That would allow us to put that into the core
pinctrl-msm.c file a little better and then only expose pins on
the gpiochip when call gpiochip_add_pin_range(). If we want to
support this in DT, I think we would have a DT property like
available-gpios = <33 3>, <90 2>, <100 34> that we can then
iterate through and add only these pins to the gpiochip. That's
better than a bitmap in DT and is still compressed somewhat.

Without going all the way down into that path, here's my patch to
make your patch smaller, but perhaps we can just look for the
ACPI property or the DT property in the pinctrl-msm.c core and
then add pin ranges directly. Then this ACPI driver doesn't
really need to change besides for the ID update. We can expose
all the pins and offsets, etc. from the hardware driver but cut
out gpios in the core layer in a generic way.

---8<---
From: Timur Tabi <timur@codeaurora.org>

Newer versions of the firmware for the Qualcomm Datacenter Technologies
QDF2400 restricts access to a subset of the GPIOs on the TLMM.  To
prevent older kernels from accidentally accessing the restricted GPIOs,
we change the ACPI HID for the TLMM block from QCOM8001 to QCOM8002,
and introduce a new property "gpios".  This property is an array of
specific GPIOs that are accessible.  When an older kernel boots on
newer (restricted) firmware, it will fail to probe.

To implement the sparse GPIO map, we register all of the GPIOs, but set
the pin count for the unavailable GPIOs to zero.  The pinctrl-msm
driver will block those unavailable GPIOs from being accessed.

To allow newer kernels to support older firmware, the driver retains
support for QCOM8001.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 89 +++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
index bb3ce5c3e18b..2ca2f40719b3 100644
--- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
+++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
@@ -38,45 +38,107 @@ static struct msm_pinctrl_soc_data qdf2xxx_pinctrl;
 /* maximum size of each gpio name (enough room for "gpioXXX" + null) */
 #define NAME_SIZE	8
 
+enum {
+	QDF2XXX_V1,
+	QDF2XXX_V2,
+};
+
 static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 {
+	const struct acpi_device_id *id;
 	struct pinctrl_pin_desc *pins;
 	struct msm_pingroup *groups;
 	char (*names)[NAME_SIZE];
-	unsigned int i;
+	unsigned int i, j;
 	u32 num_gpios;
+	unsigned int avail_gpios; /* The number of GPIOs we support */
+	u16 *gpios = NULL; /* An array of supported GPIOs */
 	int ret;
 
 	/* Query the number of GPIOs from ACPI */
 	ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios);
 	if (ret < 0) {
-		dev_warn(&pdev->dev, "missing num-gpios property\n");
+		dev_err(&pdev->dev, "missing 'num-gpios' property\n");
 		return ret;
 	}
-
 	if (!num_gpios || num_gpios > MAX_GPIOS) {
-		dev_warn(&pdev->dev, "invalid num-gpios property\n");
+		dev_err(&pdev->dev, "invalid 'num-gpios' property\n");
 		return -ENODEV;
 	}
 
+	/*
+	 * The QCOM8001 HID contains only the number of GPIOs, and assumes
+	 * that all of them are available. avail_gpios is the same as num_gpios.
+	 *
+	 * The QCOM8002 HID introduces the 'gpios' DSD, which lists
+	 * specific GPIOs that the driver is allowed to access.
+	 *
+	 * The make the common code simpler, in both cases we create an
+	 * array of GPIOs that are accessible.  So for QCOM8001, that would
+	 * be all of the GPIOs.
+	 */
+	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
+
+	if (id->driver_data == QDF2XXX_V1) {
+		avail_gpios = num_gpios;
+	} else {
+		/* The number of GPIOs in the approved list */
+		ret = device_property_read_u16_array(&pdev->dev, "gpios",
+						     NULL, 0);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "missing 'gpios' property\n");
+			return ret;
+		}
+		/*
+		 * The number of available GPIOs should be non-zero, and no
+		 * more than the total number of GPIOS.
+		 */
+		if (!ret || ret > num_gpios) {
+			dev_err(&pdev->dev, "invalid 'gpios' property\n");
+			return -ENODEV;
+		}
+		avail_gpios = ret;
+
+		gpios = devm_kmalloc_array(&pdev->dev, avail_gpios,
+					   sizeof(gpios[0]), GFP_KERNEL);
+		if (!gpios)
+			return -ENOMEM;
+
+		/* Assume the array of available GPIOs is sorted */
+		ret = device_property_read_u16_array(&pdev->dev, "gpios", gpios,
+						     avail_gpios);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "could not read list of GPIOs\n");
+			return ret;
+		}
+	}
+
 	pins = devm_kcalloc(&pdev->dev, num_gpios,
 		sizeof(struct pinctrl_pin_desc), GFP_KERNEL);
 	groups = devm_kcalloc(&pdev->dev, num_gpios,
 		sizeof(struct msm_pingroup), GFP_KERNEL);
-	names = devm_kcalloc(&pdev->dev, num_gpios, NAME_SIZE, GFP_KERNEL);
+	names = devm_kcalloc(&pdev->dev, avail_gpios, NAME_SIZE, GFP_KERNEL);
 
 	if (!pins || !groups || !names)
 		return -ENOMEM;
 
-	for (i = 0; i < num_gpios; i++) {
-		snprintf(names[i], NAME_SIZE, "gpio%u", i);
-
+	/*
+	 * Initialize the array.  GPIOs not listed in the 'gpios' bitmap
+	 * still need a number, but nothing else.
+	 */
+	for (i = 0, j = 0; i < num_gpios; i++) {
 		pins[i].number = i;
-		pins[i].name = names[i];
+		groups[i].pins = &pins[i].number;
+
+		/* Only expose GPIOs that are available */
+		if (gpios && gpios[j] != i)
+			continue;
 
 		groups[i].npins = 1;
-		groups[i].name = names[i];
-		groups[i].pins = &pins[i].number;
+		snprintf(names[j], NAME_SIZE, "gpio%u", i);
+		pins[i].name = names[j];
+		groups[i].name = names[j];
+		j++;
 
 		groups[i].ctl_reg = 0x10000 * i;
 		groups[i].io_reg = 0x04 + 0x10000 * i;
@@ -100,6 +162,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 		groups[i].intr_detection_width = 2;
 	}
 
+	devm_kfree(&pdev->dev, gpios);
+
 	qdf2xxx_pinctrl.pins = pins;
 	qdf2xxx_pinctrl.groups = groups;
 	qdf2xxx_pinctrl.npins = num_gpios;
@@ -110,7 +174,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id qdf2xxx_acpi_ids[] = {
-	{"QCOM8001"},
+	{"QCOM8001", QDF2XXX_V1},
+	{"QCOM8002", QDF2XXX_V2},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/18/17 8:39 PM, Stephen Boyd wrote:
> Ah I missed that the u16 array can't be iterated through. Any
> chance the ACPI tables can be changed to list pin ranges, like
> <33 3>, <90 2>, to indicate that pins 33, 34, 35 and pins 90, 91
> are available?

It's too late.  Firmware is already shipping with the current layout. 
Unfortunately, there's no good peer review process for DSDs that don't 
have a DT equivalent.

> That would allow us to put that into the core
> pinctrl-msm.c file a little better and then only expose pins on
> the gpiochip when call gpiochip_add_pin_range(). If we want to
> support this in DT, I think we would have a DT property like
> available-gpios = <33 3>, <90 2>, <100 34> that we can then
> iterate through and add only these pins to the gpiochip. That's
> better than a bitmap in DT and is still compressed somewhat.

Keep in mind that all this ACPI junk is localized to pinctrl-qdf2xxx. 
pinctrl-msm does not define any new data structures, it just reuses the 
existing one.  You can still define your DT properties any way you want 
in your client drivers.  pinctrl-qdf2xxx is specific to the Centriq chips.

> Without going all the way down into that path, here's my patch to
> make your patch smaller, but perhaps we can just look for the
> ACPI property or the DT property in the pinctrl-msm.c core and
> then add pin ranges directly. Then this ACPI driver doesn't
> really need to change besides for the ID update. We can expose
> all the pins and offsets, etc. from the hardware driver but cut
> out gpios in the core layer in a generic way.

Ok, let me review this.  I don't think there's any gain in moving the 
ACPI processing to pinctrl-msm, however.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Stephen Boyd]

On 12/18, Timur Tabi wrote:
> On 12/18/17 8:39 PM, Stephen Boyd wrote:
> >Ah I missed that the u16 array can't be iterated through. Any
> >chance the ACPI tables can be changed to list pin ranges, like
> ><33 3>, <90 2>, to indicate that pins 33, 34, 35 and pins 90, 91
> >are available?
> 
> It's too late.  Firmware is already shipping with the current
> layout. Unfortunately, there's no good peer review process for DSDs
> that don't have a DT equivalent.

Alright!

> 
> >That would allow us to put that into the core
> >pinctrl-msm.c file a little better and then only expose pins on
> >the gpiochip when call gpiochip_add_pin_range(). If we want to
> >support this in DT, I think we would have a DT property like
> >available-gpios = <33 3>, <90 2>, <100 34> that we can then
> >iterate through and add only these pins to the gpiochip. That's
> >better than a bitmap in DT and is still compressed somewhat.
> 
> Keep in mind that all this ACPI junk is localized to
> pinctrl-qdf2xxx. pinctrl-msm does not define any new data
> structures, it just reuses the existing one.  You can still define
> your DT properties any way you want in your client drivers.
> pinctrl-qdf2xxx is specific to the Centriq chips.

Of course.

> 
> >Without going all the way down into that path, here's my patch to
> >make your patch smaller, but perhaps we can just look for the
> >ACPI property or the DT property in the pinctrl-msm.c core and
> >then add pin ranges directly. Then this ACPI driver doesn't
> >really need to change besides for the ID update. We can expose
> >all the pins and offsets, etc. from the hardware driver but cut
> >out gpios in the core layer in a generic way.
> 
> Ok, let me review this.  I don't think there's any gain in moving
> the ACPI processing to pinctrl-msm, however.
> 

I will attempt to implement the DT part today. It may make the
get_direction() revert irrelevant if the gpios aren't even
exposed to gpiolib in the first place.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/18/2017 08:39 PM, Stephen Boyd wrote:
> +	for (i = 0, j = 0; i < num_gpios; i++) {
>   		pins[i].number = i;
> -		pins[i].name = names[i];
> +		groups[i].pins = &pins[i].number;
> +
> +		/* Only expose GPIOs that are available */
> +		if (gpios && gpios[j] != i)
> +			continue;

I don't know if I would say this is an improvement.  For one thing, 
QCOM8001 systems are deprecated and don't really exist any more.  At the 
time I originally wrote this patch, they were still in the wild, but 
they're all gone now.  So it's no longer efficient to treat QCOM8001 as 
the default case.  This means that the for-loop will iterate over the 
full range now, instead of the partial range that it does with my v10 patch.

If I post another version of this patch, I'm just going to remove 
support for QCOM8001.

If you want to avoid kmalloc'ing the GPIOs array, we can put it on the 
stack with a dynamic size, since it will be no more than MAX_GPIOS * 2 
(i.e. 512) bytes in size.

	u16 gpios[avail_gpios];

It would be a little hackish since it needs to be defined at the 
beginning of a code block, so I would probably put into its own 
function, but I still fail to see what's wrong with using kmalloc to 
allocate that array for short-term use temporarily.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Stephen Boyd]

On 12/19, Timur Tabi wrote:
> On 12/18/2017 08:39 PM, Stephen Boyd wrote:
> >+	for (i = 0, j = 0; i < num_gpios; i++) {
> >  		pins[i].number = i;
> >-		pins[i].name = names[i];
> >+		groups[i].pins = &pins[i].number;
> >+
> >+		/* Only expose GPIOs that are available */
> >+		if (gpios && gpios[j] != i)
> >+			continue;
> 
> I don't know if I would say this is an improvement.  For one thing,
> QCOM8001 systems are deprecated and don't really exist any more.  At
> the time I originally wrote this patch, they were still in the wild,
> but they're all gone now.  So it's no longer efficient to treat
> QCOM8001 as the default case.  This means that the for-loop will
> iterate over the full range now, instead of the partial range that
> it does with my v10 patch.
> 
> If I post another version of this patch, I'm just going to remove
> support for QCOM8001.

That sounds good too. The diff was really noisy because all the
foo[i] became foo[gpio] which causes the diff to increase for no
real purpose. My patch was rewriting that stuff so it doesn't
come into the diff and we can concentrate on what's actually
changing. We already iterate over the full range to fill in the
two fields anyway, so I'm not sure what you're getting at with
your for-loop comment. Seems like a micro-optimization on probe
that probably isn't going to be noticed.

> 
> If you want to avoid kmalloc'ing the GPIOs array, we can put it on
> the stack with a dynamic size, since it will be no more than
> MAX_GPIOS * 2 (i.e. 512) bytes in size.
> 
> 	u16 gpios[avail_gpios];
> 
> It would be a little hackish since it needs to be defined at the
> beginning of a code block, so I would probably put into its own
> function, but I still fail to see what's wrong with using kmalloc to
> allocate that array for short-term use temporarily.
> 

Yeah I wouldn't do that. I'm not trying to avoid allocating the
array anymore. Dynamically sized arrays on the stack are not a
great idea in the kernel where we have limited stack sizes.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/19/2017 02:30 PM, Stephen Boyd wrote:
> Yeah I wouldn't do that. I'm not trying to avoid allocating the
> array anymore. Dynamically sized arrays on the stack are not a
> great idea in the kernel where we have limited stack sizes.

So do you just want to see a v11 that drops support for QCOM8001?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/18/2017 08:39 PM, Stephen Boyd wrote:
> +		if (gpios && gpios[j] != i)
> +			continue;
...
 > +		j++;

Doesn't this assume that the available GPIOs are listed in numerical 
order in the ACPI table?  If so, then this patch won't work because that 
order is not guaranteed.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Stephen Boyd]

On 12/19, Timur Tabi wrote:
> On 12/18/2017 08:39 PM, Stephen Boyd wrote:
> >+		if (gpios && gpios[j] != i)
> >+			continue;
> ...
> > +		j++;
> 
> Doesn't this assume that the available GPIOs are listed in numerical
> order in the ACPI table?  If so, then this patch won't work because
> that order is not guaranteed.
> 

Yes, I added a comment in the patch about that assumption. It's
simple enough to sort the array in place with sort() though.

I was looking at the gpiochip_add_pin_ranges() code to try and
understand how to only expose pins that are supported as gpios
into gpiolib, and then I looked at the history of these patches
and wrote some code around pin ranges, got super confused and
started thinking about adding gpiochips for each range (ugh),
talked to Bjorn, and now I'm writing this mail. The approach of
multiple ranges or chips looks like a dead-end that you've
already gone down so let's not do that.

The thing that I don't like about this patch is that we're
modifying npins to indicate what gpios are available or not and
then having a huge diff in this patch to do the 's/i/gpio/'.
Ideally, everything would flow directly from the request callback
and the SoC specific pinctrl driver would just tell the core code
what all pins exist in hardware even if they're locked away and
in use by something non-linux. That way, we don't have to rejig
things in the SoC specific driver when the system configuration
changes. I'm hoping we can do the valid mask part generically in
the core pinctrl-msm.c file once so that things aren't spread
around the SoC specific drivers and we solve ACPI and DT at the
same time.

We will want irq lines to be unallocated for things that aren't
GPIOs, I'm not sure about ACPI and if it cares here, and we have
a one to one mapping between irqs, GPIOs, and pinctrl pins with
this hardware. Furthermore, we have the irq_valid_mask support in
place already, so it seems that we can at least use the mask to
be the one place where we indicate which pins are allowed to be
used. And it seems like the simplest solution is to set the irq
valid mask to be the GPIOs from the device property and then test
that bitmask in the pinmux_ops structure's request callback so we
cover both gpio (via the gpiochip_generic_request() ->
pinmux_request_gpio() -> pin_request() path) and pinctrl (via the
pin_request() path). Debugfs will need to test the mask too, but
that should be simple. I believe you don't care about pin muxing,
but it would make things work in both cases and will help if we
want to limit access on platforms that use pin muxing.

Let's resolve this by the end of this week. If this plan works we
can have the revert patch for get_direction() and the
pinctrl-msm.c patch to update the valid mask on gpiochip
registration.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/19/17 8:26 PM, Stephen Boyd wrote:
> The thing that I don't like about this patch is that we're
> modifying npins to indicate what gpios are available or not and
> then having a huge diff in this patch to do the 's/i/gpio/'.

Considering how small the driver is, I'm not sure if I'd say it's a 
"huge" diff.

Frankly, I think this is a very elegant re-purposing of 'npins'.

> Ideally, everything would flow directly from the request callback
> and the SoC specific pinctrl driver would just tell the core code
> what all pins exist in hardware even if they're locked away and
> in use by something non-linux. 

So you want the request callback to propagated to the SOC driver?  I 
guess that could work.

> That way, we don't have to rejig
> things in the SoC specific driver when the system configuration
> changes. I'm hoping we can do the valid mask part generically in
> the core pinctrl-msm.c file once so that things aren't spread
> around the SoC specific drivers and we solve ACPI and DT at the
> same time.

Well, now I'm confused.  First I thought you wanted to move the valid 
check into pinctrl-qdf2xxx, but now you say you want it done in 
pinctrl-msm, but isn't that what my patches are doing now?

> We will want irq lines to be unallocated for things that aren't
> GPIOs, I'm not sure about ACPI and if it cares here, and we have
> a one to one mapping between irqs, GPIOs, and pinctrl pins with
> this hardware. 

If the pin can't be requested, doesn't that take care of IRQ lines 
automatically?  I don't touch the irq_valid_mask code.

> Furthermore, we have the irq_valid_mask support in
> place already, so it seems that we can at least use the mask to
> be the one place where we indicate which pins are allowed to be
> used. 

Well, I really didn't want to do that.  Keep in mind that the root 
problem is getting pinctrl-qdf2xxx to be able to tell pinctrl-msm what 
pins are valid.  That is the bulk of my code.

I'm understanding you less and less the more I read.

 >And it seems like the simplest solution is to set the irq
> valid mask to be the GPIOs from the device property and then test
> that bitmask in the pinmux_ops structure's request callback so we
> cover both gpio (via the gpiochip_generic_request() ->
> pinmux_request_gpio() -> pin_request() path) and pinctrl (via the
> pin_request() path). 

I do not understand that.  To be honest, I think I already have the 
simplest solution, at least for ACPI.  I don't really want to complicate 
my patches to support DT, since I don't really know what the DT-specific 
problems are.

> Debugfs will need to test the mask too, but
> that should be simple. I believe you don't care about pin muxing,
> but it would make things work in both cases and will help if we
> want to limit access on platforms that use pin muxing.

I don't care about pin muxing, but my patches already take care of debugfs.

> Let's resolve this by the end of this week. If this plan works we
> can have the revert patch for get_direction() and the
> pinctrl-msm.c patch to update the valid mask on gpiochip
> registration.

Frankly, I thought I had everything resolved already, and it sounds like 
you want me to start over from scratch anyway.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Stephen Boyd]

On 12/19, Timur Tabi wrote:
> Frankly, I thought I had everything resolved already, and it sounds
> like you want me to start over from scratch anyway.
> 

Here's the patch. I get a hang when dumping debugfs, but at least
sysfs expose fails when trying to request blocked gpios. I need
to check if we need to say "yes" to pins that are above the gpio
max for pinctrl. I'll do that tomorrow.

---
 drivers/gpio/gpiolib.c             |  4 +-
 drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++++++++--
 include/linux/gpio/driver.h        |  3 ++
 3 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8db2680bf872..5f118f044caa 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1475,8 +1475,8 @@ static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip)
 	gpiochip->irq_valid_mask = NULL;
 }
 
-static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
-				       unsigned int offset)
+bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
+				unsigned int offset)
 {
 	/* No mask means all valid */
 	if (likely(!gpiochip->irq_valid_mask))
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 273badd92561..4c2ce1f7d449 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -105,6 +105,17 @@ static const struct pinctrl_ops msm_pinctrl_ops = {
 	.dt_free_map		= pinctrl_utils_free_map,
 };
 
+static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset)
+{
+	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct gpio_chip *chip = &pctrl->chip;
+
+	if (gpiochip_irqchip_irq_valid(chip, offset))
+		return 0;
+
+	return -EINVAL;
+}
+
 static int msm_get_functions_count(struct pinctrl_dev *pctldev)
 {
 	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
@@ -166,6 +177,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinmux_ops msm_pinmux_ops = {
+	.request		= msm_pinmux_request,
 	.get_functions_count	= msm_get_functions_count,
 	.get_function_name	= msm_get_function_name,
 	.get_function_groups	= msm_get_function_groups,
@@ -493,6 +505,9 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 		"pull up"
 	};
 
+	if (!gpiochip_irqchip_irq_valid(chip, offset))
+		return;
+
 	g = &pctrl->soc->groups[offset];
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
@@ -503,7 +518,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 
 	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
 	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
-	seq_printf(s, " %s", pulls[pull]);
+	seq_printf(s, " %s\n", pulls[pull]);
 }
 
 static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -511,10 +526,8 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned gpio = chip->base;
 	unsigned i;
 
-	for (i = 0; i < chip->ngpio; i++, gpio++) {
+	for (i = 0; i < chip->ngpio; i++, gpio++)
 		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
-		seq_puts(s, "\n");
-	}
 }
 
 #else
@@ -795,6 +808,76 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int msm_gpio_init_irq_valid_mask(struct gpio_chip *chip,
+					struct msm_pinctrl *pctrl)
+{
+	int ret;
+	unsigned int len, i;
+	unsigned int max_gpios = pctrl->soc->ngpios;
+	struct device_node *np = pctrl->dev->of_node;
+
+	/* The number of GPIOs in the ACPI tables */
+	ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
+	if (ret > 0 && ret < max_gpios) {
+		u16 *tmp;
+
+		len = ret;
+		tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL);
+		if (!tmp)
+			return -ENOMEM;
+
+		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
+						     len);
+		if (ret < 0) {
+			dev_err(pctrl->dev, "could not read list of GPIOs\n");
+			kfree(tmp);
+			return ret;
+		}
+
+		bitmap_zero(chip->irq_valid_mask, max_gpios);
+		for (i = 0; i < len; i++)
+			set_bit(tmp[i], chip->irq_valid_mask);
+
+		return 0;
+	}
+
+	/* If there's a DT ngpios-ranges property then add those ranges */
+	ret = of_property_count_u32_elems(np,  "ngpios-ranges");
+	if (ret > 0 && ret % 2 == 0 && ret / 2 < max_gpios) {
+		u32 start;
+		u32 count;
+
+		len = ret / 2;
+		bitmap_zero(chip->irq_valid_mask, max_gpios);
+
+		for (i = 0; i < len; i++) {
+			of_property_read_u32_index(np, "ngpios-ranges",
+						   i * 2, &start);
+			of_property_read_u32_index(np, "ngpios-ranges",
+						   i * 2 + 1, &count);
+			bitmap_set(chip->irq_valid_mask, start, count);
+		}
+	}
+
+	return 0;
+}
+
+static bool msm_gpio_needs_irq_valid_mask(struct msm_pinctrl *pctrl)
+{
+	int ret;
+	struct device_node *np = pctrl->dev->of_node;
+
+	ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
+	if (ret > 0)
+		return true;
+
+	ret = of_property_count_u32_elems(np,  "ngpios-ranges");
+	if (ret > 0 && ret % 2 == 0)
+		return true;
+
+	return false;
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
@@ -811,6 +894,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->parent = pctrl->dev;
 	chip->owner = THIS_MODULE;
 	chip->of_node = pctrl->dev->of_node;
+	chip->irq_need_valid_mask = msm_gpio_needs_irq_valid_mask(pctrl);
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
@@ -818,6 +902,12 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return ret;
 	}
 
+	ret = msm_gpio_init_irq_valid_mask(chip, pctrl);
+	if (ret) {
+		dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
+		return ret;
+	}
+
 	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed to add pin range\n");
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index af20369ec8e7..be977c1c7498 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -262,6 +262,9 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 			     bool nested,
 			     struct lock_class_key *lock_key);
 
+bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
+				unsigned int offset);
+
 #ifdef CONFIG_LOCKDEP
 
 /*
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/20/2017 02:15 AM, Stephen Boyd wrote:
> Here's the patch. I get a hang when dumping debugfs, but at least
> sysfs expose fails when trying to request blocked gpios. I need
> to check if we need to say "yes" to pins that are above the gpio
> max for pinctrl. I'll do that tomorrow.

Sorry, I just don't see how this is better than my patches.  I don't 
understand the need for involving the IRQ valid mask.  I also don't see 
the value in adding code to look for a property that exists only in one 
ACPI HID (QCOM8002) as if it were generic.  The "num-gpios" and "gpios" 
DSDs are not supposed to exist in any other HID, so there should be no 
code that reads it in pinctrl-msm.

I'm going on vacation soon.  I will post a v11 that eliminates support 
for QCOM8001.  Maybe that version is "good enough" for now and you can 
add DT and/or IRQ support on top of it.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Stephen Boyd]

On 12/20, Timur Tabi wrote:
> On 12/20/2017 02:15 AM, Stephen Boyd wrote:
> >Here's the patch. I get a hang when dumping debugfs, but at least
> >sysfs expose fails when trying to request blocked gpios. I need
> >to check if we need to say "yes" to pins that are above the gpio
> >max for pinctrl. I'll do that tomorrow.
> 
> Sorry, I just don't see how this is better than my patches.  I don't
> understand the need for involving the IRQ valid mask.  I also don't
> see the value in adding code to look for a property that exists only
> in one ACPI HID (QCOM8002) as if it were generic.  The "num-gpios"
> and "gpios" DSDs are not supposed to exist in any other HID, so
> there should be no code that reads it in pinctrl-msm.

I don't see how it hurts to treat it generically. Presumably
that's the way it will be done on ACPI platforms going forward?
No need to tie it to some ACPI HID.

I'm trying to resolve everything at once: gpios, pinctrl pins,
and irqs exposed by the TLMM hardware. The value is that we solve
it all, once, now. The DT binding can also be resolved at the
same time, so when we need to express this in DT it's already
done. Otherwise, something can request irqs from the irqdomain
even if the irq can't be enabled, or it can try to mux the pin to
some other function, even if the function selection can't be
configured.

Boiling everything down into the irq valid mask should cover all
these cases, and not require us to strip const from all the data
in the non-ACPI pinctrl drivers to replace the value in the npins
field at runtime.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/20/17 6:39 PM, Stephen Boyd wrote:
> I don't see how it hurts to treat it generically. Presumably
> that's the way it will be done on ACPI platforms going forward?
> No need to tie it to some ACPI HID.

But it is tied to a HID.  The "num-gpios" and "gpios" properties belong 
to a specific HID.  Someone could create a new HID with different 
properties, and then what?  That's why I want all the ACPI stuff in the 
client driver.

At this point I don't really care any more about what the patches look 
like, but I really do think that putting the ACPI code in pinctrl-msm is 
a bad idea.

We're debating adding support for multiple TLMMs, and we may create a 
new HID for that, so that we can define all pins on all TLMMs in one 
device.  We would need to create a new HID and new DSDs to go with it.

> I'm trying to resolve everything at once: gpios, pinctrl pins,
> and irqs exposed by the TLMM hardware. The value is that we solve
> it all, once, now.

Keep in mind that I am now in vacation, and so I won't be able to submit 
any more patches for a while.

> The DT binding can also be resolved at the
> same time, so when we need to express this in DT it's already
> done.

Ok.

> Otherwise, something can request irqs from the irqdomain
> even if the irq can't be enabled, or it can try to mux the pin to
> some other function, even if the function selection can't be
> configured.

Is it possible to request an IRQ for a pin if the pin itself can't be 
requested?

> Boiling everything down into the irq valid mask should cover all
> these cases, and not require us to strip const from all the data
> in the non-ACPI pinctrl drivers to replace the value in the npins
> field at runtime.

Ok.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Stephen Boyd]

On 12/20, Timur Tabi wrote:
> On 12/20/17 6:39 PM, Stephen Boyd wrote:
> >I don't see how it hurts to treat it generically. Presumably
> >that's the way it will be done on ACPI platforms going forward?
> >No need to tie it to some ACPI HID.
> 
> But it is tied to a HID.  The "num-gpios" and "gpios" properties
> belong to a specific HID.  Someone could create a new HID with
> different properties, and then what?  That's why I want all the ACPI
> stuff in the client driver.
> 
> At this point I don't really care any more about what the patches
> look like, but I really do think that putting the ACPI code in
> pinctrl-msm is a bad idea.
> 
> We're debating adding support for multiple TLMMs, and we may create
> a new HID for that, so that we can define all pins on all TLMMs in
> one device.  We would need to create a new HID and new DSDs to go
> with it.

Ok. That's testable with acpi_match_device_ids() though. I can
add that into pinctrl-msm.c so we don't have to pass info about
available gpios from ACPI specific driver into the pinctrl-msm
core driver. That's why I'm trying to avoid doing it in the ACPI
specific driver. Do it close to where the gpiochip is created
instead.

Maybe future HIDs could follow the DT design and then we can look
for the same device property name in both firmwares. Parsing
ranges is simpler.

> 
> >I'm trying to resolve everything at once: gpios, pinctrl pins,
> >and irqs exposed by the TLMM hardware. The value is that we solve
> >it all, once, now.
> 
> Keep in mind that I am now in vacation, and so I won't be able to
> submit any more patches for a while.
> 
> >The DT binding can also be resolved at the
> >same time, so when we need to express this in DT it's already
> >done.
> 
> Ok.
> 
> >Otherwise, something can request irqs from the irqdomain
> >even if the irq can't be enabled, or it can try to mux the pin to
> >some other function, even if the function selection can't be
> >configured.
> 
> Is it possible to request an IRQ for a pin if the pin itself can't
> be requested?

I don't see any place blocking GPIOs turning into IRQs once we
setup the irqdomain with interrupts. Maybe I missed something,
but I think you can request an IRQ once the domain has the hwirqs
associated with it. I will test it out.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/21/2017 07:46 PM, Stephen Boyd wrote:
> Ok. That's testable with acpi_match_device_ids() though. I can
> add that into pinctrl-msm.c so we don't have to pass info about
> available gpios from ACPI specific driver into the pinctrl-msm
> core driver. That's why I'm trying to avoid doing it in the ACPI
> specific driver. Do it close to where the gpiochip is created
> instead.

I guess we're just going to have to agree to disagree.  I see the DSDs 
as being SOC-specific, and therefore belong in the SOC driver.  I'm 
still going to need an SOC driver to define the TLMM register layout.

But as I said earlier, I've already spent way too much time working on a 
driver that, in all likelihood, never be used in any production system 
anyway.

I look forward to reviewing the next version of your patch.

> Maybe future HIDs could follow the DT design and then we can look
> for the same device property name in both firmwares. 

DSDs generally don't have the vendor prefix that DT properties do.

> Parsing
> ranges is simpler.

I'm not sure I agree with that.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Andy Shevchenko]

On Thu, 2018-01-04 at 09:46 -0600, Timur Tabi wrote:
> On 12/21/2017 07:46 PM, Stephen Boyd wrote:
> > 
> > Maybe future HIDs could follow the DT design and then we can look
> > for the same device property name in both firmwares. 
> 
> DSDs generally don't have the vendor prefix that DT properties do.

There are more means to check hardware revisions:
HID - Hardware ID
CID - Compatible ID
UID - Unique ID (good to distinguish instances of the same device on the
board)
_HRV - Hardware Revision (6.1.6 describes this one)

Everything is described in the spec. Does anybody care to read?

P.S. More I reading this thread more I become thinking that people screw
ACPI use in many ways...

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

Re: [PATCH 3/3] [v6] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Linus Walleij]

On Thu, Jan 4, 2018 at 5:04 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2018-01-04 at 09:46 -0600, Timur Tabi wrote:
>> On 12/21/2017 07:46 PM, Stephen Boyd wrote:
>> >
>> > Maybe future HIDs could follow the DT design and then we can look
>> > for the same device property name in both firmwares.
>>
>> DSDs generally don't have the vendor prefix that DT properties do.
>
> There are more means to check hardware revisions:
> HID - Hardware ID
> CID - Compatible ID
> UID - Unique ID (good to distinguish instances of the same device on the
> board)
> _HRV - Hardware Revision (6.1.6 describes this one)

Thanks Andy.

I expect a patch using these features, also include Andy on CC
as it appears he knows how this should be done. (In difference
from me...) I'm pretty much relying on other people to understand
ACPI for me, maybe I should attend a course or something.

Yours,
Linus Walleij

[PATCH 0/3] [v11] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

A series of patches that add support for GPIO maps that have holes in
them.  That is, even though a client driver has N consecutive GPIOs,
some are just unavailable for whatever reason, and the hardware should
not be accessed for those GPIOs.

Patch 1 reverts an old patch that triggers a get_direction of every
pin upon init, without attempting to request the pins first.  The
direction is already being queried when the pin is requested.

Patch 2 adds support to pinctrl-msm for "unavailable" GPIOs.

Patch 3 extends that support to pinctrl-qdf2xxx.  A recent ACPI change
on QDF2400 platforms blocks access to most pins, so the driver can only
register a subset.

This version drops the availability check in gpiolib, because it's no
necessary.  Instead, just having pinctrl-msm return -EACCES is enough
to block all unavailable GPIOs.  Patch 1 removes the only instance where
an unrequested GPIO is being accessed.

v11:
  Drop support for QCOM8001

Timur Tabi (3):
  [v2] Revert "gpio: set up initial state from .get_direction()"
  [v8] pinctrl: qcom: disable GPIO groups with no pins
  [v7] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

 drivers/gpio/gpiolib.c                 | 31 +++--------
 drivers/pinctrl/qcom/pinctrl-msm.c     | 28 ++++++++--
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 96 ++++++++++++++++++++++------------
 3 files changed, 94 insertions(+), 61 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/3] [v8] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

pinctrl-msm only accepts an array of GPIOs from 0 to n-1, and it expects
each group to support have only one pin (npins == 1).

We can support "sparse" GPIO maps if we allow for some groups to have zero
pins (npins == 0).  These pins are "hidden" from the rest of the driver
and gpiolib.

Access to unavailable GPIOs is blocked via a request callback.  If the
requested GPIO is unavailable, -EACCES is returned, which prevents
further access to that GPIO.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7a960590ecaa..d45b4c2b5af1 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -507,6 +507,11 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 	};
 
 	g = &pctrl->soc->groups[offset];
+
+	/* If the GPIO group has no pins, then don't show it. */
+	if (!g->npins)
+		return;
+
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
 	is_out = !!(ctl_reg & BIT(g->oe_bit));
@@ -516,7 +521,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 
 	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
 	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
-	seq_printf(s, " %s", pulls[pull]);
+	seq_printf(s, " %s\n", pulls[pull]);
 }
 
 static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -524,23 +529,36 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned gpio = chip->base;
 	unsigned i;
 
-	for (i = 0; i < chip->ngpio; i++, gpio++) {
+	for (i = 0; i < chip->ngpio; i++, gpio++)
 		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
-		seq_puts(s, "\n");
-	}
 }
 
 #else
 #define msm_gpio_dbg_show NULL
 #endif
 
+/*
+ * If the requested GPIO has no pins, then treat it as unavailable.
+ * Otherwise, call the standard request function.
+ */
+static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct msm_pingroup *g = &pctrl->soc->groups[offset];
+
+	if (!g->npins)
+		return -EACCES;
+
+	return gpiochip_generic_request(chip, offset);
+}
+
 static const struct gpio_chip msm_gpio_template = {
 	.direction_input  = msm_gpio_direction_input,
 	.direction_output = msm_gpio_direction_output,
 	.get_direction    = msm_gpio_get_direction,
 	.get              = msm_gpio_get,
 	.set              = msm_gpio_set,
-	.request          = gpiochip_generic_request,
+	.request          = msm_gpio_request,
 	.free             = gpiochip_generic_free,
 	.dbg_show         = msm_gpio_dbg_show,
 };
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 0/3] [v9] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

A series of patches that add support for GPIO maps that have holes in
them.  That is, even though a client driver has N consecutive GPIOs,
some are just unavailable for whatever reason, and the hardware should
not be accessed for those GPIOs.

Patch 1 reverts an old patch that triggers a get_direction of every
pin upon init, without attempting to request the pins first.  The
direction is already being queried when the pin is requested.

Patch 2 adds support to pinctrl-msm for "unavailable" GPIOs.

Patch 3 extends that support to pinctrl-qdf2xxx.  A recent ACPI change
on QDF2400 platforms blocks access to most pins, so the driver can only
register a subset.

This version drops the availability check in gpiolib, because it's no
necessary.  Instead, just having pinctrl-msm return -EACCES is enough
to block all unavailable GPIOs.  Patch 1 removes the only instance where
an unrequested GPIO is being accessed.

v9:
  Removed "gpiolib: add bitmask for valid GPIO lines"

Timur Tabi (3):
  [v2] Revert "gpio: set up initial state from .get_direction()"
  [v8] pinctrl: qcom: disable GPIO groups with no pins
  [v5] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

 drivers/gpio/gpiolib.c                 |  31 ++-----
 drivers/pinctrl/qcom/pinctrl-msm.c     |  28 +++++--
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 143 ++++++++++++++++++++++++---------
 3 files changed, 137 insertions(+), 65 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/3] [v8] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

pinctrl-msm only accepts an array of GPIOs from 0 to n-1, and it expects
each group to support have only one pin (npins == 1).

We can support "sparse" GPIO maps if we allow for some groups to have zero
pins (npins == 0).  These pins are "hidden" from the rest of the driver
and gpiolib.

Access to unavailable GPIOs is blocked via a request callback.  If the
requested GPIO is unavailable, -EACCES is returned, which prevents
further access to that GPIO.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7a960590ecaa..d45b4c2b5af1 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -507,6 +507,11 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 	};
 
 	g = &pctrl->soc->groups[offset];
+
+	/* If the GPIO group has no pins, then don't show it. */
+	if (!g->npins)
+		return;
+
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
 	is_out = !!(ctl_reg & BIT(g->oe_bit));
@@ -516,7 +521,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 
 	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
 	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
-	seq_printf(s, " %s", pulls[pull]);
+	seq_printf(s, " %s\n", pulls[pull]);
 }
 
 static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -524,23 +529,36 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned gpio = chip->base;
 	unsigned i;
 
-	for (i = 0; i < chip->ngpio; i++, gpio++) {
+	for (i = 0; i < chip->ngpio; i++, gpio++)
 		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
-		seq_puts(s, "\n");
-	}
 }
 
 #else
 #define msm_gpio_dbg_show NULL
 #endif
 
+/*
+ * If the requested GPIO has no pins, then treat it as unavailable.
+ * Otherwise, call the standard request function.
+ */
+static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct msm_pingroup *g = &pctrl->soc->groups[offset];
+
+	if (!g->npins)
+		return -EACCES;
+
+	return gpiochip_generic_request(chip, offset);
+}
+
 static const struct gpio_chip msm_gpio_template = {
 	.direction_input  = msm_gpio_direction_input,
 	.direction_output = msm_gpio_direction_output,
 	.get_direction    = msm_gpio_get_direction,
 	.get              = msm_gpio_get,
 	.set              = msm_gpio_set,
-	.request          = gpiochip_generic_request,
+	.request          = msm_gpio_request,
 	.free             = gpiochip_generic_free,
 	.dbg_show         = msm_gpio_dbg_show,
 };
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/3] [v8] pinctrl: qcom: disable GPIO groups with no pins

[Andy Shevchenko]

On Tue, 2017-12-12 at 14:50 -0600, Timur Tabi wrote:
> pinctrl-msm only accepts an array of GPIOs from 0 to n-1, and it
> expects
> each group to support have only one pin (npins == 1).
> 
> We can support "sparse" GPIO maps if we allow for some groups to have
> zero
> pins (npins == 0).  These pins are "hidden" from the rest of the
> driver
> and gpiolib.
> 
> Access to unavailable GPIOs is blocked via a request callback.  If the
> requested GPIO is unavailable, -EACCES is returned, which prevents
> further access to that GPIO.

We recently have some interesting BIOS/Windows driver design which makes
a need of something similar. Mika did patched pinctrl-intel for that. I
dunno that approach can be used here, or your proposal be utilized in
pinctrl-intel. Mika, any comments?

See some nitpicks below.

>  
>  	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" :
> "in", func);
>  	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
> -	seq_printf(s, " %s", pulls[pull]);
> +	seq_printf(s, " %s\n", pulls[pull]);

I would rather do

 seq_putc(s, '\n');

which makes code slightly more flexible for maintenance and reading.


>  }
>  
>  static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip
> *chip)
> @@ -524,23 +529,36 @@ static void msm_gpio_dbg_show(struct seq_file
> *s, struct gpio_chip *chip)
>  	unsigned gpio = chip->base;
>  	unsigned i;
>  
> -	for (i = 0; i < chip->ngpio; i++, gpio++) {
> +	for (i = 0; i < chip->ngpio; i++, gpio++)
>  		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
> -		seq_puts(s, "\n");
> -	}

This kind of change looks like a candidate to a separate patch,
though I mentioned it's just a nit.


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