[v5,2/2] dt-bindings: rtc: add max313xx RTCs

  Devicetree binding documentation for Analog Devices MAX313XX RTCs

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
---
 .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
  

On 03/04/2023 17:43, Ibrahim Tilki wrote:
> Devicetree binding documentation for Analog Devices MAX313XX RTCs
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> ---
>  .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
>  1 file changed, 144 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> new file mode 100644
> index 000000000..0c17a395e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX313XX series I2C RTCs
> +
> +maintainers:
> +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> +
> +description: Analog Devices MAX313XX series I2C RTCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,max31328
> +      - adi,max31329
> +      - adi,max31331
> +      - adi,max31334
> +      - adi,max31341
> +      - adi,max31342
> +      - adi,max31343
> +
> +  reg:
> +    description: I2C address of the RTC
> +    items:
> +      - enum: [0x68, 0x69]
> +
> +  interrupts:
> +    description: |

Do not need '|'.

> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> +      configuration.
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    description: |

Do not need '|'.

> +      RTC can be used as a clock source through its clock output pin when
> +      supplied.
> +    const: 0
> +
> +  clocks:
> +    description: |

Do not need '|'.

> +      RTC uses this clock for clock input when supplied. Clock has to provide
> +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> +    maxItems: 1
> +
> +  aux-voltage-chargeable:
> +    enum: [0, 1, 2]
> +    description: |
> +      Enables trickle charger.
> +      0: Charger is disabled (default)
> +      1: Charger is enabled
> +      2: Charger is enabled with a diode

2 is not an allowed value. I asked to drop this property. It is coming
from rtc.yaml. I also do not understand "with a diode". So otherwise it
is charging with, I don't know, FET?

> +
> +  trickle-resistor-ohms:
> +    description: Selected resistor for trickle charger.
> +    enum: [3000, 6000, 11000]
> +
> +required:
> +  - compatible
> +  - reg
> +
Best regards,
Krzysztof
  

On 04/04/2023 08:17:40+0200, Krzysztof Kozlowski wrote:
> On 03/04/2023 17:43, Ibrahim Tilki wrote:
> > Devicetree binding documentation for Analog Devices MAX313XX RTCs
> > 
> > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > ---
> >  .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
> >  1 file changed, 144 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > new file mode 100644
> > index 000000000..0c17a395e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > @@ -0,0 +1,144 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2022 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices MAX313XX series I2C RTCs
> > +
> > +maintainers:
> > +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > +
> > +description: Analog Devices MAX313XX series I2C RTCs.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max31328
> > +      - adi,max31329
> > +      - adi,max31331
> > +      - adi,max31334
> > +      - adi,max31341
> > +      - adi,max31342
> > +      - adi,max31343
> > +
> > +  reg:
> > +    description: I2C address of the RTC
> > +    items:
> > +      - enum: [0x68, 0x69]
> > +
> > +  interrupts:
> > +    description: |
> 
> Do not need '|'.
> 
> > +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> > +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> > +      configuration.
> > +    maxItems: 1
> > +
> > +  "#clock-cells":
> > +    description: |
> 
> Do not need '|'.
> 
> > +      RTC can be used as a clock source through its clock output pin when
> > +      supplied.
> > +    const: 0
> > +
> > +  clocks:
> > +    description: |
> 
> Do not need '|'.
> 
> > +      RTC uses this clock for clock input when supplied. Clock has to provide
> > +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> > +    maxItems: 1
> > +
> > +  aux-voltage-chargeable:
> > +    enum: [0, 1, 2]
> > +    description: |
> > +      Enables trickle charger.
> > +      0: Charger is disabled (default)
> > +      1: Charger is enabled
> > +      2: Charger is enabled with a diode
> 
> 2 is not an allowed value. I asked to drop this property. It is coming
> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> is charging with, I don't know, FET?

