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@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> 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);
> +}
>
