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.
On Wed, Aug 21, 2013 at 08:33:55PM +0200, Jean-Francois Moine wrote:
On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote:
+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.
And by doing so create an obtuse API. No thanks.
+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?
Because I was not aware of them.
- /* 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).
Correct - however not strange, if you've read the HDMI specification.
- 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.
Correct, but the driver has no way to know this. The above reflects how the NXP driver on the Cubox implements this (which we know works for multiple different sample rates). This is because we use SPDIF, which encodes the sample rate in the channel status information, which is then presumably passed through by the TDA998x. I wouldn't know, I don't have a HDMI debugger.
What I do know is that it works for multiple sample rates.
dri-devel@lists.freedesktop.org
-
Jean-Francois Moine -
Russell King - ARM Linux