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

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

[Hans de Goede]

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;

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

[RFC 1/2] ACPI: x86: Move acpi_quirk_skip_serdev_enumeration() out of CONFIG_X86_ANDROID_TABLETS

[Hans de Goede]

Some recent(ish) Dell AIO devices have a backlight controller board
connected to an UART.

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 next patch in this series will use 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.

This new acpi_quirk_skip_serdev_enumeration() use is not limited to Android
X86 tablets, so move it out of the ifdef CONFIG_X86_ANDROID_TABLETS block.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/x86/utils.c | 18 ++++++++++++++----
 include/acpi/acpi_bus.h  | 22 +++++++++++-----------
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index b37a4f44d05f..185176a521ad 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -437,7 +437,7 @@ bool acpi_quirk_skip_i2c_client_enumeration(struct acpi_device *adev)
 }
 EXPORT_SYMBOL_GPL(acpi_quirk_skip_i2c_client_enumeration);
 
-int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
+static int acpi_dmi_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
 {
 	struct acpi_device *adev = ACPI_COMPANION(controller_parent);
 	const struct dmi_system_id *dmi_id;
@@ -445,8 +445,6 @@ int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *s
 	u64 uid;
 	int ret;
 
-	*skip = false;
-
 	ret = acpi_dev_uid_to_integer(adev, &uid);
 	if (ret)
 		return 0;
@@ -472,7 +470,6 @@ int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *s
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(acpi_quirk_skip_serdev_enumeration);
 
 bool acpi_quirk_skip_gpio_event_handlers(void)
 {
@@ -487,8 +484,21 @@ bool acpi_quirk_skip_gpio_event_handlers(void)
 	return (quirks & ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS);
 }
 EXPORT_SYMBOL_GPL(acpi_quirk_skip_gpio_event_handlers);
+#else
+static int acpi_dmi_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
+{
+	return 0;
+}
 #endif
 
+int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
+{
+	*skip = false;
+
+	return acpi_dmi_skip_serdev_enumeration(controller_parent, skip);
+}
+EXPORT_SYMBOL_GPL(acpi_quirk_skip_serdev_enumeration);
+
 /* Lists of PMIC ACPI HIDs with an (often better) native charger driver */
 static const struct {
 	const char *hid;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index e4d24d3f9abb..446225aada50 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -749,6 +749,7 @@ bool acpi_device_override_status(struct acpi_device *adev, unsigned long long *s
 bool acpi_quirk_skip_acpi_ac_and_battery(void);
 int acpi_install_cmos_rtc_space_handler(acpi_handle handle);
 void acpi_remove_cmos_rtc_space_handler(acpi_handle handle);
+int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip);
 #else
 static inline bool acpi_device_override_status(struct acpi_device *adev,
 					       unsigned long long *status)
@@ -766,23 +767,22 @@ static inline int acpi_install_cmos_rtc_space_handler(acpi_handle handle)
 static inline void acpi_remove_cmos_rtc_space_handler(acpi_handle handle)
 {
 }
-#endif
-
-#if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)
-bool acpi_quirk_skip_i2c_client_enumeration(struct acpi_device *adev);
-int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip);
-bool acpi_quirk_skip_gpio_event_handlers(void);
-#else
-static inline bool acpi_quirk_skip_i2c_client_enumeration(struct acpi_device *adev)
-{
-	return false;
-}
 static inline int
 acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
 {
 	*skip = false;
 	return 0;
 }
+#endif
+
+#if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)
+bool acpi_quirk_skip_i2c_client_enumeration(struct acpi_device *adev);
+bool acpi_quirk_skip_gpio_event_handlers(void);
+#else
+static inline bool acpi_quirk_skip_i2c_client_enumeration(struct acpi_device *adev)
+{
+	return false;
+}
 static inline bool acpi_quirk_skip_gpio_event_handlers(void)
 {
 	return false;
-- 
2.43.0

Re: [RFC 1/2] ACPI: x86: Move acpi_quirk_skip_serdev_enumeration() out of CONFIG_X86_ANDROID_TABLETS

[Andy Shevchenko]

On Sun, Feb 18, 2024 at 5:15 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Some recent(ish) Dell AIO devices have a backlight controller board
> connected to an UART.
>
> 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 next patch in this series will use 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.
>
> This new acpi_quirk_skip_serdev_enumeration() use is not limited to Android
> X86 tablets, so move it out of the ifdef CONFIG_X86_ANDROID_TABLETS block.

...

> +#else
> +static int acpi_dmi_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
> +{
> +       return 0;
> +}
>  #endif

...

>  static inline int
>  acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
>  {
>         *skip = false;
>         return 0;
>  }
> +#endif

Now you have basically two identical blocks in two files. I believe
you may reorganize the code to have only one of these.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 1/2] ACPI: x86: Move acpi_quirk_skip_serdev_enumeration() out of CONFIG_X86_ANDROID_TABLETS

[Hans de Goede]

Hi,

On 2/18/24 19:39, Andy Shevchenko wrote:
> On Sun, Feb 18, 2024 at 5:15 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Some recent(ish) Dell AIO devices have a backlight controller board
>> connected to an UART.
>>
>> 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 next patch in this series will use 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.
>>
>> This new acpi_quirk_skip_serdev_enumeration() use is not limited to Android
>> X86 tablets, so move it out of the ifdef CONFIG_X86_ANDROID_TABLETS block.
> 
> ...
> 
>> +#else
>> +static int acpi_dmi_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
>> +{
>> +       return 0;
>> +}
>>  #endif
> 
> ...
> 
>>  static inline int
>>  acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
>>  {
>>         *skip = false;
>>         return 0;
>>  }
>> +#endif
> 
> Now you have basically two identical blocks in two files. I believe
> you may reorganize the code to have only one of these.

One is #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS) the other is
#ifdef CONFIG_X86

Also one is for a private helper, the other is for a public function.

The whole idea of this patch is actually to have both, with the
upcoming DELL0501 handling we want acpi_quirk_skip_serdev_enumeration()
to be defined (and not just an inline stub in the .h) independent
of IS_ENABLED(CONFIG_X86_ANDROID_TABLETS), the first #if block
provides a dummy for the x86-android specific bits for
the now always defined acpi_quirk_skip_serdev_enumeration() to call
when CONFIG_X86_ANDROID_TABLETS is not set.

The second #if is because acpi_quirk_skip_serdev_enumeration() still
is only defined on X86 platforms and on non x86 platforms we need
a stub for it in the public .h file.

Regards,

Hans

[RFC 2/2] ACPI: x86: Add DELL0501 handling to acpi_quirk_skip_serdev_enumeration()

[Hans de Goede]

Some recent(ish) Dell AIO devices have a backlight controller board
connected to an UART.

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.
This causes the kernel to create a /dev/ttyS0 char-device for the UART
instead of creating an in kernel serdev-controller + serdev-device pair
for a kernel backlight driver.

Use the existing acpi_quirk_skip_serdev_enumeration() mechanism to work
around this by returning skip=true for tty-ctrl parents with a HID
of DELL0501.

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 platform code.

Unfortunately in this case there is no device for the platform-code
instantiating the serdev-device to bind to. So also create
a platform_device for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/x86/utils.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index 185176a521ad..c7af2d2986fd 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -493,8 +493,28 @@ static int acpi_dmi_skip_serdev_enumeration(struct device *controller_parent, bo
 
 int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip)
 {
+	struct acpi_device *adev = ACPI_COMPANION(controller_parent);
+
 	*skip = false;
 
+	/*
+	 * 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.
+	 * Set skip to true so that the tty core creates a serdev ctrl device.
+	 * The backlight driver will manually create the serdev client device.
+	 */
+	if (acpi_dev_hid_match(adev, "DELL0501")) {
+		*skip = true;
+		/*
+		 * Create a platform dev for dell-uart-backlight to bind to.
+		 * This is a static device, so no need to store the result.
+		 */
+		platform_device_register_simple("dell-uart-backlight", PLATFORM_DEVID_NONE,
+						NULL, 0);
+		return 0;
+	}
+
 	return acpi_dmi_skip_serdev_enumeration(controller_parent, skip);
 }
 EXPORT_SYMBOL_GPL(acpi_quirk_skip_serdev_enumeration);
-- 
2.43.0

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

[Rafael J. Wysocki]

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!

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

[Kai-Heng Feng]

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
>

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

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 6439 bytes --]

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
>>
> 

