Message-ID: <c32970a8-c1d1-4130-839b-981bca5373f3@kernel.org>
Date: Mon, 27 Oct 2025 09:38:23 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Junjie Cao <caojunjie650@...il.com>
Cc: Lee Jones <lee@...nel.org>, Daniel Thompson <danielt@...nel.org>,
 Jingoo Han <jingoohan1@...il.com>, Pavel Machek <pavel@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Helge Deller <deller@....de>,
 dri-devel@...ts.freedesktop.org, linux-leds@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-fbdev@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: leds: backlight: Add Awinic AW99706
 backlight

On 27/10/2025 07:58, Junjie Cao wrote:
> On Sun, Oct 26, 2025 at 9:48 PM Krzysztof Kozlowski <krzk@...nel.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

Powered by blists - more mailing lists