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

[William Zhang <william.zhang@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]



On 01/26/2023 05:56 AM, Rob Herring wrote:
> On Wed, Jan 25, 2023 at 3:41 PM William Zhang
> <william.zhang@broadcom.com> 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 #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]