Re: [PATCH 1/5] leds: Add support for AXP20X CHGLED
From: Jacek Anaszewski
Date: Thu Jan 31 2019 - 16:38:10 EST
Hi Stefan,
Thank you for the patch.
On 1/31/19 9:23 AM, Stefan Mavrodiev wrote:
Most of AXP20x PMIC chips have built-in battery charger with LED indicator.
The LED can be controlled ether by the charger or manually by a register.
The default is (except for AXP209) manual control, which makes this LED
useless, since there is no device driver.
The driver rely on AXP20X MFD driver.
Signed-off-by: Stefan Mavrodiev <stefan@xxxxxxxxxx>
---
drivers/leds/Kconfig | 10 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-axp20x.c | 283 +++++++++++++++++++++++++++++++++++++
3 files changed, 294 insertions(+)
create mode 100644 drivers/leds/leds-axp20x.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a72f97fca57b..82dce9063d41 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -766,6 +766,16 @@ config LEDS_NIC78BX
To compile this driver as a module, choose M here: the module
will be called leds-nic78bx.
+config LEDS_AXP20X
+ tristate "LED support for X-Powers PMICs"
+ depends on MFD_AXP20X
+ help
+ This option enables support for CHGLED found on most of X-Powers
+ PMICs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-axp20x.
+
comment "LED Triggers"
source "drivers/leds/trigger/Kconfig"
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b0054f379..d3fb76e119d8 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o
obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o
obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o
+obj-$(CONFIG_LEDS_AXP20X) += leds-axp20x.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
diff --git a/drivers/leds/leds-axp20x.c b/drivers/leds/leds-axp20x.c
new file mode 100644
index 000000000000..9c03410833a3
--- /dev/null
+++ b/drivers/leds/leds-axp20x.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright 2019 Stefan Mavrodiev <stefan@xxxxxxxxxx>
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include <linux/mfd/axp20x.h>
+
+#define AXP20X_CHGLED_CTRL_REG AXP20X_OFF_CTRL
+#define AXP20X_CHGLED_FUNC_MASK GENMASK(5, 4)
+#define AXP20X_CHGLED_FUNC_OFF (0 << 4)
+#define AXP20X_CHGLED_FUNC_1HZ (1 << 4)
+#define AXP20X_CHGLED_FUNC_4HZ (2 << 4)
+#define AXP20X_CHGLED_FUNC_FULL (3 << 4)
+#define AXP20X_CHGLED_CTRL_MASK BIT(3)
+#define AXP20X_CHGLED_CTRL_MANUAL 0
+#define AXP20X_CHGLED_CTRL_CHARGER 1
+#define AXP20X_CHGLED_CTRL(_ctrl) (_ctrl << 3)
+
+#define AXP20X_CHGLED_MODE_REG AXP20X_CHRG_CTRL2
+#define AXP20X_CHGLED_MODE_MASK BIT(4)
+#define AXP20X_CHGLED_MODE_A 0
+#define AXP20X_CHGLED_MODE_B 1
+#define AXP20X_CHGLED_MODE(_mode) (_mode << 4)
+
+struct axp20x_led {
+ struct led_classdev cdev;
+ enum led_brightness current_brightness;
You don't need this. Please use the one from struct led_classdev.
+ u8 mode : 1;
+ u8 ctrl : 1;
+ u8 ctrl_inverted : 1;
+ struct axp20x_dev *axp20x;
+};
+
+static inline struct axp20x_led *to_axp20x_led(struct led_classdev *cdev)
+{
+ return container_of(cdev, struct axp20x_led, cdev);
+}
+
+static int axp20x_led_setup(struct axp20x_led *priv)
+{
+ int ret;
+ u8 val;
+
+ /* Invert the logic, if necessary */
+ val = priv->ctrl ^ priv->ctrl_inverted;
You need mutex protection in all places where the hardware is accessed.
It is possible that brightness_set_blocking() will be called from
trigger e.g. after first regmap_update_bits_below().
+
+ ret = regmap_update_bits(priv->axp20x->regmap, AXP20X_CHGLED_CTRL_REG,
+ AXP20X_CHGLED_CTRL_MASK,
+ AXP20X_CHGLED_CTRL(val));
+ if (ret < 0)
+ return ret;
+
+ return regmap_update_bits(priv->axp20x->regmap, AXP20X_CHGLED_MODE_REG,
+ AXP20X_CHGLED_MODE_MASK,
+ AXP20X_CHGLED_MODE(priv->mode));
+}
+
+static ssize_t control_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct axp20x_led *priv = to_axp20x_led(cdev);
+
+ return sprintf(buf, "%d\n", priv->ctrl);
s/%d/%u/
+}
+
+static ssize_t control_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct axp20x_led *priv = to_axp20x_led(cdev);
+ unsigned long val;
+ int ret;
+
+ ret = kstrtoul(buf, 0, &val);
+ if (ret)
+ return ret;
+
+ /**
+ * Supported values are:
+ * - 0 : Manual control
+ * - 1 : Charger control
+ */
+ if (val > 1)
+ return -EINVAL;
+
+ priv->ctrl = val;
+
+ return axp20x_led_setup(priv) ? : size;
+}
+static DEVICE_ATTR_RW(control);
+
+static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct axp20x_led *priv = to_axp20x_led(cdev);
+
+ return sprintf(buf, "%d\n", priv->mode);
Ditto.
+}
+
+static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct axp20x_led *priv = to_axp20x_led(cdev);
+ unsigned long val;
+ int ret;
+
+ ret = kstrtoul(buf, 0, &val);
+ if (ret)
+ return ret;
+ /**
+ * Supported values are:
+ * - 0 : Mode A
+ * - 1 : Mode B
+ */
+ if (val > 1)
+ return -EINVAL;
+
+ priv->mode = val;
+
+ return axp20x_led_setup(priv) ? : size;
+}
+static DEVICE_ATTR_RW(mode);
+
+static struct attribute *axp20x_led_attrs[] = {
+ &dev_attr_control.attr,
+ &dev_attr_mode.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(axp20x_led);
+
+enum led_brightness axp20x_led_brightness_get(struct led_classdev *cdev)
+{
+ struct axp20x_led *priv = to_axp20x_led(cdev);
+ u32 val;
+ int ret;
+
+ ret = regmap_read(priv->axp20x->regmap, AXP20X_CHGLED_CTRL_REG, &val);
+ if (ret < 0)
+ return LED_OFF;
+
+ return (val & AXP20X_CHGLED_FUNC_FULL) ? LED_FULL : LED_OFF;
+}
+
+static int axp20x_led_brightness_set_blocking(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ struct axp20x_led *priv = to_axp20x_led(cdev);
+ int ret = 0;
+
+ if (!priv->current_brightness && brightness)
+ ret = regmap_update_bits(priv->axp20x->regmap,
+ AXP20X_CHGLED_CTRL_REG,
+ AXP20X_CHGLED_FUNC_MASK,
+ AXP20X_CHGLED_FUNC_FULL);
+ else if (priv->current_brightness && !brightness)
+ ret = regmap_update_bits(priv->axp20x->regmap,
+ AXP20X_CHGLED_CTRL_REG,
+ AXP20X_CHGLED_FUNC_MASK,
+ AXP20X_CHGLED_FUNC_OFF);
+
+ if (ret < 0)
+ return ret;
+
+ priv->current_brightness = brightness;
+ return 0;
+}
+
+static int axp20x_led_parse_dt(struct axp20x_led *priv, struct device_node *np)
+{
+ const char *state;
+ int ret = 0;
+ u8 value;
+
+ priv->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
We now compose LED names differently. Please refer e.g. to:
drivers/leds/leds-cr0014114.c.
+
+ if (!of_property_read_u8(np, "x-powers,charger-mode", &value)) {
+ priv->ctrl = AXP20X_CHGLED_CTRL_CHARGER;
+ priv->mode = (value < 2) ? value : 0;
+ } else {
+ priv->ctrl = AXP20X_CHGLED_CTRL_MANUAL;
+ }
+
+ priv->cdev.default_trigger = of_get_property(np,
+ "linux,default-trigger",
+ NULL);
+
+ state = of_get_property(np, "default-state", NULL);
+ if (state) {
+ if (!strcmp(state, "keep")) {
+ ret = axp20x_led_brightness_get(&priv->cdev);
+ if (ret < 0)
+ return ret;
+ priv->current_brightness = ret;
+ } else if (!strcmp(state, "on")) {
+ ret = axp20x_led_brightness_set_blocking(&priv->cdev,
+ LED_FULL);
+ } else {
+ ret = axp20x_led_brightness_set_blocking(&priv->cdev,
+ LED_OFF);
+ }
+ }
+
+ return ret;
+}
+
+static const struct of_device_id axp20x_led_of_match[] = {
+ { .compatible = "x-powers,axp20x-led" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, axp20x_led_of_match);
+
+static int axp20x_led_probe(struct platform_device *pdev)
+{
+ struct axp20x_led *priv;
+ int ret;
+
+ if (!of_device_is_available(pdev->dev.of_node))
+ return -ENODEV;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(struct axp20x_led),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->axp20x = dev_get_drvdata(pdev->dev.parent);
+ if (!priv->axp20x) {
+ dev_err(&pdev->dev, "Failed to get parent data\n");
+ return -ENXIO;
+ }
+
+ priv->cdev.max_brightness = LED_FULL;
LED core will initialize it to LED_FULL when set to 0 by kzalloc().
+ priv->cdev.brightness_set_blocking = axp20x_led_brightness_set_blocking;
+ priv->cdev.brightness_get = axp20x_led_brightness_get;
+ priv->cdev.groups = axp20x_led_groups;
+
+ ret = axp20x_led_parse_dt(priv, pdev->dev.of_node);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to set parameters\n");
+ return ret;
+ }
+
+ /**
+ * For some reason in AXP209 the bit that controls CHGLED is with
+ * inverted logic compared to all other PMICs.
+ * If the PMIC is actually AXP209, set inverted flag and later use it
+ * when configuring the LED.
+ */
+ if (priv->axp20x->variant == AXP209_ID)
+ priv->ctrl_inverted = 1;
+
+ ret = axp20x_led_setup(priv);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to configure led");
+ return ret;
+ }
+
+ return devm_led_classdev_register(&pdev->dev, &priv->cdev);
+}
+
+static struct platform_driver axp20x_led_driver = {
+ .driver = {
+ .name = "axp20x-led",
+ .of_match_table = of_match_ptr(axp20x_led_of_match),
+ },
+ .probe = axp20x_led_probe,
+};
+
+module_platform_driver(axp20x_led_driver);
+
+MODULE_AUTHOR("Stefan Mavrodiev <stefan@xxxxxxxxxx");
+MODULE_DESCRIPTION("X-Powers PMIC CHGLED driver");
+MODULE_LICENSE("GPL");
--
Best regards,
Jacek Anaszewski