ACPI serdev support

Signed-off-by Frédéric Danis <[email protected]>

---
 drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 5 deletions(-)

Quoted 4 lines

Signed-off-by Frédéric Danis <[email protected]>

---
drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---

1 file changed, 94 insertions(+), 5 deletions(-)

I can not take patches without any changelog text, and you should feel bad about trying to get me to do so :)

Signed-off-by Frédéric Danis <[email protected]>

---
drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---

1 file changed, 94 insertions(+), 5 deletions(-)

drivers/tty/serdev/core.c +69 -2

index ae1aaa0..923dd4ad 100644
@@ -14,6 +14,7 @@
 * GNU General Public License for more details.
 */
 
+#include <linux/acpi.h>
 #include <linux/errno.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>

@@ -49,13 +50,22 @@ static const struct device_type serdev_ctrl_type = {
 
 static int serdev_device_match(struct device *dev, struct device_driver *drv)
 {
-	/* TODO: ACPI and platform matching */
+	/* TODO: platform matching */
+	if (acpi_driver_match_device(dev, drv))
+		return 1;
+
 	return of_driver_match_device(dev, drv);
 }
 
 static int serdev_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	/* TODO: ACPI and platform modalias */
+	int rc;
+
+	/* TODO: platform modalias */
+	rc = acpi_device_uevent_modalias(dev, env);
+	if (rc != -ENODEV)
+		return rc;
+
 	return of_device_uevent_modalias(dev, env);
 }
 

@@ -260,6 +270,12 @@ static int serdev_drv_remove(struct device *dev)
 static ssize_t modalias_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
+	int len;
+
+	len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
+	if (len != -ENODEV)
+		return len;
+
 	return of_device_modalias(dev, buf, PAGE_SIZE);
 }
 DEVICE_ATTR_RO(modalias);

@@ -385,6 +401,74 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
+					    struct acpi_device *adev)
+{
+	struct serdev_device *serdev = NULL;
+	int err;
+
+	if (acpi_bus_get_status(adev) || !adev->status.present ||
+	    acpi_device_enumerated(adev))
+		return AE_OK;
+
+	serdev = serdev_device_alloc(ctrl);
+	if (!serdev) {
+		dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
+			dev_name(&adev->dev));
+		return AE_NO_MEMORY;
+	}
+
+	ACPI_COMPANION_SET(&serdev->dev, adev);
+	acpi_device_set_enumerated(adev);
+
+	err = serdev_device_add(serdev);
+	if (err) {
+		dev_err(&serdev->dev,
+			"failure adding ACPI device. status %d\n", err);
+		serdev_device_put(serdev);
+	}
+
+	return AE_OK;
+}
+
+static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
+				       void *data, void **return_value)
+{
+	struct serdev_controller *ctrl = data;
+	struct acpi_device *adev;
+
+	if (acpi_bus_get_device(handle, &adev))
+		return AE_OK;
+
+	return acpi_serdev_register_device(ctrl, adev);
+}
+
+static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+	acpi_status status;
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(ctrl->dev.parent);
+	if (!handle)
+		return -ENODEV;

drivers/tty/serdev/core.c +9 -3

+#else
+static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_ACPI */
+
 /**
 * serdev_controller_add() - Add an serdev controller
 * @ctrl:	controller to be registered.

@@ -394,7 +478,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
 */
 int serdev_controller_add(struct serdev_controller *ctrl)
 {
-	int ret;
+	int ret_of, ret_acpi, ret;
 
 	/* Can't register until after driver model init */
 	if (WARN_ON(!is_registered))

@@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller *ctrl)
 	if (ret)
 		return ret;
 
-	ret = of_serdev_register_devices(ctrl);
-	if (ret)
+	ret_of = of_serdev_register_devices(ctrl);

Hi Fred,

Quoted 3 lines

Signed-off-by Fr=C3=A9d=C3=A9ric Danis <[email protected]>

---
drivers/tty/serdev/core.c | 99 +++++++++++++++++++++++++++++++++++++++++=

Quoted 1 line

1 file changed, 94 insertions(+), 5 deletions(-)

