lkml.org 
[lkml]   [2019]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/5] leds: Add support for AXP20X CHGLED
From
Date
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@olimex.com>
> ---
> 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@olimex.com>
> +
> +#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@olimex.com");
> +MODULE_DESCRIPTION("X-Powers PMIC CHGLED driver");
> +MODULE_LICENSE("GPL");
>

--
Best regards,
Jacek Anaszewski

\
 
 \ /
  Last update: 2019-01-31 22:38    [W:0.115 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site