| Message ID | cover.1551303673.git.jsarha@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/bridge: sii902x: HDMI-audio support and some fixes | expand |
Hi Jyri, On Wed, Feb 27, 2019 at 11:54:18PM +0200, Jyri Sarha wrote: > Changes since first version: > - Moved reviewed patches to front: > - drm/bridge: sii902x: add input_bus_flags > - drm/bridge: sii902x: Set output mode to HDMI or DVI according to EDID > - drm/bridge: sii902x: pixel clock unit is 10kHz instead of 1kHz > - Added a new fix: > - drm/bridge: sii902x: Select I2C_MUX > - Applied some review suggestions to > - drm/bridge: sii902x: Implement HDMI audio support > - use clock-names property to name mclk > - move comment describing added mutex to struct sii902x and improve it > - cleanup sii902x_mute() > - cleanup sii902x_select_mclk_div() > - fix condition for checking ENABLE_BIT from i2s_fifo_routing in > sii902x_audio_codec_init() > > Still to do > > - Agree on i2s wires to HDMI audio fifo routing in dts. > > The current scheme is quite straight forward, but there is maybe > there is even more straight forward solutions like: > > audio-fifo-enable = <1 1 1 1>; > audio-i2s-pin-to-fifo = <0 1 2 3>; > > Meaning that all fifos are enabled and SD0 is routed to fifo 0, SD1 > to fifo 1, etc. I am not sure if the channel swap functionality > should show in dts binding. Please forgive my lack of audio knowledge, but it this a system description that should be encoded in DT, or a policy that should be handled purely in software (either fully inside the kernel or with the help of userspace) ? > Jyri Sarha (4): > drm/bridge: sii902x: Set output mode to HDMI or DVI according to EDID > drm/bridge: sii902x: pixel clock unit is 10kHz instead of 1kHz > drm/bridge: sii902x: Select I2C_MUX > drm/bridge: sii902x: Implement HDMI audio support > > Tomi Valkeinen (1): > drm/bridge: sii902x: add input_bus_flags > > .../bindings/display/bridge/sii902x.txt | 36 +- > drivers/gpu/drm/bridge/Kconfig | 1 + > drivers/gpu/drm/bridge/sii902x.c | 472 +++++++++++++++++- > include/dt-bindings/sound/sii902x-audio.h | 11 + > 4 files changed, 512 insertions(+), 8 deletions(-) > create mode 100644 include/dt-bindings/sound/sii902x-audio.h
On 04/03/2019 14:42, Laurent Pinchart wrote: > Hi Jyri, > > On Wed, Feb 27, 2019 at 11:54:18PM +0200, Jyri Sarha wrote: >> Changes since first version: >> - Moved reviewed patches to front: >> - drm/bridge: sii902x: add input_bus_flags >> - drm/bridge: sii902x: Set output mode to HDMI or DVI according to EDID >> - drm/bridge: sii902x: pixel clock unit is 10kHz instead of 1kHz >> - Added a new fix: >> - drm/bridge: sii902x: Select I2C_MUX >> - Applied some review suggestions to >> - drm/bridge: sii902x: Implement HDMI audio support >> - use clock-names property to name mclk >> - move comment describing added mutex to struct sii902x and improve it >> - cleanup sii902x_mute() >> - cleanup sii902x_select_mclk_div() >> - fix condition for checking ENABLE_BIT from i2s_fifo_routing in >> sii902x_audio_codec_init() >> >> Still to do >> >> - Agree on i2s wires to HDMI audio fifo routing in dts. >> >> The current scheme is quite straight forward, but there is maybe >> there is even more straight forward solutions like: >> >> audio-fifo-enable = <1 1 1 1>; >> audio-i2s-pin-to-fifo = <0 1 2 3>; >> >> Meaning that all fifos are enabled and SD0 is routed to fifo 0, SD1 >> to fifo 1, etc. I am not sure if the channel swap functionality >> should show in dts binding. > Please forgive my lack of audio knowledge, but it this a system > description that should be encoded in DT, or a policy that should be > handled purely in software (either fully inside the kernel or with the > help of userspace) ? > This property describes how many i2s wires are connected to sii902x and in what order, so I think it belongs to DTS. One might of course wonder why anybody would put the i2s wires to any other order than 0 <-> 0, 1 <-> 1, 2 <-> 2, and 3 <-> 3, but then a again I've seen weirder board designs. Best regards, Jyri
Hi Jyri, On Mon, Mar 04, 2019 at 04:29:17PM +0200, Jyri Sarha wrote: > On 04/03/2019 14:42, Laurent Pinchart wrote: > > On Wed, Feb 27, 2019 at 11:54:18PM +0200, Jyri Sarha wrote: > >> Changes since first version: > >> - Moved reviewed patches to front: > >> - drm/bridge: sii902x: add input_bus_flags > >> - drm/bridge: sii902x: Set output mode to HDMI or DVI according to EDID > >> - drm/bridge: sii902x: pixel clock unit is 10kHz instead of 1kHz > >> - Added a new fix: > >> - drm/bridge: sii902x: Select I2C_MUX > >> - Applied some review suggestions to > >> - drm/bridge: sii902x: Implement HDMI audio support > >> - use clock-names property to name mclk > >> - move comment describing added mutex to struct sii902x and improve it > >> - cleanup sii902x_mute() > >> - cleanup sii902x_select_mclk_div() > >> - fix condition for checking ENABLE_BIT from i2s_fifo_routing in > >> sii902x_audio_codec_init() > >> > >> Still to do > >> > >> - Agree on i2s wires to HDMI audio fifo routing in dts. > >> > >> The current scheme is quite straight forward, but there is maybe > >> there is even more straight forward solutions like: > >> > >> audio-fifo-enable = <1 1 1 1>; > >> audio-i2s-pin-to-fifo = <0 1 2 3>; > >> > >> Meaning that all fifos are enabled and SD0 is routed to fifo 0, SD1 > >> to fifo 1, etc. I am not sure if the channel swap functionality > >> should show in dts binding. > > > > Please forgive my lack of audio knowledge, but it this a system > > description that should be encoded in DT, or a policy that should be > > handled purely in software (either fully inside the kernel or with the > > help of userspace) ? > > This property describes how many i2s wires are connected to sii902x and > in what order, so I think it belongs to DTS. That would belong to DT, yes. We have solved a similar problem with CSI-2 receivers, where the number of data lanes and their mapping can often be configured. See the definition of the data-lanes property in Documentation/devicetree/bindings/media/video-interfaces.txt for more information. How about using similar bindings ? It would also solve Andrzej's concerns that you don't describe the audio connection in DT. > One might of course wonder why anybody would put the i2s wires to any > other order than 0 <-> 0, 1 <-> 1, 2 <-> 2, and 3 <-> 3, but then a > again I've seen weirder board designs. It can be useful to simplify signal routing on the board.
Hi Jyri,
I also implemented HDMI audio support for sii902x to enable audio on a
STM32 board. As you submitted your patches first, I will align on it.
I had a first look at the current patch and I have some comments below.
I will review more in details and make some tests, asap.
I agree with Laurent and Andrzej regarding the missing audio connection
in DT. I would expect a subnode in DT describing the connection between
the HDMI codec and the CPU DAI.
Typically:
port@1 {
reg = <1>;
codec_endpoint: endpoint {
remote-endpoint = <&cpu_dai_endpoint>;
};
};
Then the hdmi codec get_dai_id callback can be implemented to check the
endpoint used for audio.
mclk and i2s-fifo-routing properties are defined as optional properties
in bindings.
However, these properties are required to initialize to the audio codec
in the code.
- master clock:
The i2s link of sii902x can be used without master clock. So the master
clock property has to be made actually optional. Probably, error code
-ENOENT should be checked on devm_clk_get() call, to ignore mclk if the
property is not defined.
- i2s-fifo-routing:
fifo mapping maybe set by default, according to i2s_fifo_defaults array
if the property is not set.
Best regards
Olivier
Changes since first version: - Moved reviewed patches to front: - drm/bridge: sii902x: add input_bus_flags - drm/bridge: sii902x: Set output mode to HDMI or DVI according to EDID - drm/bridge: sii902x: pixel clock unit is 10kHz instead of 1kHz - Added a new fix: - drm/bridge: sii902x: Select I2C_MUX - Applied some review suggestions to - drm/bridge: sii902x: Implement HDMI audio support - use clock-names property to name mclk - move comment describing added mutex to struct sii902x and improve it - cleanup sii902x_mute() - cleanup sii902x_select_mclk_div() - fix condition for checking ENABLE_BIT from i2s_fifo_routing in sii902x_audio_codec_init() Still to do - Agree on i2s wires to HDMI audio fifo routing in dts. The current scheme is quite straight forward, but there is maybe there is even more straight forward solutions like: audio-fifo-enable = <1 1 1 1>; audio-i2s-pin-to-fifo = <0 1 2 3>; Meaning that all fifos are enabled and SD0 is routed to fifo 0, SD1 to fifo 1, etc. I am not sure if the channel swap functionality should show in dts binding. Jyri Sarha (4): drm/bridge: sii902x: Set output mode to HDMI or DVI according to EDID drm/bridge: sii902x: pixel clock unit is 10kHz instead of 1kHz drm/bridge: sii902x: Select I2C_MUX drm/bridge: sii902x: Implement HDMI audio support Tomi Valkeinen (1): drm/bridge: sii902x: add input_bus_flags .../bindings/display/bridge/sii902x.txt | 36 +- drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/sii902x.c | 472 +++++++++++++++++- include/dt-bindings/sound/sii902x-audio.h | 11 + 4 files changed, 512 insertions(+), 8 deletions(-) create mode 100644 include/dt-bindings/sound/sii902x-audio.h