No, what is not explained here (and maybe not unsterstood by the
submitter) is that the RTC has an extra diode so, charging will always
enable a diode, select a resistor and then have or not an extra diode.
Figure2 of the MAX31329 datasheet is great.

> 
> > +
> > +  trickle-resistor-ohms:
> > +    description: Selected resistor for trickle charger.
> > +    enum: [3000, 6000, 11000]
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> Best regards,
> Krzysztof
>
  

On 04/04/2023 09:10, Alexandre Belloni wrote:
>>
>>> +      RTC can be used as a clock source through its clock output pin when
>>> +      supplied.
>>> +    const: 0
>>> +
>>> +  clocks:
>>> +    description: |
>>
>> Do not need '|'.
>>
>>> +      RTC uses this clock for clock input when supplied. Clock has to provide
>>> +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
>>> +    maxItems: 1
>>> +
>>> +  aux-voltage-chargeable:
>>> +    enum: [0, 1, 2]
>>> +    description: |
>>> +      Enables trickle charger.
>>> +      0: Charger is disabled (default)
>>> +      1: Charger is enabled
>>> +      2: Charger is enabled with a diode
>>
>> 2 is not an allowed value. I asked to drop this property. It is coming
>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
>> is charging with, I don't know, FET?
> 
> No, what is not explained here (and maybe not unsterstood by the
> submitter) is that the RTC has an extra diode so, 

Value of 2 is still not allowed and if the patch was tested, it would be
clearly visible. Unfortunately patch was not tested...

> charging will always
> enable a diode, select a resistor and then have or not an extra diode.
> Figure2 of the MAX31329 datasheet is great.

So the diode is in the max313xx? Then why enabling it is a property of
DT? Either this should be inferred from compatible or is even a policy,
not a DT property. Just because device has a register for something, is
not an argument that "something" should be in DT.


Best regards,
Krzysztof
  

On 04/04/2023 09:21:56+0200, Krzysztof Kozlowski wrote:
> On 04/04/2023 09:10, Alexandre Belloni wrote:
> >>
> >>> +      RTC can be used as a clock source through its clock output pin when
> >>> +      supplied.
> >>> +    const: 0
> >>> +
> >>> +  clocks:
> >>> +    description: |
> >>
> >> Do not need '|'.
> >>
> >>> +      RTC uses this clock for clock input when supplied. Clock has to provide
> >>> +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> >>> +    maxItems: 1
> >>> +
> >>> +  aux-voltage-chargeable:
> >>> +    enum: [0, 1, 2]
> >>> +    description: |
> >>> +      Enables trickle charger.
> >>> +      0: Charger is disabled (default)
> >>> +      1: Charger is enabled
> >>> +      2: Charger is enabled with a diode
> >>
> >> 2 is not an allowed value. I asked to drop this property. It is coming
> >> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> >> is charging with, I don't know, FET?
> > 
> > No, what is not explained here (and maybe not unsterstood by the
> > submitter) is that the RTC has an extra diode so, 
> 
> Value of 2 is still not allowed and if the patch was tested, it would be
> clearly visible. Unfortunately patch was not tested...
> 
> > charging will always
> > enable a diode, select a resistor and then have or not an extra diode.
> > Figure2 of the MAX31329 datasheet is great.
> 
> So the diode is in the max313xx? Then why enabling it is a property of
> DT? Either this should be inferred from compatible or is even a policy,
> not a DT property. Just because device has a register for something, is
> not an argument that "something" should be in DT.

Well, it depends on the battery that is installed on the board so it
makes sense to have it in DT.
  

On 04/04/2023 09:44, Alexandre Belloni wrote:
>>
>>> charging will always
>>> enable a diode, select a resistor and then have or not an extra diode.
>>> Figure2 of the MAX31329 datasheet is great.
>>
>> So the diode is in the max313xx? Then why enabling it is a property of
>> DT? Either this should be inferred from compatible or is even a policy,
>> not a DT property. Just because device has a register for something, is
>> not an argument that "something" should be in DT.
> 
> Well, it depends on the battery that is installed on the board so it
> makes sense to have it in DT.

