Re: [PATCH 1/2] spi: dt-binding: add Microchip CoreQSPI compatible

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

On 03/08/2022 15:29, Mark Brown wrote:
> On Wed, Aug 03, 2022 at 08:11:03AM +0200, Krzysztof Kozlowski wrote:
>> On 02/08/2022 15:13, Mark Brown wrote:
>>> On Tue, Aug 02, 2022 at 10:52:25AM +0200, Krzysztof Kozlowski wrote:
>>>> On 01/08/2022 11:42, Naga Sureshkumar Relli wrote:
> 
>>>>> +    oneOf:
>>>>> +      - description: Microchip's Polarfire SoC SPI controller.
>>>>> +        const: microchip,mpfs-spi
>>>>> +      - description: Microchip's Polarfire SoC QSPI controller.
> 
>>>> Useless descriptions - they repeat compatible. Just keep it as enum and
>>>> skip descriptions. What value do they bring?
> 
>>> Someone not familiar with the full Microchip product line might not be
>>> aware of the expansion of mpfs, it's not blindingly obvious.
> 
>> Then it should be explained in title/description of the binding, not in
>> compatible. This is the usual way of providing some text description,
>> not for each compatible by repeating the compatible text.
> 
> I'm not convinced this is a useful rule to try to enforce, and I'm not
> sure how well it will work if the same IP is used in several different
> places.  It's not clear to me what the benefit is intended to be.

First, the description here is really not adding any useful information.

"description: Microchip's Polarfire SoC SPI controller."
Microchip - already in comaptible
SPI controller - already in compatible and in device description

The only useful piece could be extending pfs to Polarfire SoC.

And now imagine every binding doing the same, adding such
acronym-explanations in every compatible list. Basically we loose easy
to read, compare, analyze and check for errors enum:
  enum
    - microchip,mpfs-spi
    - microchip,mpfs-qspi
    - microchip,coreqspi-rtl-v2
    - microchip,mpfs-some-more-spi
    - microchip,mpfs-even-newer-spi

into double-sized oneOf with additional descriptions each one explaining
"mpfs".

  oneOf:
    - description: Microchip's Polarfire SoC SPI controller.
      const: microchip,mpfs-spi
    - description: Microchip's Polarfire SoC QSPI controller.
      const: microchip,mpfs-qspi
    - description: Microchip's FPGA QSPI controller.
      const: microchip,coreqspi-rtl-v2
    - description: Microchip's Polarfire SoC some-more SPI controller.
      const: microchip,mpfs-some-more-spi
    - description: Microchip's Polarfire SoC even newer SPI controller.
      const: microchip,mpfs-even-newer-spi

Why do you need to explain "mpfs" more than once? Why explaining these
are SPI or QSPI controllers? It's obvious from compatible.

Just keep it simple and small. We all have too much code to look at...

Best regards,
Krzysztof