Re: [PATCH v2 02/14] dt-bindings: spi: Add bcmbca-hsspi controller support

[William Zhang]

On 01/26/2023 05:56 AM, Rob Herring wrote:
> On Wed, Jan 25, 2023 at 3:41 PM William Zhang
> <william.zhang@xxxxxxxxxxxx> wrote:
> > On 01/25/2023 12:51 PM, Rob Herring wrote:
> > > On Wed, Jan 25, 2023 at 11:23:52AM -0800, William Zhang wrote:
> > > > On 01/24/2023 11:35 PM, Krzysztof Kozlowski wrote:
> > > > > On 24/01/2023 23:12, William Zhang wrote:
> > > > > > The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI
> > > > > > controller. Add new compatible strings to differentiate the old and new
> > > > > > controller while keeping MIPS based chip with the old compatible. Update
> > > > > > property requirements for these two revisions of the controller.  Also
> > > > > > add myself and Kursad as the maintainers.
>
> [...]
>
> > > > > >     properties:
> > > > > >       compatible:
> > > > > > -    const: brcm,bcm6328-hsspi
> > > > > > +    oneOf:
> > > > > > +      - const: brcm,bcm6328-hsspi
> > > > > > +      - items:
> > > > > > +          - enum:
> > > > > > +              - brcm,bcm47622-hsspi
> > > > > > +              - brcm,bcm4908-hsspi
> > > > > > +              - brcm,bcm63138-hsspi
> > > > > > +              - brcm,bcm63146-hsspi
> > > > > > +              - brcm,bcm63148-hsspi
> > > > > > +              - brcm,bcm63158-hsspi
> > > > > > +              - brcm,bcm63178-hsspi
> > > > > > +              - brcm,bcm6846-hsspi
> > > > > > +              - brcm,bcm6856-hsspi
> > > > > > +              - brcm,bcm6858-hsspi
> > > > > > +              - brcm,bcm6878-hsspi
> > > > > > +          - const: brcm,bcmbca-hsspi-v1.0
> > > > > > +          - const: brcm,bcmbca-hsspi
> > > > >
> > > > > Why do you need "brcm,bcmbca-hsspi"? Nothing binds to it, so it's
> > > > > useless and very generic.
> > > > This was from Florian's suggestion and Broadcom's convention. See [1] and
> > > > you are okay with that [2].  I added the rev compatible and you were not
> > > > objecting it finally if I understand you correctly.
> > >
> > > Can you have a driver that only understands what 'brcm,bcmbca-hsspi' is
> > > work on all h/w that includes the compatible string? It doesn't seem
> > > like it since v1.1 is a completely new driver. Therefore
> > > 'brcm,bcmbca-hsspi' is pretty much useless.
> > 'brcm,bcmbca-hsspi' should be added to the binding table of
> > spi-bcm63xx-hsspi.c driver.   This is the initial driver that works for
> > v1.0 controller.  For v1.1 controller, yes it can fallback and work with
> > 1.0 driver spi-bcm63xx-hsspi.c simply not using the new feature in
> > v1.1(chip select signal control through software) and keeping using the
> > prepend mode or dummy cs workaround supported in 1.0 driver.
>
> If v1.1 is compatible with v1.0, then say that:
>
> soc-compat, "brcm,bcmbca-hsspi-v1.1", "brcm,bcmbca-hsspi-v1.0"
>
> IOW, 'brcm,bcmbca-hsspi' is redundant with 'brcm,bcmbca-hsspi-v1.0'.
> They have the same meaning. So pick which one you want to use. Not
> both.
Agree brcm,bcmbca-hsspi fallback is redundant to brcm,bcmbca-hsspi-v1.0. 
I added it to conform Broadcom convention.  But I understand your and 
Krzystof's concern so I'll drop the brcm,bcmbca-hsspi in v3 to get 
things going.

> Also, if that is the case, you shouldn't be introducing a whole new
> driver for v1.1.
Technically I can combine the new feature to the existing driver v1.0 
but I prefer to start the a new driver so it will be much cleaner and 
simpler to follow without all the workarounds and complex logic.

> Rob

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature