Received-SPF: pass (google.com: domain of linux-kernel+bounces-116458-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 25 Mar 2024 11:24:56 +0800
From: Tzung-Bi Shih <tzungbi@kernel.org>
To: Stephen Horvath <s.horvath@outlook.com.au>
Cc: Lee Jones <lee@kernel.org>, Benson Leung <bleung@chromium.org>,
	Guenter Roeck <groeck@chromium.org>,
	chrome-platform@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] platform/chrome: cros_kbd_led_backlight:
 Automatically enable keyboard backlight control if feature present in EC
Message-ID: <ZgDuiEYcgMTuZAeA@google.com>
References: <20240322102800.1322022-1-s.horvath@outlook.com.au>
 <SY4P282MB306333469B31348E4A25A1A5C5312@SY4P282MB3063.AUSP282.PROD.OUTLOOK.COM>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SY4P282MB306333469B31348E4A25A1A5C5312@SY4P282MB3063.AUSP282.PROD.OUTLOOK.COM>

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