OK, that would be a good reason, but I wonder why? Why choosing diode or
not depends on the battery? Wouldn't you always want to have the diode?

Best regards,
Krzysztof
  

> On 04/04/2023 08:17:40+0200, Krzysztof Kozlowski wrote:
> > On 03/04/2023 17:43, Ibrahim Tilki wrote:
> > > Devicetree binding documentation for Analog Devices MAX313XX RTCs
> > > 
> > > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > > Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > > ---
> > > .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
> > > 1 file changed, 144 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml \
> > > b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml new file mode 100644
> > > index 000000000..0c17a395e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> > > @@ -0,0 +1,144 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright 2022 Analog Devices Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Analog Devices MAX313XX series I2C RTCs
> > > +
> > > +maintainers:
> > > +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > > +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> > > +
> > > +description: Analog Devices MAX313XX series I2C RTCs.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,max31328
> > > +      - adi,max31329
> > > +      - adi,max31331
> > > +      - adi,max31334
> > > +      - adi,max31341
> > > +      - adi,max31342
> > > +      - adi,max31343
> > > +
> > > +  reg:
> > > +    description: I2C address of the RTC
> > > +    items:
> > > +      - enum: [0x68, 0x69]
> > > +
> > > +  interrupts:
> > > +    description: |
> > 
> > Do not need '|'.
> > 
> > > +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> > > +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> > > +      configuration.
> > > +    maxItems: 1
> > > +
> > > +  "#clock-cells":
> > > +    description: |
> > 
> > Do not need '|'.
> > 
> > > +      RTC can be used as a clock source through its clock output pin when
> > > +      supplied.
> > > +    const: 0
> > > +
> > > +  clocks:
> > > +    description: |
> > 
> > Do not need '|'.
> > 
> > > +      RTC uses this clock for clock input when supplied. Clock has to provide
> > > +      one of these four frequencies: 1Hz, 50Hz, 60Hz or 32.768kHz.
> > > +    maxItems: 1
> > > +
> > > +  aux-voltage-chargeable:
> > > +    enum: [0, 1, 2]
> > > +    description: |
> > > +      Enables trickle charger.
> > > +      0: Charger is disabled (default)
> > > +      1: Charger is enabled
> > > +      2: Charger is enabled with a diode
> > 
> > 2 is not an allowed value. I asked to drop this property. It is coming
> > from rtc.yaml. I also do not understand "with a diode". So otherwise it
> > is charging with, I don't know, FET?
> 
> No, what is not explained here (and maybe not unsterstood by the
> submitter) is that the RTC has an extra diode so, charging will always
> enable a diode, select a resistor and then have or not an extra diode.
> Figure2 of the MAX31329 datasheet is great.
> 

That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
So if I can't have "adi,trickle-diode-enable" and can't add an additional value
to "aux-voltage-chargeable", I am not sure how to add support for the extra
diode at this point.

Best regards,
Ibrahim

> > 
> > > +
> > > +  trickle-resistor-ohms:
> > > +    description: Selected resistor for trickle charger.
> > > +    enum: [3000, 6000, 11000]
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > Best regards,
> > Krzysztof
> >
  

> On 04/04/2023 09:44, Alexandre Belloni wrote:
> >>
> >>> charging will always
> >>> enable a diode, select a resistor and then have or not an extra diode.
> >>> Figure2 of the MAX31329 datasheet is great.
> >>
> >> So the diode is in the max313xx? Then why enabling it is a property of
> >> DT? Either this should be inferred from compatible or is even a policy,
> >> not a DT property. Just because device has a register for something, is
> >> not an argument that "something" should be in DT.
> > 
> > Well, it depends on the battery that is installed on the board so it
> > makes sense to have it in DT.
> 
> OK, that would be a good reason, but I wonder why? Why choosing diode or
> not depends on the battery? Wouldn't you always want to have the diode?
> 
> Best regards,
> Krzysztof

