| 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 |
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>; > + }; > + }; > };
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
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
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
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
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
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 :-)
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 --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>; + }; + }; };