[PATCH v2 0/2] backlight: aw99706: Add support for Awinic AW99706 backlight

[PATCH v2 0/2] backlight: aw99706: Add support for Awinic AW99706 backlight

[Junjie Cao]

From: Pengyu Luo <mitltlatltl@gmail.com>

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: Junjie Cao <caojunjie650@gmail.com>
---
base-commit: 72fb0170ef1f45addf726319c52a0562b6913707
---
Changes in v2:
- add handler for max-brightness and default-brightness
- add properties(max-brightness, default-brightness) (Krzysztof)
- use proper units for properties (Krzysztof)
- drop non-fixed properties (Krzysztof)
- include default values in the aw99706_dt_props table (Daniel)
- warn when a property value from DT is invalid (Daniel)
- drop warning when optional properties are missing (Daniel)
- add a function pointer into the aw99706_dt_props table to handle lookup (Daniel)
- use a lookup function instead of hardcoding the formula for the iLED max (Daniel)
- move BL enalbe handler into aw99706_update_brightness (Daniel)
- Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-3-caojunjie650@gmail.com

Junjie Cao (2):
  dt-bindings: leds: backlight: Add Awinic AW99706 backlight
  backlight: aw99706: Add support for Awinic AW99706 backlight

 .../leds/backlight/awinic,aw99706.yaml        | 100 ++++
 MAINTAINERS                                   |   6 +
 drivers/video/backlight/Kconfig               |   8 +
 drivers/video/backlight/Makefile              |   1 +
 drivers/video/backlight/aw99706.c             | 492 ++++++++++++++++++
 5 files changed, 607 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml
 create mode 100644 drivers/video/backlight/aw99706.c

-- 
2.51.1.dirty

[PATCH v2 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight

[Junjie Cao]

From: Pengyu Luo <mitltlatltl@gmail.com>

Add Awinic AW99706 backlight binding documentation.

Signed-off-by: Junjie Cao <caojunjie650@gmail.com>
---
Changes in v2:
- use proper units for properties (Krzysztof)
- drop non-fixed properties (Krzysztof)
- add properties(max-brightness, default-brightness) (Krzysztof)
- Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-2-caojunjie650@gmail.com

 .../leds/backlight/awinic,aw99706.yaml        | 100 ++++++++++++++++++
 1 file changed, 100 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..9b7266e61
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml
@@ -0,0 +1,100 @@
+# 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(PWM at low brightness and DC at high brightness).
+        3 = MIX-26k mode(MIX mode with different PWM frequency).
+    enum: [ 0, 1, 2, 3 ]
+    default: 1
+
+  awinic,sw-freq-hz:
+    description: Boost switching frequency in Hz.
+    enum: [ 300000, 400000, 500000, 600000, 660000, 750000, 850000, 1000000, 1200000, 1330000, 1500000, 1700000 ]
+    default: 750000
+
+  awinic,sw-ilmt-microamp:
+    description: Switching current limitation in uA.
+    enum: [ 1500000, 2000000, 2500000, 3000000 ]
+    default: 3000000
+
+  awinic,iled-max-microamp:
+    description: Maximum LED current setting in uA.
+    minimum: 5000
+    maximum: 50000
+    multipleOf: 500
+    default: 20000
+
+  awinic,uvlo-thres-microvolt:
+    description: UVLO(Under Voltage Lock Out) in uV.
+    enum: [ 2200000, 5000000 ]
+    default: 2200000
+
+  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
+
+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>;
+            default-brightness = <2047>;
+            max-brightness = <4095>;
+            awinic,dim-mode = <1>;
+            awinic,sw-freq-hz = <750000>;
+            awinic,sw-ilmt-microamp = <3000000>;
+            awinic,uvlo-thres-microvolt = <2200000>;
+            awinic,iled-max-microamp = <20000>;
+            awinic,ramp-ctl = <2>;
+        };
+    };
+
+...
-- 
2.51.1.dirty

Re: [PATCH v2 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight

[Rob Herring (Arm)]

On Mon, 03 Nov 2025 19:06:47 +0800, Junjie Cao wrote:
> From: Pengyu Luo <mitltlatltl@gmail.com>
> 
> Add Awinic AW99706 backlight binding documentation.
> 
> Signed-off-by: Junjie Cao <caojunjie650@gmail.com>
> ---
> Changes in v2:
> - use proper units for properties (Krzysztof)
> - drop non-fixed properties (Krzysztof)
> - add properties(max-brightness, default-brightness) (Krzysztof)
> - Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-2-caojunjie650@gmail.com
> 
>  .../leds/backlight/awinic,aw99706.yaml        | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/leds/backlight/awinic,aw99706.yaml:39:111: [warning] line too long (113 > 110 characters) (line-length)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20251103110648.878325-2-caojunjie650@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight

[Krzysztof Kozlowski]

On Mon, Nov 03, 2025 at 07:06:47PM +0800, Junjie Cao wrote:
> From: Pengyu Luo <mitltlatltl@gmail.com>
> 
> Add Awinic AW99706 backlight binding documentation.
> 
> Signed-off-by: Junjie Cao <caojunjie650@gmail.com>

Messed DCO chain. This wasn't here, so you must have altered v1 to add
some weird change.

This is a blocker, please read carefully submitting patches and DCO.

> ---
> Changes in v2:
> - use proper units for properties (Krzysztof)
> - drop non-fixed properties (Krzysztof)
> - add properties(max-brightness, default-brightness) (Krzysztof)
> - Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-2-caojunjie650@gmail.com

...

> +  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(PWM at low brightness and DC at high brightness).
> +        3 = MIX-26k mode(MIX mode with different PWM frequency).
> +    enum: [ 0, 1, 2, 3 ]
> +    default: 1
> +
> +  awinic,sw-freq-hz:
> +    description: Boost switching frequency in Hz.
> +    enum: [ 300000, 400000, 500000, 600000, 660000, 750000, 850000, 1000000, 1200000, 1330000, 1500000, 1700000 ]

Please wrap code according to the preferred limit expressed in Kernel
coding style (checkpatch is not a coding style description, but only a
tool).

> +    default: 750000
> +
> +  awinic,sw-ilmt-microamp:
> +    description: Switching current limitation in uA.
> +    enum: [ 1500000, 2000000, 2500000, 3000000 ]
> +    default: 3000000
> +
> +  awinic,iled-max-microamp:
> +    description: Maximum LED current setting in uA.
> +    minimum: 5000
> +    maximum: 50000
> +    multipleOf: 500
> +    default: 20000
> +
> +  awinic,uvlo-thres-microvolt:
> +    description: UVLO(Under Voltage Lock Out) in uV.
> +    enum: [ 2200000, 5000000 ]
> +    default: 2200000
> +
> +  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
> +
> +required:
> +  - compatible
> +  - reg
> +  - enable-gpios
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        aw99706@76 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
If you cannot find a name matching your device, please check in kernel
sources for similar cases or you can grow the spec (via pull request to
DT spec repo).

> +            compatible = "awinic,aw99706";
> +            reg = <0x76>;
> +            enable-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
> +            default-brightness = <2047>;
> +            max-brightness = <4095>;
> +            awinic,dim-mode = <1>;
> +            awinic,sw-freq-hz = <750000>;
> +            awinic,sw-ilmt-microamp = <3000000>;
> +            awinic,uvlo-thres-microvolt = <2200000>;
> +            awinic,iled-max-microamp = <20000>;
> +            awinic,ramp-ctl = <2>;
> +        };
> +    };
> +
> +...
> -- 
> 2.51.1.dirty
> 

Re: [PATCH v2 1/2] dt-bindings: leds: backlight: Add Awinic AW99706 backlight

[Junjie Cao]

On Tue, Nov 4, 2025 at 3:30 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Nov 03, 2025 at 07:06:47PM +0800, Junjie Cao wrote:
> > From: Pengyu Luo <mitltlatltl@gmail.com>
> >
> > Add Awinic AW99706 backlight binding documentation.
> >
> > Signed-off-by: Junjie Cao <caojunjie650@gmail.com>
>
> Messed DCO chain. This wasn't here, so you must have altered v1 to add
> some weird change.
>
> This is a blocker, please read carefully submitting patches and DCO.
>

Apologies for the DCO mess.

The tablet device is currently with Pengyu. He helped with testing and
tweaked the .c driver file (patch 2/2) after my change. The entire
series was then re-formatted on his machine, which caused his git
configuration to be incorrectly applied to the From: line of this
dt-binding patch (patch 1/2).

I am the actual author of this dt-binding file. I will correct the
authorship and DCO chain in v2.

> > ---
> > Changes in v2:
> > - use proper units for properties (Krzysztof)
> > - drop non-fixed properties (Krzysztof)
> > - add properties(max-brightness, default-brightness) (Krzysztof)
> > - Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-2-caojunjie650@gmail.com
>
> ...
>
> > +  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(PWM at low brightness and DC at high brightness).
> > +        3 = MIX-26k mode(MIX mode with different PWM frequency).
> > +    enum: [ 0, 1, 2, 3 ]
> > +    default: 1
> > +
> > +  awinic,sw-freq-hz:
> > +    description: Boost switching frequency in Hz.
> > +    enum: [ 300000, 400000, 500000, 600000, 660000, 750000, 850000, 1000000, 1200000, 1330000, 1500000, 1700000 ]
>
> Please wrap code according to the preferred limit expressed in Kernel
> coding style (checkpatch is not a coding style description, but only a
> tool).
>

ACK.

