Re: [alsa-devel] [PATCH v5] dt-bindings: sound: Convert Samsung I2S controller to dt-schema

On 9/25/19 15:26, Marek Szyprowski wrote:

From: Maciej Falkowski m.falkowski@samsung.com

Convert Samsung I2S controller to newer dt-schema format.

Signed-off-by: Maciej Falkowski m.falkowski@samsung.com Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

---

Thanks Maciej, it looks good to me, I just think it might make sense to improve the comments a little while we are doing such a conversion. Please see my comments below.

With those corrections made: Reviewed-by: Sylwester Nawrocki s.nawrocki@samsung.com

.../devicetree/bindings/sound/samsung-i2s.txt | 84 ----------- .../bindings/sound/samsung-i2s.yaml | 136 ++++++++++++++++++ 2 files changed, 136 insertions(+), 84 deletions(-) delete mode 100644 Documentation/devicetree/bindings/sound/samsung-i2s.txt create mode 100644 Documentation/devicetree/bindings/sound/samsung-i2s.yaml

+++ b/Documentation/devicetree/bindings/sound/samsung-i2s.yaml @@ -0,0 +1,136 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: https://protect2.fireeye.com/url?k=9b0307ba8b0d1f39.9b028cf5-9870da798974f20... +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Samsung SoC I2S controller

+maintainers:

• • Krzysztof Kozlowski krzk@kernel.org
• • Sylwester Nawrocki s.nawrocki@samsung.com

+properties:

• compatible:
• description: |

•  samsung,s3c6410-i2s: for 8/16/24bit stereo I2S.
  

•  samsung,s5pv210-i2s: for 8/16/24bit multichannel(5.1) I2S with
  

space before "(5.1)" ?

•  secondary fifo, s/w reset control and internal mux for root clk src.
  

s/fifo/FIFO ? s/clk src/clock source ?

•  samsung,exynos5420-i2s: for 8/16/24bit multichannel(5.1) I2S for
  

space before "(5.1)" ?

•  playback, stereo channel capture, secondary fifo using internal
  

s/fifo/FIFO

•  or external dma, s/w reset control, internal mux for root clk src
  

s/dma/DMA ? s/clk src/clock source ?

•  and 7.1 channel TDM support for playback. TDM (Time division multiplexing)
  

•  is to allow transfer of multiple channel audio data on single data line.
  

•  samsung,exynos7-i2s: with all the available features of exynos5 i2s.
  

s/exynos5 i2s/ exynos5 I2S ?

•  exynos7 I2S has 7.1 channel TDM support for capture, secondary fifo
  

s/fifo/FIFO ?

•  with only external dma and more no.of root clk sampling frequencies.
  

s/dma/DMA ? s/no.of/number of ?

•  samsung,exynos7-i2s1: I2S1 on previous samsung platforms supports
  

•  stereo channels. exynos7 i2s1 upgraded to 5.1 multichannel with
  

s/i2s1/I2S1 ? It would be good to convert all i2s0, i2s1, i2s2 occurrences in comments/descriptions into either upper or lower case for consistency.

• clock-names:
• oneOf:

•  - items:
  

•      - const: iis
  

•  - items: # for i2s0
  

•      - const: iis
  

•      - const: i2s_opclk0
  

•      - const: i2s_opclk1
  

•  - items: # for i2s1 and i2s2
  

•      - const: iis
  

•      - const: i2s_opclk0
  

• description: |

•  "iis" is the i2s bus clock and i2s_opclk0, i2s_opclk1 are sources
  

•  of the root clk. i2s0 has internal mux to select the source
  

•  of root clk and i2s1 and i2s2 doesn't have any such mux.
  

• clock-output-names:
• deprecated: true
• oneOf:

•  - items: # for i2s0
  

•      - const: i2s_cdclk0
  

•  - items: # for i2s1> +          - const: i2s_cdclk1
  

•  - items: # for i2s2
  

•      - const: i2s_cdclk2
  

• description: Names of the CDCLK I2S output clocks.

• samsung,idma-addr:
• $ref: /schemas/types.yaml#/definitions/uint32
• description: |

•  Internal DMA register base address of the audio
  

•  sub system(used in secondary sound source).
  

s/sub system(used/subsystem (used ?