Re: [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property

["Vankar, Chintan" <c-vankar@ti.com>]

Hello Conor,

On 3/3/2025 10:28 PM, Conor Dooley wrote:
> On Sat, Mar 01, 2025 at 03:08:40AM +0530, Vankar, Chintan wrote:
>> Hello Conor, Andrew,
>>
>> On 3/1/2025 12:22 AM, Conor Dooley wrote:
>>> On Thu, Feb 27, 2025 at 03:26:31PM -0600, Andrew Davis wrote:
>>>> On 2/27/25 2:22 PM, Chintan Vankar wrote:
>>>>> DT-binding of reg-mux is defined in such a way that one need to provide
>>>>> register offset and mask in a "mux-reg-masks" property and corresponding
>>>>> register value in "idle-states" property. This constraint forces to define
>>>>> these values in such a way that "mux-reg-masks" and "idle-states" must be
>>>>> in sync with each other. This implementation would be more complex if
>>>>> specific register or set of registers need to be configured which has
>>>>> large memory space. Introduce a new property "mux-reg-masks-state" which
>>>>> allow to specify offset, mask and value as a tuple in a single property.
>>>>>
>>>>> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
>>>>> ---
>>>>>     .../devicetree/bindings/mux/reg-mux.yaml      | 29 +++++++++++++++++--
>>>>>     1 file changed, 27 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml b/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>>>> index dc4be092fc2f..a73c5efcf860 100644
>>>>> --- a/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>>>> +++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>>>> @@ -32,11 +32,36 @@ properties:
>>>>>             - description: pre-shifted bitfield mask
>>>>>         description: Each entry pair describes a single mux control.
>>>>> -  idle-states: true
>>>>> +  idle-states:
>>>>> +    description: Each entry describes mux register state.
>>>>> +
>>>>> +  mux-reg-masks-state:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>>>> +    items:
>>>>> +      items:
>>>>> +        - description: register offset
>>>>> +        - description: pre-shifted bitfield mask
>>>>> +        - description: register value to be set
>>>>> +    description: This property is an extension of mux-reg-masks which
>>>>> +                 allows specifying register offset, mask and register
>>>>> +                 value to be set in a single property.
>>>>> +
>>>>> +allOf:
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            enum:
>>>>> +              - reg-mux
>>>>> +              - mmio-mux
>>>>
>>>> These are the only two possible compatibles, is this "if" check needed?
>>>
>>> Aye.
>>>
>>>>> +    then:
>>>>> +      properties:
>>>>> +        mux-reg-masks: true
>>>>> +        mux-reg-masks-state: true
>>>>
>>>> You need one, but cannot have both, right? There should be some
>>>> way to describe that.
>>>>
>>>> Also an example added below would be good.
>>>
>>>   From the example schema:
>>> # if/then schema can be used to handle conditions on a property affecting
>>> # another property. A typical case is a specific 'compatible' value changes the
>>> # constraints on other properties.
>>> #
>>> # For multiple 'if' schema, group them under an 'allOf'.
>>> #
>>> # If the conditionals become too unweldy, then it may be better to just split
>>> # the binding into separate schema documents.
>>> allOf:
>>>     - if:
>>>         properties:
>>>           compatible:
>>>             contains:
>>>               const: vendor,soc2-ip
>>>       then:
>>>         required:
>>>           - foo-supply
>>>       else:
>>>         # If otherwise the property is not allowed:
>>>         properties:
>>>           foo-supply: false
>>>
>>> What's missing from here is making one of the properties required,
>>> so
>>> oneOf:
>>>     - required:
>>>         - masks
>>>     - required:
>>>         - masks-state
>>>
>>>>
>>>> Andrew
>>
>> Thanks for reviewing this patch.
>>
>> For the use-case we have following three rules to be followed:
>> 1. "mux-reg-masks" and "mux-reg-masks-state" should be mutually
>>     exclusive.
>> 2. "mux-reg-masks-state" and "idle-states" should also be mutually
>>     exclusive.
>> 3. If "mux-reg-masks" is present then "idle-states" might or might not
>>     be there.
>>
>> For the above conditions I have tried to write a binding as:
>>
>> allOf:
>>    - not:
>>        required: [mux-reg-masks, mux-reg-masks-state]
>>
>>    - if:
>>        required: [mux-reg-masks-state]
>>      then:
>>        not:
>>          required: [idle-states]
> 
> Why'd you pick two different syntax here?
> The normal syntax for mutual exclusion is:
> if:
>    required:
>      - foo
> then:
>    properties:
>      foobar: false
> 
> 
>>
> 
>>    - if:
>>        required: [mux-reg-masks]
>>      then:
>>        properties:
>>          idle-states:
>>            description: It can be present with mux-reg-masks, but not
>> required
> 
> This one here is the default, I don't think it needs an if/else.
> 

Thank you for reviewing the patch. I modified binding according to your
suggestions and that worked for all conditions mentioned above. I will
update it and post v2.


Regards,
Chintan.