| Message ID | SY4P282MB306333469B31348E4A25A1A5C5312@SY4P282MB3063.AUSP282.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | platform/chrome: cros_kbd_led_backlight: Automatically enable keyboard backlight control if feature present in EC | expand |
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
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
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, };
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(-)