Re: [PATCH 3/4] dt-bindings: PCI: microchip,pcie-host: fix incorrect child node name

[Conor.Dooley]

On 12/08/2022 08:42, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 11/08/2022 23:33, Conor Dooley wrote:
>> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>
>> v2022.08 of dt-schema improved checking of unevaluatedProperties, and
>> exposed a previously unseen warning for the PCIe controller's interrupt
>> controller node name:
>>
>> arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dtb: pcie@2000000000: Unevaluated properties are not allowed ('clock-names', 'clocks', 'legacy-interrupt-controller', 'microchip,axi-m-atr0' were unexpected)
>>          From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>>
>> Make the property in the binding match the node name actually used in
>> the dts.
>>
>> Fixes: dcd49679fb3a ("dt-bindings: PCI: Fix 'unevaluatedProperties' warnings")
>> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>> ---
>> This is another one Rob where I feel like I'm doing the wrong thing.
>> The Linux driver gets the child node without using the name, but
>> another OS etc could in theory (or reality), right?
> 
> Yes and we had such cases when renaming device nodes caused regression.
> My interpretation is that node name is not part of ABI, so anyone
> depending on it made a mistake and they need to fix their stuff. I think
> actually that is really poor coding and poor solution to parse device
> node names and expect specific name.
> 
> Other folks interpretation is that we never break the users of kernel,
> regardless what is documented in the ABI... so it depends. :)
> 
> Here however it is not a device node name, but a property name (although
> still a node). Bindings require these to be specific, thus such name is
> a part of ABI.

Yup, pretty much aligned to my thoughts on this.

> For your case, I wonder why it was called "legacy-interrupt-controller"
> in the first place? Node names - also for properties - should be
> generic, so generic name is just "interrupt-controller".

I don't know. It's what we had in our internal tree prior to upstreaming.
"We" don't rely on the name for the Linux driver, so I am not really that
bothered if we change the binding or the dts.

> 
>> ---
>>   .../devicetree/bindings/pci/microchip,pcie-host.yaml          | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> index 2a2166f09e2c..9b123bcd034c 100644
>> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> @@ -71,7 +71,7 @@ properties:
>>     msi-parent:
>>       description: MSI controller the device is capable of using.
>>
>> -  interrupt-controller:
>> +  legacy-interrupt-controller:
> 
> 
> Best regards,
> Krzysztof
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv