This commit adds support for AWINIC AW20036/AW20054/AW20072 LED driver.
This driver supports following AW200XX features:
- Individual 64-level DIM currents
Signed-off-by: Martin Kurbanov <[email protected]>
---
.../testing/sysfs-class-led-driver-aw200xx | 5 +
drivers/leds/Kconfig | 14 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-aw200xx.c | 638 ++++++++++++++++++
4 files changed, 658 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-aw200xx
create mode 100644 drivers/leds/leds-aw200xx.c
diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-aw200xx b/Documentation/ABI/testing/sysfs-class-led-driver-aw200xx
new file mode 100644
index 000000000000..2122e3ee3d95
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-aw200xx
@@ -0,0 +1,5 @@
+What: /sys/class/leds/<led>/dim
+Date: March 2023
+Description: 64-level DIM current. If you write a negative value or
+ "auto", the dim will be calculated according to the
+ brightness.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 499d0f215a8b..546e5aeccc43 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -94,6 +94,20 @@ config LEDS_ARIEL
Say Y to if your machine is a Dell Wyse 3020 thin client.
+config LEDS_AW200XX
+ tristate "LED support for Awinic AW20036/AW20054/AW20072"
+ depends on LEDS_CLASS
+ depends on I2C
+ depends on OF
+ help
+ This option enables support for the AW20036/AW20054/AW20072 LED driver.
+ It is a 3x12/6x9/6x12 matrix LED driver programmed via
+ an I2C interface, up to 36/54/72 LEDs or 12/18/24 RGBs,
+ 3 pattern controllers for auto breathing or group dimming control.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-aw200xx.
+
config LEDS_AW2013
tristate "LED support for Awinic AW2013"
depends on LEDS_CLASS && I2C && OF
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4fd2f92cd198..f611e48cd3f5 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_ASIC3) += leds-asic3.o
+obj-$(CONFIG_LEDS_AW200XX) += leds-aw200xx.o
obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o
diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
new file mode 100644
index 000000000000..83cabb85c26f
--- /dev/null
+++ b/drivers/leds/leds-aw200xx.c
@@ -0,0 +1,638 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Awinic AW20036/AW20054/AW20072 LED driver
+ *
+ * Copyright (c) 2023, SberDevices. All Rights Reserved.
+ *
+ * Author: Martin Kurbanov <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+
+#define AW200XX_LEDS_MAX 72
+#define AW200XX_PATTERN_MAX 3
+#define AW200XX_DIM_MAX (BIT(6) - 1)
+#define AW200XX_FADE_MAX (BIT(8) - 1)
+#define AW200XX_IMAX_DEFAULT_uA 60000
+#define AW200XX_IMAX_MAX_uA 160000
+#define AW200XX_IMAX_MIN_uA 3300
+
+/* Page 0 */
+#define AW200XX_REG_PAGE0_BASE 0xc000
+
+/* Select page register */
+#define AW200XX_REG_PAGE 0xF0
+#define AW200XX_PAGE_MASK (GENMASK(7, 6) | GENMASK(2, 0))
+#define AW200XX_PAGE_SHIFT 0
+#define AW200XX_NUM_PAGES 6
+#define AW200XX_PAGE_SIZE 256
+#define AW200XX_REG(page, reg) \
+ (AW200XX_REG_PAGE0_BASE + (page) * AW200XX_PAGE_SIZE + (reg))
+#define AW200XX_REG_MAX \
+ AW200XX_REG(AW200XX_NUM_PAGES - 1, AW200XX_PAGE_SIZE - 1)
+#define AW200XX_PAGE0 0
+#define AW200XX_PAGE1 1
+#define AW200XX_PAGE2 2
+#define AW200XX_PAGE3 3
+#define AW200XX_PAGE4 4
+#define AW200XX_PAGE5 5
+
+/* Chip ID register */
+#define AW200XX_REG_IDR AW200XX_REG(AW200XX_PAGE0, 0x00)
+#define AW200XX_IDR_CHIPID 0x18
+
+/* Sleep mode register */
+#define AW200XX_REG_SLPCR AW200XX_REG(AW200XX_PAGE0, 0x01)
+#define AW200XX_SLPCR_ACTIVE 0x00
+
+/* Reset register */
+#define AW200XX_REG_RSTR AW200XX_REG(AW200XX_PAGE0, 0x02)
+#define AW200XX_RSTR_RESET 0x01
+
+/* Global current configuration register */
+#define AW200XX_REG_GCCR AW200XX_REG(AW200XX_PAGE0, 0x03)
+#define AW200XX_GCCR_IMAX_MASK GENMASK(7, 4)
+#define AW200XX_GCCR_IMAX(x) ((x) << 4)
+#define AW200XX_GCCR_ALLON BIT(3)
+
+/* Fast clear display control register */
+#define AW200XX_REG_FCD AW200XX_REG(AW200XX_PAGE0, 0x04)
+#define AW200XX_FCD_CLEAR 0x01
+
+/* Interrupt status register */
+#define AW200XX_REG_ISRFLT AW200XX_REG(AW200XX_PAGE0, 0x0B)
+#define AW200XX_ISRFLT_PATIS_MASK GENMASK(6, 4)
+
+/* Pattern enable control register */
+#define AW200XX_REG_PATCR AW200XX_REG(AW200XX_PAGE0, 0x43)
+#define AW200XX_PATCR_PAT_IE_MASK GENMASK(6, 4)
+#define AW200XX_PATCR_PAT_IE_ALL AW200XX_PATCR_PAT_IE_MASK
+#define AW200XX_PATCR_PAT_ENABLE(x) BIT(x)
+
+/*
+ * Maximum breathing level registers
+ * For patterns 0 - 0x44, 1 - 0x45, 2 - 0x46 (step 1)
+ */
+#define AW200XX_REG_PAT0_MAX_BREATH AW200XX_REG(AW200XX_PAGE0, 0x44)
+
+/*
+ * Minimum breathing level registers
+ * For patterns 0 - 0x47, 1 - 0x48, 2 - 0x49 (step 1)
+ */
+#define AW200XX_REG_PAT0_MIN_BREATH AW200XX_REG(AW200XX_PAGE0, 0x47)
+
+/*
+ * Template 1 (rise-time) & template 2 (on-time) configuration register
+ * For patterns 0 - 0x4A, 1 - 0x4E, 2 - 0x52 (step 4)
+ */
+#define AW200XX_REG_PAT0_T0 AW200XX_REG(AW200XX_PAGE0, 0x4A)
+
+/*
+ * Template 3 (fall-time) & template 4 (off-time) configuration register
+ * For patterns 0 - 0x4B, 1 - 0x4F, 2 - 0x53 (step 4)
+ */
+#define AW200XX_REG_PAT0_T1 AW200XX_REG(AW200XX_PAGE0, 0x4B)
+
+/*
+ * Loop configuration registers:
+ * loop end point setting (LE)
+ * loop beginning point setting (LB)
+ * MSB of loop times (LT)
+ * For patterns 0 - 0x4C, 1 - 0x50, 2 - 0x54 (step 4)
+ */
+#define AW200XX_REG_PAT0_T2 AW200XX_REG(AW200XX_PAGE0, 0x4C)
+#define AW200XX_REG_PATX_T2(x) (AW200XX_REG_PAT0_T2 + (x))
+
+/*
+ * Loop configuration registers:
+ * LSB of loop times (LT)
+ * For patterns 0 - 0x4D, 1 - 0x51, 2 - 0x55 (step 4)
+ */
+#define AW200XX_REG_PAT0_T3 AW200XX_REG(AW200XX_PAGE0, 0x4D)
+#define AW200XX_REG_PATX_T3(x) (AW200XX_REG_PAT0_T3 + (x))
+
+#define AW200XX_PAT_T2_LE_MASK GENMASK(7, 6)
+#define AW200XX_PAT_T2_LB_MASK GENMASK(5, 4)
+#define AW200XX_PAT_T2_LT_MASK GENMASK(3, 0)
+#define AW200XX_PAT_T3_LT_MASK GENMASK(7, 0)
+#define AW200XX_PAT0_T2_LT_MSB(x) ((x) >> 8)
+#define AW200XX_PAT0_T3_LT_LSB(x) ((x) & 0xFF)
+#define AW200XX_PAT0_T_LT(msb, lsb) ((msb) << 8 | (lsb))
+#define AW200XX_PAT0_T_LT_MAX (BIT(12) - 1)
+
+#define AW200XX_PAT_T_STEP 4
+
+#define AW200XX_PAT_T1_T3_MASK GENMASK(7, 4)
+#define AW200XX_PAT_T2_T4_MASK GENMASK(3, 0)
+#define AW200XX_TEMPLATE_TIME_MAX (BIT(4) - 1)
+
+/*
+ * Pattern mode configuration register
+ * For patterns 0 - 0x56, 1 - 0x57, 2 - 0x58 (step 1)
+ */
+#define AW200XX_REG_PAT0_CFG AW200XX_REG(AW200XX_PAGE0, 0x56)
+#define AW200XX_PAT_CFG_MODE_MASK BIT(0)
+#define AW200XX_PAT_CFG_RAMP_MASK BIT(1)
+#define AW200XX_PAT_CFG_SWITCH_MASK BIT(2)
+
+/* Start pattern register */
+#define AW200XX_REG_PATGO AW200XX_REG(AW200XX_PAGE0, 0x59)
+#define AW200XX_PATGO(x) BIT(x)
+#define AW200XX_PATGO_RUN(x, run) ((run) << (x))
+#define AW200XX_PATGO_STATE(x) BIT((x) + 4)
+
+/* Display size configuration */
+#define AW200XX_REG_DSIZE AW200XX_REG(AW200XX_PAGE0, 0x80)
+#define AW200XX_DSIZE_COLUMNS_MAX 12
+
+#define AW200XX_LED2REG(x, columns) \
+ ((x) + (((x) / (columns)) * (AW200XX_DSIZE_COLUMNS_MAX - (columns))))
+
+/* Patern selection register*/
+#define AW200XX_REG_PAT_SELECT(x, columns) \
+ AW200XX_REG(AW200XX_PAGE3, AW200XX_LED2REG(x, columns))
+#define AW200XX_PATX_SELECT(x) ((x) + 1)
+
+/*
+ * DIM current configuration register (page 4).
+ * The even address for current DIM configuration.
+ * The odd address for current FADE configuration
+ */
+#define AW200XX_REG_DIM(x, columns) \
+ AW200XX_REG(AW200XX_PAGE4, AW200XX_LED2REG(x, columns) * 2)
+#define AW200XX_REG_DIM2FADE(x) ((x) + 1)
+#define AW200XX_REG_FADE(x, columns) (AW200XX_REG_DIM(x, columns) + 1)
+
+/* Duty ratio of display scan (see p.15 of datasheet for formula) */
+#define AW200XX_DUTY_RATIO(rows) \
+ (((592UL * 1000000UL) / 600500UL) * (1000UL / (rows)) / 1000UL)
+
+struct aw200xx_led {
+ struct led_classdev cdev;
+ struct aw200xx *chip;
+ int dim;
+ u32 num;
+};
+
+struct aw200xx {
+ const struct aw200xx_chipdef *cdef;
+ struct i2c_client *client;
+ struct regmap *regmap;
+ struct mutex mutex;
+ u32 num_leds;
+ u32 display_rows;
+ struct aw200xx_led leds[];
+};
+
+struct aw200xx_chipdef {
+ u32 channels;
+ u32 display_size_rows_max;
+ u32 display_size_columns;
+};
+
+static ssize_t aw200xx_dim_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct aw200xx_led *led = container_of(cdev, struct aw200xx_led, cdev);
+ int dim = led->dim;
+
+ if (dim < 0)
+ return sysfs_emit(buf, "auto\n");
+
+ return sysfs_emit(buf, "%d\n", dim);
+}
+
+static ssize_t aw200xx_dim_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct aw200xx_led *led = container_of(cdev, struct aw200xx_led, cdev);
+ struct aw200xx *chip = led->chip;
+ u32 columns = chip->cdef->display_size_columns;
+ int dim;
+ ssize_t ret;
+
+ if (sysfs_streq(buf, "auto")) {
+ dim = -1;
+ } else {
+ ret = kstrtoint(buf, 0, &dim);
+ if (ret)
+ return ret;
+
+ if (dim > AW200XX_DIM_MAX)
+ return -EINVAL;
+ }
+
+ mutex_lock(&chip->mutex);
+
+ if (dim >= 0) {
+ ret = regmap_write(chip->regmap,
+ AW200XX_REG_DIM(led->num, columns), dim);
+ if (ret)
+ goto exit;
+ }
+
+ led->dim = dim;
+ ret = count;
+
+exit:
+ mutex_unlock(&chip->mutex);
+ return ret;
+}
+static DEVICE_ATTR(dim, 0644, aw200xx_dim_show, aw200xx_dim_store);
+
+static struct attribute *dim_attrs[] = {
+ &dev_attr_dim.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(dim);
+
+static int aw200xx_brightness_set(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ struct aw200xx_led *led = container_of(cdev, struct aw200xx_led, cdev);
+ struct aw200xx *chip = led->chip;
+ int dim;
+ u32 reg;
+ int ret;
+
+ mutex_lock(&chip->mutex);
+
+ reg = AW200XX_REG_DIM(led->num, chip->cdef->display_size_columns);
+ dim = led->dim;
+
+ if (dim < 0) {
+ dim = brightness / (AW200XX_FADE_MAX / AW200XX_DIM_MAX);
+ dim = max(dim, 1);
+ }
+
+ ret = regmap_write(chip->regmap, reg, dim);
+ if (ret)
+ goto error;
+
+ ret = regmap_write(chip->regmap,
+ AW200XX_REG_DIM2FADE(reg), brightness);
+
+error:
+ mutex_unlock(&chip->mutex);
+
+ return ret;
+}
+
+static u32 aw200xx_imax_from_global(const struct aw200xx *const chip,
+ u32 global_imax_uA)
+{
+ unsigned long long duty = AW200XX_DUTY_RATIO(chip->display_rows);
+
+ /* The output current of each LED (see p.14 of datasheet for formula) */
+ return (duty * global_imax_uA) / 1000U;
+}
+
+static u32 aw200xx_imax_to_global(const struct aw200xx *const chip,
+ u32 led_imax_uA)
+{
+ u32 duty = AW200XX_DUTY_RATIO(chip->display_rows);
+
+ /* The output current of each LED (see p.14 of datasheet for formula) */
+ return (led_imax_uA * 1000U) / duty;
+}
+
+static int aw200xx_set_imax(const struct aw200xx *const chip,
+ u32 led_imax_uA)
+{
+ struct imax_global {
+ u32 regval;
+ u32 microamp;
+ } imaxs[] = {
+ { 8, 3300 },
+ { 9, 6700 },
+ { 0, 10000 },
+ { 11, 13300 },
+ { 1, 20000 },
+ { 13, 26700 },
+ { 2, 30000 },
+ { 3, 40000 },
+ { 15, 53300 },
+ { 4, 60000 },
+ { 5, 80000 },
+ { 6, 120000 },
+ { 7, 160000 },
+ };
+ u32 g_imax_uA = aw200xx_imax_to_global(chip, led_imax_uA);
+ int i = ARRAY_SIZE(imaxs);
+
+ while (i--) {
+ if (g_imax_uA >= imaxs[i].microamp)
+ break;
+ }
+ if (i < 0)
+ return -EINVAL;
+
+ return regmap_update_bits(chip->regmap, AW200XX_REG_GCCR,
+ AW200XX_GCCR_IMAX_MASK,
+ AW200XX_GCCR_IMAX(imaxs[i].regval));
+}
+
+static int aw200xx_chip_reset(const struct aw200xx *const chip)
+{
+ int ret;
+
+ ret = regmap_write(chip->regmap, AW200XX_REG_RSTR, AW200XX_RSTR_RESET);
+ if (ret)
+ return ret;
+
+ regcache_mark_dirty(chip->regmap);
+ return regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
+}
+
+static int aw200xx_chip_init(const struct aw200xx *const chip)
+{
+ int ret;
+
+ ret = regmap_write(chip->regmap, AW200XX_REG_DSIZE,
+ chip->display_rows - 1);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(chip->regmap, AW200XX_REG_SLPCR,
+ AW200XX_SLPCR_ACTIVE);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(chip->regmap, AW200XX_REG_GCCR,
+ AW200XX_GCCR_ALLON, AW200XX_GCCR_ALLON);
+}
+
+static int aw200xx_chip_check(const struct aw200xx *const chip)
+{
+ struct device *dev = &chip->client->dev;
+ u32 chipid;
+ int ret;
+
+ ret = regmap_read(chip->regmap, AW200XX_REG_IDR, &chipid);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to read chip ID\n");
+
+ if (chipid != AW200XX_IDR_CHIPID)
+ return dev_err_probe(dev, -ENODEV,
+ "Chip reported wrong ID: %x\n", chipid);
+
+ return 0;
+}
+
+static int aw200xx_probe_dt(struct device *dev, struct aw200xx *chip)
+{
+ struct fwnode_handle *child;
+ u32 current_min, current_max, min_uA;
+ int ret;
+ int i = 0;
+
+ ret = device_property_read_u32(dev, "awinic,display-rows",
+ &chip->display_rows);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to read 'display-rows' property\n");
+
+ if (!chip->display_rows ||
+ chip->display_rows > chip->cdef->display_size_rows_max) {
+ return dev_err_probe(dev, ret,
+ "Invalid leds display size %u\n",
+ chip->display_rows);
+ }
+
+ current_max = aw200xx_imax_from_global(chip, AW200XX_IMAX_MAX_uA);
+ current_min = aw200xx_imax_from_global(chip, AW200XX_IMAX_MIN_uA);
+ min_uA = UINT_MAX;
+
+ device_for_each_child_node(dev, child) {
+ struct led_init_data init_data = {};
+ struct aw200xx_led *led;
+ u32 source, imax;
+
+ ret = fwnode_property_read_u32(child, "reg", &source);
+ if (ret) {
+ dev_err(dev, "Missing reg property\n");
+ chip->num_leds--;
+ continue;
+ }
+
+ if (source >= chip->cdef->channels) {
+ dev_err(dev, "LED reg %u out of range (max %u)\n",
+ source, chip->cdef->channels);
+ chip->num_leds--;
+ continue;
+ }
+
+ ret = fwnode_property_read_u32(child, "led-max-microamp",
+ &imax);
+ if (ret) {
+ dev_info(&chip->client->dev,
+ "DT property led-max-microamp is missing\n");
+ } else if (imax < current_min || imax > current_max) {
+ dev_err(dev, "Invalid value %u for led-max-microamp\n",
+ imax);
+ chip->num_leds--;
+ continue;
+ } else {
+ min_uA = min(min_uA, imax);
+ }
+
+ led = &chip->leds[i];
+ led->dim = -1;
+ led->num = source;
+ led->chip = chip;
+ led->cdev.brightness_set_blocking = aw200xx_brightness_set;
+ led->cdev.groups = dim_groups;
+ init_data.fwnode = child;
+
+ ret = devm_led_classdev_register_ext(dev, &led->cdev,
+ &init_data);
+ if (ret) {
+ fwnode_handle_put(child);
+ break;
+ }
+
+ i++;
+ }
+
+ if (!chip->num_leds)
+ return -EINVAL;
+
+ if (min_uA == UINT_MAX) {
+ min_uA = aw200xx_imax_from_global(chip,
+ AW200XX_IMAX_DEFAULT_uA);
+ }
+
+ return aw200xx_set_imax(chip, min_uA);
+}
+
+static const struct regmap_range_cfg aw200xx_ranges[] = {
+ {
+ .name = "aw200xx",
+ .range_min = 0,
+ .range_max = AW200XX_REG_MAX,
+ .selector_reg = AW200XX_REG_PAGE,
+ .selector_mask = AW200XX_PAGE_MASK,
+ .selector_shift = AW200XX_PAGE_SHIFT,
+ .window_start = 0,
+ .window_len = AW200XX_PAGE_SIZE,
+ },
+};
+
+static const struct regmap_range aw200xx_writeonly_ranges[] = {
+ regmap_reg_range(AW200XX_REG(AW200XX_PAGE1, 0x00), AW200XX_REG_MAX),
+};
+
+static const struct regmap_access_table aw200xx_readable_table = {
+ .no_ranges = aw200xx_writeonly_ranges,
+ .n_no_ranges = ARRAY_SIZE(aw200xx_writeonly_ranges),
+};
+
+static const struct regmap_range aw200xx_readonly_ranges[] = {
+ regmap_reg_range(AW200XX_REG_IDR, AW200XX_REG_IDR),
+ regmap_reg_range(AW200XX_REG_ISRFLT, AW200XX_REG_ISRFLT),
+};
+
+static const struct regmap_access_table aw200xx_writeable_table = {
+ .no_ranges = aw200xx_readonly_ranges,
+ .n_no_ranges = ARRAY_SIZE(aw200xx_readonly_ranges),
+};
+
+static const struct regmap_range aw200xx_volatile_registers[] = {
+ regmap_reg_range(AW200XX_REG_ISRFLT, AW200XX_REG_ISRFLT),
+};
+
+static const struct regmap_access_table aw200xx_volatile_table = {
+ .yes_ranges = aw200xx_volatile_registers,
+ .n_yes_ranges = ARRAY_SIZE(aw200xx_volatile_registers),
+};
+
+static const struct regmap_config aw200xx_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = AW200XX_REG_MAX,
+ .ranges = aw200xx_ranges,
+ .num_ranges = ARRAY_SIZE(aw200xx_ranges),
+ .rd_table = &aw200xx_readable_table,
+ .wr_table = &aw200xx_writeable_table,
+ .volatile_table = &aw200xx_volatile_table,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+static int aw200xx_probe(struct i2c_client *client)
+{
+ const struct aw200xx_chipdef *cdef;
+ struct aw200xx *chip;
+ int count;
+ int ret;
+
+ cdef = device_get_match_data(&client->dev);
+
+ count = device_get_child_node_count(&client->dev);
+ if (!count || count > cdef->channels)
+ return dev_err_probe(&client->dev, -EINVAL,
+ "Incorrect number of leds (%d)", count);
+
+ chip = devm_kzalloc(&client->dev, struct_size(chip, leds, count),
+ GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->cdef = cdef;
+ chip->num_leds = count;
+ chip->client = client;
+ i2c_set_clientdata(client, chip);
+
+ chip->regmap = devm_regmap_init_i2c(client, &aw200xx_regmap_config);
+ if (IS_ERR(chip->regmap))
+ return PTR_ERR(chip->regmap);
+
+ ret = aw200xx_chip_check(chip);
+ if (ret)
+ return ret;
+
+ mutex_init(&chip->mutex);
+
+ /* Need a lock now since after call aw200xx_probe_dt, sysfs nodes created */
+ mutex_lock(&chip->mutex);
+
+ ret = aw200xx_probe_dt(&client->dev, chip);
+ if (ret < 0)
+ goto exit;
+
+ ret = aw200xx_chip_reset(chip);
+ if (ret)
+ goto exit;
+
+ ret = aw200xx_chip_init(chip);
+
+exit:
+ mutex_unlock(&chip->mutex);
+ return ret;
+}
+
+static void aw200xx_remove(struct i2c_client *client)
+{
+ struct aw200xx *chip = i2c_get_clientdata(client);
+
+ aw200xx_chip_reset(chip);
+ mutex_destroy(&chip->mutex);
+}
+
+static const struct aw200xx_chipdef aw20036_cdef = {
+ .channels = 36,
+ .display_size_rows_max = 3,
+ .display_size_columns = 12,
+};
+
+static const struct aw200xx_chipdef aw20054_cdef = {
+ .channels = 54,
+ .display_size_rows_max = 6,
+ .display_size_columns = 9,
+};
+
+static const struct aw200xx_chipdef aw20072_cdef = {
+ .channels = 72,
+ .display_size_rows_max = 6,
+ .display_size_columns = 12,
+};
+
+static const struct i2c_device_id aw200xx_id[] = {
+ { "aw20036" },
+ { "aw20054" },
+ { "aw20072" },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, aw200xx_id);
+
+static const struct of_device_id aw200xx_match_table[] = {
+ { .compatible = "awinic,aw20036", .data = &aw20036_cdef, },
+ { .compatible = "awinic,aw20054", .data = &aw20054_cdef, },
+ { .compatible = "awinic,aw20072", .data = &aw20072_cdef, },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aw200xx_match_table);
+
+static struct i2c_driver aw200xx_driver = {
+ .driver = {
+ .name = "aw200xx",
+ .of_match_table = aw200xx_match_table,
+ },
+ .probe_new = aw200xx_probe,
+ .remove = aw200xx_remove,
+ .id_table = aw200xx_id,
+};
+module_i2c_driver(aw200xx_driver);
+
+MODULE_AUTHOR("Martin Kurbanov <[email protected]>");
+MODULE_DESCRIPTION("AW200XX LED driver");
+MODULE_LICENSE("GPL");
--
2.37.2
Hello Andy. Thank you for review.
I have fixed most of your comments. Please take a look below.
On 2023-03-01 00:51, Andy Shevchenko wrote:
>> + /* The output current of each LED (see p.14 of datasheet for formula) */
>> + return (duty * global_imax_microamp) / 1000U;
>
> units.h ?
These constants are needed to improve the accuracy of calculations.
units.h doesn’t have any helpful definitions to use here.
>> +static int aw200xx_set_imax(const struct aw200xx *const chip,
>> + u32 led_imax_microamp)
>> +{
>> + struct imax_global {
>> + u32 regval;
>> + u32 microamp;
>> + } imaxs[] = {
>> + { 8, 3300 },
>> + { 9, 6700 },
>> + { 0, 10000 },
>> + { 11, 13300 },
>> + { 1, 20000 },
>> + { 13, 26700 },
>> + { 2, 30000 },
>> + { 3, 40000 },
>> + { 15, 53300 },
>> + { 4, 60000 },
>> + { 5, 80000 },
>> + { 6, 120000 },
>> + { 7, 160000 },
>
> This looks a bit random. Is there any pattern on how value is
> connected to the register value?
There is no ability to create any pattern here, because this table data
doesn’t have any regularity.
--
Best Regards,
Kurbanov Martin
On Tue, Mar 14, 2023 at 2:12 PM Martin Kurbanov
<[email protected]> wrote:
>
> Hello Andy. Thank you for review.
> I have fixed most of your comments. Please take a look below.
Good, but you can postpone issuing a new version before letting me answer.
> On 2023-03-01 00:51, Andy Shevchenko wrote:
> >> + /* The output current of each LED (see p.14 of datasheet for formula) */
> >> + return (duty * global_imax_microamp) / 1000U;
> >
> > units.h ?
>
> These constants are needed to improve the accuracy of calculations.
> units.h doesn’t have any helpful definitions to use here.
Okay, let me look at v3 and I will comment there.
...
> >> +static int aw200xx_set_imax(const struct aw200xx *const chip,
> >> + u32 led_imax_microamp)
> >> +{
> >> + struct imax_global {
> >> + u32 regval;
> >> + u32 microamp;
> >> + } imaxs[] = {
> >> + { 8, 3300 },
> >> + { 9, 6700 },
> >> + { 0, 10000 },
> >> + { 11, 13300 },
> >> + { 1, 20000 },
> >> + { 13, 26700 },
> >> + { 2, 30000 },
> >> + { 3, 40000 },
> >> + { 15, 53300 },
> >> + { 4, 60000 },
> >> + { 5, 80000 },
> >> + { 6, 120000 },
> >> + { 7, 160000 },
> >
> > This looks a bit random. Is there any pattern on how value is
> > connected to the register value?
>
> There is no ability to create any pattern here, because this table data
> doesn’t have any regularity.
There is a clear pattern.
You have two tables, i.e. with multiplier 10000 and second one with
multiplier 3333 (table in the datasheet seems bad from a math
perspective). And it's even shown correctly in the datasheet.
With this mix you missed 10.
The coefficient table is 1,2,3,4,6,8,12,16 for both tables.
Hence you need one table and two multipliers.
Please, rewrite accordingly.
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 14, 2023 at 2:03 PM Martin Kurbanov
<[email protected]> wrote:
>
> This commit adds support for AWINIC AW20036/AW20054/AW20072 LED driver.
> This driver supports following AW200XX features:
> - Individual 64-level DIM currents
As said, please give a chance reviewer to answer your questions before
issuing a new version. You will save yours time in the first place and
reviewer's and others' at the second.
...
> +config LEDS_AW200XX
> + tristate "LED support for Awinic AW20036/AW20054/AW20072"
> + depends on LEDS_CLASS
> + depends on I2C
> + depends on OF
I don't see any dependency. Do you?
...
> +#include <linux/bitfield.h>
bits.h needs to be included, it's not guaranteed to be by any of the
present headers in this list.
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
+ container_of.h
...
> +#define AW200XX_PAT0_T2_LT_MSB(x) ((x) >> 8)
> +#define AW200XX_PAT0_T3_LT_LSB(x) ((x) & 0xFF)
> +#define AW200XX_PAT0_T_LT(msb, lsb) ((msb) << 8 | (lsb))
Are these being in use anyhow?
Please, do not add dead definitions if they are not currently in use
and do not add value to understanding how hardware works for the
implemented things.
...
> +/* Patern selection register*/
Pattern
...
> +/* Duty ratio of display scan (see p.15 of datasheet for formula) */
> +#define AW200XX_DUTY_RATIO(rows) \
> + (((592UL * 1000000UL) / 600500UL) * (1000UL / (rows)) / 1000UL)
Instead of referring to a page in the datasheet, quote the formula and
a bit of text.
So, now as I read it I can tell how it can be improved.
The first six 0:s multiplier is actually USEC_PER_SEC in the kernel.
The 1000 is your addition which needs to be explained. It may be just
MILLI or KILO depending on what you have in mind with it.
...
> +struct aw200xx {
> + const struct aw200xx_chipdef *cdef;
> + struct i2c_client *client;
> + struct regmap *regmap;
You may derive this from &client->dev, right? The API dev_get_regmap().
> + struct mutex mutex;
> + u32 num_leds;
> + u32 display_rows;
> + struct aw200xx_led leds[];
> +};
...
> + if (sysfs_streq(buf, "auto")) {
> + dim = -1;
> + } else {
> + ret = kstrtoint(buf, 0, &dim);
> + if (ret)
> + return ret;
> +
> + if (dim > AW200XX_DIM_MAX)
> + return -EINVAL;
And if dim is < 0, does ir mean "auto"? If so, is it documented?
> + }
...
> +static DEVICE_ATTR(dim, 0644, aw200xx_dim_show, aw200xx_dim_store);
DEVICE_ATTR_RW() ?
...
> + mutex_lock(&chip->mutex);
> +
> + reg = AW200XX_REG_DIM(led->num, chip->cdef->display_size_columns);
> + dim = led->dim;
> +
This blank line should go before a dim assignment.
> + if (dim < 0) {
> + dim = brightness / (AW200XX_FADE_MAX / AW200XX_DIM_MAX);
> + dim = max(dim, 1);
Can it be written in a single assignment?
dim = max(brightness / (AW200XX_FADE_MAX / AW200XX_DIM_MAX), 1)
> + }
...
> +error:
In one function it's called the exit, here it's the error, please be
consistent and I think the more precise is to use something like
'out_unlock'.
> + mutex_unlock(&chip->mutex);
> +
> + return ret;
> +}
...
> + /* The output current of each LED (see p.14 of datasheet for formula) */
Again, put more comments. Do not push the reader to find a Datasheet
which I do not see the link to in the commit message.
> + return (duty * global_imax_uA) / 1000U;
So, this seems like the same meaning as 1000 in the macro, correct?
Please explain the use of it.
The best, if you think units.h doesn't provide it, define in your driver.
...
> + /* The output current of each LED (see p.14 of datasheet for formula) */
> + return (led_imax_uA * 1000U) / duty;
All the same as per above.
...
> + {
> + { 8, 3300 },
> + { 9, 6700 },
> + { 0, 10000 },
> + { 11, 13300 },
> + { 1, 20000 },
> + { 13, 26700 },
> + { 2, 30000 },
> + { 3, 40000 },
> + { 15, 53300 },
> + { 4, 60000 },
> + { 5, 80000 },
> + { 6, 120000 },
> + { 7, 160000 },
> + };
Yeah, as per previous email, use exactly what Datasheet provides you.
There is a clear pattern and clear math mistakes in the datasheet
itself.
...
> +static const struct regmap_config aw200xx_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = AW200XX_REG_MAX,
> + .ranges = aw200xx_ranges,
> + .num_ranges = ARRAY_SIZE(aw200xx_ranges),
> + .rd_table = &aw200xx_readable_table,
> + .wr_table = &aw200xx_writeable_table,
> + .volatile_table = &aw200xx_volatile_table,
> + .cache_type = REGCACHE_RBTREE,
You have a mutex, why do you need to use regmap's lock?
> +};
...
> + cdef = device_get_match_data(&client->dev);
Is it fine if it's NULL?
...
> + chip->cdef = cdef;
Can it be assigned directly here w.o temporary variable?
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 14, 2023 at 6:22 PM Andy Shevchenko
<[email protected]> wrote:
> On Tue, Mar 14, 2023 at 2:12 PM Martin Kurbanov
> <[email protected]> wrote:
> > On 2023-03-01 00:51, Andy Shevchenko wrote:
...
> > >> +static int aw200xx_set_imax(const struct aw200xx *const chip,
> > >> + u32 led_imax_microamp)
> > >> +{
> > >> + struct imax_global {
> > >> + u32 regval;
> > >> + u32 microamp;
> > >> + } imaxs[] = {
> > >> + { 8, 3300 },
> > >> + { 9, 6700 },
> > >> + { 0, 10000 },
> > >> + { 11, 13300 },
> > >> + { 1, 20000 },
> > >> + { 13, 26700 },
> > >> + { 2, 30000 },
> > >> + { 3, 40000 },
> > >> + { 15, 53300 },
> > >> + { 4, 60000 },
> > >> + { 5, 80000 },
> > >> + { 6, 120000 },
> > >> + { 7, 160000 },
> > >
> > > This looks a bit random. Is there any pattern on how value is
> > > connected to the register value?
> >
> > There is no ability to create any pattern here, because this table data
> > doesn’t have any regularity.
>
> There is a clear pattern.
>
> You have two tables, i.e. with multiplier 10000 and second one with
> multiplier 3333 (table in the datasheet seems bad from a math
> perspective). And it's even shown correctly in the datasheet.
>
> With this mix you missed 10.
>
> The coefficient table is 1,2,3,4,6,8,12,16 for both tables.
>
> Hence you need one table and two multipliers.
>
> Please, rewrite accordingly.
JFYI: You may see how I killed a table in one driver due to missing
understanding that there is a pattern.
9df461eca18f ("spi: pxa2xx: replace ugly table by approximation")
--
With Best Regards,
Andy Shevchenko
Hi Martin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pavel-leds/for-next]
[cannot apply to lee-leds/for-leds-next robh/for-next linus/master v6.3-rc2 next-20230315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Martin-Kurbanov/leds-add-aw20xx-driver/20230314-210251
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link: https://lore.kernel.org/r/20230314120252.48263-3-mmkurbanov%40sberdevices.ru
patch subject: [PATCH v3 2/2] leds: add aw20xx driver
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230315/[email protected]/config)
compiler: mips-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/9760204574f3c53a9753b6daf20125c8552ce8ae
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Martin-Kurbanov/leds-add-aw20xx-driver/20230314-210251
git checkout 9760204574f3c53a9753b6daf20125c8552ce8ae
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
arch/mips/kernel/head.o: in function `kernel_entry':
(.ref.text+0xac): relocation truncated to fit: R_MIPS_26 against `start_kernel'
init/main.o: in function `set_reset_devices':
main.c:(.init.text+0x20): relocation truncated to fit: R_MIPS_26 against `_mcount'
main.c:(.init.text+0x30): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
init/main.o: in function `debug_kernel':
main.c:(.init.text+0xa4): relocation truncated to fit: R_MIPS_26 against `_mcount'
main.c:(.init.text+0xb4): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
init/main.o: in function `quiet_kernel':
main.c:(.init.text+0x128): relocation truncated to fit: R_MIPS_26 against `_mcount'
main.c:(.init.text+0x138): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
init/main.o: in function `warn_bootconfig':
main.c:(.init.text+0x1ac): relocation truncated to fit: R_MIPS_26 against `_mcount'
main.c:(.init.text+0x1bc): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
init/main.o: in function `init_setup':
main.c:(.init.text+0x234): relocation truncated to fit: R_MIPS_26 against `_mcount'
main.c:(.init.text+0x254): additional relocation overflows omitted from the output
mips-linux-ld: drivers/leds/leds-aw200xx.o: in function `aw200xx_probe_dt':
>> leds-aw200xx.c:(.text.aw200xx_probe_dt+0x164): undefined reference to `__udivdi3'
>> mips-linux-ld: leds-aw200xx.c:(.text.aw200xx_probe_dt+0x184): undefined reference to `__udivdi3'
mips-linux-ld: leds-aw200xx.c:(.text.aw200xx_probe_dt+0x53c): undefined reference to `__udivdi3'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests