Re: [RFC 0/2] ACPI: Adding new acpi_driver type drivers ?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>





[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux