Message-ID: <aa330123-e6d9-44ce-b030-b266cba1df9c@kernel.org>
Date: Sat, 8 Nov 2025 15:46:01 +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 08/11/2025 15:21, Andreas Kemnade wrote:
> On Sat, 8 Nov 2025 13:17:31 +0100
> Krzysztof Kozlowski <krzk@...nel.org> wrote:
> 
>> 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.
>>
> well, I just took the SY7636A as reference. Is there any document describing
> the terme "thermal zone sensor". I would define a thermal zone as an area
> with things influencing each other thermically. These things are
> sensors, heat sources and sinks. Well, the panel typically does not produce
> much heat.
> But I do not insist on having that property here. As far as I understand,
> the hwmon uses this property as an indication to also create a thermal zone
> sensor.

That's Linux detail, but anyway you don't need it. This device does not
look like a part of thermal zones.

> 
>>> +    const: 0
>>> +
>>> +  enable-gpios:
>>> +    maxItems: 1
>>> +
>>> +  pg-gpios:
>>> +    maxItems: 1
>>> +
>>> +  ts-en-gpios:  
>>
>> It's called EN_TS, so en-ts-gpios.
>>
> ok
>>
>>> +    maxItems: 1
>>> +
>>> +  xon-gpios:  
>>
>> That's powerdown-gpios, see gpio-consumer-common.
>>
> looking a bit around: powerdown-gpios e.g. in the MCP4801
> describe an *input*, which should be connected to an output of the SoC. 
> Looking at the datasheet, I see "XON Open Drain N-MOS On-Resistance" so it is
> an *output* (same as for PG). So it is something different then the
> powerdown-gpios in e.g. the MCP4801.
> So it is a signal coming from the JD9930 after EN goes low in the power down
> sequence.

OK, I just briefly skimmed through datasheet.

> 
>>> +    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.
>>
> Hmm, no to what? I do not understand your comment.

Please use proper units for the field expressed in the property name
suffix and possible values (enum).
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

You also need default.

> So I guess a bit what might be options to discuss here:
> - put raw value for the bitfield here (what is currently done).
> - put the ms values here (then I would expect a suffix in the property name)
>   We have the mapping 0ms - 0, 1ms - 1, 2ms - 2, 4ms - 3, so it is
>   not identical.
I don't know what has to be identical. You want here 0, 1, 2 or 4 ms.
BTW, if you speak about driver complexity, getting register value out of
above is absolutely trivial, so not a suitable argument.

Best regards,
Krzysztof

Powered by blists - more mailing lists