[v11,1/2] dt-bindings: spi: add loongson spi

Message ID 20230522071030.5193-2-zhuyinbo@loongson.cn
State New
Headers
Series spi: loongson: add bus driver for the loongson spi |

Commit Message

Yinbo Zhu May 22, 2023, 7:10 a.m. UTC
  Add the Loongson platform spi binding with DT schema format using
json-schema.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
  

Comments

Conor Dooley May 24, 2023, 8:56 a.m. UTC | #1
On Mon, May 22, 2023 at 03:10:29PM +0800, Yinbo Zhu wrote:
> Add the Loongson platform spi binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
> new file mode 100644
> index 000000000000..d0be6e5378d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson SPI controller
> +
> +maintainers:
> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - loongson,ls2k-spi

I am sorry to jump in here at such a late stage with a (potentially)
trivial question. "ls2k" is the SoC family rather than a specific model
as far as I understand.
The answer is probably yes, but do all SoCs in the family have an
identical version of the IP?

Cheers,
Conor.
  
Yinbo Zhu May 24, 2023, 9:44 a.m. UTC | #2
在 2023/5/24 下午4:56, Conor Dooley 写道:
> On Mon, May 22, 2023 at 03:10:29PM +0800, Yinbo Zhu wrote:
>> Add the Loongson platform spi binding with DT schema format using
>> json-schema.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>   .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
>>   MAINTAINERS                                   |  6 +++
>>   2 files changed, 47 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>> new file mode 100644
>> index 000000000000..d0be6e5378d7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>> @@ -0,0 +1,41 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Loongson SPI controller
>> +
>> +maintainers:
>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>> +
>> +allOf:
>> +  - $ref: /schemas/spi/spi-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - loongson,ls2k-spi
> 
> I am sorry to jump in here at such a late stage with a (potentially)
> trivial question. "ls2k" is the SoC family rather than a specific model
> as far as I understand.
> The answer is probably yes, but do all SoCs in the family have an
> identical version of the IP?


No, but the spi supported by this loongson spi driver are all the same
identical version, and other type or verion spi will be supported as
needed in the future.

Thanks.
  
Conor Dooley May 24, 2023, 10:29 a.m. UTC | #3
On Wed, May 24, 2023 at 05:44:38PM +0800, zhuyinbo wrote:
> 
> 
> 在 2023/5/24 下午4:56, Conor Dooley 写道:
> > On Mon, May 22, 2023 at 03:10:29PM +0800, Yinbo Zhu wrote:
> > > Add the Loongson platform spi binding with DT schema format using
> > > json-schema.
> > > 
> > > Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > ---
> > >   .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
> > >   MAINTAINERS                                   |  6 +++
> > >   2 files changed, 47 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
> > > new file mode 100644
> > > index 000000000000..d0be6e5378d7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
> > > @@ -0,0 +1,41 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Loongson SPI controller
> > > +
> > > +maintainers:
> > > +  - Yinbo Zhu <zhuyinbo@loongson.cn>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/spi/spi-controller.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - loongson,ls2k-spi
> > 
> > I am sorry to jump in here at such a late stage with a (potentially)
> > trivial question. "ls2k" is the SoC family rather than a specific model
> > as far as I understand.
> > The answer is probably yes, but do all SoCs in the family have an
> > identical version of the IP?
> 
> 
> No, but the spi supported by this loongson spi driver are all the same
> identical version, and other type or verion spi will be supported as
> needed in the future.

Does having a catch-all compatible make sense then when not all SoCs in
the ls2k family will actually be able to use this driver?
Or am I misunderstanding and all ls2k SoCs do work with this driver and
you were talking about other, future products?
  
Yinbo Zhu May 25, 2023, 2:22 a.m. UTC | #4
在 2023/5/24 下午6:29, Conor Dooley 写道:
> On Wed, May 24, 2023 at 05:44:38PM +0800, zhuyinbo wrote:
>>
>>
>> 在 2023/5/24 下午4:56, Conor Dooley 写道:
>>> On Mon, May 22, 2023 at 03:10:29PM +0800, Yinbo Zhu wrote:
>>>> Add the Loongson platform spi binding with DT schema format using
>>>> json-schema.
>>>>
>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> ---
>>>>    .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
>>>>    MAINTAINERS                                   |  6 +++
>>>>    2 files changed, 47 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>> new file mode 100644
>>>> index 000000000000..d0be6e5378d7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>> @@ -0,0 +1,41 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Loongson SPI controller
>>>> +
>>>> +maintainers:
>>>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>>>> +
>>>> +allOf:
>>>> +  - $ref: /schemas/spi/spi-controller.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - loongson,ls2k-spi
>>>
>>> I am sorry to jump in here at such a late stage with a (potentially)
>>> trivial question. "ls2k" is the SoC family rather than a specific model
>>> as far as I understand.
>>> The answer is probably yes, but do all SoCs in the family have an
>>> identical version of the IP?
>>
>>
>> No, but the spi supported by this loongson spi driver are all the same
>> identical version, and other type or verion spi will be supported as
>> needed in the future.
> 
> Does having a catch-all compatible make sense then when not all SoCs in
> the ls2k family will actually be able to use this driver?


Yes, it is make sense as it can reduce the workload of the community.
For the Loongson platform, the versions of spi peripherals are almost
the same, except for a few  or individual SoCs.  And we have also
discussed compatible internally, and we tend to define it this way.

> Or am I misunderstanding and all ls2k SoCs do work with this driver and
> you were talking about other, future products?

Actually, in 2k500 has one special type spi was only one cs and their's
register definition was different from common type spi thus this driver
doesn't support but this driver can support another common type spi in
2k500.  for this special type spi I will add support as needed in the
future.


Thanks,
Yinbo.
>
  
Krzysztof Kozlowski May 31, 2023, 7:46 p.m. UTC | #5
On 25/05/2023 04:22, zhuyinbo wrote:
> 
> 
> 在 2023/5/24 下午6:29, Conor Dooley 写道:
>> On Wed, May 24, 2023 at 05:44:38PM +0800, zhuyinbo wrote:
>>>
>>>
>>> 在 2023/5/24 下午4:56, Conor Dooley 写道:
>>>> On Mon, May 22, 2023 at 03:10:29PM +0800, Yinbo Zhu wrote:
>>>>> Add the Loongson platform spi binding with DT schema format using
>>>>> json-schema.
>>>>>
>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>> ---
>>>>>    .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
>>>>>    MAINTAINERS                                   |  6 +++
>>>>>    2 files changed, 47 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..d0be6e5378d7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>> @@ -0,0 +1,41 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Loongson SPI controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: /schemas/spi/spi-controller.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - loongson,ls2k-spi
>>>>
>>>> I am sorry to jump in here at such a late stage with a (potentially)
>>>> trivial question. "ls2k" is the SoC family rather than a specific model
>>>> as far as I understand.
>>>> The answer is probably yes, but do all SoCs in the family have an
>>>> identical version of the IP?
>>>
>>>
>>> No, but the spi supported by this loongson spi driver are all the same
>>> identical version, and other type or verion spi will be supported as
>>> needed in the future.
>>
>> Does having a catch-all compatible make sense then when not all SoCs in
>> the ls2k family will actually be able to use this driver?
> 
> 
> Yes, it is make sense as it can reduce the workload of the community.
> For the Loongson platform, the versions of spi peripherals are almost
> the same, except for a few  or individual SoCs.  And we have also
> discussed compatible internally, and we tend to define it this way.

So you have chosen different path than what's clearly recommended by
community, existing experience and documentation:

https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Family names are not accepted as specific compatibles. Whenever they
were accepted, it lead to problems. All the time.

https://lore.kernel.org/all/20220822181701.GA89665-robh@kernel.org/
https://lore.kernel.org/all/78651e07-6b3e-4243-8e1f-fcd1dfb3ffe1@www.fastmail.com/
https://lore.kernel.org/all/288f56ba9cfad46354203b7698babe91@walle.cc/
https://lore.kernel.org/all/106e443a-e765-51fe-b556-e4e7e2aa771c@linaro.org/
and many many more discussions.

You should choose carefully, because we will keep NAK-ing adding
properties to circumvent missing compatibles.
> 
>> Or am I misunderstanding and all ls2k SoCs do work with this driver and
>> you were talking about other, future products?
> 
> Actually, in 2k500 has one special type spi was only one cs and their's
> register definition was different from common type spi thus this driver
> doesn't support but this driver can support another common type spi in
> 2k500.  for this special type spi I will add support as needed in the
> future.

