[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

First patch allows for for pinctrl-msm to understand GPIO groups with
no pins.  Such pins are "hidden" and can't be exported or accessed.

Second patch updates the QDF2xxx driver to take advantage of all that.

v5:
 Since gpiochip_add_data no longer requests GPIOs before scanning for
 the direction (that patch was reverted), pinctrl-msm.c now specifically
 checks for special case.

 Also added msm_gpio_get_next_range() to reduce the number of pin
 ranges registered.

Timur Tabi (2):
  [v5] pinctrl: qcom: disable GPIO groups with no pins
  [v3] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

 drivers/pinctrl/qcom/pinctrl-msm.c     | 110 +++++++++++++++++++++++--
 drivers/pinctrl/qcom/pinctrl-msm.h     |   2 +
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 145 +++++++++++++++++++++++++--------
 3 files changed, 215 insertions(+), 42 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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

First patch allows for for pinctrl-msm to understand GPIO groups with
no pins.  Such pins are "hidden" and can't be exported or accessed.

Second patch updates the QDF2xxx driver to take advantage of all that.

v5:
 Since gpiochip_add_data no longer requests GPIOs before scanning for
 the direction (that patch was reverted), pinctrl-msm.c now specifically
 checks for special case.

 Also added msm_gpio_get_next_range() to reduce the number of pin
 ranges registered.

Timur Tabi (2):
  [v5] pinctrl: qcom: disable GPIO groups with no pins
  [v3] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

 drivers/pinctrl/qcom/pinctrl-msm.c     | 110 +++++++++++++++++++++++--
 drivers/pinctrl/qcom/pinctrl-msm.h     |   2 +
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 145 +++++++++++++++++++++++++--------
 3 files changed, 215 insertions(+), 42 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/2] [v5] 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.

A new boolean 'sparse' indicates whether the GPIO map is sparse.  If any
GPIO has an 'npins' value of 0, then 'sparse' must be set to True.

Most access to unavailable GPIOs can be blocked via the gpio_chip.request
function.  The one exception is when gpiochip_add_data() scans all of
the GPIOs without "requesting" them.  To cover this case,
msm_gpio_get_direction() separately checks if the GPIO is available.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 110 +++++++++++++++++++++++++++++++++++--
 drivers/pinctrl/qcom/pinctrl-msm.h |   2 +
 2 files changed, 106 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index ff491da64dab..ca4ae3d76eb4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -443,6 +443,14 @@ static int msm_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 
 	g = &pctrl->soc->groups[offset];
 
+	/*
+	 * During initialization, gpiolib may query all GPIOs for their
+	 * initial direction, regardless if they exist, so block access
+	 * to those that are unavailable.
+	 */
+	if (!g->npins)
+		return -ENODEV;
+
 	val = readl(pctrl->regs + g->ctl_reg);
 
 	/* 0 = output, 1 = input */
@@ -507,6 +515,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 +529,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 +537,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 -ENODEV;
+
+	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,
 };
@@ -808,11 +834,57 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+/**
+ * msm_gpio_get_next_range() - find next range of consecutive gpios
+ * @pctrl: msm_pinctrl object
+ * @pstart: pointer to index of next starting position
+ *
+ * In a sparse GPIO map, available GPIOs are typically collected into blocks.
+ * Given a starting index into the groups[] array, this function will scan all
+ * the gpios until it finds the next block of available GPIOs.
+ *
+ * If groups[*pstart] already points to an available GPIO, then assume that
+ * it is the start of a block.
+ *
+ * The caller is responsible for moving *pstart to after the end of the
+ * previous block so that it this function can find the next block.
+ *
+ * Return: The count of the block starting at @pstart, or 0 if there are no
+ *         more blocks.
+ */
+static unsigned int msm_gpio_get_next_range(struct msm_pinctrl *pctrl,
+					    unsigned int *pstart)
+{
+	const struct msm_pingroup *groups = pctrl->soc->groups;
+	unsigned int ngpio = pctrl->soc->ngpios;
+	unsigned int count = 0;
+	unsigned int start = *pstart;
+
+	/* Find the first available GPIO */
+	while (!groups[start].npins)
+		if (++start == ngpio)
+			/* None found, so just exit */
+			return 0;
+
+	*pstart = start;
+
+	/* Count the number of those GPIOs in a row */
+	while (groups[start].npins) {
+		count++;
+		if (++start == ngpio)
+			break;
+	}
+
+	return count;
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
 	int ret;
 	unsigned ngpio = pctrl->soc->ngpios;
+	const struct msm_pingroup *groups = pctrl->soc->groups;
+	unsigned int i, start = 0, count;
 
 	if (WARN_ON(ngpio > MAX_NR_GPIO))
 		return -EINVAL;
@@ -825,13 +897,39 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->owner = THIS_MODULE;
 	chip->of_node = pctrl->dev->of_node;
 
+	/* If the GPIO map is sparse, then we need to disable specific IRQs */
+	chip->irq_need_valid_mask = pctrl->soc->sparse;
+
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed register gpiochip\n");
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
+	/*
+	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
+	 * initialize irq_valid_mask to all 1s.  We need to clear all the
+	 * GPIOs that are unavailable, and we need to find each block
+	 * of consecutive available GPIOs are add them as pin ranges.
+	 */
+	if (chip->irq_need_valid_mask) {
+		for (i = 0; i < ngpio; i++)
+			if (!groups[i].npins)
+				clear_bit(i, pctrl->chip.irq_valid_mask);
+
+		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
+			ret = gpiochip_add_pin_range(&pctrl->chip,
+						     dev_name(pctrl->dev),
+						     start, start, count);
+			if (ret)
+				break;
+			start += count;
+		}
+	} else {
+		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
+					     0, 0, ngpio);
+	}
+
 	if (ret) {
 		dev_err(pctrl->dev, "Failed to add pin range\n");
 		gpiochip_remove(&pctrl->chip);
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 9b9feea540ff..70762bcb84cb 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -107,6 +107,7 @@ struct msm_pingroup {
  * @ngroups:	    The numbmer of entries in @groups.
  * @ngpio:	    The number of pingroups the driver should expose as GPIOs.
  * @pull_no_keeper: The SoC does not support keeper bias.
+ * @sparse:         The GPIO map is sparse (some GPIOs have npins == 0)
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -117,6 +118,7 @@ struct msm_pinctrl_soc_data {
 	unsigned ngroups;
 	unsigned ngpios;
 	bool pull_no_keeper;
+	bool sparse;
 };
 
 int msm_pinctrl_probe(struct platform_device *pdev,
-- 
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/2] [v5] 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.

A new boolean 'sparse' indicates whether the GPIO map is sparse.  If any
GPIO has an 'npins' value of 0, then 'sparse' must be set to True.

Most access to unavailable GPIOs can be blocked via the gpio_chip.request
function.  The one exception is when gpiochip_add_data() scans all of
the GPIOs without "requesting" them.  To cover this case,
msm_gpio_get_direction() separately checks if the GPIO is available.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 110 +++++++++++++++++++++++++++++++++++--
 drivers/pinctrl/qcom/pinctrl-msm.h |   2 +
 2 files changed, 106 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index ff491da64dab..ca4ae3d76eb4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -443,6 +443,14 @@ static int msm_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 
 	g = &pctrl->soc->groups[offset];
 
+	/*
+	 * During initialization, gpiolib may query all GPIOs for their
+	 * initial direction, regardless if they exist, so block access
+	 * to those that are unavailable.
+	 */
+	if (!g->npins)
+		return -ENODEV;
+
 	val = readl(pctrl->regs + g->ctl_reg);
 
 	/* 0 = output, 1 = input */
@@ -507,6 +515,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 +529,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 +537,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 -ENODEV;
+
+	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,
 };
@@ -808,11 +834,57 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+/**
+ * msm_gpio_get_next_range() - find next range of consecutive gpios
+ * @pctrl: msm_pinctrl object
+ * @pstart: pointer to index of next starting position
+ *
+ * In a sparse GPIO map, available GPIOs are typically collected into blocks.
+ * Given a starting index into the groups[] array, this function will scan all
+ * the gpios until it finds the next block of available GPIOs.
+ *
+ * If groups[*pstart] already points to an available GPIO, then assume that
+ * it is the start of a block.
+ *
+ * The caller is responsible for moving *pstart to after the end of the
+ * previous block so that it this function can find the next block.
+ *
+ * Return: The count of the block starting at @pstart, or 0 if there are no
+ *         more blocks.
+ */
+static unsigned int msm_gpio_get_next_range(struct msm_pinctrl *pctrl,
+					    unsigned int *pstart)
+{
+	const struct msm_pingroup *groups = pctrl->soc->groups;
+	unsigned int ngpio = pctrl->soc->ngpios;
+	unsigned int count = 0;
+	unsigned int start = *pstart;
+
+	/* Find the first available GPIO */
+	while (!groups[start].npins)
+		if (++start == ngpio)
+			/* None found, so just exit */
+			return 0;
+
+	*pstart = start;
+
+	/* Count the number of those GPIOs in a row */
+	while (groups[start].npins) {
+		count++;
+		if (++start == ngpio)
+			break;
+	}
+
+	return count;
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
 	int ret;
 	unsigned ngpio = pctrl->soc->ngpios;
+	const struct msm_pingroup *groups = pctrl->soc->groups;
+	unsigned int i, start = 0, count;
 
 	if (WARN_ON(ngpio > MAX_NR_GPIO))
 		return -EINVAL;
@@ -825,13 +897,39 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->owner = THIS_MODULE;
 	chip->of_node = pctrl->dev->of_node;
 
+	/* If the GPIO map is sparse, then we need to disable specific IRQs */
+	chip->irq_need_valid_mask = pctrl->soc->sparse;
+
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed register gpiochip\n");
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
+	/*
+	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
+	 * initialize irq_valid_mask to all 1s.  We need to clear all the
+	 * GPIOs that are unavailable, and we need to find each block
+	 * of consecutive available GPIOs are add them as pin ranges.
+	 */
+	if (chip->irq_need_valid_mask) {
+		for (i = 0; i < ngpio; i++)
+			if (!groups[i].npins)
+				clear_bit(i, pctrl->chip.irq_valid_mask);
+
+		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
+			ret = gpiochip_add_pin_range(&pctrl->chip,
+						     dev_name(pctrl->dev),
+						     start, start, count);
+			if (ret)
+				break;
+			start += count;
+		}
+	} else {
+		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
+					     0, 0, ngpio);
+	}
+
 	if (ret) {
 		dev_err(pctrl->dev, "Failed to add pin range\n");
 		gpiochip_remove(&pctrl->chip);
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 9b9feea540ff..70762bcb84cb 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -107,6 +107,7 @@ struct msm_pingroup {
  * @ngroups:	    The numbmer of entries in @groups.
  * @ngpio:	    The number of pingroups the driver should expose as GPIOs.
  * @pull_no_keeper: The SoC does not support keeper bias.
+ * @sparse:         The GPIO map is sparse (some GPIOs have npins == 0)
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -117,6 +118,7 @@ struct msm_pinctrl_soc_data {
 	unsigned ngroups;
 	unsigned ngpios;
 	bool pull_no_keeper;
+	bool sparse;
 };
 
 int msm_pinctrl_probe(struct platform_device *pdev,
-- 
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/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Bjorn Andersson]

On Thu 07 Sep 08:33 PDT 2017, Timur Tabi wrote:

Sorry for the slow response, I finally met with Linus last week to
discuss this.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
[..]
> @@ -825,13 +897,39 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  	chip->owner = THIS_MODULE;
>  	chip->of_node = pctrl->dev->of_node;
>  
> +	/* If the GPIO map is sparse, then we need to disable specific IRQs */
> +	chip->irq_need_valid_mask = pctrl->soc->sparse;
> +
>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>  	if (ret) {
>  		dev_err(pctrl->dev, "Failed register gpiochip\n");
>  		return ret;
>  	}
>  
> -	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
> +	/*
> +	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
> +	 * initialize irq_valid_mask to all 1s.  We need to clear all the
> +	 * GPIOs that are unavailable, and we need to find each block
> +	 * of consecutive available GPIOs are add them as pin ranges.
> +	 */
> +	if (chip->irq_need_valid_mask) {
> +		for (i = 0; i < ngpio; i++)
> +			if (!groups[i].npins)
> +				clear_bit(i, pctrl->chip.irq_valid_mask);
> +
> +		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
> +			ret = gpiochip_add_pin_range(&pctrl->chip,
> +						     dev_name(pctrl->dev),
> +						     start, start, count);
> +			if (ret)
> +				break;
> +			start += count;

I do not fancy the idea of specifying a bitmap of valid irq pins and
then having the driver register the pin-ranges in-between. If we provide
a bitmap of validity to the core it should support using this for the
pins as well. (Which I believe is what Linus answered in the discussion
following patch 0/2)

> +		}
> +	} else {
> +		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> +					     0, 0, ngpio);
> +	}
> +

Regards,
Bjorn

[PATCH 1/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Bjorn Andersson]

On Thu 07 Sep 08:33 PDT 2017, Timur Tabi wrote:

Sorry for the slow response, I finally met with Linus last week to
discuss this.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
[..]
> @@ -825,13 +897,39 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  	chip->owner = THIS_MODULE;
>  	chip->of_node = pctrl->dev->of_node;
>  
> +	/* If the GPIO map is sparse, then we need to disable specific IRQs */
> +	chip->irq_need_valid_mask = pctrl->soc->sparse;
> +
>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>  	if (ret) {
>  		dev_err(pctrl->dev, "Failed register gpiochip\n");
>  		return ret;
>  	}
>  
> -	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
> +	/*
> +	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
> +	 * initialize irq_valid_mask to all 1s.  We need to clear all the
> +	 * GPIOs that are unavailable, and we need to find each block
> +	 * of consecutive available GPIOs are add them as pin ranges.
> +	 */
> +	if (chip->irq_need_valid_mask) {
> +		for (i = 0; i < ngpio; i++)
> +			if (!groups[i].npins)
> +				clear_bit(i, pctrl->chip.irq_valid_mask);
> +
> +		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
> +			ret = gpiochip_add_pin_range(&pctrl->chip,
> +						     dev_name(pctrl->dev),
> +						     start, start, count);
> +			if (ret)
> +				break;
> +			start += count;

I do not fancy the idea of specifying a bitmap of valid irq pins and
then having the driver register the pin-ranges in-between. If we provide
a bitmap of validity to the core it should support using this for the
pins as well. (Which I believe is what Linus answered in the discussion
following patch 0/2)

> +		}
> +	} else {
> +		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> +					     0, 0, ngpio);
> +	}
> +

Regards,
Bjorn

Re: [PATCH 1/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

On 10/02/2017 12:44 PM, Bjorn Andersson wrote:
>> +	/*
>> +	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
>> +	 * initialize irq_valid_mask to all 1s.  We need to clear all the
>> +	 * GPIOs that are unavailable, and we need to find each block
>> +	 * of consecutive available GPIOs are add them as pin ranges.
>> +	 */
>> +	if (chip->irq_need_valid_mask) {
>> +		for (i = 0; i < ngpio; i++)
>> +			if (!groups[i].npins)
>> +				clear_bit(i, pctrl->chip.irq_valid_mask);
>> +
>> +		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
>> +			ret = gpiochip_add_pin_range(&pctrl->chip,
>> +						     dev_name(pctrl->dev),
>> +						     start, start, count);
>> +			if (ret)
>> +				break;
>> +			start += count;
> I do not fancy the idea of specifying a bitmap of valid irq pins and
> then having the driver register the pin-ranges in-between. 

But that's exactly what abx500_gpio_probe() in pinctrl-abx500.c does. 
Here's even a reference to holes in the GPIO space:

/*
  * Compute number of GPIOs from the last SoC gpio range descriptors
  * These ranges may include "holes" but the GPIO number space shall
  * still be homogeneous, so we need to detect and account for any
  * such holes so that these are included in the number of GPIO pins.
  */

 > If we provide
> a bitmap of validity to the core it should support using this for the
> pins as well. (Which I believe is what Linus answered in the discussion
> following patch 0/2)

So you want to change "gpio_chip" to add an "available" callback?  And 
every time gpiolib wants to call a gpio_chip callback, it should call 
->available first?  Like this:

if (chip->available && chip->available())
	status = chip->direction_input(chip, gpio_chip_hwgpio(desc));

I can do that, but it just seems very redundant.  The core already knows 
not to touch GPIOs that are not in a pin range.  The only exception is 
gpiochip_add_data(), as I've stated before.

It just seems wrong to call an API every time to ask permission before 
we can call any other API.  But since the API may not be defined, we 
have to first check if the API exists before we can ask permission.

-- 
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/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

On 10/02/2017 12:44 PM, Bjorn Andersson wrote:
>> +	/*
>> +	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
>> +	 * initialize irq_valid_mask to all 1s.  We need to clear all the
>> +	 * GPIOs that are unavailable, and we need to find each block
>> +	 * of consecutive available GPIOs are add them as pin ranges.
>> +	 */
>> +	if (chip->irq_need_valid_mask) {
>> +		for (i = 0; i < ngpio; i++)
>> +			if (!groups[i].npins)
>> +				clear_bit(i, pctrl->chip.irq_valid_mask);
>> +
>> +		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
>> +			ret = gpiochip_add_pin_range(&pctrl->chip,
>> +						     dev_name(pctrl->dev),
>> +						     start, start, count);
>> +			if (ret)
>> +				break;
>> +			start += count;
> I do not fancy the idea of specifying a bitmap of valid irq pins and
> then having the driver register the pin-ranges in-between. 

But that's exactly what abx500_gpio_probe() in pinctrl-abx500.c does. 
Here's even a reference to holes in the GPIO space:

/*
  * Compute number of GPIOs from the last SoC gpio range descriptors
  * These ranges may include "holes" but the GPIO number space shall
  * still be homogeneous, so we need to detect and account for any
  * such holes so that these are included in the number of GPIO pins.
  */

 > If we provide
> a bitmap of validity to the core it should support using this for the
> pins as well. (Which I believe is what Linus answered in the discussion
> following patch 0/2)

So you want to change "gpio_chip" to add an "available" callback?  And 
every time gpiolib wants to call a gpio_chip callback, it should call 
->available first?  Like this:

if (chip->available && chip->available())
	status = chip->direction_input(chip, gpio_chip_hwgpio(desc));

I can do that, but it just seems very redundant.  The core already knows 
not to touch GPIOs that are not in a pin range.  The only exception is 
gpiochip_add_data(), as I've stated before.

It just seems wrong to call an API every time to ask permission before 
we can call any other API.  But since the API may not be defined, we 
have to first check if the API exists before we can ask permission.

-- 
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/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Linus Walleij]

On Mon, Oct 2, 2017 at 10:47 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 10/02/2017 12:44 PM, Bjorn Andersson wrote:
>>>
>>> +       /*
>>> +        * If irq_need_valid_mask is true, then gpiochip_add_data() will
>>> +        * initialize irq_valid_mask to all 1s.  We need to clear all the
>>> +        * GPIOs that are unavailable, and we need to find each block
>>> +        * of consecutive available GPIOs are add them as pin ranges.
>>> +        */
>>> +       if (chip->irq_need_valid_mask) {
>>> +               for (i = 0; i < ngpio; i++)
>>> +                       if (!groups[i].npins)
>>> +                               clear_bit(i, pctrl->chip.irq_valid_mask);
>>> +
>>> +               while ((count = msm_gpio_get_next_range(pctrl, &start)))
>>> {
>>> +                       ret = gpiochip_add_pin_range(&pctrl->chip,
>>> +
>>> dev_name(pctrl->dev),
>>> +                                                    start, start,
>>> count);
>>> +                       if (ret)
>>> +                               break;
>>> +                       start += count;
>>
>> I do not fancy the idea of specifying a bitmap of valid irq pins and
>> then having the driver register the pin-ranges in-between.
>
> But that's exactly what abx500_gpio_probe() in pinctrl-abx500.c does. Here's
> even a reference to holes in the GPIO space:

This driver is not a good example of what is desireable.

I am sorry that the kernel contains a lot of bad examples. These
are historical artifacts, they cannot be used as an argument to write
code in the same style.

>> If we provide
>>
>> a bitmap of validity to the core it should support using this for the
>> pins as well. (Which I believe is what Linus answered in the discussion
>> following patch 0/2)
>
> So you want to change "gpio_chip" to add an "available" callback?  And every
> time gpiolib wants to call a gpio_chip callback, it should call ->available
> first?  Like this:
>
> if (chip->available && chip->available())
>         status = chip->direction_input(chip, gpio_chip_hwgpio(desc));

I mean that you add
unsigned long *line_valid_mask;
to struct gpio_chip using the same type of logic
as .irq_valid_mask. The mask should be optional.

Then the operation gpiod_get[_index]() will FAIL if the
line is not valid.

There is no need to check on every operation, there should
be no way to get a handle on the pin any other way.

All new code should be using descriptors at this point so we
only need to design for that case.

Yours,
Linus Walleij

[PATCH 1/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Linus Walleij]

On Mon, Oct 2, 2017 at 10:47 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 10/02/2017 12:44 PM, Bjorn Andersson wrote:
>>>
>>> +       /*
>>> +        * If irq_need_valid_mask is true, then gpiochip_add_data() will
>>> +        * initialize irq_valid_mask to all 1s.  We need to clear all the
>>> +        * GPIOs that are unavailable, and we need to find each block
>>> +        * of consecutive available GPIOs are add them as pin ranges.
>>> +        */
>>> +       if (chip->irq_need_valid_mask) {
>>> +               for (i = 0; i < ngpio; i++)
>>> +                       if (!groups[i].npins)
>>> +                               clear_bit(i, pctrl->chip.irq_valid_mask);
>>> +
>>> +               while ((count = msm_gpio_get_next_range(pctrl, &start)))
>>> {
>>> +                       ret = gpiochip_add_pin_range(&pctrl->chip,
>>> +
>>> dev_name(pctrl->dev),
>>> +                                                    start, start,
>>> count);
>>> +                       if (ret)
>>> +                               break;
>>> +                       start += count;
>>
>> I do not fancy the idea of specifying a bitmap of valid irq pins and
>> then having the driver register the pin-ranges in-between.
>
> But that's exactly what abx500_gpio_probe() in pinctrl-abx500.c does. Here's
> even a reference to holes in the GPIO space:

This driver is not a good example of what is desireable.

I am sorry that the kernel contains a lot of bad examples. These
are historical artifacts, they cannot be used as an argument to write
code in the same style.

>> If we provide
>>
>> a bitmap of validity to the core it should support using this for the
>> pins as well. (Which I believe is what Linus answered in the discussion
>> following patch 0/2)
>
> So you want to change "gpio_chip" to add an "available" callback?  And every
> time gpiolib wants to call a gpio_chip callback, it should call ->available
> first?  Like this:
>
> if (chip->available && chip->available())
>         status = chip->direction_input(chip, gpio_chip_hwgpio(desc));

I mean that you add
unsigned long *line_valid_mask;
to struct gpio_chip using the same type of logic
as .irq_valid_mask. The mask should be optional.

Then the operation gpiod_get[_index]() will FAIL if the
line is not valid.

There is no need to check on every operation, there should
be no way to get a handle on the pin any other way.

All new code should be using descriptors at this point so we
only need to design for that case.

Yours,
Linus Walleij

Re: [PATCH 1/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

On 10/07/2017 06:07 AM, Linus Walleij wrote:
> I mean that you add
> unsigned long *line_valid_mask;
> to struct gpio_chip using the same type of logic
> as .irq_valid_mask. The mask should be optional.

Hmmm okay

> Then the operation gpiod_get[_index]() will FAIL if the
> line is not valid.

Just to be clear, you want gpiod_get_index() to check line_valid_mask 
(if present) and return -ENODEV if not valid?

> There is no need to check on every operation, there should
> be no way to get a handle on the pin any other way.
> 
> All new code should be using descriptors at this point so we
> only need to design for that case.

Ok, I will check this.

-- 
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/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

On 10/07/2017 06:07 AM, Linus Walleij wrote:
> I mean that you add
> unsigned long *line_valid_mask;
> to struct gpio_chip using the same type of logic
> as .irq_valid_mask. The mask should be optional.

Hmmm okay

> Then the operation gpiod_get[_index]() will FAIL if the
> line is not valid.

Just to be clear, you want gpiod_get_index() to check line_valid_mask 
(if present) and return -ENODEV if not valid?

> There is no need to check on every operation, there should
> be no way to get a handle on the pin any other way.
> 
> All new code should be using descriptors at this point so we
> only need to design for that case.

Ok, I will check this.

-- 
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/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

On 10/07/2017 06:07 AM, Linus Walleij wrote:
> I mean that you add
> unsigned long *line_valid_mask;
> to struct gpio_chip using the same type of logic
> as .irq_valid_mask. The mask should be optional.
> 
> Then the operation gpiod_get[_index]() will FAIL if the
> line is not valid.
> 
> There is no need to check on every operation, there should
> be no way to get a handle on the pin any other way.
> 
> All new code should be using descriptors at this point so we
> only need to design for that case.

I think I'm missing something, because I implemented what you're asking 
for, but whenever I try to expose a GPIO from sysfs (e.g. echo 37 > 
/sys/class/gpio/export), gpiod_get_index() is not called.  The only 
thing preventing the GPIO from being exported is the ->request callback:

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 -ENODEV;

What operation is supposed to trigger a call to gpiod_get_index?

-- 
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/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

On 10/07/2017 06:07 AM, Linus Walleij wrote:
> I mean that you add
> unsigned long *line_valid_mask;
> to struct gpio_chip using the same type of logic
> as .irq_valid_mask. The mask should be optional.
> 
> Then the operation gpiod_get[_index]() will FAIL if the
> line is not valid.
> 
> There is no need to check on every operation, there should
> be no way to get a handle on the pin any other way.
> 
> All new code should be using descriptors at this point so we
> only need to design for that case.

I think I'm missing something, because I implemented what you're asking 
for, but whenever I try to expose a GPIO from sysfs (e.g. echo 37 > 
/sys/class/gpio/export), gpiod_get_index() is not called.  The only 
thing preventing the GPIO from being exported is the ->request callback:

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 -ENODEV;

What operation is supposed to trigger a call to gpiod_get_index?

-- 
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/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2846 bytes --]

On Thu, Sep 07, 2017 at 10:33:28AM -0500, Timur Tabi wrote:
[...]
>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>  	if (ret) {
>  		dev_err(pctrl->dev, "Failed register gpiochip\n");
>  		return ret;
>  	}
>  
> -	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
> +	/*
> +	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
> +	 * initialize irq_valid_mask to all 1s.  We need to clear all the
> +	 * GPIOs that are unavailable, and we need to find each block
> +	 * of consecutive available GPIOs are add them as pin ranges.
> +	 */
> +	if (chip->irq_need_valid_mask) {
> +		for (i = 0; i < ngpio; i++)
> +			if (!groups[i].npins)
> +				clear_bit(i, pctrl->chip.irq_valid_mask);
> +
> +		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
> +			ret = gpiochip_add_pin_range(&pctrl->chip,
> +						     dev_name(pctrl->dev),
> +						     start, start, count);
> +			if (ret)
> +				break;
> +			start += count;
> +		}
> +	} else {
> +		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> +					     0, 0, ngpio);
> +	}

This is the bit that I don't understand. If you only tell gpiolib about
the GPIOs that exist, then it won't be initializing the .irq_valid_mask
in a wrong way.

In other words, what I'm trying to say is that in your case, ngpio isn't
equal to the sum of .npins over all groups. If instead you make the chip
register only the lines that actually exist, .irq_valid_mask will only
contain bits that do exist.

The reason I'm bringing this up is because we had the same discussion
back in November last year (yes, that driver still isn't upstream...):

	https://lkml.org/lkml/2016/11/22/543

In a nutshell: the Tegra driver was assuming that each port had a fixed
number (8) of lines, but when gpiolib changed to query the direction of
GPIOs at driver probe time this broke badly because on of the instances
of the GPIO controller is very strict about what it allows access to.

This seems to be similar to what you're experiencing. In our case we'd
run into a hard hang trying to access a register for a non-existing
GPIO. One of the possibilities discussed in the thread was to introduce
something akin to what's being proposed here.

In the end it turned out to be easiest to just don't tell gpiolib about
any non-existing GPIOs, because then you don't get to deal with any of
these workarounds. The downside is that you need a little more code to
find out exactly what GPIO you're talking about, or to determine how
many you have. In the end I think it's all worth it by making it a lot
easier in general to deal with. I think it's really confusing if you
expose, say, 200 pins, and for 50 of them users will get -EACCES, or
-ENOENT or whatever if they try to use them.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Thierry Reding]

On Thu, Sep 07, 2017 at 10:33:28AM -0500, Timur Tabi wrote:
[...]
>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>  	if (ret) {
>  		dev_err(pctrl->dev, "Failed register gpiochip\n");
>  		return ret;
>  	}
>  
> -	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
> +	/*
> +	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
> +	 * initialize irq_valid_mask to all 1s.  We need to clear all the
> +	 * GPIOs that are unavailable, and we need to find each block
> +	 * of consecutive available GPIOs are add them as pin ranges.
> +	 */
> +	if (chip->irq_need_valid_mask) {
> +		for (i = 0; i < ngpio; i++)
> +			if (!groups[i].npins)
> +				clear_bit(i, pctrl->chip.irq_valid_mask);
> +
> +		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
> +			ret = gpiochip_add_pin_range(&pctrl->chip,
> +						     dev_name(pctrl->dev),
> +						     start, start, count);
> +			if (ret)
> +				break;
> +			start += count;
> +		}
> +	} else {
> +		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> +					     0, 0, ngpio);
> +	}

This is the bit that I don't understand. If you only tell gpiolib about
the GPIOs that exist, then it won't be initializing the .irq_valid_mask
in a wrong way.

In other words, what I'm trying to say is that in your case, ngpio isn't
equal to the sum of .npins over all groups. If instead you make the chip
register only the lines that actually exist, .irq_valid_mask will only
contain bits that do exist.

The reason I'm bringing this up is because we had the same discussion
back in November last year (yes, that driver still isn't upstream...):

	https://lkml.org/lkml/2016/11/22/543

In a nutshell: the Tegra driver was assuming that each port had a fixed
number (8) of lines, but when gpiolib changed to query the direction of
GPIOs at driver probe time this broke badly because on of the instances
of the GPIO controller is very strict about what it allows access to.

This seems to be similar to what you're experiencing. In our case we'd
run into a hard hang trying to access a register for a non-existing
GPIO. One of the possibilities discussed in the thread was to introduce
something akin to what's being proposed here.

In the end it turned out to be easiest to just don't tell gpiolib about
any non-existing GPIOs, because then you don't get to deal with any of
these workarounds. The downside is that you need a little more code to
find out exactly what GPIO you're talking about, or to determine how
many you have. In the end I think it's all worth it by making it a lot
easier in general to deal with. I think it's really confusing if you
expose, say, 200 pins, and for 50 of them users will get -EACCES, or
-ENOENT or whatever if they try to use them.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171016/2a2bf792/attachment.sig>

Re: [PATCH 1/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

On 10/16/2017 03:01 AM, Thierry Reding wrote:
> This is the bit that I don't understand. If you only tell gpiolib about
> the GPIOs that exist, then it won't be initializing the .irq_valid_mask
> in a wrong way.

I know, and that's what my patches do.  I tell gpiolib that I need a 
valid mask:

/* If the GPIO map is sparse, then we need to disable specific IRQs */
chip->irq_need_valid_mask = pctrl->soc->sparse;

Then I proceed to provide the specific GPIOs that exist, one block at a 
time:

	ret = gpiochip_add_pin_range(&pctrl->chip,
				     dev_name(pctrl->dev),
				     start, start, count);

> In other words, what I'm trying to say is that in your case, ngpio isn't
> equal to the sum of .npins over all groups.

That's true.  I set it to the maximum number of GPIOs that exist on the 
chip.  I could set it to the highest number of GPIOs that I register 
(e.g. the highest value of "start + count"), but that's not necessary 
for my patches to work.

> If instead you make the chip
> register only the lines that actually exist, .irq_valid_mask will only
> contain bits that do exist.

That's what I'm doing.

> The reason I'm bringing this up is because we had the same discussion
> back in November last year (yes, that driver still isn't upstream...):
> 
> 	https://lkml.org/lkml/2016/11/22/543
> 
> In a nutshell: the Tegra driver was assuming that each port had a fixed
> number (8) of lines, but when gpiolib changed to query the direction of
> GPIOs at driver probe time this broke badly because on of the instances
> of the GPIO controller is very strict about what it allows access to.

It appear we're re-hashing that same exact argument.

> This seems to be similar to what you're experiencing. In our case we'd
> run into a hard hang trying to access a register for a non-existing
> GPIO. One of the possibilities discussed in the thread was to introduce
> something akin to what's being proposed here.

I guess great minds do think alike!

> In the end it turned out to be easiest to just don't tell gpiolib about
> any non-existing GPIOs, because then you don't get to deal with any of
> these workarounds.

I agree.

> The downside is that you need a little more code to
> find out exactly what GPIO you're talking about, or to determine how
> many you have.

In my case, the ACPI table provides an exact list that I use at probe() 
time.

> In the end I think it's all worth it by making it a lot
> easier in general to deal with. I think it's really confusing if you
> expose, say, 200 pins, and for 50 of them users will get -EACCES, or
> -ENOENT or whatever if they try to use them.

True, but /sys/kernel/debug/gpio is the only mechanism I found where the 
user can get a list of valid GPIOs.  Maybe we need something similar in 
/sys/class/gpio/.

-- 
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/2] [v5] pinctrl: qcom: disable GPIO groups with no pins

[Timur Tabi]

On 10/16/2017 03:01 AM, Thierry Reding wrote:
> This is the bit that I don't understand. If you only tell gpiolib about
> the GPIOs that exist, then it won't be initializing the .irq_valid_mask
> in a wrong way.

I know, and that's what my patches do.  I tell gpiolib that I need a 
valid mask:

/* If the GPIO map is sparse, then we need to disable specific IRQs */
chip->irq_need_valid_mask = pctrl->soc->sparse;

Then I proceed to provide the specific GPIOs that exist, one block at a 
time:

	ret = gpiochip_add_pin_range(&pctrl->chip,
				     dev_name(pctrl->dev),
				     start, start, count);

> In other words, what I'm trying to say is that in your case, ngpio isn't
> equal to the sum of .npins over all groups.

That's true.  I set it to the maximum number of GPIOs that exist on the 
chip.  I could set it to the highest number of GPIOs that I register 
(e.g. the highest value of "start + count"), but that's not necessary 
for my patches to work.

> If instead you make the chip
> register only the lines that actually exist, .irq_valid_mask will only
> contain bits that do exist.

That's what I'm doing.

> The reason I'm bringing this up is because we had the same discussion
> back in November last year (yes, that driver still isn't upstream...):
> 
> 	https://lkml.org/lkml/2016/11/22/543
> 
> In a nutshell: the Tegra driver was assuming that each port had a fixed
> number (8) of lines, but when gpiolib changed to query the direction of
> GPIOs at driver probe time this broke badly because on of the instances
> of the GPIO controller is very strict about what it allows access to.

It appear we're re-hashing that same exact argument.

> This seems to be similar to what you're experiencing. In our case we'd
> run into a hard hang trying to access a register for a non-existing
> GPIO. One of the possibilities discussed in the thread was to introduce
> something akin to what's being proposed here.

I guess great minds do think alike!

> In the end it turned out to be easiest to just don't tell gpiolib about
> any non-existing GPIOs, because then you don't get to deal with any of
> these workarounds.

I agree.

> The downside is that you need a little more code to
> find out exactly what GPIO you're talking about, or to determine how
> many you have.

In my case, the ACPI table provides an exact list that I use at probe() 
time.

> In the end I think it's all worth it by making it a lot
> easier in general to deal with. I think it's really confusing if you
> expose, say, 200 pins, and for 50 of them users will get -EACCES, or
> -ENOENT or whatever if they try to use them.

True, but /sys/kernel/debug/gpio is the only mechanism I found where the 
user can get a list of valid GPIOs.  Maybe we need something similar in 
/sys/class/gpio/.

-- 
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/2] [v3] 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 | 145 +++++++++++++++++++++++++--------
 1 file changed, 109 insertions(+), 36 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
index bb3ce5c3e18b..37f746f6eb8c 100644
--- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
+++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
@@ -38,68 +38,147 @@
 /* maximum size of each gpio name (enough room for "gpioXXX" + null) */
 #define NAME_SIZE	8
 
+enum {
+	QDF2XXX_V1,
+	QDF2XXX_V2,
+};
+
+static const struct acpi_device_id qdf2xxx_acpi_ids[] = {
+	{"QCOM8001", QDF2XXX_V1},
+	{"QCOM8002", QDF2XXX_V2},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
+
 static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 {
+	const struct acpi_device_id *id =
+		acpi_match_device(qdf2xxx_acpi_ids, &pdev->dev);
 	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.
+	 */
+	if (id->driver_data == QDF2XXX_V1) {
+		avail_gpios = num_gpios;
+
+		gpios = devm_kcalloc(&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 'num-gpios' property\n");
+			return ret;
+		}
+		if (!ret || ret > MAX_GPIOS) {
+			dev_err(&pdev->dev, "invalid 'num-gpios' property\n");
+			return -ENODEV;
+		}
+		avail_gpios = ret;
+
+		gpios = devm_kcalloc(&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;
+		}
+
+		/*
+		 * Because we have a specific list of GPIOs, the GPIO map
+		 * is 'sparse'.
+		 */
+		qdf2xxx_pinctrl.sparse = true;
+	}
+
 	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;
@@ -109,12 +188,6 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 	return msm_pinctrl_probe(pdev, &qdf2xxx_pinctrl);
 }
 
-static const struct acpi_device_id qdf2xxx_acpi_ids[] = {
-	{"QCOM8001"},
-	{},
-};
-MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
-
 static struct platform_driver qdf2xxx_pinctrl_driver = {
 	.driver = {
 		.name = "qdf2xxx-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.

[PATCH 2/2] [v3] 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 | 145 +++++++++++++++++++++++++--------
 1 file changed, 109 insertions(+), 36 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
index bb3ce5c3e18b..37f746f6eb8c 100644
--- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
+++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
@@ -38,68 +38,147 @@
 /* maximum size of each gpio name (enough room for "gpioXXX" + null) */
 #define NAME_SIZE	8
 
+enum {
+	QDF2XXX_V1,
+	QDF2XXX_V2,
+};
+
+static const struct acpi_device_id qdf2xxx_acpi_ids[] = {
+	{"QCOM8001", QDF2XXX_V1},
+	{"QCOM8002", QDF2XXX_V2},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
+
 static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 {
+	const struct acpi_device_id *id =
+		acpi_match_device(qdf2xxx_acpi_ids, &pdev->dev);
 	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.
+	 */
+	if (id->driver_data == QDF2XXX_V1) {
+		avail_gpios = num_gpios;
+
+		gpios = devm_kcalloc(&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 'num-gpios' property\n");
+			return ret;
+		}
+		if (!ret || ret > MAX_GPIOS) {
+			dev_err(&pdev->dev, "invalid 'num-gpios' property\n");
+			return -ENODEV;
+		}
+		avail_gpios = ret;
+
+		gpios = devm_kcalloc(&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;
+		}
+
+		/*
+		 * Because we have a specific list of GPIOs, the GPIO map
+		 * is 'sparse'.
+		 */
+		qdf2xxx_pinctrl.sparse = true;
+	}
+
 	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;
@@ -109,12 +188,6 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 	return msm_pinctrl_probe(pdev, &qdf2xxx_pinctrl);
 }
 
-static const struct acpi_device_id qdf2xxx_acpi_ids[] = {
-	{"QCOM8001"},
-	{},
-};
-MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
-
 static struct platform_driver qdf2xxx_pinctrl_driver = {
 	.driver = {
 		.name = "qdf2xxx-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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Thu, Sep 7, 2017 at 5:33 PM, Timur Tabi <timur@codeaurora.org> wrote:

> First patch allows for for pinctrl-msm to understand GPIO groups with
> no pins.  Such pins are "hidden" and can't be exported or accessed.
>
> Second patch updates the QDF2xxx driver to take advantage of all that.
>
> v5:
>  Since gpiochip_add_data no longer requests GPIOs before scanning for
>  the direction (that patch was reverted), pinctrl-msm.c now specifically
>  checks for special case.

Waiting for Bjorn's review on these.

Yours,
Linus Walleij

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Thu, Sep 7, 2017 at 5:33 PM, Timur Tabi <timur@codeaurora.org> wrote:

> First patch allows for for pinctrl-msm to understand GPIO groups with
> no pins.  Such pins are "hidden" and can't be exported or accessed.
>
> Second patch updates the QDF2xxx driver to take advantage of all that.
>
> v5:
>  Since gpiochip_add_data no longer requests GPIOs before scanning for
>  the direction (that patch was reverted), pinctrl-msm.c now specifically
>  checks for special case.

Waiting for Bjorn's review on these.

Yours,
Linus Walleij

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 09/08/2017 07:50 AM, Linus Walleij wrote:
>>   Since gpiochip_add_data no longer requests GPIOs before scanning for
>>   the direction (that patch was reverted), pinctrl-msm.c now specifically
>>   checks for special case.
> Waiting for Bjorn's review on these.

Bjorn,

Do you think you will have a chance to review these patches in time for 
4.14?

-- 
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/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 09/08/2017 07:50 AM, Linus Walleij wrote:
>>   Since gpiochip_add_data no longer requests GPIOs before scanning for
>>   the direction (that patch was reverted), pinctrl-msm.c now specifically
>>   checks for special case.
> Waiting for Bjorn's review on these.

Bjorn,

Do you think you will have a chance to review these patches in time for 
4.14?

-- 
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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 09/07, Timur Tabi wrote:
> First patch allows for for pinctrl-msm to understand GPIO groups with
> no pins.  Such pins are "hidden" and can't be exported or accessed.
> 
> Second patch updates the QDF2xxx driver to take advantage of all that.
> 
> v5:
>  Since gpiochip_add_data no longer requests GPIOs before scanning for
>  the direction (that patch was reverted), pinctrl-msm.c now specifically
>  checks for special case.

Can we add a new gpiochip op that checks for "availability". I
read the other thread where the change was reverted (please add a
pointer next time), and as I understand it the gpio request
method can also change the muxing to a gpio instead of something
else. Perhaps we can add another hook for our purposes here that
tells gpiolib that the gpio is not usable and to skip it. The
semantics would be clear, it's just about probing availability of
this pin as a gpio and doesn't mux any pins.

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

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 09/07, Timur Tabi wrote:
> First patch allows for for pinctrl-msm to understand GPIO groups with
> no pins.  Such pins are "hidden" and can't be exported or accessed.
> 
> Second patch updates the QDF2xxx driver to take advantage of all that.
> 
> v5:
>  Since gpiochip_add_data no longer requests GPIOs before scanning for
>  the direction (that patch was reverted), pinctrl-msm.c now specifically
>  checks for special case.

Can we add a new gpiochip op that checks for "availability". I
read the other thread where the change was reverted (please add a
pointer next time), and as I understand it the gpio request
method can also change the muxing to a gpio instead of something
else. Perhaps we can add another hook for our purposes here that
tells gpiolib that the gpio is not usable and to skip it. The
semantics would be clear, it's just about probing availability of
this pin as a gpio and doesn't mux any pins.

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

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Tue, Sep 19, 2017 at 9:04 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> Perhaps we can add another hook for our purposes here that
> tells gpiolib that the gpio is not usable and to skip it. The
> semantics would be clear, it's just about probing availability of
> this pin as a gpio and doesn't mux any pins.

Oh we already have that I think, Mika Westerberg and Andy Shevcheno
implemented that for anyone using CONFIG_GPIOLIB_IRQCHIP, and
this driver does. Timur please check: irq_need_valid_mask, irq_valid_mask
usage.

Helpful commits:
commit 49c03096263871a68c9dea3e86b7d1e163d2fba8
"pinctrl: baytrail: Do not add all GPIOs to IRQ domain"

Then you can see in commits:
commit 7036502783729c2aaf7a3c24c89087c58721430f
"pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again"
commit 2a8209fa68236ad65363dba03db5dbced520268a
"pinctrl: cherryview: Extend the Chromebook DMI quirk to Intel_Strago systems"

How this valid mask is used to work around specific ACPI
issues on Intel chips.

I bet a million to one that you have the same problems as them,
and then we should also deal with it the same way.

Sorry for not seeing the obvious connection earlier.

Yours,
Linus Walleij

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Tue, Sep 19, 2017 at 9:04 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> Perhaps we can add another hook for our purposes here that
> tells gpiolib that the gpio is not usable and to skip it. The
> semantics would be clear, it's just about probing availability of
> this pin as a gpio and doesn't mux any pins.

Oh we already have that I think, Mika Westerberg and Andy Shevcheno
implemented that for anyone using CONFIG_GPIOLIB_IRQCHIP, and
this driver does. Timur please check: irq_need_valid_mask, irq_valid_mask
usage.

Helpful commits:
commit 49c03096263871a68c9dea3e86b7d1e163d2fba8
"pinctrl: baytrail: Do not add all GPIOs to IRQ domain"

Then you can see in commits:
commit 7036502783729c2aaf7a3c24c89087c58721430f
"pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again"
commit 2a8209fa68236ad65363dba03db5dbced520268a
"pinctrl: cherryview: Extend the Chromebook DMI quirk to Intel_Strago systems"

How this valid mask is used to work around specific ACPI
issues on Intel chips.

I bet a million to one that you have the same problems as them,
and then we should also deal with it the same way.

Sorry for not seeing the obvious connection earlier.

Yours,
Linus Walleij

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 9/19/17 3:15 AM, Linus Walleij wrote:
> Oh we already have that I think, Mika Westerberg and Andy Shevcheno
> implemented that for anyone using CONFIG_GPIOLIB_IRQCHIP, and
> this driver does. Timur please check: irq_need_valid_mask, irq_valid_mask
> usage.

These patches already use irq_valid_mask!  But that doesn't block 
complete access to the GPIO.


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 9/19/17 3:15 AM, Linus Walleij wrote:
> Oh we already have that I think, Mika Westerberg and Andy Shevcheno
> implemented that for anyone using CONFIG_GPIOLIB_IRQCHIP, and
> this driver does. Timur please check: irq_need_valid_mask, irq_valid_mask
> usage.

These patches already use irq_valid_mask!  But that doesn't block 
complete access to the GPIO.


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Tue, Sep 19, 2017 at 2:32 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 9/19/17 3:15 AM, Linus Walleij wrote:
>>
>> Oh we already have that I think, Mika Westerberg and Andy Shevcheno
>> implemented that for anyone using CONFIG_GPIOLIB_IRQCHIP, and
>> this driver does. Timur please check: irq_need_valid_mask, irq_valid_mask
>> usage.
>
> These patches already use irq_valid_mask!  But that doesn't block complete
> access to the GPIO.

Aha sorry for my ignorance :(

Doesn't that mean we need something like irq_valid_mask but rather
gpio_valid_mask that just block all usage of certain GPIOs?

Yours,
Linus Walleij

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Tue, Sep 19, 2017 at 2:32 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 9/19/17 3:15 AM, Linus Walleij wrote:
>>
>> Oh we already have that I think, Mika Westerberg and Andy Shevcheno
>> implemented that for anyone using CONFIG_GPIOLIB_IRQCHIP, and
>> this driver does. Timur please check: irq_need_valid_mask, irq_valid_mask
>> usage.
>
> These patches already use irq_valid_mask!  But that doesn't block complete
> access to the GPIO.

Aha sorry for my ignorance :(

Doesn't that mean we need something like irq_valid_mask but rather
gpio_valid_mask that just block all usage of certain GPIOs?

Yours,
Linus Walleij

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 9/20/17 6:43 AM, Linus Walleij wrote:
> Doesn't that mean we need something like irq_valid_mask but rather
> gpio_valid_mask that just block all usage of certain GPIOs?

That raises a lot of questions.  In the meantime, my current patches for 
4.14 work fine.

Do we replace irq_valid_mask with gpio_valid_mask?  That would break 
drivers where the GPIO is valid but the interrupt is not.  If we keep 
both, what happens if gpio_valid_mask is false but irq_valid_mask is 
true?  And then we would need to audit all gpio drivers to see which 
ones should be updated for the new infrastructure.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 9/20/17 6:43 AM, Linus Walleij wrote:
> Doesn't that mean we need something like irq_valid_mask but rather
> gpio_valid_mask that just block all usage of certain GPIOs?

That raises a lot of questions.  In the meantime, my current patches for 
4.14 work fine.

Do we replace irq_valid_mask with gpio_valid_mask?  That would break 
drivers where the GPIO is valid but the interrupt is not.  If we keep 
both, what happens if gpio_valid_mask is false but irq_valid_mask is 
true?  And then we would need to audit all gpio drivers to see which 
ones should be updated for the new infrastructure.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Wed, Sep 20, 2017 at 3:04 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 9/20/17 6:43 AM, Linus Walleij wrote:
>>
>> Doesn't that mean we need something like irq_valid_mask but rather
>> gpio_valid_mask that just block all usage of certain GPIOs?
>
>
> That raises a lot of questions.  In the meantime, my current patches for
> 4.14 work fine.
>
> Do we replace irq_valid_mask with gpio_valid_mask?  That would break drivers
> where the GPIO is valid but the interrupt is not.  If we keep both, what
> happens if gpio_valid_mask is false but irq_valid_mask is true?  And then we
> would need to audit all gpio drivers to see which ones should be updated for
> the new infrastructure.

I guess gpio_valid_mask would take precedence over irq_valid_mask.
I.e if the GPIO is not valid then the IRQ is per definition not valid either.

Since it is a new thing, we can simply define a semantic like that
and document it.

Yours,
Linus Walleij

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Wed, Sep 20, 2017 at 3:04 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 9/20/17 6:43 AM, Linus Walleij wrote:
>>
>> Doesn't that mean we need something like irq_valid_mask but rather
>> gpio_valid_mask that just block all usage of certain GPIOs?
>
>
> That raises a lot of questions.  In the meantime, my current patches for
> 4.14 work fine.
>
> Do we replace irq_valid_mask with gpio_valid_mask?  That would break drivers
> where the GPIO is valid but the interrupt is not.  If we keep both, what
> happens if gpio_valid_mask is false but irq_valid_mask is true?  And then we
> would need to audit all gpio drivers to see which ones should be updated for
> the new infrastructure.

I guess gpio_valid_mask would take precedence over irq_valid_mask.
I.e if the GPIO is not valid then the IRQ is per definition not valid either.

Since it is a new thing, we can simply define a semantic like that
and document it.

Yours,
Linus Walleij

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 9/21/17 7:08 AM, Linus Walleij wrote:
> I guess gpio_valid_mask would take precedence over irq_valid_mask.
> I.e if the GPIO is not valid then the IRQ is per definition not valid either.
> 
> Since it is a new thing, we can simply define a semantic like that
> and document it.

So what about my current patches?  I hope you're not asking me to 
rewrite them again.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 9/21/17 7:08 AM, Linus Walleij wrote:
> I guess gpio_valid_mask would take precedence over irq_valid_mask.
> I.e if the GPIO is not valid then the IRQ is per definition not valid either.
> 
> Since it is a new thing, we can simply define a semantic like that
> and document it.

So what about my current patches?  I hope you're not asking me to 
rewrite them again.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Thu, Sep 21, 2017 at 2:12 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 9/21/17 7:08 AM, Linus Walleij wrote:
>>
>> I guess gpio_valid_mask would take precedence over irq_valid_mask.
>> I.e if the GPIO is not valid then the IRQ is per definition not valid
>> either.
>>
>> Since it is a new thing, we can simply define a semantic like that
>> and document it.
>
> So what about my current patches?

I am waiting for the maintainer, Bjorn Andersson, to provide review.

>  I hope you're not asking me to rewrite
> them again.

I don't understand your remark. If you are impatient, such is life.

What is your response to Stephen's comment:

> [Stephen Boyd]
> Perhaps we can add another hook for our purposes here that
> tells gpiolib that the gpio is not usable and to skip it. The
> semantics would be clear, it's just about probing availability of
> this pin as a gpio and doesn't mux any pins.

I think this kind of related to my response (after I realized it
was not just about IRQs):

> Doesn't that mean we need something like irq_valid_mask but rather
> gpio_valid_mask that just block all usage of certain GPIOs?

Yours,
Linus Walleij

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Thu, Sep 21, 2017 at 2:12 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 9/21/17 7:08 AM, Linus Walleij wrote:
>>
>> I guess gpio_valid_mask would take precedence over irq_valid_mask.
>> I.e if the GPIO is not valid then the IRQ is per definition not valid
>> either.
>>
>> Since it is a new thing, we can simply define a semantic like that
>> and document it.
>
> So what about my current patches?

I am waiting for the maintainer, Bjorn Andersson, to provide review.

>  I hope you're not asking me to rewrite
> them again.

I don't understand your remark. If you are impatient, such is life.

What is your response to Stephen's comment:

> [Stephen Boyd]
> Perhaps we can add another hook for our purposes here that
> tells gpiolib that the gpio is not usable and to skip it. The
> semantics would be clear, it's just about probing availability of
> this pin as a gpio and doesn't mux any pins.

I think this kind of related to my response (after I realized it
was not just about IRQs):

> Doesn't that mean we need something like irq_valid_mask but rather
> gpio_valid_mask that just block all usage of certain GPIOs?

Yours,
Linus Walleij

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 09/22/2017 08:29 AM, Linus Walleij wrote:
> 
> What is your response to Stephen's comment:
> 
>> [Stephen Boyd]
>> Perhaps we can add another hook for our purposes here that
>> tells gpiolib that the gpio is not usable and to skip it. The
>> semantics would be clear, it's just about probing availability of
>> this pin as a gpio and doesn't mux any pins.

> I think this kind of related to my response (after I realized it
> was not just about IRQs):

We already have 95% of this.  We can already specify individual pin 
ranges, and the vast majority of the code recognizes the ranges.  There 
is only one small loophole, and that's in gpiochip_add_data().  The 
for-loop iterates over all GPIOs:

	for (i = 0; i < chip->ngpio; i++) {
		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.
		 */

I believe the real problem is that this for-loop should be moved from 
gpiochip_add_data() into some other function that is called *after* the 
pin ranges are defined.  We can put it in gpiochip_add_pin_range(), maybe.

My patch covers the loophole by adding a check inside get_direction(). 
If we fix gpiochip_add_data(), I can remove that patch.

However, I think that change is risky and will require a lot of testing 
and review.

-- 
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/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 09/22/2017 08:29 AM, Linus Walleij wrote:
> 
> What is your response to Stephen's comment:
> 
>> [Stephen Boyd]
>> Perhaps we can add another hook for our purposes here that
>> tells gpiolib that the gpio is not usable and to skip it. The
>> semantics would be clear, it's just about probing availability of
>> this pin as a gpio and doesn't mux any pins.

> I think this kind of related to my response (after I realized it
> was not just about IRQs):

We already have 95% of this.  We can already specify individual pin 
ranges, and the vast majority of the code recognizes the ranges.  There 
is only one small loophole, and that's in gpiochip_add_data().  The 
for-loop iterates over all GPIOs:

	for (i = 0; i < chip->ngpio; i++) {
		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.
		 */

I believe the real problem is that this for-loop should be moved from 
gpiochip_add_data() into some other function that is called *after* the 
pin ranges are defined.  We can put it in gpiochip_add_pin_range(), maybe.

My patch covers the loophole by adding a check inside get_direction(). 
If we fix gpiochip_add_data(), I can remove that patch.

However, I think that change is risky and will require a lot of testing 
and review.

-- 
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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 09/22, Timur Tabi wrote:
> On 09/22/2017 08:29 AM, Linus Walleij wrote:
> >
> >What is your response to Stephen's comment:
> >
> >>[Stephen Boyd]
> >>Perhaps we can add another hook for our purposes here that
> >>tells gpiolib that the gpio is not usable and to skip it. The
> >>semantics would be clear, it's just about probing availability of
> >>this pin as a gpio and doesn't mux any pins.
> 
> >I think this kind of related to my response (after I realized it
> >was not just about IRQs):
> 
> We already have 95% of this.  We can already specify individual pin
> ranges, and the vast majority of the code recognizes the ranges.
> There is only one small loophole, and that's in gpiochip_add_data().
> The for-loop iterates over all GPIOs:
> 
> 	for (i = 0; i < chip->ngpio; i++) {
> 		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.
> 		 */
> 
> I believe the real problem is that this for-loop should be moved
> from gpiochip_add_data() into some other function that is called
> *after* the pin ranges are defined.  We can put it in
> gpiochip_add_pin_range(), maybe.
> 
> My patch covers the loophole by adding a check inside
> get_direction(). If we fix gpiochip_add_data(), I can remove that
> patch.
> 
> However, I think that change is risky and will require a lot of
> testing and review.
> 

I've run into this now on our mobile SoCs after I pull in commit
8e51533780ba ("pinctrl: qcom: add get_direction function").
Before that commit we never read each pin of the device. On our
mobile SoCs we have devicetree and it feels like having that
describe which pins are available and not available is
half-duplicating information we would already have via consumers
indicating which pins they care about. I don't see any value
beyond system wide debug in figuring out the default pin
configuration of a pin that doesn't have a consumer in Linux.

Could we remove the pin direction finding part here in
gpiochip_add_pin_range() and lazily resolve the pin direction
when a pin is requested? We would need a similar check in the msm
specific debugfs code where we skip pins that aren't requested.
This is basically a revert of commit 72d320006177 ("gpio: set up
initial state from .get_direction()").

ACPI can still describe only the pin ranges that they care about
exposing, but from the devicetree side it's been working well
enough to not touch pins that aren't used by anything in Linux.

---8<----
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cd003b74512f..673028823bc5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
 		 * 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 (!chip->direction_input) {
 			/*
 			 * If the chip lacks the .direction_input callback
 			 * we logically assume all lines are outputs.
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 273badd92561..4a0aeceb42f1 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -24,7 +24,7 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/reboot.h>
@@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 	};
 
 	g = &pctrl->soc->groups[offset];
+
+	if (!gpiochip_is_requested(chip, gpio)) {
+		seq_printf(s, " %-8s:", g->name);
+		return;
+	}
+
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
 	is_out = !!(ctl_reg & BIT(g->oe_bit));

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

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 09/22, Timur Tabi wrote:
> On 09/22/2017 08:29 AM, Linus Walleij wrote:
> >
> >What is your response to Stephen's comment:
> >
> >>[Stephen Boyd]
> >>Perhaps we can add another hook for our purposes here that
> >>tells gpiolib that the gpio is not usable and to skip it. The
> >>semantics would be clear, it's just about probing availability of
> >>this pin as a gpio and doesn't mux any pins.
> 
> >I think this kind of related to my response (after I realized it
> >was not just about IRQs):
> 
> We already have 95% of this.  We can already specify individual pin
> ranges, and the vast majority of the code recognizes the ranges.
> There is only one small loophole, and that's in gpiochip_add_data().
> The for-loop iterates over all GPIOs:
> 
> 	for (i = 0; i < chip->ngpio; i++) {
> 		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.
> 		 */
> 
> I believe the real problem is that this for-loop should be moved
> from gpiochip_add_data() into some other function that is called
> *after* the pin ranges are defined.  We can put it in
> gpiochip_add_pin_range(), maybe.
> 
> My patch covers the loophole by adding a check inside
> get_direction(). If we fix gpiochip_add_data(), I can remove that
> patch.
> 
> However, I think that change is risky and will require a lot of
> testing and review.
> 

I've run into this now on our mobile SoCs after I pull in commit
8e51533780ba ("pinctrl: qcom: add get_direction function").
Before that commit we never read each pin of the device. On our
mobile SoCs we have devicetree and it feels like having that
describe which pins are available and not available is
half-duplicating information we would already have via consumers
indicating which pins they care about. I don't see any value
beyond system wide debug in figuring out the default pin
configuration of a pin that doesn't have a consumer in Linux.

Could we remove the pin direction finding part here in
gpiochip_add_pin_range() and lazily resolve the pin direction
when a pin is requested? We would need a similar check in the msm
specific debugfs code where we skip pins that aren't requested.
This is basically a revert of commit 72d320006177 ("gpio: set up
initial state from .get_direction()").

ACPI can still describe only the pin ranges that they care about
exposing, but from the devicetree side it's been working well
enough to not touch pins that aren't used by anything in Linux.

---8<----
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cd003b74512f..673028823bc5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
 		 * 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 (!chip->direction_input) {
 			/*
 			 * If the chip lacks the .direction_input callback
 			 * we logically assume all lines are outputs.
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 273badd92561..4a0aeceb42f1 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -24,7 +24,7 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/reboot.h>
@@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 	};
 
 	g = &pctrl->soc->groups[offset];
+
+	if (!gpiochip_is_requested(chip, gpio)) {
+		seq_printf(s, " %-8s:", g->name);
+		return;
+	}
+
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
 	is_out = !!(ctl_reg & BIT(g->oe_bit));

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

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/03/2017 05:03 PM, Stephen Boyd wrote:
> I've run into this now on our mobile SoCs after I pull in commit
> 8e51533780ba ("pinctrl: qcom: add get_direction function").
> Before that commit we never read each pin of the device. On our
> mobile SoCs we have devicetree and it feels like having that
> describe which pins are available and not available is
> half-duplicating information we would already have via consumers
> indicating which pins they care about. I don't see any value
> beyond system wide debug in figuring out the default pin
> configuration of a pin that doesn't have a consumer in Linux.

At the time I wrote that patch, the ACPI tables exposed all of the 
GPIOs, even the ones it didn't care about.  The new ACPI tables list 
only specific GPIOs, and so we no longer need to blindly read the 
direction of all GPIOs.

> Could we remove the pin direction finding part here in
> gpiochip_add_pin_range() and lazily resolve the pin direction
> when a pin is requested?

That makes a lot more sense.

> We would need a similar check in the msm
> specific debugfs code where we skip pins that aren't requested.

I have that in patch #1.

> This is basically a revert of commit 72d320006177 ("gpio: set up
> initial state from .get_direction()").

I would be in favor of either reverting that patch, or moving the code 
into gpiochip_add_pin_range().

> ACPI can still describe only the pin ranges that they care about
> exposing, but from the devicetree side it's been working well
> enough to not touch pins that aren't used by anything in Linux.

I do hate having to hack up the driver to support crappy ACPI 
definitions, but I'm stuck between a rock and a hard place.

-- 
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/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/03/2017 05:03 PM, Stephen Boyd wrote:
> I've run into this now on our mobile SoCs after I pull in commit
> 8e51533780ba ("pinctrl: qcom: add get_direction function").
> Before that commit we never read each pin of the device. On our
> mobile SoCs we have devicetree and it feels like having that
> describe which pins are available and not available is
> half-duplicating information we would already have via consumers
> indicating which pins they care about. I don't see any value
> beyond system wide debug in figuring out the default pin
> configuration of a pin that doesn't have a consumer in Linux.

At the time I wrote that patch, the ACPI tables exposed all of the 
GPIOs, even the ones it didn't care about.  The new ACPI tables list 
only specific GPIOs, and so we no longer need to blindly read the 
direction of all GPIOs.

> Could we remove the pin direction finding part here in
> gpiochip_add_pin_range() and lazily resolve the pin direction
> when a pin is requested?

That makes a lot more sense.

> We would need a similar check in the msm
> specific debugfs code where we skip pins that aren't requested.

I have that in patch #1.

> This is basically a revert of commit 72d320006177 ("gpio: set up
> initial state from .get_direction()").

I would be in favor of either reverting that patch, or moving the code 
into gpiochip_add_pin_range().

> ACPI can still describe only the pin ranges that they care about
> exposing, but from the devicetree side it's been working well
> enough to not touch pins that aren't used by anything in Linux.

I do hate having to hack up the driver to support crappy ACPI 
definitions, but I'm stuck between a rock and a hard place.

-- 
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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 10/03, Timur Tabi wrote:
> On 10/03/2017 05:03 PM, Stephen Boyd wrote:
> >I've run into this now on our mobile SoCs after I pull in commit
> >8e51533780ba ("pinctrl: qcom: add get_direction function").
> >Before that commit we never read each pin of the device. On our
> >mobile SoCs we have devicetree and it feels like having that
> >describe which pins are available and not available is
> >half-duplicating information we would already have via consumers
> >indicating which pins they care about. I don't see any value
> >beyond system wide debug in figuring out the default pin
> >configuration of a pin that doesn't have a consumer in Linux.
> 
> At the time I wrote that patch, the ACPI tables exposed all of the
> GPIOs, even the ones it didn't care about.  The new ACPI tables list
> only specific GPIOs, and so we no longer need to blindly read the
> direction of all GPIOs.
> 

Do you avoid this problem on new ACPI tables because only pins
that are able to be read are exposed?

> 
> >This is basically a revert of commit 72d320006177 ("gpio: set up
> >initial state from .get_direction()").
> 
> I would be in favor of either reverting that patch, or moving the
> code into gpiochip_add_pin_range().

If it's in gpiochip_add_pin_range() would we still read the
hardware when creating the pin ranges? I don't want to have to
describe pin ranges of "valid" pins that won't cause the system
to blow up if we touch them, because those pins are never used by
Linux so reading them is not useful.

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

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 10/03, Timur Tabi wrote:
> On 10/03/2017 05:03 PM, Stephen Boyd wrote:
> >I've run into this now on our mobile SoCs after I pull in commit
> >8e51533780ba ("pinctrl: qcom: add get_direction function").
> >Before that commit we never read each pin of the device. On our
> >mobile SoCs we have devicetree and it feels like having that
> >describe which pins are available and not available is
> >half-duplicating information we would already have via consumers
> >indicating which pins they care about. I don't see any value
> >beyond system wide debug in figuring out the default pin
> >configuration of a pin that doesn't have a consumer in Linux.
> 
> At the time I wrote that patch, the ACPI tables exposed all of the
> GPIOs, even the ones it didn't care about.  The new ACPI tables list
> only specific GPIOs, and so we no longer need to blindly read the
> direction of all GPIOs.
> 

Do you avoid this problem on new ACPI tables because only pins
that are able to be read are exposed?

> 
> >This is basically a revert of commit 72d320006177 ("gpio: set up
> >initial state from .get_direction()").
> 
> I would be in favor of either reverting that patch, or moving the
> code into gpiochip_add_pin_range().

If it's in gpiochip_add_pin_range() would we still read the
hardware when creating the pin ranges? I don't want to have to
describe pin ranges of "valid" pins that won't cause the system
to blow up if we touch them, because those pins are never used by
Linux so reading them is not useful.

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

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/04/2017 04:50 PM, Stephen Boyd wrote:

>> At the time I wrote that patch, the ACPI tables exposed all of the
>> GPIOs, even the ones it didn't care about.  The new ACPI tables list
>> only specific GPIOs, and so we no longer need to blindly read the
>> direction of all GPIOs.
>>
> 
> Do you avoid this problem on new ACPI tables because only pins
> that are able to be read are exposed?

Yes.  A recent firmware update enabled the "XPU" block which is being 
programmed with a select subset of individual GPIOs.  On our silicon, 
each TLMM GPIO is in a separate 64k page, and so the XPU can block any 
individual GPIO.  Any attempt to touch those registers causes an XPU 
violation which takes the whole system down.

>>> This is basically a revert of commit 72d320006177 ("gpio: set up
>>> initial state from .get_direction()").
>>
>> I would be in favor of either reverting that patch, or moving the
>> code into gpiochip_add_pin_range().
> 
> If it's in gpiochip_add_pin_range() would we still read the
> hardware when creating the pin ranges?

I presume so.  The idea is that pinctrl-qdf2xxx/pinctrl-msm only submit 
pin ranges that are present in the ACPI tables.

 > I don't want to have to> describe pin ranges of "valid" pins that 
won't cause the system
> to blow up if we touch them, because those pins are never used by
> Linux so reading them is not useful.

Well, that's exactly what I'm trying to do with current patch set :-) 
It seems the most logical approach to me.  I don't understand the 
dislike for it.  What else are pin ranges for, other than to specify 
ranges of pins that can be accessed?

Another alternative was to enumerate all of the GPIOs starting from 0. 
So the first GPIO in ACPI would be gpio0, regardless of what gpio number 
it actually was.  E.g. GPIO 37 would appear as gpio0, GPIO 38 would 
appear as gpio1, and so on.  That also worked, but it meant that 
customers would need to figure out which GPIO that "gpio0" actually 
pointed to.  That was not acceptable, so I dropped it.

I'm at a loss on how else to do it.  I think a gpio_chip.available 
callback is far less elegant than define pin ranges.  There is no chance 
that unavailable GPIOs can be accessed because the physical addresses 
are not in the msm_pingroup array.  That is, groups[0].ctrl_reg == 0, 
not 0xFF02010000.


-- 
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/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/04/2017 04:50 PM, Stephen Boyd wrote:

>> At the time I wrote that patch, the ACPI tables exposed all of the
>> GPIOs, even the ones it didn't care about.  The new ACPI tables list
>> only specific GPIOs, and so we no longer need to blindly read the
>> direction of all GPIOs.
>>
> 
> Do you avoid this problem on new ACPI tables because only pins
> that are able to be read are exposed?

Yes.  A recent firmware update enabled the "XPU" block which is being 
programmed with a select subset of individual GPIOs.  On our silicon, 
each TLMM GPIO is in a separate 64k page, and so the XPU can block any 
individual GPIO.  Any attempt to touch those registers causes an XPU 
violation which takes the whole system down.

>>> This is basically a revert of commit 72d320006177 ("gpio: set up
>>> initial state from .get_direction()").
>>
>> I would be in favor of either reverting that patch, or moving the
>> code into gpiochip_add_pin_range().
> 
> If it's in gpiochip_add_pin_range() would we still read the
> hardware when creating the pin ranges?

I presume so.  The idea is that pinctrl-qdf2xxx/pinctrl-msm only submit 
pin ranges that are present in the ACPI tables.

 > I don't want to have to> describe pin ranges of "valid" pins that 
won't cause the system
> to blow up if we touch them, because those pins are never used by
> Linux so reading them is not useful.

Well, that's exactly what I'm trying to do with current patch set :-) 
It seems the most logical approach to me.  I don't understand the 
dislike for it.  What else are pin ranges for, other than to specify 
ranges of pins that can be accessed?

Another alternative was to enumerate all of the GPIOs starting from 0. 
So the first GPIO in ACPI would be gpio0, regardless of what gpio number 
it actually was.  E.g. GPIO 37 would appear as gpio0, GPIO 38 would 
appear as gpio1, and so on.  That also worked, but it meant that 
customers would need to figure out which GPIO that "gpio0" actually 
pointed to.  That was not acceptable, so I dropped it.

I'm at a loss on how else to do it.  I think a gpio_chip.available 
callback is far less elegant than define pin ranges.  There is no chance 
that unavailable GPIOs can be accessed because the physical addresses 
are not in the msm_pingroup array.  That is, groups[0].ctrl_reg == 0, 
not 0xFF02010000.


-- 
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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 10/04, Timur Tabi wrote:
> On 10/04/2017 04:50 PM, Stephen Boyd wrote:
> 
> Yes.  A recent firmware update enabled the "XPU" block which is
> being programmed with a select subset of individual GPIOs.  On our
> silicon, each TLMM GPIO is in a separate 64k page, and so the XPU
> can block any individual GPIO.  Any attempt to touch those registers
> causes an XPU violation which takes the whole system down.

Yes it's the same sort of design with the hardware I have too.

> 
> >
> >If it's in gpiochip_add_pin_range() would we still read the
> >hardware when creating the pin ranges?
> 
> I presume so.  The idea is that pinctrl-qdf2xxx/pinctrl-msm only
> submit pin ranges that are present in the ACPI tables.

Ok.

> 
> > I don't want to have to describe pin ranges of "valid" pins that
> > won't cause the system
> > to blow up if we touch them, because those pins are never used by
> > Linux so reading them is not useful.
> 
> Well, that's exactly what I'm trying to do with current patch set
> :-) It seems the most logical approach to me.  I don't understand
> the dislike for it.  What else are pin ranges for, other than to
> specify ranges of pins that can be accessed?

I have no idea. To describe non-contiguous pin ranges? Linus?

> 
> Another alternative was to enumerate all of the GPIOs starting from
> 0. So the first GPIO in ACPI would be gpio0, regardless of what gpio
> number it actually was.  E.g. GPIO 37 would appear as gpio0, GPIO 38
> would appear as gpio1, and so on.  That also worked, but it meant
> that customers would need to figure out which GPIO that "gpio0"
> actually pointed to.  That was not acceptable, so I dropped it.

Agreed.

> 
> I'm at a loss on how else to do it.  I think a gpio_chip.available
> callback is far less elegant than define pin ranges.  There is no
> chance that unavailable GPIOs can be accessed because the physical
> addresses are not in the msm_pingroup array.  That is,
> groups[0].ctrl_reg == 0, not 0xFF02010000.
> 

Yes, thinking more about it I don't want an available callback
either. It will add burden on DT platforms where we have to
describe per-firmware pin ranges just because gpiolib is reading
the direction of gpios we don't use.

Instead, I'd prefer we delay reading the direction until a
consumer requests the gpio, this way we don't touch the hardware
unless a consumer wants to. That seems simpler and doesn't
require anything special from the driver.

Don't get me wrong, I'm willing to describe with DT/ACPI which
pins are available if we have a need for it, but so far I don't
see the requirement and I'm a lazy person so I like avoiding more
work.

Does my patch fail on your platform for some reason? I can only
guess that somewhere we don't request the gpio before using it
and then you don't see the proper direction.

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

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 10/04, Timur Tabi wrote:
> On 10/04/2017 04:50 PM, Stephen Boyd wrote:
> 
> Yes.  A recent firmware update enabled the "XPU" block which is
> being programmed with a select subset of individual GPIOs.  On our
> silicon, each TLMM GPIO is in a separate 64k page, and so the XPU
> can block any individual GPIO.  Any attempt to touch those registers
> causes an XPU violation which takes the whole system down.

Yes it's the same sort of design with the hardware I have too.

> 
> >
> >If it's in gpiochip_add_pin_range() would we still read the
> >hardware when creating the pin ranges?
> 
> I presume so.  The idea is that pinctrl-qdf2xxx/pinctrl-msm only
> submit pin ranges that are present in the ACPI tables.

Ok.

> 
> > I don't want to have to describe pin ranges of "valid" pins that
> > won't cause the system
> > to blow up if we touch them, because those pins are never used by
> > Linux so reading them is not useful.
> 
> Well, that's exactly what I'm trying to do with current patch set
> :-) It seems the most logical approach to me.  I don't understand
> the dislike for it.  What else are pin ranges for, other than to
> specify ranges of pins that can be accessed?

I have no idea. To describe non-contiguous pin ranges? Linus?

> 
> Another alternative was to enumerate all of the GPIOs starting from
> 0. So the first GPIO in ACPI would be gpio0, regardless of what gpio
> number it actually was.  E.g. GPIO 37 would appear as gpio0, GPIO 38
> would appear as gpio1, and so on.  That also worked, but it meant
> that customers would need to figure out which GPIO that "gpio0"
> actually pointed to.  That was not acceptable, so I dropped it.

Agreed.

> 
> I'm at a loss on how else to do it.  I think a gpio_chip.available
> callback is far less elegant than define pin ranges.  There is no
> chance that unavailable GPIOs can be accessed because the physical
> addresses are not in the msm_pingroup array.  That is,
> groups[0].ctrl_reg == 0, not 0xFF02010000.
> 

Yes, thinking more about it I don't want an available callback
either. It will add burden on DT platforms where we have to
describe per-firmware pin ranges just because gpiolib is reading
the direction of gpios we don't use.

Instead, I'd prefer we delay reading the direction until a
consumer requests the gpio, this way we don't touch the hardware
unless a consumer wants to. That seems simpler and doesn't
require anything special from the driver.

Don't get me wrong, I'm willing to describe with DT/ACPI which
pins are available if we have a need for it, but so far I don't
see the requirement and I'm a lazy person so I like avoiding more
work.

Does my patch fail on your platform for some reason? I can only
guess that somewhere we don't request the gpio before using it
and then you don't see the proper direction.

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

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Wed, Oct 4, 2017 at 12:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> Could we remove the pin direction finding part here in
> gpiochip_add_pin_range() and lazily resolve the pin direction
> when a pin is requested? We would need a similar check in the msm
> specific debugfs code where we skip pins that aren't requested.
> This is basically a revert of commit 72d320006177 ("gpio: set up
> initial state from .get_direction()").

It seems reasonable for the gpiolib to be able to call this
function immediately after registering the new GPIO chip
with its vtable.

I think it is more up to the driver to numb the reply with
some dummy return value (i.e. input mode) or refactor the
callback so that it is acceptable for gpiolib to get an -EINVAL
or so from the driver (again it will assume input mode) if the
driver can't return the direction at this time.

Yours,
Linus Walleij

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Wed, Oct 4, 2017 at 12:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> Could we remove the pin direction finding part here in
> gpiochip_add_pin_range() and lazily resolve the pin direction
> when a pin is requested? We would need a similar check in the msm
> specific debugfs code where we skip pins that aren't requested.
> This is basically a revert of commit 72d320006177 ("gpio: set up
> initial state from .get_direction()").

It seems reasonable for the gpiolib to be able to call this
function immediately after registering the new GPIO chip
with its vtable.

I think it is more up to the driver to numb the reply with
some dummy return value (i.e. input mode) or refactor the
callback so that it is acceptable for gpiolib to get an -EINVAL
or so from the driver (again it will assume input mode) if the
driver can't return the direction at this time.

Yours,
Linus Walleij

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 10/11, Linus Walleij wrote:
> On Wed, Oct 4, 2017 at 12:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> > Could we remove the pin direction finding part here in
> > gpiochip_add_pin_range() and lazily resolve the pin direction
> > when a pin is requested? We would need a similar check in the msm
> > specific debugfs code where we skip pins that aren't requested.
> > This is basically a revert of commit 72d320006177 ("gpio: set up
> > initial state from .get_direction()").
> 
> It seems reasonable for the gpiolib to be able to call this
> function immediately after registering the new GPIO chip
> with its vtable.

I agree. I don't see the benefit though. Reading the direction
later would achieve the same effect and also work for ACPI qcom
platforms.

> 
> I think it is more up to the driver to numb the reply with
> some dummy return value (i.e. input mode) or refactor the
> callback so that it is acceptable for gpiolib to get an -EINVAL
> or so from the driver (again it will assume input mode) if the
> driver can't return the direction at this time.
> 

For qcom platforms the driver will never be able to return the
direction for these certain pins because reading the register is
not allowed by the firmware. Doing so will cause the device to
crash with a security violation.

If you don't want to delay reading the direction until request
time, we should have the DT msm pinctrl drivers leave the
get_direction() pointer as NULL. We don't need to read the
direction on DT platforms to make anything work.

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

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Stephen Boyd]

On 10/11, Linus Walleij wrote:
> On Wed, Oct 4, 2017 at 12:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> > Could we remove the pin direction finding part here in
> > gpiochip_add_pin_range() and lazily resolve the pin direction
> > when a pin is requested? We would need a similar check in the msm
> > specific debugfs code where we skip pins that aren't requested.
> > This is basically a revert of commit 72d320006177 ("gpio: set up
> > initial state from .get_direction()").
> 
> It seems reasonable for the gpiolib to be able to call this
> function immediately after registering the new GPIO chip
> with its vtable.

I agree. I don't see the benefit though. Reading the direction
later would achieve the same effect and also work for ACPI qcom
platforms.

> 
> I think it is more up to the driver to numb the reply with
> some dummy return value (i.e. input mode) or refactor the
> callback so that it is acceptable for gpiolib to get an -EINVAL
> or so from the driver (again it will assume input mode) if the
> driver can't return the direction at this time.
> 

For qcom platforms the driver will never be able to return the
direction for these certain pins because reading the register is
not allowed by the firmware. Doing so will cause the device to
crash with a security violation.

If you don't want to delay reading the direction until request
time, we should have the DT msm pinctrl drivers leave the
get_direction() pointer as NULL. We don't need to read the
direction on DT platforms to make anything work.

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

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Thu, Oct 12, 2017 at 9:39 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> For qcom platforms the driver will never be able to return the
> direction for these certain pins because reading the register is
> not allowed by the firmware. Doing so will cause the device to
> crash with a security violation.

So I guess the driver needs to know what pin registers it can't
access so the user does not get a gun to shoot in the foot with.

If we augment gpiolib to just handle -EACCES or something
(-EIO?) from the driver .get_direction() callback for these lines,
things should be smooth?

Yours,
Linus Walleij

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Linus Walleij]

On Thu, Oct 12, 2017 at 9:39 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> For qcom platforms the driver will never be able to return the
> direction for these certain pins because reading the register is
> not allowed by the firmware. Doing so will cause the device to
> crash with a security violation.

So I guess the driver needs to know what pin registers it can't
access so the user does not get a gun to shoot in the foot with.

If we augment gpiolib to just handle -EACCES or something
(-EIO?) from the driver .get_direction() callback for these lines,
things should be smooth?

Yours,
Linus Walleij

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/14/2017 05:43 PM, Linus Walleij wrote:
> So I guess the driver needs to know what pin registers it can't
> access so the user does not get a gun to shoot in the foot with.
> 
> If we augment gpiolib to just handle -EACCES or something
> (-EIO?) from the driver .get_direction() callback for these lines,
> things should be smooth?

You mean like this:

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c 
b/drivers/pinctrl/qcom/pinctrl-msm.c
index ff491da64dab..ca4ae3d76eb4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -443,6 +443,14 @@ static int msm_gpio_get_direction(struct gpio_chip 
*chip, unsigned int offset)

  	g = &pctrl->soc->groups[offset];

+	/*
+	 * During initialization, gpiolib may query all GPIOs for their
+	 * initial direction, regardless if they exist, so block access
+	 * to those that are unavailable.
+	 */
+	if (!g->npins)
+		return -ENODEV;
+
  	val = readl(pctrl->regs + g->ctl_reg);

  	/* 0 = output, 1 = input */


This is what I have in my patch already.  I can return any error message 
you like, but -ENODEV already works.

The problem is that it's insufficient.  I also want the non-available 
GPIOs to be as absent as possible.  I don't want them to show up in 
/sys/kernel/debug/gpio, and I don't want to be able to create them via 
/sys/class/gpio/export.

-- 
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/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/14/2017 05:43 PM, Linus Walleij wrote:
> So I guess the driver needs to know what pin registers it can't
> access so the user does not get a gun to shoot in the foot with.
> 
> If we augment gpiolib to just handle -EACCES or something
> (-EIO?) from the driver .get_direction() callback for these lines,
> things should be smooth?

You mean like this:

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c 
b/drivers/pinctrl/qcom/pinctrl-msm.c
index ff491da64dab..ca4ae3d76eb4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -443,6 +443,14 @@ static int msm_gpio_get_direction(struct gpio_chip 
*chip, unsigned int offset)

  	g = &pctrl->soc->groups[offset];

+	/*
+	 * During initialization, gpiolib may query all GPIOs for their
+	 * initial direction, regardless if they exist, so block access
+	 * to those that are unavailable.
+	 */
+	if (!g->npins)
+		return -ENODEV;
+
  	val = readl(pctrl->regs + g->ctl_reg);

  	/* 0 = output, 1 = input */


This is what I have in my patch already.  I can return any error message 
you like, but -ENODEV already works.

The problem is that it's insufficient.  I also want the non-available 
GPIOs to be as absent as possible.  I don't want them to show up in 
/sys/kernel/debug/gpio, and I don't want to be able to create them via 
/sys/class/gpio/export.

-- 
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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/03/2017 05:03 PM, Stephen Boyd wrote:
> I don't see any value
> beyond system wide debug in figuring out the default pin
> configuration of a pin that doesn't have a consumer in Linux.

I can agree with that.

> Could we remove the pin direction finding part here in
> gpiochip_add_pin_range() and lazily resolve the pin direction
> when a pin is requested? We would need a similar check in the msm
> specific debugfs code where we skip pins that aren't requested.
> This is basically a revert of commit 72d320006177 ("gpio: set up
> initial state from .get_direction()").
> 
> ACPI can still describe only the pin ranges that they care about
> exposing, but from the devicetree side it's been working well
> enough to not touch pins that aren't used by anything in Linux.
> 
> ---8<----
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index cd003b74512f..673028823bc5 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
>   		 * 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 (!chip->direction_input) {
>   			/*
>   			 * If the chip lacks the .direction_input callback
>   			 * we logically assume all lines are outputs.
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 273badd92561..4a0aeceb42f1 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -24,7 +24,7 @@
>   #include <linux/pinctrl/pinconf.h>
>   #include <linux/pinctrl/pinconf-generic.h>
>   #include <linux/slab.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/driver.h>
>   #include <linux/interrupt.h>
>   #include <linux/spinlock.h>
>   #include <linux/reboot.h>
> @@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
>   	};
>   
>   	g = &pctrl->soc->groups[offset];
> +
> +	if (!gpiochip_is_requested(chip, gpio)) {
> +		seq_printf(s, " %-8s:", g->name);
> +		return;
> +	}
> +
>   	ctl_reg = readl(pctrl->regs + g->ctl_reg);
>   
>   	is_out = !!(ctl_reg & BIT(g->oe_bit));

In order for this to work, I had to add this function from patch #1:

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 -ENODEV;

	return gpiochip_generic_request(chip, offset);
}

The problem with this is that none of the GPIOs are "requested", so it 
displays an output like this:

# cat /sys/kernel/debug/gpio
gpiochip0: GPIOs 0-149, parent: platform/QCOM8002:00, QCOM8002:00:
  (null)  :
  (null)  :
  (null)  :
[... truncated ]]
  (null)  :
  (null)  :
  (null)  :
  (null)  :
  gpio36  :
  gpio37  :
  gpio38  :
  gpio39  :
  (null)  :
  (null)  :

It can't differentiate between GPIOs that don't exist and GPIOs that 
haven't been requested.  Plus, the "(null)" entries are what I've been 
trying to avoid in the first place.

So overall, this patch seems okay, although it needs a little work. 
However, it doesn't address Bjorn's complaint that he doesn't want me to 
use pin ranges at all.

-- 
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/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/03/2017 05:03 PM, Stephen Boyd wrote:
> I don't see any value
> beyond system wide debug in figuring out the default pin
> configuration of a pin that doesn't have a consumer in Linux.

I can agree with that.

> Could we remove the pin direction finding part here in
> gpiochip_add_pin_range() and lazily resolve the pin direction
> when a pin is requested? We would need a similar check in the msm
> specific debugfs code where we skip pins that aren't requested.
> This is basically a revert of commit 72d320006177 ("gpio: set up
> initial state from .get_direction()").
> 
> ACPI can still describe only the pin ranges that they care about
> exposing, but from the devicetree side it's been working well
> enough to not touch pins that aren't used by anything in Linux.
> 
> ---8<----
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index cd003b74512f..673028823bc5 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
>   		 * 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 (!chip->direction_input) {
>   			/*
>   			 * If the chip lacks the .direction_input callback
>   			 * we logically assume all lines are outputs.
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 273badd92561..4a0aeceb42f1 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -24,7 +24,7 @@
>   #include <linux/pinctrl/pinconf.h>
>   #include <linux/pinctrl/pinconf-generic.h>
>   #include <linux/slab.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/driver.h>
>   #include <linux/interrupt.h>
>   #include <linux/spinlock.h>
>   #include <linux/reboot.h>
> @@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
>   	};
>   
>   	g = &pctrl->soc->groups[offset];
> +
> +	if (!gpiochip_is_requested(chip, gpio)) {
> +		seq_printf(s, " %-8s:", g->name);
> +		return;
> +	}
> +
>   	ctl_reg = readl(pctrl->regs + g->ctl_reg);
>   
>   	is_out = !!(ctl_reg & BIT(g->oe_bit));

In order for this to work, I had to add this function from patch #1:

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 -ENODEV;

	return gpiochip_generic_request(chip, offset);
}

The problem with this is that none of the GPIOs are "requested", so it 
displays an output like this:

# cat /sys/kernel/debug/gpio
gpiochip0: GPIOs 0-149, parent: platform/QCOM8002:00, QCOM8002:00:
  (null)  :
  (null)  :
  (null)  :
[... truncated ]]
  (null)  :
  (null)  :
  (null)  :
  (null)  :
  gpio36  :
  gpio37  :
  gpio38  :
  gpio39  :
  (null)  :
  (null)  :

It can't differentiate between GPIOs that don't exist and GPIOs that 
haven't been requested.  Plus, the "(null)" entries are what I've been 
trying to avoid in the first place.

So overall, this patch seems okay, although it needs a little work. 
However, it doesn't address Bjorn's complaint that he doesn't want me to 
use pin ranges at all.

-- 
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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3849 bytes --]

On Fri, Oct 13, 2017 at 06:26:23PM -0500, Timur Tabi wrote:
> On 10/03/2017 05:03 PM, Stephen Boyd wrote:
> > I don't see any value
> > beyond system wide debug in figuring out the default pin
> > configuration of a pin that doesn't have a consumer in Linux.
> 
> I can agree with that.
> 
> > Could we remove the pin direction finding part here in
> > gpiochip_add_pin_range() and lazily resolve the pin direction
> > when a pin is requested? We would need a similar check in the msm
> > specific debugfs code where we skip pins that aren't requested.
> > This is basically a revert of commit 72d320006177 ("gpio: set up
> > initial state from .get_direction()").
> > 
> > ACPI can still describe only the pin ranges that they care about
> > exposing, but from the devicetree side it's been working well
> > enough to not touch pins that aren't used by anything in Linux.
> > 
> > ---8<----
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index cd003b74512f..673028823bc5 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
> >   		 * 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 (!chip->direction_input) {
> >   			/*
> >   			 * If the chip lacks the .direction_input callback
> >   			 * we logically assume all lines are outputs.
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 273badd92561..4a0aeceb42f1 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -24,7 +24,7 @@
> >   #include <linux/pinctrl/pinconf.h>
> >   #include <linux/pinctrl/pinconf-generic.h>
> >   #include <linux/slab.h>
> > -#include <linux/gpio.h>
> > +#include <linux/gpio/driver.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/spinlock.h>
> >   #include <linux/reboot.h>
> > @@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
> >   	};
> >   	g = &pctrl->soc->groups[offset];
> > +
> > +	if (!gpiochip_is_requested(chip, gpio)) {
> > +		seq_printf(s, " %-8s:", g->name);
> > +		return;
> > +	}
> > +
> >   	ctl_reg = readl(pctrl->regs + g->ctl_reg);
> >   	is_out = !!(ctl_reg & BIT(g->oe_bit));
> 
> In order for this to work, I had to add this function from patch #1:
> 
> 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 -ENODEV;
> 
> 	return gpiochip_generic_request(chip, offset);
> }
> 
> The problem with this is that none of the GPIOs are "requested", so it
> displays an output like this:
> 
> # cat /sys/kernel/debug/gpio
> gpiochip0: GPIOs 0-149, parent: platform/QCOM8002:00, QCOM8002:00:
>  (null)  :
>  (null)  :
>  (null)  :
> [... truncated ]]
>  (null)  :
>  (null)  :
>  (null)  :
>  (null)  :
>  gpio36  :
>  gpio37  :
>  gpio38  :
>  gpio39  :
>  (null)  :
>  (null)  :
> 
> It can't differentiate between GPIOs that don't exist and GPIOs that haven't
> been requested.

This confuses me. Why would you even want to register pins that don't
exist? It sounds to me like you're lying to gpiolib and then try to work
around it trying to access the GPIOs that don't exist but which you told
it were there.

Why not just tell gpiolib about only the GPIOs that exist?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Thierry Reding]

On Fri, Oct 13, 2017 at 06:26:23PM -0500, Timur Tabi wrote:
> On 10/03/2017 05:03 PM, Stephen Boyd wrote:
> > I don't see any value
> > beyond system wide debug in figuring out the default pin
> > configuration of a pin that doesn't have a consumer in Linux.
> 
> I can agree with that.
> 
> > Could we remove the pin direction finding part here in
> > gpiochip_add_pin_range() and lazily resolve the pin direction
> > when a pin is requested? We would need a similar check in the msm
> > specific debugfs code where we skip pins that aren't requested.
> > This is basically a revert of commit 72d320006177 ("gpio: set up
> > initial state from .get_direction()").
> > 
> > ACPI can still describe only the pin ranges that they care about
> > exposing, but from the devicetree side it's been working well
> > enough to not touch pins that aren't used by anything in Linux.
> > 
> > ---8<----
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index cd003b74512f..673028823bc5 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
> >   		 * 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 (!chip->direction_input) {
> >   			/*
> >   			 * If the chip lacks the .direction_input callback
> >   			 * we logically assume all lines are outputs.
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 273badd92561..4a0aeceb42f1 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -24,7 +24,7 @@
> >   #include <linux/pinctrl/pinconf.h>
> >   #include <linux/pinctrl/pinconf-generic.h>
> >   #include <linux/slab.h>
> > -#include <linux/gpio.h>
> > +#include <linux/gpio/driver.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/spinlock.h>
> >   #include <linux/reboot.h>
> > @@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
> >   	};
> >   	g = &pctrl->soc->groups[offset];
> > +
> > +	if (!gpiochip_is_requested(chip, gpio)) {
> > +		seq_printf(s, " %-8s:", g->name);
> > +		return;
> > +	}
> > +
> >   	ctl_reg = readl(pctrl->regs + g->ctl_reg);
> >   	is_out = !!(ctl_reg & BIT(g->oe_bit));
> 
> In order for this to work, I had to add this function from patch #1:
> 
> 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 -ENODEV;
> 
> 	return gpiochip_generic_request(chip, offset);
> }
> 
> The problem with this is that none of the GPIOs are "requested", so it
> displays an output like this:
> 
> # cat /sys/kernel/debug/gpio
> gpiochip0: GPIOs 0-149, parent: platform/QCOM8002:00, QCOM8002:00:
>  (null)  :
>  (null)  :
>  (null)  :
> [... truncated ]]
>  (null)  :
>  (null)  :
>  (null)  :
>  (null)  :
>  gpio36  :
>  gpio37  :
>  gpio38  :
>  gpio39  :
>  (null)  :
>  (null)  :
> 
> It can't differentiate between GPIOs that don't exist and GPIOs that haven't
> been requested.

This confuses me. Why would you even want to register pins that don't
exist? It sounds to me like you're lying to gpiolib and then try to work
around it trying to access the GPIOs that don't exist but which you told
it were there.

Why not just tell gpiolib about only the GPIOs that exist?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171015/b8052700/attachment.sig>

Re: [PATCH 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/15/17 3:18 PM, Thierry Reding wrote:
> This confuses me. Why would you even want to register pins that don't
> exist? It sounds to me like you're lying to gpiolib and then try to work
> around it trying to access the GPIOs that don't exist but which you told
> it were there.
> 
> Why not just tell gpiolib about only the GPIOs that exist?

Please look at my patches.  That's exactly what they do, but no one else 
likes that approach.

-- 
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/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 10/15/17 3:18 PM, Thierry Reding wrote:
> This confuses me. Why would you even want to register pins that don't
> exist? It sounds to me like you're lying to gpiolib and then try to work
> around it trying to access the GPIOs that don't exist but which you told
> it were there.
> 
> Why not just tell gpiolib about only the GPIOs that exist?

Please look at my patches.  That's exactly what they do, but no one else 
likes that approach.

-- 
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 0/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 09/22/2017 08:29 AM, Linus Walleij wrote:
> I am waiting for the maintainer, Bjorn Andersson, to provide review.

Bjorn,

Would you please take a moment to review these patches?  I'm guessing 
it's too late for 4.14, so I would like these merged into 4.15.  But 
without your review, very little progress is being made.

-- 
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/2] [v5] pinctrl: qcom: add support for sparse GPIOs

[Timur Tabi]

On 09/22/2017 08:29 AM, Linus Walleij wrote:
> I am waiting for the maintainer, Bjorn Andersson, to provide review.

Bjorn,

Would you please take a moment to review these patches?  I'm guessing 
it's too late for 4.14, so I would like these merged into 4.15.  But 
without your review, very little progress is being made.

-- 
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.