Hi Hans, On Sun, Feb 18, 2024 at 11:15 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi Rafael, > > I recently learned that some Dell AIOs (1) use a backlight controller board > connected to an UART. Canonical even submitted a driver for this in 2017: > https://lkml.org/lkml/2017/10/26/78 > > This UART has a DELL0501 HID with CID set to PNP0501 so that the UART is > still handled by 8250_pnp.c. Unfortunately there is no separate ACPI device > with an UartSerialBusV2() resource to model the backlight-controller. > > The RFC patch 2/2 in this series uses acpi_quirk_skip_serdev_enumeration() > to still create a serdev for this for a backlight driver to bind to > instead of creating a /dev/ttyS0. > > Like other cases where the UartSerialBusV2() resource is missing or broken > this will only create the serdev-controller device and the serdev-device > itself will need to be instantiated by the consumer (the backlight driver). > > Unlike existing other cases which use DMI modaliases to load on a specific > board to work around brokeness of that board's specific ACPI tables, the > intend here is to have a single driver for all Dell AIOs using the DELL0501 > HID for their UART, without needing to maintain a list of DMI matches. > > This means that the dell-uart-backlight driver will need something to bind > to. The original driver from 2017 used an acpi_driver for this matching on > and binding to the DELL0501 acpi_device. > > AFAIK you are trying to get rid of having drivers bind directly to > acpi_device-s so I assume that you don't want me to introduce a new one. > So to get a device to bind to without introducing a new acpi_driver > patch 2/2 if this series creates a platform_device for this. > > The creation of this platform_device is why this is marked as RFC, > if you are ok with this solution I guess you can merge this series > already as is. With the caveat that the matching dell-uart-backlight > driver is still under development (its progressing nicely and the > serdev-device instantation + binding a serdev driver to it already > works). I was about to work on this and found you're already working on it. Please add me to Cc list when the driver is ready to be tested, thanks! Kai-Heng > > If you have a different idea how to handle this I'm certainly open > to suggestions. > > Regards, > > Hans > > 1) All In One a monitor with a PC builtin > > > p.s. > > I also tried this approach, but that did not work: > > This was an attempt to create both a pdev from acpi_default_enumeration() > by making the PNP scan handler attach() method return 0 rather then 1; > and get a pnp_device created for the UART driver as well by > making acpi_is_pnp_device() return true. > > This approach does not work due to the following code in pnpacpi_add_device(): > > /* Skip devices that are already bound */ > if (device->physical_node_count) > return 0; > > diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c > index 01abf26764b0..847c08deea7b 100644 > --- a/drivers/acpi/acpi_pnp.c > +++ b/drivers/acpi/acpi_pnp.c > @@ -353,10 +353,17 @@ static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matc > * given ACPI device object, the PNP scan handler will not attach to that > * object, because there is a proper non-PNP driver in the kernel for the > * device represented by it. > + * > + * The DELL0501 ACPI HID represents an UART (CID is set to PNP0501) with > + * a backlight-controller attached. There is no separate ACPI device with > + * an UartSerialBusV2() resource to model the backlight-controller. > + * This setup requires instantiating both a pnp_device for the UART as well > + * as a platform_device for the backlight-controller driver to bind too. > */ > static const struct acpi_device_id acpi_nonpnp_device_ids[] = { > {"INTC1080"}, > {"INTC1081"}, > + {"DELL0501"}, > {""}, > }; > > @@ -376,13 +383,16 @@ static struct acpi_scan_handler acpi_pnp_handler = { > * For CMOS RTC devices, the PNP ACPI scan handler does not work, because > * there is a CMOS RTC ACPI scan handler installed already, so we need to > * check those devices and enumerate them to the PNP bus directly. > + * For DELL0501 devices the PNP ACPI scan handler is skipped to create > + * a platform_device, see the acpi_nonpnp_device_ids[] comment. > */ > -static int is_cmos_rtc_device(struct acpi_device *adev) > +static int is_special_pnp_device(struct acpi_device *adev) > { > static const struct acpi_device_id ids[] = { > { "PNP0B00" }, > { "PNP0B01" }, > { "PNP0B02" }, > + { "DELL0501" }, > {""}, > }; > return !acpi_match_device_ids(adev, ids); > @@ -390,7 +400,7 @@ static int is_cmos_rtc_device(struct acpi_device *adev) > > bool acpi_is_pnp_device(struct acpi_device *adev) > { > - return adev->handler == &acpi_pnp_handler || is_cmos_rtc_device(adev); > + return adev->handler == &acpi_pnp_handler || is_special_pnp_device(adev); > } > EXPORT_SYMBOL_GPL(acpi_is_pnp_device); > > > Hans de Goede (2): > ACPI: x86: Move acpi_quirk_skip_serdev_enumeration() out of > CONFIG_X86_ANDROID_TABLETS > ACPI: x86: Add DELL0501 handling to > acpi_quirk_skip_serdev_enumeration() > > drivers/acpi/x86/utils.c | 38 ++++++++++++++++++++++++++++++++++---- > include/acpi/acpi_bus.h | 22 +++++++++++----------- > 2 files changed, 45 insertions(+), 15 deletions(-) > > -- > 2.43.0 >