Review int serdev_controller_add(struct serdev_controller =

Quoted 1 line

(quoted diff) +0 -0

@@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller =

drivers/tty/serdev/core.c int serdev_controller_add(struct serdev_controller *ctrl)

Quoted 9 lines

drivers/tty/serdev/core.c +4 -2

      if (ret)
              return ret;
 
-     ret =3D of_serdev_register_devices(ctrl);
-     if (ret)
+     ret_of =3D of_serdev_register_devices(ctrl);
+     ret_acpi =3D acpi_serdev_register_devices(ctrl);
+     if (ret_of && ret_acpi) {
+             dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d=

drivers/tty/serdev/core.c int serdev_controller_add(struct serdev_controller *ctrl) · line 501

Quoted 7 lines

drivers/tty/serdev/core.c +3 -0

+                     ctrl->nr, ret_of, ret_acpi);
+             ret =3D -ENODEV;
              goto out_dev_del;
+     }
 
      dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
              ctrl->nr, &ctrl->dev);

Shouldn=E2=80=99t we just consider to always register the controller? Eve=

Quoted 4 lines

Quoted 4 lines

Signed-off-by Frédéric Danis <[email protected]>

---
drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---

1 file changed, 94 insertions(+), 5 deletions(-)

[...]

Quoted 15 lines

Quoted 15 lines

drivers/tty/serdev/core.c +7 -2

@@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller *ctrl)
     if (ret)
             return ret;
 
-     ret = of_serdev_register_devices(ctrl);
-     if (ret)
+     ret_of = of_serdev_register_devices(ctrl);
+     ret_acpi = acpi_serdev_register_devices(ctrl);
+     if (ret_of && ret_acpi) {
+             dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
+                     ctrl->nr, ret_of, ret_acpi);
+             ret = -ENODEV;
             goto out_dev_del;
+     }
 

Quoted 4 lines

drivers/tty/serdev/core.c int serdev_controller_add(struct serdev_controller *ctrl) · line 501

Quoted 2 lines

drivers/tty/serdev/core.c +0 -0

     dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
             ctrl->nr, &ctrl->dev);

Shouldn’t we just consider to always register the controller? Even if there are no devices attached to it.

You argued for the opposite at least in regards to a serdev ldisc. :) The problem is we use the success or failure here to decide if we create a tty char dev or not. I guess we could move that decision out of the core and let the tty code check for devices and decide.

Hi Rob,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Signed-off-by Fr=C3=A9d=C3=A9ric Danis <[email protected]>

---
drivers/tty/serdev/core.c | 99 +++++++++++++++++++++++++++++++++++++++=

Quoted 5 lines

Quoted 1 line

Quoted 1 line

1 file changed, 94 insertions(+), 5 deletions(-)

[...]

Quoted 1 line

Review int serdev_controller_add(struct serdev_controlle=

Quoted 1 line

(quoted diff) +0 -0

@@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controlle=

Quoted 9 lines

Quoted 9 lines

drivers/tty/serdev/core.c int serdev_controller_add(struct serdev_controller *ctrl)

Quoted 9 lines

drivers/tty/serdev/core.c +4 -2

     if (ret)
             return ret;
 
-     ret =3D of_serdev_register_devices(ctrl);
-     if (ret)
+     ret_of =3D of_serdev_register_devices(ctrl);
+     ret_acpi =3D acpi_serdev_register_devices(ctrl);
+     if (ret_of && ret_acpi) {
+             dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:=

Quoted 9 lines

Quoted 9 lines

drivers/tty/serdev/core.c int serdev_controller_add(struct serdev_controller *ctrl) · line 501

Quoted 7 lines

drivers/tty/serdev/core.c +3 -0

+                     ctrl->nr, ret_of, ret_acpi);
+             ret =3D -ENODEV;
             goto out_dev_del;
+     }
 
     dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
             ctrl->nr, &ctrl->dev);

Shouldn=E2=80=99t we just consider to always register the controller? E=

Quoted 5 lines

