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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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