linux-kernel - Re: [PATCH 2/3] dt-bindings: regulator: Add Fitipower FP9931/JD9930

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251108-vagabond-lyrical-hawk-ad3490@kuoka>
Date: Sat, 8 Nov 2025 13:17:31 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Andreas Kemnade <andreas@...nade.info>
Cc: Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
	Guenter Roeck <linux@...ck-us.net>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-hwmon@...r.kernel.org
Subject: Re: [PATCH 2/3] dt-bindings: regulator: Add Fitipower FP9931/JD9930

On Fri, Nov 07, 2025 at 09:06:45PM +0100, Andreas Kemnade wrote:
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: fiti,fp9931
> +
> +      - items:
> +          - const: fiti,jd9930
> +          - const: fiti,fp9931
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#thermal-sensor-cells':

Why is this a thermal zone sensor? Aren't you mixing temperature
reading with soc? For temperature reading you can use hwmon, for
example.

> +    const: 0
> +
> +  enable-gpios:
> +    maxItems: 1
> +
> +  pg-gpios:
> +    maxItems: 1
> +
> +  ts-en-gpios:

It's called EN_TS, so en-ts-gpios.


> +    maxItems: 1
> +
> +  xon-gpios:

That's powerdown-gpios, see gpio-consumer-common.

> +    maxItems: 1
> +
> +  vin-supply:
> +    description:
> +      Supply for the whole chip. Some vendor kernels and devicetrees
> +      declare this as a non-existing GPIO named "pwrall".
> +
> +  fiti,tdly:

No, look at datasheet. What values are there? ms.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      Power up soft start delay settings tDLY1-4 bitfields in the
> +      POWERON_DELAY register
> +

Drop blank line

> +    minItems: 4
> +    maxItems: 4
> +
> +  VCOM:

Lowercase, just group them under regulators node and use patterns.


> +    type: object
> +    $ref: /schemas/regulator/regulator.yaml#
> +    unevaluatedProperties: false
> +    description:
> +      The regulator for the compenstation voltage.
> +    properties:
> +      regulator-name:
> +        const: VCOM

No, why? Board designers could call it differently. Drop.

> +
> +  VPOSNEG:
> +    type: object
> +    $ref: /schemas/regulator/regulator.yaml#
> +    unevaluatedProperties: false
> +    description:
> +      The pair of symmetric LDOs
> +    properties:
> +      regulator-name:
> +        const: VPOSNEG

Drop

> +
> +  V3P3:
> +    type: object
> +    $ref: /schemas/regulator/regulator.yaml#
> +    unevaluatedProperties: false
> +    description:
> +      The pair of symmetric LDOs
> +    properties:
> +      regulator-name:
> +        const: V3P3

Drop

> +
> +required:
> +  - compatible
> +  - reg
> +  - '#thermal-sensor-cells'
> +  - pg-gpios
> +  - enable-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        fp9931: pmic@18 {

Drop unused label.

> +          compatible = "fiti,fp9931";
> +          reg = <0x18>;
> +          pinctrl-names = "default";
> +          pinctrl-0 = <&pinctrl_fp9931_gpio>;
> +          #thermal-sensor-cells = <0>;
> +          vin-supply = <&epd_pmic_supply>;
> +          pg-gpios = <&gpio2 7 GPIO_ACTIVE_HIGH>;
> +          ts-en-gpios = <&gpio2 9 GPIO_ACTIVE_HIGH>;
> +          enable-gpios = <&gpio2 8 GPIO_ACTIVE_HIGH>;
> +          fiti,tdly = <2 2 3 3>;
> +
> +          vcom_reg: VCOM {
> +            regulator-name = "VCOM";

Names are always lowercase.

Best regards,
Krzysztof