[PATCH 0/4] dln2: add support for ACPI

[PATCH 0/4] dln2: add support for ACPI

[Octavian Purdila]

This patch sets adds support for ACPI enumeration for devices
connected to the DLN2 bridge via a custom ACPI table.

The first two patches fix a couple of issues with the ACPI load/unload
table APIs.

The 3rd patch adds ACPI support to the MFD driver and the last patch
configures pull-up/pull-down on GPIO pins based on the APCI
configuration.

Octavian Purdila (4):
  ACPICA: take ACPI_MTX_INTERPRETER in acpi_unload_table_id
  ACPICA: don't release ACPI_MTX_TABLES in
    acpi_tb_install_standard_table
  mfd: dln2: add support for ACPI
  gpio: dln2: add support for ACPI pin configuration

 Documentation/acpi/dln2-acpi.txt |  62 ++++++++++++++++++
 drivers/acpi/acpica/tbinstal.c   |   1 -
 drivers/acpi/acpica/tbxface.c    |   7 ++
 drivers/gpio/gpio-dln2.c         |  76 ++++++++++++++++++++++
 drivers/mfd/dln2.c               | 134 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 279 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/acpi/dln2-acpi.txt

-- 
1.9.1

[PATCH 1/4] ACPICA: take ACPI_MTX_INTERPRETER in acpi_unload_table_id

[Octavian Purdila]

acpi_tb_delete_namespace_by_owner expects ACPI_MTX_INTERPRETER to be
taken. This fixes the following issue:

ACPI Error: Mutex [0x0] is not acquired, cannot release (20141107/utmutex-322)
Call Trace:
  [<ffffffff81b0bd28>] dump_stack+0x4f/0x7b
  [<ffffffff81546bfc>] acpi_ut_release_mutex+0x47/0x67
  [<ffffffff81542cf1>] acpi_tb_delete_namespace_by_owner+0x57/0x8d
  [<ffffffff81543ef1>] acpi_unload_table_id+0x3a/0x5e

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/acpi/acpica/tbxface.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
index 6482b0d..9520ae1 100644
--- a/drivers/acpi/acpica/tbxface.c
+++ b/drivers/acpi/acpica/tbxface.c
@@ -281,6 +281,11 @@ acpi_status acpi_unload_table_id(acpi_owner_id id)
 
 	ACPI_FUNCTION_TRACE(acpi_unload_table_id);
 
+	status = acpi_ut_acquire_mutex(ACPI_MTX_INTERPRETER);
+	if (ACPI_FAILURE(status)) {
+		return_ACPI_STATUS(status);
+	}
+
 	/* Find table in the global table list */
 	for (i = 0; i < acpi_gbl_root_table_list.current_table_count; ++i) {
 		if (id != acpi_gbl_root_table_list.tables[i].owner_id) {
@@ -297,6 +302,8 @@ acpi_status acpi_unload_table_id(acpi_owner_id id)
 		acpi_tb_set_table_loaded_flag(i, FALSE);
 		break;
 	}
+
+	(void)acpi_ut_release_mutex(ACPI_MTX_INTERPRETER);
 	return_ACPI_STATUS(status);
 }
 
-- 
1.9.1

Re: [PATCH 1/4] ACPICA: take ACPI_MTX_INTERPRETER in acpi_unload_table_id

[Sergei Shtylyov]

Hello.

On 12/16/2014 07:12 PM, Octavian Purdila wrote:

> acpi_tb_delete_namespace_by_owner expects ACPI_MTX_INTERPRETER to be
> taken. This fixes the following issue:

> ACPI Error: Mutex [0x0] is not acquired, cannot release (20141107/utmutex-322)
> Call Trace:
>    [<ffffffff81b0bd28>] dump_stack+0x4f/0x7b
>    [<ffffffff81546bfc>] acpi_ut_release_mutex+0x47/0x67
>    [<ffffffff81542cf1>] acpi_tb_delete_namespace_by_owner+0x57/0x8d
>    [<ffffffff81543ef1>] acpi_unload_table_id+0x3a/0x5e

> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>   drivers/acpi/acpica/tbxface.c | 7 +++++++
>   1 file changed, 7 insertions(+)

> diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
> index 6482b0d..9520ae1 100644
> --- a/drivers/acpi/acpica/tbxface.c
> +++ b/drivers/acpi/acpica/tbxface.c
> @@ -281,6 +281,11 @@ acpi_status acpi_unload_table_id(acpi_owner_id id)
>
>   	ACPI_FUNCTION_TRACE(acpi_unload_table_id);
>
> +	status = acpi_ut_acquire_mutex(ACPI_MTX_INTERPRETER);
> +	if (ACPI_FAILURE(status)) {
> +		return_ACPI_STATUS(status);
> +	}

    {} not needed here. Please run your patches thru scripts/checkpatch.pl, it 
should complain in this case.

