| Message ID | cover.1552492550.git.jsarha@ti.com |
|---|---|
| Headers | show |
| Series | drm/bridge: sii902x: HDMI-audio support and some fixes | expand |
Hello Jyri, Thank you for the patch. 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 ? > + 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>; > 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 */
On 13/03/2019 20:12, Laurent Pinchart wrote: > 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. > Usually (or always AFAIK) there is only one bit clock and one frame clock lane, and 1 - n data lanes. But still being more explicit does not hurt, let's make it i2s-data-lanes. Thanks for the prompt review. I'll try to make the changes for tomorrow. Best regards, Jyri
Hello Jyri, On 3/13/19 6:52 PM, Jyri Sarha wrote: > On 13/03/2019 18:47, Laurent Pinchart wrote: >> 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 ? > > 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? > >> >>>> + 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 > > My understanding is that of graph is now the preferred way to describe audio topology. So a think that it needs to be more detailed here. Moreover, the support of audio-graph-card requires a driver update. audio graph card parses endpoints to retrieve codec DAI id. For hdmi codec, get_dai_id() callback has to be implemented, as stated in snd_soc_get_dai_id(), to retrieve the right DAI id: * For example HDMI case, HDMI has video/sound port, * but ALSA SoC needs sound port number only. * Thus counting HDMI DT port/endpoint doesn't work. * Then, it should have .of_xlate_dai_id Regards Olivier >>>> 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 */ >> > >