[PATCH v2 0/6] platform/x86: x86-android-tablets: Add Bluetooth support for Vexia EDU ATLA 10

[PATCH v2 0/6] platform/x86: x86-android-tablets: Add Bluetooth support for Vexia EDU ATLA 10

[Hans de Goede]

Hi All,

Here is a patch-series for adding Bluetooth support for the Vexia EDU ATLA
10 tablet to x86-android-tablets.

Due to the LPSS UARTs being enumerated through PCI rather than through
ACPI, this is somewhat involved. Just like how this special case needed
some extra work for instantiating the various i2c-clients.

Changes in v2:
- Use a union in struct x86_serdev_info which stores either ACPI HID + UID
  or the PCI devfn of the serdev-controller

Regards,

Hans


Hans de Goede (6):
  platform/x86: serdev_helpers: Add get_serdev_controller_from_parent()
    helper
  platform/x86: x86-android-tablets: Add missing __init to
    get_i2c_adap_by_*()
  platform/x86: x86-android-tablets: Change x86_instantiate_serdev()
    prototype
  platform/x86: x86-android-tablets: Store serdev-controller ACPI HID +
    UID in a union
  platform/x86: x86-android-tablets: Add support for getting
    serdev-controller by PCI parent
  platform/x86: x86-android-tablets: Add Bluetooth support for Vexia EDU
    ATLA 10

 drivers/platform/x86/serdev_helpers.h         | 60 +++++++++++--------
 .../platform/x86/x86-android-tablets/asus.c   |  4 +-
 .../platform/x86/x86-android-tablets/core.c   | 31 +++++++---
 .../platform/x86/x86-android-tablets/lenovo.c |  4 +-
 .../platform/x86/x86-android-tablets/other.c  | 12 +++-
 .../x86-android-tablets/x86-android-tablets.h | 13 +++-
 6 files changed, 83 insertions(+), 41 deletions(-)

-- 
2.47.0

[PATCH v2 1/6] platform/x86: serdev_helpers: Add get_serdev_controller_from_parent() helper

[Hans de Goede]

The x86-android-tablets code needs to be able to get a serdev_controller
device from a PCI parent, rather then by the ACPI HID+UID of the parent,
because on some tablets the UARTs are enumerated as PCI devices instead
of ACPI devices.

Split the code to walk the device hierarchy to find the serdev_controller
from its parents out into a get_serdev_controller_from_parent() helper
so that the x86-android-tablets code can re-use it.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/serdev_helpers.h | 60 +++++++++++++++------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/serdev_helpers.h b/drivers/platform/x86/serdev_helpers.h
index bcf3a0c356ea..b592b9ff6d93 100644
--- a/drivers/platform/x86/serdev_helpers.h
+++ b/drivers/platform/x86/serdev_helpers.h
@@ -22,32 +22,14 @@
 #include <linux/string.h>
 
 static inline struct device *