> +
>   	/* Find table in the global table list */
>   	for (i = 0; i < acpi_gbl_root_table_list.current_table_count; ++i) {
>   		if (id != acpi_gbl_root_table_list.tables[i].owner_id) {
> @@ -297,6 +302,8 @@ acpi_status acpi_unload_table_id(acpi_owner_id id)
>   		acpi_tb_set_table_loaded_flag(i, FALSE);
>   		break;
>   	}
> +
> +	(void)acpi_ut_release_mutex(ACPI_MTX_INTERPRETER);

    Cast to *void*  not necessary either.

[...]

WBR, Sergei

Re: [PATCH 1/4] ACPICA: take ACPI_MTX_INTERPRETER in acpi_unload_table_id

[Octavian Purdila]

On Tue, Dec 16, 2014 at 8:14 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
> On 12/16/2014 07:12 PM, Octavian Purdila wrote:
>
>> acpi_tb_delete_namespace_by_owner expects ACPI_MTX_INTERPRETER to be
>> taken. This fixes the following issue:
>
>
>> ACPI Error: Mutex [0x0] is not acquired, cannot release
>> (20141107/utmutex-322)
>> Call Trace:
>>    [<ffffffff81b0bd28>] dump_stack+0x4f/0x7b
>>    [<ffffffff81546bfc>] acpi_ut_release_mutex+0x47/0x67
>>    [<ffffffff81542cf1>] acpi_tb_delete_namespace_by_owner+0x57/0x8d
>>    [<ffffffff81543ef1>] acpi_unload_table_id+0x3a/0x5e
>
>
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>> ---
>>   drivers/acpi/acpica/tbxface.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>
>
>> diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
>> index 6482b0d..9520ae1 100644
>> --- a/drivers/acpi/acpica/tbxface.c
>> +++ b/drivers/acpi/acpica/tbxface.c
>> @@ -281,6 +281,11 @@ acpi_status acpi_unload_table_id(acpi_owner_id id)
>>
>>         ACPI_FUNCTION_TRACE(acpi_unload_table_id);
>>
>> +       status = acpi_ut_acquire_mutex(ACPI_MTX_INTERPRETER);
>> +       if (ACPI_FAILURE(status)) {
>> +               return_ACPI_STATUS(status);
>> +       }
>
>
>    {} not needed here. Please run your patches thru scripts/checkpatch.pl,
> it should complain in this case.
>

I always run checkpatch. It does not complain.

>> +
>>         /* Find table in the global table list */
>>         for (i = 0; i < acpi_gbl_root_table_list.current_table_count; ++i)
>> {
>>                 if (id != acpi_gbl_root_table_list.tables[i].owner_id) {
>> @@ -297,6 +302,8 @@ acpi_status acpi_unload_table_id(acpi_owner_id id)
>>                 acpi_tb_set_table_loaded_flag(i, FALSE);
>>                 break;
>>         }
>> +
>> +       (void)acpi_ut_release_mutex(ACPI_MTX_INTERPRETER);
>
>
>    Cast to *void*  not necessary either.
>

AFAICS ACPICA has a slight different coding style than the rest of the
kernel and I kept using it in these patches. Rafael, please let me
know if I am wrong.

Re: [PATCH 1/4] ACPICA: take ACPI_MTX_INTERPRETER in acpi_unload_table_id

[Rafael J. Wysocki]

On Tuesday, December 16, 2014 06:12:30 PM Octavian Purdila wrote:
> acpi_tb_delete_namespace_by_owner expects ACPI_MTX_INTERPRETER to be
> taken. This fixes the following issue:
> 
> ACPI Error: Mutex [0x0] is not acquired, cannot release (20141107/utmutex-322)
> Call Trace:
>   [<ffffffff81b0bd28>] dump_stack+0x4f/0x7b
>   [<ffffffff81546bfc>] acpi_ut_release_mutex+0x47/0x67
>   [<ffffffff81542cf1>] acpi_tb_delete_namespace_by_owner+0x57/0x8d
>   [<ffffffff81543ef1>] acpi_unload_table_id+0x3a/0x5e
> 
> Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

I've queued up this one for 3.20.

> ---
>  drivers/acpi/acpica/tbxface.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
> index 6482b0d..9520ae1 100644
> --- a/drivers/acpi/acpica/tbxface.c
> +++ b/drivers/acpi/acpica/tbxface.c
> @@ -281,6 +281,11 @@ acpi_status acpi_unload_table_id(acpi_owner_id id)
>  
>  	ACPI_FUNCTION_TRACE(acpi_unload_table_id);
>  
> +	status = acpi_ut_acquire_mutex(ACPI_MTX_INTERPRETER);
> +	if (ACPI_FAILURE(status)) {
> +		return_ACPI_STATUS(status);
> +	}
> +
>  	/* Find table in the global table list */
>  	for (i = 0; i < acpi_gbl_root_table_list.current_table_count; ++i) {
>  		if (id != acpi_gbl_root_table_list.tables[i].owner_id) {
> @@ -297,6 +302,8 @@ acpi_status acpi_unload_table_id(acpi_owner_id id)
>  		acpi_tb_set_table_loaded_flag(i, FALSE);
>  		break;
>  	}
> +
> +	(void)acpi_ut_release_mutex(ACPI_MTX_INTERPRETER);
>  	return_ACPI_STATUS(status);
>  }
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] ACPICA: don't release ACPI_MTX_TABLES in acpi_tb_install_standard_table

[Octavian Purdila]

ACPI_MTX_TABLES is taken and released by the callers of
acpi_tb_install_standard_table so releasing it in the function itself
is causing the following error if the table is reloaded:

ACPI Error: Mutex [0x2] is not acquired, cannot release (20141107/utmutex-321)
Call Trace:
  [<ffffffff81b0bd48>] dump_stack+0x4f/0x7b
  [<ffffffff81546bf5>] acpi_ut_release_mutex+0x47/0x67
  [<ffffffff81544357>] acpi_load_table+0x73/0xcb

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/acpi/acpica/tbinstal.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/acpi/acpica/tbinstal.c b/drivers/acpi/acpica/tbinstal.c
index 755b90c..c0b39f3 100644
--- a/drivers/acpi/acpica/tbinstal.c
+++ b/drivers/acpi/acpica/tbinstal.c
@@ -346,7 +346,6 @@ acpi_tb_install_standard_table(acpi_physical_address address,
 				 */
 				acpi_tb_uninstall_table(&new_table_desc);
 				*table_index = i;
-				(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
 				return_ACPI_STATUS(AE_OK);
 			}
 		}
-- 
1.9.1

[PATCH 3/4] mfd: dln2: add support for ACPI

[Octavian Purdila]

This patch adds support to load a custom ACPI table that describes
devices connected via the DLN2 USB to I2C/SPI/GPIO bridge.

The ACPI table can be loaded either externally (from QEMU or with
CONFIG_ACPI_CUSTOM_DSDT) or it can be loaded as firmware file with the
name dln2.aml. The driver looks for an ACPI device entry with _HID set
to "DLN20000" and makes it the ACPI companion for DLN2 USB
sub-drivers.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/acpi/dln2-acpi.txt |  62 ++++++++++++++++++
 drivers/mfd/dln2.c               | 134 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 196 insertions(+)
 create mode 100644 Documentation/acpi/dln2-acpi.txt

diff --git a/Documentation/acpi/dln2-acpi.txt b/Documentation/acpi/dln2-acpi.txt
new file mode 100644
index 0000000..d76605f
--- /dev/null
+++ b/Documentation/acpi/dln2-acpi.txt
@@ -0,0 +1,62 @@
+Diolan DLN2 custom APCI table
+
+The Diolan DLN2 is an USB to I2C/SPI/GPIO bridge and as such it can be used to
+connect to various I2C or SPI devices. Because these busses lack an enumeration
+protocol, the driver obtains various information about the device (such as I2C
+address and GPIO pins) from either ACPI or device tree.
+
+To allow enumerating devices and their properties via ACPI, the Diolan
+driver looks for an ACPI tree with the root _HID set to "DLN20000". If
+it finds such an ACPI object it will set the ACPI companion to the
+DLN2 MFD driver and from their it will be propagated to all its
+sub-devices (I2C, GPIO, SPI).
+
+The user can either load the custom DSDT table with three methods:
+
+1. Via QEMU (see -acpitable)
+
+2. Via the CONFIG_ACPI_CUSTOM_DSDT kernel config option (see
+Documentation/acpi/dsdt-override.txt)
+
+3. By placing the custom DSDT in the firmware paths in a file name
+dln2.aml.
+
+Here is an example ACPI table that enumerates a BMC150 accelerometer
+and defines its I2C address and GPIO pin used as an interrupt source:
+
+DefinitionBlock ("ssdt.aml", "SSDT", 1, "INTEL ", "CpuDptf", 0x00000003)
+{
+	Device (DLN0)
+	{
+		Name (_ADR, Zero)
+		Name (_HID, "DLN2000")
+
+		Device (STAC)
+		{
+			Name (_ADR, Zero)
+			Name (_HID, "BMC150A")
+			Name (_CID, "INTACCL")
+			Name (_UID, One)
+
+			Method (_CRS, 0, Serialized)
+			{
+				Name (RBUF, ResourceTemplate ()
+				{
+					I2cSerialBus (0x0010, ControllerInitiated, 0x00061A80,
+						      AddressingMode7Bit, "\\DLN0",
+						      0x00, ResourceConsumer, ,)
+
+					GpioInt (Level, ActiveHigh, Exclusive, PullDown, 0x0000,
+						 "\\DLN0", 0x00, ResourceConsumer, , )
+					{ // Pin list
+						0
+					}
+				})
+				Return (RBUF)
+		       }
+		}
+	}
+}
+
+The resources defined in the devices under the DLN0 are those
+supported by the I2C, GPIO and SPI sub-systems.
diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index f9c4a0b..93f6d1d 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -23,6 +23,8 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/dln2.h>
 #include <linux/rculist.h>
+#include <linux/acpi.h>
+#include <linux/firmware.h>
 
 struct dln2_header {
 	__le16 size;
@@ -714,6 +716,134 @@ static void dln2_stop(struct dln2_dev *dln2)
 
 	dln2_stop_rx_urbs(dln2);
 }
+
+#if IS_ENABLED(CONFIG_ACPI)
+
+static struct dln2_acpi_info {
+	const struct firmware *fw;
+	acpi_owner_id table_id;
+	struct acpi_device *dev;
+	int users;
+} dln2_acpi_info;
+
+static DEFINE_MUTEX(dln2_acpi_lock);
+
+static acpi_status dln2_find_acpi_handle(acpi_handle handle, u32 level,
+					 void *ctxt, void **retv)
+{
+	acpi_handle *dln2_handle = (acpi_handle *)retv;
+
+	*dln2_handle = handle;
+
+	return AE_CTRL_TERMINATE;
+}
+
+static void dln2_probe_acpi(struct dln2_dev *dln2)
+{
+	struct device *dev = &dln2->interface->dev;
+	struct dln2_acpi_info *ai = &dln2_acpi_info;
+	acpi_handle h = NULL;
+	int ret;
+	bool fw_loaded = false;
+
+	mutex_lock(&dln2_acpi_lock);
+
+	if (ai->dev)
+		goto out_success;
+
+	/*
+	 * Look for the DLN2000 HID in case the ACPI table was loaded
+	 * externally (e.g. from qemu).
+	 */
+	acpi_get_devices("DLN20000", dln2_find_acpi_handle, NULL, &h);
+	if (!h) {
+		/* Try to load the ACPI table via a firmware file */
+		ret = request_firmware(&ai->fw, "dln2.aml", NULL);
+		if (ret)
+			goto out_unlock;
+
+		ret = acpi_load_table((void *)ai->fw->data);
+		if (ret) {
+			dev_err(dev, "invalid ACPI table\n");
+			goto out_release_fw;
+		}
+
+		acpi_get_devices("DLN20000", dln2_find_acpi_handle, NULL, &h);
+		if (!h) {
+			dev_err(dev, "not a DLN2 ACPI table\n");
+			goto out_leak_fw;
+		}
+
+		ret = acpi_get_id(h, &ai->table_id);
+		if (ret) {
+			dev_err(dev, "acpi_get_id failed: %d\n", ret);
+			goto out_leak_fw;
+		}
+
+		ret = acpi_bus_scan(h);
+		if (ret) {
+			dev_err(dev, "acpi_bus_scan failed: %d\n", ret);
+			goto out_leak_fw;
+		}
+
+		fw_loaded = true;
+	}
+
+	ret = acpi_bus_get_device(h, &ai->dev);
+	if (ret) {
+		dev_err(dev, "failed to get ACPI device: %d\n", ret);
+		if (fw_loaded) {
+			acpi_unload_table_id(ai->table_id);
+			goto out_leak_fw;
+		}
+		goto out_unlock;
+	}
+
+out_success:
+	ACPI_COMPANION_SET(dev, ai->dev);
+	ai->users++;
+	mutex_unlock(&dln2_acpi_lock);
+	return;
+
+out_release_fw:
+	release_firmware(ai->fw);
+out_leak_fw:
+	/*
+	 * Once a table is loaded we can't release the firmware anymore because
+	 * acpi_unload_table does not actually unload the table but keeps it in
+	 * memory to speed up subsequent loads.
+	 */
+	ai->fw = NULL;
+out_unlock:
+	mutex_unlock(&dln2_acpi_lock);
+}
+
+static void dln2_disconnect_acpi(struct dln2_dev *dln2)
+{
+	struct dln2_acpi_info *ai = &dln2_acpi_info;
+
+	mutex_lock(&dln2_acpi_lock);
+	if (--ai->users == 0 && ai->fw) {
+		acpi_scan_lock_acquire();
+		acpi_bus_trim(ai->dev);
+		acpi_scan_lock_release();
+		acpi_unload_table_id(ai->table_id);
+		ai->dev = NULL;
+		/* we can't release firmware see comment in dln2_probe_acpi */
+		ai->fw = NULL;
+	}
+	mutex_unlock(&dln2_acpi_lock);
+}
+#else
+static void dln2_probe_acpi(struct dln2_dev *dln2)
+{
+}
+
+static void dln2_disconnect_acpi(struct dln2_dev *dln2)
+{
+}
+#endif
+
 static void dln2_disconnect(struct usb_interface *interface)
 {
 	struct dln2_dev *dln2 = usb_get_intfdata(interface);
@@ -722,6 +852,8 @@ static void dln2_disconnect(struct usb_interface *interface)
 
 	mfd_remove_devices(&interface->dev);
 
+	dln2_disconnect_acpi(dln2);
+
 	dln2_free(dln2);
 }
 
@@ -774,6 +906,8 @@ static int dln2_probe(struct usb_interface *interface,
 		goto out_stop_rx;
 	}
 
+	dln2_probe_acpi(dln2);
+
 	ret = mfd_add_hotplug_devices(dev, dln2_devs, ARRAY_SIZE(dln2_devs));
 	if (ret != 0) {
 		dev_err(dev, "failed to add mfd devices to core\n");
-- 
1.9.1

Re: [PATCH 3/4] mfd: dln2: add support for ACPI

[Lee Jones]

I need an ACPI Ack on this.

> This patch adds support to load a custom ACPI table that describes
> devices connected via the DLN2 USB to I2C/SPI/GPIO bridge.
> 
> The ACPI table can be loaded either externally (from QEMU or with
> CONFIG_ACPI_CUSTOM_DSDT) or it can be loaded as firmware file with the
> name dln2.aml. The driver looks for an ACPI device entry with _HID set
> to "DLN20000" and makes it the ACPI companion for DLN2 USB
> sub-drivers.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  Documentation/acpi/dln2-acpi.txt |  62 ++++++++++++++++++
>  drivers/mfd/dln2.c               | 134 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 196 insertions(+)
>  create mode 100644 Documentation/acpi/dln2-acpi.txt
> 
> diff --git a/Documentation/acpi/dln2-acpi.txt b/Documentation/acpi/dln2-acpi.txt
> new file mode 100644
> index 0000000..d76605f
> --- /dev/null
> +++ b/Documentation/acpi/dln2-acpi.txt
> @@ -0,0 +1,62 @@
> +Diolan DLN2 custom APCI table
> +
> +The Diolan DLN2 is an USB to I2C/SPI/GPIO bridge and as such it can be used to
> +connect to various I2C or SPI devices. Because these busses lack an enumeration
> +protocol, the driver obtains various information about the device (such as I2C
> +address and GPIO pins) from either ACPI or device tree.
> +
> +To allow enumerating devices and their properties via ACPI, the Diolan
> +driver looks for an ACPI tree with the root _HID set to "DLN20000". If
> +it finds such an ACPI object it will set the ACPI companion to the
> +DLN2 MFD driver and from their it will be propagated to all its
> +sub-devices (I2C, GPIO, SPI).
> +
> +The user can either load the custom DSDT table with three methods:
> +
> +1. Via QEMU (see -acpitable)
> +
> +2. Via the CONFIG_ACPI_CUSTOM_DSDT kernel config option (see
> +Documentation/acpi/dsdt-override.txt)
> +
> +3. By placing the custom DSDT in the firmware paths in a file name
> +dln2.aml.
> +
> +Here is an example ACPI table that enumerates a BMC150 accelerometer
> +and defines its I2C address and GPIO pin used as an interrupt source:
> +
> +DefinitionBlock ("ssdt.aml", "SSDT", 1, "INTEL ", "CpuDptf", 0x00000003)
> +{
> +	Device (DLN0)
> +	{
> +		Name (_ADR, Zero)
> +		Name (_HID, "DLN2000")
> +
> +		Device (STAC)
> +		{
> +			Name (_ADR, Zero)
> +			Name (_HID, "BMC150A")
> +			Name (_CID, "INTACCL")
> +			Name (_UID, One)
> +
> +			Method (_CRS, 0, Serialized)
> +			{
> +				Name (RBUF, ResourceTemplate ()
> +				{
> +					I2cSerialBus (0x0010, ControllerInitiated, 0x00061A80,
> +						      AddressingMode7Bit, "\\DLN0",
> +						      0x00, ResourceConsumer, ,)
> +
> +					GpioInt (Level, ActiveHigh, Exclusive, PullDown, 0x0000,
> +						 "\\DLN0", 0x00, ResourceConsumer, , )
> +					{ // Pin list
> +						0
> +					}
> +				})
> +				Return (RBUF)
> +		       }
> +		}
> +	}
> +}
> +
> +The resources defined in the devices under the DLN0 are those
> +supported by the I2C, GPIO and SPI sub-systems.
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index f9c4a0b..93f6d1d 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -23,6 +23,8 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/dln2.h>
>  #include <linux/rculist.h>
> +#include <linux/acpi.h>
> +#include <linux/firmware.h>
>  
>  struct dln2_header {
>  	__le16 size;
> @@ -714,6 +716,134 @@ static void dln2_stop(struct dln2_dev *dln2)
>  
>  	dln2_stop_rx_urbs(dln2);
>  }
> +
> +#if IS_ENABLED(CONFIG_ACPI)
> +
> +static struct dln2_acpi_info {
> +	const struct firmware *fw;
> +	acpi_owner_id table_id;
> +	struct acpi_device *dev;
> +	int users;
> +} dln2_acpi_info;
> +
> +static DEFINE_MUTEX(dln2_acpi_lock);
> +
> +static acpi_status dln2_find_acpi_handle(acpi_handle handle, u32 level,
> +					 void *ctxt, void **retv)
> +{
> +	acpi_handle *dln2_handle = (acpi_handle *)retv;
> +
> +	*dln2_handle = handle;
> +
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +static void dln2_probe_acpi(struct dln2_dev *dln2)
> +{
> +	struct device *dev = &dln2->interface->dev;
> +	struct dln2_acpi_info *ai = &dln2_acpi_info;
> +	acpi_handle h = NULL;
> +	int ret;
> +	bool fw_loaded = false;
> +
> +	mutex_lock(&dln2_acpi_lock);
> +
> +	if (ai->dev)
> +		goto out_success;
> +
> +	/*
> +	 * Look for the DLN2000 HID in case the ACPI table was loaded
> +	 * externally (e.g. from qemu).
> +	 */
> +	acpi_get_devices("DLN20000", dln2_find_acpi_handle, NULL, &h);
> +	if (!h) {
> +		/* Try to load the ACPI table via a firmware file */
> +		ret = request_firmware(&ai->fw, "dln2.aml", NULL);
> +		if (ret)
> +			goto out_unlock;
> +
> +		ret = acpi_load_table((void *)ai->fw->data);
> +		if (ret) {
> +			dev_err(dev, "invalid ACPI table\n");
> +			goto out_release_fw;
> +		}
> +
> +		acpi_get_devices("DLN20000", dln2_find_acpi_handle, NULL, &h);
> +		if (!h) {
> +			dev_err(dev, "not a DLN2 ACPI table\n");
> +			goto out_leak_fw;
> +		}
> +
> +		ret = acpi_get_id(h, &ai->table_id);
> +		if (ret) {
> +			dev_err(dev, "acpi_get_id failed: %d\n", ret);
> +			goto out_leak_fw;
> +		}
> +
> +		ret = acpi_bus_scan(h);
> +		if (ret) {
> +			dev_err(dev, "acpi_bus_scan failed: %d\n", ret);
> +			goto out_leak_fw;
> +		}
> +
> +		fw_loaded = true;
> +	}
> +
> +	ret = acpi_bus_get_device(h, &ai->dev);
> +	if (ret) {
> +		dev_err(dev, "failed to get ACPI device: %d\n", ret);
> +		if (fw_loaded) {
> +			acpi_unload_table_id(ai->table_id);
> +			goto out_leak_fw;
> +		}
> +		goto out_unlock;
> +	}
> +
> +out_success:
> +	ACPI_COMPANION_SET(dev, ai->dev);
> +	ai->users++;
> +	mutex_unlock(&dln2_acpi_lock);
> +	return;
> +
> +out_release_fw:
> +	release_firmware(ai->fw);
> +out_leak_fw:
> +	/*
> +	 * Once a table is loaded we can't release the firmware anymore because
> +	 * acpi_unload_table does not actually unload the table but keeps it in
> +	 * memory to speed up subsequent loads.
> +	 */
> +	ai->fw = NULL;
> +out_unlock:
> +	mutex_unlock(&dln2_acpi_lock);
> +}
> +
> +static void dln2_disconnect_acpi(struct dln2_dev *dln2)
> +{
> +	struct dln2_acpi_info *ai = &dln2_acpi_info;
> +
> +	mutex_lock(&dln2_acpi_lock);
> +	if (--ai->users == 0 && ai->fw) {
> +		acpi_scan_lock_acquire();
> +		acpi_bus_trim(ai->dev);
> +		acpi_scan_lock_release();
> +		acpi_unload_table_id(ai->table_id);
> +		ai->dev = NULL;
> +		/* we can't release firmware see comment in dln2_probe_acpi */
> +		ai->fw = NULL;
> +	}
> +	mutex_unlock(&dln2_acpi_lock);
> +}
> +#else
> +static void dln2_probe_acpi(struct dln2_dev *dln2)
> +{
> +}
> +
> +static void dln2_disconnect_acpi(struct dln2_dev *dln2)
> +{
> +}
> +#endif
> +
>  static void dln2_disconnect(struct usb_interface *interface)
>  {
>  	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> @@ -722,6 +852,8 @@ static void dln2_disconnect(struct usb_interface *interface)
>  
>  	mfd_remove_devices(&interface->dev);
>  
> +	dln2_disconnect_acpi(dln2);
> +
>  	dln2_free(dln2);
>  }
>  
> @@ -774,6 +906,8 @@ static int dln2_probe(struct usb_interface *interface,
>  		goto out_stop_rx;
>  	}
>  
> +	dln2_probe_acpi(dln2);
> +
>  	ret = mfd_add_hotplug_devices(dev, dln2_devs, ARRAY_SIZE(dln2_devs));
>  	if (ret != 0) {
>  		dev_err(dev, "failed to add mfd devices to core\n");
> -- 
> 1.9.1
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] mfd: dln2: add support for ACPI

[Rafael J. Wysocki]

On Tuesday, December 16, 2014 06:12:32 PM Octavian Purdila wrote:
> This patch adds support to load a custom ACPI table that describes
> devices connected via the DLN2 USB to I2C/SPI/GPIO bridge.
> 
> The ACPI table can be loaded either externally (from QEMU or with
> CONFIG_ACPI_CUSTOM_DSDT) or it can be loaded as firmware file with the
> name dln2.aml. The driver looks for an ACPI device entry with _HID set
> to "DLN20000" and makes it the ACPI companion for DLN2 USB
> sub-drivers.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  Documentation/acpi/dln2-acpi.txt |  62 ++++++++++++++++++
>  drivers/mfd/dln2.c               | 134 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 196 insertions(+)
>  create mode 100644 Documentation/acpi/dln2-acpi.txt
> 
> diff --git a/Documentation/acpi/dln2-acpi.txt b/Documentation/acpi/dln2-acpi.txt
> new file mode 100644
> index 0000000..d76605f
> --- /dev/null
> +++ b/Documentation/acpi/dln2-acpi.txt
> @@ -0,0 +1,62 @@
> +Diolan DLN2 custom APCI table
> +
> +The Diolan DLN2 is an USB to I2C/SPI/GPIO bridge and as such it can be used to
> +connect to various I2C or SPI devices. Because these busses lack an enumeration
> +protocol, the driver obtains various information about the device (such as I2C
> +address and GPIO pins) from either ACPI or device tree.
> +
> +To allow enumerating devices and their properties via ACPI, the Diolan
> +driver looks for an ACPI tree with the root _HID set to "DLN20000". If
> +it finds such an ACPI object it will set the ACPI companion to the
> +DLN2 MFD driver and from their it will be propagated to all its
> +sub-devices (I2C, GPIO, SPI).
> +
> +The user can either load the custom DSDT table with three methods:

s/DSDT/ACPI/

> +
> +1. Via QEMU (see -acpitable)
> +
> +2. Via the CONFIG_ACPI_CUSTOM_DSDT kernel config option (see
> +Documentation/acpi/dsdt-override.txt)
> +
> +3. By placing the custom DSDT in the firmware paths in a file name
> +dln2.aml.

Surely SSDT?

> +
> +Here is an example ACPI table that enumerates a BMC150 accelerometer
> +and defines its I2C address and GPIO pin used as an interrupt source:
> +
> +DefinitionBlock ("ssdt.aml", "SSDT", 1, "INTEL ", "CpuDptf", 0x00000003)
> +{
> +	Device (DLN0)
> +	{
> +		Name (_ADR, Zero)
> +		Name (_HID, "DLN2000")
> +
> +		Device (STAC)
> +		{
> +			Name (_ADR, Zero)
> +			Name (_HID, "BMC150A")
> +			Name (_CID, "INTACCL")
> +			Name (_UID, One)
> +
> +			Method (_CRS, 0, Serialized)
> +			{
> +				Name (RBUF, ResourceTemplate ()
> +				{
> +					I2cSerialBus (0x0010, ControllerInitiated, 0x00061A80,
> +						      AddressingMode7Bit, "\\DLN0",
> +						      0x00, ResourceConsumer, ,)
> +
> +					GpioInt (Level, ActiveHigh, Exclusive, PullDown, 0x0000,
> +						 "\\DLN0", 0x00, ResourceConsumer, , )
> +					{ // Pin list
> +						0
> +					}
> +				})
> +				Return (RBUF)
> +		       }
> +		}
> +	}
> +}
> +
> +The resources defined in the devices under the DLN0 are those
> +supported by the I2C, GPIO and SPI sub-systems.
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index f9c4a0b..93f6d1d 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -23,6 +23,8 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/dln2.h>
>  #include <linux/rculist.h>
> +#include <linux/acpi.h>
> +#include <linux/firmware.h>
>  

OK, so correct me if I'm wrong.

When the (USB) dnl2 device is probed, it is supposed to find an ACPI companion
for itself and if it's not there, the driver will try to load a custom SSDT from
a firmware blob in the hope that the companion will be there?

So if I'm not wrong, why is this not broken?

It is not sufficient to call ACPI_COMPANION_SET(dev, ai->dev) to set the device's
companion.  acpi_bind_one() needs to be run in addition to that, but acpi_bind_one()
is not to be called from drivers.  It is called by the core automatically during
device registration and ACPI_COMPANION_SET() is to be used *before* that, not after.

So if I'm not missing anything, the design here is entirely backwards and we
need to talk about how to implement it correctly at the design level in the
first place.

And no, "Let's come up with a patch that sort of works, throw it at the maintainers
and see what happens" is not an acceptable approach, sorry.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/4] mfd: dln2: add support for ACPI

[Octavian Purdila]

On Thu, Jan 22, 2015 at 4:00 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>

Hi Rafael,

Thanks for reviewing the patch.

> On Tuesday, December 16, 2014 06:12:32 PM Octavian Purdila wrote:
> > This patch adds support to load a custom ACPI table that describes
> > devices connected via the DLN2 USB to I2C/SPI/GPIO bridge.
> >
> > The ACPI table can be loaded either externally (from QEMU or with
> > CONFIG_ACPI_CUSTOM_DSDT) or it can be loaded as firmware file with the
> > name dln2.aml. The driver looks for an ACPI device entry with _HID set
> > to "DLN20000" and makes it the ACPI companion for DLN2 USB
> > sub-drivers.
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > ---
> >  Documentation/acpi/dln2-acpi.txt |  62 ++++++++++++++++++
> >  drivers/mfd/dln2.c               | 134 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 196 insertions(+)
> >  create mode 100644 Documentation/acpi/dln2-acpi.txt
> >
> > diff --git a/Documentation/acpi/dln2-acpi.txt b/Documentation/acpi/dln2-acpi.txt
> > new file mode 100644
> > index 0000000..d76605f
> > --- /dev/null
> > +++ b/Documentation/acpi/dln2-acpi.txt
> > @@ -0,0 +1,62 @@
> > +Diolan DLN2 custom APCI table
> > +
> > +The Diolan DLN2 is an USB to I2C/SPI/GPIO bridge and as such it can be used to
> > +connect to various I2C or SPI devices. Because these busses lack an enumeration
> > +protocol, the driver obtains various information about the device (such as I2C
> > +address and GPIO pins) from either ACPI or device tree.
> > +
> > +To allow enumerating devices and their properties via ACPI, the Diolan
> > +driver looks for an ACPI tree with the root _HID set to "DLN20000". If
> > +it finds such an ACPI object it will set the ACPI companion to the
> > +DLN2 MFD driver and from their it will be propagated to all its
> > +sub-devices (I2C, GPIO, SPI).
> > +
> > +The user can either load the custom DSDT table with three methods:
>
> s/DSDT/ACPI/

OK.

> > +
> > +1. Via QEMU (see -acpitable)
> > +
> > +2. Via the CONFIG_ACPI_CUSTOM_DSDT kernel config option (see
> > +Documentation/acpi/dsdt-override.txt)
> > +
> > +3. By placing the custom DSDT in the firmware paths in a file name
> > +dln2.aml.
>
> Surely SSDT?
>

Correct.

<snip>

> > diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> > index f9c4a0b..93f6d1d 100644
> > --- a/drivers/mfd/dln2.c
> > +++ b/drivers/mfd/dln2.c
> > @@ -23,6 +23,8 @@
> >  #include <linux/mfd/core.h>
> >  #include <linux/mfd/dln2.h>
> >  #include <linux/rculist.h>
> > +#include <linux/acpi.h>
> > +#include <linux/firmware.h>
> >
>
> OK, so correct me if I'm wrong.
>
> When the (USB) dnl2 device is probed, it is supposed to find an ACPI companion
> for itself and if it's not there, the driver will try to load a custom SSDT from
> a firmware blob in the hope that the companion will be there?
>
> So if I'm not wrong, why is this not broken?
>
> It is not sufficient to call ACPI_COMPANION_SET(dev, ai->dev) to set the device's
> companion.  acpi_bind_one() needs to be run in addition to that, but acpi_bind_one()
> is not to be called from drivers.  It is called by the core automatically during
> device registration and ACPI_COMPANION_SET() is to be used *before* that, not after.
>
> So if I'm not missing anything, the design here is entirely backwards and we
> need to talk about how to implement it correctly at the design level in the
> first place.
>

The idea here is to set the companion for the MFD sub-devices. Mika's
commit "mfd: Add ACPI support" propagates the parent's companion to
the MFD sub-devices, so it is sufficient to set the ACPI companion to
the USB device.

Then, the companion will be propagated to the sub-devices after which
acpi_bind_one() will be called for the sub-devices from
mfd_add_devices (via platform_device_add -> device_add ->
platform_notify).

It is true that the USB dev will have its ACPI companion set without
having acpi_bind_one called but I do not see any harm in that. Even
though acpi_unbind_one() is called it will not find the USB dev on the
physical node list so no put_device() imbalance is caused.

> And no, "Let's come up with a patch that sort of works, throw it at the maintainers
> and see what happens" is not an acceptable approach, sorry.

This patch is based on your feedback of the previous RFC patch set:

On Thu, Dec 11, 2014 at 11:44 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> it seems to me that it would be much more straightforward to check for the existence
> of the "DLN20000" device when you are about to register DLN2 USB sub-devices
> and set it directly as an ACPI companion for them if present.

Re: [PATCH 3/4] mfd: dln2: add support for ACPI

[Rafael J. Wysocki]

On Thursday, January 22, 2015 12:13:13 PM Octavian Purdila wrote:
> On Thu, Jan 22, 2015 at 4:00 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> 
> Hi Rafael,
> 
> Thanks for reviewing the patch.
> 
> > On Tuesday, December 16, 2014 06:12:32 PM Octavian Purdila wrote:
> > > This patch adds support to load a custom ACPI table that describes
> > > devices connected via the DLN2 USB to I2C/SPI/GPIO bridge.
> > >
> > > The ACPI table can be loaded either externally (from QEMU or with
> > > CONFIG_ACPI_CUSTOM_DSDT) or it can be loaded as firmware file with the
> > > name dln2.aml. The driver looks for an ACPI device entry with _HID set
> > > to "DLN20000" and makes it the ACPI companion for DLN2 USB
> > > sub-drivers.
> > >
> > > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > > ---
> > >  Documentation/acpi/dln2-acpi.txt |  62 ++++++++++++++++++
> > >  drivers/mfd/dln2.c               | 134 +++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 196 insertions(+)
> > >  create mode 100644 Documentation/acpi/dln2-acpi.txt
> > >
> > > diff --git a/Documentation/acpi/dln2-acpi.txt b/Documentation/acpi/dln2-acpi.txt
> > > new file mode 100644
> > > index 0000000..d76605f
> > > --- /dev/null
> > > +++ b/Documentation/acpi/dln2-acpi.txt
> > > @@ -0,0 +1,62 @@
> > > +Diolan DLN2 custom APCI table
> > > +
> > > +The Diolan DLN2 is an USB to I2C/SPI/GPIO bridge and as such it can be used to
> > > +connect to various I2C or SPI devices. Because these busses lack an enumeration
> > > +protocol, the driver obtains various information about the device (such as I2C
> > > +address and GPIO pins) from either ACPI or device tree.
> > > +
> > > +To allow enumerating devices and their properties via ACPI, the Diolan
> > > +driver looks for an ACPI tree with the root _HID set to "DLN20000". If
> > > +it finds such an ACPI object it will set the ACPI companion to the
> > > +DLN2 MFD driver and from their it will be propagated to all its
> > > +sub-devices (I2C, GPIO, SPI).
> > > +
> > > +The user can either load the custom DSDT table with three methods:
> >
> > s/DSDT/ACPI/
> 
> OK.
> 
> > > +
> > > +1. Via QEMU (see -acpitable)
> > > +
> > > +2. Via the CONFIG_ACPI_CUSTOM_DSDT kernel config option (see
> > > +Documentation/acpi/dsdt-override.txt)
> > > +
> > > +3. By placing the custom DSDT in the firmware paths in a file name
> > > +dln2.aml.
> >
> > Surely SSDT?
> >
> 
> Correct.
> 
> <snip>
> 
> > > diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> > > index f9c4a0b..93f6d1d 100644
> > > --- a/drivers/mfd/dln2.c
> > > +++ b/drivers/mfd/dln2.c
> > > @@ -23,6 +23,8 @@
> > >  #include <linux/mfd/core.h>
> > >  #include <linux/mfd/dln2.h>
> > >  #include <linux/rculist.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/firmware.h>
> > >
> >
> > OK, so correct me if I'm wrong.
> >
> > When the (USB) dnl2 device is probed, it is supposed to find an ACPI companion
> > for itself and if it's not there, the driver will try to load a custom SSDT from
> > a firmware blob in the hope that the companion will be there?
> >
> > So if I'm not wrong, why is this not broken?
> >
> > It is not sufficient to call ACPI_COMPANION_SET(dev, ai->dev) to set the device's
> > companion.  acpi_bind_one() needs to be run in addition to that, but acpi_bind_one()
> > is not to be called from drivers.  It is called by the core automatically during
> > device registration and ACPI_COMPANION_SET() is to be used *before* that, not after.
> >
> > So if I'm not missing anything, the design here is entirely backwards and we
> > need to talk about how to implement it correctly at the design level in the
> > first place.
> >
> 
> The idea here is to set the companion for the MFD sub-devices. Mika's
> commit "mfd: Add ACPI support" propagates the parent's companion to
> the MFD sub-devices, so it is sufficient to set the ACPI companion to
> the USB device.

For the USB device itself you'll then end up with an incomplete binding (you
can't get back to the USB device from the ACPI object), so no, it isn't
sufficient.

> Then, the companion will be propagated to the sub-devices after which
> acpi_bind_one() will be called for the sub-devices from
> mfd_add_devices (via platform_device_add -> device_add ->
> platform_notify).

In fact, your use case doesn't seem to cover any of the use cases that
the Mika's commit addressed.  Namely, your parent device doesn't have
an ACPI companion to start with and you want your MFD cells to be bound
to the "DLN2000" thing.  That's why you're setting the ACPI companion
for the USB device, isn't it?

> It is true that the USB dev will have its ACPI companion set without
> having acpi_bind_one called but I do not see any harm in that. Even
> though acpi_unbind_one() is called it will not find the USB dev on the
> physical node list so no put_device() imbalance is caused.

Well, there are places where it matters.  Some links in sysfs will be missing
for one example.  Also please see the changelog of commit 52870786ff5d (ACPI:
Use ACPI companion to match only the first physical device).

Bottom line: You really should be using acpi_bind_one() here and
acpi_unbind_one() on disconnect if you have to.

> > And no, "Let's come up with a patch that sort of works, throw it at the maintainers
> > and see what happens" is not an acceptable approach, sorry.
> 
> This patch is based on your feedback of the previous RFC patch set:

Oh, is it?  I can't recall advising you to use request_firmware() for
uploading ACPI tables or some other questionable things that the patch is
doing.

And if it still was an RFC, that wouldn't be a problem, but if you just send
non-RFC patches out, that means you want people to merge them.  This is bad
if the patches in question are not in a good enough shape and this one isn't.

Now, why is this a bad idea to load ACPI tables from a driver using
request_firmware()?  Because those tables are not device firmare.  They are
not firmware that you load into a device to make it work (which then works
with all instances of the given hardware), they are part of system configuration
information and have to match what's there in the system.  For instance, if you
ship your example SSDT with a general-purpose distro, it may just not match the
hardware configuration of systems that people will try to use it with.

So, while it is sort of OK to look up "DLN2000" and bind the USB device to
that by hand (although this looks ugly to me), it is not OK to load a random
custom SSDT if it is missing.

We need to add a generic mechanism for loading custom SSDTs not present in the
firmware image and maybe even to load them on demand, but that cannot happen in
an ad-hoc way.  So the entire table loading-unloading code in your patch can go
away for now and you need to fail probing if the "DLN2000" ACPI object is not
present.

Also I think you need to add "DLN2000" to the blacklist in drivers/acpi/acpi_platform.c
to prevent the ACPI core from creating a platform device for it automatically.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/4] mfd: dln2: add support for ACPI

[Octavian Purdila]

On Fri, Jan 23, 2015 at 12:09 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, January 22, 2015 12:13:13 PM Octavian Purdila wrote:
>> On Thu, Jan 22, 2015 at 4:00 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
<snip>
>> The idea here is to set the companion for the MFD sub-devices. Mika's
>> commit "mfd: Add ACPI support" propagates the parent's companion to
>> the MFD sub-devices, so it is sufficient to set the ACPI companion to
>> the USB device.
>
> For the USB device itself you'll then end up with an incomplete binding (you
> can't get back to the USB device from the ACPI object), so no, it isn't
> sufficient.
>
>> Then, the companion will be propagated to the sub-devices after which
>> acpi_bind_one() will be called for the sub-devices from
>> mfd_add_devices (via platform_device_add -> device_add ->
>> platform_notify).
>
> In fact, your use case doesn't seem to cover any of the use cases that
> the Mika's commit addressed.  Namely, your parent device doesn't have
> an ACPI companion to start with and you want your MFD cells to be bound
> to the "DLN2000" thing.  That's why you're setting the ACPI companion
> for the USB device, isn't it?
>
>> It is true that the USB dev will have its ACPI companion set without
>> having acpi_bind_one called but I do not see any harm in that. Even
>> though acpi_unbind_one() is called it will not find the USB dev on the
>> physical node list so no put_device() imbalance is caused.
>
> Well, there are places where it matters.  Some links in sysfs will be missing
> for one example.  Also please see the changelog of commit 52870786ff5d (ACPI:
> Use ACPI companion to match only the first physical device).
>
> Bottom line: You really should be using acpi_bind_one() here and
> acpi_unbind_one() on disconnect if you have to.
>

OK, I understand now.

>> > And no, "Let's come up with a patch that sort of works, throw it at the maintainers
>> > and see what happens" is not an acceptable approach, sorry.
>>
>> This patch is based on your feedback of the previous RFC patch set:
>
> Oh, is it?  I can't recall advising you to use request_firmware() for
> uploading ACPI tables or some other questionable things that the patch is
> doing.
>
> And if it still was an RFC, that wouldn't be a problem, but if you just send
> non-RFC patches out, that means you want people to merge them.  This is bad
> if the patches in question are not in a good enough shape and this one isn't.
>

Yes, this should have been tagged with RFC, sorry about that.

> Now, why is this a bad idea to load ACPI tables from a driver using
> request_firmware()?  Because those tables are not device firmare.  They are
> not firmware that you load into a device to make it work (which then works
> with all instances of the given hardware), they are part of system configuration
> information and have to match what's there in the system.  For instance, if you
> ship your example SSDT with a general-purpose distro, it may just not match the
> hardware configuration of systems that people will try to use it with.
>
> So, while it is sort of OK to look up "DLN2000" and bind the USB device to
> that by hand (although this looks ugly to me), it is not OK to load a random
> custom SSDT if it is missing.
>
> We need to add a generic mechanism for loading custom SSDTs not present in the
> firmware image and maybe even to load them on demand, but that cannot happen in
> an ad-hoc way.  So the entire table loading-unloading code in your patch can go
> away for now and you need to fail probing if the "DLN2000" ACPI object is not
> present.
>

That sounds interesting, I like the idea of a generic mechanism for
loading custom SSDTs. Do you have any initial thoughts/pointers for
starting that?

Thanks for the review and feedback.

Re: [PATCH 3/4] mfd: dln2: add support for ACPI

[Rafael J. Wysocki]

On Friday, January 23, 2015 08:47:58 AM Octavian Purdila wrote:
> On Fri, Jan 23, 2015 at 12:09 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, January 22, 2015 12:13:13 PM Octavian Purdila wrote:
> >> On Thu, Jan 22, 2015 at 4:00 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> <snip>
> >> The idea here is to set the companion for the MFD sub-devices. Mika's
> >> commit "mfd: Add ACPI support" propagates the parent's companion to
> >> the MFD sub-devices, so it is sufficient to set the ACPI companion to
> >> the USB device.
> >
> > For the USB device itself you'll then end up with an incomplete binding (you
> > can't get back to the USB device from the ACPI object), so no, it isn't
> > sufficient.
> >
> >> Then, the companion will be propagated to the sub-devices after which
> >> acpi_bind_one() will be called for the sub-devices from
> >> mfd_add_devices (via platform_device_add -> device_add ->
> >> platform_notify).
> >
> > In fact, your use case doesn't seem to cover any of the use cases that
> > the Mika's commit addressed.  Namely, your parent device doesn't have
> > an ACPI companion to start with and you want your MFD cells to be bound
> > to the "DLN2000" thing.  That's why you're setting the ACPI companion
> > for the USB device, isn't it?
> >
> >> It is true that the USB dev will have its ACPI companion set without
> >> having acpi_bind_one called but I do not see any harm in that. Even
> >> though acpi_unbind_one() is called it will not find the USB dev on the
> >> physical node list so no put_device() imbalance is caused.
> >
> > Well, there are places where it matters.  Some links in sysfs will be missing
> > for one example.  Also please see the changelog of commit 52870786ff5d (ACPI:
> > Use ACPI companion to match only the first physical device).
> >
> > Bottom line: You really should be using acpi_bind_one() here and
> > acpi_unbind_one() on disconnect if you have to.
> >
> 
> OK, I understand now.
> 
> >> > And no, "Let's come up with a patch that sort of works, throw it at the maintainers
> >> > and see what happens" is not an acceptable approach, sorry.
> >>
> >> This patch is based on your feedback of the previous RFC patch set:
> >
> > Oh, is it?  I can't recall advising you to use request_firmware() for
> > uploading ACPI tables or some other questionable things that the patch is
> > doing.
> >
> > And if it still was an RFC, that wouldn't be a problem, but if you just send
> > non-RFC patches out, that means you want people to merge them.  This is bad
> > if the patches in question are not in a good enough shape and this one isn't.
> >
> 
> Yes, this should have been tagged with RFC, sorry about that.
> 
> > Now, why is this a bad idea to load ACPI tables from a driver using
> > request_firmware()?  Because those tables are not device firmare.  They are
> > not firmware that you load into a device to make it work (which then works
> > with all instances of the given hardware), they are part of system configuration
> > information and have to match what's there in the system.  For instance, if you
> > ship your example SSDT with a general-purpose distro, it may just not match the
> > hardware configuration of systems that people will try to use it with.
> >
> > So, while it is sort of OK to look up "DLN2000" and bind the USB device to
> > that by hand (although this looks ugly to me), it is not OK to load a random
> > custom SSDT if it is missing.
> >
> > We need to add a generic mechanism for loading custom SSDTs not present in the
> > firmware image and maybe even to load them on demand, but that cannot happen in
> > an ad-hoc way.  So the entire table loading-unloading code in your patch can go
> > away for now and you need to fail probing if the "DLN2000" ACPI object is not
> > present.
> >
> 
> That sounds interesting, I like the idea of a generic mechanism for
> loading custom SSDTs. Do you have any initial thoughts/pointers for
> starting that?

Thoughts - yes, pointers - not so much.  I'll get back to you when I'm done
with the e-mail backlog from the last few weeks.

> Thanks for the review and feedback.

No problem.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 4/4] gpio: dln2: add support for ACPI pin configuration

[Octavian Purdila]

This patch configures the pull-up/pull-down properties based on the
ACPI configuration. It scans the children of the DLN2 root entry and
looks for GPIO resources and applies the pull-up/pull-down
configurations on the pins.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/gpio/gpio-dln2.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/gpio/gpio-dln2.c b/drivers/gpio/gpio-dln2.c
index 2fdb138..98189d5 100644
--- a/drivers/gpio/gpio-dln2.c
+++ b/drivers/gpio/gpio-dln2.c
@@ -19,6 +19,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/platform_device.h>
 #include <linux/mfd/dln2.h>
+#include <linux/acpi.h>
 
 #define DLN2_GPIO_ID			0x01
 
@@ -36,6 +37,10 @@
 #define DLN2_GPIO_PIN_GET_DIRECTION	DLN2_CMD(0x14, DLN2_GPIO_ID)
 #define DLN2_GPIO_PIN_SET_EVENT_CFG	DLN2_CMD(0x1E, DLN2_GPIO_ID)
 #define DLN2_GPIO_PIN_GET_EVENT_CFG	DLN2_CMD(0x1F, DLN2_GPIO_ID)
+#define CMD_GPIO_PIN_PULLUP_ENABLE	DLN2_CMD(0x18, DLN2_GPIO_ID)
+#define CMD_GPIO_PIN_PULLUP_DISABLE	DLN2_CMD(0x19, DLN2_GPIO_ID)
+#define CMD_GPIO_PIN_PULLDOWN_ENABLE	DLN2_CMD(0x20, DLN2_GPIO_ID)
+#define CMD_GPIO_PIN_PULLDOWN_DISABLE	DLN2_CMD(0x21, DLN2_GPIO_ID)
 
 #define DLN2_GPIO_EVENT_NONE		0
 #define DLN2_GPIO_EVENT_CHANGE		1
@@ -428,6 +433,75 @@ static void dln2_gpio_event(struct platform_device *pdev, u16 echo,
 	}
 }
 
