Re: [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Nick,

Thank you for the patch.

I have one comment for the kbd_led_backlight.c

On 4/3/19 4:05 AM, Nick Crews wrote:
> The EC is in charge of controlling the keyboard backlight on
> the Wilco platform. We expose a standard LED class device at
> /sys/class/leds/chromeos::kbd_backlight. This driver is modeled
> after the standard Chrome OS keyboard backlight driver at
> drivers/platform/chrome/cros_kbd_led_backlight.c
[...]
> +++ b/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
[,,,]
> +static int initialize_brightness(struct wilco_keyboard_led_data *data)
> +{
> +	struct wilco_ec_kbbl_msg request;
> +	struct wilco_ec_kbbl_msg response;
> +	int ret;
> +
> +	memset(&request, 0, sizeof(request));
> +	request.command = WILCO_EC_COMMAND_KBBL;
> +	request.subcmd = WILCO_KBBL_SUBCMD_GET_STATE;
> +
> +	ret = send_kbbl_msg(data->ec, &request, &response);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (response.mode & WILCO_KBBL_MODE_FLAG_PWM)
> +		return response.percent;
> +
> +	dev_warn(data->ec->dev, "Keyboard brightness not initialized by BIOS");
> +	ret = led_set_brightness_sync(&data->keyboard, KBBL_DEFAULT_BRIGHTNESS);

This is a bit weird. led_set_brightness_sync() has been introduced
specifically for use cases when torch setting should have guaranteed
immediate effect like in case of
drivers/media/v4l2-core/v4l2-flash-led-class.c (this is actually its
sole user). It bypasses internal LED core workqueue. While its use
here is not incorrect per se, it is of no avail to use public LED
subsystem API just for calling brightness_set_blocking op initialized
also in this driver.

Just call keyboard_led_set_brightness() directly.

With that:

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

> +	if (ret < 0)
> +		return ret;
> +
> +	return KBBL_DEFAULT_BRIGHTNESS;
> +}
> +
> +static int keyboard_led_probe(struct platform_device *pdev)
> +{
> +	struct wilco_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct wilco_keyboard_led_data *data;
> +	int ret;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->ec = ec;
> +	/* This acts the same as the CrOS backlight, so use the same name */
> +	data->keyboard.name = "chromeos::kbd_backlight";
> +	data->keyboard.max_brightness = 100;
> +	data->keyboard.flags = LED_CORE_SUSPENDRESUME;
> +	data->keyboard.brightness_set_blocking = keyboard_led_set_brightness;
> +	ret = initialize_brightness(data);
> +	if (ret < 0)
> +		return ret;
> +	data->keyboard.brightness = ret;
> +
> +	ret = devm_led_classdev_register(&pdev->dev, &data->keyboard);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static struct platform_driver keyboard_led_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = keyboard_led_probe,
> +};
> +module_platform_driver(keyboard_led_driver);
> +
> +MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
> +MODULE_DESCRIPTION("Wilco keyboard backlight LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h
> index 1ff224793c99..c3965b7f397d 100644
> --- a/include/linux/platform_data/wilco-ec.h
> +++ b/include/linux/platform_data/wilco-ec.h
> @@ -32,6 +32,7 @@
>    * @data_size: Size of the data buffer used for EC communication.
>    * @debugfs_pdev: The child platform_device used by the debugfs sub-driver.
>    * @rtc_pdev: The child platform_device used by the RTC sub-driver.
> + * @kbbl_pdev: The child pdev used by the keyboard backlight sub-driver.
>    */
>   struct wilco_ec_device {
>   	struct device *dev;
> @@ -43,6 +44,7 @@ struct wilco_ec_device {
>   	size_t data_size;
>   	struct platform_device *debugfs_pdev;
>   	struct platform_device *rtc_pdev;
> +	struct platform_device *kbbl_pdev;
>   };
>   
>   /**
> @@ -114,6 +116,42 @@ struct wilco_ec_message {
>   	void *response_data;
>   };
>   
> +/* Constants and structs useful for keyboard backlight (KBBL) control */
> +
> +#define WILCO_EC_COMMAND_KBBL		0x75
> +#define WILCO_KBBL_MODE_FLAG_PWM	BIT(1)	/* Set brightness by percent. */
> +
> +/**
> + * enum kbbl_subcommand - What action does the EC perform?
> + * @WILCO_KBBL_SUBCMD_GET_FEATURES: Request available functionality from EC.
> + * @WILCO_KBBL_SUBCMD_GET_STATE: Request current mode and brightness from EC.
> + * @WILCO_KBBL_SUBCMD_SET_STATE: Write mode and brightness to EC.
> + */
> +enum kbbl_subcommand {
> +	WILCO_KBBL_SUBCMD_GET_FEATURES = 0x00,
> +	WILCO_KBBL_SUBCMD_GET_STATE = 0x01,
> +	WILCO_KBBL_SUBCMD_SET_STATE = 0x02,
> +};
> +
> +/**
> + * struct wilco_ec_kbbl_msg - Message to/from EC for keyboard backlight control.
> + * @command: Always WILCO_EC_COMMAND_KBBL.
> + * @status: Set by EC to 0 on success, 0xFF on failure.
> + * @subcmd: One of enum kbbl_subcommand.
> + * @mode: Bit flags for used mode, we want to use WILCO_KBBL_MODE_FLAG_PWM.
> + * @percent: Brightness in 0-100. Only meaningful in PWM mode.
> + */
> +struct wilco_ec_kbbl_msg {
> +	u8 command;
> +	u8 status;
> +	u8 subcmd;
> +	u8 reserved3;
> +	u8 mode;
> +	u8 reserved5to8[4];
> +	u8 percent;
> +	u8 reserved10to15[6];
> +} __packed;
> +
>   /**
>    * wilco_ec_mailbox() - Send request to the EC and receive the response.
>    * @ec: Wilco EC device.
> 

-- 
Best regards,
Jacek Anaszewski