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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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);
> +}
>