+#if IS_ENABLED(CONFIG_ACPI)
+static int dln2_gpio_do_acpi_setup(struct acpi_resource *ares, void *data)
+{
+	int i;
+	u16 cmd[2];
+	const char *info;
+	struct dln2_gpio *dln2 = data;
+	const struct acpi_resource_gpio *agpio = &ares->data.gpio;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
+		return 1;
+
+	switch (agpio->pin_config) {
+	case  ACPI_PIN_CONFIG_PULLUP:
+		info = "pullup";
+		cmd[0] = CMD_GPIO_PIN_PULLDOWN_DISABLE;
+		cmd[1] = CMD_GPIO_PIN_PULLUP_ENABLE;
+		break;
+	case  ACPI_PIN_CONFIG_PULLDOWN:
+		info = "pulldown";
+		cmd[0] = CMD_GPIO_PIN_PULLDOWN_ENABLE;
+		cmd[1] = CMD_GPIO_PIN_PULLUP_DISABLE;
+		break;
+	case  ACPI_PIN_CONFIG_NOPULL:
+		info = "nopull";
+		cmd[0] = CMD_GPIO_PIN_PULLDOWN_DISABLE;
+		cmd[1] = CMD_GPIO_PIN_PULLUP_DISABLE;
+		break;
+	default:
+		return 1;
+	}
+
+	for (i = 0; i < agpio->pin_table_length; i++) {
+		int pin = agpio->pin_table[i];
+
+		dev_info(dln2->gpio.dev, "setting %s on pin %d\n", info, pin);
+		dln2_gpio_pin_cmd(dln2, cmd[0], pin);
+		dln2_gpio_pin_cmd(dln2, cmd[1], pin);
+	}
+
+	return 1;
+}
+
+static void dln2_gpio_acpi_setup(struct dln2_gpio *dln2)
+{
+	struct acpi_device *adev, *child;
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(dln2->gpio.dev);
+	if (!handle || acpi_bus_get_device(handle, &adev))
+		return;
+
+	list_for_each_entry(child, &adev->children, node) {
+		LIST_HEAD(res);
+		int ret;
+
+		ret = acpi_dev_get_resources(child, &res,
+					     dln2_gpio_do_acpi_setup, dln2);
+		if (ret < 0)
+			continue;
+		acpi_dev_free_resource_list(&res);
+	}
+}
+#else
+static void dln2_gpio_acpi_setup(struct dln2_gpio *dln2)
+{
+}
+#endif
+
 static int dln2_gpio_probe(struct platform_device *pdev)
 {
 	struct dln2_gpio *dln2;
@@ -492,6 +566,8 @@ static int dln2_gpio_probe(struct platform_device *pdev)
 		goto out_gpiochip_remove;
 	}
 
+	dln2_gpio_acpi_setup(dln2);
+
 	return 0;
 
 out_gpiochip_remove:
-- 
1.9.1