Re: [PATCH v2 3/8] dt-bindings: rtc: isl12022: add bindings for battery alarm trip levels

[Rasmus Villemoes]

On 13/06/2023 21.09, Krzysztof Kozlowski wrote:
> On 13/06/2023 15:00, Rasmus Villemoes wrote:
>> The isl12022 has a built-in support for monitoring the voltage of the
>> backup battery, and setting bits in the status register when that
>> voltage drops below two predetermined levels (usually 85% and 75% of
>> the nominal voltage). However, since it can operate at wide range of
>> battery voltages (2.5V - 5.5V), one must configure those trip levels
>> according to which battery is used on a given board.
>>
>> Add bindings for defining these two trip levels. While the register
>> and bit names suggest that they should correspond to 85% and 75% of
>> the nominal battery voltage, the data sheet also says
>>
>>   There are total of 7 levels that could be selected for the first
>>   alarm. Any of the of levels could be selected as the first alarm
>>   with no reference as to nominal Battery voltage level.
>>
>> Hence this provides the hardware designer the ability to choose values
>> based on the discharge characteristics of the battery chosen for the
>> given product, rather than just having one battery-microvolt property
>> and having the driver choose levels close to 0.85/0.75 times that.
>>
>> Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
>> ---
>>  .../devicetree/bindings/rtc/intersil,isl12022.yaml | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>> index 7c1e638d657a..d5d3a687a34d 100644
>> --- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>> @@ -22,6 +22,18 @@ properties:
>>    interrupts:
>>      maxItems: 1
>>  
>> +  isil,trip-level85-microvolt:
> 
> Why encoding level85 in the property name? Your commit msg (datasheet)
> suggests this is quite flexible, so why it cannot be just list of two
> trip levels - for first and second interrupt?

Yeah, so I did consider just making it a two-element array
isil,trip-levels-microvolt. But then I didn't know how to express the
enum constraint, i.e. that the first must be one of the 2125000, ...,
4675000 values and the second one of the 1875000, ..., 4125000 ones. Is
that possible, without providing a list of 49 possible pairs? Or is it
sufficient to just write this out in prose?

I'm also happy to use other names for these. I just chose to use the 85
and 75 nomenclature because that matches the field names.

>> +    description: |
> 
> Do not need '|' unless you need to preserve formatting.

OK.

Rasmus