Re: [PATCH v3] dt-bindings: drm/bridge: Update reg-name and reg description list for cdns,mhdp8546 compatible

[Harikrishna shenoy <h-shenoy@ti.com>]

On 23/11/25 15:30, Krzysztof Kozlowski wrote:
> On Fri, Nov 21, 2025 at 06:04:37PM +0530, Harikrishna Shenoy wrote:
>> Remove j721e-intg register name from reg-name list for cdns,mhdp8546
>> compatible. The j721e-integ registers are specific to TI SoCs, so they
>> are not required for compatibles other than ti,j721e-mhdp8546.
>>
>> Update reg and reg-names top level constraints with lists according
>> to compatibles.
>>
>> Move the register name constraints and reg description list to the
>> appropriate compatibility sections to ensure the correct register
>> names are used with each compatible value also adding the DSC register
>> to make bindings align with what the hardware supports.
>>
>> Fixes: 7169d082e7e6 ("dt-bindings: drm/bridge: MHDP8546 bridge binding changes for HDCP")
>> Signed-off-by: Harikrishna Shenoy <h-shenoy@ti.com>
>> ---
>>
>> Links to some discussions pointing to need for a fixes patch:
>> https://lore.kernel.org/all/20250903220312.GA2903503-robh@kernel.org/
>> https://lore.kernel.org/all/d2367789-6b54-4fc2-bb7c-609c0fe084d3@ti.com/
>>
>> Link to v2:
>> <https://lore.kernel.org/all/20251119122447.514729-1-h-shenoy@ti.com/>
>>
>> Changelog v2 --> v3:
>> -Add the reg description list and reg-name list in top level constraints
>> using oneOf for either of compatible.
>> Logs after testing some cases: https://gist.github.com/h-shenoy/a422f7278859cd95447e674963caabd9
>>
>> Link to v1:
>> <https://lore.kernel.org/all/20251107131535.1841393-1-h-shenoy@ti.com/>
>>
>> Changelog v1 --> v2:
>> -Update the reg description list for each compatible and add register space
>> for dsc to make the bindings reflect what hardware supports although
>> the driver doesn't support dsc yet.
>>
>> Note: j721e-integ are not optional registers for ti-compatible.
>>
>>   .../display/bridge/cdns,mhdp8546.yaml         | 85 ++++++++++++++-----
>>   1 file changed, 66 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
>> index c2b369456e4e2..632595ef32f63 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
>> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
>> @@ -17,23 +17,45 @@ properties:
>>         - ti,j721e-mhdp8546
>>   
>>     reg:
>> -    minItems: 1
>> -    items:
>> -      - description:
>> -          Register block of mhdptx apb registers up to PHY mapped area (AUX_CONFIG_P).
>> -          The AUX and PMA registers are not part of this range, they are instead
>> -          included in the associated PHY.
>> -      - description:
>> -          Register block for DSS_EDP0_INTG_CFG_VP registers in case of TI J7 SoCs.
>> -      - description:
>> -          Register block of mhdptx sapb registers.
>> +    oneOf:
>> +      - minItems: 2
>> +      - items:
> 
> This is wrong syntax. You created here a list, so you now allow
> anything with minItems 2.
Hi Krzysztof,

The list defined here restricts what lists are accepted, so for 
cdns,mhdp8546 compatible anything more than 3 items is rejected 
(example: 
https://gist.github.com/h-shenoy/a422f7278859cd95447e674963caabd9). 
Could you please help me with an
example where you think the bindings are incorrect?

> 
>> +          - description:
>> +              Register block of mhdptx apb registers up to PHY mapped area (AUX_CONFIG_P).
>> +              The AUX and PMA registers are not part of this range, they are instead
>> +              included in the associated PHY.
>> +          - description:
>> +              Register block for DSS_EDP0_INTG_CFG_VP registers in case of TI J7 SoCs.
>> +          - description:
>> +              Register block of mhdptx sapb registers.
>> +          - description:
>> +              Register block for mhdptx DSC encoder registers.
>> +
>> +      - minItems: 1
> 
> Actually anything with minItems 1... I asked for list of TWO, not FOUR,
> items. Or if syntax is getting to complicated, just min and maxItems.
> 
> 
>> +      - items:
>> +          - description:
>> +              Register block of mhdptx apb registers up to PHY mapped area (AUX_CONFIG_P).
>> +              The AUX and PMA registers are not part of this range, they are instead
>> +              included in the associated PHY.
>> +          - description:
>> +              Register block of mhdptx sapb registers.
>> +          - description:
>> +              Register block for mhdptx DSC encoder registers.
>>   
>>     reg-names:
>> -    minItems: 1
>> -    items:
>> -      - const: mhdptx
>> -      - const: j721e-intg
>> -      - const: mhdptx-sapb
>> +    oneOf:
>> +      - minItems: 2
>> +      - items:
> 
> Also wrong.
> 
>> +          - const: mhdptx
>> +          - const: j721e-intg
>> +          - const: mhdptx-sapb
>> +          - const: dsc
>> +
>> +      - minItems: 1
>> +      - items:
>> +          - const: mhdptx
>> +          - const: mhdptx-sapb
>> +          - const: dsc
>>   
>>     clocks:
>>       maxItems: 1
>> @@ -100,18 +122,43 @@ allOf:
>>         properties:
>>           reg:
>>             minItems: 2
>> -          maxItems: 3
> 
> Your commit msg says you "remove" but here you ADD one more item, thus
> growing this 3->4.
> 
> How remove can result in 3 becoming 4?
> 
Yes, remove is for j721e-intg for cdns,mhdp8546 compatible, and to make
bindings complete have added dsc reg-blocks, these changes reflects 
correct capabilities of hardware, have mentioned these in commit message 
as well.

Thanks.
> 
> Best regards,
> Krzysztof
>