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

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

Hello again,

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 ?

> > +	  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" ?
> 
> > +	- 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.
> 
> > +	    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.

> > 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