Re: [PATCH 1/2] devicetree: bindings: mux: reg-mux: Add support for new property 'mux-reg-masks-state'

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/06/2025 08:34, Chintan Vankar wrote:
> The DT binding for reg-mux currenly requires specifying register offset
> and masks in the "mux-reg-masks" property, while corresponding register
> values are defined in the "idle-states" property. This approach imposes a
> constraint where "mux-reg-masks" and "idle-states" must remain
> synchroniszed, adding complexity when configuring specific registers or a

Typo

> set of registers with large memory spaces.
> 
> Add support of a new property "mux-reg-masks-state" to remove this
> constraint, allowing offset, mask and value to be specified together as a
> tuple.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters


> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
> ---
>  .../devicetree/bindings/mux/reg-mux.yaml      | 32 +++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml b/Documentation/devicetree/bindings/mux/reg-mux.yaml
> index dc4be092fc2f..b029e2f28df0 100644
> --- a/Documentation/devicetree/bindings/mux/reg-mux.yaml
> +++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml
> @@ -32,11 +32,39 @@ 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

How one is supposed to use both (since this is extension)?

> +                 allows specifying register offset, mask and register
> +                 value to be set.

Anyway, this is getting too complex (next will be
mux-reg-masks-enable-state and mux-reg-masks-foo-state), just define
your own device schema.

> +
> +allOf:
> +  - oneOf:
> +      - required: [mux-reg-masks]
> +      - required: [mux-reg-masks-state]

So not an extension but replacement?

> +
> +  - if:
> +      required: [mux-reg-masks-state]
> +    then:
> +      not:
> +        required: [idle-states]

You cannot un-require a field. Where was it made required?

> +
> +  - if:
> +      required: [mux-reg-masks]
> +    then:
> +      properties:
> +        idle-states: true

This is no-op. I have no clue what you wanted to express here.


>  
>  required:
>    - compatible
> -  - mux-reg-masks
>    - '#mux-control-cells'
>  
>  additionalProperties: false


Best regards,
Krzysztof