[-- Attachment #2: 0001-platform-x86-Add-new-Dell-UART-backlight-driver.patch --]
[-- Type: text/x-patch, Size: 15243 bytes --]

From 5717af4e556f34d2a96bc33596b1c6866e3b09bc Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Sun, 18 Feb 2024 16:16:14 +0100
Subject: [PATCH 1/3] platform/x86: Add new Dell UART backlight driver

Dell All In One (AIO) models released after 2017 use a backlight controller
board connected to an UART.

In DSDT this uart port will be defined as:

   Name (_HID, "DELL0501")
   Name (_CID, EisaId ("PNP0501")

Instead of having a separate ACPI device with an UartSerialBusV2() resource
to model the backlight-controller, which would be the standard way to do
this.

The acpi_quirk_skip_serdev_enumeration() has special handling for this
and it will make the serial port code create a serdev controller device
for the UART instead of a /dev/ttyS0 char-dev. It will also create
a dell-uart-backlight driver platform device for this driver to bind too.

This new kernel module contains 2 drivers for this:

1. A simple platform driver which creates the actual serdev device
   (with the serdev controller device as parent)

2. A serdev driver for the created serdev device which exports
   the backlight functionality uses a standard backlight class device.

Co-developed-by: AceLan Kao <acelan.kao@canonical.com>
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell/Kconfig             |  15 +
 drivers/platform/x86/dell/Makefile            |   1 +
 .../platform/x86/dell/dell-uart-backlight.c   | 409 ++++++++++++++++++
 3 files changed, 425 insertions(+)
 create mode 100644 drivers/platform/x86/dell/dell-uart-backlight.c

diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index bd9f445974cc..195a8bf532cc 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -145,6 +145,21 @@ config DELL_SMO8800
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-smo8800.
 
+config DELL_UART_BACKLIGHT
+	tristate "Dell AIO UART Backlight driver"
+	depends on ACPI
+	depends on BACKLIGHT_CLASS_DEVICE
+	depends on SERIAL_DEV_BUS
+	help
+	  Say Y here if you want to support Dell AIO UART backlight interface.
+	  The Dell AIO machines released after 2017 come with a UART interface
+	  to communicate with the backlight scalar board. This driver creates
+	  a standard backlight interface and talks to the scalar board through
+	  UART to adjust the AIO screen brightness.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called dell_uart_backlight.
+
 config DELL_WMI
 	tristate "Dell WMI notifications"
 	default m
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index 1b8942426622..8176a257d9c3 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -14,6 +14,7 @@ dell-smbios-objs			:= dell-smbios-base.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_WMI)	+= dell-smbios-wmi.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_SMM)	+= dell-smbios-smm.o
 obj-$(CONFIG_DELL_SMO8800)		+= dell-smo8800.o