The datasheet doesn't say much about the extra diode but there is always a
schottky diode. The extra diode might be there to cause voltage drop.

Best regards,
Ibrahim
  

On 04/04/2023 10:14:33+0200, Krzysztof Kozlowski wrote:
> On 04/04/2023 09:44, Alexandre Belloni wrote:
> >>
> >>> charging will always
> >>> enable a diode, select a resistor and then have or not an extra diode.
> >>> Figure2 of the MAX31329 datasheet is great.
> >>
> >> So the diode is in the max313xx? Then why enabling it is a property of
> >> DT? Either this should be inferred from compatible or is even a policy,
> >> not a DT property. Just because device has a register for something, is
> >> not an argument that "something" should be in DT.
> > 
> > Well, it depends on the battery that is installed on the board so it
> > makes sense to have it in DT.
> 
> OK, that would be a good reason, but I wonder why? Why choosing diode or
> not depends on the battery? Wouldn't you always want to have the diode?
> 

It limits the maximum current used to charge the battery or supercap to
not exceed what is allowed.
  

On 04/04/2023 11:56, Alexandre Belloni wrote:
> On 04/04/2023 10:14:33+0200, Krzysztof Kozlowski wrote:
>> On 04/04/2023 09:44, Alexandre Belloni wrote:
>>>>
>>>>> charging will always
>>>>> enable a diode, select a resistor and then have or not an extra diode.
>>>>> Figure2 of the MAX31329 datasheet is great.
>>>>
>>>> So the diode is in the max313xx? Then why enabling it is a property of
>>>> DT? Either this should be inferred from compatible or is even a policy,
>>>> not a DT property. Just because device has a register for something, is
>>>> not an argument that "something" should be in DT.
>>>
>>> Well, it depends on the battery that is installed on the board so it
>>> makes sense to have it in DT.
>>
>> OK, that would be a good reason, but I wonder why? Why choosing diode or
>> not depends on the battery? Wouldn't you always want to have the diode?
>>
> 
> It limits the maximum current used to charge the battery or supercap to
> not exceed what is allowed.

and I still wonder why someone would like to allow exceeding what is
allowed. IOW, what is the use case to disable the diode?

Best regards,
Krzysztof
  

On 04/04/2023 11:26, Tilki, Ibrahim wrote:
>>>> +  aux-voltage-chargeable:
>>>> +    enum: [0, 1, 2]
>>>> +    description: |
>>>> +      Enables trickle charger.
>>>> +      0: Charger is disabled (default)
>>>> +      1: Charger is enabled
>>>> +      2: Charger is enabled with a diode
>>>
>>> 2 is not an allowed value. I asked to drop this property. It is coming
>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
>>> is charging with, I don't know, FET?
>>
>> No, what is not explained here (and maybe not unsterstood by the
>> submitter) is that the RTC has an extra diode so, charging will always
>> enable a diode, select a resistor and then have or not an extra diode.
>> Figure2 of the MAX31329 datasheet is great.
>>
> 
> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
> to "aux-voltage-chargeable", I am not sure how to add support for the extra
> diode at this point.

Ask the person who asked you to remove adi,trickle-diode-enable...
Anyway even if you decided to go with such advise, the solution is not
to create binding which fails.


Best regards,
Krzysztof
  

>>>>> +  aux-voltage-chargeable:
>>>>> +    enum: [0, 1, 2]
>>>>> +    description: |
>>>>> +      Enables trickle charger.
>>>>> +      0: Charger is disabled (default)
>>>>> +      1: Charger is enabled
>>>>> +      2: Charger is enabled with a diode
>>>>
>>>> 2 is not an allowed value. I asked to drop this property. It is coming
>>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
>>>> is charging with, I don't know, FET?
>>>
>>> No, what is not explained here (and maybe not unsterstood by the
>>> submitter) is that the RTC has an extra diode so, charging will always
>>> enable a diode, select a resistor and then have or not an extra diode.
>>> Figure2 of the MAX31329 datasheet is great.
>>>
>> 
>> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
>> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
>> to "aux-voltage-chargeable", I am not sure how to add support for the extra
>> diode at this point.
>
> Ask the person who asked you to remove adi,trickle-diode-enable...

That was the purpose.

> Anyway even if you decided to go with such advise, the solution is not
> to create binding which fails.

I didn't see that it would fail, should have checked the binding with using
the property in examples. I'm sorry about that.

Best regards,
Ibrahim
  

On 04/04/2023 12:06:14+0200, Krzysztof Kozlowski wrote:
> On 04/04/2023 11:56, Alexandre Belloni wrote:
> > On 04/04/2023 10:14:33+0200, Krzysztof Kozlowski wrote:
> >> On 04/04/2023 09:44, Alexandre Belloni wrote:
> >>>>
> >>>>> charging will always
> >>>>> enable a diode, select a resistor and then have or not an extra diode.
> >>>>> Figure2 of the MAX31329 datasheet is great.
> >>>>
> >>>> So the diode is in the max313xx? Then why enabling it is a property of
> >>>> DT? Either this should be inferred from compatible or is even a policy,
> >>>> not a DT property. Just because device has a register for something, is
> >>>> not an argument that "something" should be in DT.
> >>>
> >>> Well, it depends on the battery that is installed on the board so it
> >>> makes sense to have it in DT.
> >>
> >> OK, that would be a good reason, but I wonder why? Why choosing diode or
> >> not depends on the battery? Wouldn't you always want to have the diode?
> >>
> > 
> > It limits the maximum current used to charge the battery or supercap to
> > not exceed what is allowed.
> 
> and I still wonder why someone would like to allow exceeding what is
> allowed. IOW, what is the use case to disable the diode?
> 

The battery or supercap is the part defining the current limit. why
would you want to limit the current if you can charge faster?
  

On 04/04/2023 10:35:53+0000, Tilki, Ibrahim wrote:
> >>>>> +  aux-voltage-chargeable:
> >>>>> +    enum: [0, 1, 2]
> >>>>> +    description: |
> >>>>> +      Enables trickle charger.
> >>>>> +      0: Charger is disabled (default)
> >>>>> +      1: Charger is enabled
> >>>>> +      2: Charger is enabled with a diode
> >>>>
> >>>> 2 is not an allowed value. I asked to drop this property. It is coming
> >>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> >>>> is charging with, I don't know, FET?
> >>>
> >>> No, what is not explained here (and maybe not unsterstood by the
> >>> submitter) is that the RTC has an extra diode so, charging will always
> >>> enable a diode, select a resistor and then have or not an extra diode.
> >>> Figure2 of the MAX31329 datasheet is great.
> >>>
> >> 
> >> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
> >> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
> >> to "aux-voltage-chargeable", I am not sure how to add support for the extra
> >> diode at this point.
> >
> > Ask the person who asked you to remove adi,trickle-diode-enable...
> 
> That was the purpose.
> 

If the earlier submission was clearer my answer would have been
different but note how I had to dig up the datasheet to understand there
were two diodes. All the trickle chargers have a schottky diode so
"adi,trickle-diode-enable" nor the commit log were explicit about the
second diode (which is a regular diode).

aux-voltage-chargeable is enabling a diode on all the existing RTC
drivers so instead of trying to make me look like the bad guy you should
rather thank for taking the time trying to get better DT bindings.
  

