Re: [PATCH v3 5/6] dt-bindings: display: sii902x: Add HDMI audio bindings

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jyri,

On Wed, Mar 13, 2019 at 07:52:08PM +0200, Jyri Sarha wrote:
> On 13/03/2019 18:47, Laurent Pinchart wrote:
> > On Wed, Mar 13, 2019 at 06:29:19PM +0200, Laurent Pinchart wrote:
> >> On Wed, Mar 13, 2019 at 06:01:07PM +0200, Jyri Sarha wrote:
> >>> The sii902x chip family supports also HDMI audio. Add binding for
> >>> describing the necessary i2s and mclk wiring for it.
> >>>
> >>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>> ---
> >>>  .../bindings/display/bridge/sii902x.txt       | 34 +++++++++++++++++++
> >>>  include/dt-bindings/sound/sii902x-audio.h     | 17 ++++++++++
> >>>  2 files changed, 51 insertions(+)
> >>>  create mode 100644 include/dt-bindings/sound/sii902x-audio.h
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >>> index c4c1855ca654..977756841193 100644
> >>> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >>> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> >>> @@ -8,6 +8,29 @@ Optional properties:
> >>>  	- interrupts: describe the interrupt line used to inform the host
> >>>  	  about hotplug events.
> >>>  	- reset-gpios: OF device-tree gpio specification for RST_N pin.
> >>> +	- sil,i2s-fifo-routing: Array of exactly 4 integers indicating i2s
> >>> +	  pins for audio fifo routing. First integer defines routing to
> >>> +	  fifo 0 and second to fifo 1, etc. Integers can be filled with
> >>> +	  definitions from: include/dt-bindings/sound/sii902x-audio.h
> >>> +	  The available definitions are:
> >>> +	  - ENABLE_BIT:	enable this audio fifo
> >>> +	  - CONNECT_SD#: route audio input from SD0, SD1, SD2, or SD3 i2s
> >>> +			 data input pin
> >>> +	  - LEFT_RIGHT_SWAP_BIT: swap i2s input channels for this fifo
> >>
> >> Are all combinations valid ? For instance, could we have D1 routed to
> >> the third FIFO, and all other FIFOs disabled ?
> > 
> > I found the answer to this question in the datasheet:
> > 
> > "Note that no gaps are allowed when mapping channels to FIFOs – SD pins
> > must be mapped to FIFO#0 and FIFO#1 before mapping a channel to FIFO#2,
> > and so on."
> > 
> > I think we can thus simplify the bindings, and use an approach similar
> > to the one taken by the data-lanes property for CSI-2. Furthermore, I
> > think this should be standardized, not left device-specific.
> > 
> > How about an sd-lanes property (better names are welcome) that would
> > store an array of N integers, where each sd-lanes[i] tells which SD pin
> > the i-th FIFO is connected to ?
> 
> I agree otherwise, but I would still rather use i2s than sd, because it
> is more explicit. SD overlaps with so many other acronyms. So how would
> i2s-lanes sound?

Sounds good to me. It's a better name, so it's welcome :-) I don't know
what terminology is usually used in the audio world for this, so I was
pretty sure my initial name proposal was bad.

Is there a risk of needing to describe the clock lane separately in the
future (for this or another I2S-related chip) ? If so, maybe
i2s-data-lanes, or just data-lanes, would be a better pick.

> >>> +	  I2S HDMI audio is configured only if this property is found.
> >>> +	- clocks: phandle mclk
> >>
> >> Maybe "clocks: phandle and clock specifier for each clock listed in
> >> the clock-names property" ?
> 
> Ok
> 
> >>> +	- clock-names: "mclk"
> >>> +	    Describes SII902x MCLK input. MCLK is used to produce
> >>> +	    HDMI audio CTS values. This property is required if
> >>> +	    "i2s-fifo-routing"-property is present. This property follows
> >>
> >> The property is named sil,i2s-fifo-routing.
> 
> Ok
> 
> >>> +	    Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>> +	    consumer binding.
> >>> +	- #sound-dai-cells = <0>: ASoC codec dai available for simple-card
> >>> +	    If audio properties are present sii902x provides an ASoC
> >>> +	    codec component driver that can be used by other ASoC
> >>> +	    components like simple-card. See binding document for
> >>> +	    details:
> >>> +	    Documentation/devicetree/bindings/sound/simple-card.txt
> >>>  
> >>>  Optional subnodes:
> >>>  	- video input: this subnode can contain a video input port node
> >>> @@ -21,6 +44,17 @@ Example:
> >>>  		compatible = "sil,sii9022";
> >>>  		reg = <0x39>;
> >>>  		reset-gpios = <&pioA 1 0>;
> >>> +
> >>> +		#sound-dai-cells = <0>;
> >>> +		sil,i2s-fifo-routing = <
> >>> +			(ENABLE_BIT|CONNECT_SD0)
> >>> +			0
> >>> +			0
> >>> +			0
> >>> +		>;
> >>> +		clocks = <&mclk>;
> >>> +		clock-names = "mclk";
> >>> +
> >>>  		ports {
> >>>  			#address-cells = <1>;
> >>>  			#size-cells = <0>;
> > 
> > I also forgot to mention that the audio connection should be modeled
> > using OF graph too.
> 
> With the audio configuration sii902x becomes an ASoC codec component.
> There are current more than one way to describe the connections between
> ASoC components (see [1] and [2] for details). The individual ASoC DAI
> drivers (digital audio interface) do not need to know about the audio
> system topology, that is taken care of by the machine driver (or card
> driver if you will), for instance simple-card or audio-graph-card.
> 
> However, since sii902x device is agnostic about the way the connections
> are described, I don't think I should go into too many details about how
> the possible bindings. However, I should add a reference at least to
> audio-graph-card too.
> 
> [1] Documentation/devicetree/bindings/sound/simple-card.txt
> [2] Documentation/devicetree/bindings/sound/audio-graph-card.txt

References to those bindings would be helpful, yes.

> >>> diff --git a/include/dt-bindings/sound/sii902x-audio.h b/include/dt-bindings/sound/sii902x-audio.h
> >>> new file mode 100644
> >>> index 000000000000..0a849904754b
> >>> --- /dev/null
> >>> +++ b/include/dt-bindings/sound/sii902x-audio.h
> >>> @@ -0,0 +1,17 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> >>> + * Author: Jyri Sarha <jsarha@ti.com>
> >>> + */
> >>> +
> >>> +#ifndef __DT_SII9022_AUDIO_H
> >>> +#define __DT_SII9022_AUDIO_H
> >>> +
> >>> +#define ENABLE_BIT			0x80
> >>> +#define CONNECT_SD0			0x00
> >>> +#define CONNECT_SD1			0x10
> >>> +#define CONNECT_SD2			0x20
> >>> +#define CONNECT_SD3			0x30
> >>> +#define LEFT_RIGHT_SWAP_BIT		0x04
> >>
> >> This is fairly generic, should you prefix the macros with SII9022 ?
> >>
> >>> +
> >>> +#endif /* __DT_SII9022_AUDIO_H */

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel