* [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight
[not found] <20251026123923.1531727-1-caojunjie650@gmail.com>
@ 2025-10-26 12:39 ` Junjie Cao
2025-10-26 13:47 ` Krzysztof Kozlowski
2025-10-26 12:39 ` [PATCH 2/2] backlight: aw99706: Add support for " Junjie Cao
1 sibling, 1 reply; 11+ messages in thread
From: Junjie Cao @ 2025-10-26 12:39 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev,
Junjie Cao
Add Awinic AW99706 backlight binding documentation.
Signed-off-by: Junjie Cao <caojunjie650@gmail.com>
---
.../leds/backlight/awinic,aw99706.yaml | 135 ++++++++++++++++++
1 file changed, 135 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml
diff --git a/Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml b/Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml
new file mode 100644
index 000000000..640af3891
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/awinic,aw99706.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Awinic AW99706 6-channel WLED Backlight Driver
+
+maintainers:
+ - Junjie Cao <caojunjie650@gmail.com>
+
+allOf:
+ - $ref: common.yaml#
+
+properties:
+ compatible:
+ const: awinic,aw99706
+
+ reg:
+ maxItems: 1
+
+ enable-gpios:
+ description: GPIO to use to enable/disable the backlight (HWEN pin).
+ maxItems: 1
+
+ awinic,dim-mode:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: >
+ Select dimming mode of the device.
+ 0 = Bypass mode.
+ 1 = DC mode.
+ 2 = MIX mode.
+ 3 = MIX-26k.
+ enum: [0, 1, 2, 3]
+ default: 1
+
+ awinic,sw-freq:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Boost switching frequency in kHz.
+ enum: [300, 400, 500, 600, 660, 750, 850, 1000, 1200, 1330, 1500, 1700]
+ default: 750
+
+ awinic,sw-ilmt:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Switching current limitation in mA.
+ enum: [1500, 2000, 2500, 3000]
+ default: 3000
+
+ awinic,iled-max:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Maximum LED current setting in uA.
+ minimum: 5000
+ maximum: 50000
+ multipleOf: 500
+ default: 20000
+
+ awinic,uvlo-thres:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: UVLO(Under Voltage Lock Out) in mV.
+ enum: [2200, 5000]
+ default: 2200
+
+ awinic,fade-time:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Fade In/Out Time(per step) in us.
+ enum: [8, 16, 32, 64, 128, 256, 512, 1024]
+ default: 16
+
+ awinic,slope-time:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Slope time in ms.
+ enum: [8, 24, 48, 96, 200, 300, 400, 500]
+ default: 300
+
+ awinic,ramp-ctl:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: >
+ Select ramp control and filter of the device.
+ 0 = Fade in/fade out.
+ 1 = Light filter.
+ 2 = Medium filter.
+ 3 = Heavy filter.
+ enum: [0, 1, 2, 3]
+ default: 2
+
+ awinic,brt-mode:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: >
+ Select brightness control of the device.
+ 0 = PWM.
+ 1 = IIC.
+ 2 = IIC x PWM.
+ 3 = IIC x PWM(P-ramp).
+ enum: [0, 1, 2, 3]
+ default: 1
+
+ awinic,onoff-time:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Turn on/off time(per step) in ns.
+ enum: [250, 500, 1000, 2000, 4000, 8000, 16000]
+ default: 2000
+
+required:
+ - compatible
+ - reg
+ - enable-gpios
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ aw99706@76 {
+ compatible = "awinic,aw99706";
+ reg = <0x76>;
+ enable-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
+ awinic,dim-mode = <1>;
+ awinic,sw-ilmt = <3000>;
+ awinic,sw-freq = <750>;
+ awinic,uvlo-thres = <2200>;
+ awinic,iled-max = <23500>;
+ awinic,ramp-ctl = <2>;
+ awinic,slope-time = <96>;
+ awinic,fade-time = <16>;
+ awinic,brt-mode = <1>;
+ awinic,onoff-time = <2000>;
+ };
+ };
+
+...
--
2.51.1.dirty
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
[not found] <20251026123923.1531727-1-caojunjie650@gmail.com>
2025-10-26 12:39 ` [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight Junjie Cao
@ 2025-10-26 12:39 ` Junjie Cao
2025-10-27 12:06 ` kernel test robot
` (2 more replies)
1 sibling, 3 replies; 11+ messages in thread
From: Junjie Cao @ 2025-10-26 12:39 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev,
Junjie Cao, Pengyu Luo
Add support for Awinic AW99706 backlight, which can be found in
tablet and notebook backlight, one case is the Lenovo Legion Y700
Gen4. This driver refers to the official datasheets and android
driver, they can be found in [1].
[1] https://www.awinic.com/en/productDetail/AW99706QNR
Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
Signed-off-by: Junjie Cao <caojunjie650@gmail.com>
---
MAINTAINERS | 6 +
drivers/video/backlight/Kconfig | 8 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/aw99706.c | 503 ++++++++++++++++++++++++++++++
4 files changed, 518 insertions(+)
create mode 100644 drivers/video/backlight/aw99706.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 5ea78444f..cef23fcaa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4132,6 +4132,12 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/adc/avia-hx711.yaml
F: drivers/iio/adc/hx711.c
+AWINIC AW99706 WLED BACKLIGHT DRIVER
+M: Junjie Cao <caojunjie650@gmail.com>
+S: Maintained
+F: Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml
+F: drivers/video/backlight/aw99706.c
+
AX.25 NETWORK LAYER
L: linux-hams@vger.kernel.org
S: Orphan
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index d9374d208..35c7bfad0 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -156,6 +156,14 @@ config BACKLIGHT_ATMEL_LCDC
If in doubt, it's safe to enable this option; it doesn't kick
in unless the board's description says it's wired that way.
+config BACKLIGHT_AW99706
+ tristate "Backlight Driver for Awinic AW99706"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ If you have a LCD backlight connected to the WLED output of AW99706
+ WLED output, say Y here to enable this driver.
+
config BACKLIGHT_EP93XX
tristate "Cirrus EP93xx Backlight Driver"
depends on FB_EP93XX
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index dfbb169bf..a5d62b018 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_BACKLIGHT_ADP8870) += adp8870_bl.o
obj-$(CONFIG_BACKLIGHT_APPLE) += apple_bl.o
obj-$(CONFIG_BACKLIGHT_APPLE_DWI) += apple_dwi_bl.o
obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
+obj-$(CONFIG_BACKLIGHT_AW99706) += aw99706.o
obj-$(CONFIG_BACKLIGHT_BD6107) += bd6107.o
obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
obj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o
diff --git a/drivers/video/backlight/aw99706.c b/drivers/video/backlight/aw99706.c
new file mode 100644
index 000000000..8dafdea45
--- /dev/null
+++ b/drivers/video/backlight/aw99706.c
@@ -0,0 +1,503 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * aw99706 - Backlight driver for the AWINIC AW99706
+ *
+ * Copyright (C) 2025 Junjie Cao <caojunjie650@gmail.com>
+ * Copyright (C) 2025 Pengyu Luo <mitltlatltl@gmail.com>
+ *
+ * Based on vendor driver:
+ * Copyright (c) 2023 AWINIC Technology CO., LTD
+ */
+
+#include <linux/backlight.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define AW99706_MAX_BRT_LVL 4095
+#define AW99706_REG_MAX 0x1F
+#define AW99706_ID 0x07
+
+/* registers list */
+#define AW99706_CFG0_REG 0x00
+#define AW99706_DIM_MODE_MASK GENMASK(1, 0)
+
+#define AW99706_CFG1_REG 0x01
+#define AW99706_SW_FREQ_MASK GENMASK(3, 0)
+#define AW99706_SW_ILMT_MASK GENMASK(5, 4)
+
+#define AW99706_CFG2_REG 0x02
+#define AW99706_ILED_MAX_MASK GENMASK(6, 0)
+#define AW99706_UVLOSEL_MASK BIT(7)
+
+#define AW99706_CFG3_REG 0x03
+#define AW99706_CFG4_REG 0x04
+#define AW99706_BRT_MSB_MASK GENMASK(3, 0)
+
+#define AW99706_CFG5_REG 0x05
+#define AW99706_BRT_LSB_MASK GENMASK(7, 0)
+
+#define AW99706_CFG6_REG 0x06
+#define AW99706_FADE_TIME_MASK GENMASK(2, 0)
+#define AW99706_SLOPE_TIME_MASK GENMASK(5, 3)
+#define AW99706_RAMP_CTL_MASK GENMASK(7, 6)
+
+#define AW99706_CFG7_REG 0x07
+#define AW99706_BRT_MODE_MASK GENMASK(1, 0)
+
+#define AW99706_CFG8_REG 0x08
+#define AW99706_ONOFF_TIME_MASK GENMASK(2, 0)
+
+#define AW99706_CFG9_REG 0x09
+#define AW99706_CFGA_REG 0x0A
+#define AW99706_CFGB_REG 0x0B
+#define AW99706_CFGC_REG 0x0C
+#define AW99706_CFGD_REG 0x0D
+#define AW99706_FLAG_REG 0x10
+#define AW99706_BACKLIGHT_EN_MASK BIT(7)
+
+#define AW99706_CHIPID_REG 0x11
+#define AW99706_LED_OPEN_FLAG_REG 0x12
+#define AW99706_LED_SHORT_FLAG_REG 0x13
+#define AW99706_MTPLDOSEL_REG 0x1E
+#define AW99706_MTPRUN_REG 0x1F
+
+#define RESV 0
+
+/* Boost switching frequency table, in kHz */
+static const u32 aw99706_sw_freq_tbl[] = {
+ RESV, RESV, RESV, RESV, 300, 400, 500, 600,
+ 660, 750, 850, 1000, 1200, 1330, 1500, 1700
+};
+
+/* Switching current limitation table, in mA */
+static const u32 aw99706_sw_ilmt_tbl[] = {
+ 1500, 2000, 2500, 3000
+};
+
+/* ULVO threshold table, in mV */
+static const u32 aw99706_ulvo_thres_tbl[] = {
+ 2200, 5000
+};
+
+/* Fade In/Out time table, in us */
+static const u32 aw99706_fade_time_tbl[] = {
+ 8, 16, 32, 64, 128, 256, 512, 1024
+};
+
+/* Slope time table, in ms */
+static const u32 aw99706_slopetime_tbl[] = {
+ 8, 24, 48, 96, 200, 300, 400, 500
+};
+
+/* Turn on/off time table, in ns */
+static const u32 aw99706_onoff_time_tbl[] = {
+ RESV, 250, 500, 1000, 2000, 4000, 8000, 16000
+};
+
+struct aw99706_device {
+ struct i2c_client *client;
+ struct device *dev;
+ struct regmap *regmap;
+ struct backlight_device *bl_dev;
+ struct gpio_desc *hwen_gpio;
+ bool bl_enable;
+};
+
+enum reg_access {
+ REG_NONE_ACCESS = 0,
+ REG_RD_ACCESS = 1,
+ REG_WR_ACCESS = 2,
+};
+
+struct aw99706_reg {
+ u8 defval;
+ u8 access;
+};
+
+const struct aw99706_reg aw99706_regs[AW99706_REG_MAX + 1] = {
+ [AW99706_CFG0_REG] = {0x65, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG1_REG] = {0x39, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG2_REG] = {0x1e, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG3_REG] = {0x04, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG4_REG] = {0x00, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG5_REG] = {0x00, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG6_REG] = {0xa9, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG7_REG] = {0x04, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG8_REG] = {0x0c, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFG9_REG] = {0x4b, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFGA_REG] = {0x72, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFGB_REG] = {0x01, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFGC_REG] = {0x6c, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_CFGD_REG] = {0xfe, REG_RD_ACCESS | REG_WR_ACCESS},
+ [AW99706_FLAG_REG] = {0x00, REG_RD_ACCESS},
+ [AW99706_CHIPID_REG] = {AW99706_ID, REG_RD_ACCESS},
+ [AW99706_LED_OPEN_FLAG_REG] = {0x00, REG_RD_ACCESS},
+ [AW99706_LED_SHORT_FLAG_REG] = {0x00, REG_RD_ACCESS},
+
+ /*
+ * Write bit is dropped here, writing BIT(0) to MTPLDOSEL will unlock
+ * Multi-time Programmable (MTP).
+ */
+ [AW99706_MTPLDOSEL_REG] = {0x00, REG_RD_ACCESS},
+ [AW99706_MTPRUN_REG] = {0x00, REG_NONE_ACCESS},
+};
+
+static bool aw99706_readable_reg(struct device *dev, unsigned int reg)
+{
+ return aw99706_regs[reg].access & REG_RD_ACCESS;
+}
+
+static bool aw99706_writeable_reg(struct device *dev, unsigned int reg)
+{
+ return aw99706_regs[reg].access & REG_WR_ACCESS;
+}
+
+static inline int aw99706_i2c_read(struct aw99706_device *aw, u8 reg,
+ unsigned int *val)
+{
+ return regmap_read(aw->regmap, reg, val);
+}
+
+static inline int aw99706_i2c_write(struct aw99706_device *aw, u8 reg, u8 val)
+{
+ return regmap_write(aw->regmap, reg, val);
+}
+
+static inline int aw99706_i2c_update_bits(struct aw99706_device *aw, u8 reg,
+ u8 mask, u8 val)
+{
+ return regmap_update_bits(aw->regmap, reg, mask, val);
+}
+
+struct aw99706_dt_prop {
+ const char * const name;
+ const u32 * const lookup_tbl;
+ u8 tbl_size;
+ u8 reg;
+ u8 mask;
+ u8 val;
+ u32 raw_val;
+};
+
+static struct aw99706_dt_prop aw99706_dt_props[] = {
+ {
+ "awinic,dim-mode", NULL,
+ 0,
+ AW99706_CFG0_REG, AW99706_DIM_MODE_MASK
+ },
+ {
+ "awinic,sw-freq", aw99706_sw_freq_tbl,
+ ARRAY_SIZE(aw99706_sw_freq_tbl),
+ AW99706_CFG1_REG, AW99706_SW_FREQ_MASK
+ },
+ {
+ "awinic,sw-ilmt", aw99706_sw_ilmt_tbl,
+ ARRAY_SIZE(aw99706_sw_ilmt_tbl),
+ AW99706_CFG1_REG, AW99706_SW_ILMT_MASK
+ },
+ {
+ "awinic,iled-max", NULL,
+ 0,
+ AW99706_CFG2_REG, AW99706_ILED_MAX_MASK
+
+ },
+ {
+ "awinic,uvlo-thres", aw99706_ulvo_thres_tbl,
+ ARRAY_SIZE(aw99706_ulvo_thres_tbl),
+ AW99706_CFG2_REG, AW99706_UVLOSEL_MASK
+ },
+ {
+ "awinic,fade-time", aw99706_fade_time_tbl,
+ ARRAY_SIZE(aw99706_fade_time_tbl),
+ AW99706_CFG6_REG, AW99706_FADE_TIME_MASK
+ },
+ {
+ "awinic,slope-time", aw99706_slopetime_tbl,
+ ARRAY_SIZE(aw99706_slopetime_tbl),
+ AW99706_CFG6_REG, AW99706_SLOPE_TIME_MASK
+ },
+ {
+ "awinic,ramp-ctl", NULL,
+ 0,
+ AW99706_CFG6_REG, AW99706_RAMP_CTL_MASK
+ },
+ {
+ "awinic,brt-mode", NULL,
+ 0,
+ AW99706_CFG7_REG, AW99706_BRT_MODE_MASK
+ },
+ {
+ "awinic,onoff-time", aw99706_onoff_time_tbl,
+ ARRAY_SIZE(aw99706_onoff_time_tbl),
+ AW99706_CFG8_REG, AW99706_ONOFF_TIME_MASK
+ },
+};
+
+static int aw99706_lookup(const u32 * const tbl, int size, u32 val)
+{
+ int i;
+
+ for (i = 0; i < size; i++)
+ if (tbl[i] == val)
+ return i;
+
+ return -1;
+}
+
+static inline void aw99706_prop_set_default(struct aw99706_dt_prop *prop)
+{
+ prop->val = prop->mask & aw99706_regs[prop->reg].defval;
+}
+
+static void aw99706_dt_property_convert(struct aw99706_dt_prop *prop)
+{
+ unsigned int val, shift;
+
+ if (prop->lookup_tbl) {
+ val = aw99706_lookup(prop->lookup_tbl, prop->tbl_size,
+ prop->raw_val);
+ if (val < 0) {
+ aw99706_prop_set_default(prop);
+ return;
+ }
+
+ } else {
+ val = prop->raw_val;
+ }
+
+ shift = ffs(prop->mask) - 1;
+ val <<= shift;
+ prop->val = prop->mask & val;
+}
+
+static void aw99706_dt_parse(struct aw99706_device *aw)
+{
+ struct aw99706_dt_prop *prop;
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
+ prop = &aw99706_dt_props[i];
+ ret = device_property_read_u32(aw->dev, prop->name,
+ &prop->raw_val);
+ if (ret < 0) {
+ dev_warn(aw->dev, "Missing property %s: %d\n",
+ prop->name, ret);
+
+ aw99706_prop_set_default(prop);
+ } else {
+ aw99706_dt_property_convert(prop);
+ }
+ }
+
+ /* This property requires a long linear array, using formula for now */
+ aw99706_dt_props[3].val = (aw99706_dt_props[3].raw_val - 5000) / 500;
+}
+
+static int aw99706_hw_init(struct aw99706_device *aw)
+{
+ int ret, i;
+
+ gpiod_set_value_cansleep(aw->hwen_gpio, 1);
+
+ for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
+ ret = aw99706_i2c_update_bits(aw, aw99706_dt_props[i].reg,
+ aw99706_dt_props[i].mask,
+ aw99706_dt_props[i].val);
+ if (ret < 0) {
+ dev_err(aw->dev, "Failed to write init data %d\n", ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int aw99706_bl_enable(struct aw99706_device *aw, bool en)
+{
+ int ret;
+ u8 val;
+
+ FIELD_MODIFY(AW99706_BACKLIGHT_EN_MASK, &val, en);
+ ret = aw99706_i2c_update_bits(aw, AW99706_CFGD_REG,
+ AW99706_BACKLIGHT_EN_MASK, val);
+ if (ret)
+ dev_err(aw->dev, "Failed to enable backlight!\n");
+
+ return ret;
+}
+
+static int aw99706_backlight_switch(struct aw99706_device *aw, u32 brt_lvl)
+{
+ bool bl_enable_now = !!brt_lvl;
+ int ret = 0;
+
+ if (aw->bl_enable != bl_enable_now) {
+ aw->bl_enable = bl_enable_now;
+ ret = aw99706_bl_enable(aw, bl_enable_now);
+ }
+
+ return ret;
+}
+
+static int aw99706_update_brightness(struct aw99706_device *aw, u32 brt_lvl)
+{
+ int ret;
+
+ ret = aw99706_i2c_write(aw, AW99706_CFG4_REG,
+ (brt_lvl >> 8) & AW99706_BRT_MSB_MASK);
+ if (ret < 0)
+ return ret;
+
+ ret = aw99706_i2c_write(aw, AW99706_CFG5_REG,
+ brt_lvl & AW99706_BRT_LSB_MASK);
+ if (ret < 0)
+ return ret;
+
+ return aw99706_backlight_switch(aw, brt_lvl);
+}
+
+static int aw99706_bl_update_status(struct backlight_device *bl)
+{
+ struct aw99706_device *aw = bl_get_data(bl);
+
+ return aw99706_update_brightness(aw, bl->props.brightness);
+}
+
+static const struct backlight_ops aw99706_bl_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = aw99706_bl_update_status,
+};
+
+static const struct regmap_config aw99706_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = AW99706_REG_MAX,
+ .writeable_reg = aw99706_writeable_reg,
+ .readable_reg = aw99706_readable_reg,
+};
+
+static int aw99706_chip_id_read(struct aw99706_device *aw)
+{
+ int ret;
+ unsigned int val;
+
+ ret = aw99706_i2c_read(aw, AW99706_CHIPID_REG, &val);
+ if (ret < 0)
+ return ret;
+
+ return val;
+}
+
+static int aw99706_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct aw99706_device *aw;
+ struct backlight_device *bl_dev;
+ struct backlight_properties props = {};
+ int ret = 0;
+
+ aw = devm_kzalloc(dev, sizeof(*aw), GFP_KERNEL);
+ if (!aw)
+ return -ENOMEM;
+
+ aw->client = client;
+ aw->dev = dev;
+ i2c_set_clientdata(client, aw);
+
+ aw->regmap = devm_regmap_init_i2c(client, &aw99706_regmap_config);
+ if (IS_ERR(aw->regmap))
+ return dev_err_probe(dev, PTR_ERR(aw->regmap),
+ "Failed to init regmap\n");
+
+ ret = aw99706_chip_id_read(aw);
+ if (ret != AW99706_ID)
+ return dev_err_probe(dev, ret,
+ "Failed to validate chip id\n");
+
+ aw99706_dt_parse(aw);
+
+ aw->hwen_gpio = devm_gpiod_get(aw->dev, "enable", GPIOD_OUT_LOW);
+ if (IS_ERR(aw->hwen_gpio))
+ return dev_err_probe(dev, PTR_ERR(aw->hwen_gpio),
+ "Failed to get enable gpio\n");
+
+ ret = aw99706_hw_init(aw);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Failed to initialize the chip\n");
+
+ props.type = BACKLIGHT_RAW;
+ props.brightness = AW99706_MAX_BRT_LVL >> 1;
+ props.max_brightness = AW99706_MAX_BRT_LVL;
+ props.scale = BACKLIGHT_SCALE_LINEAR;
+
+ bl_dev = devm_backlight_device_register(dev, "aw99706-backlight", dev,
+ aw, &aw99706_bl_ops, &props);
+ if (IS_ERR(bl_dev))
+ return dev_err_probe(dev, PTR_ERR(bl_dev),
+ "Failed to register backlight!\n");
+
+ aw->bl_dev = bl_dev;
+
+ return 0;
+}
+
+static void aw99706_remove(struct i2c_client *client)
+{
+ struct aw99706_device *aw = i2c_get_clientdata(client);
+
+ aw99706_update_brightness(aw, 0);
+
+ msleep(50);
+
+ gpiod_set_value_cansleep(aw->hwen_gpio, 0);
+}
+
+static int aw99706_suspend(struct device *dev)
+{
+ struct aw99706_device *aw = dev_get_drvdata(dev);
+
+ return aw99706_update_brightness(aw, 0);
+}
+
+static int aw99706_resume(struct device *dev)
+{
+ struct aw99706_device *aw = dev_get_drvdata(dev);
+
+ return aw99706_hw_init(aw);
+}
+
+static SIMPLE_DEV_PM_OPS(aw99706_pm_ops, aw99706_suspend, aw99706_resume);
+
+static const struct i2c_device_id aw99706_ids[] = {
+ { "aw99706" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, aw99706_ids);
+
+static const struct of_device_id aw99706_match_table[] = {
+ { .compatible = "awinic,aw99706", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, aw99706_match_table);
+
+static struct i2c_driver aw99706_i2c_driver = {
+ .probe = aw99706_probe,
+ .remove = aw99706_remove,
+ .id_table = aw99706_ids,
+ .driver = {
+ .name = "aw99706",
+ .of_match_table = aw99706_match_table,
+ .pm = &aw99706_pm_ops,
+ },
+};
+
+module_i2c_driver(aw99706_i2c_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("BackLight driver for aw99706");
--
2.51.1.dirty
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight
2025-10-26 12:39 ` [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight Junjie Cao
@ 2025-10-26 13:47 ` Krzysztof Kozlowski
2025-10-27 6:58 ` Junjie Cao
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-10-26 13:47 UTC (permalink / raw)
To: Junjie Cao, Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Helge Deller
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev
On 26/10/2025 13:39, Junjie Cao wrote:
> +
> + reg:
> + maxItems: 1
> +
> + enable-gpios:
> + description: GPIO to use to enable/disable the backlight (HWEN pin).
> + maxItems: 1
> +
> + awinic,dim-mode:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: >
> + Select dimming mode of the device.
> + 0 = Bypass mode.
> + 1 = DC mode.
> + 2 = MIX mode.
> + 3 = MIX-26k.
> + enum: [0, 1, 2, 3]
> + default: 1
> +
> + awinic,sw-freq:
Please use proper units, see:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
and other examples
Same everywhere else.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Boost switching frequency in kHz.
> + enum: [300, 400, 500, 600, 660, 750, 850, 1000, 1200, 1330, 1500, 1700]
> + default: 750
> +
> + awinic,sw-ilmt:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Switching current limitation in mA.
> + enum: [1500, 2000, 2500, 3000]
> + default: 3000
> +
> + awinic,iled-max:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Maximum LED current setting in uA.
> + minimum: 5000
> + maximum: 50000
> + multipleOf: 500
> + default: 20000
> +
> + awinic,uvlo-thres:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: UVLO(Under Voltage Lock Out) in mV.
> + enum: [2200, 5000]
> + default: 2200
> +
> + awinic,fade-time:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Fade In/Out Time(per step) in us.
> + enum: [8, 16, 32, 64, 128, 256, 512, 1024]
Why would this be fixed setting? This really looks like runtime, drop.
> + default: 16
> +
> + awinic,slope-time:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Slope time in ms.
Slope of what?
> + enum: [8, 24, 48, 96, 200, 300, 400, 500]
> + default: 300
> +
> + awinic,ramp-ctl:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: >
> + Select ramp control and filter of the device.
> + 0 = Fade in/fade out.
> + 1 = Light filter.
> + 2 = Medium filter.
> + 3 = Heavy filter.
> + enum: [0, 1, 2, 3]
> + default: 2
> +
> + awinic,brt-mode:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: >
> + Select brightness control of the device.
> + 0 = PWM.
> + 1 = IIC.
> + 2 = IIC x PWM.
> + 3 = IIC x PWM(P-ramp).
> + enum: [0, 1, 2, 3]
> + default: 1
> +
> + awinic,onoff-time:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Turn on/off time(per step) in ns.
> + enum: [250, 500, 1000, 2000, 4000, 8000, 16000]
Not a DT property.
> + default: 2000
> +
> +required:
> + - compatible
> + - reg
> + - enable-gpios
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + aw99706@76 {
> + compatible = "awinic,aw99706";
> + reg = <0x76>;
> + enable-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
Where are other properties from common.yaml? Looks like you re-invented
some parts.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight
2025-10-26 13:47 ` Krzysztof Kozlowski
@ 2025-10-27 6:58 ` Junjie Cao
2025-10-27 8:38 ` Krzysztof Kozlowski
0 siblings, 1 reply; 11+ messages in thread
From: Junjie Cao @ 2025-10-27 6:58 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev
On Sun, Oct 26, 2025 at 9:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 26/10/2025 13:39, Junjie Cao wrote:
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + enable-gpios:
> > + description: GPIO to use to enable/disable the backlight (HWEN pin).
> > + maxItems: 1
> > +
> > + awinic,dim-mode:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: >
> > + Select dimming mode of the device.
> > + 0 = Bypass mode.
> > + 1 = DC mode.
> > + 2 = MIX mode.
> > + 3 = MIX-26k.
> > + enum: [0, 1, 2, 3]
> > + default: 1
> > +
> > + awinic,sw-freq:
>
> Please use proper units, see:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> and other examples
>
> Same everywhere else.
>
ACK
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: Boost switching frequency in kHz.
> > + enum: [300, 400, 500, 600, 660, 750, 850, 1000, 1200, 1330, 1500, 1700]
> > + default: 750
> > +
> > + awinic,sw-ilmt:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: Switching current limitation in mA.
> > + enum: [1500, 2000, 2500, 3000]
> > + default: 3000
> > +
> > + awinic,iled-max:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: Maximum LED current setting in uA.
> > + minimum: 5000
> > + maximum: 50000
> > + multipleOf: 500
> > + default: 20000
> > +
> > + awinic,uvlo-thres:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: UVLO(Under Voltage Lock Out) in mV.
> > + enum: [2200, 5000]
> > + default: 2200
> > +
> > + awinic,fade-time:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: Fade In/Out Time(per step) in us.
> > + enum: [8, 16, 32, 64, 128, 256, 512, 1024]
>
> Why would this be fixed setting? This really looks like runtime, drop.
>
Yes, it is fixed. I am quoting this from the datasheet.
AW99706B provides Fade in/out mode to transform backlight from one brightness
to another or turn on/off backlight with a fixed slope. Writing 0b00 into
RAMP_CTR (CFG 0x06) to enter Fade in/out mode, and the the slope of current
transition can be set in FADE_TIME (CFG 0x06).
> > + default: 16
> > +
> > + awinic,slope-time:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: Slope time in ms.
>
> Slope of what?
>
Ramp time in slope mode, it is retained from downstream drivers, it will
be more clear in the next version.
> > + enum: [8, 24, 48, 96, 200, 300, 400, 500]
> > + default: 300
> > +
> > + awinic,ramp-ctl:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: >
> > + Select ramp control and filter of the device.
> > + 0 = Fade in/fade out.
> > + 1 = Light filter.
> > + 2 = Medium filter.
> > + 3 = Heavy filter.
> > + enum: [0, 1, 2, 3]
> > + default: 2
> > +
> > + awinic,brt-mode:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: >
> > + Select brightness control of the device.
> > + 0 = PWM.
> > + 1 = IIC.
> > + 2 = IIC x PWM.
> > + 3 = IIC x PWM(P-ramp).
> > + enum: [0, 1, 2, 3]
> > + default: 1
> > +
> > + awinic,onoff-time:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: Turn on/off time(per step) in ns.
> > + enum: [250, 500, 1000, 2000, 4000, 8000, 16000]
>
> Not a DT property.
>
It is mandatory in the downstream driver, I keep it.
The following is the description about it,
If the value in ONOFF_CTR(CFG 0x08 [4:3]) is 0b00, the turning on/off ramp of
AW99706B is soft start and fast end. In this mode, the ramp time can be
programmed by ONOFF_TIME (CFG 0x08 [2:0]).
> > + default: 2000
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - enable-gpios
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > +
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + aw99706@76 {
> > + compatible = "awinic,aw99706";
> > + reg = <0x76>;
> > + enable-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
>
> Where are other properties from common.yaml? Looks like you re-invented
> some parts.
>
Sorry, I forgot it, when writing the bindings, I used ktz8866.yaml as a
template. I should have dropped the common.yaml. This driver does
not require other properties in common.yaml.
Regards,
Junjie
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight
2025-10-27 6:58 ` Junjie Cao
@ 2025-10-27 8:38 ` Krzysztof Kozlowski
2025-10-27 10:29 ` Junjie Cao
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-10-27 8:38 UTC (permalink / raw)
To: Junjie Cao
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev
On 27/10/2025 07:58, Junjie Cao wrote:
> On Sun, Oct 26, 2025 at 9:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 26/10/2025 13:39, Junjie Cao wrote:
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + enable-gpios:
>>> + description: GPIO to use to enable/disable the backlight (HWEN pin).
>>> + maxItems: 1
>>> +
>>> + awinic,dim-mode:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: >
>>> + Select dimming mode of the device.
>>> + 0 = Bypass mode.
>>> + 1 = DC mode.
>>> + 2 = MIX mode.
>>> + 3 = MIX-26k.
>>> + enum: [0, 1, 2, 3]
>>> + default: 1
>>> +
>>> + awinic,sw-freq:
>>
>> Please use proper units, see:
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
>> and other examples
>>
>> Same everywhere else.
>>
>
> ACK
>
>>
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: Boost switching frequency in kHz.
>>> + enum: [300, 400, 500, 600, 660, 750, 850, 1000, 1200, 1330, 1500, 1700]
>>> + default: 750
>>> +
>>> + awinic,sw-ilmt:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: Switching current limitation in mA.
>>> + enum: [1500, 2000, 2500, 3000]
>>> + default: 3000
>>> +
>>> + awinic,iled-max:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: Maximum LED current setting in uA.
>>> + minimum: 5000
>>> + maximum: 50000
>>> + multipleOf: 500
>>> + default: 20000
>>> +
>>> + awinic,uvlo-thres:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: UVLO(Under Voltage Lock Out) in mV.
>>> + enum: [2200, 5000]
>>> + default: 2200
>>> +
>>> + awinic,fade-time:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: Fade In/Out Time(per step) in us.
>>> + enum: [8, 16, 32, 64, 128, 256, 512, 1024]
>>
>> Why would this be fixed setting? This really looks like runtime, drop.
>>
>
> Yes, it is fixed. I am quoting this from the datasheet.
Fixed per board.
> AW99706B provides Fade in/out mode to transform backlight from one brightness
> to another or turn on/off backlight with a fixed slope. Writing 0b00 into
> RAMP_CTR (CFG 0x06) to enter Fade in/out mode, and the the slope of current
> transition can be set in FADE_TIME (CFG 0x06).
>
>>> + default: 16
>>> +
>>> + awinic,slope-time:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: Slope time in ms.
>>
>> Slope of what?
>>
>
> Ramp time in slope mode, it is retained from downstream drivers, it will
> be more clear in the next version.
>
>>> + enum: [8, 24, 48, 96, 200, 300, 400, 500]
>>> + default: 300
>>> +
>>> + awinic,ramp-ctl:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: >
>>> + Select ramp control and filter of the device.
>>> + 0 = Fade in/fade out.
>>> + 1 = Light filter.
>>> + 2 = Medium filter.
>>> + 3 = Heavy filter.
>>> + enum: [0, 1, 2, 3]
>>> + default: 2
>>> +
>>> + awinic,brt-mode:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: >
>>> + Select brightness control of the device.
>>> + 0 = PWM.
>>> + 1 = IIC.
>>> + 2 = IIC x PWM.
>>> + 3 = IIC x PWM(P-ramp).
>>> + enum: [0, 1, 2, 3]
>>> + default: 1
>>> +
>>> + awinic,onoff-time:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: Turn on/off time(per step) in ns.
>>> + enum: [250, 500, 1000, 2000, 4000, 8000, 16000]
>>
>> Not a DT property.
>>
>
> It is mandatory in the downstream driver, I keep it.
Huh? I don't care about downstream driver. Again, not a DT property. You
cannot add here runtime properties and when, we tell you that, you just
ignore our review.
NAK
>
> The following is the description about it,
>
> If the value in ONOFF_CTR(CFG 0x08 [4:3]) is 0b00, the turning on/off ramp of
> AW99706B is soft start and fast end. In this mode, the ramp time can be
> programmed by ONOFF_TIME (CFG 0x08 [2:0]).
>
>>> + default: 2000
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - enable-gpios
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/gpio/gpio.h>
>>> +
>>> + i2c {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + aw99706@76 {
>>> + compatible = "awinic,aw99706";
>>> + reg = <0x76>;
>>> + enable-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
>>
>> Where are other properties from common.yaml? Looks like you re-invented
>> some parts.
>>
>
> Sorry, I forgot it, when writing the bindings, I used ktz8866.yaml as a
> template. I should have dropped the common.yaml. This driver does
> not require other properties in common.yaml.
I don't care about driver much, but anyway it should use common.yaml.
Please read the feedback very carefully.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight
2025-10-27 8:38 ` Krzysztof Kozlowski
@ 2025-10-27 10:29 ` Junjie Cao
0 siblings, 0 replies; 11+ messages in thread
From: Junjie Cao @ 2025-10-27 10:29 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev
On Mon, Oct 27, 2025 at 4:38 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 27/10/2025 07:58, Junjie Cao wrote:
> > On Sun, Oct 26, 2025 at 9:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 26/10/2025 13:39, Junjie Cao wrote:
> >>> +
> >>> + reg:
> >>> + maxItems: 1
> >>> +
> >>> + enable-gpios:
> >>> + description: GPIO to use to enable/disable the backlight (HWEN pin).
> >>> + maxItems: 1
> >>> +
> >>> + awinic,dim-mode:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: >
> >>> + Select dimming mode of the device.
> >>> + 0 = Bypass mode.
> >>> + 1 = DC mode.
> >>> + 2 = MIX mode.
> >>> + 3 = MIX-26k.
> >>> + enum: [0, 1, 2, 3]
> >>> + default: 1
> >>> +
> >>> + awinic,sw-freq:
> >>
> >> Please use proper units, see:
> >> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> >> and other examples
> >>
> >> Same everywhere else.
> >>
> >
> > ACK
> >
> >>
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: Boost switching frequency in kHz.
> >>> + enum: [300, 400, 500, 600, 660, 750, 850, 1000, 1200, 1330, 1500, 1700]
> >>> + default: 750
> >>> +
> >>> + awinic,sw-ilmt:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: Switching current limitation in mA.
> >>> + enum: [1500, 2000, 2500, 3000]
> >>> + default: 3000
> >>> +
> >>> + awinic,iled-max:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: Maximum LED current setting in uA.
> >>> + minimum: 5000
> >>> + maximum: 50000
> >>> + multipleOf: 500
> >>> + default: 20000
> >>> +
> >>> + awinic,uvlo-thres:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: UVLO(Under Voltage Lock Out) in mV.
> >>> + enum: [2200, 5000]
> >>> + default: 2200
> >>> +
> >>> + awinic,fade-time:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: Fade In/Out Time(per step) in us.
> >>> + enum: [8, 16, 32, 64, 128, 256, 512, 1024]
> >>
> >> Why would this be fixed setting? This really looks like runtime, drop.
> >>
> >
> > Yes, it is fixed. I am quoting this from the datasheet.
>
> Fixed per board.
>
>
> > AW99706B provides Fade in/out mode to transform backlight from one brightness
> > to another or turn on/off backlight with a fixed slope. Writing 0b00 into
> > RAMP_CTR (CFG 0x06) to enter Fade in/out mode, and the the slope of current
> > transition can be set in FADE_TIME (CFG 0x06).
> >
> >>> + default: 16
> >>> +
> >>> + awinic,slope-time:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: Slope time in ms.
> >>
> >> Slope of what?
> >>
> >
> > Ramp time in slope mode, it is retained from downstream drivers, it will
> > be more clear in the next version.
> >
> >>> + enum: [8, 24, 48, 96, 200, 300, 400, 500]
> >>> + default: 300
> >>> +
> >>> + awinic,ramp-ctl:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: >
> >>> + Select ramp control and filter of the device.
> >>> + 0 = Fade in/fade out.
> >>> + 1 = Light filter.
> >>> + 2 = Medium filter.
> >>> + 3 = Heavy filter.
> >>> + enum: [0, 1, 2, 3]
> >>> + default: 2
> >>> +
> >>> + awinic,brt-mode:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: >
> >>> + Select brightness control of the device.
> >>> + 0 = PWM.
> >>> + 1 = IIC.
> >>> + 2 = IIC x PWM.
> >>> + 3 = IIC x PWM(P-ramp).
> >>> + enum: [0, 1, 2, 3]
> >>> + default: 1
> >>> +
> >>> + awinic,onoff-time:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + description: Turn on/off time(per step) in ns.
> >>> + enum: [250, 500, 1000, 2000, 4000, 8000, 16000]
> >>
> >> Not a DT property.
> >>
> >
> > It is mandatory in the downstream driver, I keep it.
>
> Huh? I don't care about downstream driver. Again, not a DT property. You
> cannot add here runtime properties and when, we tell you that, you just
> ignore our review.
>
> NAK
>
My apologies for the misunderstanding and my poorly worded previous
comment. I absolutely did not intend to ignore your review.
I mentioned the "downstream driver" only to explain why I had originally
included the property.
I now understand your point clearly. I will remove them in the next
version.
Thanks for your fast reviews and for clarifying this principle for me.
>
> >
> > The following is the description about it,
> >
> > If the value in ONOFF_CTR(CFG 0x08 [4:3]) is 0b00, the turning on/off ramp of
> > AW99706B is soft start and fast end. In this mode, the ramp time can be
> > programmed by ONOFF_TIME (CFG 0x08 [2:0]).
> >
> >>> + default: 2000
> >>> +
> >>> +required:
> >>> + - compatible
> >>> + - reg
> >>> + - enable-gpios
> >>> +
> >>> +unevaluatedProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + #include <dt-bindings/gpio/gpio.h>
> >>> +
> >>> + i2c {
> >>> + #address-cells = <1>;
> >>> + #size-cells = <0>;
> >>> +
> >>> + aw99706@76 {
> >>> + compatible = "awinic,aw99706";
> >>> + reg = <0x76>;
> >>> + enable-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
> >>
> >> Where are other properties from common.yaml? Looks like you re-invented
> >> some parts.
> >>
> >
> > Sorry, I forgot it, when writing the bindings, I used ktz8866.yaml as a
> > template. I should have dropped the common.yaml. This driver does
> > not require other properties in common.yaml.
>
>
> I don't care about driver much, but anyway it should use common.yaml.
> Please read the feedback very carefully.
>
ACK
Regards,
Junjie
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
2025-10-26 12:39 ` [PATCH 2/2] backlight: aw99706: Add support for " Junjie Cao
@ 2025-10-27 12:06 ` kernel test robot
2025-10-27 18:59 ` kernel test robot
2025-10-28 13:22 ` Daniel Thompson
2 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-10-27 12:06 UTC (permalink / raw)
To: Junjie Cao, Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Helge Deller
Cc: oe-kbuild-all, dri-devel, linux-leds, devicetree, linux-kernel,
linux-fbdev, Junjie Cao, Pengyu Luo
Hi Junjie,
kernel test robot noticed the following build warnings:
[auto build test WARNING on lee-backlight/for-backlight-next]
[also build test WARNING on lee-leds/for-leds-next linus/master lee-backlight/for-backlight-fixes v6.18-rc3 next-20251027]
[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/Junjie-Cao/backlight-aw99706-Add-support-for-Awinic-AW99706-backlight/20251026-214135
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link: https://lore.kernel.org/r/20251026123923.1531727-3-caojunjie650%40gmail.com
patch subject: [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20251027/202510271932.kN86aCge-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251027/202510271932.kN86aCge-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510271932.kN86aCge-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/video/backlight/aw99706.c:468:12: warning: 'aw99706_resume' defined but not used [-Wunused-function]
468 | static int aw99706_resume(struct device *dev)
| ^~~~~~~~~~~~~~
>> drivers/video/backlight/aw99706.c:461:12: warning: 'aw99706_suspend' defined but not used [-Wunused-function]
461 | static int aw99706_suspend(struct device *dev)
| ^~~~~~~~~~~~~~~
vim +/aw99706_resume +468 drivers/video/backlight/aw99706.c
460
> 461 static int aw99706_suspend(struct device *dev)
462 {
463 struct aw99706_device *aw = dev_get_drvdata(dev);
464
465 return aw99706_update_brightness(aw, 0);
466 }
467
> 468 static int aw99706_resume(struct device *dev)
469 {
470 struct aw99706_device *aw = dev_get_drvdata(dev);
471
472 return aw99706_hw_init(aw);
473 }
474
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
2025-10-26 12:39 ` [PATCH 2/2] backlight: aw99706: Add support for " Junjie Cao
2025-10-27 12:06 ` kernel test robot
@ 2025-10-27 18:59 ` kernel test robot
2025-10-28 13:22 ` Daniel Thompson
2 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-10-27 18:59 UTC (permalink / raw)
To: Junjie Cao, Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Helge Deller
Cc: oe-kbuild-all, dri-devel, linux-leds, devicetree, linux-kernel,
linux-fbdev, Junjie Cao, Pengyu Luo
Hi Junjie,
kernel test robot noticed the following build warnings:
[auto build test WARNING on lee-backlight/for-backlight-next]
[also build test WARNING on lee-leds/for-leds-next linus/master v6.18-rc3 next-20251027]
[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/Junjie-Cao/backlight-aw99706-Add-support-for-Awinic-AW99706-backlight/20251026-214135
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link: https://lore.kernel.org/r/20251026123923.1531727-3-caojunjie650%40gmail.com
patch subject: [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20251028/202510280208.NhQyE0y1-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251028/202510280208.NhQyE0y1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510280208.NhQyE0y1-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/fortify-string.h:5,
from include/linux/string.h:382,
from arch/powerpc/include/asm/paca.h:16,
from arch/powerpc/include/asm/current.h:13,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/backlight.h:12,
from drivers/video/backlight/aw99706.c:12:
drivers/video/backlight/aw99706.c: In function 'aw99706_bl_enable':
>> include/linux/bitfield.h:172:27: warning: 'val' is used uninitialized [-Wuninitialized]
172 | *(_reg_p) &= ~(_mask); \
| ^~
drivers/video/backlight/aw99706.c:325:9: note: in expansion of macro 'FIELD_MODIFY'
325 | FIELD_MODIFY(AW99706_BACKLIGHT_EN_MASK, &val, en);
| ^~~~~~~~~~~~
drivers/video/backlight/aw99706.c:323:12: note: 'val' was declared here
323 | u8 val;
| ^~~
vim +/val +172 include/linux/bitfield.h
e2192de59e457a Johannes Berg 2023-01-18 120
e2192de59e457a Johannes Berg 2023-01-18 121 /**
e2192de59e457a Johannes Berg 2023-01-18 122 * FIELD_PREP_CONST() - prepare a constant bitfield element
e2192de59e457a Johannes Berg 2023-01-18 123 * @_mask: shifted mask defining the field's length and position
e2192de59e457a Johannes Berg 2023-01-18 124 * @_val: value to put in the field
e2192de59e457a Johannes Berg 2023-01-18 125 *
e2192de59e457a Johannes Berg 2023-01-18 126 * FIELD_PREP_CONST() masks and shifts up the value. The result should
e2192de59e457a Johannes Berg 2023-01-18 127 * be combined with other fields of the bitfield using logical OR.
e2192de59e457a Johannes Berg 2023-01-18 128 *
e2192de59e457a Johannes Berg 2023-01-18 129 * Unlike FIELD_PREP() this is a constant expression and can therefore
e2192de59e457a Johannes Berg 2023-01-18 130 * be used in initializers. Error checking is less comfortable for this
e2192de59e457a Johannes Berg 2023-01-18 131 * version, and non-constant masks cannot be used.
e2192de59e457a Johannes Berg 2023-01-18 132 */
e2192de59e457a Johannes Berg 2023-01-18 133 #define FIELD_PREP_CONST(_mask, _val) \
e2192de59e457a Johannes Berg 2023-01-18 134 ( \
e2192de59e457a Johannes Berg 2023-01-18 135 /* mask must be non-zero */ \
e2192de59e457a Johannes Berg 2023-01-18 136 BUILD_BUG_ON_ZERO((_mask) == 0) + \
e2192de59e457a Johannes Berg 2023-01-18 137 /* check if value fits */ \
e2192de59e457a Johannes Berg 2023-01-18 138 BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
e2192de59e457a Johannes Berg 2023-01-18 139 /* check if mask is contiguous */ \
e2192de59e457a Johannes Berg 2023-01-18 140 __BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) + \
e2192de59e457a Johannes Berg 2023-01-18 141 /* and create the value */ \
e2192de59e457a Johannes Berg 2023-01-18 142 (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)) \
e2192de59e457a Johannes Berg 2023-01-18 143 )
e2192de59e457a Johannes Berg 2023-01-18 144
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 145 /**
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 146 * FIELD_GET() - extract a bitfield element
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 147 * @_mask: shifted mask defining the field's length and position
7240767450d6d8 Masahiro Yamada 2017-10-03 148 * @_reg: value of entire bitfield
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 149 *
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 150 * FIELD_GET() extracts the field specified by @_mask from the
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 151 * bitfield passed in as @_reg by masking and shifting it down.
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 152 */
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 153 #define FIELD_GET(_mask, _reg) \
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 154 ({ \
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 155 __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 156 (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 157 })
3e9b3112ec74f1 Jakub Kicinski 2016-08-31 158
a256ae22570ee4 Luo Jie 2025-04-17 159 /**
a256ae22570ee4 Luo Jie 2025-04-17 160 * FIELD_MODIFY() - modify a bitfield element
a256ae22570ee4 Luo Jie 2025-04-17 161 * @_mask: shifted mask defining the field's length and position
a256ae22570ee4 Luo Jie 2025-04-17 162 * @_reg_p: pointer to the memory that should be updated
a256ae22570ee4 Luo Jie 2025-04-17 163 * @_val: value to store in the bitfield
a256ae22570ee4 Luo Jie 2025-04-17 164 *
a256ae22570ee4 Luo Jie 2025-04-17 165 * FIELD_MODIFY() modifies the set of bits in @_reg_p specified by @_mask,
a256ae22570ee4 Luo Jie 2025-04-17 166 * by replacing them with the bitfield value passed in as @_val.
a256ae22570ee4 Luo Jie 2025-04-17 167 */
a256ae22570ee4 Luo Jie 2025-04-17 168 #define FIELD_MODIFY(_mask, _reg_p, _val) \
a256ae22570ee4 Luo Jie 2025-04-17 169 ({ \
a256ae22570ee4 Luo Jie 2025-04-17 170 typecheck_pointer(_reg_p); \
a256ae22570ee4 Luo Jie 2025-04-17 171 __BF_FIELD_CHECK(_mask, *(_reg_p), _val, "FIELD_MODIFY: "); \
a256ae22570ee4 Luo Jie 2025-04-17 @172 *(_reg_p) &= ~(_mask); \
a256ae22570ee4 Luo Jie 2025-04-17 173 *(_reg_p) |= (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)); \
a256ae22570ee4 Luo Jie 2025-04-17 174 })
a256ae22570ee4 Luo Jie 2025-04-17 175
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
2025-10-26 12:39 ` [PATCH 2/2] backlight: aw99706: Add support for " Junjie Cao
2025-10-27 12:06 ` kernel test robot
2025-10-27 18:59 ` kernel test robot
@ 2025-10-28 13:22 ` Daniel Thompson
2025-10-29 11:49 ` Junjie Cao
2 siblings, 1 reply; 11+ messages in thread
From: Daniel Thompson @ 2025-10-28 13:22 UTC (permalink / raw)
To: Junjie Cao
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev, Pengyu Luo
On Sun, Oct 26, 2025 at 08:39:23PM +0800, Junjie Cao wrote:
> Add support for Awinic AW99706 backlight, which can be found in
> tablet and notebook backlight, one case is the Lenovo Legion Y700
> Gen4. This driver refers to the official datasheets and android
> driver, they can be found in [1].
>
> [1] https://www.awinic.com/en/productDetail/AW99706QNR
>
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> Signed-off-by: Junjie Cao <caojunjie650@gmail.com>
> ---
> diff --git a/drivers/video/backlight/aw99706.c b/drivers/video/backlight/aw99706.c
> new file mode 100644
> index 000000000..8dafdea45
> --- /dev/null
> +++ b/drivers/video/backlight/aw99706.c
> @@ -0,0 +1,503 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * aw99706 - Backlight driver for the AWINIC AW99706
> + *
> + * Copyright (C) 2025 Junjie Cao <caojunjie650@gmail.com>
> + * Copyright (C) 2025 Pengyu Luo <mitltlatltl@gmail.com>
> + *
> + * Based on vendor driver:
> + * Copyright (c) 2023 AWINIC Technology CO., LTD
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define AW99706_MAX_BRT_LVL 4095
> +#define AW99706_REG_MAX 0x1F
> +#define AW99706_ID 0x07
> +
> +/* registers list */
> +#define AW99706_CFG0_REG 0x00
> +#define AW99706_DIM_MODE_MASK GENMASK(1, 0)
> +
> +#define AW99706_CFG1_REG 0x01
> +#define AW99706_SW_FREQ_MASK GENMASK(3, 0)
> +#define AW99706_SW_ILMT_MASK GENMASK(5, 4)
> +
> +#define AW99706_CFG2_REG 0x02
> +#define AW99706_ILED_MAX_MASK GENMASK(6, 0)
> +#define AW99706_UVLOSEL_MASK BIT(7)
> +
> +#define AW99706_CFG3_REG 0x03
> +#define AW99706_CFG4_REG 0x04
> +#define AW99706_BRT_MSB_MASK GENMASK(3, 0)
> +
> +#define AW99706_CFG5_REG 0x05
> +#define AW99706_BRT_LSB_MASK GENMASK(7, 0)
> +
> +#define AW99706_CFG6_REG 0x06
> +#define AW99706_FADE_TIME_MASK GENMASK(2, 0)
> +#define AW99706_SLOPE_TIME_MASK GENMASK(5, 3)
> +#define AW99706_RAMP_CTL_MASK GENMASK(7, 6)
> +
> +#define AW99706_CFG7_REG 0x07
> +#define AW99706_BRT_MODE_MASK GENMASK(1, 0)
> +
> +#define AW99706_CFG8_REG 0x08
> +#define AW99706_ONOFF_TIME_MASK GENMASK(2, 0)
> +
> +#define AW99706_CFG9_REG 0x09
> +#define AW99706_CFGA_REG 0x0A
> +#define AW99706_CFGB_REG 0x0B
> +#define AW99706_CFGC_REG 0x0C
> +#define AW99706_CFGD_REG 0x0D
> +#define AW99706_FLAG_REG 0x10
> +#define AW99706_BACKLIGHT_EN_MASK BIT(7)
> +
> +#define AW99706_CHIPID_REG 0x11
> +#define AW99706_LED_OPEN_FLAG_REG 0x12
> +#define AW99706_LED_SHORT_FLAG_REG 0x13
> +#define AW99706_MTPLDOSEL_REG 0x1E
> +#define AW99706_MTPRUN_REG 0x1F
> +
> +#define RESV 0
> +
> +/* Boost switching frequency table, in kHz */
> +static const u32 aw99706_sw_freq_tbl[] = {
> + RESV, RESV, RESV, RESV, 300, 400, 500, 600,
> + 660, 750, 850, 1000, 1200, 1330, 1500, 1700
> +};
> +
> +/* Switching current limitation table, in mA */
> +static const u32 aw99706_sw_ilmt_tbl[] = {
> + 1500, 2000, 2500, 3000
> +};
> +
> +/* ULVO threshold table, in mV */
> +static const u32 aw99706_ulvo_thres_tbl[] = {
> + 2200, 5000
> +};
> +
> +/* Fade In/Out time table, in us */
> +static const u32 aw99706_fade_time_tbl[] = {
> + 8, 16, 32, 64, 128, 256, 512, 1024
> +};
> +
> +/* Slope time table, in ms */
> +static const u32 aw99706_slopetime_tbl[] = {
> + 8, 24, 48, 96, 200, 300, 400, 500
> +};
> +
> +/* Turn on/off time table, in ns */
> +static const u32 aw99706_onoff_time_tbl[] = {
> + RESV, 250, 500, 1000, 2000, 4000, 8000, 16000
> +};
> +
> +struct aw99706_device {
> + struct i2c_client *client;
> + struct device *dev;
> + struct regmap *regmap;
> + struct backlight_device *bl_dev;
> + struct gpio_desc *hwen_gpio;
> + bool bl_enable;
> +};
> +
> +enum reg_access {
> + REG_NONE_ACCESS = 0,
> + REG_RD_ACCESS = 1,
> + REG_WR_ACCESS = 2,
> +};
> +
> +struct aw99706_reg {
> + u8 defval;
> + u8 access;
> +};
> +
> +const struct aw99706_reg aw99706_regs[AW99706_REG_MAX + 1] = {
> + [AW99706_CFG0_REG] = {0x65, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG1_REG] = {0x39, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG2_REG] = {0x1e, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG3_REG] = {0x04, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG4_REG] = {0x00, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG5_REG] = {0x00, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG6_REG] = {0xa9, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG7_REG] = {0x04, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG8_REG] = {0x0c, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFG9_REG] = {0x4b, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFGA_REG] = {0x72, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFGB_REG] = {0x01, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFGC_REG] = {0x6c, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_CFGD_REG] = {0xfe, REG_RD_ACCESS | REG_WR_ACCESS},
> + [AW99706_FLAG_REG] = {0x00, REG_RD_ACCESS},
> + [AW99706_CHIPID_REG] = {AW99706_ID, REG_RD_ACCESS},
> + [AW99706_LED_OPEN_FLAG_REG] = {0x00, REG_RD_ACCESS},
> + [AW99706_LED_SHORT_FLAG_REG] = {0x00, REG_RD_ACCESS},
> +
> + /*
> + * Write bit is dropped here, writing BIT(0) to MTPLDOSEL will unlock
> + * Multi-time Programmable (MTP).
> + */
> + [AW99706_MTPLDOSEL_REG] = {0x00, REG_RD_ACCESS},
> + [AW99706_MTPRUN_REG] = {0x00, REG_NONE_ACCESS},
> +};
> +
> +static bool aw99706_readable_reg(struct device *dev, unsigned int reg)
> +{
> + return aw99706_regs[reg].access & REG_RD_ACCESS;
> +}
> +
> +static bool aw99706_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + return aw99706_regs[reg].access & REG_WR_ACCESS;
> +}
> +
> +static inline int aw99706_i2c_read(struct aw99706_device *aw, u8 reg,
> + unsigned int *val)
> +{
> + return regmap_read(aw->regmap, reg, val);
> +}
> +
> +static inline int aw99706_i2c_write(struct aw99706_device *aw, u8 reg, u8 val)
> +{
> + return regmap_write(aw->regmap, reg, val);
> +}
> +
> +static inline int aw99706_i2c_update_bits(struct aw99706_device *aw, u8 reg,
> + u8 mask, u8 val)
> +{
> + return regmap_update_bits(aw->regmap, reg, mask, val);
> +}
> +
> +struct aw99706_dt_prop {
> + const char * const name;
> + const u32 * const lookup_tbl;
> + u8 tbl_size;
> + u8 reg;
> + u8 mask;
> + u8 val;
> + u32 raw_val;
> +};
> +
> +static struct aw99706_dt_prop aw99706_dt_props[] = {
> + {
> + "awinic,dim-mode", NULL,
> + 0,
> + AW99706_CFG0_REG, AW99706_DIM_MODE_MASK
> + },
> + {
> + "awinic,sw-freq", aw99706_sw_freq_tbl,
> + ARRAY_SIZE(aw99706_sw_freq_tbl),
> + AW99706_CFG1_REG, AW99706_SW_FREQ_MASK
> + },
> + {
> + "awinic,sw-ilmt", aw99706_sw_ilmt_tbl,
> + ARRAY_SIZE(aw99706_sw_ilmt_tbl),
> + AW99706_CFG1_REG, AW99706_SW_ILMT_MASK
> + },
> + {
> + "awinic,iled-max", NULL,
> + 0,
> + AW99706_CFG2_REG, AW99706_ILED_MAX_MASK
> +
> + },
> + {
> + "awinic,uvlo-thres", aw99706_ulvo_thres_tbl,
> + ARRAY_SIZE(aw99706_ulvo_thres_tbl),
> + AW99706_CFG2_REG, AW99706_UVLOSEL_MASK
> + },
> + {
> + "awinic,fade-time", aw99706_fade_time_tbl,
> + ARRAY_SIZE(aw99706_fade_time_tbl),
> + AW99706_CFG6_REG, AW99706_FADE_TIME_MASK
> + },
> + {
> + "awinic,slope-time", aw99706_slopetime_tbl,
> + ARRAY_SIZE(aw99706_slopetime_tbl),
> + AW99706_CFG6_REG, AW99706_SLOPE_TIME_MASK
> + },
> + {
> + "awinic,ramp-ctl", NULL,
> + 0,
> + AW99706_CFG6_REG, AW99706_RAMP_CTL_MASK
> + },
> + {
> + "awinic,brt-mode", NULL,
> + 0,
> + AW99706_CFG7_REG, AW99706_BRT_MODE_MASK
> + },
> + {
> + "awinic,onoff-time", aw99706_onoff_time_tbl,
> + ARRAY_SIZE(aw99706_onoff_time_tbl),
> + AW99706_CFG8_REG, AW99706_ONOFF_TIME_MASK
> + },
> +};
> +
> +static int aw99706_lookup(const u32 * const tbl, int size, u32 val)
> +{
> + int i;
> +
> + for (i = 0; i < size; i++)
> + if (tbl[i] == val)
> + return i;
> +
> + return -1;
> +}
> +
> +static inline void aw99706_prop_set_default(struct aw99706_dt_prop *prop)
> +{
> + prop->val = prop->mask & aw99706_regs[prop->reg].defval;
Why included the default value in the register descriptions?
defval is only used to provide values for missing DT properties so using
the raw register values is cryptic and hard to read.
Including a default value in the aw99706_dt_props table instead would be
much more readable (because the defaults could use the same units at the
device tree).
> +}
> +
> +static void aw99706_dt_property_convert(struct aw99706_dt_prop *prop)
> +{
> + unsigned int val, shift;
> +
> + if (prop->lookup_tbl) {
> + val = aw99706_lookup(prop->lookup_tbl, prop->tbl_size,
> + prop->raw_val);
> + if (val < 0) {
> + aw99706_prop_set_default(prop);
This should not happen silently.
If the DT has provided an invalid value then we be issuing *at minimum*
a message at warning level or above. Many drivers will simply refuse to
probe when the DT is broken.
> + return;
> + }
> +
> + } else {
> + val = prop->raw_val;
> + }
> +
> + shift = ffs(prop->mask) - 1;
> + val <<= shift;
> + prop->val = prop->mask & val;
> +}
> +
> +static void aw99706_dt_parse(struct aw99706_device *aw)
> +{
> + struct aw99706_dt_prop *prop;
> + int ret, i;
> +
> + for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> + prop = &aw99706_dt_props[i];
> + ret = device_property_read_u32(aw->dev, prop->name,
> + &prop->raw_val);
> + if (ret < 0) {
> + dev_warn(aw->dev, "Missing property %s: %d\n",
> + prop->name, ret);
Why is there a warning when an optional property is not present. A DT
not including an optional property needs no message at all.
> +
> + aw99706_prop_set_default(prop);
> + } else {
> + aw99706_dt_property_convert(prop);
> + }
> + }
> +
> + /* This property requires a long linear array, using formula for now */
> + aw99706_dt_props[3].val = (aw99706_dt_props[3].raw_val - 5000) / 500;
Using a formula is fine, but I don't like doing it retrospectively.
Hard coding the 3 makes maintenance difficult and we end up making the
whole of aw99706_dt_props writeable just so we can store raw_val once!
Much better, IMHO, to embed a function pointer into the table and make
the whole table const. The function pointer can be
aw99706_dt_property_convert() in most cases (although rename it
`aw99706_dt_property_lookup_from_table() ) and can implement any
formula you need.
> +}
> +
> +static int aw99706_hw_init(struct aw99706_device *aw)
> +{
> + int ret, i;
> +
> + gpiod_set_value_cansleep(aw->hwen_gpio, 1);
> +
> + for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> + ret = aw99706_i2c_update_bits(aw, aw99706_dt_props[i].reg,
> + aw99706_dt_props[i].mask,
> + aw99706_dt_props[i].val);
> + if (ret < 0) {
> + dev_err(aw->dev, "Failed to write init data %d\n", ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int aw99706_bl_enable(struct aw99706_device *aw, bool en)
> +{
> + int ret;
> + u8 val;
> +
> + FIELD_MODIFY(AW99706_BACKLIGHT_EN_MASK, &val, en);
> + ret = aw99706_i2c_update_bits(aw, AW99706_CFGD_REG,
> + AW99706_BACKLIGHT_EN_MASK, val);
> + if (ret)
> + dev_err(aw->dev, "Failed to enable backlight!\n");
> +
> + return ret;
> +}
> +
> +static int aw99706_backlight_switch(struct aw99706_device *aw, u32 brt_lvl)
> +{
> + bool bl_enable_now = !!brt_lvl;
> + int ret = 0;
> +
> + if (aw->bl_enable != bl_enable_now) {
> + aw->bl_enable = bl_enable_now;
> + ret = aw99706_bl_enable(aw, bl_enable_now);
> + }
> +
> + return ret;
> +}
> +
> +static int aw99706_update_brightness(struct aw99706_device *aw, u32 brt_lvl)
> +{
> + int ret;
> +
> + ret = aw99706_i2c_write(aw, AW99706_CFG4_REG,
> + (brt_lvl >> 8) & AW99706_BRT_MSB_MASK);
> + if (ret < 0)
> + return ret;
> +
> + ret = aw99706_i2c_write(aw, AW99706_CFG5_REG,
> + brt_lvl & AW99706_BRT_LSB_MASK);
> + if (ret < 0)
> + return ret;
> +
> + return aw99706_backlight_switch(aw, brt_lvl);
I'm not sure there is much benefit pushing this out into a seperate
function. Merge this inline.
> +}
Everything below this function looked OK at first glance.
Daniel.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
2025-10-28 13:22 ` Daniel Thompson
@ 2025-10-29 11:49 ` Junjie Cao
2025-10-29 17:53 ` Daniel Thompson
0 siblings, 1 reply; 11+ messages in thread
From: Junjie Cao @ 2025-10-29 11:49 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev, Pengyu Luo
On Tue, Oct 28, 2025 at 9:21 PM Daniel Thompson <danielt@kernel.org> wrote:
>
> On Sun, Oct 26, 2025 at 08:39:23PM +0800, Junjie Cao wrote:
> > Add support for Awinic AW99706 backlight, which can be found in
> > tablet and notebook backlight, one case is the Lenovo Legion Y700
> > Gen4. This driver refers to the official datasheets and android
> > driver, they can be found in [1].
> >
> > [1] https://www.awinic.com/en/productDetail/AW99706QNR
> >
> > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > Signed-off-by: Junjie Cao <caojunjie650@gmail.com>
> > ---
> > diff --git a/drivers/video/backlight/aw99706.c b/drivers/video/backlight/aw99706.c
> > new file mode 100644
> > index 000000000..8dafdea45
> > --- /dev/null
> > +++ b/drivers/video/backlight/aw99706.c
> > @@ -0,0 +1,503 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * aw99706 - Backlight driver for the AWINIC AW99706
> > + *
> > + * Copyright (C) 2025 Junjie Cao <caojunjie650@gmail.com>
> > + * Copyright (C) 2025 Pengyu Luo <mitltlatltl@gmail.com>
> > + *
> > + * Based on vendor driver:
> > + * Copyright (c) 2023 AWINIC Technology CO., LTD
> > + */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#define AW99706_MAX_BRT_LVL 4095
> > +#define AW99706_REG_MAX 0x1F
> > +#define AW99706_ID 0x07
> > +
> > +/* registers list */
> > +#define AW99706_CFG0_REG 0x00
> > +#define AW99706_DIM_MODE_MASK GENMASK(1, 0)
> > +
> > +#define AW99706_CFG1_REG 0x01
> > +#define AW99706_SW_FREQ_MASK GENMASK(3, 0)
> > +#define AW99706_SW_ILMT_MASK GENMASK(5, 4)
> > +
> > +#define AW99706_CFG2_REG 0x02
> > +#define AW99706_ILED_MAX_MASK GENMASK(6, 0)
> > +#define AW99706_UVLOSEL_MASK BIT(7)
> > +
> > +#define AW99706_CFG3_REG 0x03
> > +#define AW99706_CFG4_REG 0x04
> > +#define AW99706_BRT_MSB_MASK GENMASK(3, 0)
> > +
> > +#define AW99706_CFG5_REG 0x05
> > +#define AW99706_BRT_LSB_MASK GENMASK(7, 0)
> > +
> > +#define AW99706_CFG6_REG 0x06
> > +#define AW99706_FADE_TIME_MASK GENMASK(2, 0)
> > +#define AW99706_SLOPE_TIME_MASK GENMASK(5, 3)
> > +#define AW99706_RAMP_CTL_MASK GENMASK(7, 6)
> > +
> > +#define AW99706_CFG7_REG 0x07
> > +#define AW99706_BRT_MODE_MASK GENMASK(1, 0)
> > +
> > +#define AW99706_CFG8_REG 0x08
> > +#define AW99706_ONOFF_TIME_MASK GENMASK(2, 0)
> > +
> > +#define AW99706_CFG9_REG 0x09
> > +#define AW99706_CFGA_REG 0x0A
> > +#define AW99706_CFGB_REG 0x0B
> > +#define AW99706_CFGC_REG 0x0C
> > +#define AW99706_CFGD_REG 0x0D
> > +#define AW99706_FLAG_REG 0x10
> > +#define AW99706_BACKLIGHT_EN_MASK BIT(7)
> > +
> > +#define AW99706_CHIPID_REG 0x11
> > +#define AW99706_LED_OPEN_FLAG_REG 0x12
> > +#define AW99706_LED_SHORT_FLAG_REG 0x13
> > +#define AW99706_MTPLDOSEL_REG 0x1E
> > +#define AW99706_MTPRUN_REG 0x1F
> > +
> > +#define RESV 0
> > +
> > +/* Boost switching frequency table, in kHz */
> > +static const u32 aw99706_sw_freq_tbl[] = {
> > + RESV, RESV, RESV, RESV, 300, 400, 500, 600,
> > + 660, 750, 850, 1000, 1200, 1330, 1500, 1700
> > +};
> > +
> > +/* Switching current limitation table, in mA */
> > +static const u32 aw99706_sw_ilmt_tbl[] = {
> > + 1500, 2000, 2500, 3000
> > +};
> > +
> > +/* ULVO threshold table, in mV */
> > +static const u32 aw99706_ulvo_thres_tbl[] = {
> > + 2200, 5000
> > +};
> > +
> > +/* Fade In/Out time table, in us */
> > +static const u32 aw99706_fade_time_tbl[] = {
> > + 8, 16, 32, 64, 128, 256, 512, 1024
> > +};
> > +
> > +/* Slope time table, in ms */
> > +static const u32 aw99706_slopetime_tbl[] = {
> > + 8, 24, 48, 96, 200, 300, 400, 500
> > +};
> > +
> > +/* Turn on/off time table, in ns */
> > +static const u32 aw99706_onoff_time_tbl[] = {
> > + RESV, 250, 500, 1000, 2000, 4000, 8000, 16000
> > +};
> > +
> > +struct aw99706_device {
> > + struct i2c_client *client;
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct backlight_device *bl_dev;
> > + struct gpio_desc *hwen_gpio;
> > + bool bl_enable;
> > +};
> > +
> > +enum reg_access {
> > + REG_NONE_ACCESS = 0,
> > + REG_RD_ACCESS = 1,
> > + REG_WR_ACCESS = 2,
> > +};
> > +
> > +struct aw99706_reg {
> > + u8 defval;
> > + u8 access;
> > +};
> > +
> > +const struct aw99706_reg aw99706_regs[AW99706_REG_MAX + 1] = {
> > + [AW99706_CFG0_REG] = {0x65, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG1_REG] = {0x39, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG2_REG] = {0x1e, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG3_REG] = {0x04, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG4_REG] = {0x00, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG5_REG] = {0x00, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG6_REG] = {0xa9, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG7_REG] = {0x04, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG8_REG] = {0x0c, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG9_REG] = {0x4b, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFGA_REG] = {0x72, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFGB_REG] = {0x01, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFGC_REG] = {0x6c, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFGD_REG] = {0xfe, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_FLAG_REG] = {0x00, REG_RD_ACCESS},
> > + [AW99706_CHIPID_REG] = {AW99706_ID, REG_RD_ACCESS},
> > + [AW99706_LED_OPEN_FLAG_REG] = {0x00, REG_RD_ACCESS},
> > + [AW99706_LED_SHORT_FLAG_REG] = {0x00, REG_RD_ACCESS},
> > +
> > + /*
> > + * Write bit is dropped here, writing BIT(0) to MTPLDOSEL will unlock
> > + * Multi-time Programmable (MTP).
> > + */
> > + [AW99706_MTPLDOSEL_REG] = {0x00, REG_RD_ACCESS},
> > + [AW99706_MTPRUN_REG] = {0x00, REG_NONE_ACCESS},
> > +};
> > +
> > +static bool aw99706_readable_reg(struct device *dev, unsigned int reg)
> > +{
> > + return aw99706_regs[reg].access & REG_RD_ACCESS;
> > +}
> > +
> > +static bool aw99706_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > + return aw99706_regs[reg].access & REG_WR_ACCESS;
> > +}
> > +
> > +static inline int aw99706_i2c_read(struct aw99706_device *aw, u8 reg,
> > + unsigned int *val)
> > +{
> > + return regmap_read(aw->regmap, reg, val);
> > +}
> > +
> > +static inline int aw99706_i2c_write(struct aw99706_device *aw, u8 reg, u8 val)
> > +{
> > + return regmap_write(aw->regmap, reg, val);
> > +}
> > +
> > +static inline int aw99706_i2c_update_bits(struct aw99706_device *aw, u8 reg,
> > + u8 mask, u8 val)
> > +{
> > + return regmap_update_bits(aw->regmap, reg, mask, val);
> > +}
> > +
> > +struct aw99706_dt_prop {
> > + const char * const name;
> > + const u32 * const lookup_tbl;
> > + u8 tbl_size;
> > + u8 reg;
> > + u8 mask;
> > + u8 val;
> > + u32 raw_val;
> > +};
> > +
> > +static struct aw99706_dt_prop aw99706_dt_props[] = {
> > + {
> > + "awinic,dim-mode", NULL,
> > + 0,
> > + AW99706_CFG0_REG, AW99706_DIM_MODE_MASK
> > + },
> > + {
> > + "awinic,sw-freq", aw99706_sw_freq_tbl,
> > + ARRAY_SIZE(aw99706_sw_freq_tbl),
> > + AW99706_CFG1_REG, AW99706_SW_FREQ_MASK
> > + },
> > + {
> > + "awinic,sw-ilmt", aw99706_sw_ilmt_tbl,
> > + ARRAY_SIZE(aw99706_sw_ilmt_tbl),
> > + AW99706_CFG1_REG, AW99706_SW_ILMT_MASK
> > + },
> > + {
> > + "awinic,iled-max", NULL,
> > + 0,
> > + AW99706_CFG2_REG, AW99706_ILED_MAX_MASK
> > +
> > + },
> > + {
> > + "awinic,uvlo-thres", aw99706_ulvo_thres_tbl,
> > + ARRAY_SIZE(aw99706_ulvo_thres_tbl),
> > + AW99706_CFG2_REG, AW99706_UVLOSEL_MASK
> > + },
> > + {
> > + "awinic,fade-time", aw99706_fade_time_tbl,
> > + ARRAY_SIZE(aw99706_fade_time_tbl),
> > + AW99706_CFG6_REG, AW99706_FADE_TIME_MASK
> > + },
> > + {
> > + "awinic,slope-time", aw99706_slopetime_tbl,
> > + ARRAY_SIZE(aw99706_slopetime_tbl),
> > + AW99706_CFG6_REG, AW99706_SLOPE_TIME_MASK
> > + },
> > + {
> > + "awinic,ramp-ctl", NULL,
> > + 0,
> > + AW99706_CFG6_REG, AW99706_RAMP_CTL_MASK
> > + },
> > + {
> > + "awinic,brt-mode", NULL,
> > + 0,
> > + AW99706_CFG7_REG, AW99706_BRT_MODE_MASK
> > + },
> > + {
> > + "awinic,onoff-time", aw99706_onoff_time_tbl,
> > + ARRAY_SIZE(aw99706_onoff_time_tbl),
> > + AW99706_CFG8_REG, AW99706_ONOFF_TIME_MASK
> > + },
> > +};
> > +
> > +static int aw99706_lookup(const u32 * const tbl, int size, u32 val)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < size; i++)
> > + if (tbl[i] == val)
> > + return i;
> > +
> > + return -1;
> > +}
> > +
> > +static inline void aw99706_prop_set_default(struct aw99706_dt_prop *prop)
> > +{
> > + prop->val = prop->mask & aw99706_regs[prop->reg].defval;
>
> Why included the default value in the register descriptions?
>
> defval is only used to provide values for missing DT properties so using
> the raw register values is cryptic and hard to read.
>
> Including a default value in the aw99706_dt_props table instead would be
> much more readable (because the defaults could use the same units at the
> device tree).
>
Agree, I will include the default values in the aw99706_dt_props table.
>
> > +}
> > +
> > +static void aw99706_dt_property_convert(struct aw99706_dt_prop *prop)
> > +{
> > + unsigned int val, shift;
> > +
> > + if (prop->lookup_tbl) {
> > + val = aw99706_lookup(prop->lookup_tbl, prop->tbl_size,
> > + prop->raw_val);
> > + if (val < 0) {
> > + aw99706_prop_set_default(prop);
>
> This should not happen silently.
>
> If the DT has provided an invalid value then we be issuing *at minimum*
> a message at warning level or above. Many drivers will simply refuse to
> probe when the DT is broken.
>
Indeed, I missed it.
>
> > + return;
> > + }
> > +
> > + } else {
> > + val = prop->raw_val;
> > + }
> > +
> > + shift = ffs(prop->mask) - 1;
> > + val <<= shift;
> > + prop->val = prop->mask & val;
> > +}
> > +
> > +static void aw99706_dt_parse(struct aw99706_device *aw)
> > +{
> > + struct aw99706_dt_prop *prop;
> > + int ret, i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> > + prop = &aw99706_dt_props[i];
> > + ret = device_property_read_u32(aw->dev, prop->name,
> > + &prop->raw_val);
> > + if (ret < 0) {
> > + dev_warn(aw->dev, "Missing property %s: %d\n",
> > + prop->name, ret);
>
> Why is there a warning when an optional property is not present. A DT
> not including an optional property needs no message at all.
>
They are mandatory in the downstream, and providing all properties is
difficult sometimes, so I set a default value if one is missing. But
one device may use a configuration different from the component
vendor's. These default values may be not optimal, so I issue a
warning for property missing. (I forgot to address it)
>
> > +
> > + aw99706_prop_set_default(prop);
> > + } else {
> > + aw99706_dt_property_convert(prop);
> > + }
> > + }
> > +
> > + /* This property requires a long linear array, using formula for now */
> > + aw99706_dt_props[3].val = (aw99706_dt_props[3].raw_val - 5000) / 500;
>
> Using a formula is fine, but I don't like doing it retrospectively.
> Hard coding the 3 makes maintenance difficult and we end up making the
> whole of aw99706_dt_props writeable just so we can store raw_val once!
>
> Much better, IMHO, to embed a function pointer into the table and make
> the whole table const. The function pointer can be
> aw99706_dt_property_convert() in most cases (although rename it
> `aw99706_dt_property_lookup_from_table() ) and can implement any
> formula you need.
>
Helpful opinion. I will do this in next version.
>
> > +}
> > +
> > +static int aw99706_hw_init(struct aw99706_device *aw)
> > +{
> > + int ret, i;
> > +
> > + gpiod_set_value_cansleep(aw->hwen_gpio, 1);
> > +
> > + for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> > + ret = aw99706_i2c_update_bits(aw, aw99706_dt_props[i].reg,
> > + aw99706_dt_props[i].mask,
> > + aw99706_dt_props[i].val);
> > + if (ret < 0) {
> > + dev_err(aw->dev, "Failed to write init data %d\n", ret);
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int aw99706_bl_enable(struct aw99706_device *aw, bool en)
> > +{
> > + int ret;
> > + u8 val;
> > +
> > + FIELD_MODIFY(AW99706_BACKLIGHT_EN_MASK, &val, en);
> > + ret = aw99706_i2c_update_bits(aw, AW99706_CFGD_REG,
> > + AW99706_BACKLIGHT_EN_MASK, val);
> > + if (ret)
> > + dev_err(aw->dev, "Failed to enable backlight!\n");
> > +
> > + return ret;
> > +}
> > +
> > +static int aw99706_backlight_switch(struct aw99706_device *aw, u32 brt_lvl)
> > +{
> > + bool bl_enable_now = !!brt_lvl;
> > + int ret = 0;
> > +
> > + if (aw->bl_enable != bl_enable_now) {
> > + aw->bl_enable = bl_enable_now;
> > + ret = aw99706_bl_enable(aw, bl_enable_now);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int aw99706_update_brightness(struct aw99706_device *aw, u32 brt_lvl)
> > +{
> > + int ret;
> > +
> > + ret = aw99706_i2c_write(aw, AW99706_CFG4_REG,
> > + (brt_lvl >> 8) & AW99706_BRT_MSB_MASK);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = aw99706_i2c_write(aw, AW99706_CFG5_REG,
> > + brt_lvl & AW99706_BRT_LSB_MASK);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return aw99706_backlight_switch(aw, brt_lvl);
>
> I'm not sure there is much benefit pushing this out into a seperate
> function. Merge this inline.
>
> > +}
>
I see.
Regards,
Junjie
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
2025-10-29 11:49 ` Junjie Cao
@ 2025-10-29 17:53 ` Daniel Thompson
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Thompson @ 2025-10-29 17:53 UTC (permalink / raw)
To: Junjie Cao
Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
linux-leds, devicetree, linux-kernel, linux-fbdev, Pengyu Luo
On Wed, Oct 29, 2025 at 07:49:35PM +0800, Junjie Cao wrote:
> On Tue, Oct 28, 2025 at 9:21 PM Daniel Thompson <danielt@kernel.org> wrote:
> >
> > On Sun, Oct 26, 2025 at 08:39:23PM +0800, Junjie Cao wrote:
> > > Add support for Awinic AW99706 backlight, which can be found in
> > > tablet and notebook backlight, one case is the Lenovo Legion Y700
> > > Gen4. This driver refers to the official datasheets and android
> > > driver, they can be found in [1].
> > >
> > > [1] https://www.awinic.com/en/productDetail/AW99706QNR
> > >
> > > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > > Signed-off-by: Junjie Cao <caojunjie650@gmail.com>
> > > ---
> > > diff --git a/drivers/video/backlight/aw99706.c b/drivers/video/backlight/aw99706.c
> > > <snip>
> > > +static void aw99706_dt_parse(struct aw99706_device *aw)
> > > +{
> > > + struct aw99706_dt_prop *prop;
> > > + int ret, i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> > > + prop = &aw99706_dt_props[i];
> > > + ret = device_property_read_u32(aw->dev, prop->name,
> > > + &prop->raw_val);
> > > + if (ret < 0) {
> > > + dev_warn(aw->dev, "Missing property %s: %d\n",
> > > + prop->name, ret);
> >
> > Why is there a warning when an optional property is not present. A DT
> > not including an optional property needs no message at all.
> >
>
> They are mandatory in the downstream, and providing all properties is
> difficult sometimes, so I set a default value if one is missing. But
> one device may use a configuration different from the component
> vendor's. These default values may be not optimal, so I issue a
> warning for property missing. (I forgot to address it)
All sensible but to be clear...
From my point-of-view the driver should match the upstream bindings.
Either the properties are required (in which case missing them can be
dev_err() and/or fail to probe) or they are optional (in which case
there should be no warnings).
Similarly if missing values is likely to lead to very sub-optimal
behavior (or something that has a risk of over-current or component
failure) then consider making the options mandatory.
Daniel.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-10-29 17:52 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20251026123923.1531727-1-caojunjie650@gmail.com>
2025-10-26 12:39 ` [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight Junjie Cao
2025-10-26 13:47 ` Krzysztof Kozlowski
2025-10-27 6:58 ` Junjie Cao
2025-10-27 8:38 ` Krzysztof Kozlowski
2025-10-27 10:29 ` Junjie Cao
2025-10-26 12:39 ` [PATCH 2/2] backlight: aw99706: Add support for " Junjie Cao
2025-10-27 12:06 ` kernel test robot
2025-10-27 18:59 ` kernel test robot
2025-10-28 13:22 ` Daniel Thompson
2025-10-29 11:49 ` Junjie Cao
2025-10-29 17:53 ` Daniel Thompson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).