> > +    default: 750000
> > +
> > +  awinic,sw-ilmt-microamp:
> > +    description: Switching current limitation in uA.
> > +    enum: [ 1500000, 2000000, 2500000, 3000000 ]
> > +    default: 3000000
> > +
> > +  awinic,iled-max-microamp:
> > +    description: Maximum LED current setting in uA.
> > +    minimum: 5000
> > +    maximum: 50000
> > +    multipleOf: 500
> > +    default: 20000
> > +
> > +  awinic,uvlo-thres-microvolt:
> > +    description: UVLO(Under Voltage Lock Out) in uV.
> > +    enum: [ 2200000, 5000000 ]
> > +    default: 2200000
> > +
> > +  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
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - enable-gpios
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        aw99706@76 {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> If you cannot find a name matching your device, please check in kernel
> sources for similar cases or you can grow the spec (via pull request to
> DT spec repo).
>

I see. backlight@76, thanks for your detailed explanation.

Regards,
Junjie

[PATCH v2 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight

[Junjie Cao]

From: Pengyu Luo <mitltlatltl@gmail.com>

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>
---
Changes in v2:
- add handler for max-brightness and default-brightness
- use proper units for properties (Krzysztof)
- drop non-fixed properties (Krzysztof)
- include default values in the aw99706_dt_props table (Daniel)
- warn when a property value from DT is invalid (Daniel)
- drop warning when optional properties are missing (Daniel)
- add a function pointer into the aw99706_dt_props table to handle lookup (Daniel)
- use a lookup function instead of hardcoding the formula for the iLED max (Daniel)
- move BL enalbe handler into aw99706_update_brightness (Daniel)
- Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-3-caojunjie650@gmail.com

 MAINTAINERS                       |   6 +
 drivers/video/backlight/Kconfig   |   8 +
 drivers/video/backlight/Makefile  |   1 +
 drivers/video/backlight/aw99706.c | 492 ++++++++++++++++++++++++++++++
 4 files changed, 507 insertions(+)
 create mode 100644 drivers/video/backlight/aw99706.c

diff --git a/MAINTAINERS b/MAINTAINERS
index be21f1fa8..551d8328e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4140,6 +4140,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..f65e35905
--- /dev/null
+++ b/drivers/video/backlight/aw99706.c
@@ -0,0 +1,492 @@
+// 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_RAMP_CTL_MASK			GENMASK(7, 6)
+
+#define AW99706_CFG7_REG			0x07
+#define AW99706_CFG8_REG			0x08
+#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 Hz */
+static const u32 aw99706_sw_freq_tbl[] = {
+	RESV, RESV, RESV, RESV, 300000, 400000, 500000, 600000,
+	660000, 750000, 850000, 1000000, 1200000, 1330000, 1500000, 1700000
+};
+
+/* Switching current limitation table, in uA */
+static const u32 aw99706_sw_ilmt_tbl[] = {
+	1500000, 2000000, 2500000, 3000000
+};
+
+/* ULVO threshold table, in uV */
+static const u32 aw99706_ulvo_thres_tbl[] = {
+	2200000, 5000000
+};
+
+struct reg_init_data;
+
+struct aw99706_device {
+	struct i2c_client *client;
+	struct device *dev;
+	struct regmap *regmap;
+	struct backlight_device *bl_dev;
+	struct gpio_desc *hwen_gpio;
+	struct reg_init_data *init_tbl;
+	int init_tbl_size;
+	bool bl_enable;
+};
+
+enum reg_access {
+	REG_NONE_ACCESS	= 0,
+	REG_RD_ACCESS	= 1,
+	REG_WR_ACCESS	= 2,
+};
+
+const u8 aw99706_regs[AW99706_REG_MAX + 1] = {
+	[AW99706_CFG0_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG1_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG2_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG3_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG4_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG5_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG6_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG7_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG8_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFG9_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFGA_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFGB_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFGC_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_CFGD_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
+	[AW99706_FLAG_REG]		= REG_RD_ACCESS,
+	[AW99706_CHIPID_REG]		= REG_RD_ACCESS,
+	[AW99706_LED_OPEN_FLAG_REG]	= REG_RD_ACCESS,
+	[AW99706_LED_SHORT_FLAG_REG]	= REG_RD_ACCESS,
+
+	/*
+	 * Write bit is dropped here, writing BIT(0) to MTPLDOSEL will unlock
+	 * Multi-time Programmable (MTP).
+	 */
+	[AW99706_MTPLDOSEL_REG]		= REG_RD_ACCESS,
+	[AW99706_MTPRUN_REG]		= REG_NONE_ACCESS,
+};
+
+static bool aw99706_readable_reg(struct device *dev, unsigned int reg)
+{
+	return aw99706_regs[reg] & REG_RD_ACCESS;
+}
+
+static bool aw99706_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return aw99706_regs[reg] & 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;
+	int (*lookup)(const struct aw99706_dt_prop *prop, u32 dt_val, u8 *val);
+	const u32 * const lookup_tbl;
+	u8 tbl_size;
+	u8 reg;
+	u8 mask;
+	u8 shift;
+	u32 def_val;
+};
+
+static int aw99706_dt_property_lookup(const struct aw99706_dt_prop *prop,
+				      u32 dt_val, u8 *val)
+{
+	int i;
+
+	if (!prop->lookup_tbl) {
+		*val = dt_val;
+		return 0;
+	}
+
+	for (i = 0; i < prop->tbl_size; i++)
+		if (prop->lookup_tbl[i] == dt_val)
+			break;
+
+	*val = i;
+
+	return i == prop->tbl_size ? -1 : 0;
+}
+
+#define MIN_ILED_MAX	5000
+#define MAX_ILED_MAX	50000
+#define STEP_ILED_MAX	500
+
+static int
+aw99706_dt_property_iled_max_convert(const struct aw99706_dt_prop *prop,
+				     u32 dt_val, u8 *val)
+{
+	if (dt_val > MAX_ILED_MAX || dt_val < MIN_ILED_MAX)
+		return -1;
+
+	*val = (dt_val - MIN_ILED_MAX) / STEP_ILED_MAX;
+
+	return (dt_val - MIN_ILED_MAX) % STEP_ILED_MAX;
+}
+
+static const struct aw99706_dt_prop aw99706_dt_props[] = {
+	{
+		"awinic,dim-mode", aw99706_dt_property_lookup,
+		NULL, 0,
+		AW99706_CFG0_REG,
+		AW99706_DIM_MODE_MASK, __builtin_ctz(AW99706_DIM_MODE_MASK),
+		1,
+	},
+	{
+		"awinic,sw-freq", aw99706_dt_property_lookup,
+		aw99706_sw_freq_tbl, ARRAY_SIZE(aw99706_sw_freq_tbl),
+		AW99706_CFG1_REG,
+		AW99706_SW_FREQ_MASK, __builtin_ctz(AW99706_SW_FREQ_MASK),
+		750000,
+	},
+	{
+		"awinic,sw-ilmt", aw99706_dt_property_lookup,
+		aw99706_sw_ilmt_tbl, ARRAY_SIZE(aw99706_sw_ilmt_tbl),
+		AW99706_CFG1_REG,
+		AW99706_SW_ILMT_MASK, __builtin_ctz(AW99706_SW_ILMT_MASK),
+		3000000,
+	},
+	{
+		"awinic,iled-max", aw99706_dt_property_iled_max_convert,
+		NULL, 0,
+		AW99706_CFG2_REG,
+		AW99706_ILED_MAX_MASK, __builtin_ctz(AW99706_ILED_MAX_MASK),
+		20000,
+
+	},
+	{
+		"awinic,uvlo-thres", aw99706_dt_property_lookup,
+		aw99706_ulvo_thres_tbl, ARRAY_SIZE(aw99706_ulvo_thres_tbl),
+		AW99706_CFG2_REG,
+		AW99706_UVLOSEL_MASK, __builtin_ctz(AW99706_UVLOSEL_MASK),
+		2200000,
+	},
+	{
+		"awinic,ramp-ctl", aw99706_dt_property_lookup,
+		NULL, 0,
+		AW99706_CFG6_REG,
+		AW99706_RAMP_CTL_MASK, __builtin_ctz(AW99706_RAMP_CTL_MASK),
+		2,
+	},
+};
+
+struct reg_init_data {
+	u8 reg;
+	u8 mask;
+	u8 val;
+};
+
+static struct reg_init_data reg_init_tbl[ARRAY_SIZE(aw99706_dt_props)];
+
+static void aw99706_dt_parse(struct aw99706_device *aw,
+			     struct backlight_properties *bl_props)
+{
+	const struct aw99706_dt_prop *prop;
+	u32 dt_val;
+	int ret, i;
+	u8 val;
+
+	for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
+		prop = &aw99706_dt_props[i];
+		ret = device_property_read_u32(aw->dev, prop->name, &dt_val);
+		if (ret < 0)
+			dt_val = prop->def_val;
+
+		if (prop->lookup(prop, dt_val, &val)) {
+			dev_warn(aw->dev, "invalid value %d for property %s, using default value %d\n",
+				 dt_val, prop->name, prop->def_val);
+
+			prop->lookup(prop, prop->def_val, &val);
+		}
+
+		reg_init_tbl[i].reg = prop->reg;
+		reg_init_tbl[i].mask = prop->mask;
+		reg_init_tbl[i].val = val << prop->shift;
+	}
+
+	aw->init_tbl = reg_init_tbl;
+	aw->init_tbl_size = ARRAY_SIZE(reg_init_tbl);
+
+	bl_props->brightness = AW99706_MAX_BRT_LVL >> 1;
+	bl_props->max_brightness = AW99706_MAX_BRT_LVL;
+	device_property_read_u32(aw->dev, "default-brightness",
+				 &bl_props->brightness);
+	device_property_read_u32(aw->dev, "max-brightness",
+				 &bl_props->max_brightness);
+
+	if (bl_props->max_brightness > AW99706_MAX_BRT_LVL)
+		bl_props->max_brightness = AW99706_MAX_BRT_LVL;
+
+	if (bl_props->brightness > bl_props->max_brightness)
+		bl_props->brightness = bl_props->max_brightness;
+}
+
+static int aw99706_hw_init(struct aw99706_device *aw)
+{
+	int ret, i;
+
+	gpiod_set_value_cansleep(aw->hwen_gpio, 1);
+
+	for (i = 0; i < aw->init_tbl_size; i++) {
+		ret = aw99706_i2c_update_bits(aw, aw->init_tbl[i].reg,
+					      aw->init_tbl[i].mask,
+					      aw->init_tbl[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_update_brightness(struct aw99706_device *aw, u32 brt_lvl)
+{
+	bool bl_enable_now = !!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;
+
+	if (aw->bl_enable != bl_enable_now) {
+		ret = aw99706_bl_enable(aw, bl_enable_now);
+		if (!ret)
+			aw->bl_enable = bl_enable_now;
+	}
+
+	return ret;
+}
+
+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, &props);
+
+	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.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

Re: [PATCH v2 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight

[Daniel Thompson]

On Mon, Nov 03, 2025 at 07:06:48PM +0800, Junjie Cao wrote:
> From: Pengyu Luo <mitltlatltl@gmail.com>
>
> 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>
> ---
> Changes in v2:
> - add handler for max-brightness and default-brightness
> - use proper units for properties (Krzysztof)
> - drop non-fixed properties (Krzysztof)
> - include default values in the aw99706_dt_props table (Daniel)
> - warn when a property value from DT is invalid (Daniel)
> - drop warning when optional properties are missing (Daniel)
> - add a function pointer into the aw99706_dt_props table to handle lookup (Daniel)
> - use a lookup function instead of hardcoding the formula for the iLED max (Daniel)
> - move BL enalbe handler into aw99706_update_brightness (Daniel)
> - Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-3-caojunjie650@gmail.com

Thanks for the changes.

I'm afraid I don't like encoding the `shift` in the DT properties table.
Caching something that is so easy to recalculate makes no sense to me.
See below:


> +struct aw99706_dt_prop {
> +	const char * const name;
> +	int (*lookup)(const struct aw99706_dt_prop *prop, u32 dt_val, u8 *val);
> +	const u32 * const lookup_tbl;
> +	u8 tbl_size;
> +	u8 reg;
> +	u8 mask;
> +	u8 shift;

There should bee no need to record `shift` here. It's just a
duplicating information already held in `mask`.


> +	u32 def_val;
> +};
> +
> +static int aw99706_dt_property_lookup(const struct aw99706_dt_prop *prop,
> +				      u32 dt_val, u8 *val)
> +{
> +	int i;
> +
> +	if (!prop->lookup_tbl) {
> +		*val = dt_val;
> +		return 0;
> +	}
> +
> +	for (i = 0; i < prop->tbl_size; i++)
> +		if (prop->lookup_tbl[i] == dt_val)
> +			break;
> +
> +	*val = i;
> +
> +	return i == prop->tbl_size ? -1 : 0;
> +}
> +
> +#define MIN_ILED_MAX	5000
> +#define MAX_ILED_MAX	50000
> +#define STEP_ILED_MAX	500
> +
> +static int
> +aw99706_dt_property_iled_max_convert(const struct aw99706_dt_prop *prop,
> +				     u32 dt_val, u8 *val)
> +{
> +	if (dt_val > MAX_ILED_MAX || dt_val < MIN_ILED_MAX)
> +		return -1;
> +
> +	*val = (dt_val - MIN_ILED_MAX) / STEP_ILED_MAX;
> +
> +	return (dt_val - MIN_ILED_MAX) % STEP_ILED_MAX;
> +}
> +
> +static const struct aw99706_dt_prop aw99706_dt_props[] = {
> +	{
> +		"awinic,dim-mode", aw99706_dt_property_lookup,
> +		NULL, 0,
> +		AW99706_CFG0_REG,
> +		AW99706_DIM_MODE_MASK, __builtin_ctz(AW99706_DIM_MODE_MASK),

These __builtin_ctz() calls shouldn't be in the lookup table (if they
are not in the lookup table then can never be inconsistant with the
mask).


> +		1,
> +	},
<snip>
> +	{
> +		"awinic,ramp-ctl", aw99706_dt_property_lookup,
> +		NULL, 0,
> +		AW99706_CFG6_REG,
> +		AW99706_RAMP_CTL_MASK, __builtin_ctz(AW99706_RAMP_CTL_MASK),
> +		2,
> +	},
> +};
> +
> +struct reg_init_data {
> +	u8 reg;
> +	u8 mask;
> +	u8 val;
> +};
> +
> +static struct reg_init_data reg_init_tbl[ARRAY_SIZE(aw99706_dt_props)];
> +
> +static void aw99706_dt_parse(struct aw99706_device *aw,
> +			     struct backlight_properties *bl_props)
> +{
> +	const struct aw99706_dt_prop *prop;
> +	u32 dt_val;
> +	int ret, i;
> +	u8 val;
> +
> +	for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> +		prop = &aw99706_dt_props[i];
> +		ret = device_property_read_u32(aw->dev, prop->name, &dt_val);
> +		if (ret < 0)
> +			dt_val = prop->def_val;
> +
> +		if (prop->lookup(prop, dt_val, &val)) {
> +			dev_warn(aw->dev, "invalid value %d for property %s, using default value %d\n",
> +				 dt_val, prop->name, prop->def_val);
> +
> +			prop->lookup(prop, prop->def_val, &val);
> +		}
> +
> +		reg_init_tbl[i].reg = prop->reg;
> +		reg_init_tbl[i].mask = prop->mask;
> +		reg_init_tbl[i].val = val << prop->shift;

Can't you just use FIELD_PREP() to set val (either here or at the point
the init table is consumed)? That why there's no ffs() or clz() at all.


> +	}
> +
> +	aw->init_tbl = reg_init_tbl;
> +	aw->init_tbl_size = ARRAY_SIZE(reg_init_tbl);

Copying a pointer to a single instance static data buffer into a
dynamically allocated data structure isn't right.

You should include the init table as part of `struct aw99706_device`.


> +
> +	bl_props->brightness = AW99706_MAX_BRT_LVL >> 1;
> +	bl_props->max_brightness = AW99706_MAX_BRT_LVL;
> +	device_property_read_u32(aw->dev, "default-brightness",
> +				 &bl_props->brightness);
> +	device_property_read_u32(aw->dev, "max-brightness",
> +				 &bl_props->max_brightness);
> +
> +	if (bl_props->max_brightness > AW99706_MAX_BRT_LVL)
> +		bl_props->max_brightness = AW99706_MAX_BRT_LVL;
> +
> +	if (bl_props->brightness > bl_props->max_brightness)
> +		bl_props->brightness = bl_props->max_brightness;
> +}
> +
> +static int aw99706_hw_init(struct aw99706_device *aw)
> +{
> +	int ret, i;
> +
> +	gpiod_set_value_cansleep(aw->hwen_gpio, 1);
> +
> +	for (i = 0; i < aw->init_tbl_size; i++) {
> +		ret = aw99706_i2c_update_bits(aw, aw->init_tbl[i].reg,
> +					      aw->init_tbl[i].mask,
> +					      aw->init_tbl[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);

This should use FIELD_PREP() not FIELD_MODIFY();


> +	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;
> +}


Daniel.

Re: [PATCH v2 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight

[Junjie Cao]

On Tue, Nov 4, 2025 at 12:30 AM Daniel Thompson <daniel@riscstar.com> wrote:
>
> On Mon, Nov 03, 2025 at 07:06:48PM +0800, Junjie Cao wrote:
> > From: Pengyu Luo <mitltlatltl@gmail.com>
> >
> > 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>
> > ---
> > Changes in v2:
> > - add handler for max-brightness and default-brightness
> > - use proper units for properties (Krzysztof)
> > - drop non-fixed properties (Krzysztof)
> > - include default values in the aw99706_dt_props table (Daniel)
> > - warn when a property value from DT is invalid (Daniel)
> > - drop warning when optional properties are missing (Daniel)
> > - add a function pointer into the aw99706_dt_props table to handle lookup (Daniel)
> > - use a lookup function instead of hardcoding the formula for the iLED max (Daniel)
> > - move BL enalbe handler into aw99706_update_brightness (Daniel)
> > - Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-3-caojunjie650@gmail.com
>
> Thanks for the changes.
>
> I'm afraid I don't like encoding the `shift` in the DT properties table.
> Caching something that is so easy to recalculate makes no sense to me.
> See below:
>
>
> > +struct aw99706_dt_prop {
> > +     const char * const name;
> > +     int (*lookup)(const struct aw99706_dt_prop *prop, u32 dt_val, u8 *val);
> > +     const u32 * const lookup_tbl;
> > +     u8 tbl_size;
> > +     u8 reg;
> > +     u8 mask;
> > +     u8 shift;
>
> There should bee no need to record `shift` here. It's just a
> duplicating information already held in `mask`.
>
>
> > +     u32 def_val;
> > +};
> > +
> > +static int aw99706_dt_property_lookup(const struct aw99706_dt_prop *prop,
> > +                                   u32 dt_val, u8 *val)
> > +{
> > +     int i;
> > +
> > +     if (!prop->lookup_tbl) {
> > +             *val = dt_val;
> > +             return 0;
> > +     }
> > +
> > +     for (i = 0; i < prop->tbl_size; i++)
> > +             if (prop->lookup_tbl[i] == dt_val)
> > +                     break;
> > +
> > +     *val = i;
> > +
> > +     return i == prop->tbl_size ? -1 : 0;
> > +}
> > +
> > +#define MIN_ILED_MAX 5000
> > +#define MAX_ILED_MAX 50000
> > +#define STEP_ILED_MAX        500
> > +
> > +static int
> > +aw99706_dt_property_iled_max_convert(const struct aw99706_dt_prop *prop,
> > +                                  u32 dt_val, u8 *val)
> > +{
> > +     if (dt_val > MAX_ILED_MAX || dt_val < MIN_ILED_MAX)
> > +             return -1;
> > +
> > +     *val = (dt_val - MIN_ILED_MAX) / STEP_ILED_MAX;
> > +
> > +     return (dt_val - MIN_ILED_MAX) % STEP_ILED_MAX;
> > +}
> > +
> > +static const struct aw99706_dt_prop aw99706_dt_props[] = {
> > +     {
> > +             "awinic,dim-mode", aw99706_dt_property_lookup,
> > +             NULL, 0,
> > +             AW99706_CFG0_REG,
> > +             AW99706_DIM_MODE_MASK, __builtin_ctz(AW99706_DIM_MODE_MASK),
>
> These __builtin_ctz() calls shouldn't be in the lookup table (if they
> are not in the lookup table then can never be inconsistant with the
> mask).
>
>
> > +             1,
> > +     },
> <snip>
> > +     {
> > +             "awinic,ramp-ctl", aw99706_dt_property_lookup,
> > +             NULL, 0,
> > +             AW99706_CFG6_REG,
> > +             AW99706_RAMP_CTL_MASK, __builtin_ctz(AW99706_RAMP_CTL_MASK),
> > +             2,
> > +     },
> > +};
> > +
> > +struct reg_init_data {
> > +     u8 reg;
> > +     u8 mask;
> > +     u8 val;
> > +};
> > +
> > +static struct reg_init_data reg_init_tbl[ARRAY_SIZE(aw99706_dt_props)];
> > +
> > +static void aw99706_dt_parse(struct aw99706_device *aw,
> > +                          struct backlight_properties *bl_props)
> > +{
> > +     const struct aw99706_dt_prop *prop;
> > +     u32 dt_val;
> > +     int ret, i;
> > +     u8 val;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> > +             prop = &aw99706_dt_props[i];
> > +             ret = device_property_read_u32(aw->dev, prop->name, &dt_val);
> > +             if (ret < 0)
> > +                     dt_val = prop->def_val;
> > +
> > +             if (prop->lookup(prop, dt_val, &val)) {
> > +                     dev_warn(aw->dev, "invalid value %d for property %s, using default value %d\n",
> > +                              dt_val, prop->name, prop->def_val);
> > +
> > +                     prop->lookup(prop, prop->def_val, &val);
> > +             }
> > +
> > +             reg_init_tbl[i].reg = prop->reg;
> > +             reg_init_tbl[i].mask = prop->mask;
> > +             reg_init_tbl[i].val = val << prop->shift;
>
> Can't you just use FIELD_PREP() to set val (either here or at the point
> the init table is consumed)? That why there's no ffs() or clz() at all.
>

Thanks for it, I tried to find something like FIELD_PREP() to avoid
keeping shift, but I failed to notice it in the bitfield.h. I will drop
the shift field and use it to handle bit operations gracefully.

>
> > +     }
> > +
> > +     aw->init_tbl = reg_init_tbl;
> > +     aw->init_tbl_size = ARRAY_SIZE(reg_init_tbl);
>
> Copying a pointer to a single instance static data buffer into a
> dynamically allocated data structure isn't right.
>
> You should include the init table as part of `struct aw99706_device`.
>

I see. Multiple instances should be taken into account.

>
> > +
> > +     bl_props->brightness = AW99706_MAX_BRT_LVL >> 1;
> > +     bl_props->max_brightness = AW99706_MAX_BRT_LVL;
> > +     device_property_read_u32(aw->dev, "default-brightness",
> > +                              &bl_props->brightness);
> > +     device_property_read_u32(aw->dev, "max-brightness",
> > +                              &bl_props->max_brightness);
> > +
> > +     if (bl_props->max_brightness > AW99706_MAX_BRT_LVL)
> > +             bl_props->max_brightness = AW99706_MAX_BRT_LVL;
> > +
> > +     if (bl_props->brightness > bl_props->max_brightness)
> > +             bl_props->brightness = bl_props->max_brightness;
> > +}
> > +
> > +static int aw99706_hw_init(struct aw99706_device *aw)
> > +{
> > +     int ret, i;
> > +
> > +     gpiod_set_value_cansleep(aw->hwen_gpio, 1);
> > +
> > +     for (i = 0; i < aw->init_tbl_size; i++) {
> > +             ret = aw99706_i2c_update_bits(aw, aw->init_tbl[i].reg,
> > +                                           aw->init_tbl[i].mask,
> > +                                           aw->init_tbl[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);
>
> This should use FIELD_PREP() not FIELD_MODIFY();
>

ACK. FIELD_PREP() makes more sense here.

Regards,
Junjie

Re: [PATCH v2 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight

[Krzysztof Kozlowski]

On Mon, Nov 03, 2025 at 07:06:48PM +0800, Junjie Cao wrote:
> +struct reg_init_data;
> +
> +struct aw99706_device {
> +	struct i2c_client *client;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct backlight_device *bl_dev;
> +	struct gpio_desc *hwen_gpio;
> +	struct reg_init_data *init_tbl;
> +	int init_tbl_size;
> +	bool bl_enable;
> +};
> +
> +enum reg_access {
> +	REG_NONE_ACCESS	= 0,
> +	REG_RD_ACCESS	= 1,
> +	REG_WR_ACCESS	= 2,
> +};
> +
> +const u8 aw99706_regs[AW99706_REG_MAX + 1] = {

Why isn't this static?

> +	[AW99706_CFG0_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG1_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG2_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG3_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG4_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG5_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG6_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG7_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG8_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFG9_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFGA_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFGB_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFGC_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_CFGD_REG]		= REG_RD_ACCESS | REG_WR_ACCESS,
> +	[AW99706_FLAG_REG]		= REG_RD_ACCESS,
> +	[AW99706_CHIPID_REG]		= REG_RD_ACCESS,
> +	[AW99706_LED_OPEN_FLAG_REG]	= REG_RD_ACCESS,
> +	[AW99706_LED_SHORT_FLAG_REG]	= REG_RD_ACCESS,
> +
> +	/*
> +	 * Write bit is dropped here, writing BIT(0) to MTPLDOSEL will unlock
> +	 * Multi-time Programmable (MTP).
> +	 */
> +	[AW99706_MTPLDOSEL_REG]		= REG_RD_ACCESS,
> +	[AW99706_MTPRUN_REG]		= REG_NONE_ACCESS,
> +};
> +

...

> +	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")

Why are you exiting the probe with a positive value like 4 or 8? This
makes no sense. Registers do not coontain Linux return codes.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight

[Junjie Cao]

On Tue, Nov 4, 2025 at 3:36 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Nov 03, 2025 at 07:06:48PM +0800, Junjie Cao wrote:
> > +struct reg_init_data;
> > +
> > +struct aw99706_device {
> > +     struct i2c_client *client;
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +     struct backlight_device *bl_dev;
> > +     struct gpio_desc *hwen_gpio;
> > +     struct reg_init_data *init_tbl;
> > +     int init_tbl_size;
> > +     bool bl_enable;
> > +};
> > +
> > +enum reg_access {
> > +     REG_NONE_ACCESS = 0,
> > +     REG_RD_ACCESS   = 1,
> > +     REG_WR_ACCESS   = 2,
> > +};
> > +
> > +const u8 aw99706_regs[AW99706_REG_MAX + 1] = {
>
> Why isn't this static?
>

That was an oversight on my part. It will be static.

> > +     [AW99706_CFG0_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG1_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG2_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG3_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG4_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG5_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG6_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG7_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG8_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFG9_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFGA_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFGB_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFGC_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_CFGD_REG]              = REG_RD_ACCESS | REG_WR_ACCESS,
> > +     [AW99706_FLAG_REG]              = REG_RD_ACCESS,
> > +     [AW99706_CHIPID_REG]            = REG_RD_ACCESS,
> > +     [AW99706_LED_OPEN_FLAG_REG]     = REG_RD_ACCESS,
> > +     [AW99706_LED_SHORT_FLAG_REG]    = REG_RD_ACCESS,
> > +
> > +     /*
> > +      * Write bit is dropped here, writing BIT(0) to MTPLDOSEL will unlock
> > +      * Multi-time Programmable (MTP).
> > +      */
> > +     [AW99706_MTPLDOSEL_REG]         = REG_RD_ACCESS,
> > +     [AW99706_MTPRUN_REG]            = REG_NONE_ACCESS,
> > +};
> > +
>
> ...
>
> > +     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")
>
> Why are you exiting the probe with a positive value like 4 or 8? This
> makes no sense. Registers do not coontain Linux return codes.
>

Thanks for pointing this out. Using -ENODEV makes more sense. I will do
it in the next version.

Regards,
Junjie