This patch series introduces new driver for controlling LEDs connected
to Intel Cherry Trail Whiskey Cove PMIC (general-purpose LED and charger
status led). Only simple 'always on' and blinking modes are supported
for now, no breathing.
Driver was tested only with Lenovo Yoga Book notebook, and I don't have
any documentation for the PMIC, so proposals and testing are welcome.
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * Based on Lenovo Yoga Book Android kernel sources
+ */
+#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;
+
+ 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;
+
+ 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;
+
+ if (!*delay_on && !*delay_off) {
+ /* Return current settings */
+ ret = regmap_read(led->pmic->regmap, led->ctrl_reg, &ctrl);
+
+ 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 <[email protected]>");
+MODULE_LICENSE("GPL");
+
--
2.20.1
Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC
mfd device driver.
Signed-off-by: Yauhen Kharuzhy <[email protected]>
---
drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
index 64a3aece9c5e..68b54173edd8 100644
--- a/drivers/mfd/intel_soc_pmic_chtwc.c
+++ b/drivers/mfd/intel_soc_pmic_chtwc.c
@@ -60,6 +60,8 @@ static struct mfd_cell cht_wc_dev[] = {
.resources = cht_wc_ext_charger_resources,
},
{ .name = "cht_wcove_region", },
+
+ { .name = "cht_wcove_leds", },
};
/*
--
2.20.1
On Sun, Feb 10, 2019 at 01:12:12AM +0300, Yauhen Kharuzhy wrote:
> This patch series introduces new driver for controlling LEDs connected
> to Intel Cherry Trail Whiskey Cove PMIC (general-purpose LED and charger
> status led). Only simple 'always on' and blinking modes are supported
> for now, no breathing.
>
> Driver was tested only with Lenovo Yoga Book notebook, and I don't have
> any documentation for the PMIC, so proposals and testing are welcome.
>
I'm not sure you have a correct Cc list here. LED maintainers are
M: Jacek Anaszewski <[email protected]>
M: Pavel Machek <[email protected]>
Also would be nice to cc to Hans de Goede.
--
With Best Regards,
Andy Shevchenko
On Mon 2019-02-11 13:59:51, Andy Shevchenko wrote:
> On Sun, Feb 10, 2019 at 01:12:12AM +0300, Yauhen Kharuzhy wrote:
> > This patch series introduces new driver for controlling LEDs connected
> > to Intel Cherry Trail Whiskey Cove PMIC (general-purpose LED and charger
> > status led). Only simple 'always on' and blinking modes are supported
> > for now, no breathing.
> >
> > Driver was tested only with Lenovo Yoga Book notebook, and I don't have
> > any documentation for the PMIC, so proposals and testing are welcome.
> >
>
> I'm not sure you have a correct Cc list here. LED maintainers are
> M: Jacek Anaszewski <[email protected]>
> M: Pavel Machek <[email protected]>
>
> Also would be nice to cc to Hans de Goede.
I follow the list. I beileve Jacek does, too.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> +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,
> + },
> +};
Unfortunately, these LED names will not be too useful for
userspace. What about "platform::charging" and "status-led::"?
Should we have some default triggers?
Pavel
-*- org -*-
It is somehow important to provide consistent interface to the
userland. LED devices have one problem there, and that is naming of
directories in /sys/class/leds. It would be nice if userland would
just know right "name" for given LED function, but situation got more
complex.
Anyway, if backwards compatibility is not an issue, new code should
use one of the "good" names from this list, and you should extend the
list where applicable.
Bad names are listed, too, in case you are writing application that
wants to use particular feature, you should probe for good name first
but then try the bad ones, too".
* Keyboards
Good: "input*:*:capslock"
Good: "input*:*:scrolllock"
Good: "input*:*:numlock"
Bad: "shift-key-light" (Motorola Droid 4, capslock)
Set of common keyboard LEDs, going back to PC AT or so.
Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)
Frontlight/backlight of main keyboard.
Bad: "button-backlight" (Motorola Droid 4)
Some phones have touch buttons below screen; it is different from main
keyboard. And this is their backlight.
* Sound subsystem
Good: "platform:*:mute"
Good: "platform:*:micmute"
LEDs on notebook body, indicating that sound input / output is muted.
* System notification
Good: "status-led:{red,green,blue}" (Motorola Droid 4)
Bad: "lp5523:{r,g,b}" (Nokia N900)
Phones usually have multi-color status LED.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 2/11/19 2:05 PM, Pavel Machek wrote:
> On Mon 2019-02-11 13:59:51, Andy Shevchenko wrote:
>> On Sun, Feb 10, 2019 at 01:12:12AM +0300, Yauhen Kharuzhy wrote:
>>> This patch series introduces new driver for controlling LEDs connected
>>> to Intel Cherry Trail Whiskey Cove PMIC (general-purpose LED and charger
>>> status led). Only simple 'always on' and blinking modes are supported
>>> for now, no breathing.
>>>
>>> Driver was tested only with Lenovo Yoga Book notebook, and I don't have
>>> any documentation for the PMIC, so proposals and testing are welcome.
>>>
>>
>> I'm not sure you have a correct Cc list here. LED maintainers are
>> M: Jacek Anaszewski <[email protected]>
>> M: Pavel Machek <[email protected]>
>>
>> Also would be nice to cc to Hans de Goede.
>
> I follow the list. I beileve Jacek does, too.
Right, there is no issue with that.
--
Best regards,
Jacek Anaszewski
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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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 <[email protected]>");
> +MODULE_LICENSE("GPL");
s/GPL/GPL v2/
> +
>
--
Best regards,
Jacek Anaszewski
On Sun, 10 Feb 2019, Yauhen Kharuzhy wrote:
> Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC
> mfd device driver.
>
> Signed-off-by: Yauhen Kharuzhy <[email protected]>
> ---
> drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
> index 64a3aece9c5e..68b54173edd8 100644
> --- a/drivers/mfd/intel_soc_pmic_chtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_chtwc.c
> @@ -60,6 +60,8 @@ static struct mfd_cell cht_wc_dev[] = {
> .resources = cht_wc_ext_charger_resources,
> },
> { .name = "cht_wcove_region", },
> +
Nit: No need for the blank line, please remove it.
> + { .name = "cht_wcove_leds", },
> };
>
> /*
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Feb 12, 2019 at 08:14:09AM +0000, Lee Jones wrote:
> On Sun, 10 Feb 2019, Yauhen Kharuzhy wrote:
>
> > Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC
> > mfd device driver.
> >
> > Signed-off-by: Yauhen Kharuzhy <[email protected]>
> > ---
> > drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
> > index 64a3aece9c5e..68b54173edd8 100644
> > --- a/drivers/mfd/intel_soc_pmic_chtwc.c
> > +++ b/drivers/mfd/intel_soc_pmic_chtwc.c
> > @@ -60,6 +60,8 @@ static struct mfd_cell cht_wc_dev[] = {
> > .resources = cht_wc_ext_charger_resources,
> > },
> > { .name = "cht_wcove_region", },
> > +
>
> Nit: No need for the blank line, please remove it.
Agree.
>
> > + { .name = "cht_wcove_leds", },
> > };
--
Yauhen Kharuzhy
On Mon, Feb 11, 2019 at 10:36:30PM +0100, Jacek Anaszewski wrote:
> Hi Yauhen,
>
> Thank you for the patch.
>
> Please find few comments in the code.
>
> On 2/9/19 11:12 PM, Yauhen Kharuzhy wrote:
> > 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 <[email protected]>
> > + *
> > + * Based on Lenovo Yoga Book Android kernel sources
> > + */
>
> Please use uniform "//" comment style above.
OK.
> > +
> > +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.
Agree.
> > +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.
OK
> > +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.
OK
>
> > + 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.
Hmm... This was attempt to set 'reasonable default' to current blink
settings from hardware. But if this is too unpredictable behaviour for
LED class, I will change it to constant values.
> > +MODULE_DESCRIPTION("Intel Cherrytrail Whiskey Cove PMIC LEDs driver");
> > +MODULE_AUTHOR("Yauhen Kharuzhy <[email protected]>");
> > +MODULE_LICENSE("GPL");
>
> s/GPL/GPL v2/
OK.
--
Yauhen Kharuzhy
On Mon, Feb 11, 2019 at 02:17:26PM +0100, Pavel Machek wrote:
> Hi!
>
> > +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,
> > + },
> > +};
>
> Unfortunately, these LED names will not be too useful for
> userspace. What about "platform::charging" and "status-led::"?
That sounds reasonable.
>
> Should we have some default triggers?
I don't know which triggers may be suitable for this. There are no
generic "is charging now" triggers in kernel. I can set default trigger
of charging led to 'bq27542-0-charging-blink-full-solid' as example but
it will valid for platforms with bq27542 battery fuel gauge, of course.
For status led, 'default-on' should be reasonable.
> Pavel
--
Yauhen Kharuzhy