Re: [RFC] Bluetooth: hci_bcm: Do not tie GPIO pin order to a specific ACPI HID

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 15-03-18 19:36, Marcel Holtmann wrote:
Hi Hans,

Since I've been doing a lot of work on Linux Bay Trail / Cherry Trail
support, I've gathered a collection of ACPI DSDTs from about 50 such
machines.

Looking at these DSTDs many have an ACPI device entry describing a bcm
bluetooth device (often disabled in the DSDT), quite a few of these ACPI
device entries have a resource-table where the order does not match with
the order currently associated with the HID of that entry in the
bcm_acpi_match table.

Looking at the Windows .inf files, there is nothing indicating a specific
order there, so I believe that there is no 1:1 mapping between the ACPI
HID and the order in which the resources are listed.

Therefor this commit replaces the hardcoded mapping based on ACPI HID,
with code which actually checks in which order the resources are listed
and bases the gpio-mapping on that.

This should ensure that we always pick the right mapping and this will
make adding new ACPI HIDs to the driver easier.

<snip>

@@ -983,6 +979,30 @@ static int bcm_acpi_probe(struct bcm_device *dev)
	}
	acpi_dev_free_resource_list(&resources);

+	/*
+	 * If the DSDT uses an Interrupt resource for the IRQ, then there are
+	 * only 2 GPIO resources, we use the irq-last mapping for this, since
+	 * we already have an irq the 3th / last mapping will not be used.
+	 */

minor nitpick that we use the net multi-line comment style. At least for new comments.

Ok, will fix for the non RFC version. As for the reason why this was RFC,
the problem turned out to be another IRQ polarity issue. I've a more generic
fix for that too now. I will submit 2 series with my work soonish (still need
to run a bunch of tests first).

+	if (dev->irq)
+		gpio_mapping = acpi_bcm_int_last_gpios;
+	else if (dev->gpio_int_idx == 0)
+		gpio_mapping = acpi_bcm_int_first_gpios;
+	else if (dev->gpio_int_idx == 2)
+		gpio_mapping = acpi_bcm_int_last_gpios;
+	else
+		dev_warn(dev->dev, "Unexpected ACPI gpio_int_idx: %d\n",
+			 dev->gpio_int_idx);
+
+	/* Warn if our expectations are not met. */
+	if (dev->gpio_count != (dev->irq ? 2 : 3))
+		dev_warn(dev->dev, "Unexpected number of ACPI GPIOs: %d\n",
+			 dev->gpio_count);
+
+	ret = devm_acpi_dev_add_driver_gpios(dev->dev, gpio_mapping);
+	if (ret)
+		return ret;
+
	dmi_id = dmi_first_match(bcm_active_low_irq_dmi_table);
	if (dmi_id) {
		dev_warn(dev->dev, "%s: Overwriting IRQ polarity to active low",
@@ -1073,26 +1093,26 @@ static const struct hci_uart_proto bcm_proto = {

#ifdef CONFIG_ACPI
static const struct acpi_device_id bcm_acpi_match[] = {
-	{ "BCM2E1A", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E3A", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E3D", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E3F", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E40", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E54", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E55", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E64", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E65", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E67", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E71", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E72", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E7B", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E7C", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E7E", (kernel_ulong_t)&acpi_bcm_int_first_gpios },
-	{ "BCM2E84", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
-	{ "BCM2E95", (kernel_ulong_t)&acpi_bcm_int_first_gpios },
-	{ "BCM2E96", (kernel_ulong_t)&acpi_bcm_int_first_gpios },
-	{ "BCM2EA4", (kernel_ulong_t)&acpi_bcm_int_first_gpios },
+	{ "BCM2E1A" },
+	{ "BCM2E39" },
+	{ "BCM2E3A" },
+	{ "BCM2E3D" },
+	{ "BCM2E3F" },
+	{ "BCM2E40" },
+	{ "BCM2E54" },
+	{ "BCM2E55" },
+	{ "BCM2E64" },
+	{ "BCM2E65" },
+	{ "BCM2E67" },
+	{ "BCM2E71" },
+	{ "BCM2E72" },
+	{ "BCM2E7B" },
+	{ "BCM2E7C" },
+	{ "BCM2E7E" },
+	{ "BCM2E84" },
+	{ "BCM2E95" },
+	{ "BCM2E96" },
+	{ "BCM2EA4" },

Otherwise I like this a lot since it makes thing easier and potentially adding new IDs via new_id should become simple (if that is actually supported for ACPI).

Yes being able to use new_id (or bind) is the idea. Note if this is
supported is not APCI specific, the ACPI code only generates the
modalias, the rest is up to the serdev bus code, which I think
should just work.

Note supporting new_id is only one reason, looking at the DSTDs I
have we're going to run out of luck at one point and have 2 machines
which both use the same ACPI HID but need a different order. I've
made a summary of various settings found in all the DSTDs I've with
BCM2E## ids in them here:

https://fedorapeople.org/~jwrdegoede/bcm-bt-stuff

As you can see there are several cases where the order in the old
bcm_acpi_match does not match with what is in the DSTD. Note not
necessarily all devices with a BCM2E## device in their DSTD actually
have one, because there is a lot of copy and paste going on in
DSTD tables.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux