| Message ID | 20200921102731.747736-1-peron.clem@gmail.com |
|---|---|
| Headers | show |
| Series | Add Allwinner H3/H5/H6/A64 HDMI audio | expand |
On Mon, Sep 21, 2020 at 12:27:11PM +0200, Clément Péron wrote: > As slots and slot_width can be overwritter in case set_tdm() is > called. Avoid to have this logic in set_chan_cfg(). > > Instead pass the required values as params to set_chan_cfg(). It's not really clear here what the issue is, and how passing the slots and slot_width as arguments addresses it > This also fix a bug when i2s->slot_width is set for TDM but not > properly used in set_chan_cfg(). Which bug? Also, Fixes tag? Thanks! Maxime
On Mon, Sep 21, 2020 at 12:27:12PM +0200, Clément Péron wrote: > We are actually using a complex formula to just return a bunch of > simple values. Also this formula is wrong for sun4i. Just like the previous patch, this could use a bit more explanation, like why it's a good thing, or how it's wrong on sun4i > Replace this with a simpler switch case. > > Also drop the i2s params not used. > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 69 +++++++++++++++++++++++-------------- > 1 file changed, 44 insertions(+), 25 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index 0633b9fba3d7..11bbcbe24d6b 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -175,8 +175,8 @@ struct sun4i_i2s_quirks { > unsigned int num_mclk_dividers; > > unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > - s8 (*get_sr)(const struct sun4i_i2s *, int); > - s8 (*get_wss)(const struct sun4i_i2s *, int); > + int (*get_sr)(unsigned int width); > + int (*get_wss)(unsigned int width); > int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > unsigned int channels, unsigned int slots, > unsigned int slot_width); > @@ -381,37 +381,56 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, > return 0; > } > > -static s8 sun4i_i2s_get_sr(const struct sun4i_i2s *i2s, int width) > +static int sun4i_i2s_get_sr(unsigned int width) > { > - if (width < 16 || width > 24) > - return -EINVAL; > - > - if (width % 4) > - return -EINVAL; > + switch (width) { > + case 16: > + return 0x0; > + case 20: > + return 0x1; > + case 24: > + return 0x2; > + } Why do we need an hex number here? Also, why is the return type change needed? Maxime
On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > From: Jernej Skrabec <jernej.skrabec@siol.net> > > Add a simple-soundcard to link audio between HDMI and I2S. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > index 28c77d6872f6..a8853ee7885a 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > @@ -67,6 +67,25 @@ de: display-engine { > status = "disabled"; > }; > > + hdmi_sound: hdmi-sound { > + compatible = "simple-audio-card"; > + simple-audio-card,format = "i2s"; > + simple-audio-card,name = "sun50i-h6-hdmi"; > + simple-audio-card,mclk-fs = <128>; > + simple-audio-card,frame-inversion; > + status = "disabled"; > + > + simple-audio-card,codec { > + sound-dai = <&hdmi>; > + }; > + > + simple-audio-card,cpu { > + sound-dai = <&i2s1>; > + dai-tdm-slot-num = <2>; > + dai-tdm-slot-width = <32>; It looks weird to have both some TDM setup here, and yet the format in i2s? Maxime
Hi Maxime, On Mon, 21 Sep 2020 at 14:29, Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Sep 21, 2020 at 12:27:11PM +0200, Clément Péron wrote: > > As slots and slot_width can be overwritter in case set_tdm() is > > called. Avoid to have this logic in set_chan_cfg(). > > > > Instead pass the required values as params to set_chan_cfg(). > > It's not really clear here what the issue is, and how passing the slots > and slot_width as arguments addresses it > > > This also fix a bug when i2s->slot_width is set for TDM but not > > properly used in set_chan_cfg(). > > Which bug? Do you mean my commit log is too short or is it a real question to understand ? To answer if set_tdm() is called then we set the i2s->slot_width and i2s->slots. But we use lrck_period = params_physical_width(params) instead of lrck_period = i2s->slot_width ? i2s->slot_width : params_physical_width(params); > > Also, Fixes tag? I think this only happens when 20/24bit is enabled so the issue has been introduced in this series. Clement > > Thanks! > Maxime
Hi Maxime, On Mon, 21 Sep 2020 at 15:55, Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Sep 21, 2020 at 12:27:12PM +0200, Clément Péron wrote: > > We are actually using a complex formula to just return a bunch of > > simple values. Also this formula is wrong for sun4i. > > Just like the previous patch, this could use a bit more explanation, > like why it's a good thing, or how it's wrong on sun4i Okay I will comment a bit more. > > > Replace this with a simpler switch case. > > > > Also drop the i2s params not used. > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > sound/soc/sunxi/sun4i-i2s.c | 69 +++++++++++++++++++++++-------------- > > 1 file changed, 44 insertions(+), 25 deletions(-) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index 0633b9fba3d7..11bbcbe24d6b 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > @@ -175,8 +175,8 @@ struct sun4i_i2s_quirks { > > unsigned int num_mclk_dividers; > > > > unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); > > - s8 (*get_sr)(const struct sun4i_i2s *, int); > > - s8 (*get_wss)(const struct sun4i_i2s *, int); > > + int (*get_sr)(unsigned int width); > > + int (*get_wss)(unsigned int width); > > int (*set_chan_cfg)(const struct sun4i_i2s *i2s, > > unsigned int channels, unsigned int slots, > > unsigned int slot_width); > > @@ -381,37 +381,56 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, > > return 0; > > } > > > > -static s8 sun4i_i2s_get_sr(const struct sun4i_i2s *i2s, int width) > > +static int sun4i_i2s_get_sr(unsigned int width) > > { > > - if (width < 16 || width > 24) > > - return -EINVAL; > > - > > - if (width % 4) > > - return -EINVAL; > > + switch (width) { > > + case 16: > > + return 0x0; > > + case 20: > > + return 0x1; > > + case 24: > > + return 0x2; > > + } > > Why do we need an hex number here? This is a register value, so I thought it's usually written using hexadecimal representation. > > Also, why is the return type change needed? This function returns a ERROR defined in errno.h which actually could be -133 but S8 only supports -128..127. There is no real reason to have a S8 here and doesn't give any optimisation. Clement > > Maxime
Hi Maxime, On Mon, 21 Sep 2020 at 15:59, Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > Add a simple-soundcard to link audio between HDMI and I2S. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > index 28c77d6872f6..a8853ee7885a 100644 > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > @@ -67,6 +67,25 @@ de: display-engine { > > status = "disabled"; > > }; > > > > + hdmi_sound: hdmi-sound { > > + compatible = "simple-audio-card"; > > + simple-audio-card,format = "i2s"; > > + simple-audio-card,name = "sun50i-h6-hdmi"; > > + simple-audio-card,mclk-fs = <128>; > > + simple-audio-card,frame-inversion; > > + status = "disabled"; > > + > > + simple-audio-card,codec { > > + sound-dai = <&hdmi>; > > + }; > > + > > + simple-audio-card,cpu { > > + sound-dai = <&i2s1>; > > + dai-tdm-slot-num = <2>; > > + dai-tdm-slot-width = <32>; > > It looks weird to have both some TDM setup here, and yet the format in > i2s? Yes, I agree I will check if it's really needed. Clement > > Maxime
On Mon, Sep 21, 2020 at 07:15:13PM +0200, Clément Péron wrote: > On Mon, 21 Sep 2020 at 14:29, Maxime Ripard <maxime@cerno.tech> wrote: > > Also, Fixes tag? > I think this only happens when 20/24bit is enabled so the issue has been > introduced in this series. For a situation like that a note in the changelog about "in preparation for adding X support which will make this matter" helps.
On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > From: Jernej Skrabec <jernej.skrabec@siol.net> > > Add a simple-soundcard to link audio between HDMI and I2S. It makes life a lot easier if you batch all the DTS changes together rather than randomly mixing them in with code changes, it both makes it clearer what's going on and makes things easier to handle.
Dne ponedeljek, 21. september 2020 ob 19:23:49 CEST je Clément Péron napisal(a): > Hi Maxime, > > On Mon, 21 Sep 2020 at 15:59, Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > Add a simple-soundcard to link audio between HDMI and I2S. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > --- > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > > > 1 file changed, 33 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/ boot/dts/allwinner/sun50i-h6.dtsi > > > index 28c77d6872f6..a8853ee7885a 100644 > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > @@ -67,6 +67,25 @@ de: display-engine { > > > status = "disabled"; > > > }; > > > > > > + hdmi_sound: hdmi-sound { > > > + compatible = "simple-audio-card"; > > > + simple-audio-card,format = "i2s"; > > > + simple-audio-card,name = "sun50i-h6-hdmi"; > > > + simple-audio-card,mclk-fs = <128>; > > > + simple-audio-card,frame-inversion; > > > + status = "disabled"; > > > + > > > + simple-audio-card,codec { > > > + sound-dai = <&hdmi>; > > > + }; > > > + > > > + simple-audio-card,cpu { > > > + sound-dai = <&i2s1>; > > > + dai-tdm-slot-num = <2>; > > > + dai-tdm-slot-width = <32>; > > > > It looks weird to have both some TDM setup here, and yet the format in > > i2s? > > Yes, I agree I will check if it's really needed. I think this was explained before. Anyway, this is needed to force width to 32, no matter actual sample width. That's a requirement of HDMI codec. I believe Marcus Cooper have another codec which also needs fixed width. There is no similar property for I2S, so TDM one is used here. Best regards, Jernej > > Clement > > > > > Maxime >
On Mon, Sep 21, 2020 at 08:37:09PM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 21. september 2020 ob 19:23:49 CEST je Clément Péron > napisal(a): > > Hi Maxime, > > > > On Mon, 21 Sep 2020 at 15:59, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > Add a simple-soundcard to link audio between HDMI and I2S. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > --- > > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > > > > 1 file changed, 33 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/ > boot/dts/allwinner/sun50i-h6.dtsi > > > > index 28c77d6872f6..a8853ee7885a 100644 > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > @@ -67,6 +67,25 @@ de: display-engine { > > > > status = "disabled"; > > > > }; > > > > > > > > + hdmi_sound: hdmi-sound { > > > > + compatible = "simple-audio-card"; > > > > + simple-audio-card,format = "i2s"; > > > > + simple-audio-card,name = "sun50i-h6-hdmi"; > > > > + simple-audio-card,mclk-fs = <128>; > > > > + simple-audio-card,frame-inversion; > > > > + status = "disabled"; > > > > + > > > > + simple-audio-card,codec { > > > > + sound-dai = <&hdmi>; > > > > + }; > > > > + > > > > + simple-audio-card,cpu { > > > > + sound-dai = <&i2s1>; > > > > + dai-tdm-slot-num = <2>; > > > > + dai-tdm-slot-width = <32>; > > > > > > It looks weird to have both some TDM setup here, and yet the format in > > > i2s? > > > > Yes, I agree I will check if it's really needed. > > I think this was explained before. Possibly, but this should be in a comment or at least the commit log > Anyway, this is needed to force width to 32, no matter actual sample > width. That's a requirement of HDMI codec. I believe Marcus Cooper > have another codec which also needs fixed width. > > There is no similar property for I2S, so TDM one is used here. Except it's really dedicated to the TDM mode and doesn't really make much sense here. If we have special requirements like this on the codec setup, that sounds like a good justification for creating a custom codec instead of shoehorning it into simple-card Maxime
On Mon, Sep 21, 2020 at 07:15:13PM +0200, Clément Péron wrote: > Hi Maxime, > > On Mon, 21 Sep 2020 at 14:29, Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Mon, Sep 21, 2020 at 12:27:11PM +0200, Clément Péron wrote: > > > As slots and slot_width can be overwritter in case set_tdm() is > > > called. Avoid to have this logic in set_chan_cfg(). > > > > > > Instead pass the required values as params to set_chan_cfg(). > > > > It's not really clear here what the issue is, and how passing the slots > > and slot_width as arguments addresses it > > > > > This also fix a bug when i2s->slot_width is set for TDM but not > > > properly used in set_chan_cfg(). > > > > Which bug? > > Do you mean my commit log is too short or is it a real question to understand ? Both, actually :) Maxime
Hi Maxime, On Mon, 28 Sep 2020 at 10:43, Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Sep 21, 2020 at 08:37:09PM +0200, Jernej Škrabec wrote: > > Dne ponedeljek, 21. september 2020 ob 19:23:49 CEST je Clément Péron > > napisal(a): > > > Hi Maxime, > > > > > > On Mon, 21 Sep 2020 at 15:59, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > Add a simple-soundcard to link audio between HDMI and I2S. > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > --- > > > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > > > > > 1 file changed, 33 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/ > > boot/dts/allwinner/sun50i-h6.dtsi > > > > > index 28c77d6872f6..a8853ee7885a 100644 > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > @@ -67,6 +67,25 @@ de: display-engine { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > + hdmi_sound: hdmi-sound { > > > > > + compatible = "simple-audio-card"; > > > > > + simple-audio-card,format = "i2s"; > > > > > + simple-audio-card,name = "sun50i-h6-hdmi"; > > > > > + simple-audio-card,mclk-fs = <128>; > > > > > + simple-audio-card,frame-inversion; > > > > > + status = "disabled"; > > > > > + > > > > > + simple-audio-card,codec { > > > > > + sound-dai = <&hdmi>; > > > > > + }; > > > > > + > > > > > + simple-audio-card,cpu { > > > > > + sound-dai = <&i2s1>; > > > > > + dai-tdm-slot-num = <2>; > > > > > + dai-tdm-slot-width = <32>; > > > > > > > > It looks weird to have both some TDM setup here, and yet the format in > > > > i2s? > > > > > > Yes, I agree I will check if it's really needed. > > > > I think this was explained before. > > Possibly, but this should be in a comment or at least the commit log > > > Anyway, this is needed to force width to 32, no matter actual sample > > width. That's a requirement of HDMI codec. I believe Marcus Cooper > > have another codec which also needs fixed width. > > > > There is no similar property for I2S, so TDM one is used here. > > Except it's really dedicated to the TDM mode and doesn't really make > much sense here. > > If we have special requirements like this on the codec setup, that > sounds like a good justification for creating a custom codec instead of > shoehorning it into simple-card When all the remarks are fixed would it be possible to merge the rest of the series without the dts changes ? I will propose another series to introduce a dedicated codec for that. > > Maxime
On Mon, Sep 28, 2020 at 04:27:42PM +0200, Clément Péron wrote: > On Mon, 28 Sep 2020 at 10:43, Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Mon, Sep 21, 2020 at 08:37:09PM +0200, Jernej Škrabec wrote: > > > Dne ponedeljek, 21. september 2020 ob 19:23:49 CEST je Clément Péron > > > napisal(a): > > > > Hi Maxime, > > > > > > > > On Mon, 21 Sep 2020 at 15:59, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > > > Add a simple-soundcard to link audio between HDMI and I2S. > > > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > > --- > > > > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > > > > > > 1 file changed, 33 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/ > > > boot/dts/allwinner/sun50i-h6.dtsi > > > > > > index 28c77d6872f6..a8853ee7885a 100644 > > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > > @@ -67,6 +67,25 @@ de: display-engine { > > > > > > status = "disabled"; > > > > > > }; > > > > > > > > > > > > + hdmi_sound: hdmi-sound { > > > > > > + compatible = "simple-audio-card"; > > > > > > + simple-audio-card,format = "i2s"; > > > > > > + simple-audio-card,name = "sun50i-h6-hdmi"; > > > > > > + simple-audio-card,mclk-fs = <128>; > > > > > > + simple-audio-card,frame-inversion; > > > > > > + status = "disabled"; > > > > > > + > > > > > > + simple-audio-card,codec { > > > > > > + sound-dai = <&hdmi>; > > > > > > + }; > > > > > > + > > > > > > + simple-audio-card,cpu { > > > > > > + sound-dai = <&i2s1>; > > > > > > + dai-tdm-slot-num = <2>; > > > > > > + dai-tdm-slot-width = <32>; > > > > > > > > > > It looks weird to have both some TDM setup here, and yet the format in > > > > > i2s? > > > > > > > > Yes, I agree I will check if it's really needed. > > > > > > I think this was explained before. > > > > Possibly, but this should be in a comment or at least the commit log > > > > > Anyway, this is needed to force width to 32, no matter actual sample > > > width. That's a requirement of HDMI codec. I believe Marcus Cooper > > > have another codec which also needs fixed width. > > > > > > There is no similar property for I2S, so TDM one is used here. > > > > Except it's really dedicated to the TDM mode and doesn't really make > > much sense here. > > > > If we have special requirements like this on the codec setup, that > > sounds like a good justification for creating a custom codec instead of > > shoehorning it into simple-card > > When all the remarks are fixed would it be possible to merge the rest > of the series without the dts changes ? > > I will propose another series to introduce a dedicated codec for that. Yeah, sure Maxime
Hi Maxime, On Wed, 30 Sep 2020 at 12:19, Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Sep 28, 2020 at 04:27:42PM +0200, Clément Péron wrote: > > On Mon, 28 Sep 2020 at 10:43, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Mon, Sep 21, 2020 at 08:37:09PM +0200, Jernej Škrabec wrote: > > > > Dne ponedeljek, 21. september 2020 ob 19:23:49 CEST je Clément Péron > > > > napisal(a): > > > > > Hi Maxime, > > > > > > > > > > On Mon, 21 Sep 2020 at 15:59, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > > > > > Add a simple-soundcard to link audio between HDMI and I2S. > > > > > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > > > --- > > > > > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > > > > > > > 1 file changed, 33 insertions(+) > > > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/ > > > > boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > index 28c77d6872f6..a8853ee7885a 100644 > > > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > @@ -67,6 +67,25 @@ de: display-engine { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > + hdmi_sound: hdmi-sound { > > > > > > > + compatible = "simple-audio-card"; > > > > > > > + simple-audio-card,format = "i2s"; > > > > > > > + simple-audio-card,name = "sun50i-h6-hdmi"; > > > > > > > + simple-audio-card,mclk-fs = <128>; > > > > > > > + simple-audio-card,frame-inversion; > > > > > > > + status = "disabled"; > > > > > > > + > > > > > > > + simple-audio-card,codec { > > > > > > > + sound-dai = <&hdmi>; > > > > > > > + }; > > > > > > > + > > > > > > > + simple-audio-card,cpu { > > > > > > > + sound-dai = <&i2s1>; > > > > > > > + dai-tdm-slot-num = <2>; > > > > > > > + dai-tdm-slot-width = <32>; > > > > > > > > > > > > It looks weird to have both some TDM setup here, and yet the format in > > > > > > i2s? I was looking at sound documentation regarding how I can properly write the multi-lane I2S support. And I think we made a wrong interpretation here. TDM slot-num and slot-width are not referencing the format called PCM or DSP_A / DSP_B. But really the physical time division representation of a format. For example Amlogic do the following representation for Multi-lane I2S: dai-link-7 { sound-dai = <&tdmif_b>; dai-format = "i2s"; dai-tdm-slot-tx-mask-0 = <1 1>; dai-tdm-slot-tx-mask-1 = <1 1>; dai-tdm-slot-tx-mask-2 = <1 1>; dai-tdm-slot-tx-mask-3 = <1 1>; mclk-fs = <256>; codec { sound-dai = <&tohdmitx TOHDMITX_I2S_IN_B>; }; }; So i think for 2 channels HDMI using the simple sound card with TDM property is not a hack but the correct way to represent it. Do you agree ? If so, can I resend the simple sound card for HDMI audio ? Thanks, Clement > > > > > > > > > > Yes, I agree I will check if it's really needed. > > > > > > > > I think this was explained before. > > > > > > Possibly, but this should be in a comment or at least the commit log > > > > > > > Anyway, this is needed to force width to 32, no matter actual sample > > > > width. That's a requirement of HDMI codec. I believe Marcus Cooper > > > > have another codec which also needs fixed width. > > > > > > > > There is no similar property for I2S, so TDM one is used here. > > > > > > Except it's really dedicated to the TDM mode and doesn't really make > > > much sense here. > > > > > > If we have special requirements like this on the codec setup, that > > > sounds like a good justification for creating a custom codec instead of > > > shoehorning it into simple-card > > > > When all the remarks are fixed would it be possible to merge the rest > > of the series without the dts changes ? > > > > I will propose another series to introduce a dedicated codec for that. > > Yeah, sure > > Maxime
On Sun, Nov 01, 2020 at 04:27:05PM +0100, Clément Péron wrote: > On Wed, 30 Sep 2020 at 12:19, Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Mon, Sep 28, 2020 at 04:27:42PM +0200, Clément Péron wrote: > > > On Mon, 28 Sep 2020 at 10:43, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Mon, Sep 21, 2020 at 08:37:09PM +0200, Jernej Škrabec wrote: > > > > > Dne ponedeljek, 21. september 2020 ob 19:23:49 CEST je Clément Péron > > > > > napisal(a): > > > > > > Hi Maxime, > > > > > > > > > > > > On Mon, 21 Sep 2020 at 15:59, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > > > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > > > > > > > Add a simple-soundcard to link audio between HDMI and I2S. > > > > > > > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > > > > --- > > > > > > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > > > > > > > > 1 file changed, 33 insertions(+) > > > > > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/ > > > > > boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > > index 28c77d6872f6..a8853ee7885a 100644 > > > > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > > @@ -67,6 +67,25 @@ de: display-engine { > > > > > > > > status = "disabled"; > > > > > > > > }; > > > > > > > > > > > > > > > > + hdmi_sound: hdmi-sound { > > > > > > > > + compatible = "simple-audio-card"; > > > > > > > > + simple-audio-card,format = "i2s"; > > > > > > > > + simple-audio-card,name = "sun50i-h6-hdmi"; > > > > > > > > + simple-audio-card,mclk-fs = <128>; > > > > > > > > + simple-audio-card,frame-inversion; > > > > > > > > + status = "disabled"; > > > > > > > > + > > > > > > > > + simple-audio-card,codec { > > > > > > > > + sound-dai = <&hdmi>; > > > > > > > > + }; > > > > > > > > + > > > > > > > > + simple-audio-card,cpu { > > > > > > > > + sound-dai = <&i2s1>; > > > > > > > > + dai-tdm-slot-num = <2>; > > > > > > > > + dai-tdm-slot-width = <32>; > > > > > > > > > > > > > > It looks weird to have both some TDM setup here, and yet the format in > > > > > > > i2s? > > > I was looking at sound documentation regarding how I can properly > write the multi-lane I2S support. > And I think we made a wrong interpretation here. > > TDM slot-num and slot-width are not referencing the format called PCM > or DSP_A / DSP_B. > But really the physical time division representation of a format. > > For example Amlogic do the following representation for Multi-lane I2S: > > dai-link-7 { > sound-dai = <&tdmif_b>; > dai-format = "i2s"; > dai-tdm-slot-tx-mask-0 = <1 1>; > dai-tdm-slot-tx-mask-1 = <1 1>; > dai-tdm-slot-tx-mask-2 = <1 1>; > dai-tdm-slot-tx-mask-3 = <1 1>; > mclk-fs = <256>; > > codec { > sound-dai = <&tohdmitx TOHDMITX_I2S_IN_B>; > }; > }; > > So i think for 2 channels HDMI using the simple sound card with TDM > property is not a hack but the correct way to represent it. > > Do you agree ? > > If so, can I resend the simple sound card for HDMI audio ? I mean, it's not less weird :) And like I said before we still have the option to write a card driver ourselves that doesn't take anything from the DT beside the phandle of the i2s controller and the HDMI controller. If it's a fixed configuration, I'm not sure why we bother trying to make it dynamic in the DT. Maxime
Hi Maxime, On Mon, 2 Nov 2020 at 11:21, Maxime Ripard <maxime@cerno.tech> wrote: > > On Sun, Nov 01, 2020 at 04:27:05PM +0100, Clément Péron wrote: > > On Wed, 30 Sep 2020 at 12:19, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Mon, Sep 28, 2020 at 04:27:42PM +0200, Clément Péron wrote: > > > > On Mon, 28 Sep 2020 at 10:43, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > On Mon, Sep 21, 2020 at 08:37:09PM +0200, Jernej Škrabec wrote: > > > > > > Dne ponedeljek, 21. september 2020 ob 19:23:49 CEST je Clément Péron > > > > > > napisal(a): > > > > > > > Hi Maxime, > > > > > > > > > > > > > > On Mon, 21 Sep 2020 at 15:59, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > > > On Mon, Sep 21, 2020 at 12:27:18PM +0200, Clément Péron wrote: > > > > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > > > > > > > > > Add a simple-soundcard to link audio between HDMI and I2S. > > > > > > > > > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > > > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > > > > > --- > > > > > > > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++ > > > > > > > > > 1 file changed, 33 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/ > > > > > > boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > > > index 28c77d6872f6..a8853ee7885a 100644 > > > > > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi > > > > > > > > > @@ -67,6 +67,25 @@ de: display-engine { > > > > > > > > > status = "disabled"; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > + hdmi_sound: hdmi-sound { > > > > > > > > > + compatible = "simple-audio-card"; > > > > > > > > > + simple-audio-card,format = "i2s"; > > > > > > > > > + simple-audio-card,name = "sun50i-h6-hdmi"; > > > > > > > > > + simple-audio-card,mclk-fs = <128>; > > > > > > > > > + simple-audio-card,frame-inversion; > > > > > > > > > + status = "disabled"; > > > > > > > > > + > > > > > > > > > + simple-audio-card,codec { > > > > > > > > > + sound-dai = <&hdmi>; > > > > > > > > > + }; > > > > > > > > > + > > > > > > > > > + simple-audio-card,cpu { > > > > > > > > > + sound-dai = <&i2s1>; > > > > > > > > > + dai-tdm-slot-num = <2>; > > > > > > > > > + dai-tdm-slot-width = <32>; > > > > > > > > > > > > > > > > It looks weird to have both some TDM setup here, and yet the format in > > > > > > > > i2s? > > > > > > I was looking at sound documentation regarding how I can properly > > write the multi-lane I2S support. > > And I think we made a wrong interpretation here. > > > > TDM slot-num and slot-width are not referencing the format called PCM > > or DSP_A / DSP_B. > > But really the physical time division representation of a format. > > > > For example Amlogic do the following representation for Multi-lane I2S: > > > > dai-link-7 { > > sound-dai = <&tdmif_b>; > > dai-format = "i2s"; > > dai-tdm-slot-tx-mask-0 = <1 1>; > > dai-tdm-slot-tx-mask-1 = <1 1>; > > dai-tdm-slot-tx-mask-2 = <1 1>; > > dai-tdm-slot-tx-mask-3 = <1 1>; > > mclk-fs = <256>; > > > > codec { > > sound-dai = <&tohdmitx TOHDMITX_I2S_IN_B>; > > }; > > }; > > > > So i think for 2 channels HDMI using the simple sound card with TDM > > property is not a hack but the correct way to represent it. > > > > Do you agree ? > > > > If so, can I resend the simple sound card for HDMI audio ? > > I mean, it's not less weird :) > > And like I said before we still have the option to write a card driver > ourselves that doesn't take anything from the DT beside the phandle of > the i2s controller and the HDMI controller. > > If it's a fixed configuration, I'm not sure why we bother trying to make > it dynamic in the DT. Ok I see what you mean here, as the link is hardcoded in the SoC it's a better representation to hardcode it in the sound card driver than having it dynamically represented in each board device-tree. Sounds correct for me, Thanks :) > > Maxime