diff mbox series

[v2,07/10] dt-bindings: adv748x: add information about serial audio interface (I2S/TDM)

Message ID c9ff553f804f178a247dca356306948e971432fb.1584639664.git.alexander.riesen@cetitec.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series media: adv748x: add support for HDMI audio | expand

Commit Message

Alex Riesen March 19, 2020, 5:42 p.m. UTC
As the driver has some support for the audio interface of the device,
the bindings file should mention it.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 .../devicetree/bindings/media/i2c/adv748x.txt    | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart March 19, 2020, 6:01 p.m. UTC | #1
Hi Alex,

Thank you for the patch.

On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> As the driver has some support for the audio interface of the device,
> the bindings file should mention it.

While at it, how about converting the bindings to YAML ? :-) It can of
course be done on top.

> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> ---
>  .../devicetree/bindings/media/i2c/adv748x.txt    | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> index 4f91686e54a6..7d6db052c294 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> @@ -2,7 +2,9 @@
>  
>  The ADV7481 and ADV7482 are multi format video decoders with an integrated
>  HDMI receiver. They can output CSI-2 on two independent outputs TXA and TXB
> -from three input sources HDMI, analog and TTL.
> +from three input sources HDMI, analog and TTL. There is also support for an
> +I2S compatible interface connected to the audio processor of the HDMI decoder.

s/I2S compatible/I2S-compatible/ ?

> +The interface has TDM capability (8 slots, 32 bits, left or right justified).
>  
>  Required Properties:
>  
> @@ -16,6 +18,8 @@ Required Properties:
>      slave device on the I2C bus. The main address is mandatory, others are
>      optional and remain at default values if not specified.
>  
> +  - #clock-cells: must be <0> if the I2S port is used

Wouldn't it be simpler to set it to 0 unconditionally ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  Optional Properties:
>  
>    - interrupt-names: Should specify the interrupts as "intrq1", "intrq2" and/or
> @@ -47,6 +51,7 @@ are numbered as follows.
>  	  TTL		sink		9
>  	  TXA		source		10
>  	  TXB		source		11
> +	  I2S		source		12
>  
>  The digital output port nodes, when present, shall contain at least one
>  endpoint. Each of those endpoints shall contain the data-lanes property as
> @@ -72,6 +77,7 @@ Example:
>  
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> +		#clock-cells = <0>;
>  
>  		interrupt-parent = <&gpio6>;
>  		interrupt-names = "intrq1", "intrq2";
> @@ -113,4 +119,12 @@ Example:
>  				remote-endpoint = <&csi20_in>;
>  			};
>  		};
> +
> +		port@c {
> +			reg = <12>;
> +
> +			adv7482_i2s: endpoint {
> +				remote-endpoint = <&i2s_in>;
> +			};
> +		};
>  	};
Alex Riesen March 20, 2020, 8:44 a.m. UTC | #2
Hi Laurent,

Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > As the driver has some support for the audio interface of the device,
> > the bindings file should mention it.
> 
> While at it, how about converting the bindings to YAML ? :-) It can of
> course be done on top.

Of course. I shall take a look at that.

> >  The ADV7481 and ADV7482 are multi format video decoders with an integrated
> >  HDMI receiver. They can output CSI-2 on two independent outputs TXA and TXB
> > -from three input sources HDMI, analog and TTL.
> > +from three input sources HDMI, analog and TTL. There is also support for an
> > +I2S compatible interface connected to the audio processor of the HDMI decoder.
> 
> s/I2S compatible/I2S-compatible/ ?

Done.

> > +The interface has TDM capability (8 slots, 32 bits, left or right justified).
> >  
> >  Required Properties:
> >  
> > @@ -16,6 +18,8 @@ Required Properties:
> >      slave device on the I2C bus. The main address is mandatory, others are
> >      optional and remain at default values if not specified.
> >  
> > +  - #clock-cells: must be <0> if the I2S port is used
> 
> Wouldn't it be simpler to set it to 0 unconditionally ?

Would it? If the port itself is optional, shouldn't the clock be an option
too?

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

Regards,
Alex
Geert Uytterhoeven March 20, 2020, 8:48 a.m. UTC | #3
Hi Alex,

On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > As the driver has some support for the audio interface of the device,
> > > the bindings file should mention it.

> > > @@ -16,6 +18,8 @@ Required Properties:
> > >      slave device on the I2C bus. The main address is mandatory, others are
> > >      optional and remain at default values if not specified.
> > >
> > > +  - #clock-cells: must be <0> if the I2S port is used
> >
> > Wouldn't it be simpler to set it to 0 unconditionally ?
>
> Would it? If the port itself is optional, shouldn't the clock be an option
> too?

You'd be surprised how many board designers would consider this a cheap
12.288 MHz clock source, without using the I2S port ;-)

Gr{oetje,eeting}s,

                        Geert
Alex Riesen March 20, 2020, 9:03 a.m. UTC | #4
Hi Geert,

Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > > As the driver has some support for the audio interface of the device,
> > > > the bindings file should mention it.
> 
> > > > @@ -16,6 +18,8 @@ Required Properties:
> > > >      slave device on the I2C bus. The main address is mandatory, others are
> > > >      optional and remain at default values if not specified.
> > > >
> > > > +  - #clock-cells: must be <0> if the I2S port is used
> > >
> > > Wouldn't it be simpler to set it to 0 unconditionally ?
> >
> > Would it? If the port itself is optional, shouldn't the clock be an option
> > too?
> 
> You'd be surprised how many board designers would consider this a cheap
> 12.288 MHz clock source, without using the I2S port ;-)

Well, I am :-)

Especially considering that the driver will not switch the MCLK pin aktive
(all I2S-related pins are tristate by default).

And how do I require it to be set unconditionally? By just removing the
"if ..." part of the statement?

Regards,
Alex
Geert Uytterhoeven March 20, 2020, 9:15 a.m. UTC | #5
Hi Alex,

On Fri, Mar 20, 2020 at 10:03 AM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> > On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > > > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > > > As the driver has some support for the audio interface of the device,
> > > > > the bindings file should mention it.
> >
> > > > > @@ -16,6 +18,8 @@ Required Properties:
> > > > >      slave device on the I2C bus. The main address is mandatory, others are
> > > > >      optional and remain at default values if not specified.
> > > > >
> > > > > +  - #clock-cells: must be <0> if the I2S port is used
> > > >
> > > > Wouldn't it be simpler to set it to 0 unconditionally ?
> > >
> > > Would it? If the port itself is optional, shouldn't the clock be an option
> > > too?
> >
> > You'd be surprised how many board designers would consider this a cheap
> > 12.288 MHz clock source, without using the I2S port ;-)
>
> Well, I am :-)
>
> Especially considering that the driver will not switch the MCLK pin aktive
> (all I2S-related pins are tristate by default).

OK, didn't consider that.  But that still won't stop the hardware designer.
E.g. on Lager, the clock input of the PMIC is tied to the clock line of an SPI
bus, so to use that feature, the SPI clock must be kept running all the time,
not just when doing a transfer.

> And how do I require it to be set unconditionally? By just removing the
> "if ..." part of the statement?

Indeed.  This is still the plain text binding, not yaml.

Gr{oetje,eeting}s,

                        Geert
Alex Riesen March 20, 2020, 9:18 a.m. UTC | #6
Geert Uytterhoeven, Fri, Mar 20, 2020 10:15:17 +0100:
> On Fri, Mar 20, 2020 at 10:03 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> > > On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > > > > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > > > > As the driver has some support for the audio interface of the device,
> > > > > > the bindings file should mention it.
> > >
> > > > > > @@ -16,6 +18,8 @@ Required Properties:
> > > > > >      slave device on the I2C bus. The main address is mandatory, others are
> > > > > >      optional and remain at default values if not specified.
> > > > > >
> > > > > > +  - #clock-cells: must be <0> if the I2S port is used
> > > > >
> > > > > Wouldn't it be simpler to set it to 0 unconditionally ?
> > > >
> > > > Would it? If the port itself is optional, shouldn't the clock be an option
> > > > too?
> > >
> > > You'd be surprised how many board designers would consider this a cheap
> > > 12.288 MHz clock source, without using the I2S port ;-)
> >
> > Well, I am :-)
> >
> > Especially considering that the driver will not switch the MCLK pin aktive
> > (all I2S-related pins are tristate by default).
> 
> OK, didn't consider that.  But that still won't stop the hardware designer.
> E.g. on Lager, the clock input of the PMIC is tied to the clock line of an SPI
> bus, so to use that feature, the SPI clock must be kept running all the time,
> not just when doing a transfer.

Well... Maybe there is a convention to spell out the default state of the
clock lines?

> > And how do I require it to be set unconditionally? By just removing the
> > "if ..." part of the statement?
> 
> Indeed.  This is still the plain text binding, not yaml.

Conversion to YAML is on the list :)

Regards,
Alex
Laurent Pinchart March 20, 2020, 9:59 a.m. UTC | #7
Hi Alex,

On Fri, Mar 20, 2020 at 10:03:39AM +0100, Alex Riesen wrote:
> Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> > On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > > > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > > > As the driver has some support for the audio interface of the device,
> > > > > the bindings file should mention it.
> > > > >
> > > > > @@ -16,6 +18,8 @@ Required Properties:
> > > > >      slave device on the I2C bus. The main address is mandatory, others are
> > > > >      optional and remain at default values if not specified.
> > > > >
> > > > > +  - #clock-cells: must be <0> if the I2S port is used
> > > >
> > > > Wouldn't it be simpler to set it to 0 unconditionally ?
> > >
> > > Would it? If the port itself is optional, shouldn't the clock be an option
> > > too?
> > 
> > You'd be surprised how many board designers would consider this a cheap
> > 12.288 MHz clock source, without using the I2S port ;-)
> 
> Well, I am :-)
> 
> Especially considering that the driver will not switch the MCLK pin aktive
> (all I2S-related pins are tristate by default).

If the MCLK can't be output without enabling the I2S then I don't mind
if we make the #clock-cells optional, although, as Geert mentioned,
someone may still want to use it.

> And how do I require it to be set unconditionally? By just removing the
> "if ..." part of the statement?

Yes. For YAML it's easy too, the hard part is making properties
conditional :-)
Alex Riesen March 20, 2020, 4:15 p.m. UTC | #8
Hi Laurent,

Laurent Pinchart, Fri, Mar 20, 2020 10:59:07 +0100:
> On Fri, Mar 20, 2020 at 10:03:39AM +0100, Alex Riesen wrote:
> > Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> > > 
> > > You'd be surprised how many board designers would consider this a cheap
> > > 12.288 MHz clock source, without using the I2S port ;-)
> > 
> > Well, I am :-)
> > 
> > Especially considering that the driver will not switch the MCLK pin aktive
> > (all I2S-related pins are tristate by default).
> 
> If the MCLK can't be output without enabling the I2S then I don't mind
> if we make the #clock-cells optional, although, as Geert mentioned,
> someone may still want to use it.

So I settled on just removing the option.

> > And how do I require it to be set unconditionally? By just removing the
> > "if ..." part of the statement?
> 
> Yes. For YAML it's easy too, the hard part is making properties
> conditional :-)

Converting it into YAML turned out a bit more than just reformatting:
some of the explicit bindings schema is only implied in the text format :-(

Takes a while to find out what is what.

Regards,
Alex
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
index 4f91686e54a6..7d6db052c294 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
@@ -2,7 +2,9 @@ 
 
 The ADV7481 and ADV7482 are multi format video decoders with an integrated
 HDMI receiver. They can output CSI-2 on two independent outputs TXA and TXB
-from three input sources HDMI, analog and TTL.
+from three input sources HDMI, analog and TTL. There is also support for an
+I2S compatible interface connected to the audio processor of the HDMI decoder.
+The interface has TDM capability (8 slots, 32 bits, left or right justified).
 
 Required Properties:
 
@@ -16,6 +18,8 @@  Required Properties:
     slave device on the I2C bus. The main address is mandatory, others are
     optional and remain at default values if not specified.
 
+  - #clock-cells: must be <0> if the I2S port is used
+
 Optional Properties:
 
   - interrupt-names: Should specify the interrupts as "intrq1", "intrq2" and/or
@@ -47,6 +51,7 @@  are numbered as follows.
 	  TTL		sink		9
 	  TXA		source		10
 	  TXB		source		11
+	  I2S		source		12
 
 The digital output port nodes, when present, shall contain at least one
 endpoint. Each of those endpoints shall contain the data-lanes property as
@@ -72,6 +77,7 @@  Example:
 
 		#address-cells = <1>;
 		#size-cells = <0>;
+		#clock-cells = <0>;
 
 		interrupt-parent = <&gpio6>;
 		interrupt-names = "intrq1", "intrq2";
@@ -113,4 +119,12 @@  Example:
 				remote-endpoint = <&csi20_in>;
 			};
 		};
+
+		port@c {
+			reg = <12>;
+
+			adv7482_i2s: endpoint {
+				remote-endpoint = <&i2s_in>;
+			};
+		};
 	};