| Message ID | 20220805053019.996484-3-nagasuresh.relli@microchip.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Microchip QSPI controller | expand |
On 05/08/2022 07:30, Naga Sureshkumar Relli wrote: > Microchip's PolarFire SoC QSPI IP core is based on coreQSPI, > so add coreqspi as a fallback to mpfs-qspi. > > Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com> > --- Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 05/08/2022 07:49, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 05/08/2022 07:30, Naga Sureshkumar Relli wrote: >> diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml >> index a47d4923b51b..84d32c1a4d60 100644 >> --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml >> +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml >> @@ -18,10 +18,12 @@ allOf: >> >> properties: >> compatible: >> - enum: >> - - microchip,mpfs-spi >> - - microchip,mpfs-qspi >> - - microchip,coreqspi-rtl-v2 # FPGA QSPI >> + oneOf: >> + - items: >> + - const: microchip,mpfs-qspi >> + - const: microchip,coreqspi-rtl-v2 > > Eh, this does not make sense after looking at your driver... What is wrong with explicitly binding the driver to both of the compatible strings? The "hard" peripheral in the SoC part of the FPGA is a superset of version 2 of the coreQSPI IP so the fallback used in the binding here makes sense to me. coreQSPI can be instantiated in the FPGA fabric and used there, so it needs a compatible of its own. That brings me back to the original point question, why not explicitly bind the driver to both of the compatible strings it is known to work for? Thanks, Conor.
On 05/08/2022 09:12, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 05/08/2022 09:34, Conor.Dooley@microchip.com wrote: >> On 05/08/2022 07:49, Krzysztof Kozlowski wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On 05/08/2022 07:30, Naga Sureshkumar Relli wrote: >>>> diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml >>>> index a47d4923b51b..84d32c1a4d60 100644 >>>> --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml >>>> +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml >>>> @@ -18,10 +18,12 @@ allOf: >>>> >>>> properties: >>>> compatible: >>>> - enum: >>>> - - microchip,mpfs-spi >>>> - - microchip,mpfs-qspi >>>> - - microchip,coreqspi-rtl-v2 # FPGA QSPI >>>> + oneOf: >>>> + - items: >>>> + - const: microchip,mpfs-qspi >>>> + - const: microchip,coreqspi-rtl-v2 >>> >>> Eh, this does not make sense after looking at your driver... >> >> What is wrong with explicitly binding the driver to both of the >> compatible strings? The "hard" peripheral in the SoC part of the >> FPGA is a superset of version 2 of the coreQSPI IP so the fallback >> used in the binding here makes sense to me. coreQSPI can be >> instantiated in the FPGA fabric and used there, so it needs a >> compatible of its own. >> >> That brings me back to the original point question, why not >> explicitly bind the driver to both of the compatible strings it >> is known to work for? > > There is nothing particularly bad with matching to both of compatibles. > It is valid code. There are however questions/issues with that: > > 1. It is redundant. I did not look too much at the driver, but none of > the of_device_id entries have some driver data to differentiate, > therefore - for the driver - the devices are identical. If they are > identical and according to binding compatible, use less code and just > one compatible. Right. Then the binding is correct and the driver should only bind against "microchip,coreqspi-rtl-v2". > > 2. Otherwise, maybe the devices are not actually fully compatible? > That's the second problem. If one writes binding like that and codes it > in driver differently, it looks like it was not investigated really and > I ask questions... > > Best regards, > Krzysztof
On 05/08/2022 10:44, Conor.Dooley@microchip.com wrote: >> 1. It is redundant. I did not look too much at the driver, but none of >> the of_device_id entries have some driver data to differentiate, >> therefore - for the driver - the devices are identical. If they are >> identical and according to binding compatible, use less code and just >> one compatible. > > Right. Then the binding is correct and the driver should only bind > against "microchip,coreqspi-rtl-v2". Yes. Best regards, Krzysztof
On Fri, 05 Aug 2022 11:00:17 +0530, Naga Sureshkumar Relli wrote: > Microchip's PolarFire SoC QSPI IP core is based on coreQSPI, > so add coreqspi as a fallback to mpfs-qspi. > > Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com> > --- > .../devicetree/bindings/spi/microchip,mpfs-spi.yaml | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml:21:4: [warning] wrong indentation: expected 4 but found 3 (indentation) ./Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml:22:5: [warning] wrong indentation: expected 5 but found 4 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml index a47d4923b51b..84d32c1a4d60 100644 --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml @@ -18,10 +18,12 @@ allOf: properties: compatible: - enum: - - microchip,mpfs-spi - - microchip,mpfs-qspi - - microchip,coreqspi-rtl-v2 # FPGA QSPI + oneOf: + - items: + - const: microchip,mpfs-qspi + - const: microchip,coreqspi-rtl-v2 + - const: microchip,coreqspi-rtl-v2 #FPGA QSPI + - const: microchip,mpfs-spi reg: maxItems: 1
Microchip's PolarFire SoC QSPI IP core is based on coreQSPI, so add coreqspi as a fallback to mpfs-qspi. Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com> --- .../devicetree/bindings/spi/microchip,mpfs-spi.yaml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)