On 04/04/2023 14:26:18+0200, Alexandre Belloni wrote:
> On 04/04/2023 10:35:53+0000, Tilki, Ibrahim wrote:
> > >>>>> +  aux-voltage-chargeable:
> > >>>>> +    enum: [0, 1, 2]
> > >>>>> +    description: |
> > >>>>> +      Enables trickle charger.
> > >>>>> +      0: Charger is disabled (default)
> > >>>>> +      1: Charger is enabled
> > >>>>> +      2: Charger is enabled with a diode
> > >>>>
> > >>>> 2 is not an allowed value. I asked to drop this property. It is coming
> > >>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> > >>>> is charging with, I don't know, FET?
> > >>>
> > >>> No, what is not explained here (and maybe not unsterstood by the
> > >>> submitter) is that the RTC has an extra diode so, charging will always
> > >>> enable a diode, select a resistor and then have or not an extra diode.
> > >>> Figure2 of the MAX31329 datasheet is great.
> > >>>
> > >> 
> > >> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
> > >> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
> > >> to "aux-voltage-chargeable", I am not sure how to add support for the extra
> > >> diode at this point.
> > >
> > > Ask the person who asked you to remove adi,trickle-diode-enable...
> > 
> > That was the purpose.
> > 
> 
> If the earlier submission was clearer my answer would have been
> different but note how I had to dig up the datasheet to understand there
> were two diodes. All the trickle chargers have a schottky diode so
> "adi,trickle-diode-enable" nor the commit log were explicit about the
> second diode (which is a regular diode).
> 
> aux-voltage-chargeable is enabling a diode on all the existing RTC
> drivers so instead of trying to make me look like the bad guy you should
> rather thank for taking the time trying to get better DT bindings.
> 

BTW, Krzysztof, you should focus more on how v5 of the driver is still
abusing the #clock-cells property to do pinmuxing after I repeatedly
explain to not do that.
  

On 04/04/2023 14:29, Alexandre Belloni wrote:
> On 04/04/2023 14:26:18+0200, Alexandre Belloni wrote:
>> On 04/04/2023 10:35:53+0000, Tilki, Ibrahim wrote:
>>>>>>>> +  aux-voltage-chargeable:
>>>>>>>> +    enum: [0, 1, 2]
>>>>>>>> +    description: |
>>>>>>>> +      Enables trickle charger.
>>>>>>>> +      0: Charger is disabled (default)
>>>>>>>> +      1: Charger is enabled
>>>>>>>> +      2: Charger is enabled with a diode
>>>>>>>
>>>>>>> 2 is not an allowed value. I asked to drop this property. It is coming
>>>>>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
>>>>>>> is charging with, I don't know, FET?
>>>>>>
>>>>>> No, what is not explained here (and maybe not unsterstood by the
>>>>>> submitter) is that the RTC has an extra diode so, charging will always
>>>>>> enable a diode, select a resistor and then have or not an extra diode.
>>>>>> Figure2 of the MAX31329 datasheet is great.
>>>>>>
>>>>>
>>>>> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
>>>>> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
>>>>> to "aux-voltage-chargeable", I am not sure how to add support for the extra
>>>>> diode at this point.
>>>>
>>>> Ask the person who asked you to remove adi,trickle-diode-enable...
>>>
>>> That was the purpose.
>>>
>>
>> If the earlier submission was clearer my answer would have been
>> different but note how I had to dig up the datasheet to understand there
>> were two diodes. All the trickle chargers have a schottky diode so
>> "adi,trickle-diode-enable" nor the commit log were explicit about the
>> second diode (which is a regular diode).
>>
>> aux-voltage-chargeable is enabling a diode on all the existing RTC
>> drivers so instead of trying to make me look like the bad guy you should
>> rather thank for taking the time trying to get better DT bindings.
>>
> 
> BTW, Krzysztof, you should focus more on how v5 of the driver is still
> abusing the #clock-cells property to do pinmuxing after I repeatedly
> explain to not do that.

Uh, I assumed #clock-cells in the binding is for its purpose - clocks. I
am rarely verifying driver implementation.

Best regards,
Krzysztof
  

On 03/04/2023 17:43, Ibrahim Tilki wrote:
> Devicetree binding documentation for Analog Devices MAX313XX RTCs
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> ---
>  .../devicetree/bindings/rtc/adi,max313xx.yaml | 144 ++++++++++++++++++
>  1 file changed, 144 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> new file mode 100644
> index 000000000..0c17a395e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/adi,max313xx.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/adi,max313xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX313XX series I2C RTCs
> +
> +maintainers:
> +  - Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
> +
> +description: Analog Devices MAX313XX series I2C RTCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,max31328
> +      - adi,max31329
> +      - adi,max31331
> +      - adi,max31334
> +      - adi,max31341
> +      - adi,max31342
> +      - adi,max31343
> +
> +  reg:
> +    description: I2C address of the RTC
> +    items:
> +      - enum: [0x68, 0x69]
> +
> +  interrupts:
> +    description: |
> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> +      configuration.
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    description: |
> +      RTC can be used as a clock source through its clock output pin when
> +      supplied.

This part is correct, but your implementation is not. I don't think you
can disable or enable interrupts, based on usage of clock. Either this
is clock (gated or not) or interrupt, not both.


Best regards,
Krzysztof
  

> >>>>>> +  aux-voltage-chargeable:
> >>>>>> +    enum: [0, 1, 2]
> >>>>>> +    description: |
> >>>>>> +      Enables trickle charger.
> >>>>>> +      0: Charger is disabled (default)
> >>>>>> +      1: Charger is enabled
> >>>>>> +      2: Charger is enabled with a diode
> >>>>>
> >>>>> 2 is not an allowed value. I asked to drop this property. It is coming
> >>>>> from rtc.yaml. I also do not understand "with a diode". So otherwise it
> >>>>> is charging with, I don't know, FET?
> >>>>
> >>>> No, what is not explained here (and maybe not unsterstood by the
> >>>> submitter) is that the RTC has an extra diode so, charging will always
> >>>> enable a diode, select a resistor and then have or not an extra diode.
> >>>> Figure2 of the MAX31329 datasheet is great.
> >>>>
> >>> 
> >>> That is exactly why I had "adi,trickle-diode-enable" property in previous patch.
> >>> So if I can't have "adi,trickle-diode-enable" and can't add an additional value
> >>> to "aux-voltage-chargeable", I am not sure how to add support for the extra
> >>> diode at this point.
> >>
> >> Ask the person who asked you to remove adi,trickle-diode-enable...
> > 
> > That was the purpose.
> > 
> 
> If the earlier submission was clearer my answer would have been
> different but note how I had to dig up the datasheet to understand there
> were two diodes. All the trickle chargers have a schottky diode so
> "adi,trickle-diode-enable" nor the commit log were explicit about the
> second diode (which is a regular diode).
> 
> aux-voltage-chargeable is enabling a diode on all the existing RTC
> drivers so instead of trying to make me look like the bad guy you should
> rather thank for taking the time trying to get better DT bindings.

With all due respect, I am not trying to make you look like the bad guy.
It wouldn't help me in any way. It is just that this is a non-verbal
communication channel, if anything. I didn't think "That was the purpose"
would sound rude here and my apoligies if it does.

I am just trying to understand your expectations and this was my interpretation
of your comment on v4. So, "adi,trickle-diode-enable" it is or should I change
the name? The datasheet doesn't say anything but "diode".

Best regards,
Ibrahim
  

> > +  interrupts:
> > +    description: |
> > +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
> > +      lines and alarm1 interrupt muxing depends on the clockin/clockout
> > +      configuration.
> > +    maxItems: 1
> > +
> > +  "#clock-cells":
> > +    description: |
> > +      RTC can be used as a clock source through its clock output pin when
> > +      supplied.
> 
> This part is correct, but your implementation is not. I don't think you
> can disable or enable interrupts, based on usage of clock. Either this
> is clock (gated or not) or interrupt, not both.
> 

The driver doesn't enable or disable interrupts based on clock usage. It checks
whether the IRQ is possible or not. Enablement of interrupt depends on the 
"interrupts" property. The tricky part is that interrupt muxing depends on
clock configuration.