-get_serdev_controller(const char *serial_ctrl_hid,
-		      const char *serial_ctrl_uid,
-		      int serial_ctrl_port,
-		      const char *serdev_ctrl_name)
+get_serdev_controller_from_parent(struct device *ctrl_dev,
+				  int serial_ctrl_port,
+				  const char *serdev_ctrl_name)
 {
-	struct device *ctrl_dev, *child;
-	struct acpi_device *ctrl_adev;
+	struct device *child;
 	char name[32];
 	int i;
 
-	ctrl_adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
-	if (!ctrl_adev) {
-		pr_err("error could not get %s/%s serial-ctrl adev\n",
-		       serial_ctrl_hid, serial_ctrl_uid);
-		return ERR_PTR(-ENODEV);
-	}
-
-	/* get_first_physical_node() returns a weak ref */
-	ctrl_dev = get_device(acpi_get_first_physical_node(ctrl_adev));
-	if (!ctrl_dev) {
-		pr_err("error could not get %s/%s serial-ctrl physical node\n",
-		       serial_ctrl_hid, serial_ctrl_uid);
-		ctrl_dev = ERR_PTR(-ENODEV);
-		goto put_ctrl_adev;
-	}
-
 	/* Walk host -> uart-ctrl -> port -> serdev-ctrl */
 	for (i = 0; i < 3; i++) {
 		switch (i) {
@@ -67,14 +49,40 @@ get_serdev_controller(const char *serial_ctrl_hid,
 		put_device(ctrl_dev);
 		if (!child) {
 			pr_err("error could not find '%s' device\n", name);
-			ctrl_dev = ERR_PTR(-ENODEV);
-			goto put_ctrl_adev;
+			return ERR_PTR(-ENODEV);
 		}
 
 		ctrl_dev = child;
 	}
 
-put_ctrl_adev:
-	acpi_dev_put(ctrl_adev);
 	return ctrl_dev;
 }
+
+static inline struct device *
+get_serdev_controller(const char *serial_ctrl_hid,
+		      const char *serial_ctrl_uid,
+		      int serial_ctrl_port,
+		      const char *serdev_ctrl_name)
+{
+	struct acpi_device *adev;
+	struct device *parent;
+
+	adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
+	if (!adev) {
+		pr_err("error could not get %s/%s serial-ctrl adev\n",
+		       serial_ctrl_hid, serial_ctrl_uid);
+		return ERR_PTR(-ENODEV);
+	}
+
+	/* get_first_physical_node() returns a weak ref */
+	parent = get_device(acpi_get_first_physical_node(adev));
+	acpi_dev_put(adev);
+	if (!parent) {
+		pr_err("error could not get %s/%s serial-ctrl physical node\n",
+		       serial_ctrl_hid, serial_ctrl_uid);
+		return ERR_PTR(-ENODEV);
+	}
+
+	/* This puts our reference on parent and returns a ref on the ctrl */
+	return get_serdev_controller_from_parent(parent, serial_ctrl_port, serdev_ctrl_name);
+}
-- 
2.47.0

[PATCH v2 2/6] platform/x86: x86-android-tablets: Add missing __init to get_i2c_adap_by_*()

[Hans de Goede]

get_i2c_adap_by_handle() and get_i2c_adap_by_pci_parent() both are only
used by x86_instantiate_i2c_client() which is __init itself and in case
of the latter it also uses match_parent() which is also __init.

Fixes: 5b78e809f948 ("platform/x86: x86-android-tablets: Add support for getting i2c_adapter by PCI parent devname()")
Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/x86-android-tablets/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
index e9f4d91496d5..affaffadd179 100644
--- a/drivers/platform/x86/x86-android-tablets/core.c
+++ b/drivers/platform/x86/x86-android-tablets/core.c
@@ -157,7 +157,7 @@ static struct gpiod_lookup_table * const *gpiod_lookup_tables;
 static const struct software_node *bat_swnode;
 static void (*exit_handler)(void);
 
-static struct i2c_adapter *
+static __init struct i2c_adapter *
 get_i2c_adap_by_handle(const struct x86_i2c_client_info *client_info)
 {
 	acpi_handle handle;
@@ -177,7 +177,7 @@ static __init int match_parent(struct device *dev, const void *data)
 	return dev->parent == data;
 }
 
-static struct i2c_adapter *
+static __init struct i2c_adapter *
 get_i2c_adap_by_pci_parent(const struct x86_i2c_client_info *client_info)
 {
 	struct i2c_adapter *adap = NULL;
-- 
2.47.0

[PATCH v2 3/6] platform/x86: x86-android-tablets: Change x86_instantiate_serdev() prototype

[Hans de Goede]

Make x86_instantiate_serdev() take a "struct x86_dev_info *" + idx as
arguments instead of a "struct x86_serdev_info *" + idx.

This makes the x86_instantiate_serdev() prototype match
the x86_instantiate_i2c_client() and x86_instantiate_spi_dev() prototypes.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/x86-android-tablets/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
index affaffadd179..800d6c84dced 100644
--- a/drivers/platform/x86/x86-android-tablets/core.c
+++ b/drivers/platform/x86/x86-android-tablets/core.c
@@ -271,8 +271,9 @@ static __init int x86_instantiate_spi_dev(const struct x86_dev_info *dev_info, i
 	return 0;
 }
 
-static __init int x86_instantiate_serdev(const struct x86_serdev_info *info, int idx)
+static __init int x86_instantiate_serdev(const struct x86_dev_info *dev_info, int idx)
 {
+	const struct x86_serdev_info *info = &dev_info->serdev_info[idx];
 	struct acpi_device *serdev_adev;
 	struct serdev_device *serdev;
 	struct device *ctrl_dev;
@@ -446,7 +447,7 @@ static __init int x86_android_tablet_probe(struct platform_device *pdev)
 
 	serdev_count = dev_info->serdev_count;
 	for (i = 0; i < serdev_count; i++) {
-		ret = x86_instantiate_serdev(&dev_info->serdev_info[i], i);
+		ret = x86_instantiate_serdev(dev_info, i);
 		if (ret < 0) {
 			x86_android_tablet_remove(pdev);
 			return ret;
-- 
2.47.0

[PATCH v2 4/6] platform/x86: x86-android-tablets: Store serdev-controller ACPI HID + UID in a union

[Hans de Goede]

Store the serdev-controller ACPI HID + UID in a union inside struct
x86_serdev_info.

This is a preparation patch for adding support for PCI enumerated serdev-
controllers which will be done by the devfn value of the PCI device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- This is a new patch in v2 of this patch-series
---
 drivers/platform/x86/x86-android-tablets/asus.c           | 4 ++--
 drivers/platform/x86/x86-android-tablets/core.c           | 2 +-
 drivers/platform/x86/x86-android-tablets/lenovo.c         | 4 ++--
 .../x86/x86-android-tablets/x86-android-tablets.h         | 8 ++++++--
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets/asus.c b/drivers/platform/x86/x86-android-tablets/asus.c
index 07fbeab2319a..7dde63b9943f 100644
--- a/drivers/platform/x86/x86-android-tablets/asus.c
+++ b/drivers/platform/x86/x86-android-tablets/asus.c
@@ -145,8 +145,8 @@ static const struct x86_i2c_client_info asus_me176c_i2c_clients[] __initconst =
 
 static const struct x86_serdev_info asus_me176c_serdevs[] __initconst = {
 	{
-		.ctrl_hid = "80860F0A",
-		.ctrl_uid = "2",
+		.ctrl.acpi.hid = "80860F0A",
+		.ctrl.acpi.uid = "2",
 		.ctrl_devname = "serial0",
 		.serdev_hid = "BCM2E3A",
 	},
diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
index 800d6c84dced..59909c53fca4 100644
--- a/drivers/platform/x86/x86-android-tablets/core.c
+++ b/drivers/platform/x86/x86-android-tablets/core.c
@@ -279,7 +279,7 @@ static __init int x86_instantiate_serdev(const struct x86_dev_info *dev_info, in
 	struct device *ctrl_dev;
 	int ret = -ENODEV;
 
-	ctrl_dev = get_serdev_controller(info->ctrl_hid, info->ctrl_uid, 0,
+	ctrl_dev = get_serdev_controller(info->ctrl.acpi.hid, info->ctrl.acpi.uid, 0,
 					 info->ctrl_devname);
 	if (IS_ERR(ctrl_dev))
 		return PTR_ERR(ctrl_dev);
diff --git a/drivers/platform/x86/x86-android-tablets/lenovo.c b/drivers/platform/x86/x86-android-tablets/lenovo.c
index ae087f1471c1..98085cb49c74 100644
--- a/drivers/platform/x86/x86-android-tablets/lenovo.c
+++ b/drivers/platform/x86/x86-android-tablets/lenovo.c
@@ -178,8 +178,8 @@ static const struct platform_device_info lenovo_yb1_x90_pdevs[] __initconst = {
  */
 static const struct x86_serdev_info lenovo_yb1_x90_serdevs[] __initconst = {
 	{
-		.ctrl_hid = "8086228A",
-		.ctrl_uid = "1",
+		.ctrl.acpi.hid = "8086228A",
+		.ctrl.acpi.uid = "1",
 		.ctrl_devname = "serial0",
 		.serdev_hid = "BCM2E1A",
 	},
diff --git a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
index 0fc7e8cff672..5ddec4beb552 100644
--- a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
+++ b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
@@ -57,8 +57,12 @@ struct x86_spi_dev_info {
 };
 
 struct x86_serdev_info {
-	const char *ctrl_hid;
-	const char *ctrl_uid;
+	union {
+		struct {
+			const char *hid;
+			const char *uid;
+		} acpi;
+	} ctrl;
 	const char *ctrl_devname;
 	/*
 	 * ATM the serdev core only supports of or ACPI matching; and so far all
-- 
2.47.0

[PATCH v2 5/6] platform/x86: x86-android-tablets: Add support for getting serdev-controller by PCI parent

[Hans de Goede]

On the Vexia EDU ATLA 10 tablet, which ships with Android + a custom Linux
(guadalinex) using the custom Android kernel the UART controllers are not
enumerated as ACPI devices as they typically are.

Instead they are enumerated through PCI and getting the serdev-controller
by ACPI HID + UID does not work.

Add support for getting the serdev-controller by the PCI devfn of its
parent instead.

This also renames the use_pci_devname flag to use_pci since the former
name now no longer is accurate.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Store PCI devfn in a union inside struct x86_dev_info
---
 .../platform/x86/x86-android-tablets/core.c   | 22 ++++++++++++++++---
 .../platform/x86/x86-android-tablets/other.c  |  2 +-
 .../x86-android-tablets/x86-android-tablets.h |  5 ++++-
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
index 59909c53fca4..379ba0916ad8 100644
--- a/drivers/platform/x86/x86-android-tablets/core.c
+++ b/drivers/platform/x86/x86-android-tablets/core.c
@@ -212,7 +212,7 @@ static __init int x86_instantiate_i2c_client(const struct x86_dev_info *dev_info
 	if (board_info.irq < 0)
 		return board_info.irq;
 
-	if (dev_info->use_pci_devname)
+	if (dev_info->use_pci)
 		adap = get_i2c_adap_by_pci_parent(client_info);
 	else
 		adap = get_i2c_adap_by_handle(client_info);
@@ -271,6 +271,19 @@ static __init int x86_instantiate_spi_dev(const struct x86_dev_info *dev_info, i
 	return 0;
 }
 
+static __init struct device *
+get_serdev_controller_by_pci_parent(const struct x86_serdev_info *info)
+{
+	struct pci_dev *pdev;
+
+	pdev = pci_get_domain_bus_and_slot(0, 0, info->ctrl.pci.devfn);
+	if (!pdev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	/* This puts our reference on pdev and returns a ref on the ctrl */
+	return get_serdev_controller_from_parent(&pdev->dev, 0, info->ctrl_devname);
+}
+
 static __init int x86_instantiate_serdev(const struct x86_dev_info *dev_info, int idx)
 {
 	const struct x86_serdev_info *info = &dev_info->serdev_info[idx];
@@ -279,8 +292,11 @@ static __init int x86_instantiate_serdev(const struct x86_dev_info *dev_info, in
 	struct device *ctrl_dev;
 	int ret = -ENODEV;
 
-	ctrl_dev = get_serdev_controller(info->ctrl.acpi.hid, info->ctrl.acpi.uid, 0,
-					 info->ctrl_devname);
+	if (dev_info->use_pci)
+		ctrl_dev = get_serdev_controller_by_pci_parent(info);
+	else
+		ctrl_dev = get_serdev_controller(info->ctrl.acpi.hid, info->ctrl.acpi.uid,
+						 0, info->ctrl_devname);
 	if (IS_ERR(ctrl_dev))
 		return PTR_ERR(ctrl_dev);
 
diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
index 725948044da4..f5140d5ce61a 100644
--- a/drivers/platform/x86/x86-android-tablets/other.c
+++ b/drivers/platform/x86/x86-android-tablets/other.c
@@ -757,7 +757,7 @@ const struct x86_dev_info vexia_edu_atla10_info __initconst = {
 	.i2c_client_count = ARRAY_SIZE(vexia_edu_atla10_i2c_clients),
 	.gpiod_lookup_tables = vexia_edu_atla10_gpios,
 	.init = vexia_edu_atla10_init,
-	.use_pci_devname = true,
+	.use_pci = true,
 };
 
 /*
diff --git a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
index 5ddec4beb552..63a38a0069ba 100644
--- a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
+++ b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
@@ -62,6 +62,9 @@ struct x86_serdev_info {
 			const char *hid;
 			const char *uid;
 		} acpi;
+		struct {
+			unsigned int devfn;
+		} pci;
 	} ctrl;
 	const char *ctrl_devname;
 	/*
@@ -95,7 +98,7 @@ struct x86_dev_info {
 	int gpio_button_count;
 	int (*init)(struct device *dev);
 	void (*exit)(void);
-	bool use_pci_devname;
+	bool use_pci;
 };
 
 int x86_android_tablet_get_gpiod(const char *chip, int pin, const char *con_id,
-- 
2.47.0

[PATCH v2 6/6] platform/x86: x86-android-tablets: Add Bluetooth support for Vexia EDU ATLA 10

[Hans de Goede]

The UART used for the Bluetooth HCI on the Vexia EDU ATLA 10 is enumerated
as a PCI device, but the ODBA7823 ACPI fwnode for the HCI expects it to
use the more standard ACPI enumeration mode.

So Bluetooth does not work out of the box. Add x86_serdev_info to make
the x86-android-tablets manually associate the fwnode with the UART.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/x86-android-tablets/other.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
index f5140d5ce61a..2157dd8c9aad 100644
--- a/drivers/platform/x86/x86-android-tablets/other.c
+++ b/drivers/platform/x86/x86-android-tablets/other.c
@@ -715,6 +715,14 @@ static const struct x86_i2c_client_info vexia_edu_atla10_i2c_clients[] __initcon
 	}
 };
 
+static const struct x86_serdev_info vexia_edu_atla10_serdevs[] __initconst = {
+	{
+		.ctrl.pci.devfn = PCI_DEVFN(0x1e, 3),
+		.ctrl_devname = "serial0",
+		.serdev_hid = "OBDA8723",
+	},
+};
+
 static struct gpiod_lookup_table vexia_edu_atla10_ft5416_gpios = {
 	.dev_id = "i2c-FTSC1000",
 	.table = {
@@ -755,6 +763,8 @@ static int __init vexia_edu_atla10_init(struct device *dev)
 const struct x86_dev_info vexia_edu_atla10_info __initconst = {
 	.i2c_client_info = vexia_edu_atla10_i2c_clients,
 	.i2c_client_count = ARRAY_SIZE(vexia_edu_atla10_i2c_clients),
+	.serdev_info = vexia_edu_atla10_serdevs,
+	.serdev_count = ARRAY_SIZE(vexia_edu_atla10_serdevs),
 	.gpiod_lookup_tables = vexia_edu_atla10_gpios,
 	.init = vexia_edu_atla10_init,
 	.use_pci = true,
-- 
2.47.0

Re: [PATCH v2 4/6] platform/x86: x86-android-tablets: Store serdev-controller ACPI HID + UID in a union

[Andy Shevchenko]

On Sat, Nov 16, 2024 at 5:35 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Store the serdev-controller ACPI HID + UID in a union inside struct
> x86_serdev_info.
>
> This is a preparation patch for adding support for PCI enumerated serdev-
> controllers which will be done by the devfn value of the PCI device.

LGTM,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] platform/x86: serdev_helpers: Add get_serdev_controller_from_parent() helper

[Ilpo Järvinen]

On Sat, 16 Nov 2024, Hans de Goede wrote:

> The x86-android-tablets code needs to be able to get a serdev_controller
> device from a PCI parent, rather then by the ACPI HID+UID of the parent,
> because on some tablets the UARTs are enumerated as PCI devices instead
> of ACPI devices.
> 
> Split the code to walk the device hierarchy to find the serdev_controller
> from its parents out into a get_serdev_controller_from_parent() helper
> so that the x86-android-tablets code can re-use it.
> 
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/serdev_helpers.h | 60 +++++++++++++++------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/serdev_helpers.h b/drivers/platform/x86/serdev_helpers.h
> index bcf3a0c356ea..b592b9ff6d93 100644
> --- a/drivers/platform/x86/serdev_helpers.h
> +++ b/drivers/platform/x86/serdev_helpers.h
> @@ -22,32 +22,14 @@
>  #include <linux/string.h>
>  
>  static inline struct device *
> -get_serdev_controller(const char *serial_ctrl_hid,
> -		      const char *serial_ctrl_uid,
> -		      int serial_ctrl_port,
> -		      const char *serdev_ctrl_name)
> +get_serdev_controller_from_parent(struct device *ctrl_dev,
> +				  int serial_ctrl_port,
> +				  const char *serdev_ctrl_name)
>  {
> -	struct device *ctrl_dev, *child;
> -	struct acpi_device *ctrl_adev;
> +	struct device *child;
>  	char name[32];
>  	int i;
>  
> -	ctrl_adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
> -	if (!ctrl_adev) {
> -		pr_err("error could not get %s/%s serial-ctrl adev\n",
> -		       serial_ctrl_hid, serial_ctrl_uid);
> -		return ERR_PTR(-ENODEV);
> -	}
> -
> -	/* get_first_physical_node() returns a weak ref */
> -	ctrl_dev = get_device(acpi_get_first_physical_node(ctrl_adev));
> -	if (!ctrl_dev) {
> -		pr_err("error could not get %s/%s serial-ctrl physical node\n",
> -		       serial_ctrl_hid, serial_ctrl_uid);
> -		ctrl_dev = ERR_PTR(-ENODEV);
> -		goto put_ctrl_adev;
> -	}
> -
>  	/* Walk host -> uart-ctrl -> port -> serdev-ctrl */
>  	for (i = 0; i < 3; i++) {
>  		switch (i) {
> @@ -67,14 +49,40 @@ get_serdev_controller(const char *serial_ctrl_hid,
>  		put_device(ctrl_dev);
>  		if (!child) {
>  			pr_err("error could not find '%s' device\n", name);
> -			ctrl_dev = ERR_PTR(-ENODEV);
> -			goto put_ctrl_adev;
> +			return ERR_PTR(-ENODEV);
>  		}
>  
>  		ctrl_dev = child;
>  	}
>  
> -put_ctrl_adev:
> -	acpi_dev_put(ctrl_adev);
>  	return ctrl_dev;
>  }
> +
> +static inline struct device *
> +get_serdev_controller(const char *serial_ctrl_hid,
> +		      const char *serial_ctrl_uid,
> +		      int serial_ctrl_port,
> +		      const char *serdev_ctrl_name)
> +{
> +	struct acpi_device *adev;
> +	struct device *parent;
> +
> +	adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
> +	if (!adev) {
> +		pr_err("error could not get %s/%s serial-ctrl adev\n",
> +		       serial_ctrl_hid, serial_ctrl_uid);

Hi,

With the current code (and I suppose this moved too), W=1 build detects 
that dell_uart_bl_pdev_probe() passed NULL which is then being formatted 
here with %s. While it "works", it would be useful to solve the warning 
and perhaps "/(null)" appearing in the print is also confusing to user 
so maybe do another patch to change serial_ctrl_uid to e.g.:

	serial_ctrl_uid ?: "*"

(There's another print below with the same problem).

--
 i.

> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	/* get_first_physical_node() returns a weak ref */
> +	parent = get_device(acpi_get_first_physical_node(adev));
> +	acpi_dev_put(adev);
> +	if (!parent) {
> +		pr_err("error could not get %s/%s serial-ctrl physical node\n",
> +		       serial_ctrl_hid, serial_ctrl_uid);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	/* This puts our reference on parent and returns a ref on the ctrl */
> +	return get_serdev_controller_from_parent(parent, serial_ctrl_port, serdev_ctrl_name);
> +}
> 

Re: [PATCH v2 1/6] platform/x86: serdev_helpers: Add get_serdev_controller_from_parent() helper

[Hans de Goede]

Hi,

On 2-Dec-24 5:34 PM, Ilpo Järvinen wrote:
> On Sat, 16 Nov 2024, Hans de Goede wrote:
> 
>> The x86-android-tablets code needs to be able to get a serdev_controller
>> device from a PCI parent, rather then by the ACPI HID+UID of the parent,
>> because on some tablets the UARTs are enumerated as PCI devices instead
>> of ACPI devices.
>>
>> Split the code to walk the device hierarchy to find the serdev_controller
>> from its parents out into a get_serdev_controller_from_parent() helper
>> so that the x86-android-tablets code can re-use it.
>>
>> Reviewed-by: Andy Shevchenko <andy@kernel.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/serdev_helpers.h | 60 +++++++++++++++------------
>>  1 file changed, 34 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/platform/x86/serdev_helpers.h b/drivers/platform/x86/serdev_helpers.h
>> index bcf3a0c356ea..b592b9ff6d93 100644
>> --- a/drivers/platform/x86/serdev_helpers.h
>> +++ b/drivers/platform/x86/serdev_helpers.h
>> @@ -22,32 +22,14 @@
>>  #include <linux/string.h>
>>  
>>  static inline struct device *
>> -get_serdev_controller(const char *serial_ctrl_hid,
>> -		      const char *serial_ctrl_uid,
>> -		      int serial_ctrl_port,
>> -		      const char *serdev_ctrl_name)
>> +get_serdev_controller_from_parent(struct device *ctrl_dev,
>> +				  int serial_ctrl_port,
>> +				  const char *serdev_ctrl_name)
>>  {
>> -	struct device *ctrl_dev, *child;
>> -	struct acpi_device *ctrl_adev;
>> +	struct device *child;
>>  	char name[32];
>>  	int i;
>>  
>> -	ctrl_adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
>> -	if (!ctrl_adev) {
>> -		pr_err("error could not get %s/%s serial-ctrl adev\n",
>> -		       serial_ctrl_hid, serial_ctrl_uid);
>> -		return ERR_PTR(-ENODEV);
>> -	}
>> -
>> -	/* get_first_physical_node() returns a weak ref */
>> -	ctrl_dev = get_device(acpi_get_first_physical_node(ctrl_adev));
>> -	if (!ctrl_dev) {
>> -		pr_err("error could not get %s/%s serial-ctrl physical node\n",
>> -		       serial_ctrl_hid, serial_ctrl_uid);
>> -		ctrl_dev = ERR_PTR(-ENODEV);
>> -		goto put_ctrl_adev;
>> -	}
>> -
>>  	/* Walk host -> uart-ctrl -> port -> serdev-ctrl */
>>  	for (i = 0; i < 3; i++) {
>>  		switch (i) {
>> @@ -67,14 +49,40 @@ get_serdev_controller(const char *serial_ctrl_hid,
>>  		put_device(ctrl_dev);
>>  		if (!child) {
>>  			pr_err("error could not find '%s' device\n", name);
>> -			ctrl_dev = ERR_PTR(-ENODEV);
>> -			goto put_ctrl_adev;
>> +			return ERR_PTR(-ENODEV);
>>  		}
>>  
>>  		ctrl_dev = child;
>>  	}
>>  
>> -put_ctrl_adev:
>> -	acpi_dev_put(ctrl_adev);
>>  	return ctrl_dev;
>>  }
>> +
>> +static inline struct device *
>> +get_serdev_controller(const char *serial_ctrl_hid,
>> +		      const char *serial_ctrl_uid,
>> +		      int serial_ctrl_port,
>> +		      const char *serdev_ctrl_name)
>> +{
>> +	struct acpi_device *adev;
>> +	struct device *parent;
>> +
>> +	adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
>> +	if (!adev) {
>> +		pr_err("error could not get %s/%s serial-ctrl adev\n",
>> +		       serial_ctrl_hid, serial_ctrl_uid);
> 
> Hi,
> 
> With the current code (and I suppose this moved too), W=1 build detects 
> that dell_uart_bl_pdev_probe() passed NULL which is then being formatted 
> here with %s. While it "works", it would be useful to solve the warning 
> and perhaps "/(null)" appearing in the print is also confusing to user 
> so maybe do another patch to change serial_ctrl_uid to e.g.:
> 
> 	serial_ctrl_uid ?: "*"
> 
> (There's another print below with the same problem).

Ack, for v3 I'll insert a new patch in the series before this patch
fixing this in the original code, including a Fixes: tag of the commit
introducing this.

Regards,

Hans




> 
> --
>  i.
> 
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	/* get_first_physical_node() returns a weak ref */
>> +	parent = get_device(acpi_get_first_physical_node(adev));
>> +	acpi_dev_put(adev);
>> +	if (!parent) {
>> +		pr_err("error could not get %s/%s serial-ctrl physical node\n",
>> +		       serial_ctrl_hid, serial_ctrl_uid);
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	/* This puts our reference on parent and returns a ref on the ctrl */
>> +	return get_serdev_controller_from_parent(parent, serial_ctrl_port, serdev_ctrl_name);
>> +}
>>
>