The series adds EC PWM as an backend option for ChromeOS keyboard LED
backlight.
The 1st patch reorder the headers alphabetically.
The 2nd patch separates the ACPI backend as an independent option.
The 3rd patch is the DT binding document for the proposed compatible string.
The 4th patch supports OF match.
The 5th patch adds EC PWM as another backend option.
Changes from v2:
(https://patchwork.kernel.org/project/chrome-platform/cover/[email protected]/)
- Fix per review comments.
Changes from v1:
(https://patchwork.kernel.org/project/chrome-platform/cover/[email protected]/)
- Update email address accordingly.
Tzung-Bi Shih (5):
platform/chrome: cros_kbd_led_backlight: sort headers alphabetically
platform/chrome: cros_kbd_led_backlight: separate ACPI backend
dt-bindings: add google,cros-kbd-led-backlight
platform/chrome: cros_kbd_led_backlight: support OF match
platform/chrome: cros_kbd_led_backlight: support EC PWM backend
.../chrome/google,cros-kbd-led-backlight.yaml | 35 +++
.../bindings/mfd/google,cros-ec.yaml | 3 +
drivers/platform/chrome/Kconfig | 14 +-
.../platform/chrome/cros_kbd_led_backlight.c | 220 ++++++++++++++++--
4 files changed, 249 insertions(+), 23 deletions(-)
create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-kbd-led-backlight.yaml
--
2.35.1.894.gb6a874cedc-goog
For letting device tree based machines to use the driver, support OF match.
Reviewed-by: Guenter Roeck <[email protected]>
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v2:
- Add commit message.
- Add R-b tag.
Changes from v1:
(https://patchwork.kernel.org/project/chrome-platform/patch/[email protected]/)
- Update email address accordingly.
- Use device_get_match_data() per review comment in v1.
drivers/platform/chrome/cros_kbd_led_backlight.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
index 05b4f274086b..5cbe27cb4610 100644
--- a/drivers/platform/chrome/cros_kbd_led_backlight.c
+++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
@@ -10,7 +10,9 @@
#include <linux/kernel.h>
#include <linux/leds.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
/**
@@ -127,7 +129,7 @@ static int keyboard_led_probe(struct platform_device *pdev)
const struct keyboard_led_drvdata *drvdata;
int error;
- drvdata = acpi_device_get_match_data(&pdev->dev);
+ drvdata = device_get_match_data(&pdev->dev);
if (!drvdata)
return -EINVAL;
@@ -163,10 +165,21 @@ 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",
+ },
+ {}
+};
+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.35.1.894.gb6a874cedc-goog
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
No changes from v2.
Changes from v1:
(https://patchwork.kernel.org/project/chrome-platform/patch/[email protected]/)
- Update email address accordingly.
- Add A-b tag.
.../chrome/google,cros-kbd-led-backlight.yaml | 35 +++++++++++++++++++
.../bindings/mfd/google,cros-ec.yaml | 3 ++
2 files changed, 38 insertions(+)
create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-kbd-led-backlight.yaml
diff --git a/Documentation/devicetree/bindings/chrome/google,cros-kbd-led-backlight.yaml b/Documentation/devicetree/bindings/chrome/google,cros-kbd-led-backlight.yaml
new file mode 100644
index 000000000000..5b875af6a95a
--- /dev/null
+++ b/Documentation/devicetree/bindings/chrome/google,cros-kbd-led-backlight.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/chrome/google,cros-kbd-led-backlight.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ChromeOS keyboard backlight LED driver.
+
+maintainers:
+ - Tzung-Bi Shih <[email protected]>
+
+properties:
+ compatible:
+ const: google,cros-kbd-led-backlight
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ spi0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cros_ec: ec@0 {
+ compatible = "google,cros-ec-spi";
+ reg = <0>;
+
+ kbd-led-backlight {
+ compatible = "google,cros-kbd-led-backlight";
+ };
+ };
+ };
diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
index d1f53bd449f7..1815ca0e8ebc 100644
--- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
+++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
@@ -90,6 +90,9 @@ properties:
ec-pwm:
$ref: "/schemas/pwm/google,cros-ec-pwm.yaml#"
+ kbd-led-backlight:
+ $ref: "/schemas/chrome/google,cros-kbd-led-backlight.yaml#"
+
keyboard-controller:
$ref: "/schemas/input/google,cros-ec-keyb.yaml#"
--
2.35.1.894.gb6a874cedc-goog
EC PWM backend uses EC_CMD_PWM_SET_KEYBOARD_BACKLIGHT and
EC_CMD_PWM_GET_KEYBOARD_BACKLIGHT for setting and getting the brightness
respectively.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v2:
- Turn CROS_KBD_LED_BACKLIGHT_EC_PWM to boolean.
- Use #ifdef for boolean CROS_KBD_LED_BACKLIGHT_EC_PWM.
Changes from v1:
- Update email address accordingly.
drivers/platform/chrome/Kconfig | 6 +
.../platform/chrome/cros_kbd_led_backlight.c | 126 +++++++++++++++---
2 files changed, 117 insertions(+), 15 deletions(-)
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 3f74679a556c..e02789d7c0d4 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -142,6 +142,12 @@ config CROS_KBD_LED_BACKLIGHT_ACPI
help
ChromeOS keyboard backlight ACPI backend.
+config CROS_KBD_LED_BACKLIGHT_EC_PWM
+ bool "ChromeOS keyboard backlight EC PWM backend"
+ depends on CROS_EC && CROS_KBD_LED_BACKLIGHT
+ help
+ ChromeOS keyboard backlight EC PWM backend.
+
config CROS_EC_CHARDEV
tristate "ChromeOS EC miscdevice"
depends on MFD_CROS_EC_DEV
diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
index 5cbe27cb4610..8c35dd2fa607 100644
--- a/drivers/platform/chrome/cros_kbd_led_backlight.c
+++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
@@ -11,10 +11,17 @@
#include <linux/leds.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
#include <linux/platform_device.h>
#include <linux/property.h>
#include <linux/slab.h>
+struct keyboard_led_private {
+ struct led_classdev cdev;
+ struct cros_ec_device *ec;
+};
+
/**
* struct keyboard_led_drvdata - keyboard LED driver data.
* @init: Init function.
@@ -40,6 +47,8 @@ struct keyboard_led_drvdata {
enum led_brightness max_brightness;
};
+#define KEYBOARD_BACKLIGHT_MAX 100
+
#ifdef CONFIG_CROS_KBD_LED_BACKLIGHT_ACPI
/* Keyboard LED ACPI Device must be defined in firmware */
@@ -47,8 +56,6 @@ struct keyboard_led_drvdata {
#define ACPI_KEYBOARD_BACKLIGHT_READ ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBQC"
#define ACPI_KEYBOARD_BACKLIGHT_WRITE ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBCM"
-#define ACPI_KEYBOARD_BACKLIGHT_MAX 100
-
static void keyboard_led_set_brightness_acpi(struct led_classdev *cdev,
enum led_brightness brightness)
{
@@ -107,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 = ACPI_KEYBOARD_BACKLIGHT_MAX,
+ .max_brightness = KEYBOARD_BACKLIGHT_MAX,
};
#else /* CONFIG_CROS_KBD_LED_BACKLIGHT_ACPI */
@@ -123,34 +130,122 @@ static const struct keyboard_led_drvdata keyboard_led_drvdata_acpi = {
#endif /* CONFIG_CROS_KBD_LED_BACKLIGHT_ACPI */
+#ifdef CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM
+
+static int
+keyboard_led_set_brightness_blocking_ec_pwm(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ struct {
+ struct cros_ec_command msg;
+ struct ec_params_pwm_set_keyboard_backlight params;
+ } __packed buf;
+ struct ec_params_pwm_set_keyboard_backlight *params = &buf.params;
+ struct cros_ec_command *msg = &buf.msg;
+ struct keyboard_led_private *private =
+ container_of(cdev, struct keyboard_led_private, cdev);
+
+ memset(&buf, 0, sizeof(buf));
+
+ msg->version = 0;
+ msg->command = EC_CMD_PWM_SET_KEYBOARD_BACKLIGHT;
+ msg->insize = 0;
+ msg->outsize = sizeof(*params);
+
+ params->percent = brightness;
+
+ return cros_ec_cmd_xfer_status(private->ec, msg);
+}
+
+static enum led_brightness
+keyboard_led_get_brightness_ec_pwm(struct led_classdev *cdev)
+{
+ struct {
+ struct cros_ec_command msg;
+ struct ec_response_pwm_get_keyboard_backlight resp;
+ } __packed buf;
+ struct ec_response_pwm_get_keyboard_backlight *resp = &buf.resp;
+ struct cros_ec_command *msg = &buf.msg;
+ struct keyboard_led_private *private =
+ container_of(cdev, struct keyboard_led_private, cdev);
+ int ret;
+
+ memset(&buf, 0, sizeof(buf));
+
+ msg->version = 0;
+ msg->command = EC_CMD_PWM_GET_KEYBOARD_BACKLIGHT;
+ msg->insize = sizeof(*resp);
+ msg->outsize = 0;
+
+ ret = cros_ec_cmd_xfer_status(private->ec, msg);
+ if (ret < 0)
+ return ret;
+
+ return resp->percent;
+}
+
+static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
+{
+ struct keyboard_led_private *private = platform_get_drvdata(pdev);
+
+ private->ec = dev_get_drvdata(pdev->dev.parent);
+ if (!private->ec) {
+ dev_err(&pdev->dev, "no parent EC device\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
+ .init = keyboard_led_init_ec_pwm,
+ .brightness_set_blocking = keyboard_led_set_brightness_blocking_ec_pwm,
+ .brightness_get = keyboard_led_get_brightness_ec_pwm,
+ .max_brightness = KEYBOARD_BACKLIGHT_MAX,
+};
+
+#else /* CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM */
+
+static int keyboard_led_init_ec_pwm_null(struct platform_device *pdev)
+{
+ return -EOPNOTSUPP;
+}
+
+static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
+ .init = keyboard_led_init_ec_pwm_null,
+};
+
+#endif /* CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM */
+
static int keyboard_led_probe(struct platform_device *pdev)
{
- struct led_classdev *cdev;
const struct keyboard_led_drvdata *drvdata;
+ struct keyboard_led_private *private;
int error;
drvdata = device_get_match_data(&pdev->dev);
if (!drvdata)
return -EINVAL;
+ private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL);
+ if (!private)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, private);
+
if (drvdata->init) {
error = drvdata->init(pdev);
if (error)
return error;
}
- cdev = devm_kzalloc(&pdev->dev, sizeof(*cdev), GFP_KERNEL);
- if (!cdev)
- return -ENOMEM;
-
- cdev->name = "chromeos::kbd_backlight";
- cdev->flags |= LED_CORE_SUSPENDRESUME;
- cdev->max_brightness = drvdata->max_brightness;
- cdev->brightness_set = drvdata->brightness_set;
- cdev->brightness_set_blocking = drvdata->brightness_set_blocking;
- cdev->brightness_get = drvdata->brightness_get;
+ private->cdev.name = "chromeos::kbd_backlight";
+ private->cdev.flags |= LED_CORE_SUSPENDRESUME;
+ private->cdev.max_brightness = drvdata->max_brightness;
+ private->cdev.brightness_set = drvdata->brightness_set;
+ private->cdev.brightness_set_blocking = drvdata->brightness_set_blocking;
+ private->cdev.brightness_get = drvdata->brightness_get;
- error = devm_led_classdev_register(&pdev->dev, cdev);
+ error = devm_led_classdev_register(&pdev->dev, &private->cdev);
if (error)
return error;
@@ -169,6 +264,7 @@ MODULE_DEVICE_TABLE(acpi, keyboard_led_acpi_match);
static const struct of_device_id keyboard_led_of_match[] = {
{
.compatible = "google,cros-kbd-led-backlight",
+ .data = &keyboard_led_drvdata_ec_pwm,
},
{}
};
--
2.35.1.894.gb6a874cedc-goog
On Mon, Mar 21, 2022 at 04:55:46PM +0800, Tzung-Bi Shih wrote:
> For letting device tree based machines to use the driver, support OF match.
>
> Reviewed-by: Guenter Roeck <[email protected]>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
> Changes from v2:
> - Add commit message.
> - Add R-b tag.
>
> Changes from v1:
> (https://patchwork.kernel.org/project/chrome-platform/patch/[email protected]/)
> - Update email address accordingly.
> - Use device_get_match_data() per review comment in v1.
>
> drivers/platform/chrome/cros_kbd_led_backlight.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
> index 05b4f274086b..5cbe27cb4610 100644
> --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> @@ -10,7 +10,9 @@
> #include <linux/kernel.h>
> #include <linux/leds.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
>
> /**
> @@ -127,7 +129,7 @@ static int keyboard_led_probe(struct platform_device *pdev)
> const struct keyboard_led_drvdata *drvdata;
> int error;
>
> - drvdata = acpi_device_get_match_data(&pdev->dev);
> + drvdata = device_get_match_data(&pdev->dev);
> if (!drvdata)
> return -EINVAL;
>
> @@ -163,10 +165,21 @@ 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",
> + },
> + {}
> +};
> +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),
In patch "[5/5] platform/chrome: cros_kbd_led_backlight: support EC PWM
backend" [1] you create the 'stubs' keyboard_led_init_ec_pwm_null() and
keyboard_led_drvdata_ec_pwm when CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM
isn't set, only to have something to assign to keyboard_led_of_match.data.
Instead you could assign .of_match_table only if CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM
is set.
[1] https://patchwork.kernel.org/project/chrome-platform/patch/[email protected]/
On Mon, Mar 21, 2022 at 04:55:47PM +0800, Tzung-Bi Shih wrote:
> EC PWM backend uses EC_CMD_PWM_SET_KEYBOARD_BACKLIGHT and
> EC_CMD_PWM_GET_KEYBOARD_BACKLIGHT for setting and getting the brightness
> respectively.
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
> Changes from v2:
> - Turn CROS_KBD_LED_BACKLIGHT_EC_PWM to boolean.
> - Use #ifdef for boolean CROS_KBD_LED_BACKLIGHT_EC_PWM.
>
> Changes from v1:
> - Update email address accordingly.
>
> drivers/platform/chrome/Kconfig | 6 +
> .../platform/chrome/cros_kbd_led_backlight.c | 126 +++++++++++++++---
> 2 files changed, 117 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 3f74679a556c..e02789d7c0d4 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -142,6 +142,12 @@ config CROS_KBD_LED_BACKLIGHT_ACPI
> help
> ChromeOS keyboard backlight ACPI backend.
>
> +config CROS_KBD_LED_BACKLIGHT_EC_PWM
> + bool "ChromeOS keyboard backlight EC PWM backend"
> + depends on CROS_EC && CROS_KBD_LED_BACKLIGHT
> + help
> + ChromeOS keyboard backlight EC PWM backend.
> +
> config CROS_EC_CHARDEV
> tristate "ChromeOS EC miscdevice"
> depends on MFD_CROS_EC_DEV
> diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
> index 5cbe27cb4610..8c35dd2fa607 100644
> --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> @@ -11,10 +11,17 @@
> #include <linux/leds.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/property.h>
> #include <linux/slab.h>
>
> +struct keyboard_led_private {
Why 'private', isn't this more a 'cros_ec_kdb_bl' or similar?
> + struct led_classdev cdev;
> + struct cros_ec_device *ec;
> +};
> +
> /**
> * struct keyboard_led_drvdata - keyboard LED driver data.
> * @init: Init function.
> @@ -40,6 +47,8 @@ struct keyboard_led_drvdata {
> enum led_brightness max_brightness;
> };
>
> +#define KEYBOARD_BACKLIGHT_MAX 100
> +
> #ifdef CONFIG_CROS_KBD_LED_BACKLIGHT_ACPI
>
> /* Keyboard LED ACPI Device must be defined in firmware */
> @@ -47,8 +56,6 @@ struct keyboard_led_drvdata {
> #define ACPI_KEYBOARD_BACKLIGHT_READ ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBQC"
> #define ACPI_KEYBOARD_BACKLIGHT_WRITE ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBCM"
>
> -#define ACPI_KEYBOARD_BACKLIGHT_MAX 100
> -
> static void keyboard_led_set_brightness_acpi(struct led_classdev *cdev,
> enum led_brightness brightness)
> {
> @@ -107,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 = ACPI_KEYBOARD_BACKLIGHT_MAX,
> + .max_brightness = KEYBOARD_BACKLIGHT_MAX,
> };
>
> #else /* CONFIG_CROS_KBD_LED_BACKLIGHT_ACPI */
> @@ -123,34 +130,122 @@ static const struct keyboard_led_drvdata keyboard_led_drvdata_acpi = {
>
> #endif /* CONFIG_CROS_KBD_LED_BACKLIGHT_ACPI */
>
> +#ifdef CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM
> +
> +static int
> +keyboard_led_set_brightness_blocking_ec_pwm(struct led_classdev *cdev,
> + enum led_brightness brightness)
nit: since there is only a blocking version of .set_brightness you could omit
'blocking' in the function name.
> +{
> + struct {
> + struct cros_ec_command msg;
> + struct ec_params_pwm_set_keyboard_backlight params;
> + } __packed buf;
> + struct ec_params_pwm_set_keyboard_backlight *params = &buf.params;
> + struct cros_ec_command *msg = &buf.msg;
> + struct keyboard_led_private *private =
> + container_of(cdev, struct keyboard_led_private, cdev);
> +
> + memset(&buf, 0, sizeof(buf));
> +
> + msg->version = 0;
not strictly needed since you do the memset above, I guess it's
fine to keep the assignment if you want to be explicit about the
version.
> + msg->command = EC_CMD_PWM_SET_KEYBOARD_BACKLIGHT;
> + msg->insize = 0;
> + msg->outsize = sizeof(*params);
> +
> + params->percent = brightness;
> +
> + return cros_ec_cmd_xfer_status(private->ec, msg);
> +}
> +
> +static enum led_brightness
> +keyboard_led_get_brightness_ec_pwm(struct led_classdev *cdev)
> +{
> + struct {
> + struct cros_ec_command msg;
> + struct ec_response_pwm_get_keyboard_backlight resp;
> + } __packed buf;
> + struct ec_response_pwm_get_keyboard_backlight *resp = &buf.resp;
> + struct cros_ec_command *msg = &buf.msg;
> + struct keyboard_led_private *private =
> + container_of(cdev, struct keyboard_led_private, cdev);
> + int ret;
> +
> + memset(&buf, 0, sizeof(buf));
> +
> + msg->version = 0;
> + msg->command = EC_CMD_PWM_GET_KEYBOARD_BACKLIGHT;
> + msg->insize = sizeof(*resp);
> + msg->outsize = 0;
> +
> + ret = cros_ec_cmd_xfer_status(private->ec, msg);
> + if (ret < 0)
> + return ret;
> +
> + return resp->percent;
> +}
> +
> +static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
> +{
> + struct keyboard_led_private *private = platform_get_drvdata(pdev);
> +
> + private->ec = dev_get_drvdata(pdev->dev.parent);
> + if (!private->ec) {
> + dev_err(&pdev->dev, "no parent EC device\n");
> + return -EINVAL;
> + }
The only thing this 'init' function does is assigning private->ec. Wouldn't
it be clearer to do this directly in probe() from where callback is called?
It could be with the condition that the device as a DT node.
Is it actually possible that the keyboard backlight device gets instantiated
if there is no EC parent?
> +
> + return 0;
> +}
> +
> +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
> + .init = keyboard_led_init_ec_pwm,
> + .brightness_set_blocking = keyboard_led_set_brightness_blocking_ec_pwm,
> + .brightness_get = keyboard_led_get_brightness_ec_pwm,
> + .max_brightness = KEYBOARD_BACKLIGHT_MAX,
> +};
> +
> +#else /* CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM */
> +
> +static int keyboard_led_init_ec_pwm_null(struct platform_device *pdev)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
> + .init = keyboard_led_init_ec_pwm_null,
Is this really needed?
keyboard_led_probe() checks if .init is assigned before invoking the callback:
if (drvdata->init) {
error = drvdata->init(pdev);
The whole 'else' branch could be eliminated if .of_match_table of the driver
only is assigned when CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is set. IMO that
would preferable over creating 'stubs'.
> +};
> +
> +#endif /* CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM */
> +
> static int keyboard_led_probe(struct platform_device *pdev)
> {
> - struct led_classdev *cdev;
> const struct keyboard_led_drvdata *drvdata;
> + struct keyboard_led_private *private;
> int error;
>
> drvdata = device_get_match_data(&pdev->dev);
> if (!drvdata)
> return -EINVAL;
>
> + private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL);
> + if (!private)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, private);
> +
> if (drvdata->init) {
> error = drvdata->init(pdev);
> if (error)
> return error;
> }
>
> - cdev = devm_kzalloc(&pdev->dev, sizeof(*cdev), GFP_KERNEL);
> - if (!cdev)
> - return -ENOMEM;
> -
> - cdev->name = "chromeos::kbd_backlight";
> - cdev->flags |= LED_CORE_SUSPENDRESUME;
> - cdev->max_brightness = drvdata->max_brightness;
> - cdev->brightness_set = drvdata->brightness_set;
> - cdev->brightness_set_blocking = drvdata->brightness_set_blocking;
> - cdev->brightness_get = drvdata->brightness_get;
> + private->cdev.name = "chromeos::kbd_backlight";
> + private->cdev.flags |= LED_CORE_SUSPENDRESUME;
> + private->cdev.max_brightness = drvdata->max_brightness;
> + private->cdev.brightness_set = drvdata->brightness_set;
> + private->cdev.brightness_set_blocking = drvdata->brightness_set_blocking;
> + private->cdev.brightness_get = drvdata->brightness_get;
>
> - error = devm_led_classdev_register(&pdev->dev, cdev);
> + error = devm_led_classdev_register(&pdev->dev, &private->cdev);
> if (error)
> return error;
>
> @@ -169,6 +264,7 @@ MODULE_DEVICE_TABLE(acpi, keyboard_led_acpi_match);
> static const struct of_device_id keyboard_led_of_match[] = {
> {
> .compatible = "google,cros-kbd-led-backlight",
> + .data = &keyboard_led_drvdata_ec_pwm,
> },
> {}
> };
> --
> 2.35.1.894.gb6a874cedc-goog
>
On Fri, May 20, 2022 at 12:53:20PM +0800, Tzung-Bi Shih wrote:
> On Thu, May 19, 2022 at 03:40:21PM -0700, Matthias Kaehlcke wrote:
> > On Mon, Mar 21, 2022 at 04:55:47PM +0800, Tzung-Bi Shih wrote:
> > > +struct keyboard_led_private {
> >
> > Why 'private', isn't this more a 'cros_ec_kdb_bl' or similar?
>
> It is just drvdata.
The data structure represents an instance of the device, as such it
is an important part of the driver, drvdata is just a way to attach
it to the platform device.
> I would prefer to keep the original prefix "keyboard_led_" if you wouldn't
> have strong opinion.
I'm fine with 'keyboard_led', but object to the 'private' part. In the
kernel 'private' fields are typically used when a driver consists of a
generic part and a device specific part. The driver has a 'private'
void* field that points to a device specific data structure about which
the generic driver is agnostic. This data structure is only used by the
device specific implementation. That isn't the case here, so naming the
structure anything 'private' is misleading.
> > > +static int
> > > +keyboard_led_set_brightness_blocking_ec_pwm(struct led_classdev *cdev,
> > > + enum led_brightness brightness)
> >
> > nit: since there is only a blocking version of .set_brightness you could omit
> > 'blocking' in the function name.
>
> Ack, will fix it in next version.
>
> > > + struct {
> > > + struct cros_ec_command msg;
> > > + struct ec_params_pwm_set_keyboard_backlight params;
> > > + } __packed buf;
> > > + struct ec_params_pwm_set_keyboard_backlight *params = &buf.params;
> > > + struct cros_ec_command *msg = &buf.msg;
> > > + struct keyboard_led_private *private =
Continuation of the argument above: the variable name 'private' doesn't
reveal anything about it's nature, something like 'kbd_led' would be much
clearer.
> > > + container_of(cdev, struct keyboard_led_private, cdev);
> > > +
> > > + memset(&buf, 0, sizeof(buf));
> > > +
> > > + msg->version = 0;
> >
> > not strictly needed since you do the memset above, I guess it's
> > fine to keep the assignment if you want to be explicit about the
> > version.
>
> Ack, let's remove them in next version.
>
> > > +static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
> > > +{
> > > + struct keyboard_led_private *private = platform_get_drvdata(pdev);
> > > +
> > > + private->ec = dev_get_drvdata(pdev->dev.parent);
> > > + if (!private->ec) {
> > > + dev_err(&pdev->dev, "no parent EC device\n");
> > > + return -EINVAL;
> > > + }
> >
> > The only thing this 'init' function does is assigning private->ec. Wouldn't
> > it be clearer to do this directly in probe() from where callback is called?
> > It could be with the condition that the device as a DT node.
>
> No. The probe() isn't aware of the device is from ACPI or OF.
But it could be:
if (pdev->dev.of_node)
kbd_led->ec = dev_get_drvdata(pdev->dev.parent);
> > Is it actually possible that the keyboard backlight device gets instantiated
> > if there is no EC parent?
>
> It shouldn't be but just in case.
If this can only occur due to an error in common kernel frameworks then
the check should be omitted IMO.
> > > +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
> > > + .init = keyboard_led_init_ec_pwm_null,
> >
> > Is this really needed?
> >
> > keyboard_led_probe() checks if .init is assigned before invoking the callback:
> >
> > if (drvdata->init) {
> > error = drvdata->init(pdev);
> >
> > The whole 'else' branch could be eliminated if .of_match_table of the driver
> > only is assigned when CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is set. IMO that
> > would preferable over creating 'stubs'.
>
> CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent. The stubs
> were created to avoid compile errors if CONFIG_OF=y but
> CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n.
Is there functional version of the driver that uses instantiation through the
device tree if CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n? If not .of_match_table
should not be assigned.
> However, I just realized it could also have compile errors if CONFIG_OF=n and
> CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=y. The `keyboard_led_drvdata_ec_pwm` is
> unused.
>
> In any case, I agree with you. Let's remove the stubs in next version. I
> would use __maybe_unused for some of them.
On Fri, May 20, 2022 at 08:29:19AM -0700, Matthias Kaehlcke wrote:
> On Fri, May 20, 2022 at 12:53:20PM +0800, Tzung-Bi Shih wrote:
> > On Thu, May 19, 2022 at 03:40:21PM -0700, Matthias Kaehlcke wrote:
> > > On Mon, Mar 21, 2022 at 04:55:47PM +0800, Tzung-Bi Shih wrote:
> > > > +struct keyboard_led_private {
> > >
> > > Why 'private', isn't this more a 'cros_ec_kdb_bl' or similar?
> >
> > It is just drvdata.
>
> The data structure represents an instance of the device, as such it
> is an important part of the driver, drvdata is just a way to attach
> it to the platform device.
>
> > I would prefer to keep the original prefix "keyboard_led_" if you wouldn't
> > have strong opinion.
>
> I'm fine with 'keyboard_led', but object to the 'private' part. In the
> kernel 'private' fields are typically used when a driver consists of a
> generic part and a device specific part. The driver has a 'private'
> void* field that points to a device specific data structure about which
> the generic driver is agnostic. This data structure is only used by the
> device specific implementation. That isn't the case here, so naming the
> structure anything 'private' is misleading.
The struct in the case is device specific. I don't see a problem to name it
*private* as there are a lot of more existing examples.
$ grep -R 'struct .*_priv.* {' drivers/
drivers/pinctrl/bcm/pinctrl-bcm6358.c:struct bcm6358_priv {
$ grep -R 'struct .*_priv.* {' sound/soc/codecs/
sound/soc/codecs/rt286.c:struct rt286_priv {
I would get rid of the term "private" if it could be confusing.
> > > > +static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
> > > > +{
> > > > + struct keyboard_led_private *private = platform_get_drvdata(pdev);
> > > > +
> > > > + private->ec = dev_get_drvdata(pdev->dev.parent);
> > > > + if (!private->ec) {
> > > > + dev_err(&pdev->dev, "no parent EC device\n");
> > > > + return -EINVAL;
> > > > + }
> > >
> > > The only thing this 'init' function does is assigning private->ec. Wouldn't
> > > it be clearer to do this directly in probe() from where callback is called?
> > > It could be with the condition that the device as a DT node.
> >
> > No. The probe() isn't aware of the device is from ACPI or OF.
>
> But it could be:
>
> if (pdev->dev.of_node)
> kbd_led->ec = dev_get_drvdata(pdev->dev.parent);
The 'init' callback isn't only for OF but also ACPI. I would prefer to keep
the 'init' function and let probe() have no awareness about them.
> > > Is it actually possible that the keyboard backlight device gets instantiated
> > > if there is no EC parent?
> >
> > It shouldn't be but just in case.
>
> If this can only occur due to an error in common kernel frameworks then
> the check should be omitted IMO.
The check is referenced from [1]. I would prefer to keep it instead of
crashing kernel if anything went wrong.
[1]: https://elixir.bootlin.com/linux/v5.18-rc7/source/drivers/pwm/pwm-cros-ec.c#L244
>
> > > > +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
> > > > + .init = keyboard_led_init_ec_pwm_null,
> > >
> > > Is this really needed?
> > >
> > > keyboard_led_probe() checks if .init is assigned before invoking the callback:
> > >
> > > if (drvdata->init) {
> > > error = drvdata->init(pdev);
> > >
> > > The whole 'else' branch could be eliminated if .of_match_table of the driver
> > > only is assigned when CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is set. IMO that
> > > would preferable over creating 'stubs'.
> >
> > CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent. The stubs
> > were created to avoid compile errors if CONFIG_OF=y but
> > CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n.
>
> Is there functional version of the driver that uses instantiation through the
> device tree if CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n? If not .of_match_table
> should not be assigned.
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent.
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is also designed to work with CONFIG_ACPI.
On Thu, May 19, 2022 at 03:40:21PM -0700, Matthias Kaehlcke wrote:
> On Mon, Mar 21, 2022 at 04:55:47PM +0800, Tzung-Bi Shih wrote:
> > +struct keyboard_led_private {
>
> Why 'private', isn't this more a 'cros_ec_kdb_bl' or similar?
It is just drvdata. I would prefer to keep the original prefix
"keyboard_led_" if you wouldn't have strong opinion.
> > +static int
> > +keyboard_led_set_brightness_blocking_ec_pwm(struct led_classdev *cdev,
> > + enum led_brightness brightness)
>
> nit: since there is only a blocking version of .set_brightness you could omit
> 'blocking' in the function name.
Ack, will fix it in next version.
> > + struct {
> > + struct cros_ec_command msg;
> > + struct ec_params_pwm_set_keyboard_backlight params;
> > + } __packed buf;
> > + struct ec_params_pwm_set_keyboard_backlight *params = &buf.params;
> > + struct cros_ec_command *msg = &buf.msg;
> > + struct keyboard_led_private *private =
> > + container_of(cdev, struct keyboard_led_private, cdev);
> > +
> > + memset(&buf, 0, sizeof(buf));
> > +
> > + msg->version = 0;
>
> not strictly needed since you do the memset above, I guess it's
> fine to keep the assignment if you want to be explicit about the
> version.
Ack, let's remove them in next version.
> > +static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
> > +{
> > + struct keyboard_led_private *private = platform_get_drvdata(pdev);
> > +
> > + private->ec = dev_get_drvdata(pdev->dev.parent);
> > + if (!private->ec) {
> > + dev_err(&pdev->dev, "no parent EC device\n");
> > + return -EINVAL;
> > + }
>
> The only thing this 'init' function does is assigning private->ec. Wouldn't
> it be clearer to do this directly in probe() from where callback is called?
> It could be with the condition that the device as a DT node.
No. The probe() isn't aware of the device is from ACPI or OF.
> Is it actually possible that the keyboard backlight device gets instantiated
> if there is no EC parent?
It shouldn't be but just in case.
> > +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
> > + .init = keyboard_led_init_ec_pwm_null,
>
> Is this really needed?
>
> keyboard_led_probe() checks if .init is assigned before invoking the callback:
>
> if (drvdata->init) {
> error = drvdata->init(pdev);
>
> The whole 'else' branch could be eliminated if .of_match_table of the driver
> only is assigned when CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is set. IMO that
> would preferable over creating 'stubs'.
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent. The stubs
were created to avoid compile errors if CONFIG_OF=y but
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n.
However, I just realized it could also have compile errors if CONFIG_OF=n and
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=y. The `keyboard_led_drvdata_ec_pwm` is
unused.
In any case, I agree with you. Let's remove the stubs in next version. I
would use __maybe_unused for some of them.
On Mon, May 23, 2022 at 01:34:55PM +0800, Tzung-Bi Shih wrote:
> On Fri, May 20, 2022 at 08:29:19AM -0700, Matthias Kaehlcke wrote:
> > On Fri, May 20, 2022 at 12:53:20PM +0800, Tzung-Bi Shih wrote:
> > > On Thu, May 19, 2022 at 03:40:21PM -0700, Matthias Kaehlcke wrote:
> > > > On Mon, Mar 21, 2022 at 04:55:47PM +0800, Tzung-Bi Shih wrote:
> > > > > +struct keyboard_led_private {
> > > >
> > > > Why 'private', isn't this more a 'cros_ec_kdb_bl' or similar?
> > >
> > > It is just drvdata.
> >
> > The data structure represents an instance of the device, as such it
> > is an important part of the driver, drvdata is just a way to attach
> > it to the platform device.
> >
> > > I would prefer to keep the original prefix "keyboard_led_" if you wouldn't
> > > have strong opinion.
> >
> > I'm fine with 'keyboard_led', but object to the 'private' part. In the
> > kernel 'private' fields are typically used when a driver consists of a
> > generic part and a device specific part. The driver has a 'private'
> > void* field that points to a device specific data structure about which
> > the generic driver is agnostic. This data structure is only used by the
> > device specific implementation. That isn't the case here, so naming the
> > structure anything 'private' is misleading.
>
> The struct in the case is device specific. I don't see a problem to name it
> *private* as there are a lot of more existing examples.
>
> $ grep -R 'struct .*_priv.* {' drivers/
> drivers/pinctrl/bcm/pinctrl-bcm6358.c:struct bcm6358_priv {
>
> $ grep -R 'struct .*_priv.* {' sound/soc/codecs/
> sound/soc/codecs/rt286.c:struct rt286_priv {
>
> I would get rid of the term "private" if it could be confusing.
Ok, I hadn't come across that use of 'private' yet, but apparently it's not
that uncommon. Personally I prefer variables with specific names, but maybe
for others 'private' just spells out clearly what a struct is about ¯\_(ツ)_/¯
> > > > > +static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
> > > > > +{
> > > > > + struct keyboard_led_private *private = platform_get_drvdata(pdev);
> > > > > +
> > > > > + private->ec = dev_get_drvdata(pdev->dev.parent);
> > > > > + if (!private->ec) {
> > > > > + dev_err(&pdev->dev, "no parent EC device\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > >
> > > > The only thing this 'init' function does is assigning private->ec. Wouldn't
> > > > it be clearer to do this directly in probe() from where callback is called?
> > > > It could be with the condition that the device as a DT node.
> > >
> > > No. The probe() isn't aware of the device is from ACPI or OF.
> >
> > But it could be:
> >
> > if (pdev->dev.of_node)
> > kbd_led->ec = dev_get_drvdata(pdev->dev.parent);
>
> The 'init' callback isn't only for OF but also ACPI. I would prefer to keep
> the 'init' function and let probe() have no awareness about them.
Ah ok, then it makes sense, I think my brain conflated this with the unnecessary
keyboard_led_init_acpi_null stub.
> > > > Is it actually possible that the keyboard backlight device gets instantiated
> > > > if there is no EC parent?
> > >
> > > It shouldn't be but just in case.
> >
> > If this can only occur due to an error in common kernel frameworks then
> > the check should be omitted IMO.
>
> The check is referenced from [1]. I would prefer to keep it instead of
> crashing kernel if anything went wrong.
>
> [1]: https://elixir.bootlin.com/linux/v5.18-rc7/source/drivers/pwm/pwm-cros-ec.c#L244
Ok, maybe there are cases where the EC parent could go away. Let's hope it
doesn't happen after the keyboard backlight got probed :)
> >
> > > > > +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
> > > > > + .init = keyboard_led_init_ec_pwm_null,
> > > >
> > > > Is this really needed?
> > > >
> > > > keyboard_led_probe() checks if .init is assigned before invoking the callback:
> > > >
> > > > if (drvdata->init) {
> > > > error = drvdata->init(pdev);
> > > >
> > > > The whole 'else' branch could be eliminated if .of_match_table of the driver
> > > > only is assigned when CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is set. IMO that
> > > > would preferable over creating 'stubs'.
> > >
> > > CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent. The stubs
> > > were created to avoid compile errors if CONFIG_OF=y but
> > > CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n.
> >
> > Is there functional version of the driver that uses instantiation through the
> > device tree if CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n? If not .of_match_table
> > should not be assigned.
>
> CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent.
> CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is also designed to work with CONFIG_ACPI.
I understand that CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM also works with
CONFIG_ACPI, but in that case the driver uses .acpi_match_table, not
.of_match_table. So you wouldn't have to define 'keyboard_led_of_match'
if you did this:
static struct platform_driver keyboard_led_driver = {
.driver = {
.name = "chromeos-keyboard-leds",
.acpi_match_table = ACPI_PTR(keyboard_led_acpi_match),
#ifdef CONFIG_OF
.of_match_table = of_match_ptr(keyboard_led_of_match),
#endif
},
.probe = keyboard_led_probe,
;