On Thu, 25 May 2023, Vladimir Barinov wrote:
> This adds support for Awinic AW2026 3-channel LED driver with
> I2C insterface. It supports hardware blinking and hardware
> pattern generator.
>
> Signed-off-by: Vladimir Barinov <v.barinov@xxxxxxxxx>
> ---
> Changes in version 2:
> - fixed typos in patch header 2016 -> 2026
>
> drivers/leds/Kconfig | 10 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-aw2026.c | 578 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 589 insertions(+)
> create mode 100644 drivers/leds/leds-aw2026.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index aaa9140bc351..574f3cc47d3e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -104,6 +104,16 @@ config LEDS_AW2013
> To compile this driver as a module, choose M here: the module
> will be called leds-aw2013.
>
> +config LEDS_AW2026
> + tristate "LED support for Awinic AW2026"
> + depends on LEDS_CLASS && I2C && OF
> + help
> + This option enables support for the AW2026 3-channel
> + LED driver.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called leds-aw2026.
> +
> config LEDS_BCM6328
> tristate "LED Support for Broadcom BCM6328"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d30395d11fd8..7fb7b48329ff 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> obj-$(CONFIG_LEDS_APU) += leds-apu.o
> obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
> obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
> +obj-$(CONFIG_LEDS_AW2026) += leds-aw2026.o
> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o
> obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o
> diff --git a/drivers/leds/leds-aw2026.c b/drivers/leds/leds-aw2026.c
> new file mode 100644
> index 000000000000..7c2d5f62797c
> --- /dev/null
> +++ b/drivers/leds/leds-aw2026.c
> @@ -0,0 +1,578 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Awinic AW2026 3-channel LED driver
> + *
> + * Author: Vladimir Barinov <v.barinov@xxxxxxxxx>
> + * Copyright (C) 2023 KNS Group LLC (YADRO)
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define AW2026_MAX_LEDS 3
> +
> +/* Chip ID and Software Reset Register */
> +#define AW2026_RSTIDR 0x00
> +#define AW2026_RSTIDR_RESET 0x55
> +#define AW2026_RSTIDR_CHIP_ID 0x31
> +
> +/* Global Control Register */
> +#define AW2026_GCR 0x01
> +#define AW2026_GCR_CHIPEN BIT(0)
> +
> +/* LED Maximum Current Register */
> +#define AW2026_IMAX 0x03
> +#define AW2026_IMAX_MASK (BIT(0) | BIT(1))
> +
> +/* LED Configure Register */
> +#define AW2026_LCFG(x) (0x04 + (x))
> +#define AW2026_LCFG_LEDMD BIT(0)
> +#define AW2026_LCFG_FADE_IN BIT(1)
> +#define AW2026_LCFG_FADE_OUT BIT(2)
> +
> +/* LED Channel Enable Register */
> +#define AW2026_LEDEN 0x07
> +
> +/* Pattern Run/Stop Register */
> +#define AW2026_PATRUN 0x09
> +
> +/* LED Current Register */
> +#define AW2026_ILED(x) (0x10 + (x))
> +#define AW2026_ILED_MAX 0xFF
> +
> +/* PWM duty level Register */
> +#define AW2026_PWM(x) (0x1C + (x))
> +#define AW2026_PWM_DUTY_MAX 0xFF
> +
> +/* T1 Time Parameter of Pattern */
> +#define AW2026_PAT_T1(x) (0x30 + 5*(x))
> +
> +/* T2 Time Parameter of Pattern */
> +#define AW2026_PAT_T2(x) (0x31 + 5*(x))
> +
> +struct aw2026;
No forward declarations.
Please reorder the struct definitions.
> +enum aw2026_state {
> + AW2026_STATE_OFF,
> + AW2026_STATE_ON,
> + AW2026_STATE_KEEP,
> +};
> +
> +struct aw2026_led {
> + struct aw2026 *chip;
> + struct fwnode_handle *fwnode;
> + struct led_classdev cdev;
> + enum aw2026_state default_state;
> + u32 idx;
> +};
> +
> +struct aw2026 {
> + struct i2c_client *client;
> + struct regmap *regmap;
> + struct mutex lock;
> + struct regulator *vcc_regulator;
> + struct aw2026_led leds[AW2026_MAX_LEDS];
> + int num_leds;
> + unsigned int imax;
> + bool enabled;
> +};
> +
> +static const struct regmap_config aw2026_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0x3e,
> +};
> +
> +struct msec_reg {
> + u32 msec;
> + u8 reg;
> +};
> +
> +static const struct msec_reg aw2026_msec_reg[] = {
> + { .msec = 4, .reg = 0x0 },
> + { .msec = 130, .reg = 0x1 },
> + { .msec = 260, .reg = 0x2 },
> + { .msec = 380, .reg = 0x3 },
> + { .msec = 510, .reg = 0x4 },
> + { .msec = 770, .reg = 0x5 },
> + { .msec = 1040, .reg = 0x6 },
> + { .msec = 1600, .reg = 0x7 },
> + { .msec = 2100, .reg = 0x8 },
> + { .msec = 2600, .reg = 0x9 },
> + { .msec = 3100, .reg = 0xa },
> + { .msec = 4200, .reg = 0xb },
> + { .msec = 5200, .reg = 0xc },
> + { .msec = 6200, .reg = 0xd },
> + { .msec = 7300, .reg = 0xe },
> + { .msec = 8300, .reg = 0xf },
> + { /* sentinel */ },
We really don't need this comment.
> +};
> +
> +static int aw2026_chip_init(struct aw2026 *chip)
> +{
> + int idx, ret;
> +
> + ret = regmap_update_bits(chip->regmap, AW2026_GCR, AW2026_GCR_CHIPEN,
> + AW2026_GCR_CHIPEN);
> + if (ret)
> + return ret;
> +
> + /* Max current */
> + ret = regmap_update_bits(chip->regmap, AW2026_IMAX,
> + AW2026_IMAX_MASK, chip->imax);
> + if (ret)
> + return ret;
> +
> + for (idx = 0; idx < chip->num_leds; idx++) {
> + /* PWM level */
Looks obvious from the quality define naming, no?
> + ret = regmap_write(chip->regmap, AW2026_PWM(idx), AW2026_PWM_DUTY_MAX);
> + if (ret)
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static void aw2026_chip_disable(struct aw2026 *chip)
> +{
> + int ret;
> +
> + if (!chip->enabled)
> + return;
What happens if we re-disable a disabled chip or re-enable an enabled
chip? If the answer is 'nothing, the op is inert' I suggest we remove
this variable from the struct and save ourselves a bunch of lines.
> + regmap_update_bits(chip->regmap, AW2026_GCR, AW2026_GCR_CHIPEN, 0);
> +
> + ret = regulator_disable(chip->vcc_regulator);
> + if (ret) {
> + dev_err(&chip->client->dev,
> + "Failed to disable regulator: %d\n", ret);
> + return;
> + }
> +
> + chip->enabled = false;
> +}
> +
> +static int aw2026_chip_enable(struct aw2026 *chip)
> +{
> + int ret;
> +
> + if (chip->enabled)
> + return 0;
> +
> + ret = regulator_enable(chip->vcc_regulator);
> + if (ret) {
> + dev_err(&chip->client->dev,
> + "Failed to enable regulator: %d\n", ret);
> + return ret;
> + }
> + chip->enabled = true;
> +
> + ret = aw2026_chip_init(chip);
> + if (ret)
> + aw2026_chip_disable(chip);
> +
> + return ret;
> +}
> +
> +static bool aw2026_chip_in_use(struct aw2026 *chip)
> +{
> + int i;
> +
> + for (i = 0; i < chip->num_leds; i++)
> + if (chip->leds[i].cdev.brightness)
> + return true;
> +
> + return false;
> +}
> +
> +static int aw2026_brightness_set(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
> + int ret, idx = led->idx;
> +
> + mutex_lock(&led->chip->lock);
> +
> + if (aw2026_chip_in_use(led->chip)) {
> + ret = aw2026_chip_enable(led->chip);
> + if (ret)
> + goto error;
> + }
> +
> + if (brightness) {
> + /* Manual mode */
> + ret = regmap_update_bits(led->chip->regmap, AW2026_LCFG(idx),
> + AW2026_LCFG_LEDMD, 0);
> + if (ret)
> + goto error;
Nit: '\n'
> + /* Current configure */
> + ret = regmap_write(led->chip->regmap, AW2026_ILED(idx), brightness);
> + if (ret)
> + goto error;
Nit: '\n'
> + /* Enable LED */
> + ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0xFF);
> + } else {
> + /* Disable LED */
> + ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0);
> + }
> + if (ret)
> + goto error;
> +
> + if (!aw2026_chip_in_use(led->chip))
> + aw2026_chip_disable(led->chip);
> +
> +error:
> + mutex_unlock(&led->chip->lock);
> +
> + return ret;
> +}
> +
> +static int aw2026_convert_msec_to_reg(struct aw2026 *chip, u32 *msec, u8 *reg)
> +{
> + const struct msec_reg *value;
> + const struct msec_reg *prev_value = NULL;
> +
> + for (value = aw2026_msec_reg; value->msec; value++) {
> + if (value->msec >= *msec)
> + break;
> + prev_value = value;
> + }
> +
> + if (!value->msec) {
> + dev_err(&chip->client->dev, "Unsupported msec (%u)", *msec);
> + return -ERANGE;
"Math result not representable"
Not sure this is correct.
Would -EINVAL suit your needs better?
> + }
> +
> + if (prev_value && ((*msec - prev_value->msec) <= (value->msec - *msec)))
Comment this please.
> + value = prev_value;
> +
> + *reg = value->reg;
> + *msec = value->msec;
> +
> + return 0;
> +}
> +
> +static int aw2026_pattern_setup(struct aw2026_led *led, u8 trise, u8 ton, u8 tfall, u8 toff)
> +{
> + int ret, idx = led->idx;
> +
> + mutex_lock(&led->chip->lock);
> +
> + if (aw2026_chip_in_use(led->chip)) {
> + ret = aw2026_chip_enable(led->chip);
> + if (ret)
> + goto error;
> + }
> +
> + /* Pattern mode */
> + ret = regmap_update_bits(led->chip->regmap, AW2026_LCFG(idx),
> + AW2026_LCFG_LEDMD, 0xFF);
> + if (ret)
> + goto error;
Nit: '\n'
Etc ... you get the idea. Throughout please.
> + /* Current configure */
> + ret = regmap_write(led->chip->regmap, AW2026_ILED(idx), led->cdev.brightness);
> + if (ret)
> + goto error;
> + /* Rise and On time of Pattern */
> + ret = regmap_write(led->chip->regmap, AW2026_PAT_T1(idx), (trise << 4) | ton);
> + if (ret)
> + goto error;
> + /* Fall and Off time of Pattern */
> + ret = regmap_write(led->chip->regmap, AW2026_PAT_T2(idx), (tfall << 4) | toff);
> + if (ret)
> + goto error;
> + /* Pattern run for individual LED */
> + ret = regmap_update_bits(led->chip->regmap, AW2026_PATRUN, BIT(idx), 0xFF);
> + if (ret)
> + goto error;
> + /* Enable LED */
> + ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0xFF);
> +
> +error:
> + mutex_unlock(&led->chip->lock);
> +
> + return ret;
> +}
> +
> +static int aw2026_pattern_set(struct led_classdev *cdev,
> + struct led_pattern *pattern,
> + u32 len, int repeat)
> +{
> + struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
> + struct aw2026 *chip = led->chip;
> + int ret;
> + u8 trise, ton, tfall, toff;
> +
> + if (len == 1) {
Comment this section please.
> + led->cdev.brightness = pattern[0].brightness;
> + return aw2026_brightness_set(cdev, led->cdev.brightness);
> + }
> +
> + if (repeat > 0 || len != 4)
> + return -EINVAL;
> +
> + ret = aw2026_convert_msec_to_reg(chip, &pattern[0].delta_t, &trise);
> + if (ret)
> + return ret;
> + ret = aw2026_convert_msec_to_reg(chip, &pattern[1].delta_t, &ton);
> + if (ret)
> + return ret;
> + ret = aw2026_convert_msec_to_reg(chip, &pattern[2].delta_t, &tfall);
> + if (ret)
> + return ret;
> + ret = aw2026_convert_msec_to_reg(chip, &pattern[3].delta_t, &toff);
> + if (ret)
> + return ret;
> +
> + dev_dbg(&chip->client->dev, "pattern timings: %d %d %d %d\n",
> + trise, ton, tfall, toff);
> +
> + led->cdev.brightness = max(max(pattern[0].brightness, pattern[1].brightness),
> + max(pattern[2].brightness, pattern[3].brightness));
> +
> + return aw2026_pattern_setup(led, trise, ton, tfall, toff);
> +}
> +
> +static int aw2026_pattern_clear(struct led_classdev *cdev)
> +{
> + return aw2026_brightness_set(cdev, LED_OFF);
> +}
> +
> +static int aw2026_blink_set(struct led_classdev *cdev,
> + unsigned long *delay_on, unsigned long *delay_off)
> +{
> + struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
> + struct aw2026 *chip = led->chip;
> + u8 ton, toff;
> + int ret;
> +
> + if (!*delay_on) {
Comment this please.
> + led->cdev.brightness = LED_OFF;
> + return aw2026_brightness_set(&led->cdev, LED_OFF);
> + }
> +
> + led->cdev.brightness = LED_FULL;
> +
> + if (!*delay_off)
As above and for everything else that you think relevant.
> + return aw2026_brightness_set(&led->cdev, LED_FULL);
> +
> + ret = aw2026_convert_msec_to_reg(chip, (u32 *)delay_on, &ton);
> + if (ret)
> + return ret;
> + ret = aw2026_convert_msec_to_reg(chip, (u32 *)delay_off, &toff);
> + if (ret)
> + return ret;
> +
> + dev_dbg(&chip->client->dev, "blink timings: %d %d\n", ton, toff);
> +
> + return aw2026_pattern_setup(led, 0, ton, 0, toff);
> +}
> +
> +static int aw2026_parse_dt(struct i2c_client *client, struct aw2026 *chip)
> +{
> + struct device_node *np = dev_of_node(&client->dev), *child;
> + int count, ret = 0, i = 0;
> + struct aw2026_led *led;
> + const char *str;
> + u32 imax;
> +
> + count = of_get_available_child_count(np);
> + if (!count || count > AW2026_MAX_LEDS)
> + return -EINVAL;
> +
> + if (!of_property_read_u32(np, "awinic,led-max-microamp", &imax)) {
> + chip->imax = min_t(u32, hweight32(imax / 3187 - 1), 3);
No magic numbers. Please define them all.
> + } else {
> + chip->imax = 1; /* 6.375mA */
> + dev_info(&client->dev,
> + "DT property led-max-microamp is missing\n");
> + }
> +
> + for_each_available_child_of_node(np, child) {
> + u32 source;
> +
> + ret = of_property_read_u32(child, "reg", &source);
> + if (ret != 0 || source >= AW2026_MAX_LEDS) {
Nit: !ret
> + dev_err(&chip->client->dev,
> + "Couldn't read LED address: %d\n", ret);
> + count--;
> + continue;
> + }
> +
> + led = &chip->leds[i];
> + led->idx = source;
> + led->chip = chip;
> + led->fwnode = of_fwnode_handle(child);
> +
> + if (!of_property_read_string(child, "default-state", &str)) {
> + if (!strcmp(str, "on"))
> + led->default_state = AW2026_STATE_ON;
> + else if (!strcmp(str, "keep"))
> + led->default_state = AW2026_STATE_KEEP;
> + else
> + led->default_state = AW2026_STATE_OFF;
> + }
> +
> + i++;
> + }
> +
> + if (!count)
> + return -EINVAL;
> +
> + chip->num_leds = i;
> +
> + return 0;
> +}
> +
> +static void aw2026_init_default_state(struct aw2026_led *led)
> +{
> + struct aw2026 *chip = led->chip;
> + int led_on;
> + int ret, idx = led->idx;
> +
> + switch (led->default_state) {
> + case AW2026_STATE_ON:
> + led->cdev.brightness = LED_FULL;
> + break;
> + case AW2026_STATE_KEEP:
> + /* keep setup made in loader */
Please use correct grammar in comments.
"Keep"
> + ret = regmap_read(chip->regmap, AW2026_LEDEN, &led_on);
> + if (ret)
> + return;
> +
> + if (!(led_on & BIT(idx))) {
> + led->cdev.brightness = LED_OFF;
> + break;
> + }
> + regmap_read(chip->regmap, AW2026_ILED(idx),
> + &led->cdev.brightness);
> + return;
> + default:
> + led->cdev.brightness = LED_OFF;
> + }
> +
> + aw2026_brightness_set(&led->cdev, led->cdev.brightness);
> +}
> +
> +static int aw2026_probe(struct i2c_client *client)
> +{
> + struct aw2026 *chip;
> + int i, ret;
> + unsigned int chipid;
Nit: Please swap the last 2 lines.
> + chip = devm_kzalloc(&client->dev, sizeof(struct aw2026), GFP_KERNEL);
sizeof(*chip)
> + if (!chip)
> + return -ENOMEM;
> +
> + ret = aw2026_parse_dt(client, chip);
> + if (ret < 0)
Can aw2026_parse_dt() return >0?
If not, just (ret).
> + return ret;
> +
> + mutex_init(&chip->lock);
> + chip->client = client;
> + i2c_set_clientdata(client, chip);
> +
> + chip->regmap = devm_regmap_init_i2c(client, &aw2026_regmap_config);
> + if (IS_ERR(chip->regmap)) {
> + ret = PTR_ERR(chip->regmap);
> + dev_err(&client->dev, "Failed to allocate register map: %d\n",
> + ret);
> + goto error;
Do you need to have the mutex init'ed by this point?
Might save a lot of trouble to do it as late as possible.
> + }
> +
> + chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc");
> + ret = PTR_ERR_OR_ZERO(chip->vcc_regulator);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
dev_err_probe()?
> + dev_err(&client->dev,
> + "Failed to request regulator: %d\n", ret);
> + goto error;
> + }
> +
> + ret = regulator_enable(chip->vcc_regulator);
> + if (ret) {
> + dev_err(&client->dev,
> + "Failed to enable regulator: %d\n", ret);
> + goto error;
> + }
> +
> + ret = regmap_read(chip->regmap, AW2026_RSTIDR, &chipid);
> + if (ret) {
> + dev_err(&client->dev, "Failed to read chip ID: %d\n", ret);
> + goto error_reg;
> + }
> +
> + if (chipid != AW2026_RSTIDR_CHIP_ID) {
> + ret = -ENODEV;
> + goto error_reg;
> + }
> +
> + for (i = 0; i < chip->num_leds; i++) {
> + struct led_init_data init_data = {};
> + struct aw2026_led *led = &chip->leds[i];
> +
> + aw2026_init_default_state(&chip->leds[i]);
> + led->cdev.brightness_set_blocking = aw2026_brightness_set;
> + led->cdev.blink_set = aw2026_blink_set;
> + led->cdev.pattern_set = aw2026_pattern_set;
> + led->cdev.pattern_clear = aw2026_pattern_clear;
> +
> + init_data.fwnode = chip->leds[i].fwnode;
> +
> + ret = devm_led_classdev_register_ext(&client->dev,
> + &led->cdev, &init_data);
> + if (ret < 0)
> + goto error_reg;
> + }
> +
> + ret = regulator_disable(chip->vcc_regulator);
> + if (ret) {
> + dev_err(&client->dev,
> + "Failed to disable regulator: %d\n", ret);
> + goto error;
> + }
> +
> + return 0;
> +
> +error_reg:
> + regulator_disable(chip->vcc_regulator);
> +error:
> + mutex_destroy(&chip->lock);
> + return ret;
> +}
> +
> +static void aw2026_remove(struct i2c_client *client)
> +{
> + struct aw2026 *chip = i2c_get_clientdata(client);
> +
> + aw2026_chip_disable(chip);
> +
> + mutex_destroy(&chip->lock);
> +}
> +
> +static const struct of_device_id aw2026_match_table[] = {
> + { .compatible = "awinic,aw2026", },
> + { /* sentinel */ },
Remove this comment please.
> +};
> +
> +MODULE_DEVICE_TABLE(of, aw2026_match_table);
> +
> +static struct i2c_driver aw2026_driver = {
> + .driver = {
> + .name = "leds-aw2026",
> + .of_match_table = of_match_ptr(aw2026_match_table),
> + },
> + .probe_new = aw2026_probe,
> + .remove = aw2026_remove,
> +};
> +
Remove this line.
> +module_i2c_driver(aw2026_driver);
> +
> +MODULE_DESCRIPTION("AW2026 LED driver");
> +MODULE_AUTHOR("Vladimir Barinov");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
>
--
Lee Jones [李琼斯]
