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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: missing 'pwms' property for PWM input
- [Low] dt-bindings: missing power supply property
--

commit bdcbdabe3e1453e58fc8f39fabc82b20db81480c
Author: Luka Panio <lukapanio@gmail.com>

dt-bindings: leds: backlight: Add Awinic AW99703 backlight

The Awinic AW99703 is a 3-string WLED driver converter with an
integrated boost converter and 11-bit brightness control over I2C,
optionally combined with a PWM input.

> diff --git a/Documentation/devicetree/bindings/leds/backlight/awinic,aw99703.yaml b/Documentation/devicetree/bindings/leds/backlight/awinic,aw99703.yaml
> new file mode 100644
> index 0000000000000..62e8cc3118f2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/awinic,aw99703.yaml
> @@ -0,0 +1,131 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/backlight/awinic,aw99703.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Awinic AW99703 WLED Backlight Driver
> +
> +maintainers:
> +  - Luka Panio <lukapanio@gmail.com>
> +
> +description:
> +  The AW99703 is a 3-string WLED driver with an integrated boost converter
> +  and 11-bit I2C (optionally PWM) brightness control.
> +
> +allOf:
> +  - $ref: common.yaml#
> +
> +properties:
> +  compatible:
> +    const: awinic,aw99703

[Severity: Low]
Is a power supply property needed for the boost converter?

The hardware has an integrated boost converter, which typically requires
a supply such as 'vin-supply' or 'vcc-supply'. Because the schema enforces
'unevaluatedProperties: false', boards with a controllable regulator powering
the backlight will fail schema validation if they attempt to define it.

[ ... ]

> +  awinic,inductor-nanohenry:
> +    description: Boost inductor value.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [4700, 10000]
> +    default: 4700
> +
> +  awinic,pwm-input:
> +    description: Combine the PWM input pin with the I2C brightness.
> +    type: boolean

[Severity: Medium]
Should the standard 'pwms' property be defined alongside this property?

The schema defines 'awinic,pwm-input' to indicate the hardware uses a PWM
input pin, but it doesn't define the standard 'pwms' property. Since this
schema uses 'unevaluatedProperties: false', board device trees attempting
to properly link a PWM controller using 'pwms = <&pwm ...>;' will fail
schema validation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016162912.64581-1-lukapanio@gmail.com?part=1

Re: [PATCH 2/2] backlight: Add Awinic AW99703 backlight driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] build: Incompatible pointer type passed to device_property_read_u32()
- [Medium] gpio: Hardware left powered on upon early probe failure
- [Low] backlight: TOCTOU race in aw99703_shutdown() missing update_lock
--

commit b010e2c6526192785d6b65bcadf761f3928c7acd
Author: Luka Panio <lukapanio@gmail.com>

backlight: Add Awinic AW99703 backlight driver

Add a driver for the Awinic AW99703 3-string WLED backlight controller.

> diff --git a/drivers/video/backlight/aw99703.c b/drivers/video/backlight/aw99703.c
> new file mode 100644
> index 0000000000000..b0a46753d1765
> --- /dev/null
> +++ b/drivers/video/backlight/aw99703.c
[ ... ]
> +static void aw99703_dt_parse(struct aw99703_device *aw,
> +			     struct backlight_properties *props)
> +{
[ ... ]
> +	props->max_brightness = AW99703_MAX_BRT_LVL;
> +	device_property_read_u32(aw->dev, "max-brightness",
> +				 &props->max_brightness);

[Severity: High]
Does this code trigger a -Wincompatible-pointer-types build failure?

device_property_read_u32() expects a u32 * as its third argument, but
&props->max_brightness is a pointer to int. In recent kernel builds, this
warning is promoted to a fatal error.

> +	if (props->max_brightness > AW99703_MAX_BRT_LVL)
> +		props->max_brightness = AW99703_MAX_BRT_LVL;
> +
> +	props->brightness = props->max_brightness >> 1;
> +	device_property_read_u32(aw->dev, "default-brightness",
> +				 &props->brightness);

[Severity: High]
Similarly, &props->brightness is also a pointer to int.

[ ... ]
> +static void aw99703_power_off(void *data)
> +{
> +	struct aw99703_device *aw = data;
> +
> +	aw99703_update_brightness(aw, 0);
> +	gpiod_set_value_cansleep(aw->hwen_gpio, 0);
> +}
> +
> +static int aw99703_probe(struct i2c_client *client)
> +{
[ ... ]
> +	if (aw->hwen_gpio) {
> +		ret = gpiod_direction_output(aw->hwen_gpio, 1);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to drive enable gpio\n");
> +		fsleep(AW99703_HWEN_ON_DELAY_US);
> +	}
> +
> +	ret = aw99703_chip_id_read(aw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "chip id check failed\n");

[Severity: Medium]
If the chip ID read fails, does this return without de-asserting the GPIO?

The probe function sets the hwen_gpio output to 1 to power on the chip,
but if aw99703_chip_id_read() or aw99703_hw_init() fail, it returns an
error code immediately. Since the devm_add_action_or_reset() for
aw99703_power_off is registered later in the function, the cleanup
action won't execute on early probe failure, leaving the hardware
permanently enabled.

> +
> +	props.type = BACKLIGHT_RAW;
> +	aw99703_dt_parse(aw, &props);
> +	aw99703_read_boot_brightness(aw, &props);
> +
> +	ret = aw99703_hw_init(aw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize chip\n");
> +
> +	ret = devm_add_action_or_reset(dev, aw99703_power_off, aw);
[ ... ]
> +static void aw99703_shutdown(struct i2c_client *client)
> +{
> +	aw99703_power_off(i2c_get_clientdata(client));
> +}

[Severity: Low]
Is there a data race between this shutdown callback and concurrent sysfs
writes to the brightness attribute?

aw99703_shutdown() invokes aw99703_power_off(), which modifies the
aw->bl_enable flag and writes to I2C registers via aw99703_update_brightness()
without acquiring the backlight bd->update_lock mutex. Since userspace tasks
are not frozen during system shutdown, a concurrent sysfs operation could
attempt to communicate with the chip while its registers are being written
or after the power pin is de-asserted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016162912.64581-1-lukapanio@gmail.com?part=2

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

[Krzysztof Kozlowski]

On Fri, Oct 16, 2026 at 06:29:11PM +0200, Luka Panio wrote:
> +  awinic,iled-max-microamp:
> +    description:
> +      Full-scale current of each LED string, reached at the maximum
> +      brightness code (and 100% PWM duty cycle if PWM dimming is used).
> +    minimum: 4800
> +    maximum: 29600
> +    multipleOf: 800
> +    default: 20000
> +
> +  awinic,ramp-on-time-us:
> +    description: Current sink ramp-up time when leaving standby.
> +    enum: [512, 1000, 2000, 4000, 8000, 16000, 32000, 64000, 128000,
> +           256000, 512000, 1024000, 2048000, 4096000, 8192000, 16384000]
> +    default: 8000
> +
> +  awinic,ramp-off-time-us:
> +    description: Current sink ramp-down time when entering standby.
> +    enum: [512, 1000, 2000, 4000, 8000, 16000, 32000, 64000, 128000,
> +           256000, 512000, 1024000, 2048000, 4096000, 8192000, 16384000]
> +    default: 8000
> +
> +  awinic,pwm-trans-time-ms:
> +    description:
> +      Transition time between brightness levels set by the PWM input,
> +      for a change over the full duty cycle range.
> +    enum: [2, 4, 8, 16, 32, 64, 128, 256]
> +    default: 2

Feels like you are re-implementing triggers. Several properties actually
feel duplicating standard LED ones.

> +
> +  awinic,i2c-trans-time-ms:
> +    description:
> +      Fixed transition time between brightness levels set over I2C,
> +      independent of the size of the change. When absent the brightness
> +      ramps with a fixed slope of 1 us per brightness code instead.
> +    enum: [128, 192, 256, 320, 384, 448, 512, 576, 640, 768, 1024, 1280,
> +           1536, 2560, 4096]

Best regards,
Krzysztof

Re: [PATCH 2/2] backlight: Add Awinic AW99703 backlight driver

[Daniel Thompson]

On Fri, Oct 16, 2026 at 06:29:12PM +0200, Luka Panio wrote:
> Add a driver for the Awinic AW99703 3-string WLED backlight controller.
>
> Register values follow the AW99703 datasheet (V1.6), the structure
> follows the aw99706 driver.

Given how much the structure follows the aw99706 driver I'd expect there
to be more code sharing between the drivers.

Cold AW99703 support be implemented by extending the existing aw99706
driver?


Daniel.

Re: [PATCH 0/2] backlight: Add Awinic AW99703 driver

[Daniel Thompson]

On Mon, Oct 19, 2026 at 01:26:21PM +0200, Luka Panio wrote:
> Hi Daniel, Krzysztof,
>
> Thanks for the reviews. Before reworking this for v2 I'd like to agree
> on the approach.
>
> Following Daniel's suggestion I'll add AW99703 support to the aw99706
> driver using per-chip data instead of adding a new driver. The register
> maps differ almost entirely, so most of the DT properties stay
> per-chip (dim-mode/UVLO/ramp-ctl on AW99706, OVP/OCP/inductor on
> AW99703).
>
> Following Krzysztof's review, the AW99703 binding will drop the
> ramp/transition timing and mapping properties and use the standard
> led-sources and led-max-microamp properties.
>
> That leaves the one common property, the maximum LED current,
> spelled differently in a single driver: awinic,iled-max-microamp for
> AW99706 and led-max-microamp for AW99703. To avoid that I'd like to:
>
>   - add led-max-microamp to awinic,aw99706.yaml and mark
>     awinic,iled-max-microamp as deprecated
>   - have the driver read led-max-microamp for both chips, falling back
>     to awinic,iled-max-microamp on AW99706 so existing DTs keep working
>
> Does that sound acceptable?

IIUC you plan to improve the binding for AW99706 before adding AW99703
support, and you will keep the old DT property working to that
existing DTBs continue to work?

Both sound good to me.


Daniel.