Message-ID: <aWzMvqzh-QXWRTWP@anton.local>
Date: Sun, 18 Jan 2026 16:18:51 +0400
From: "Anton D. Stavinskii" <stavinsky@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Chen Wang <unicorn_wang@...look.com>, 
	Inochi Amaoto <inochiama@...il.com>, Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>, 
	Paul Walmsley <pjw@...nel.org>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, linux-sound@...r.kernel.org, 
	devicetree@...r.kernel.org, sophgo@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 1/7] dt-bindings: sound: sophgo: add CV1800B I2S/TDM
 controller binding

On Sun, Jan 18, 2026 at 11:14:34AM +0400, Krzysztof Kozlowski wrote:
> On Sun, Jan 18, 2026 at 12:18:53AM +0400, Anton D. Stavinskii wrote:
> > Purpose: introduce DT schema for the CPU driver
> 
> Bindings are for hardware, not drivers. Drop the purpose.

Noted. Will do in v3

> 
> > The driver uses dma to transfer data. The dma it self has 8 channels.
> 
> Describe the hardware.

Will try to rephrase. Thanks.

> 
> 
> > Each channel can be connected only to a specific i2s node. But each
> > of dma channel can have multiple purposes so in order to save dma
> > channels the configurations allows to use tx and rx, only rx, only tx
> > or none channels. I2S controller without channels can be useful in
> > configuration where I2S is used as clock source only and doesn't
> > produce any data.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with 'git log --oneline -- DIRECTORY_OR_FILE' on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

I will do my best in V3. And will read provided links again. 

> 
> A nit, subject: drop second/last, redundant "binding". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> > 
> > Signed-off-by: Anton D. Stavinskii <stavinsky@...il.com>
> > ---
> >  .../bindings/sound/sophgo,cv1800b-i2s.yaml         | 75 ++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/sophgo,cv1800b-i2s.yaml b/Documentation/devicetree/bindings/sound/sophgo,cv1800b-i2s.yaml
> > new file mode 100644
> > index 000000000000..cf30880a62da
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/sophgo,cv1800b-i2s.yaml
> > @@ -0,0 +1,75 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/sophgo,cv1800b-i2s.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sophgo CV1800B I2S/TDM controller
> > +
> > +maintainers:
> > +  - Anton D. Stavinskii <stavinsky@...il.com>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
noted. will be fixed. 

> 
> > +  I2S/TDM controller found in CV1800B / Sophgo SG2002/SG2000 SoCs.
> > +
> 
> Miss allOf with ref to dai-common.
> 
> > +properties:
> > +  compatible:
> > +    const: sophgo,cv1800b-i2s
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#sound-dai-cells":
> > +    const: 0
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 2
> 
> 1. Why is it flexible?

It's my mistake. 

> 
> 2. And then why names are not flexible. These should be synced in
> constraints.
> 
> > +
> > +  clock-names:
> > +    items:
> > +      - const: i2s
> > +      - const: mclk
> > +
> > +  dmas:
> > +    maxItems: 2
> > +
> > +  dma-names:
> > +    description: |
> > +      Names of DMA channels. May be omitted. If present, one entry
> > +      selects a single direction, while two entries select RX and TX.

Will drop.

> 
> Drop desription. Don't repeat constraints in free form text.
> 
> > +    minItems: 1
> > +    maxItems: 2
> 
> Again, messed constraints.
> 
> > +    items:
> > +      enum: [rx, tx]
> 
> No, it has to be a specific/fixed list.

Here is the question. Can you please help to understand how to describe
this properly. The idea is that TDM module is usable even without
specified dmas. Each TDM can work as clock source, only rx only tx or
both. I can force to use both channels but the user probably will want 
to not consume all the channels for if it is not needed. DMA can provide
channels for something else like SPI/I2C/UART etc. I'm asking because
I'm afraid I will do some mess again here.
> 
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> 
> Why? Drop these.

noted. Will remove. 

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - "#sound-dai-cells"
> > +
> > +additionalProperties: false
> 
> unevaluatedProperties instead

Thanks. 

> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/sophgo,cv1800.h>
> > +
> > +    i2s1: i2s@...0000 {
> 
> Drop unused label.

will be fixed. Thanks

> 
> > +        compatible = "sophgo,cv1800b-i2s";
> > +        reg = <0x04110000 0x10000>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> 
> Drop useless properties.
> 
> Best regards,
> Krzysztof
> 

Sorry for the mess. It is my first ever patch. Thank you for your
review and your time. Will do my best to improve the patch series. 

Powered by blists - more mailing lists