| Message ID | E1gxILh-0004MG-EZ@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tda998x updates for DAI formats and bclk_ratio | expand |
On 22/02/2019 23:27, Russell King wrote: > Add support for the left and right justified I2S formats as well as the > more tranditional "Philips" I2S format. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> I do not have a spec to check REG_I2S_FORMAT bits, but at least philips and left justified formats work on BBB (McASP does not support right justified format). With the above conditions: Reviewed-by: Jyri Sarha <jsarha@ti.com> Tested-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 57 ++++++++++++++++++++++++++------------- > include/drm/i2c/tda998x.h | 11 +++++--- > 2 files changed, 47 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index a7c39f39793f..645d884fb9e8 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -242,7 +242,9 @@ struct tda998x_priv { > # define HVF_CNTRL_1_SEMI_PLANAR (1 << 6) > #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */ > #define REG_I2S_FORMAT REG(0x00, 0xfc) /* read/write */ > -# define I2S_FORMAT(x) (((x) & 3) << 0) > +# define I2S_FORMAT_PHILIPS (0 << 0) > +# define I2S_FORMAT_LEFT_J (2 << 0) > +# define I2S_FORMAT_RIGHT_J (3 << 0) > #define REG_AIP_CLKSEL REG(0x00, 0xfd) /* write */ > # define AIP_CLKSEL_AIP_SPDIF (0 << 3) > # define AIP_CLKSEL_AIP_I2S (1 << 3) > @@ -872,14 +874,14 @@ static int > tda998x_configure_audio(struct tda998x_priv *priv, > struct tda998x_audio_params *params) > { > - u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv; > + u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv, i2s_fmt; > u32 n; > > /* Enable audio ports */ > reg_write(priv, REG_ENA_AP, params->config); > > /* Set audio input source */ > - switch (params->format) { > + switch (params->format & AFMT_MASK) { > case AFMT_SPDIF: > reg_write(priv, REG_ENA_ACLK, 0); > reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF); > @@ -907,6 +909,19 @@ tda998x_configure_audio(struct tda998x_priv *priv, > cts_n = CTS_N_M(3) | CTS_N_K(3); > break; > } > + > + switch (params->format & AFMT_I2S_MASK) { > + case AFMT_I2S_LEFT_J: > + i2s_fmt = I2S_FORMAT_LEFT_J; > + break; > + case AFMT_I2S_RIGHT_J: > + i2s_fmt = I2S_FORMAT_RIGHT_J; > + break; > + default: > + i2s_fmt = I2S_FORMAT_PHILIPS; > + break; > + } > + reg_write(priv, REG_I2S_FORMAT, i2s_fmt); > break; > > default: > @@ -992,23 +1007,15 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, > > switch (daifmt->fmt) { > case HDMI_I2S: > - if (daifmt->bit_clk_inv || daifmt->frame_clk_inv || > - daifmt->bit_clk_master || daifmt->frame_clk_master) { > - dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, > - daifmt->bit_clk_inv, daifmt->frame_clk_inv, > - daifmt->bit_clk_master, > - daifmt->frame_clk_master); > - return -EINVAL; > - } > - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) > - if (priv->audio_port[i].format == AFMT_I2S) > - audio.config = priv->audio_port[i].config; > - audio.format = AFMT_I2S; > + audio.format = AFMT_I2S | AFMT_I2S_PHILIPS; > + break; > + case HDMI_LEFT_J: > + audio.format = AFMT_I2S | AFMT_I2S_LEFT_J; > + break; > + case HDMI_RIGHT_J: > + audio.format = AFMT_I2S | AFMT_I2S_RIGHT_J; > break; > case HDMI_SPDIF: > - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) > - if (priv->audio_port[i].format == AFMT_SPDIF) > - audio.config = priv->audio_port[i].config; > audio.format = AFMT_SPDIF; > break; > default: > @@ -1016,11 +1023,25 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, > return -EINVAL; > } > > + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) > + if (priv->audio_port[i].format == (audio.format & AFMT_MASK)) > + audio.config = priv->audio_port[i].config; > + > if (audio.config == 0) { > dev_err(dev, "%s: No audio configuration found\n", __func__); > return -EINVAL; > } > > + if ((audio.format & AFMT_MASK) == HDMI_I2S && > + (daifmt->bit_clk_inv || daifmt->frame_clk_inv || > + daifmt->bit_clk_master || daifmt->frame_clk_master)) { > + dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, > + daifmt->bit_clk_inv, daifmt->frame_clk_inv, > + daifmt->bit_clk_master, > + daifmt->frame_clk_master); > + return -EINVAL; > + } > + > mutex_lock(&priv->audio_mutex); > if (priv->supports_infoframes && priv->sink_has_audio) > ret = tda998x_configure_audio(priv, &audio); > diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h > index 3cb25ccbe5e6..b0864f0be017 100644 > --- a/include/drm/i2c/tda998x.h > +++ b/include/drm/i2c/tda998x.h > @@ -6,9 +6,14 @@ > #include <dt-bindings/display/tda998x.h> > > enum { > - AFMT_UNUSED = 0, > - AFMT_SPDIF = TDA998x_SPDIF, > - AFMT_I2S = TDA998x_I2S, > + AFMT_UNUSED = 0, > + AFMT_SPDIF = TDA998x_SPDIF, > + AFMT_I2S = TDA998x_I2S, > + AFMT_MASK = AFMT_SPDIF | AFMT_I2S, > + AFMT_I2S_PHILIPS = 0 << 4, > + AFMT_I2S_LEFT_J = 1 << 4, > + AFMT_I2S_RIGHT_J = 2 << 4, > + AFMT_I2S_MASK = 3 << 4, > }; > > struct tda998x_audio_params { >
hi Russell, On 22/02/2019 23.27, Russell King wrote: > Add support for the left and right justified I2S formats as well as the > more tranditional "Philips" I2S format. First of all, thank you for the patch, it works. Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com> There is however one thing I'm not sure about. the 3.8 kernel configured the page0:0xfc register [1]: /* select I2S format, and datasize */ reg_write(encoder, REG_I2S_FORMAT, 0x0a); In theory this should select left_j and set bit3 which does something. It looks like that the McASP is configured to I2S mode in 3.8 kernel which would result channel swap at least there (I2S vs left_j). Do you know what the bit3 is configuring and to what? [1] https://github.com/beagleboard/linux/blob/1f2f3402a6e4b8c90148c0ca2fd3acba91738eb3/drivers/gpu/drm/i2c/tda998x_drv.c#L851 > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 57 ++++++++++++++++++++++++++------------- > include/drm/i2c/tda998x.h | 11 +++++--- > 2 files changed, 47 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index a7c39f39793f..645d884fb9e8 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -242,7 +242,9 @@ struct tda998x_priv { > # define HVF_CNTRL_1_SEMI_PLANAR (1 << 6) > #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */ > #define REG_I2S_FORMAT REG(0x00, 0xfc) /* read/write */ > -# define I2S_FORMAT(x) (((x) & 3) << 0) > +# define I2S_FORMAT_PHILIPS (0 << 0) > +# define I2S_FORMAT_LEFT_J (2 << 0) > +# define I2S_FORMAT_RIGHT_J (3 << 0) > #define REG_AIP_CLKSEL REG(0x00, 0xfd) /* write */ > # define AIP_CLKSEL_AIP_SPDIF (0 << 3) > # define AIP_CLKSEL_AIP_I2S (1 << 3) > @@ -872,14 +874,14 @@ static int > tda998x_configure_audio(struct tda998x_priv *priv, > struct tda998x_audio_params *params) > { > - u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv; > + u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv, i2s_fmt; > u32 n; > > /* Enable audio ports */ > reg_write(priv, REG_ENA_AP, params->config); > > /* Set audio input source */ > - switch (params->format) { > + switch (params->format & AFMT_MASK) { > case AFMT_SPDIF: > reg_write(priv, REG_ENA_ACLK, 0); > reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF); > @@ -907,6 +909,19 @@ tda998x_configure_audio(struct tda998x_priv *priv, > cts_n = CTS_N_M(3) | CTS_N_K(3); > break; > } > + > + switch (params->format & AFMT_I2S_MASK) { > + case AFMT_I2S_LEFT_J: > + i2s_fmt = I2S_FORMAT_LEFT_J; > + break; > + case AFMT_I2S_RIGHT_J: > + i2s_fmt = I2S_FORMAT_RIGHT_J; > + break; > + default: > + i2s_fmt = I2S_FORMAT_PHILIPS; > + break; > + } > + reg_write(priv, REG_I2S_FORMAT, i2s_fmt); > break; > > default: > @@ -992,23 +1007,15 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, > > switch (daifmt->fmt) { > case HDMI_I2S: > - if (daifmt->bit_clk_inv || daifmt->frame_clk_inv || > - daifmt->bit_clk_master || daifmt->frame_clk_master) { > - dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, > - daifmt->bit_clk_inv, daifmt->frame_clk_inv, > - daifmt->bit_clk_master, > - daifmt->frame_clk_master); > - return -EINVAL; > - } > - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) > - if (priv->audio_port[i].format == AFMT_I2S) > - audio.config = priv->audio_port[i].config; > - audio.format = AFMT_I2S; > + audio.format = AFMT_I2S | AFMT_I2S_PHILIPS; > + break; > + case HDMI_LEFT_J: > + audio.format = AFMT_I2S | AFMT_I2S_LEFT_J; > + break; > + case HDMI_RIGHT_J: > + audio.format = AFMT_I2S | AFMT_I2S_RIGHT_J; > break; > case HDMI_SPDIF: > - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) > - if (priv->audio_port[i].format == AFMT_SPDIF) > - audio.config = priv->audio_port[i].config; > audio.format = AFMT_SPDIF; > break; > default: > @@ -1016,11 +1023,25 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, > return -EINVAL; > } > > + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) > + if (priv->audio_port[i].format == (audio.format & AFMT_MASK)) > + audio.config = priv->audio_port[i].config; > + > if (audio.config == 0) { > dev_err(dev, "%s: No audio configuration found\n", __func__); > return -EINVAL; > } > > + if ((audio.format & AFMT_MASK) == HDMI_I2S && > + (daifmt->bit_clk_inv || daifmt->frame_clk_inv || > + daifmt->bit_clk_master || daifmt->frame_clk_master)) { > + dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, > + daifmt->bit_clk_inv, daifmt->frame_clk_inv, > + daifmt->bit_clk_master, > + daifmt->frame_clk_master); > + return -EINVAL; > + } > + > mutex_lock(&priv->audio_mutex); > if (priv->supports_infoframes && priv->sink_has_audio) > ret = tda998x_configure_audio(priv, &audio); > diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h > index 3cb25ccbe5e6..b0864f0be017 100644 > --- a/include/drm/i2c/tda998x.h > +++ b/include/drm/i2c/tda998x.h > @@ -6,9 +6,14 @@ > #include <dt-bindings/display/tda998x.h> > > enum { > - AFMT_UNUSED = 0, > - AFMT_SPDIF = TDA998x_SPDIF, > - AFMT_I2S = TDA998x_I2S, > + AFMT_UNUSED = 0, > + AFMT_SPDIF = TDA998x_SPDIF, > + AFMT_I2S = TDA998x_I2S, > + AFMT_MASK = AFMT_SPDIF | AFMT_I2S, > + AFMT_I2S_PHILIPS = 0 << 4, > + AFMT_I2S_LEFT_J = 1 << 4, > + AFMT_I2S_RIGHT_J = 2 << 4, > + AFMT_I2S_MASK = 3 << 4, > }; > > struct tda998x_audio_params { > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Mon, Feb 25, 2019 at 03:28:51PM +0200, Peter Ujfalusi wrote: > hi Russell, > > On 22/02/2019 23.27, Russell King wrote: > > Add support for the left and right justified I2S formats as well as the > > more tranditional "Philips" I2S format. > > First of all, thank you for the patch, it works. > > Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > There is however one thing I'm not sure about. > the 3.8 kernel configured the page0:0xfc register [1]: > /* select I2S format, and datasize */ > reg_write(encoder, REG_I2S_FORMAT, 0x0a); > > In theory this should select left_j and set bit3 which does something. > It looks like that the McASP is configured to I2S mode in 3.8 kernel > which would result channel swap at least there (I2S vs left_j). > > Do you know what the bit3 is configuring and to what? Bits 2 and 3 are something to do with "data size" which is as much information as I have on those two bits. Maybe they apply to the right justified mode as that would certainly need to know the width of the supplied I2S sample data.
On Fri, Feb 22, 2019 at 4:27 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > Add support for the left and right justified I2S formats as well as the > more tranditional "Philips" I2S format. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> My imx6q ssi has i2s and left-justified in common with the tda998x. Left justified does not appear to work, but I believe this is because of missing support for left-justified master mode in fsl_ssi ? So on an imx6q ssi, using i2s philips format only: Tested-by: Sven Van Asbroeck <TheSven73@gmail.com>
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a7c39f39793f..645d884fb9e8 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -242,7 +242,9 @@ struct tda998x_priv { # define HVF_CNTRL_1_SEMI_PLANAR (1 << 6) #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */ #define REG_I2S_FORMAT REG(0x00, 0xfc) /* read/write */ -# define I2S_FORMAT(x) (((x) & 3) << 0) +# define I2S_FORMAT_PHILIPS (0 << 0) +# define I2S_FORMAT_LEFT_J (2 << 0) +# define I2S_FORMAT_RIGHT_J (3 << 0) #define REG_AIP_CLKSEL REG(0x00, 0xfd) /* write */ # define AIP_CLKSEL_AIP_SPDIF (0 << 3) # define AIP_CLKSEL_AIP_I2S (1 << 3) @@ -872,14 +874,14 @@ static int tda998x_configure_audio(struct tda998x_priv *priv, struct tda998x_audio_params *params) { - u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv; + u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv, i2s_fmt; u32 n; /* Enable audio ports */ reg_write(priv, REG_ENA_AP, params->config); /* Set audio input source */ - switch (params->format) { + switch (params->format & AFMT_MASK) { case AFMT_SPDIF: reg_write(priv, REG_ENA_ACLK, 0); reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF); @@ -907,6 +909,19 @@ tda998x_configure_audio(struct tda998x_priv *priv, cts_n = CTS_N_M(3) | CTS_N_K(3); break; } + + switch (params->format & AFMT_I2S_MASK) { + case AFMT_I2S_LEFT_J: + i2s_fmt = I2S_FORMAT_LEFT_J; + break; + case AFMT_I2S_RIGHT_J: + i2s_fmt = I2S_FORMAT_RIGHT_J; + break; + default: + i2s_fmt = I2S_FORMAT_PHILIPS; + break; + } + reg_write(priv, REG_I2S_FORMAT, i2s_fmt); break; default: @@ -992,23 +1007,15 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, switch (daifmt->fmt) { case HDMI_I2S: - if (daifmt->bit_clk_inv || daifmt->frame_clk_inv || - daifmt->bit_clk_master || daifmt->frame_clk_master) { - dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, - daifmt->bit_clk_inv, daifmt->frame_clk_inv, - daifmt->bit_clk_master, - daifmt->frame_clk_master); - return -EINVAL; - } - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) - if (priv->audio_port[i].format == AFMT_I2S) - audio.config = priv->audio_port[i].config; - audio.format = AFMT_I2S; + audio.format = AFMT_I2S | AFMT_I2S_PHILIPS; + break; + case HDMI_LEFT_J: + audio.format = AFMT_I2S | AFMT_I2S_LEFT_J; + break; + case HDMI_RIGHT_J: + audio.format = AFMT_I2S | AFMT_I2S_RIGHT_J; break; case HDMI_SPDIF: - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) - if (priv->audio_port[i].format == AFMT_SPDIF) - audio.config = priv->audio_port[i].config; audio.format = AFMT_SPDIF; break; default: @@ -1016,11 +1023,25 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, return -EINVAL; } + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) + if (priv->audio_port[i].format == (audio.format & AFMT_MASK)) + audio.config = priv->audio_port[i].config; + if (audio.config == 0) { dev_err(dev, "%s: No audio configuration found\n", __func__); return -EINVAL; } + if ((audio.format & AFMT_MASK) == HDMI_I2S && + (daifmt->bit_clk_inv || daifmt->frame_clk_inv || + daifmt->bit_clk_master || daifmt->frame_clk_master)) { + dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, + daifmt->bit_clk_inv, daifmt->frame_clk_inv, + daifmt->bit_clk_master, + daifmt->frame_clk_master); + return -EINVAL; + } + mutex_lock(&priv->audio_mutex); if (priv->supports_infoframes && priv->sink_has_audio) ret = tda998x_configure_audio(priv, &audio); diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3cb25ccbe5e6..b0864f0be017 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -6,9 +6,14 @@ #include <dt-bindings/display/tda998x.h> enum { - AFMT_UNUSED = 0, - AFMT_SPDIF = TDA998x_SPDIF, - AFMT_I2S = TDA998x_I2S, + AFMT_UNUSED = 0, + AFMT_SPDIF = TDA998x_SPDIF, + AFMT_I2S = TDA998x_I2S, + AFMT_MASK = AFMT_SPDIF | AFMT_I2S, + AFMT_I2S_PHILIPS = 0 << 4, + AFMT_I2S_LEFT_J = 1 << 4, + AFMT_I2S_RIGHT_J = 2 << 4, + AFMT_I2S_MASK = 3 << 4, }; struct tda998x_audio_params {
Add support for the left and right justified I2S formats as well as the more tranditional "Philips" I2S format. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/gpu/drm/i2c/tda998x_drv.c | 57 ++++++++++++++++++++++++++------------- include/drm/i2c/tda998x.h | 11 +++++--- 2 files changed, 47 insertions(+), 21 deletions(-)