Let me briefly explain the behavior of RTCs and the driver[v4]

MAX31328:
  - Has a single pin which is either used as IRQ or clock output.
  - Driver aborts probe with "-EOPNOTSUPP" when user requests irq and clockout
    at the same time. In other words, when both "interrupts" and "#clock-cells"
    properties are present. Otherwise, we are fine.

MAX31331:
MAX31334:
  - Has two pins: INTA and INTB/CLOCKOUT. INTA pin is dedicated for interrupt.
    INTB pin is used as either interrupt or clockout. The Alarm1 interrupt is
    muxed into INTB by default. If the CLOCKOUT is enabled, Alarm1 irq is muxed
    into INTA. We don't have further control over interrupt muxing.
  - Driver checks for "#clock-cells". If it is present, it enables the clockout
    so that we can get interrupt from INTA.

The Rest:
  - Has two pins: INTA/CLKIN and INTB/CLOCKOUT. Alarm1 interrupt is muxed into
    INTA by default, muxed into INTB if and only if we enable CLKIN.
  - Driver aborts probe with -EOPNOTSUPP when user requests interrupt, clockin
    and clockout at the same time. We can't have all three with two pins.
	

Unfortunately we don't have control over the interrupt muxing other than clock
configuration. How should the driver approach this?

Best regards,
Ibrahim
  

On 04/04/2023 17:40, Tilki, Ibrahim wrote:
>>> +  interrupts:
>>> +    description: |
>>> +      Alarm1 interrupt line of the RTC. Some of the RTCs have two interrupt
>>> +      lines and alarm1 interrupt muxing depends on the clockin/clockout
>>> +      configuration.
>>> +    maxItems: 1
>>> +
>>> +  "#clock-cells":
>>> +    description: |
>>> +      RTC can be used as a clock source through its clock output pin when
>>> +      supplied.
>>
>> This part is correct, but your implementation is not. I don't think you
>> can disable or enable interrupts, based on usage of clock. Either this
>> is clock (gated or not) or interrupt, not both.
>>
> 
> The driver doesn't enable or disable interrupts based on clock usage. It checks
> whether the IRQ is possible or not. Enablement of interrupt depends on the 
> "interrupts" property. The tricky part is that interrupt muxing depends on
> clock configuration.

I think it is not what your driver is doing. It checks if clock-cells...

> 
> Let me briefly explain the behavior of RTCs and the driver[v4]
> 
> MAX31328:
>   - Has a single pin which is either used as IRQ or clock output.
>   - Driver aborts probe with "-EOPNOTSUPP" when user requests irq and clockout
>     at the same time. In other words, when both "interrupts" and "#clock-cells"
>     properties are present. Otherwise, we are fine.

Why? These are fixed properties of the device. It is always a clock
controller and always has interrupt line. The choice between them is
depending on pins, so pin control.

> 
> MAX31331:
> MAX31334:
>   - Has two pins: INTA and INTB/CLOCKOUT. INTA pin is dedicated for interrupt.
>     INTB pin is used as either interrupt or clockout. The Alarm1 interrupt is
>     muxed into INTB by default. If the CLOCKOUT is enabled, Alarm1 irq is muxed
>     into INTA. We don't have further control over interrupt muxing.
>   - Driver checks for "#clock-cells". If it is present, it enables the clockout
>     so that we can get interrupt from INTA.
> 
> The Rest:
>   - Has two pins: INTA/CLKIN and INTB/CLOCKOUT. Alarm1 interrupt is muxed into
>     INTA by default, muxed into INTB if and only if we enable CLKIN.
>   - Driver aborts probe with -EOPNOTSUPP when user requests interrupt, clockin
>     and clockout at the same time. We can't have all three with two pins.
> 	
> 
> Unfortunately we don't have control over the interrupt muxing other than clock
> configuration. How should the driver approach this?

Pinmux with two states - interrupt or clock.

Best regards,
Krzysztof