drivers/mfd/cros_ec_dev.c | 9 ++++++ drivers/platform/chrome/cros_kbd_led_backlight.c | 41 +++++++++++++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-)
The ChromeOS EC used in Framework laptops supports the standard cros
keyboard backlight protocol.
However the firmware on these laptops don't implement the ACPI ID
GOOG0002 that is recognized by cros_kbd_led_backlight and they also
don't use device tree.
Extend the cros_ec MFD device to also load cros_kbd_led_backlight
when the EC reports EC_FEATURE_PWM_KEYB.
Tested on a Framework 13 AMD, Bios 3.05.
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
This is based on
https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
The helper keyboard_led_is_mfd_device is a bit iffy, but I couldn't find
a nicer way.
* driver_data from platform_device_id is overwritten by the mfd platform data
* Setting the driver_data in drivers/mfd/cros_ec_dev.c would expose the
internals of cros_kbd_led_backlight
---
drivers/mfd/cros_ec_dev.c | 9 ++++++
drivers/platform/chrome/cros_kbd_led_backlight.c | 41 +++++++++++++++++++++++-
2 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index a52d59cc2b1e..4444b361aeae 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_keyboard_leds_cells[] = {
+ { .name = "cros-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_keyboard_leds_cells,
+ .num_cells = ARRAY_SIZE(cros_ec_keyboard_leds_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 b83e4f328620..88dd3848e5da 100644
--- a/drivers/platform/chrome/cros_kbd_led_backlight.c
+++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
@@ -194,13 +194,52 @@ static const __maybe_unused struct keyboard_led_drvdata keyboard_led_drvdata_ec_
#endif /* IS_ENABLED(CONFIG_CROS_EC) */
+#if IS_ENABLED(CONFIG_MFD_CROS_EC_DEV)
+static int keyboard_led_init_ec_pwm_mfd(struct platform_device *pdev)
+{
+ struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
+ struct cros_ec_device *cros_ec = ec_dev->ec_dev;
+ struct keyboard_led *keyboard_led = platform_get_drvdata(pdev);
+
+ keyboard_led->ec = cros_ec;
+
+ return 0;
+}
+
+static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm_mfd = {
+ .init = keyboard_led_init_ec_pwm_mfd,
+ .brightness_set_blocking = keyboard_led_set_brightness_ec_pwm,
+ .brightness_get = keyboard_led_get_brightness_ec_pwm,
+ .max_brightness = KEYBOARD_BACKLIGHT_MAX,
+};
+
+#else /* IS_ENABLED(CONFIG_MFD_CROS_EC_DEV) */
+
+static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
+
+#endif /* IS_ENABLED(CONFIG_MFD_CROS_EC_DEV) */
+
+static int keyboard_led_is_mfd_device(struct platform_device *pdev)
+{
+ if (!IS_ENABLED(CONFIG_MFD_CROS_EC_DEV))
+ return 0;
+
+ if (!pdev->dev.parent)
+ return 0;
+
+ return strcmp(pdev->dev.parent->driver->name, "cros-ec-dev") == 0;
+}
+
static int keyboard_led_probe(struct platform_device *pdev)
{
const struct keyboard_led_drvdata *drvdata;
struct keyboard_led *keyboard_led;
int error;
- drvdata = device_get_match_data(&pdev->dev);
+ if (keyboard_led_is_mfd_device(pdev))
+ drvdata = &keyboard_led_drvdata_ec_pwm_mfd;
+ else
+ drvdata = device_get_match_data(&pdev->dev);
if (!drvdata)
return -EINVAL;
---
base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
change-id: 20240505-cros_ec-kbd-led-framework-7e2e831bc79c
Best regards,
--
Thomas Weißschuh <linux@weissschuh.net>
On Sun, 05 May 2024, Thomas Weißschuh wrote: > The ChromeOS EC used in Framework laptops supports the standard cros > keyboard backlight protocol. > However the firmware on these laptops don't implement the ACPI ID > GOOG0002 that is recognized by cros_kbd_led_backlight and they also > don't use device tree. > > Extend the cros_ec MFD device to also load cros_kbd_led_backlight > when the EC reports EC_FEATURE_PWM_KEYB. > > Tested on a Framework 13 AMD, Bios 3.05. > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > This is based on > https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next > > The helper keyboard_led_is_mfd_device is a bit iffy, but I couldn't find > a nicer way. > > * driver_data from platform_device_id is overwritten by the mfd platform data > * Setting the driver_data in drivers/mfd/cros_ec_dev.c would expose the > internals of cros_kbd_led_backlight > --- > drivers/mfd/cros_ec_dev.c | 9 ++++++ Split this out please. > drivers/platform/chrome/cros_kbd_led_backlight.c | 41 +++++++++++++++++++++++- > 2 files changed, 49 insertions(+), 1 deletion(-) -- Lee Jones [李琼斯]
Hi Thomas,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 2fbe479c0024e1c6b992184a799055e19932aa48]
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Wei-schuh/platform-chrome-cros_kbd_led_backlight-enable-probing-through-EC_FEATURE_PWM_KEYB/20240505-174413
base: 2fbe479c0024e1c6b992184a799055e19932aa48
patch link: https://lore.kernel.org/r/20240505-cros_ec-kbd-led-framework-v1-1-bfcca69013d2%40weissschuh.net
patch subject: [PATCH] platform/chrome: cros_kbd_led_backlight: enable probing through EC_FEATURE_PWM_KEYB
config: x86_64-randconfig-014-20240506 (https://download.01.org/0day-ci/archive/20240506/202405060653.uskC4eSJ-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240506/202405060653.uskC4eSJ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405060653.uskC4eSJ-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/platform/chrome/cros_kbd_led_backlight.c:218:42: error: redefinition of 'keyboard_led_drvdata_ec_pwm'
218 | static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/platform/chrome/cros_kbd_led_backlight.c:184:57: note: previous definition of 'keyboard_led_drvdata_ec_pwm' with type 'const struct keyboard_led_drvdata'
184 | static const __maybe_unused struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/platform/chrome/cros_kbd_led_backlight.c: In function 'keyboard_led_probe':
drivers/platform/chrome/cros_kbd_led_backlight.c:240:28: error: 'keyboard_led_drvdata_ec_pwm_mfd' undeclared (first use in this function); did you mean 'keyboard_led_drvdata_ec_pwm'?
240 | drvdata = &keyboard_led_drvdata_ec_pwm_mfd;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| keyboard_led_drvdata_ec_pwm
drivers/platform/chrome/cros_kbd_led_backlight.c:240:28: note: each undeclared identifier is reported only once for each function it appears in
drivers/platform/chrome/cros_kbd_led_backlight.c: At top level:
>> drivers/platform/chrome/cros_kbd_led_backlight.c:218:42: warning: 'keyboard_led_drvdata_ec_pwm' defined but not used [-Wunused-const-variable=]
218 | static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/keyboard_led_drvdata_ec_pwm +218 drivers/platform/chrome/cros_kbd_led_backlight.c
217
> 218 static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
219
220 #endif /* IS_ENABLED(CONFIG_MFD_CROS_EC_DEV) */
221
222 static int keyboard_led_is_mfd_device(struct platform_device *pdev)
223 {
224 if (!IS_ENABLED(CONFIG_MFD_CROS_EC_DEV))
225 return 0;
226
227 if (!pdev->dev.parent)
228 return 0;
229
230 return strcmp(pdev->dev.parent->driver->name, "cros-ec-dev") == 0;
231 }
232
233 static int keyboard_led_probe(struct platform_device *pdev)
234 {
235 const struct keyboard_led_drvdata *drvdata;
236 struct keyboard_led *keyboard_led;
237 int error;
238
239 if (keyboard_led_is_mfd_device(pdev))
> 240 drvdata = &keyboard_led_drvdata_ec_pwm_mfd;
241 else
242 drvdata = device_get_match_data(&pdev->dev);
243 if (!drvdata)
244 return -EINVAL;
245
246 keyboard_led = devm_kzalloc(&pdev->dev, sizeof(*keyboard_led), GFP_KERNEL);
247 if (!keyboard_led)
248 return -ENOMEM;
249 platform_set_drvdata(pdev, keyboard_led);
250
251 if (drvdata->init) {
252 error = drvdata->init(pdev);
253 if (error)
254 return error;
255 }
256
257 keyboard_led->cdev.name = "chromeos::kbd_backlight";
258 keyboard_led->cdev.flags |= LED_CORE_SUSPENDRESUME;
259 keyboard_led->cdev.max_brightness = drvdata->max_brightness;
260 keyboard_led->cdev.brightness_set = drvdata->brightness_set;
261 keyboard_led->cdev.brightness_set_blocking = drvdata->brightness_set_blocking;
262 keyboard_led->cdev.brightness_get = drvdata->brightness_get;
263
264 error = devm_led_classdev_register(&pdev->dev, &keyboard_led->cdev);
265 if (error)
266 return error;
267
268 return 0;
269 }
270
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Thomas,
kernel test robot noticed the following build errors:
[auto build test ERROR on 2fbe479c0024e1c6b992184a799055e19932aa48]
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Wei-schuh/platform-chrome-cros_kbd_led_backlight-enable-probing-through-EC_FEATURE_PWM_KEYB/20240505-174413
base: 2fbe479c0024e1c6b992184a799055e19932aa48
patch link: https://lore.kernel.org/r/20240505-cros_ec-kbd-led-framework-v1-1-bfcca69013d2%40weissschuh.net
patch subject: [PATCH] platform/chrome: cros_kbd_led_backlight: enable probing through EC_FEATURE_PWM_KEYB
config: x86_64-randconfig-072-20240506 (https://download.01.org/0day-ci/archive/20240506/202405060655.dD06crN0-lkp@intel.com/config)
compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240506/202405060655.dD06crN0-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405060655.dD06crN0-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/platform/chrome/cros_kbd_led_backlight.c:218:42: error: redefinition of 'keyboard_led_drvdata_ec_pwm'
218 | static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
| ^
drivers/platform/chrome/cros_kbd_led_backlight.c:184:57: note: previous definition is here
184 | static const __maybe_unused struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
| ^
>> drivers/platform/chrome/cros_kbd_led_backlight.c:240:14: error: use of undeclared identifier 'keyboard_led_drvdata_ec_pwm_mfd'; did you mean 'keyboard_led_drvdata_acpi'?
240 | drvdata = &keyboard_led_drvdata_ec_pwm_mfd;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| keyboard_led_drvdata_acpi
drivers/platform/chrome/cros_kbd_led_backlight.c:114:42: note: 'keyboard_led_drvdata_acpi' declared here
114 | static const struct keyboard_led_drvdata keyboard_led_drvdata_acpi = {
| ^
2 errors generated.
vim +240 drivers/platform/chrome/cros_kbd_led_backlight.c
217
> 218 static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
219
220 #endif /* IS_ENABLED(CONFIG_MFD_CROS_EC_DEV) */
221
222 static int keyboard_led_is_mfd_device(struct platform_device *pdev)
223 {
224 if (!IS_ENABLED(CONFIG_MFD_CROS_EC_DEV))
225 return 0;
226
227 if (!pdev->dev.parent)
228 return 0;
229
230 return strcmp(pdev->dev.parent->driver->name, "cros-ec-dev") == 0;
231 }
232
233 static int keyboard_led_probe(struct platform_device *pdev)
234 {
235 const struct keyboard_led_drvdata *drvdata;
236 struct keyboard_led *keyboard_led;
237 int error;
238
239 if (keyboard_led_is_mfd_device(pdev))
> 240 drvdata = &keyboard_led_drvdata_ec_pwm_mfd;
241 else
242 drvdata = device_get_match_data(&pdev->dev);
243 if (!drvdata)
244 return -EINVAL;
245
246 keyboard_led = devm_kzalloc(&pdev->dev, sizeof(*keyboard_led), GFP_KERNEL);
247 if (!keyboard_led)
248 return -ENOMEM;
249 platform_set_drvdata(pdev, keyboard_led);
250
251 if (drvdata->init) {
252 error = drvdata->init(pdev);
253 if (error)
254 return error;
255 }
256
257 keyboard_led->cdev.name = "chromeos::kbd_backlight";
258 keyboard_led->cdev.flags |= LED_CORE_SUSPENDRESUME;
259 keyboard_led->cdev.max_brightness = drvdata->max_brightness;
260 keyboard_led->cdev.brightness_set = drvdata->brightness_set;
261 keyboard_led->cdev.brightness_set_blocking = drvdata->brightness_set_blocking;
262 keyboard_led->cdev.brightness_get = drvdata->brightness_get;
263
264 error = devm_led_classdev_register(&pdev->dev, &keyboard_led->cdev);
265 if (error)
266 return error;
267
268 return 0;
269 }
270
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Thomas,
kernel test robot noticed the following build errors:
[auto build test ERROR on 2fbe479c0024e1c6b992184a799055e19932aa48]
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Wei-schuh/platform-chrome-cros_kbd_led_backlight-enable-probing-through-EC_FEATURE_PWM_KEYB/20240505-174413
base: 2fbe479c0024e1c6b992184a799055e19932aa48
patch link: https://lore.kernel.org/r/20240505-cros_ec-kbd-led-framework-v1-1-bfcca69013d2%40weissschuh.net
patch subject: [PATCH] platform/chrome: cros_kbd_led_backlight: enable probing through EC_FEATURE_PWM_KEYB
config: x86_64-buildonly-randconfig-002-20240506 (https://download.01.org/0day-ci/archive/20240506/202405060439.O5mY5RbW-lkp@intel.com/config)
compiler: gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240506/202405060439.O5mY5RbW-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405060439.O5mY5RbW-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/platform/chrome/cros_kbd_led_backlight.c:218:42: error: redefinition of 'keyboard_led_drvdata_ec_pwm'
static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/platform/chrome/cros_kbd_led_backlight.c:193:57: note: previous definition of 'keyboard_led_drvdata_ec_pwm' was here
static const __maybe_unused struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/platform/chrome/cros_kbd_led_backlight.c: In function 'keyboard_led_probe':
>> drivers/platform/chrome/cros_kbd_led_backlight.c:240:14: error: 'keyboard_led_drvdata_ec_pwm_mfd' undeclared (first use in this function); did you mean 'keyboard_led_drvdata_ec_pwm'?
drvdata = &keyboard_led_drvdata_ec_pwm_mfd;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
keyboard_led_drvdata_ec_pwm
drivers/platform/chrome/cros_kbd_led_backlight.c:240:14: note: each undeclared identifier is reported only once for each function it appears in
vim +/keyboard_led_drvdata_ec_pwm +218 drivers/platform/chrome/cros_kbd_led_backlight.c
217
> 218 static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
219
220 #endif /* IS_ENABLED(CONFIG_MFD_CROS_EC_DEV) */
221
222 static int keyboard_led_is_mfd_device(struct platform_device *pdev)
223 {
224 if (!IS_ENABLED(CONFIG_MFD_CROS_EC_DEV))
225 return 0;
226
227 if (!pdev->dev.parent)
228 return 0;
229
230 return strcmp(pdev->dev.parent->driver->name, "cros-ec-dev") == 0;
231 }
232
233 static int keyboard_led_probe(struct platform_device *pdev)
234 {
235 const struct keyboard_led_drvdata *drvdata;
236 struct keyboard_led *keyboard_led;
237 int error;
238
239 if (keyboard_led_is_mfd_device(pdev))
> 240 drvdata = &keyboard_led_drvdata_ec_pwm_mfd;
241 else
242 drvdata = device_get_match_data(&pdev->dev);
243 if (!drvdata)
244 return -EINVAL;
245
246 keyboard_led = devm_kzalloc(&pdev->dev, sizeof(*keyboard_led), GFP_KERNEL);
247 if (!keyboard_led)
248 return -ENOMEM;
249 platform_set_drvdata(pdev, keyboard_led);
250
251 if (drvdata->init) {
252 error = drvdata->init(pdev);
253 if (error)
254 return error;
255 }
256
257 keyboard_led->cdev.name = "chromeos::kbd_backlight";
258 keyboard_led->cdev.flags |= LED_CORE_SUSPENDRESUME;
259 keyboard_led->cdev.max_brightness = drvdata->max_brightness;
260 keyboard_led->cdev.brightness_set = drvdata->brightness_set;
261 keyboard_led->cdev.brightness_set_blocking = drvdata->brightness_set_blocking;
262 keyboard_led->cdev.brightness_get = drvdata->brightness_get;
263
264 error = devm_led_classdev_register(&pdev->dev, &keyboard_led->cdev);
265 if (error)
266 return error;
267
268 return 0;
269 }
270
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 5/5/2024 04:41, Thomas Weißschuh wrote:
> The ChromeOS EC used in Framework laptops supports the standard cros
> keyboard backlight protocol.
> However the firmware on these laptops don't implement the ACPI ID
> GOOG0002 that is recognized by cros_kbd_led_backlight and they also
> don't use device tree.
Something I'd wonder is if the GOOG0002 ACPI ID can go away entirely
with this type of change. Presumably the Chromebooks with ChromeOS EC
/also/ advertise EC_FEATURE_PWM_KEYB.
>
> Extend the cros_ec MFD device to also load cros_kbd_led_backlight
> when the EC reports EC_FEATURE_PWM_KEYB.
>
> Tested on a Framework 13 AMD, Bios 3.05.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> This is based on
> https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
>
> The helper keyboard_led_is_mfd_device is a bit iffy, but I couldn't find
> a nicer way.
>
> * driver_data from platform_device_id is overwritten by the mfd platform data
> * Setting the driver_data in drivers/mfd/cros_ec_dev.c would expose the
> internals of cros_kbd_led_backlight
> ---
> drivers/mfd/cros_ec_dev.c | 9 ++++++
> drivers/platform/chrome/cros_kbd_led_backlight.c | 41 +++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index a52d59cc2b1e..4444b361aeae 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_keyboard_leds_cells[] = {
> + { .name = "cros-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_keyboard_leds_cells,
> + .num_cells = ARRAY_SIZE(cros_ec_keyboard_leds_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 b83e4f328620..88dd3848e5da 100644
> --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> @@ -194,13 +194,52 @@ static const __maybe_unused struct keyboard_led_drvdata keyboard_led_drvdata_ec_
>
> #endif /* IS_ENABLED(CONFIG_CROS_EC) */
>
> +#if IS_ENABLED(CONFIG_MFD_CROS_EC_DEV)
> +static int keyboard_led_init_ec_pwm_mfd(struct platform_device *pdev)
> +{
> + struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
> + struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> + struct keyboard_led *keyboard_led = platform_get_drvdata(pdev);
> +
> + keyboard_led->ec = cros_ec;
> +
> + return 0;
> +}
> +
> +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm_mfd = {
> + .init = keyboard_led_init_ec_pwm_mfd,
> + .brightness_set_blocking = keyboard_led_set_brightness_ec_pwm,
> + .brightness_get = keyboard_led_get_brightness_ec_pwm,
> + .max_brightness = KEYBOARD_BACKLIGHT_MAX,
> +};
> +
> +#else /* IS_ENABLED(CONFIG_MFD_CROS_EC_DEV) */
> +
> +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {};
> +
> +#endif /* IS_ENABLED(CONFIG_MFD_CROS_EC_DEV) */
> +
> +static int keyboard_led_is_mfd_device(struct platform_device *pdev)
> +{
> + if (!IS_ENABLED(CONFIG_MFD_CROS_EC_DEV))
> + return 0;
> +
> + if (!pdev->dev.parent)
> + return 0;
> +
> + return strcmp(pdev->dev.parent->driver->name, "cros-ec-dev") == 0;
> +}
> +
> static int keyboard_led_probe(struct platform_device *pdev)
> {
> const struct keyboard_led_drvdata *drvdata;
> struct keyboard_led *keyboard_led;
> int error;
>
> - drvdata = device_get_match_data(&pdev->dev);
> + if (keyboard_led_is_mfd_device(pdev))
> + drvdata = &keyboard_led_drvdata_ec_pwm_mfd;
> + else
> + drvdata = device_get_match_data(&pdev->dev);
> if (!drvdata)
> return -EINVAL;
>
>
> ---
> base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
> change-id: 20240505-cros_ec-kbd-led-framework-7e2e831bc79c
>
> Best regards,
On 2024-05-05 08:42:21+0000, Mario Limonciello wrote: > On 5/5/2024 04:41, Thomas Weißschuh wrote: > > The ChromeOS EC used in Framework laptops supports the standard cros > > keyboard backlight protocol. > > However the firmware on these laptops don't implement the ACPI ID > > GOOG0002 that is recognized by cros_kbd_led_backlight and they also > > don't use device tree. > > Something I'd wonder is if the GOOG0002 ACPI ID can go away entirely with > this type of change. Presumably the Chromebooks with ChromeOS EC /also/ > advertise EC_FEATURE_PWM_KEYB. Sounds good to me in general. It would make the code cleaner. But I have no idea how CrOS kernels are set up in general. If they are not using CONFIG_MFD_CROS_EC_DEV for some reason that wouldn't work. If the CrOS folks agree with that aproach I'll be happy to implement it. > <snip> Thomas
On Mon, May 06, 2024 at 07:38:09PM +0200, Thomas Weißschuh wrote: > On 2024-05-05 08:42:21+0000, Mario Limonciello wrote: > > On 5/5/2024 04:41, Thomas Weißschuh wrote: > > > The ChromeOS EC used in Framework laptops supports the standard cros > > > keyboard backlight protocol. > > > However the firmware on these laptops don't implement the ACPI ID > > > GOOG0002 that is recognized by cros_kbd_led_backlight and they also > > > don't use device tree. If implementing ACPI ID GOOG0002 is not an option, how about adding a new ACPI ID? For the new ACPI ID, it can use EC PWM for setting the brightness. > > Something I'd wonder is if the GOOG0002 ACPI ID can go away entirely with > > this type of change. Presumably the Chromebooks with ChromeOS EC /also/ > > advertise EC_FEATURE_PWM_KEYB. > > Sounds good to me in general. It would make the code cleaner. > > But I have no idea how CrOS kernels are set up in general. > If they are not using CONFIG_MFD_CROS_EC_DEV for some reason that > wouldn't work. > > If the CrOS folks agree with that aproach I'll be happy to implement it. I would say NO as some existing devices (with legacy firmware and kernel) may rely on it.
On 2024-05-09 12:25:01+0000, Tzung-Bi Shih wrote: > On Mon, May 06, 2024 at 07:38:09PM +0200, Thomas Weißschuh wrote: > > On 2024-05-05 08:42:21+0000, Mario Limonciello wrote: > > > On 5/5/2024 04:41, Thomas Weißschuh wrote: > > > > The ChromeOS EC used in Framework laptops supports the standard cros > > > > keyboard backlight protocol. > > > > However the firmware on these laptops don't implement the ACPI ID > > > > GOOG0002 that is recognized by cros_kbd_led_backlight and they also > > > > don't use device tree. > > If implementing ACPI ID GOOG0002 is not an option, how about adding a new ACPI > ID? For the new ACPI ID, it can use EC PWM for setting the brightness. Adding a new ACPI ID would be easier than a full-blown ACPI interface. This would still need changes to the drivers probing setup, however. What are the advantages of the ACPI ID aproach over EC_FEATURE_PWM_KEYB? The EC feature also automatically works on device-tree platforms and without any work from system vendors. Adding ACPI ID only for signalling without using ACPI for communication on the other hand seems weird. Also with MFD the device hierarchy is much better. > > > Something I'd wonder is if the GOOG0002 ACPI ID can go away entirely with > > > this type of change. Presumably the Chromebooks with ChromeOS EC /also/ > > > advertise EC_FEATURE_PWM_KEYB. > > > > Sounds good to me in general. It would make the code cleaner. > > > > But I have no idea how CrOS kernels are set up in general. > > If they are not using CONFIG_MFD_CROS_EC_DEV for some reason that > > wouldn't work. > > > > If the CrOS folks agree with that aproach I'll be happy to implement it. > > I would say NO as some existing devices (with legacy firmware and kernel) may > rely on it. Ack, makes sense. You mention legacy kernels, but these would not be affected.
On Thu, May 09, 2024 at 10:13:37AM +0200, Thomas Weißschuh wrote: > On 2024-05-09 12:25:01+0000, Tzung-Bi Shih wrote: > > On Mon, May 06, 2024 at 07:38:09PM +0200, Thomas Weißschuh wrote: > > > On 2024-05-05 08:42:21+0000, Mario Limonciello wrote: > > > > On 5/5/2024 04:41, Thomas Weißschuh wrote: > > > > > The ChromeOS EC used in Framework laptops supports the standard cros > > > > > keyboard backlight protocol. > > > > > However the firmware on these laptops don't implement the ACPI ID > > > > > GOOG0002 that is recognized by cros_kbd_led_backlight and they also > > > > > don't use device tree. > > > > If implementing ACPI ID GOOG0002 is not an option, how about adding a new ACPI > > ID? For the new ACPI ID, it can use EC PWM for setting the brightness. > > Adding a new ACPI ID would be easier than a full-blown ACPI interface. > This would still need changes to the drivers probing setup, however. > > What are the advantages of the ACPI ID aproach over EC_FEATURE_PWM_KEYB? > The EC feature also automatically works on device-tree platforms and > without any work from system vendors. Perhaps no advantages but just following its original design. The driver uses ACPI table for matching devices since it appears. We shouldn't remove the ACPI matching anyway as some existing devices may rely on it. In addition, adding a new ACPI ID sounds more reasonable than using keyboard_led_is_mfd_device() to me. > Adding ACPI ID only for signalling without using ACPI for > communication on the other hand seems weird. I have a different view: using a new ACPI ID and another driver data fits current framework better. I'm not sure if the reason is strong enough for applying a new ACPI ID though. We could wait to see if others in the mailing list may have more inputs. > > > > Something I'd wonder is if the GOOG0002 ACPI ID can go away entirely with > > > > this type of change. Presumably the Chromebooks with ChromeOS EC /also/ > > > > advertise EC_FEATURE_PWM_KEYB. > > > > > > Sounds good to me in general. It would make the code cleaner. > > > > > > But I have no idea how CrOS kernels are set up in general. > > > If they are not using CONFIG_MFD_CROS_EC_DEV for some reason that > > > wouldn't work. > > > > > > If the CrOS folks agree with that aproach I'll be happy to implement it. > > > > I would say NO as some existing devices (with legacy firmware and kernel) may > > rely on it. > > Ack, makes sense. > > You mention legacy kernels, but these would not be affected. We never know if a device would run legacy firmware with new kernel in some day.
© 2016 - 2024 Red Hat, Inc.