+obj-$(CONFIG_DELL_UART_BACKLIGHT)	+= dell-uart-backlight.o
 obj-$(CONFIG_DELL_WMI)			+= dell-wmi.o
 dell-wmi-objs				:= dell-wmi-base.o
 dell-wmi-$(CONFIG_DELL_WMI_PRIVACY)	+= dell-wmi-privacy.o
diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
new file mode 100644
index 000000000000..d43f5990d93f
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -0,0 +1,409 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Dell AIO Serial Backlight Driver
+ *
+ * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
+ * Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/serdev.h>
+#include <linux/wait.h>
+#include "../serdev_helpers.h"
+
+/* The backlight controller must respond within 1 second */
+#define DELL_BL_TIMEOUT		msecs_to_jiffies(1000)
+#define DELL_BL_MIN_RESP_SIZE	3
+
+struct dell_uart_backlight {
+	struct mutex mutex;
+	wait_queue_head_t wait_queue;
+	struct device *dev;
+	struct backlight_device *bl;
+	u8 *resp;
+	u8 resp_idx;
+	u8 resp_len;
+	u8 resp_max_len;
+	u8 pending_cmd;
+	int status;
+	int power;
+};
+
+/* Checksum: SUM(Length and Cmd and Data) xor 0xFF */
+static u8 dell_uart_checksum(u8 *buf, int len)
+{
+	u8 val = 0;
+
+	while (len-- > 0)
+		val += buf[len];
+
+	return val ^ 0xff;
+}
+
+static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
+				const u8 *cmd, int cmd_len,
+				u8 *resp, int resp_max_len)
+{
+	int ret;
+
+	ret = mutex_lock_killable(&dell_bl->mutex);
+	if (ret)
+		return ret;
+
+	dell_bl->status = 0;
+	dell_bl->resp = resp;
+	dell_bl->resp_idx = 0;
+	dell_bl->resp_max_len = resp_max_len;
+	dell_bl->pending_cmd = cmd[1];
+
+	/* The TTY buffer should be big enough to take the entire cmd in one go */
+	ret = serdev_device_write_buf(to_serdev_device(dell_bl->dev), cmd, cmd_len);
+	if (ret != cmd_len) {
+		dev_err(dell_bl->dev, "Error writing command: %d\n", ret);
+		ret = (ret < 0) ? ret : -EIO;
+		goto out;
+	}
+
+	ret = wait_event_timeout(dell_bl->wait_queue, dell_bl->status, DELL_BL_TIMEOUT);
+	if (ret == 0) {
+		dev_err(dell_bl->dev, "Timed out waiting for response.\n");
+		dell_bl->status = -ETIMEDOUT;
+	}
+
+	if (dell_bl->status == 1)
+		ret = 0;
+	else
+		ret = dell_bl->status;
+
+out:
+	mutex_unlock(&dell_bl->mutex);
+	return ret;
+}
+
+static int dell_uart_set_brightness(struct dell_uart_backlight *dell_bl, int brightness)
+{
+	/*
+	 * Set Brightness level: Application uses this command to set brightness.
+	 * Command: 0x8A 0x0B <brightness-level> Checksum (Length:4 Type:0x0A Cmd:0x0B)
+	 * 	    <brightness-level> ranges from 0~100.
+	 * Return data: 0x03 0x0B 0xF1 (Length:3 Cmd:0x0B Checksum:0xF1)
+	 */
+	u8 set_brightness[] = { 0x8A, 0x0B, 0x00, 0x00 };
+	u8 resp[3];
+
+	set_brightness[2] = brightness;
+	set_brightness[3] = dell_uart_checksum(set_brightness, 3);
+
+	return dell_uart_bl_command(dell_bl, set_brightness, ARRAY_SIZE(set_brightness),
+				    resp, ARRAY_SIZE(resp));
+}
+
+static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl)
+{
+	/*
+	 * Get Brightness level: Application uses this command to get brightness.
+	 * Command: 0x6A 0x0C 0x89 (Length:3 Type:0x0A Cmd:0x0C Checksum:0x89)
+	 * Return data: 0x04 0x0C Data Checksum
+	 *              (Length:4 Cmd:0x0C Data:<brightness level>
+	 *               Checksum: SUM(Length and Cmd and Data) xor 0xFF)
+	 *              <brightness level> ranges from 0~100.
+	 */
+	const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 };
+	u8 resp[4];
+	int ret;
+
+	ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness),
+				   resp, ARRAY_SIZE(resp));
+	if (ret)
+		return ret;
+
+	if (resp[0] != 4) {
+		dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n", resp[0]);
+		return -EIO;
+	}
+
+	if (resp[2] > 100) {
+		dev_err(dell_bl->dev, "Unexpected get brightness response: %d\n", resp[2]);
+		return -EIO;
+	}
+
+	return resp[2];
+}
+
+static int dell_uart_set_bl_power(struct dell_uart_backlight *dell_bl, int power)
+{
+	/*
+	 * Screen ON/OFF Control: Application uses this command to control screen ON or OFF.
+	 * Command: 0x8A 0x0E Data Checksum (Length:4 Type:0x0A Cmd:0x0E) where
+	 * 	    Data=0 to turn OFF the screen.
+	 * 	    Data=1 to turn ON the screen.
+	 * 	    Other value of Data is reserved and invalid.
+	 * Return data: 0x03 0x0E 0xEE (Length:3 Cmd:0x0E Checksum:0xEE)
+	 */
+	u8 set_power[] = { 0x8A, 0x0E, 0x00, 0x00 };
+	u8 resp[3];
+	int ret;
+
+	set_power[2] = (power == FB_BLANK_UNBLANK) ? 1 : 0;
+	set_power[3] = dell_uart_checksum(set_power, 3);
+
+	ret = dell_uart_bl_command(dell_bl, set_power, ARRAY_SIZE(set_power),
+				   resp, ARRAY_SIZE(resp));
+	if (ret)
+		return ret;
+
+	dell_bl->power = power;
+	return 0;
+}
+
+/*
+ * There is no command to get backlight power status,
+ * so we set the backlight power to "on" while initializing,
+ * and then track and report its status by power variable
+ */
+static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_bl)
+{
+	return dell_bl->power;
+}
+
+static int dell_uart_update_status(struct backlight_device *bd)
+{
+	struct dell_uart_backlight *dell_bl = bl_get_data(bd);
+	int ret;
+
+	ret = dell_uart_set_brightness(dell_bl, bd->props.brightness);
+	if (ret)
+		return ret;
+
+	if (bd->props.power != dell_uart_get_bl_power(dell_bl))
+		ret = dell_uart_set_bl_power(dell_bl, bd->props.power);
+
+	return ret;
+}
+
+static int dell_uart_get_brightness_op(struct backlight_device *bd)
+{
+	return dell_uart_get_brightness(bl_get_data(bd));
+}
+
+static const struct backlight_ops dell_uart_backlight_ops = {
+	.update_status = dell_uart_update_status,
+	.get_brightness = dell_uart_get_brightness_op,
+};
+
+static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len)
+{
+	struct dell_uart_backlight *dell_bl = serdev_device_get_drvdata(serdev);
+	size_t i;
+	u8 csum;
+
+	dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data);
+
+	/* Throw away unexpected bytes / remainder of response after an error */
+	if (dell_bl->status) {
+		dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n");
+		return len;
+	}
+
+	for (i = 0; i < len; i++) {
+		dell_bl->resp[dell_bl->resp_idx] = data[i];
+
+		switch (dell_bl->resp_idx) {
+		case 0: /* Length byte */
+			dell_bl->resp_len = dell_bl->resp[0];
+			if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) {
+				dev_err(dell_bl->dev, "Response length too small %d < %d\n",
+					dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE);
+				dell_bl->status = -EIO;
+				goto wakeup;
+			} else if (dell_bl->resp_len > dell_bl->resp_max_len) {
+				dev_err(dell_bl->dev, "Response length too big %d > %d\n",
+					dell_bl->resp_len, dell_bl->resp_max_len);
+				dell_bl->status = -EIO;
+				goto wakeup;
+			}
+			break;
+		case 1: /* CMD byte */
+			if (dell_bl->resp[1] != dell_bl->pending_cmd) {
+				dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
+					dell_bl->resp[1], dell_bl->pending_cmd);
+				dell_bl->status = -EIO;
+				goto wakeup;
+			}
+			break;
+		}
+
+		dell_bl->resp_idx++;
+		if (dell_bl->resp_idx < dell_bl->resp_len)
+			continue;
+
+		csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
+		if (dell_bl->resp[dell_bl->resp_len - 1] != csum) {
+			dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n",
+				dell_bl->resp[dell_bl->resp_len - 1], csum);
+			dell_bl->status = -EIO;
+			goto wakeup;
+		}
+
+		dell_bl->status = 1; /* Success */
+		goto wakeup;
+	}
+
+	return len;
+
+wakeup:
+	wake_up(&dell_bl->wait_queue);
+	return i + 1;
+}
+
+static const struct serdev_device_ops dell_uart_bl_serdev_ops = {
+	.receive_buf = dell_uart_bl_receive,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static int dell_uart_bl_serdev_probe(struct serdev_device *serdev)
+{
+	/*
+	 * Get Firmware Version: Tool uses this command to get firmware version.
+	 * Command: 0x6A 0x06 0x8F (Length:3 Type:0x0A Cmd:6 Checksum:0x8F)
+	 * Return data: 0x0D 0x06 Data Checksum (Length:13 Cmd:0x06
+	 * 	        Data:F/W version(APRILIA=APR27-Vxxx/PHINE=PHI23-Vxxx)
+	 * 	        Checksum:SUM(Length and Cmd and Data) xor 0xFF)
+	 */
+	const u8 get_firmware_ver[] = { 0x6A, 0x06, 0x8F };
+	struct dell_uart_backlight *dell_bl;
+	struct backlight_properties props;
+	struct device *dev = &serdev->dev;
+	u8 get_firmware_ver_resp[80];
+	int ret;
+
+	dell_bl = devm_kzalloc(dev, sizeof(*dell_bl), GFP_KERNEL);
+	if (!dell_bl)
+		return -ENOMEM;
+
+	mutex_init(&dell_bl->mutex);
+	init_waitqueue_head(&dell_bl->wait_queue);
+	dell_bl->dev = dev;
+
+	ret = devm_serdev_device_open(dev, serdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "opening UART device\n");
+
+	/* 9600 bps, no flow control, these are the default but set them to be sure */
+	serdev_device_set_baudrate(serdev, 9600);
+	serdev_device_set_flow_control(serdev, false);
+	serdev_device_set_drvdata(serdev, dell_bl);
+	serdev_device_set_client_ops(serdev, &dell_uart_bl_serdev_ops);
+
+	ret = dell_uart_bl_command(dell_bl, get_firmware_ver, ARRAY_SIZE(get_firmware_ver),
+				   get_firmware_ver_resp, ARRAY_SIZE(get_firmware_ver_resp));
+	if (ret)
+		return dev_err_probe(dev, ret, "getting firmware version\n");
+
+	dev_dbg(dev, "Firmware version: %.*s\n", get_firmware_ver_resp[0] - 3,
+		get_firmware_ver_resp + 2);
+
+	/* Initialize bl_power to a known value */
+	ret = dell_uart_set_bl_power(dell_bl, FB_BLANK_UNBLANK);
+	if (ret)
+		return ret;
+
+	ret = dell_uart_get_brightness(dell_bl);
+	if (ret < 0)
+		return ret;
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_PLATFORM;
+	props.brightness = ret;
+	props.max_brightness = 100;
+	props.power = dell_bl->power;
+
+	dell_bl->bl = devm_backlight_device_register(dev, "dell_uart_backlight",
+						     dev, dell_bl,
+						     &dell_uart_backlight_ops,
+						     &props);
+	if (IS_ERR(dell_bl->bl))
+		return PTR_ERR(dell_bl->bl);
+
+	return 0;
+}
+
+struct serdev_device_driver dell_uart_bl_serdev_driver = {
+	.probe = dell_uart_bl_serdev_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+static int dell_uart_bl_pdev_probe(struct platform_device *pdev)
+{
+	struct serdev_device *serdev;
+	struct device *ctrl_dev;
+	int ret;
+
+	ctrl_dev = get_serdev_controller("DELL0501", "0", 0, "serial0");
+	if (IS_ERR(ctrl_dev))
+		return PTR_ERR(ctrl_dev);
+
+	serdev = serdev_device_alloc(to_serdev_controller(ctrl_dev));
+	put_device(ctrl_dev);
+	if (!serdev)
+		return -ENOMEM;
+
+	ret = serdev_device_add(serdev);
+	if (ret) {
+		dev_err(&pdev->dev, "error %d adding serdev\n", ret);
+		serdev_device_put(serdev);
+		return ret;
+	}
+
+	ret = serdev_device_driver_register(&dell_uart_bl_serdev_driver);
+	if (ret) {
+		serdev_device_remove(serdev);
+		return ret;
+	}
+
+	/*
+	 * serdev device <-> driver matching relies on OF or ACPI matches and
+	 * neither is available here, manually bind the driver.
+	 */
+	ret = device_driver_attach(&dell_uart_bl_serdev_driver.driver, &serdev->dev);
+	if (ret) {
+		serdev_device_driver_unregister(&dell_uart_bl_serdev_driver);
+		serdev_device_remove(serdev);
+		return ret;
+	}
+
+	/* So that dell_uart_bl_pdev_remove() can remove the serdev */
+	platform_set_drvdata(pdev, serdev);
+	return 0;
+}
+
+static void dell_uart_bl_pdev_remove(struct platform_device *pdev)
+{
+	struct serdev_device *serdev = platform_get_drvdata(pdev);
+
+	serdev_device_driver_unregister(&dell_uart_bl_serdev_driver);
+	serdev_device_remove(serdev);
+}
+
+static struct platform_driver dell_uart_bl_pdev_driver = {
+	.probe = dell_uart_bl_pdev_probe,
+	.remove_new = dell_uart_bl_pdev_remove,
+	.driver = {
+		.name = "dell-uart-backlight",
+	},
+};
+module_platform_driver(dell_uart_bl_pdev_driver);
+
+MODULE_ALIAS("platform:dell-uart-backlight");
+MODULE_DESCRIPTION("Dell AIO Serial Backlight driver");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_LICENSE("GPL");
-- 
2.44.0


[-- Attachment #3: 0002-platform-x86-dell-uart-backlight-Add-ifdefs-to-make-.patch --]
[-- Type: text/x-patch, Size: 2173 bytes --]

From e91eb2c85ce633957b3406c2c07dfe048dbd4dd1 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 20 Feb 2024 22:09:42 +0100
Subject: [PATCH 2/3] platform/x86: dell-uart-backlight: Add ifdefs to make it
 build with kernel 6.7/6.8

Local modification, do not upstream. Make the serdev helpers and
dell-uart-backlight work with kernel 6.7/6.8 for testing purposes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell/dell-uart-backlight.c | 4 ++++
 drivers/platform/x86/serdev_helpers.h           | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
index d43f5990d93f..da4a640c0d88 100644
--- a/drivers/platform/x86/dell/dell-uart-backlight.c
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -198,7 +198,11 @@ static const struct backlight_ops dell_uart_backlight_ops = {
 	.get_brightness = dell_uart_get_brightness_op,
 };
 
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 9, 0)
 static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len)
+#else
+static ssize_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len)
+#endif
 {
 	struct dell_uart_backlight *dell_bl = serdev_device_get_drvdata(serdev);
 	size_t i;
diff --git a/drivers/platform/x86/serdev_helpers.h b/drivers/platform/x86/serdev_helpers.h
index bcf3a0c356ea..3d254926c9ba 100644
--- a/drivers/platform/x86/serdev_helpers.h
+++ b/drivers/platform/x86/serdev_helpers.h
@@ -20,6 +20,7 @@
 #include <linux/printk.h>
 #include <linux/sprintf.h>
 #include <linux/string.h>
+#include <linux/version.h>
 
 static inline struct device *
 get_serdev_controller(const char *serial_ctrl_hid,
@@ -49,7 +50,12 @@ get_serdev_controller(const char *serial_ctrl_hid,
 	}
 
 	/* Walk host -> uart-ctrl -> port -> serdev-ctrl */
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 8, 0)
 	for (i = 0; i < 3; i++) {
+#else
+	/* Kernels < 6.8 have host -> serdev-ctrl */
+	for (i = 2; i < 3; i++) {
+#endif
 		switch (i) {
 		case 0:
 			snprintf(name, sizeof(name), "%s:0", dev_name(ctrl_dev));
-- 
2.44.0


[-- Attachment #4: 0003-tools-arch-x86-Add-dell-uart-backlight-emulator.patch --]
[-- Type: text/x-patch, Size: 8847 bytes --]

From 9677480af27d6f1d3c981e62b86a2ec56f9afe11 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 19 Feb 2024 17:41:57 +0100
Subject: [PATCH 3/3] tools arch x86: Add dell-uart-backlight-emulator

Dell All In One (AIO) models released after 2017 use a backlight controller
board connected to an UART.

Add a small emulator to allow development and testing of
the drivers/platform/x86/dell/dell-uart-backlight.c driver for
this board, without requiring access to an actual Dell All In One.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../dell-uart-backlight-emulator/.gitignore   |   1 +
 .../x86/dell-uart-backlight-emulator/Makefile |  19 ++
 .../x86/dell-uart-backlight-emulator/README   |  46 +++++
 .../dell-uart-backlight-emulator.c            | 166 ++++++++++++++++++
 4 files changed, 232 insertions(+)
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/.gitignore
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/Makefile
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/README
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c

diff --git a/tools/arch/x86/dell-uart-backlight-emulator/.gitignore b/tools/arch/x86/dell-uart-backlight-emulator/.gitignore
new file mode 100644
index 000000000000..5c8cad8d72b9
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/.gitignore
@@ -0,0 +1 @@
+dell-uart-backlight-emulator
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/Makefile b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
new file mode 100644
index 000000000000..6ea1d9fd534b
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for Intel Software Defined Silicon provisioning tool
+
+dell-uart-backlight-emulator: dell-uart-backlight-emulator.c
+
+BINDIR ?= /usr/bin
+
+override CFLAGS += -O2 -Wall
+
+%: %.c
+	$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)
+
+.PHONY : clean
+clean :
+	@rm -f dell-uart-backlight-emulator
+
+install : dell-uart-backlight-emulator
+	install -d $(DESTDIR)$(BINDIR)
+	install -m 755 -p dell-uart-backlight-emulator $(DESTDIR)$(BINDIR)/dell-uart-backlight-emulator
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/README b/tools/arch/x86/dell-uart-backlight-emulator/README
new file mode 100644
index 000000000000..a532d000d34a
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/README
@@ -0,0 +1,46 @@
+Emulator for DELL0501 UART attached backlight controller
+--------------------------------------------------------
+
+Dell All In One (AIO) models released after 2017 use a backlight controller
+board connected to an UART.
+
+In DSDT this uart port will be defined as:
+
+   Name (_HID, "DELL0501")
+   Name (_CID, EisaId ("PNP0501")
+
+With the DELL0501 indicating that we are dealing with an UART with
+the backlight controller board attached.
+
+This small emulator allows testing
+the drivers/platform/x86/dell/dell-uart-backlight.c driver without access
+to an actual Dell All In One.
+
+This requires:
+1. A (desktop) PC with a 16550 UART on the motherboard and a standard DB9
+   connector connected to this UART.
+2. A DB9 NULL modem cable.
+3. A second DB9 serial port, this can e.g. be a USB to serial convertor
+   with a DB9 connector plugged into the same desktop PC.
+4. A DSDT overlay for the desktop PC replacing the _HID of the 16550 UART
+   ACPI Device() with "DELL0501" and adding a _CID of "PNP0501", see
+   DSDT.patch for an example of the necessary DSDT changes.
+
+With everything setup and the NULL modem cable connected between
+the 2 serial ports run:
+
+./dell-uart-backlight-emulator <path-to-/dev/tty*S#-for-second-port>
+
+For example when using an USB to serial convertor for the second port:
+
+./dell-uart-backlight-emulator /dev/ttyUSB0
+
+And then (re)load the dell-uart-backlight driver:
+
+sudo rmmod dell-uart-backlight; sudo modprobe dell-uart-backlight dyndbg
+
+After this check "dmesg" to see if the driver correctly received
+the firmware version string from the emulator. If this works there
+should be a /sys/class/backlight/dell_uart_backlight/ directory now
+and writes to the brightness or bl_power files should be reflected
+by matching output from the emulator.
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c b/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c
new file mode 100644
index 000000000000..fab1babb26d0
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c
@@ -0,0 +1,166 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Dell AIO Serial Backlight board emulator for testing
+ * the Linux dell-uart-backlight driver.
+ *
+ * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <termios.h>
+#include <unistd.h>
+
+int serial_fd;
+int brightness = 50;
+
+static unsigned char dell_uart_checksum(unsigned char *buf, int len)
+{
+        unsigned char val = 0;
+
+        while (len-- > 0)
+                val += buf[len];
+
+        return val ^ 0xff;
+}
+
+/* read() will return -1 on SIGINT / SIGTERM causing the mainloop to cleanly exit */
+void signalhdlr(int signum)
+{
+}
+
+int main(int argc, char *argv[])
+{
+	struct sigaction sigact = { .sa_handler = signalhdlr };
+	unsigned char buf[4], csum, response[32];
+	const char *version_str = "PHI23-V321";
+	struct termios tty, saved_tty;
+	int ret, idx, len = 0;
+
+	if (argc != 2) {
+		fprintf(stderr, "Invalid or missing arguments\n");
+		fprintf(stderr, "Usage: %s <serial-port>\n", argv[0]);
+		return 1;
+	}
+
+	serial_fd = open(argv[1], O_RDWR | O_NOCTTY);
+	if (serial_fd == -1) {
+		fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));
+		return 1;
+	}
+
+	ret = tcgetattr(serial_fd, &tty);
+	if (ret == -1) {
+		fprintf(stderr, "Error getting tcattr: %s\n", strerror(errno));
+		goto out_close;
+	}
+	saved_tty = tty;
+
+	cfsetspeed(&tty, 9600);
+	cfmakeraw(&tty);
+	tty.c_cflag &= ~CSTOPB;
+	tty.c_cflag &= ~CRTSCTS;
+	tty.c_cflag |= CLOCAL | CREAD;
+	
+	ret = tcsetattr(serial_fd, TCSANOW, &tty);
+	if (ret == -1) {
+		fprintf(stderr, "Error setting tcattr: %s\n", strerror(errno));
+		goto out_restore;
+	}
+
+	sigaction(SIGINT, &sigact, 0);
+	sigaction(SIGTERM, &sigact, 0);
+
+	idx = 0;
+	while (read(serial_fd, &buf[idx], 1) == 1) {
+		if (idx == 0) {
+			switch (buf[0]) {
+			/* 3 MSB bits: cmd-len + 01010 SOF marker */
+			case 0x6a: len = 3; break;
+			case 0x8a: len = 4; break;
+			default:
+				fprintf(stderr, "Error unexpected first byte: 0x%02x\n", buf[0]);
+				continue; /* Try to sync up with sender */
+			}
+		}
+
+		/* Process msg when len bytes have been received */
+		if (idx != (len - 1)) {
+			idx++;
+			continue;
+		}
+
+		/* Reset idx for next command */
+		idx = 0;
+
+		csum = dell_uart_checksum(buf, len - 1);
+		if (buf[len - 1] != csum) {
+			fprintf(stderr, "Error checksum mismatch got 0x%02x expected 0x%02x\n",
+				buf[len - 1], csum);
+			continue;
+		}
+
+		switch ((buf[0] << 8) | buf[1]) {
+		case 0x6a06: 
+			/* cmd = 0x06, get version */
+			len = strlen(version_str);
+			strcpy((char *)&response[2], version_str);
+			printf("Get version, reply: %s\n", version_str);
+			break;
+		case 0x8a0b: /* 3 MSB bits: cmd-len + 01010 SOF marker */
+			/* cmd = 0x0b, set brightness */
+			if (buf[2] > 100) {
+				fprintf(stderr, "Error invalid brightness param: %d\n", buf[2]);
+				continue;
+			}
+
+			len = 0;
+			brightness = buf[2];
+			printf("Set brightness %d\n", brightness);
+			break;
+		case 0x6a0c:
+			/* cmd = 0x0c, get brightness */
+			len = 1;
+			response[2] = brightness;
+			printf("Get brightness, reply: %d\n", brightness);
+			break;
+		case 0x8a0e:
+			/* cmd = 0x0e, set backlight power */
+			if (buf[2] != 0 && buf[2] != 1) {
+				fprintf(stderr, "Error invalid set power param: %d\n", buf[2]);
+				continue;
+			}
+
+			len = 0;
+			printf("Set power %d\n", buf[2]);
+			break;
+		default:
+			fprintf(stderr, "Error unknown cmd 0x%04x\n",
+				(buf[0] << 8) | buf[1]);
+			continue;
+		}
+
+		/* Respond with <total-len> <cmd> <data...> <csum> */
+		response[0] = len + 3; /* response length in bytes */
+		response[1] = buf[1];  /* ack cmd */
+		csum = dell_uart_checksum(response, len + 2);
+		response[len + 2] = csum;
+		ret = write(serial_fd, response, len + 3);
+		if (ret != (len + 3))
+			fprintf(stderr, "Error writing %d bytes: %d\n",
+				len + 3, ret);
+	}
+
+out_restore:
+	tcsetattr(serial_fd, TCSANOW, &saved_tty);
+out_close:
+	close(serial_fd);
+	return ret;
+}
-- 
2.44.0

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

[Kai-Heng Feng]

[+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
> >>
> >

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Kai-Heng Feng]

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
>
>

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

[Hans de Goede]

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

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

[Hans de Goede]

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