Messages in this thread | | | Subject | Re: [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs | From | Jacek Anaszewski <> | Date | Mon, 11 Feb 2019 22:36:30 +0100 |
| |
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 <jekhor@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 <jekhor@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 <jekhor@gmail.com>"); > +MODULE_LICENSE("GPL");
s/GPL/GPL v2/
> + >
-- Best regards, Jacek Anaszewski
| |