Re: [PATCH RFC 1/3] drm/i2c: tda998x: implement different I2S flavours

From: Jyri Sarha <jsarha@ti.com>
To: Russell King <rmk+kernel@armlinux.org.uk>,
	Sven Van Asbroeck <thesven73@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH RFC 1/3] drm/i2c: tda998x: implement different I2S flavours
Date: Mon, 25 Feb 2019 15:26:48 +0200	[thread overview]
Message-ID: <606ddb94-0cc1-76cc-036b-b232a55f272d@ti.com> (raw)
In-Reply-To: <E1gxILh-0004MG-EZ@rmk-PC.armlinux.org.uk>

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 {
> 

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel