Re: [PATCH 2/3] dt-bindings: fpga: xlnx,fpga-slave-selectmap: add DT schema

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

On 30/01/2024 18:05, Charles Perry wrote:
> 
> 
> ----- On Jan 30, 2024, at 11:05 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
> 
>> On 30/01/2024 16:45, Charles Perry wrote:
>>>
>>>>> +
>>>>> +  reg:
>>>>> +    description:
>>>>> +      At least 1 byte of memory mapped IO
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  prog_b-gpios:
>>>>
>>>>
>>>> No underscores in names.
>>>>
>>>
>>> This is heavily based on "xlnx,fpga-slave-serial.yaml" which uses an underscore.
>>> I can use a dash instead but that would make things inconsistent across the two
>>> schemas.
>>
>> Inconsistency is not a problem. Duplicating technical debt is.
>>
>>>
>>>>
>>>>> +    description:
>>>>> +      config pin (referred to as PROGRAM_B in the manual)
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  done-gpios:
>>>>> +    description:
>>>>> +      config status pin (referred to as DONE in the manual)
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  init-b-gpios:
>>>>
>>>> Is there init-a? Open other bindings and look how these are called there.
>>>>
>>>
>>> No, the "-b" is there to denote that the signal is active low. I think its
>>> shorthand
>>> for "bar" which is the overline (‾) that electronic engineer put on top of the
>>> name of the
>>> signal on schematics. It comes from the datasheet.
>>
>> Then just "init-gpios"
>>
>> ...
>>
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - reg
>>>>> +  - prog_b-gpios
>>>>> +  - done-gpios
>>>>> +  - init-b-gpios
>>>>> +
>>>>> +additionalProperties: true
>>>>
>>>> Nope, this cannot bue true.
>>>>
>>>
>>> Ok, I'll put this to false but I'm not quite sure I understand the implications.
>>>
>>> My reasoning behind assigning this to true was that the FPGA is an external
>>> device on a bus that needs to be configured by a bus controller. The bus
>>> controller
>>> would be the parent of the fpga DT node and the later would contain properties
>>> parsed by the bus controller driver.
>>
>> Which bus controller? MMIO bus does not parse children properties.
>> Anyway, if that's the case you miss $ref to respective
>> peripheral-props.yaml matching your bus and then "unevaluatedProperties:
>> false".
> 
> This one: https://elixir.bootlin.com/linux/v6.8-rc2/source/Documentation/devicetree/bindings/bus/imx-weim.txt#L56

Eh, ok, so after fast check WEIM looks like some memory interface bus,
so the bus bindings should be moved to memory-controllers and converted
to YAML. Then you add child node properties to own schema and reference
in mc-peripheral-props, which is then referenced in your binding here,
as I mentioned.

Best regards,
Krzysztof