[PATCH 1/2] platform/chrome: cros_kbd_led_backlight: Automatically enable keyboard backlight control if feature present in EC

[PATCH 1/2] platform/chrome: cros_kbd_led_backlight: Automatically enable keyboard backlight control if feature present in EC

[Stephen Horvath]

The keyboard backlight control is not enabled by default on all devices
with a Chromium EC, e.g. my Framework 13.
This patch adds a check for the feature and enables backlight control if
it is present.

I also fixed a bug that seemed like there was some confusion between
`cros_ec_dev` and `cros_ec_device`, so I'm not sure if this module even
worked until now.

Signed-off-by: Stephen Horvath <s.horvath@outlook.com.au>
---
 drivers/mfd/cros_ec_dev.c                     |  9 +++++++
 .../platform/chrome/cros_kbd_led_backlight.c  | 24 +++++++------------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index a52d59cc2b1e..29b85ed5fece 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -99,6 +99,10 @@ static const struct mfd_cell cros_ec_wdt_cells[] = {
 	{ .name = "cros-ec-wdt", }
 };
 
+static const struct mfd_cell cros_ec_pwm_kbd_cells[] = {
+	{ .name = "chromeos-keyboard-leds", }
+};
+
 static const struct cros_feature_to_cells cros_subdevices[] = {
 	{
 		.id		= EC_FEATURE_CEC,
@@ -125,6 +129,11 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
 		.mfd_cells	= cros_ec_wdt_cells,
 		.num_cells	= ARRAY_SIZE(cros_ec_wdt_cells),
 	},
+	{
+		.id		= EC_FEATURE_PWM_KEYB,
+		.mfd_cells	= cros_ec_pwm_kbd_cells,
+		.num_cells	= ARRAY_SIZE(cros_ec_pwm_kbd_cells),
+	},
 };
 
 static const struct mfd_cell cros_ec_platform_cells[] = {
diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
index 793fd3f1015d..06e9a57536af 100644
--- a/drivers/platform/chrome/cros_kbd_led_backlight.c
+++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
@@ -171,12 +171,15 @@ static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
 {
 	struct keyboard_led *keyboard_led = platform_get_drvdata(pdev);
 
-	keyboard_led->ec = dev_get_drvdata(pdev->dev.parent);
-	if (!keyboard_led->ec) {
+	struct cros_ec_dev *parent_ec = dev_get_drvdata(pdev->dev.parent);
+
+	if (!parent_ec) {
 		dev_err(&pdev->dev, "no parent EC device\n");
 		return -EINVAL;
 	}
 
+	keyboard_led->ec = parent_ec->ec_dev;
+
 	return 0;
 }
 
@@ -200,8 +203,10 @@ static int keyboard_led_probe(struct platform_device *pdev)
 	int error;
 
 	drvdata = device_get_match_data(&pdev->dev);
-	if (!drvdata)
-		return -EINVAL;
+	if (!drvdata) {
+		/* Assume EC if no match data is provided */
+		drvdata = &keyboard_led_drvdata_ec_pwm;
+	}
 
 	keyboard_led = devm_kzalloc(&pdev->dev, sizeof(*keyboard_led), GFP_KERNEL);
 	if (!keyboard_led)
@@ -236,22 +241,10 @@ static const struct acpi_device_id keyboard_led_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, keyboard_led_acpi_match);
 #endif
 
-#ifdef CONFIG_OF
-static const struct of_device_id keyboard_led_of_match[] = {
-	{
-		.compatible = "google,cros-kbd-led-backlight",
-		.data = &keyboard_led_drvdata_ec_pwm,
-	},
-	{}
-};
-MODULE_DEVICE_TABLE(of, keyboard_led_of_match);
-#endif
-
 static struct platform_driver keyboard_led_driver = {
 	.driver		= {
 		.name	= "chromeos-keyboard-leds",
 		.acpi_match_table = ACPI_PTR(keyboard_led_acpi_match),
-		.of_match_table = of_match_ptr(keyboard_led_of_match),
 	},
 	.probe		= keyboard_led_probe,
 };
-- 
2.43.0

Re: [PATCH 1/2] platform/chrome: cros_kbd_led_backlight: Automatically enable keyboard backlight control if feature present in EC

[Tzung-Bi Shih]

On Fri, Mar 22, 2024 at 08:27:58PM +1000, Stephen Horvath wrote:
> I also fixed a bug that seemed like there was some confusion between
> `cros_ec_dev` and `cros_ec_device`, so I'm not sure if this module even
> worked until now.

If they aren't part of commit message, please put them after "---".  Please
separate to an independent patch if they fix different things.

> diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
> index 793fd3f1015d..06e9a57536af 100644
> --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> @@ -171,12 +171,15 @@ static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
>  {
>  	struct keyboard_led *keyboard_led = platform_get_drvdata(pdev);
>  
> -	keyboard_led->ec = dev_get_drvdata(pdev->dev.parent);
> -	if (!keyboard_led->ec) {
> +	struct cros_ec_dev *parent_ec = dev_get_drvdata(pdev->dev.parent);

I'm not sure if it really fixes anything.  See [1] or [2].

[1]: https://elixir.bootlin.com/linux/v6.8/source/drivers/platform/chrome/cros_ec_lpc.c#L417
[2]: https://elixir.bootlin.com/linux/v6.8/source/drivers/platform/chrome/cros_ec_spi.c#L759

> @@ -200,8 +203,10 @@ static int keyboard_led_probe(struct platform_device *pdev)
>  	int error;
>  
>  	drvdata = device_get_match_data(&pdev->dev);
> -	if (!drvdata)
> -		return -EINVAL;
> +	if (!drvdata) {
> +		/* Assume EC if no match data is provided */
> +		drvdata = &keyboard_led_drvdata_ec_pwm;
> +	}

By current design, the `drvdata` should be provided by either ACPI or OF match.

> @@ -236,22 +241,10 @@ static const struct acpi_device_id keyboard_led_acpi_match[] = {
>  MODULE_DEVICE_TABLE(acpi, keyboard_led_acpi_match);
>  #endif
>  
> -#ifdef CONFIG_OF
> -static const struct of_device_id keyboard_led_of_match[] = {
> -	{
> -		.compatible = "google,cros-kbd-led-backlight",
> -		.data = &keyboard_led_drvdata_ec_pwm,
> -	},
> -	{}
> -};
> -MODULE_DEVICE_TABLE(of, keyboard_led_of_match);
> -#endif
> -
>  static struct platform_driver keyboard_led_driver = {
>  	.driver		= {
>  		.name	= "chromeos-keyboard-leds",
>  		.acpi_match_table = ACPI_PTR(keyboard_led_acpi_match),
> -		.of_match_table = of_match_ptr(keyboard_led_of_match),

The patch is ruining the use cases in OF world (e.g. [3]).

[3]: https://elixir.bootlin.com/linux/v6.8/source/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi#L1154

Re: [PATCH 1/2] platform/chrome: cros_kbd_led_backlight: Automatically enable keyboard backlight control if feature present in EC

[Stephen Horvath]

On 25/3/24 13:24, Tzung-Bi Shih wrote:
> If they aren't part of commit message, please put them after "---". Please
> separate to an independent patch if they fix different things.

Alright thanks, the separation thing did kinda occur to me after I'd 
send it, but I will do it next time.

> I'm not sure if it really fixes anything.  See [1] or [2].

Ahh okay, I guess it was probably a side effect of me breaking something 
else then, since the module only seemed to crash without that change. 
But I was also mildly confused how it seemed to work fine like that 
elsewhere though.

> By current design, the `drvdata` should be provided by either ACPI or OF match.

Yeah sorry, I didn't really know of a simpler way to do it, since it 
gets started from `mfd_add_hotplug_devices` in cros_ec_dev.c in this 
patch. I'll try and find a better way if I decide to make a v2.

> The patch is ruining the use cases in OF world (e.g. [3]).

I was kinda worried that the module would somehow launch twice, but I 
have a feeling that wouldn't be possible anyway.

Thanks for your feedback!
Steve

[PATCH 2/2] platform/chrome: cros_kbd_led_backlight: Remove obsolete commands (EC_CMD_PWM_*_KEYBOARD_BACKLIGHT)

[Stephen Horvath]

EC_CMD_PWM_GET_KEYBOARD_BACKLIGHT and EC_CMD_PWM_SET_KEYBOARD_BACKLIGHT
are marked as obsolete in `cros_ec_commands.h`, this patch removes the
usage of these commands from the driver.

The max brightness value is now set to `EC_PWM_MAX_DUTY` (65535) when
running on the EC, as this is the maximum value that the EC can accept.

Signed-off-by: Stephen Horvath <s.horvath@outlook.com.au>
---
 .../platform/chrome/cros_kbd_led_backlight.c  | 37 +++++++++++++------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
index 06e9a57536af..bc021437e8b1 100644
--- a/drivers/platform/chrome/cros_kbd_led_backlight.c
+++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
@@ -47,10 +47,10 @@ struct keyboard_led_drvdata {
 	enum led_brightness max_brightness;
 };
 
-#define KEYBOARD_BACKLIGHT_MAX 100
-
 #ifdef CONFIG_ACPI
 
+#define ACPI_KEYBOARD_BACKLIGHT_MAX 100
+
 /* Keyboard LED ACPI Device must be defined in firmware */
 #define ACPI_KEYBOARD_BACKLIGHT_DEVICE	"\\_SB.KBLT"
 #define ACPI_KEYBOARD_BACKLIGHT_READ	ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBQC"
@@ -114,7 +114,7 @@ static const struct keyboard_led_drvdata keyboard_led_drvdata_acpi = {
 	.init = keyboard_led_init_acpi,
 	.brightness_set = keyboard_led_set_brightness_acpi,
 	.brightness_get = keyboard_led_get_brightness_acpi,
-	.max_brightness = KEYBOARD_BACKLIGHT_MAX,
+	.max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX,
 };
 
 #endif /* CONFIG_ACPI */
@@ -127,18 +127,22 @@ keyboard_led_set_brightness_ec_pwm(struct led_classdev *cdev,
 {
 	struct {
 		struct cros_ec_command msg;
-		struct ec_params_pwm_set_keyboard_backlight params;
+		struct ec_params_pwm_set_duty params;
 	} __packed buf;
-	struct ec_params_pwm_set_keyboard_backlight *params = &buf.params;
+	struct ec_params_pwm_set_duty *params = &buf.params;
 	struct cros_ec_command *msg = &buf.msg;
 	struct keyboard_led *keyboard_led = container_of(cdev, struct keyboard_led, cdev);
 
 	memset(&buf, 0, sizeof(buf));
 
-	msg->command = EC_CMD_PWM_SET_KEYBOARD_BACKLIGHT;
+	msg->version = 0;
+	msg->command = EC_CMD_PWM_SET_DUTY;
+	msg->insize = 0;
 	msg->outsize = sizeof(*params);
 
-	params->percent = brightness;
+	params->duty = brightness;
+	params->pwm_type = EC_PWM_TYPE_KB_LIGHT;
+	params->index = 0;
 
 	return cros_ec_cmd_xfer_status(keyboard_led->ec, msg);
 }
@@ -148,23 +152,32 @@ keyboard_led_get_brightness_ec_pwm(struct led_classdev *cdev)
 {
 	struct {
 		struct cros_ec_command msg;
-		struct ec_response_pwm_get_keyboard_backlight resp;
+		union {
+			struct ec_params_pwm_get_duty params;
+			struct ec_response_pwm_get_duty resp;
+		};
 	} __packed buf;
-	struct ec_response_pwm_get_keyboard_backlight *resp = &buf.resp;
+	struct ec_params_pwm_get_duty *params = &buf.params;
+	struct ec_response_pwm_get_duty *resp = &buf.resp;
 	struct cros_ec_command *msg = &buf.msg;
 	struct keyboard_led *keyboard_led = container_of(cdev, struct keyboard_led, cdev);
 	int ret;
 
 	memset(&buf, 0, sizeof(buf));
 
-	msg->command = EC_CMD_PWM_GET_KEYBOARD_BACKLIGHT;
+	msg->version = 0;
+	msg->command = EC_CMD_PWM_GET_DUTY;
 	msg->insize = sizeof(*resp);
+	msg->outsize = sizeof(*params);
+
+	params->pwm_type = EC_PWM_TYPE_KB_LIGHT;
+	params->index = 0;
 
 	ret = cros_ec_cmd_xfer_status(keyboard_led->ec, msg);
 	if (ret < 0)
 		return ret;
 
-	return resp->percent;
+	return resp->duty;
 }
 
 static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
@@ -186,7 +199,7 @@ static const __maybe_unused struct keyboard_led_drvdata keyboard_led_drvdata_ec_
 	.init = keyboard_led_init_ec_pwm,
 	.brightness_set_blocking = keyboard_led_set_brightness_ec_pwm,
 	.brightness_get = keyboard_led_get_brightness_ec_pwm,
-	.max_brightness = KEYBOARD_BACKLIGHT_MAX,
+	.max_brightness = EC_PWM_MAX_DUTY,
 };
 
 #else /* IS_ENABLED(CONFIG_CROS_EC) */
-- 
2.43.0

Re: [PATCH 2/2] platform/chrome: cros_kbd_led_backlight: Remove obsolete commands (EC_CMD_PWM_*_KEYBOARD_BACKLIGHT)

[Brian Norris]

On Fri, Mar 22, 2024 at 3:34 AM Stephen Horvath
<s.horvath@outlook.com.au> wrote:
>
> EC_CMD_PWM_GET_KEYBOARD_BACKLIGHT and EC_CMD_PWM_SET_KEYBOARD_BACKLIGHT
> are marked as obsolete in `cros_ec_commands.h`, this patch removes the
> usage of these commands from the driver.

Just because the EC firmware repository marks these as obsolete (and
yes, we copy that header mostly as-is into the kernel repository ...
but it's still a firmware header) doesn't mean it's truly ready to be
removed. I believe the intention is to direct *firmware* developers
not to use them -- any new developments should be using the new
commands.

From a kernel perspective, we could still be supporting old firmware
on old devices, and so we may want/need to continue to support these
commands.

I don't know off the top of my head which firmware branches support
which commands, on devices that have such keyboard backlights. (The
Chromium EC repository is open source though, with various firmware-*
branches still around, so this information is available.) But without
a better explanation as to why these are truly ready to be removed,
I'll say "NAK."

Brian

Re: [PATCH 2/2] platform/chrome: cros_kbd_led_backlight: Remove obsolete commands (EC_CMD_PWM_*_KEYBOARD_BACKLIGHT)

[Stephen Horvath]

On 26/3/24 04:45, Brian Norris wrote:
> Just because the EC firmware repository marks these as obsolete (and
> yes, we copy that header mostly as-is into the kernel repository ...
> but it's still a firmware header) doesn't mean it's truly ready to be
> removed. I believe the intention is to direct *firmware* developers
> not to use them -- any new developments should be using the new
> commands.
> 
> From a kernel perspective, we could still be supporting old firmware
> on old devices, and so we may want/need to continue to support these
> commands.

Alright that makes sense.

> I don't know off the top of my head which firmware branches support
> which commands, on devices that have such keyboard backlights. (The
> Chromium EC repository is open source though, with various firmware-*
> branches still around, so this information is available.) But without
> a better explanation as to why these are truly ready to be removed,
> I'll say "NAK."

Yeah that's fair enough, my laptop seems to support both so I'll agree 
the older commands are probably the safer option.

Thanks a lot for your feedback!
Steve

Re: [PATCH 2/2] platform/chrome: cros_kbd_led_backlight: Remove obsolete commands (EC_CMD_PWM_*_KEYBOARD_BACKLIGHT)

[Brian Norris]

On Mon, Mar 25, 2024 at 5:30 PM Stephen Horvath
<s.horvath@outlook.com.au> wrote:
> my laptop seems to support both so I'll agree
> the older commands are probably the safer option.

Huh, it's surprising to me that it supports both, if one was marked
obsolete. But I haven't tracked the development in the EC firmware
that closely recently.

If the old command works, it's probably safe to keep using it. But
it's possible we'll eventually see some device with firmware that
doesn't support the old one, and we'll have to update the kernel
drivers for both. Note that there's an existing driver for the newer
generic PWM command protocol:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-cros-ec.c?h=v6.8

So far, it's only been used on Device Tree systems as far as I can
tell, and one would probably have to wire it up differently for an
ACPI system. I just mention it as an FYI.

Brian