linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/8] Several NXP TDA998x patches
@ 2013-08-14 19:43 Sebastian Hesselbarth
  2013-08-14 19:43 ` [PATCH v2 1/8] drm/i2c: tda998x: fix EDID reading on TDA19988 devices Sebastian Hesselbarth
                   ` (8 more replies)
  0 siblings, 9 replies; 16+ messages in thread
From: Sebastian Hesselbarth @ 2013-08-14 19:43 UTC (permalink / raw)
  To: Sebastian Hesselbarth
  Cc: David Airlie, Darren Etheridge, Rob Clark, Russell King,
	Daniel Vetter, dri-devel, linux-kernel

This patch set picks up several patches sent during the past months
related with NXP TDA998x HDMI transmitter driver. The patches have
been tested on Marvell Dove (Armada DRM) on several HDMI and DVI modes
with audio playing on S/PDIF.

I bumped all patches to v2, although only patches 2, 5, and 6 have
changed. I also fixed a typo in commit line of patch 8. As Darren
Etheridge already gave his Tested-by and tilcdc related patches have
not changed, I added it to all patches for v2, too.

I have not added Russell King's Tested-by, as I hope he will have a
close look at what I changed after his review comments.

Darren Etheridge (1):
  drm/tilcdc fixup mode to workaround sync for tda998x

Russell King (5):
  drm/i2c: tda998x: fix EDID reading on TDA19988 devices
  drm/i2c: tda998x: ensure VIP output mux is properly set
  drm/i2c: tda998x: fix npix/nline programming
  drm/i2c: tda998x: prepare for video input configuration
  drm/i2c: tda998x: add video and audio input configuration

Sebastian Hesselbarth (2):
  drm/i2c: tda998x: fix sync generation and calculation
  drm/i2c: tda998x: prepare for broken sync workaround

 drivers/gpu/drm/i2c/tda998x_drv.c     |  481 +++++++++++++++++++++++++++------
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |    7 +-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   27 +-
 include/drm/i2c/tda998x.h             |   30 ++
 4 files changed, 467 insertions(+), 78 deletions(-)
 create mode 100644 include/drm/i2c/tda998x.h

---
Cc: David Airlie <airlied@linux.ie>
Cc: Darren Etheridge <detheridge@ti.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
-- 
1.7.10.4


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
@ 2013-08-21 18:33 Jean-Francois Moine
  2013-08-21 22:41 ` Russell King - ARM Linux
  0 siblings, 1 reply; 16+ messages in thread
From: Jean-Francois Moine @ 2013-08-21 18:33 UTC (permalink / raw)
  To: Sebastian Hesselbarth, Russell King, dri-devel
  Cc: David Airlie, Darren Etheridge, Rob Clark, Daniel Vetter,
	linux-kernel

On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote:
> From: Russell King <rmk+kernel at arm.linux.org.uk>
> 
> This patch adds tda998x specific parameters to allow it to be configured
> for different boards using it. Also, this implements rudimentary audio
> support for S/PDIF attached controllers.

It seems that this patch mixes two different functions:
- configuration
- audio

About configuration, why don't you use the standard way to set the
device parameters, i.e. board info and DT?

> Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth at gmail.com>
> Tested-by: Darren Etheridge <detheridge at ti.com>
> ---
> Changelog:
> for v1:
> - set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz
> - also calculate CTS
> v1->v2:
> - Remove CTS calculation as it isn't used in current TDA998x setup
>   (Reported by Russell King)
> - Remove debug left-over (Reported by Russell King)
> - Use correct tda998x include (Reported by Russell King)
> 
> Cc: David Airlie <airlied at linux.ie>
> Cc: Darren Etheridge <detheridge at ti.com>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Russell King <rmk+kernel at arm.linux.org.uk>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: dri-devel at lists.freedesktop.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c |  268 +++++++++++++++++++++++++++++++++++--
>  include/drm/i2c/tda998x.h         |   30 +++++
>  2 files changed, 290 insertions(+), 8 deletions(-)
>  create mode 100644 include/drm/i2c/tda998x.h
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 527d11b..2b64dfa 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
	[snip]
> @@ -344,6 +390,23 @@ fail:
>  	return ret;
>  }
>  
> +static void
> +reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int cnt)
> +{
> +	struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
> +	uint8_t buf[cnt+1];
> +	int ret;
> +
> +	buf[0] = REG2ADDR(reg);
> +	memcpy(&buf[1], p, cnt);
> +
> +	set_page(encoder, reg);
> +
> +	ret = i2c_master_send(client, buf, cnt + 1);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
> +}
> +

It seems simpler to reserve one byte in the caller buffer for the
register address and avoid a memcpy.

>  static uint8_t
>  reg_read(struct drm_encoder *encoder, uint16_t reg)
>  {
> @@ -412,7 +475,7 @@ tda998x_reset(struct drm_encoder *encoder)
>  	reg_write(encoder, REG_SERIALIZER,   0x00);
>  	reg_write(encoder, REG_BUFFER_OUT,   0x00);
>  	reg_write(encoder, REG_PLL_SCG1,     0x00);
> -	reg_write(encoder, REG_AUDIO_DIV,    0x03);
> +	reg_write(encoder, REG_AUDIO_DIV,    AUDIO_DIV_SERCLK_8);
>  	reg_write(encoder, REG_SEL_CLK,      SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
>  	reg_write(encoder, REG_PLL_SCGN1,    0xfa);
>  	reg_write(encoder, REG_PLL_SCGN2,    0x00);
> @@ -424,11 +487,184 @@ tda998x_reset(struct drm_encoder *encoder)
>  	reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
>  }
>  
> +static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
> +{
> +	uint8_t sum = 0;
> +
> +	while (bytes--)
> +		sum += *buf++;
> +	return (255 - sum) + 1;
> +}

Simpler:

static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
{
	int sum = 0;			/* avoid byte computation */

	while (bytes--)
		sum -= *buf++;		/* avoid a substraction */
	return sum;
}

> +
> +#define HB(x) (x)
> +#define PB(x) (HB(2) + 1 + (x))
> +
> +static void
> +tda998x_write_if(struct drm_encoder *encoder, uint8_t bit, uint16_t addr,
> +		 uint8_t *buf, size_t size)
> +{
> +	buf[PB(0)] = tda998x_cksum(buf, size);
> +
> +	reg_clear(encoder, REG_DIP_IF_FLAGS, bit);
> +	reg_write_range(encoder, addr, buf, size);
> +	reg_set(encoder, REG_DIP_IF_FLAGS, bit);
> +}
> +
> +static void
> +tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params *p)
> +{
> +	uint8_t buf[PB(5) + 1];
> +
> +	buf[HB(0)] = 0x84;
> +	buf[HB(1)] = 0x01;
> +	buf[HB(2)] = 10;

Why don't you use the constants which are defined in hdmi.h?

> +	buf[PB(0)] = 0;
> +	buf[PB(1)] = p->audio_frame[1] & 0x07; /* CC */
> +	buf[PB(2)] = p->audio_frame[2] & 0x1c; /* SF */
> +	buf[PB(4)] = p->audio_frame[4];
> +	buf[PB(5)] = p->audio_frame[5] & 0xf8; /* DM_INH + LSV */
> +
> +	tda998x_write_if(encoder, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf,
> +			 sizeof(buf));
> +}
> +
> +static void
> +tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode)
> +{
> +	uint8_t buf[PB(13) + 1];
> +
> +	memset(buf, 0, sizeof(buf));
> +	buf[HB(0)] = 0x82;
> +	buf[HB(1)] = 0x02;
> +	buf[HB(2)] = 13;
> +	buf[PB(4)] = drm_match_cea_mode(mode);
> +
> +	tda998x_write_if(encoder, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf,
> +			 sizeof(buf));
> +}
> +
> +static void tda998x_audio_mute(struct drm_encoder *encoder, bool on)
> +{
> +	if (on) {
> +		reg_set(encoder, REG_SOFTRESET, SOFTRESET_AUDIO);
> +		reg_clear(encoder, REG_SOFTRESET, SOFTRESET_AUDIO);
> +		reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
> +	} else {
> +		reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
> +	}
> +}
> +
> +static void
> +tda998x_configure_audio(struct drm_encoder *encoder,
> +		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
> +{
> +	uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv;
> +	uint32_t n;
> +
> +	/* Enable audio ports */
> +	reg_write(encoder, REG_ENA_AP, p->audio_cfg);
> +	reg_write(encoder, REG_ENA_ACLK, p->audio_clk_cfg);
> +
> +	/* Set audio input source */
> +	switch (p->audio_format) {
> +	case AFMT_SPDIF:
> +		reg_write(encoder, REG_MUX_AP, 0x40);
> +		clksel_aip = AIP_CLKSEL_AIP(0);
> +		/* FS64SPDIF */
> +		clksel_fs = AIP_CLKSEL_FS(2);
> +		cts_n = CTS_N_M(3) | CTS_N_K(3);
> +		ca_i2s = 0;
> +		break;
> +
> +	case AFMT_I2S:
> +		reg_write(encoder, REG_MUX_AP, 0x64);
> +		clksel_aip = AIP_CLKSEL_AIP(1);
> +		/* ACLK */
> +		clksel_fs = AIP_CLKSEL_FS(0);
> +		cts_n = CTS_N_M(3) | CTS_N_K(3);
> +		ca_i2s = CA_I2S_CA_I2S(0);
> +		break;
> +	}
> +
> +	reg_write(encoder, REG_AIP_CLKSEL, clksel_aip);
> +	reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT);
> +
> +	/* Enable automatic CTS generation */
> +	reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN);
> +	reg_write(encoder, REG_CTS_N, cts_n);
> +
> +	/*
> +	 * Audio input somehow depends on HDMI line rate which is
> +	 * related to pixclk. Testing showed that modes with pixclk
> +	 * >100MHz need a larger divider while <40MHz need the default.
> +	 * There is no detailed info in the datasheet, so we just
> +	 * assume 100MHz requires larger divider.
> +	 */
> +	if (mode->clock > 100000)
> +		adiv = AUDIO_DIV_SERCLK_16;
> +	else
> +		adiv = AUDIO_DIV_SERCLK_8;
> +	reg_write(encoder, REG_AUDIO_DIV, adiv);
> +
> +	/*
> +	 * This is the approximate value of N, which happens to be
> +	 * the recommended values for non-coherent clocks.
> +	 */
> +	n = 128 * p->audio_sample_rate / 1000;
> +
> +	/* Write the CTS and N values */
> +	buf[0] = 0x44;
> +	buf[1] = 0x42;
> +	buf[2] = 0x01;

The CTS value is strange, but that does not matter: its generation is
automatic (see above).

> +	buf[3] = n;
> +	buf[4] = n >> 8;
> +	buf[5] = n >> 16;
> +	reg_write_range(encoder, REG_ACR_CTS_0, buf, 6);
> +
> +	/* Set CTS clock reference */
> +	reg_write(encoder, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
> +
> +	/* Reset CTS generator */
> +	reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
> +	reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
> +
> +	/* Write the channel status */
> +	buf[0] = 0x04;
> +	buf[1] = 0x00;
> +	buf[2] = 0x00;
> +	buf[3] = 0xf1;
> +	reg_write_range(encoder, REG_CH_STAT_B(0), buf, 4);
	[snip]

>From what I understood in the NXP driver, buf[3] depends on the sample
rate: 0xf1 for 44.1kHz and 0xd1 for 48kHz.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-09-22 21:54 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-14 19:43 [PATCH v2 0/8] Several NXP TDA998x patches Sebastian Hesselbarth
2013-08-14 19:43 ` [PATCH v2 1/8] drm/i2c: tda998x: fix EDID reading on TDA19988 devices Sebastian Hesselbarth
2013-08-14 19:43 ` [PATCH v2 2/8] drm/i2c: tda998x: ensure VIP output mux is properly set Sebastian Hesselbarth
2013-08-14 19:43 ` [PATCH v2 3/8] drm/i2c: tda998x: fix npix/nline programming Sebastian Hesselbarth
2013-08-14 19:43 ` [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration Sebastian Hesselbarth
2013-08-14 19:43 ` [PATCH v2 5/8] drm/i2c: tda998x: add video and audio " Sebastian Hesselbarth
2013-08-18 23:23   ` Dave Airlie
2013-08-18 23:29     ` Russell King - ARM Linux
2013-09-02 14:50   ` [PATCH] drm/i2c: Fix broken TDA998x audio (was: Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration) Russell King - ARM Linux
2013-09-22 21:53     ` Russell King - ARM Linux
2013-08-14 19:43 ` [PATCH v2 6/8] drm/i2c: tda998x: fix sync generation and calculation Sebastian Hesselbarth
2013-08-14 19:43 ` [PATCH v2 7/8] drm/i2c: tda998x: prepare for broken sync workaround Sebastian Hesselbarth
2013-08-14 19:43 ` [PATCH v2 8/8] drm/tilcdc fixup mode to workaround sync for tda998x Sebastian Hesselbarth
2013-08-15 22:32 ` [PATCH v2 0/8] Several NXP TDA998x patches Russell King - ARM Linux
  -- strict thread matches above, loose matches on Subject: below --
2013-08-21 18:33 [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration Jean-Francois Moine
2013-08-21 22:41 ` Russell King - ARM Linux

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).