[+Cc AceLan] Hi Hans, On Wed, Apr 24, 2024 at 5:58 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi Kai-Heng Feng, > > On 4/24/24 10:04 AM, Kai-Heng Feng wrote: > > 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! > > I hope you have access to actual hw with such a backlight device ? > > The driver actually has been ready for testing for quite a while now, > but the person who reported this backlight controller not being > supported to me has been testing this on a AIO of a friend of theirs > and this has been going pretty slow. > > So if you can test the driver (attached) then that would be great :) > > I even wrote an emulator to test it locally and that works, so > assuming I got the protocol right from the original posting of > the driver for this years ago then things should work. > > Note this depends on the kernel also having the patches from this > RFC (which Rafael has already merged) applied. There are newer AIO have UID other than 0, like "SIOBUAR2". Once change the "0" to NULL in 'get_serdev_controller("DELL0501", "0", 0, "serial0");', everything works perfectly. With that change, Tested-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> Kai-Heng > > Regards, > > Hans > > > > > > > >> 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 > >> > >
