| Message ID | 20241109220530.83394-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: x86-android-tablets: Add Bluetooth support for Vexia EDU ATLA 10 | expand |
On Sun, Nov 10, 2024 at 12:05 AM Hans de Goede <hdegoede@redhat.com> wrote: > > 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. ... > + if (dev_info->use_pci) > + ctrl_dev = get_serdev_controller_by_pci_parent(info); > + else > + ctrl_dev = get_serdev_controller(info->ctrl_hid, info->ctrl_uid, 0, > + info->ctrl_devname); I would expect that they both take info as an argument... > if (IS_ERR(ctrl_dev)) > return PTR_ERR(ctrl_dev); ... > struct x86_serdev_info { > + /* For ACPI enumerated controllers */ > const char *ctrl_hid; > const char *ctrl_uid; > + /* For PCI enumerated controllers */ > + unsigned int ctrl_devfn; > + /* Typically "serial0" */ > const char *ctrl_devname; Why not union as we have a type selector, i.e. use_pci ?
Hi, On 10-Nov-24 12:51 PM, Andy Shevchenko wrote: > On Sun, Nov 10, 2024 at 12:05 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> 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. > > ... > >> + if (dev_info->use_pci) >> + ctrl_dev = get_serdev_controller_by_pci_parent(info); >> + else >> + ctrl_dev = get_serdev_controller(info->ctrl_hid, info->ctrl_uid, 0, >> + info->ctrl_devname); > > I would expect that they both take info as an argument... The "old" get_serdev_controller() helper for getting the controller by ACPI HID + UID is also used outside of the x86-android-tablets code and struct x86_serdev_info is x86-android-tablets specific. >> if (IS_ERR(ctrl_dev)) >> return PTR_ERR(ctrl_dev); > > ... > >> struct x86_serdev_info { >> + /* For ACPI enumerated controllers */ >> const char *ctrl_hid; >> const char *ctrl_uid; >> + /* For PCI enumerated controllers */ >> + unsigned int ctrl_devfn; >> + /* Typically "serial0" */ >> const char *ctrl_devname; > > Why not union as we have a type selector, i.e. use_pci ? A union would be possible. I simply did not think of that. Not sure if it is worth the trouble though, since it saves only 8 bytes on a struct which is currently used only 3 times in the driver. Regards, Hans
On Sun, Nov 10, 2024 at 11:27:31PM +0100, Hans de Goede wrote: > On 10-Nov-24 12:51 PM, Andy Shevchenko wrote: > > On Sun, Nov 10, 2024 at 12:05 AM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> + if (dev_info->use_pci) > >> + ctrl_dev = get_serdev_controller_by_pci_parent(info); > >> + else > >> + ctrl_dev = get_serdev_controller(info->ctrl_hid, info->ctrl_uid, 0, > >> + info->ctrl_devname); > > > > I would expect that they both take info as an argument... > > The "old" get_serdev_controller() helper for getting the controller > by ACPI HID + UID is also used outside of the x86-android-tablets code > and struct x86_serdev_info is x86-android-tablets specific. I see, thanks for elaborating this. > >> if (IS_ERR(ctrl_dev)) > >> return PTR_ERR(ctrl_dev); ... > >> struct x86_serdev_info { > >> + /* For ACPI enumerated controllers */ > >> const char *ctrl_hid; > >> const char *ctrl_uid; > >> + /* For PCI enumerated controllers */ > >> + unsigned int ctrl_devfn; > >> + /* Typically "serial0" */ > >> const char *ctrl_devname; > > > > Why not union as we have a type selector, i.e. use_pci ? > > A union would be possible. I simply did not think of that. > > Not sure if it is worth the trouble though, since it saves > only 8 bytes on a struct which is currently used only 3 > times in the driver. Saving bytes is not the main point, the main point is to make the code robust against writing both fields and make things confusing. Along with that, union gives a reader the clue that those fields are never used at the same time. So, can you consider using union?
Hi, On 11-Nov-24 10:37 AM, Andy Shevchenko wrote: > On Sun, Nov 10, 2024 at 11:27:31PM +0100, Hans de Goede wrote: >> On 10-Nov-24 12:51 PM, Andy Shevchenko wrote: >>> On Sun, Nov 10, 2024 at 12:05 AM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>>> + if (dev_info->use_pci) >>>> + ctrl_dev = get_serdev_controller_by_pci_parent(info); >>>> + else >>>> + ctrl_dev = get_serdev_controller(info->ctrl_hid, info->ctrl_uid, 0, >>>> + info->ctrl_devname); >>> >>> I would expect that they both take info as an argument... >> >> The "old" get_serdev_controller() helper for getting the controller >> by ACPI HID + UID is also used outside of the x86-android-tablets code >> and struct x86_serdev_info is x86-android-tablets specific. > > I see, thanks for elaborating this. > >>>> if (IS_ERR(ctrl_dev)) >>>> return PTR_ERR(ctrl_dev); > > ... > >>>> struct x86_serdev_info { >>>> + /* For ACPI enumerated controllers */ >>>> const char *ctrl_hid; >>>> const char *ctrl_uid; >>>> + /* For PCI enumerated controllers */ >>>> + unsigned int ctrl_devfn; >>>> + /* Typically "serial0" */ >>>> const char *ctrl_devname; >>> >>> Why not union as we have a type selector, i.e. use_pci ? >> >> A union would be possible. I simply did not think of that. >> >> Not sure if it is worth the trouble though, since it saves >> only 8 bytes on a struct which is currently used only 3 >> times in the driver. > > Saving bytes is not the main point, the main point is to make the code robust > against writing both fields and make things confusing. Along with that, union > gives a reader the clue that those fields are never used at the same time. So, > can you consider using union? Ack, I will prepare a v2 using an union when I have some time to work on this. Regards, Hans
diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c index 800d6c84dced..4633a9560bd2 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_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_hid, info->ctrl_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_hid, info->ctrl_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 0fc7e8cff672..bab27fdcd873 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 { + /* For ACPI enumerated controllers */ const char *ctrl_hid; const char *ctrl_uid; + /* For PCI enumerated controllers */ + unsigned int ctrl_devfn; + /* Typically "serial0" */ const char *ctrl_devname; /* * ATM the serdev core only supports of or ACPI matching; and so far all @@ -91,7 +95,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,
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../platform/x86/x86-android-tablets/core.c | 22 ++++++++++++++++--- .../platform/x86/x86-android-tablets/other.c | 2 +- .../x86-android-tablets/x86-android-tablets.h | 6 ++++- 3 files changed, 25 insertions(+), 5 deletions(-)