Hi Yauhen,
Thank you for the patch.
Please find few comments in the code.
On 2/9/19 11:12 PM, Yauhen Kharuzhy wrote:
Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove
PMIC. Charger and general-purpose leds are supported. Hardware blinking
is implemented, breathing is not.
This driver was tested with Lenovo Yoga Book notebook.
Signed-off-by: Yauhen Kharuzhy <jek...@gmail.com>
---
drivers/leds/Kconfig | 11 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-cht-wcove.c | 278 ++++++++++++++++++++++++++++++++++
3 files changed, 290 insertions(+)
create mode 100644 drivers/leds/leds-cht-wcove.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a72f97fca57b..8f50f38af57e 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -106,6 +106,17 @@ config LEDS_BCM6358
This option enables support for LEDs connected to the BCM6358
LED HW controller accessed via MMIO registers.
+config LEDS_CHT_WCOVE
+ tristate "LED support for Intel Cherry Trail Whiskey Cove PMIC"
+ depends on LEDS_CLASS
+ depends on INTEL_SOC_PMIC_CHTWC
+ help
+ This option enables support for charger and general purpose LEDs
+ connected to the Intel Cherrytrail Whiskey Cove PMIC.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-cht-wcove.
+
config LEDS_CPCAP
tristate "LED Support for Motorola CPCAP"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b0054f379..1c1995d3441c 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o
obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o
+obj-$(CONFIG_LEDS_CHT_WCOVE) += leds-cht-wcove.o
obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o
obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o
obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o
diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
new file mode 100644
index 000000000000..82ed0845bf72
--- /dev/null
+++ b/drivers/leds/leds-cht-wcove.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC
+ *
+ * Copyright 2019 Yauhen Kharuzhy <jek...@gmail.com>
+ *
+ * Based on Lenovo Yoga Book Android kernel sources
+ */
Please use uniform "//" comment style above.
+#include <linux/kernel.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define CHT_WC_LED1_CTRL 0x5e1f
+#define CHT_WC_LED1_FSM 0x5e20
+#define CHT_WC_LED1_PWM 0x5e21
+
+#define CHT_WC_LED2_CTRL 0x4fdf
+#define CHT_WC_LED2_FSM 0x4fe0
+#define CHT_WC_LED2_PWM 0x4fe1
+
+/* HW or SW control of charging led */
+#define CHT_WC_LED1_SWCTL BIT(0)
+#define CHT_WC_LED1_ON BIT(1)
+
+#define CHT_WC_LED2_ON BIT(0)
+#define CHT_WC_LED_I_MA2_5 (2 << 2)
+/* LED current limit */
+#define CHT_WC_LED_I_MASK GENMASK(3, 2)
+
+#define CHT_WC_LED_F_1_4_HZ (0 << 4)
+#define CHT_WC_LED_F_1_2_HZ (1 << 4)
+#define CHT_WC_LED_F_1_HZ (2 << 4)
+#define CHT_WC_LED_F_2_HZ (3 << 4)
+#define CHT_WC_LED_F_MASK 0x30
+
+#define CHT_WC_LED_EFF_ON BIT(1)
+#define CHT_WC_LED_EFF_BLINKING BIT(2)
+#define CHT_WC_LED_EFF_BREATHING BIT(3)
+#define CHT_WC_LED_EFF_MASK 0x06
+
+struct cht_wc_led {
+ struct led_classdev cdev;
+ struct intel_soc_pmic *pmic;
+ const char *name;
+ u16 ctrl_reg;
+ u8 enable_mask;
+ u16 fsm_reg;
+ u16 pwm_reg;
+};
+
+static struct cht_wc_led cht_wc_leds[] = {
+ {
+ .name = "pmic::charge",
+ .ctrl_reg = CHT_WC_LED1_CTRL,
+ .fsm_reg = CHT_WC_LED1_FSM,
+ .pwm_reg = CHT_WC_LED1_PWM,
+ .enable_mask = CHT_WC_LED1_ON,
+ },
+ {
+ .name = "pmic::gpled",
+ .ctrl_reg = CHT_WC_LED2_CTRL,
+ .fsm_reg = CHT_WC_LED2_FSM,
+ .pwm_reg = CHT_WC_LED2_PWM,
+ .enable_mask = CHT_WC_LED2_ON,
+ },
+};
+
+static int cht_wc_leds_brightness_set(struct led_classdev *cdev,
+ enum led_brightness value)
+{
+ struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
+ int ret;
You need mutex protection here. Think of the cases where brightness
is set from sysfs and the call from trigger jumps in the middle.
+ if (!value) {
+ ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg,
+ led->enable_mask, 0);
+ if (ret)
+ dev_err(cdev->dev, "Failed to turn off: %d\n", ret);
+
+ ret = regmap_update_bits(led->pmic->regmap, led->fsm_reg,
+ CHT_WC_LED_EFF_MASK,
+ CHT_WC_LED_EFF_ON);
+ if (ret < 0)
+ dev_err(cdev->dev,
+ "Failed to update LED FSM reg: %d\n", ret);
+ } else {
+ ret = regmap_write(led->pmic->regmap, led->pwm_reg, value);
+ if (ret)
+ dev_err(cdev->dev,
+ "Failed to set brightness: %d\n", ret);
+
+ ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg,
+ led->enable_mask, led->enable_mask);
+ if (ret)
+ dev_err(cdev->dev, "Failed to turn on: %d\n", ret);
+ }
+ return ret;
+}
+
+enum led_brightness cht_wc_leds_brightness_get(struct led_classdev *cdev)
+{
+ struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
+ int ret;
+ unsigned int val;
Ditto.
+ ret = regmap_read(led->pmic->regmap, led->ctrl_reg, &val);
+ if (ret < 0) {
+ dev_err(cdev->dev, "Failed to read LED CTRL reg: %d\n", ret);
+ return LED_OFF;
+ }
+
+ val &= led->enable_mask;
+
+ if (!val)
+ return LED_OFF;
+
+ ret = regmap_read(led->pmic->regmap, led->pwm_reg, &val);
+ if (ret < 0) {
+ dev_err(cdev->dev, "Failed to read LED PWM reg: %d\n", ret);
+ return LED_ON;
+ }
+
+ return val;
+}
+
+/* Return blinking period for given CTRL reg value */
+static unsigned long cht_wc_leds_get_period(int ctrl)
+{
+ ctrl &= CHT_WC_LED_F_MASK;
+
+ switch (ctrl) {
+ case CHT_WC_LED_F_1_4_HZ:
+ return 1000 * 4;
+ case CHT_WC_LED_F_1_2_HZ:
+ return 1000 * 2;
+ case CHT_WC_LED_F_1_HZ:
+ return 1000;
+ case CHT_WC_LED_F_2_HZ:
+ return 1000 / 2;
+ };
+
+ return 0;
+}
+
+/*
+ * Find suitable hardware blink mode for given period.
+ * period < 750 ms - select 2 HZ
+ * 750 ms <= period < 1500 ms - select 1 HZ
+ * 1500 ms <= period < 3000 ms - select 1/2 HZ
+ * 3000 ms <= period < 5000 ms - select 1/4 HZ
+ * 5000 ms <= period - return -1
+ */
+static int cht_wc_leds_find_freq(unsigned long period)
+{
+ if (period < 750)
+ return CHT_WC_LED_F_2_HZ;
+ else if (period < 1500)
+ return CHT_WC_LED_F_1_HZ;
+ else if (period < 3000)
+ return CHT_WC_LED_F_1_2_HZ;
+ else if (period < 5000)
+ return CHT_WC_LED_F_1_4_HZ;
+ else
+ return -1;
+}
+
+static int cht_wc_leds_blink_set(struct led_classdev *cdev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
+ unsigned int ctrl;
+ int ret;
mutex needed as well.
+ if (!*delay_on && !*delay_off) {
+ /* Return current settings */
+ ret = regmap_read(led->pmic->regmap, led->ctrl_reg, &ctrl);
Why read? This op sets blinking. Please refer to the blink_set semantics
description from include/linux/leds.h:
/*
* Activate hardware accelerated blink, delays are in milliseconds
* and if both are zero then a sensible default should be chosen.
* The call should adjust the timings in that case and if it can't
* match the values specified exactly.
* Deactivate blinking again when the brightness is set to LED_OFF
* via the brightness_set() callback.
*/
int (*blink_set)(struct led_classdev *led_cdev,
unsigned long *delay_on,
unsigned long *delay_off);
Please compare how other LED class drivers do that.
+
+ if (ret < 0) {
+ dev_err(cdev->dev,
+ "Failed to read LED CTRL reg: %d\n", ret);
+ return ret;
+ }
+
+ *delay_off = *delay_on = cht_wc_leds_get_period(ctrl) / 2;
+
+ return 0;
+ }
+
+ ctrl = cht_wc_leds_find_freq(*delay_on + *delay_off);
+ if (ctrl < 0) {
+ /* Disable HW blinking */
+ ret = regmap_update_bits(led->pmic->regmap, led->fsm_reg,
+ CHT_WC_LED_EFF_MASK,
+ CHT_WC_LED_EFF_ON);
+ if (ret < 0)
+ dev_err(cdev->dev,
+ "Failed to update LED FSM reg: %d\n", ret);
+
+ /* Fallback to software timer */
+ *delay_on = *delay_off = 0;
+ return -EINVAL;
+ }
+
+ ret = regmap_update_bits(led->pmic->regmap, led->fsm_reg,
+ CHT_WC_LED_EFF_MASK, CHT_WC_LED_EFF_BLINKING);
+ if (ret < 0)
+ dev_err(cdev->dev,
+ "Failed to update LED FSM reg: %d\n", ret);
+
+ ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg,
+ CHT_WC_LED_F_MASK, ctrl);
+ if (ret < 0)
+ dev_err(cdev->dev,
+ "Failed to update LED CTRL reg: %d\n", ret);
+
+ *delay_off = *delay_on = cht_wc_leds_get_period(ctrl) / 2;
+
+ return 0;
+}
+
+static int cht_wc_leds_probe(struct platform_device *pdev)
+{
+ struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+ int ret;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(cht_wc_leds); i++) {
+ struct cht_wc_led *led = &cht_wc_leds[i];
+
+ led->pmic = pmic;
+ led->cdev.name = cht_wc_leds[i].name;
+ led->cdev.brightness_set_blocking = cht_wc_leds_brightness_set;
+ led->cdev.brightness_get = cht_wc_leds_brightness_get;
+ led->cdev.blink_set = cht_wc_leds_blink_set;
+ led->cdev.max_brightness = 255;
+
+ ret = devm_led_classdev_register(&pdev->dev, &led->cdev);
+ if (ret < 0)
+ return ret;
+ }
+
+ ret = regmap_update_bits(pmic->regmap, CHT_WC_LED1_CTRL,
+ CHT_WC_LED1_SWCTL, 1);
+
+ if (ret)
+ dev_err(&pdev->dev,
+ "Failed to set SW control bit for charger LED: %d\n",
+ ret);
+
+ platform_set_drvdata(pdev, cht_wc_leds);
+
+ return 0;
+}
+
+static const struct platform_device_id cht_wc_leds_table[] = {
+ { .name = "cht_wcove_leds" },
+ {},
+};
+MODULE_DEVICE_TABLE(platform, cht_wc_leds_table);
+
+static struct platform_driver cht_wc_leds_driver = {
+ .probe = cht_wc_leds_probe,
+ .id_table = cht_wc_leds_table,
+ .driver = {
+ .name = "cht_wcove_leds",
+ },
+};
+module_platform_driver(cht_wc_leds_driver);
+
+MODULE_DESCRIPTION("Intel Cherrytrail Whiskey Cove PMIC LEDs driver");
+MODULE_AUTHOR("Yauhen Kharuzhy <jek...@gmail.com>");
+MODULE_LICENSE("GPL");
s/GPL/GPL v2/
+
--
Best regards,
Jacek Anaszewski