Bindings are for hardware, not driver. What does your driver support or
does not, matters less.

Best regards,
Krzysztof
  
Krzysztof Kozlowski May 31, 2023, 8:06 p.m. UTC | #6
On 25/05/2023 04:22, zhuyinbo wrote:
> 
> 
> 在 2023/5/24 下午6:29, Conor Dooley 写道:
>> On Wed, May 24, 2023 at 05:44:38PM +0800, zhuyinbo wrote:
>>>
>>>
>>> 在 2023/5/24 下午4:56, Conor Dooley 写道:
>>>> On Mon, May 22, 2023 at 03:10:29PM +0800, Yinbo Zhu wrote:
>>>>> Add the Loongson platform spi binding with DT schema format using
>>>>> json-schema.
>>>>>
>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>> ---
>>>>>    .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
>>>>>    MAINTAINERS                                   |  6 +++
>>>>>    2 files changed, 47 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..d0be6e5378d7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>> @@ -0,0 +1,41 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Loongson SPI controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: /schemas/spi/spi-controller.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - loongson,ls2k-spi
>>>>
>>>> I am sorry to jump in here at such a late stage with a (potentially)
>>>> trivial question. "ls2k" is the SoC family rather than a specific model
>>>> as far as I understand.
>>>> The answer is probably yes, but do all SoCs in the family have an
>>>> identical version of the IP?
>>>
>>>
>>> No, but the spi supported by this loongson spi driver are all the same
>>> identical version, and other type or verion spi will be supported as
>>> needed in the future.
>>
>> Does having a catch-all compatible make sense then when not all SoCs in
>> the ls2k family will actually be able to use this driver?
> 
> 
> Yes, it is make sense as it can reduce the workload of the community.

I missed it - that's a great comment. Hm, I don't know... Reviewing
Loongson patches is quite a work, so I don't see here reduced workload.

Please read existing guidelines:
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst

All of them.

Best regards,
Krzysztof
  
Yinbo Zhu June 1, 2023, 9:51 a.m. UTC | #7
在 2023/6/1 上午3:46, Krzysztof Kozlowski 写道:
> On 25/05/2023 04:22, zhuyinbo wrote:
>>
>>
>> 在 2023/5/24 下午6:29, Conor Dooley 写道:
>>> On Wed, May 24, 2023 at 05:44:38PM +0800, zhuyinbo wrote:
>>>>
>>>>
>>>> 在 2023/5/24 下午4:56, Conor Dooley 写道:
>>>>> On Mon, May 22, 2023 at 03:10:29PM +0800, Yinbo Zhu wrote:
>>>>>> Add the Loongson platform spi binding with DT schema format using
>>>>>> json-schema.
>>>>>>
>>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>> ---
>>>>>>     .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
>>>>>>     MAINTAINERS                                   |  6 +++
>>>>>>     2 files changed, 47 insertions(+)
>>>>>>     create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..d0be6e5378d7
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>>> @@ -0,0 +1,41 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Loongson SPI controller
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>>> +
>>>>>> +allOf:
>>>>>> +  - $ref: /schemas/spi/spi-controller.yaml#
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    enum:
>>>>>> +      - loongson,ls2k-spi
>>>>>
>>>>> I am sorry to jump in here at such a late stage with a (potentially)
>>>>> trivial question. "ls2k" is the SoC family rather than a specific model
>>>>> as far as I understand.
>>>>> The answer is probably yes, but do all SoCs in the family have an
>>>>> identical version of the IP?
>>>>
>>>>
>>>> No, but the spi supported by this loongson spi driver are all the same
>>>> identical version, and other type or verion spi will be supported as
>>>> needed in the future.
>>>
>>> Does having a catch-all compatible make sense then when not all SoCs in
>>> the ls2k family will actually be able to use this driver?
>>
>>
>> Yes, it is make sense as it can reduce the workload of the community.
>> For the Loongson platform, the versions of spi peripherals are almost
>> the same, except for a few  or individual SoCs.  And we have also
>> discussed compatible internally, and we tend to define it this way.
> 
> So you have chosen different path than what's clearly recommended by
> community, existing experience and documentation:
> 
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> Family names are not accepted as specific compatibles. Whenever they
> were accepted, it lead to problems. All the time.


Thank you for your documentation and advice and the Loongson platform
have loongson-2h (ls2h), loongson-2k (ls2k), loongson-2p (ls2p) or other
series SoC, which loongson-2 seems to be the family name you mentioned
and the "loongson,ls2k-spi" should be a speific compatible name.

> 
> https://lore.kernel.org/all/20220822181701.GA89665-robh@kernel.org/
> https://lore.kernel.org/all/78651e07-6b3e-4243-8e1f-fcd1dfb3ffe1@www.fastmail.com/
> https://lore.kernel.org/all/288f56ba9cfad46354203b7698babe91@walle.cc/
> https://lore.kernel.org/all/106e443a-e765-51fe-b556-e4e7e2aa771c@linaro.org/
> and many many more discussions.
> 
> You should choose carefully, because we will keep NAK-ing adding
> properties to circumvent missing compatibles.


I have read the documention and patch link that you mentioned and it
seems to advice that We don't have wildcard names in the compatible
string and use wildcard names that will cause issue. and the compatible
"loongson,ls2k-spi" that wasn't a wildcard names, and if the loongson-2k
spi controller hardware upgraded or changed the I will use
"loongson,ls2k-spi-version" as a compatible, such as,
"loongson,ls2k-spi-v1.1", "loongson,ls2k-spi-v1.1a" or other.

>>
>>> Or am I misunderstanding and all ls2k SoCs do work with this driver and
>>> you were talking about other, future products?
>>
>> Actually, in 2k500 has one special type spi was only one cs and their's
>> register definition was different from common type spi thus this driver
>> doesn't support but this driver can support another common type spi in
>> 2k500.  for this special type spi I will add support as needed in the
>> future.
> 
> Bindings are for hardware, not driver. What does your driver support or
> does not, matters less.


okay, I got it, and the loongson spi bindings was for loongson spi
controller hardware. if the spi controller hardware not changed in
different ls2k SoC and the spi compatible should be same thus loongson
spi compatible seems to be adhere to the bindings aggrement.

Thanks
  
Yinbo Zhu June 1, 2023, 11:38 a.m. UTC | #8
在 2023/6/1 上午4:06, Krzysztof Kozlowski 写道:
> On 25/05/2023 04:22, zhuyinbo wrote:
>>
>>
>> 在 2023/5/24 下午6:29, Conor Dooley 写道:
>>> On Wed, May 24, 2023 at 05:44:38PM +0800, zhuyinbo wrote:
>>>>
>>>>
>>>> 在 2023/5/24 下午4:56, Conor Dooley 写道:
>>>>> On Mon, May 22, 2023 at 03:10:29PM +0800, Yinbo Zhu wrote:
>>>>>> Add the Loongson platform spi binding with DT schema format using
>>>>>> json-schema.
>>>>>>
>>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>> ---
>>>>>>     .../bindings/spi/loongson,ls2k-spi.yaml       | 41 +++++++++++++++++++
>>>>>>     MAINTAINERS                                   |  6 +++
>>>>>>     2 files changed, 47 insertions(+)
>>>>>>     create mode 100644 Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..d0be6e5378d7
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
>>>>>> @@ -0,0 +1,41 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Loongson SPI controller
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
>>>>>> +
>>>>>> +allOf:
>>>>>> +  - $ref: /schemas/spi/spi-controller.yaml#
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    enum:
>>>>>> +      - loongson,ls2k-spi
>>>>>
>>>>> I am sorry to jump in here at such a late stage with a (potentially)
>>>>> trivial question. "ls2k" is the SoC family rather than a specific model
>>>>> as far as I understand.
>>>>> The answer is probably yes, but do all SoCs in the family have an
>>>>> identical version of the IP?
>>>>
>>>>
>>>> No, but the spi supported by this loongson spi driver are all the same
>>>> identical version, and other type or verion spi will be supported as
>>>> needed in the future.
>>>
>>> Does having a catch-all compatible make sense then when not all SoCs in
>>> the ls2k family will actually be able to use this driver?
>>
>>
>> Yes, it is make sense as it can reduce the workload of the community.
> 
> I missed it - that's a great comment. Hm, I don't know... Reviewing
> Loongson patches is quite a work, so I don't see here reduced workload.


If we do not consider the differences in SPI hardware and consider the
differences in SoC, it will result for each new a SoC adaptation, a new
compatible patch needs to be submitted to the community but spi hardware
was same and that will increase the workload of the community, It seems
to be more appropriate that use same compatible when spi hardware was
same and use different compatible when spi hardware was different.

> 
> Please read existing guidelines:
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst >
> All of them.
> 

okay, I got it.

Thanks.
  
Krzysztof Kozlowski June 1, 2023, 3:30 p.m. UTC | #9
On 01/06/2023 11:51, zhuyinbo wrote:
>>> Yes, it is make sense as it can reduce the workload of the community.
>>> For the Loongson platform, the versions of spi peripherals are almost
>>> the same, except for a few  or individual SoCs.  And we have also
>>> discussed compatible internally, and we tend to define it this way.
>>
>> So you have chosen different path than what's clearly recommended by
>> community, existing experience and documentation:
>>
>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>>
>> Family names are not accepted as specific compatibles. Whenever they
>> were accepted, it lead to problems. All the time.
> 
> 
> Thank you for your documentation and advice and the Loongson platform
> have loongson-2h (ls2h), loongson-2k (ls2k), loongson-2p (ls2p) or other
> series SoC, which loongson-2 seems to be the family name you mentioned
> and the "loongson,ls2k-spi" should be a speific compatible name.
> 
>>
>> https://lore.kernel.org/all/20220822181701.GA89665-robh@kernel.org/
>> https://lore.kernel.org/all/78651e07-6b3e-4243-8e1f-fcd1dfb3ffe1@www.fastmail.com/
>> https://lore.kernel.org/all/288f56ba9cfad46354203b7698babe91@walle.cc/
>> https://lore.kernel.org/all/106e443a-e765-51fe-b556-e4e7e2aa771c@linaro.org/
>> and many many more discussions.
>>
>> You should choose carefully, because we will keep NAK-ing adding
>> properties to circumvent missing compatibles.
> 
> 
> I have read the documention and patch link that you mentioned and it
> seems to advice that We don't have wildcard names in the compatible
> string and use wildcard names that will cause issue. and the compatible
> "loongson,ls2k-spi" that wasn't a wildcard names, and if the loongson-2k
> spi controller hardware upgraded or changed the I will use
> "loongson,ls2k-spi-version" as a compatible, such as,
> "loongson,ls2k-spi-v1.1", "loongson,ls2k-spi-v1.1a" or other.

Versions? Why? They received a lot of comments in the past, let me just
paste to avoid repeating the same:

https://lore.kernel.org/all/20220926231238.GA3132756-robh@kernel.org/

(and many more discussions on devicetree mailing list)

> 
>>>
>>>> Or am I misunderstanding and all ls2k SoCs do work with this driver and
>>>> you were talking about other, future products?
>>>
>>> Actually, in 2k500 has one special type spi was only one cs and their's
>>> register definition was different from common type spi thus this driver
>>> doesn't support but this driver can support another common type spi in
>>> 2k500.  for this special type spi I will add support as needed in the
>>> future.
>>
>> Bindings are for hardware, not driver. What does your driver support or
>> does not, matters less.
> 
> 
> okay, I got it, and the loongson spi bindings was for loongson spi
> controller hardware. if the spi controller hardware not changed in
> different ls2k SoC and the spi compatible should be same thus loongson
> spi compatible seems to be adhere to the bindings aggrement.

Specific compatible - yes. Unspecific - not, because you disregard the
clear message in the guideline.

Best regards,
Krzysztof
  
Yinbo Zhu June 2, 2023, 6:46 a.m. UTC | #10
在 2023/6/1 下午11:30, Krzysztof Kozlowski 写道:
> On 01/06/2023 11:51, zhuyinbo wrote:
>>>> Yes, it is make sense as it can reduce the workload of the community.
>>>> For the Loongson platform, the versions of spi peripherals are almost
>>>> the same, except for a few  or individual SoCs.  And we have also
>>>> discussed compatible internally, and we tend to define it this way.
>>>
>>> So you have chosen different path than what's clearly recommended by
>>> community, existing experience and documentation:
>>>
>>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>>>
>>> Family names are not accepted as specific compatibles. Whenever they
>>> were accepted, it lead to problems. All the time.
>>
>>
>> Thank you for your documentation and advice and the Loongson platform
>> have loongson-2h (ls2h), loongson-2k (ls2k), loongson-2p (ls2p) or other
>> series SoC, which loongson-2 seems to be the family name you mentioned
>> and the "loongson,ls2k-spi" should be a speific compatible name.
>>
>>>
>>> https://lore.kernel.org/all/20220822181701.GA89665-robh@kernel.org/
>>> https://lore.kernel.org/all/78651e07-6b3e-4243-8e1f-fcd1dfb3ffe1@www.fastmail.com/
>>> https://lore.kernel.org/all/288f56ba9cfad46354203b7698babe91@walle.cc/
>>> https://lore.kernel.org/all/106e443a-e765-51fe-b556-e4e7e2aa771c@linaro.org/
>>> and many many more discussions.
>>>
>>> You should choose carefully, because we will keep NAK-ing adding
>>> properties to circumvent missing compatibles.
>>
>>
>> I have read the documention and patch link that you mentioned and it
>> seems to advice that We don't have wildcard names in the compatible
>> string and use wildcard names that will cause issue. and the compatible
>> "loongson,ls2k-spi" that wasn't a wildcard names, and if the loongson-2k
>> spi controller hardware upgraded or changed the I will use
>> "loongson,ls2k-spi-version" as a compatible, such as,
>> "loongson,ls2k-spi-v1.1", "loongson,ls2k-spi-v1.1a" or other.
> 
> Versions? Why? They received a lot of comments in the past, let me just
> paste to avoid repeating the same:
> 
> https://lore.kernel.org/all/20220926231238.GA3132756-robh@kernel.org/
> 
> (and many more discussions on devicetree mailing list)
> 


I didn't notice the following words earlier about compatible in
documention and I will use "loongson,ls2k1000-spi" as ls2k1000 SoC spi
compatible, which is a very specific compatible.

"For sub-blocks/components of bigger device (e.g. SoC blocks) use rather
device-based compatible (e.g. SoC-based compatible), instead of custom
versioning of that component.For example use "vendor,soc1234-i2c"
instead of "vendor,i2c-v2".

>>
>>>>
>>>>> Or am I misunderstanding and all ls2k SoCs do work with this driver and
>>>>> you were talking about other, future products?
>>>>
>>>> Actually, in 2k500 has one special type spi was only one cs and their's
>>>> register definition was different from common type spi thus this driver
>>>> doesn't support but this driver can support another common type spi in
>>>> 2k500.  for this special type spi I will add support as needed in the
>>>> future.
>>>
>>> Bindings are for hardware, not driver. What does your driver support or
>>> does not, matters less.
>>
>>
>> okay, I got it, and the loongson spi bindings was for loongson spi
>> controller hardware. if the spi controller hardware not changed in
>> different ls2k SoC and the spi compatible should be same thus loongson
>> spi compatible seems to be adhere to the bindings aggrement.
> 
> Specific compatible - yes. Unspecific - not, because you disregard the
> clear message in the guideline.

okay, I got it.

Thanks.
  

Patch

diff --git a/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
new file mode 100644
index 000000000000..d0be6e5378d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
@@ -0,0 +1,41 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/loongson,ls2k-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson SPI controller
+
+maintainers:
+  - Yinbo Zhu <zhuyinbo@loongson.cn>
+
+allOf:
+  - $ref: /schemas/spi/spi-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - loongson,ls2k-spi
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi0: spi@1fff0220{
+        compatible = "loongson,ls2k-spi";
+        reg = <0x1fff0220 0x10>;
+        clocks = <&clk 17>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index fea4317faf75..e49c04c53c99 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12201,6 +12201,12 @@  F:	Documentation/devicetree/bindings/clock/loongson,ls2k-clk.yaml
 F:	drivers/clk/clk-loongson2.c
 F:	include/dt-bindings/clock/loongson,ls2k-clk.h
 
+LOONGSON SPI DRIVER
+M:	Yinbo Zhu <zhuyinbo@loongson.cn>
+L:	linux-spi@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
+
 LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
 M:	Sathya Prakash <sathya.prakash@broadcom.com>
 M:	Sreekanth Reddy <sreekanth.reddy@broadcom.com>