You argued for the opposite at least in regards to a serdev ldisc. :) The problem is we use the success or failure here to decide if we create a tty char dev or not. I guess we could move that decision out of the core and let the tty code check for devices and decide.

we never got the serdev ldisc working. Is there still an attempt to get t=

Okay, I've got this kind of working. I can run "ldattach HCI /dev/ttyblah" to start the LL driver (nothing attached, just tries firmware loading), then kill ldattach and remove the serdev device. And it actually works a 2nd time. I've updated my serdev-ldisc branch with the fixes. The real question is what have I broken in the tty device. At least "echo rob > /dev/ttyS1" still works (in qemu) after attaching and de-attaching.

Quoted 54 lines

drivers/tty/serdev/core.c +54 -0

+#ifdef CONFIG_ACPI
+static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
+					    struct acpi_device *adev)
+{
+	struct serdev_device *serdev = NULL;
+	int err;
+
+	if (acpi_bus_get_status(adev) || !adev->status.present ||
+	    acpi_device_enumerated(adev))
+		return AE_OK;
+
+	serdev = serdev_device_alloc(ctrl);
+	if (!serdev) {
+		dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
+			dev_name(&adev->dev));
+		return AE_NO_MEMORY;
+	}
+
+	ACPI_COMPANION_SET(&serdev->dev, adev);
+	acpi_device_set_enumerated(adev);
+
+	err = serdev_device_add(serdev);
+	if (err) {
+		dev_err(&serdev->dev,
+			"failure adding ACPI device. status %d\n", err);
+		serdev_device_put(serdev);
+	}
+
+	return AE_OK;
+}
+
+static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
+				       void *data, void **return_value)
+{
+	struct serdev_controller *ctrl = data;
+	struct acpi_device *adev;
+
+	if (acpi_bus_get_device(handle, &adev))
+		return AE_OK;
+
+	return acpi_serdev_register_device(ctrl, adev);
+}
+
+static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+	acpi_status status;
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(ctrl->dev.parent);
+	if (!handle)
+		return -ENODEV;
+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+				     acpi_serdev_add_device, NULL, ctrl, NULL);

drivers/tty/serdev/core.c static int of_serdev_register_devices(struct serdev_controller *ctrl) · line 464

Quoted 7 lines

drivers/tty/serdev/core.c +7 -0

+	if (ACPI_FAILURE(status)) {
+		dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}

how are we ensuring that we only take UART devices into account here?

Quoted 54 lines

Quoted 54 lines

drivers/tty/serdev/core.c +54 -0

+#ifdef CONFIG_ACPI
+static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
+					    struct acpi_device *adev)
+{
+	struct serdev_device *serdev = NULL;
+	int err;
+
+	if (acpi_bus_get_status(adev) || !adev->status.present ||
+	    acpi_device_enumerated(adev))
+		return AE_OK;
+
+	serdev = serdev_device_alloc(ctrl);
+	if (!serdev) {
+		dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
+			dev_name(&adev->dev));
+		return AE_NO_MEMORY;
+	}
+
+	ACPI_COMPANION_SET(&serdev->dev, adev);
+	acpi_device_set_enumerated(adev);
+
+	err = serdev_device_add(serdev);
+	if (err) {
+		dev_err(&serdev->dev,
+			"failure adding ACPI device. status %d\n", err);
+		serdev_device_put(serdev);
+	}
+
+	return AE_OK;
+}
+
+static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
+				       void *data, void **return_value)
+{
+	struct serdev_controller *ctrl = data;
+	struct acpi_device *adev;
+
+	if (acpi_bus_get_device(handle, &adev))
+		return AE_OK;
+
+	return acpi_serdev_register_device(ctrl, adev);
+}
+
+static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+	acpi_status status;
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(ctrl->dev.parent);
+	if (!handle)
+		return -ENODEV;
+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+				     acpi_serdev_add_device, NULL, ctrl, NULL);

Quoted 8 lines

drivers/tty/serdev/core.c static int of_serdev_register_devices(struct serdev_controller *ctrl) · line 464

Quoted 7 lines

drivers/tty/serdev/core.c +7 -0

+	if (ACPI_FAILURE(status)) {
+		dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}

how are we ensuring that we only take UART devices into account here?

acpi_serdev_add_device() callback will only take into account entries without enumerated flag set.
This flags is set for all entries during ACPI scan, except for SPI and I2C serial devices, and for UART with 2nd patch in the series.

Quoted 63 lines

Quoted 62 lines

Quoted 51 lines

drivers/tty/serdev/core.c +51 -0

+#ifdef CONFIG_ACPI
+static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
+					    struct acpi_device *adev)
+{
+	struct serdev_device *serdev = NULL;
+	int err;
+
+	if (acpi_bus_get_status(adev) || !adev->status.present ||
+	    acpi_device_enumerated(adev))
+		return AE_OK;
+
+	serdev = serdev_device_alloc(ctrl);
+	if (!serdev) {
+		dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
+			dev_name(&adev->dev));
+		return AE_NO_MEMORY;
+	}
+
+	ACPI_COMPANION_SET(&serdev->dev, adev);
+	acpi_device_set_enumerated(adev);
+
+	err = serdev_device_add(serdev);
+	if (err) {
+		dev_err(&serdev->dev,
+			"failure adding ACPI device. status %d\n", err);
+		serdev_device_put(serdev);
+	}
+
+	return AE_OK;
+}
+
+static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
+				       void *data, void **return_value)
+{
+	struct serdev_controller *ctrl = data;
+	struct acpi_device *adev;
+
+	if (acpi_bus_get_device(handle, &adev))
+		return AE_OK;
+
+	return acpi_serdev_register_device(ctrl, adev);
+}
+
+static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+	acpi_status status;
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(ctrl->dev.parent);
+	if (!handle)
+		return -ENODEV;

drivers/tty/serdev/core.c static int of_serdev_register_devices(struct serdev_controller *ctrl) · line 464

Quoted 10 lines

drivers/tty/serdev/core.c +10 -0

+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+				     acpi_serdev_add_device, NULL, ctrl, NULL);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}

how are we ensuring that we only take UART devices into account here?

Quoted 2 lines

acpi_serdev_add_device() callback will only take into account entries without enumerated flag set.
This flags is set for all entries during ACPI scan, except for SPI and I2C serial devices, and for UART with 2nd patch in the series.

sounds good to me. Can you respin this series with the other comments addressed and maybe add some extra comments in the code or the commit message to make this clear.

Hi Fred,

Quoted 62 lines

Quoted 62 lines

Quoted 62 lines

Quoted 51 lines

drivers/tty/serdev/core.c +51 -0

+#ifdef CONFIG_ACPI
+static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
+					    struct acpi_device *adev)
+{
+	struct serdev_device *serdev = NULL;
+	int err;
+
+	if (acpi_bus_get_status(adev) || !adev->status.present ||
+	    acpi_device_enumerated(adev))
+		return AE_OK;
+
+	serdev = serdev_device_alloc(ctrl);
+	if (!serdev) {
+		dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
+			dev_name(&adev->dev));
+		return AE_NO_MEMORY;
+	}
+
+	ACPI_COMPANION_SET(&serdev->dev, adev);
+	acpi_device_set_enumerated(adev);
+
+	err = serdev_device_add(serdev);
+	if (err) {
+		dev_err(&serdev->dev,
+			"failure adding ACPI device. status %d\n", err);
+		serdev_device_put(serdev);
+	}
+
+	return AE_OK;
+}
+
+static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
+				       void *data, void **return_value)
+{
+	struct serdev_controller *ctrl = data;
+	struct acpi_device *adev;
+
+	if (acpi_bus_get_device(handle, &adev))
+		return AE_OK;
+
+	return acpi_serdev_register_device(ctrl, adev);
+}
+
+static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+	acpi_status status;
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(ctrl->dev.parent);
+	if (!handle)
+		return -ENODEV;

drivers/tty/serdev/core.c static int of_serdev_register_devices(struct serdev_controller *ctrl) · line 464

Quoted 10 lines

drivers/tty/serdev/core.c +10 -0

+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+				     acpi_serdev_add_device, NULL, ctrl, NULL);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}

how are we ensuring that we only take UART devices into account here?

Quoted 3 lines

Quoted 2 lines

acpi_serdev_add_device() callback will only take into account entries without enumerated flag set.
This flags is set for all entries during ACPI scan, except for SPI and I2C serial devices, and for UART with 2nd patch in the series.

sounds good to me. Can you respin this series with the other comments addressed and maybe add some extra comments in the code or the commit message to make this clear.

any updates on this?

UART devices is expected to be enumerated by SerDev subsystem. Rename spi_i2c_slave to serial_slave as this is no more specific to spi or i2c buses.

Signed-off-by Frédéric Danis <[email protected]>

---
drivers/acpi/scan.c | 29 ++++++++++++-----------------
include/acpi/acpi_bus.h | 2 +-

2 files changed, 13 insertions(+), 18 deletions(-)

drivers/acpi/scan.c +2 -4

index 70fd550..04c5ecc 100644
@@ -1429,35 +1429,30 @@ static void acpi_init_coherency(struct acpi_device *adev)
 	adev->flags.coherent_dma = cca;
 }
 
-static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
+static int acpi_check_serial_slave(struct acpi_resource *ares, void *data)
 {
-	bool *is_spi_i2c_slave_p = data;
+	bool *is_serial_slave_p = data;
 
 	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
 		return 1;
 
-	/*
-	 * devices that are connected to UART still need to be enumerated to

drivers/acpi/scan.c +9 -9

-static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
+static bool acpi_is_serial_slave(struct acpi_device *device)
 {
 	struct list_head resource_list;
-	bool is_spi_i2c_slave = false;
+	bool is_serial_slave = false;
 
 	INIT_LIST_HEAD(&resource_list);
-	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
-			       &is_spi_i2c_slave);
+	acpi_dev_get_resources(device, &resource_list, acpi_check_serial_slave,
+			       &is_serial_slave);
 	acpi_dev_free_resource_list(&resource_list);
 
-	return is_spi_i2c_slave;
+	return is_serial_slave;
 }
 
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,

@@ -1476,7 +1471,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	acpi_bus_get_flags(device);
 	device->flags.match_driver = false;
 	device->flags.initialized = true;
-	device->flags.spi_i2c_slave = acpi_is_spi_i2c_slave(device);
+	device->flags.serial_slave = acpi_is_serial_slave(device);
 	acpi_device_clear_enumerated(device);
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);

@@ -1763,7 +1758,7 @@ static void acpi_default_enumeration(struct acpi_device *device)
 	 * Do not enumerate SPI/I2C slaves as they will be enumerated by their
 	 * respective parents.
 	 */
-	if (!device->flags.spi_i2c_slave) {
+	if (!device->flags.serial_slave) {
 		acpi_create_platform_device(device, NULL);
 		acpi_device_set_enumerated(device);
 	} else {

@@ -1860,7 +1855,7 @@ static void acpi_bus_attach(struct acpi_device *device)
 		return;
 
 	device->flags.match_driver = true;
-	if (ret > 0 && !device->flags.spi_i2c_slave) {
+	if (ret > 0 && !device->flags.serial_slave) {
 		acpi_device_set_enumerated(device);
 		goto ok;
 	}

@@ -1869,7 +1864,7 @@ static void acpi_bus_attach(struct acpi_device *device)
 	if (ret < 0)
 		return;
 
-	if (!device->pnp.type.platform_id && !device->flags.spi_i2c_slave)
+	if (!device->pnp.type.platform_id && !device->flags.serial_slave)
 		acpi_device_set_enumerated(device);
 	else
 		acpi_default_enumeration(device);

include/acpi/acpi_bus.h +0 -0

index 68bc6be..49a82f8 100644

Hi Fred,

Quoted 35 lines

UART devices is expected to be enumerated by SerDev subsystem. Rename spi_i2c_slave to serial_slave as this is no more specific to spi or i2c buses.

Signed-off-by Frédéric Danis <[email protected]>

---
drivers/acpi/scan.c | 29 ++++++++++++-----------------
include/acpi/acpi_bus.h | 2 +-

2 files changed, 13 insertions(+), 18 deletions(-)

drivers/acpi/scan.c +3 -8

index 70fd550..04c5ecc 100644
@@ -1429,35 +1429,30 @@ static void acpi_init_coherency(struct acpi_device *adev)
 	adev->flags.coherent_dma = cca;
 }
 
-static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
+static int acpi_check_serial_slave(struct acpi_resource *ares, void *data)
 {
-	bool *is_spi_i2c_slave_p = data;
+	bool *is_serial_slave_p = data;
 
 	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
 		return 1;
 
-	/*
-	 * devices that are connected to UART still need to be enumerated to
-	 * platform bus
-	 */
-	if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)
-		*is_spi_i2c_slave_p = true;
+	*is_serial_slave_p = true;
 

drivers/acpi/scan.c static void acpi_init_coherency(struct acpi_device *adev) · line 1444

Quoted 4 lines

drivers/acpi/scan.c +0 -0

 	 /* no need to do more checking */
 	return -1;
 }
 

isn’t this disabled the I2C support? I mean serial bus doesn’t always mean UART. There are other serial buses and even USB is actually a serial bus.

Quoted 61 lines

drivers/acpi/scan.c +9 -9

-static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
+static bool acpi_is_serial_slave(struct acpi_device *device)
 {
 	struct list_head resource_list;
-	bool is_spi_i2c_slave = false;
+	bool is_serial_slave = false;
 
 	INIT_LIST_HEAD(&resource_list);
-	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
-			       &is_spi_i2c_slave);
+	acpi_dev_get_resources(device, &resource_list, acpi_check_serial_slave,
+			       &is_serial_slave);
 	acpi_dev_free_resource_list(&resource_list);
 
-	return is_spi_i2c_slave;
+	return is_serial_slave;
 }
 
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,

@@ -1476,7 +1471,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	acpi_bus_get_flags(device);
 	device->flags.match_driver = false;
 	device->flags.initialized = true;
-	device->flags.spi_i2c_slave = acpi_is_spi_i2c_slave(device);
+	device->flags.serial_slave = acpi_is_serial_slave(device);
 	acpi_device_clear_enumerated(device);
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);

@@ -1763,7 +1758,7 @@ static void acpi_default_enumeration(struct acpi_device *device)
 	 * Do not enumerate SPI/I2C slaves as they will be enumerated by their
 	 * respective parents.
 	 */
-	if (!device->flags.spi_i2c_slave) {
+	if (!device->flags.serial_slave) {
 		acpi_create_platform_device(device, NULL);
 		acpi_device_set_enumerated(device);
 	} else {

@@ -1860,7 +1855,7 @@ static void acpi_bus_attach(struct acpi_device *device)
 		return;
 
 	device->flags.match_driver = true;
-	if (ret > 0 && !device->flags.spi_i2c_slave) {
+	if (ret > 0 && !device->flags.serial_slave) {
 		acpi_device_set_enumerated(device);
 		goto ok;
 	}

@@ -1869,7 +1864,7 @@ static void acpi_bus_attach(struct acpi_device *device)
 	if (ret < 0)
 		return;
 
-	if (!device->pnp.type.platform_id && !device->flags.spi_i2c_slave)
+	if (!device->pnp.type.platform_id && !device->flags.serial_slave)
 		acpi_device_set_enumerated(device);
 	else
 		acpi_default_enumeration(device);

include/acpi/acpi_bus.h +0 -0

index 68bc6be..49a82f8 100644
@@ -211,7 +211,7 @@ struct acpi_device_flags {

include/acpi/acpi_bus.h struct acpi_device_flags { · line 216

Quoted 7 lines

include/acpi/acpi_bus.h +1 -1

 	u32 of_compatible_ok:1;
 	u32 coherent_dma:1;
 	u32 cca_seen:1;
-	u32 spi_i2c_slave:1;
+	u32 serial_slave:1;
 	u32 reserved:19;
 };

I am not an ACPI expert, but wouldn’t we better have a serial_bus_slave here. And the serial_bus_slave can be either UART or I2C? Or have a pretty good commit message explaining why this is serial_slave only.

Quoted 5 lines · Marcel Holtmann

Quoted 5 lines

include/acpi/acpi_bus.h +0 -0

@@ -211,7 +211,7 @@ struct acpi_device_flags {
 	u32 of_compatible_ok:1;
 	u32 coherent_dma:1;

Quoted 8 lines

include/acpi/acpi_bus.h struct acpi_device_flags { · line 216

Quoted 5 lines

include/acpi/acpi_bus.h +1 -1

 	u32 cca_seen:1;
-	u32 spi_i2c_slave:1;
+	u32 serial_slave:1;
 	u32 reserved:19;
 };

I am not an ACPI expert, but wouldn't we better have a serial_bus_slave here. And the serial_bus_slave can be either UART or I2C? Or have a pretty good commit message explaining why this is serial_slave only.

I will rename it.
serial_bus_slave can be either SPI, I2C or UART
(cf. http://elixir.free-electrons.com/linux/latest/source/include/acpi/acrestyp.h#L446).

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

include/acpi/acpi_bus.h +0 -0

@@ -211,7 +211,7 @@ struct acpi_device_flags {
 	u32 of_compatible_ok:1;
 	u32 coherent_dma:1;

Quoted 8 lines

include/acpi/acpi_bus.h struct acpi_device_flags { · line 216

Quoted 5 lines

include/acpi/acpi_bus.h +1 -1

 	u32 cca_seen:1;
-	u32 spi_i2c_slave:1;
+	u32 serial_slave:1;
 	u32 reserved:19;
 };

I am not an ACPI expert, but wouldn't we better have a serial_bus_slave here. And the serial_bus_slave can be either UART or I2C? Or have a pretty good commit message explaining why this is serial_slave only.

Quoted 3 lines

I will rename it.
serial_bus_slave can be either SPI, I2C or UART
(cf. http://elixir.free-electrons.com/linux/latest/source/include/acpi/acrestyp.h#L446).

Another idea would be to describe the *effect*, rather than the devices affected, i.e. something like:

(quoted diff) +1 -1

-	u32 spi_i2c_slave:1;
+	u32 parent_enumerated:1;

such that it could be extended to slaves on a non-serial bus in the future.

drivers/acpi/scan.c +2 -2

-static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
+static bool acpi_is_serial_slave(struct acpi_device *device)
 {
 	struct list_head resource_list;
-	bool is_spi_i2c_slave = false;
+	bool is_serial_slave = false;
 

Signed-off-by Frédéric Danis <[email protected]>

---
drivers/bluetooth/hci_bcm.c | 10 +++++++++-

1 file changed, 9 insertions(+), 1 deletion(-)

drivers/bluetooth/hci_bcm.c +8 -1

index 2e358cc..b1cf07e 100644
@@ -922,7 +922,6 @@ 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 },

@@ -942,6 +941,14 @@ static const struct acpi_device_id bcm_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
 #endif
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id bcm_serdev_acpi_match[] = {
+	{ "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, bcm_serdev_acpi_match);
+#endif
+
 /* Platform suspend and resume callbacks */
 static const struct dev_pm_ops bcm_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)

Signed-off-by Frédéric Danis <[email protected]>

---
  drivers/bluetooth/hci_bcm.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)

drivers/bluetooth/hci_bcm.c +9 -1

index 2e358cc..b1cf07e 100644
@@ -922,7 +922,6 @@ 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 },

@@ -942,6 +941,14 @@ static const struct acpi_device_id bcm_acpi_match[] = {
  MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
  #endif
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id bcm_serdev_acpi_match[] = {
+	{ "BCM2E39", (kernel_ulong_t)&acpi_bcm_int_last_gpios },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, bcm_serdev_acpi_match);
+#endif
+
  /* Platform suspend and resume callbacks */
  static const struct dev_pm_ops bcm_pm_ops = {
  	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)

@@ -999,6 +1006,7 @@ static struct serdev_device_driver bcm_serdev_driver = {
  	.driver = {
  		.name = "hci_uart_bcm",
  		.of_match_table = of_match_ptr(bcm_bluetooth_of_match),
+		.acpi_match_table = ACPI_PTR(bcm_serdev_acpi_match),
  	},
  };
 

From ce62881000fc076326220356036b65074c385a5e Mon Sep 17 00:00:00 2001

Hi,

First of all Frédéric, thank you very much for working on this, this is a very much needed patch series.

I've been testing this on a GPD pocket which has a BCM2E7E ACPI device for the BT which is part of the BCM43562A wifi/bt combo on this device.

2 remarks:

1) The following errors show up with this series:

[   87.059091] synth uevent: /devices/pci0000:00/8086228A:00/serial1: failed to send uevent [   87.059097] serial serial1: uevent: failed to send synthetic uevent
[   87.059178] synth uevent: /devices/pci0000:00/8086228A:01/serial2: failed to send uevent [   87.059180] serial serial2: uevent: failed to send synthetic uevent
[   87.062665] synth uevent: /devices/pnp0/00:01/serial0: failed to send uevent [   87.062671] serial serial0: uevent: failed to send synthetic uevent

This needs to be fixed :)  I haven't looked into this yet.

2) As I already suspected when reading the patches, we cannot move the ACPI ids over 1 by 1. The first 2 patches in the series cause all BCM2E* ACPI devices to no longer get enumerated as platform devices, instead they now get enumerated as serdevs.

So adding only the known to work ACPI ids to the acpi_match_table for the bcm_serdev_driver has the undesirable result that it makes sure that the other ACPI-ids will not work, as there are not platform devices instantiated for the platform driver to bind to.

I started with simplifying your patch to this:

From f8728f5440c85f7e5584ac1eafa1b090b2042276 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Danis?= <[email protected]>
Date: Thu, 7 Sep 2017 14:10:14 +0200
Subject: [PATCH] Bluetooth: hci_bcm: Make ACPI enumerated HCIs use serdev
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Now that the ACPI subsys instantiates ACPI enumerated HCIs as serdevs, the ACPI ids for them should be part of the bcm_serdev_driver's acpi_match_table.

Signed-off-by Frédéric Danis <[email protected]>

[hdegoede: Move all ACPI ids over]

Signed-off-by Hans de Goede <[email protected]>

---
   drivers/bluetooth/hci_bcm.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

drivers/bluetooth/hci_bcm.c +1 -1

index 2285ca673ae3..4db777bb0049 100644
@@ -942,7 +942,6 @@ static struct platform_driver bcm_driver = {
       .remove = bcm_remove,
       .driver = {
           .name = "hci_bcm",
-        .acpi_match_table = ACPI_PTR(bcm_acpi_match),
           .pm = &bcm_pm_ops,
       },
   };

@@ -988,6 +987,7 @@ static struct serdev_device_driver bcm_serdev_driver = {
       .driver = {
           .name = "hci_uart_bcm",
           .of_match_table = of_match_ptr(bcm_bluetooth_of_match),
+        .acpi_match_table = ACPI_PTR(bcm_acpi_match),
       },
   };
 

Signature · 20 lines

2.14.2

Unfortunately that is not good enough. The platform driver code also
has (runtime) pm support using gpios and a host-wake interrupt, which
the serdev code all lacks and without driving the shutdown gpio to not
shutdown the BT device connected to the UART we not only are lacking
pm support, but BT does not work at all.

So yesterday evening I did a patch to merge all that into the serdev hci_bcm
code and throw away the platform code as I don't think any users will be left
after this. With this functionality for the BCM2E7E ACPI device is restored
again, without needing to do a btattach from userspace, which is great :)

I've attached the resulting patch. In the mean time I've realized that
this approach is going to make merging hard. So I'm going to redo the changes
so that we can have both a complete (with runtime-pm, gpios, etc.) serdev
driver and keep the platform driver for now. Then we can first merge the
hci_bcm changes to make the serdev driver side complete and once those are
merged merge the matching ACPI subsys changes without having a window in
between where things will not work.