pinctrl: qcom: add support for sparse GPIOs

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

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

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

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

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

v10:
Use driver_stuct to obtain ACPI match table entry

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

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

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

This reverts commit 72d3200061776264941be1b5a9bb8e926b3b30a5.

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

---
 drivers/gpio/gpiolib.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

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

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

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

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

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

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

index 7a960590ecaa..d45b4c2b5af1 100644
@@ -507,6 +507,11 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 	};
 
 	g = &pctrl->soc->groups[offset];
+
+	/* If the GPIO group has no pins, then don't show it. */
+	if (!g->npins)
+		return;
+
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
 	is_out = !!(ctl_reg & BIT(g->oe_bit));

@@ -516,7 +521,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 
 	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
 	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
-	seq_printf(s, " %s", pulls[pull]);
+	seq_printf(s, " %s\n", pulls[pull]);
 }
 
 static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)

@@ -524,23 +529,36 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned gpio = chip->base;
 	unsigned i;
 
-	for (i = 0; i < chip->ngpio; i++, gpio++) {
+	for (i = 0; i < chip->ngpio; i++, gpio++)
 		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
-		seq_puts(s, "\n");
-	}
 }
 
 #else
 #define msm_gpio_dbg_show NULL
 #endif
 
+/*
+ * If the requested GPIO has no pins, then treat it as unavailable.
+ * Otherwise, call the standard request function.
+ */
+static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct msm_pingroup *g = &pctrl->soc->groups[offset];
+
+	if (!g->npins)
+		return -EACCES;
+
+	return gpiochip_generic_request(chip, offset);
+}
+
 static const struct gpio_chip msm_gpio_template = {
 	.direction_input  = msm_gpio_direction_input,
 	.direction_output = msm_gpio_direction_output,
 	.get_direction    = msm_gpio_get_direction,
 	.get              = msm_gpio_get,
 	.set              = msm_gpio_set,
-	.request          = gpiochip_generic_request,
+	.request          = msm_gpio_request,
 	.free             = gpiochip_generic_free,
 	.dbg_show         = msm_gpio_dbg_show,
 };

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

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.

---
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 134 +++++++++++++++++++++++++--------
 1 file changed, 103 insertions(+), 31 deletions(-)

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

@@ -110,7 +181,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id qdf2xxx_acpi_ids[] = {
-	{"QCOM8001"},
+	{"QCOM8001", QDF2XXX_V1},
+	{"QCOM8002", QDF2XXX_V2},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);

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

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

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

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

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

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.

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

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

---8<---
From: Timur Tabi <[email protected]>

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.

---
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 89 +++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 12 deletions(-)

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

@@ -100,6 +162,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 		groups[i].intr_detection_width = 2;
 	}
 
+	devm_kfree(&pdev->dev, gpios);
+
 	qdf2xxx_pinctrl.pins = pins;
 	qdf2xxx_pinctrl.groups = groups;
 	qdf2xxx_pinctrl.npins = num_gpios;

@@ -110,7 +174,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id qdf2xxx_acpi_ids[] = {
-	{"QCOM8001"},
+	{"QCOM8001", QDF2XXX_V1},
+	{"QCOM8002", QDF2XXX_V2},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);

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

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

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

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

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

Alright!

Of course.

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

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

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

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

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

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

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

...

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

index 8db2680bf872..5f118f044caa 100644
@@ -1475,8 +1475,8 @@ static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip)
 	gpiochip->irq_valid_mask = NULL;
 }
 
-static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
-				       unsigned int offset)
+bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
+				unsigned int offset)
 {
 	/* No mask means all valid */
 	if (likely(!gpiochip->irq_valid_mask))

index af20369ec8e7..be977c1c7498 100644
@@ -262,6 +262,9 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 			     bool nested,
 			     struct lock_class_key *lock_key);
 
+bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
+				unsigned int offset);
+
 #ifdef CONFIG_LOCKDEP
 
 /*

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

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

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

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

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

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

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

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

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

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

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

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

Ok.

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

Ok.

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

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

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

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

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

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

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

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

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

I'm not sure I agree with that.

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

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

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

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

Andy Shevchenko <[email protected]>
Intel Finland Oy

Thanks Andy.

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

Yours,
Linus Walleij