| Message ID | 20240218151533.5720-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Hans, On Sun, Feb 18, 2024 at 4:15 PM Hans de Goede <hdegoede@redhat.com> 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. OK > 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). > > If you have a different idea how to handle this I'm certainly open > to suggestions. I agree with the approach, thanks!
Hi Hans, On Sun, Feb 18, 2024 at 11:15 PM Hans de Goede <hdegoede@redhat.com> 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 >
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@redhat.com> 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. 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 >> >
[+Cc AceLan] Hi Hans, On Wed, Apr 24, 2024 at 5:58 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> 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@canonical.com> 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 > >> > >
On Wed, May 08, 2024 at 12:42:05PM +0800, Kai-Heng Feng wrote: > [+Cc AceLan] > On Wed, Apr 24, 2024 at 5:58 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 4/24/24 10:04 AM, Kai-Heng Feng wrote: > > > On Sun, Feb 18, 2024 at 11:15 PM Hans de Goede <hdegoede@redhat.com> 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@canonical.com> Do we have tables with _UID set to 0? If so, we would need more complex approach.
On Wed, May 8, 2024 at 5:44 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Wed, May 08, 2024 at 12:42:05PM +0800, Kai-Heng Feng wrote: > > [+Cc AceLan] > > On Wed, Apr 24, 2024 at 5:58 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > On 4/24/24 10:04 AM, Kai-Heng Feng wrote: > > > > On Sun, Feb 18, 2024 at 11:15 PM Hans de Goede <hdegoede@redhat.com> 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@canonical.com> > > Do we have tables with _UID set to 0? > If so, we would need more complex approach. Yes, some tables have _UID set to 0 and some have other _UID values. Kai-Heng > > -- > With Best Regards, > Andy Shevchenko > >
Hi Andy, On 5/8/24 11:43 AM, Andy Shevchenko wrote: > On Wed, May 08, 2024 at 12:42:05PM +0800, Kai-Heng Feng wrote: >> [+Cc AceLan] >> On Wed, Apr 24, 2024 at 5:58 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> On 4/24/24 10:04 AM, Kai-Heng Feng wrote: >>>> On Sun, Feb 18, 2024 at 11:15 PM Hans de Goede <hdegoede@redhat.com> 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@canonical.com> > > Do we have tables with _UID set to 0? > If so, we would need more complex approach. passing NULL as uid argument to get_serdev_controller() makes it skip the UID check altogether and there is no reaosn to expect the special "DELL0501" HID to be attached to more then 1 uart, so this should be fine. Regards, Hans
Hi Kai-Heng Feng, On 5/8/24 6:42 AM, Kai-Heng Feng wrote: > [+Cc AceLan] > > Hi Hans, > > On Wed, Apr 24, 2024 at 5:58 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> 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@canonical.com> Great thank you for testing. Luck has it that the user for who's Dell AOI I started working on this also just reported back that the driver works for them :) So I'm going to send out the patch series for this now with the following diff squashed in vs what I send you: diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c index da4a640c0d88..3882bb7d6c71 100644 --- a/drivers/platform/x86/dell/dell-uart-backlight.c +++ b/drivers/platform/x86/dell/dell-uart-backlight.c @@ -352,7 +352,7 @@ static int dell_uart_bl_pdev_probe(struct platform_device *pdev) struct device *ctrl_dev; int ret; - ctrl_dev = get_serdev_controller("DELL0501", "0", 0, "serial0"); + ctrl_dev = get_serdev_controller("DELL0501", NULL, 0, "serial0"); if (IS_ERR(ctrl_dev)) return PTR_ERR(ctrl_dev); Regards, Hans
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);
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). 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; 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(-)