| Message ID | 5adcd94104080abcd5b072a5d6a85b85c5ea0584.1470129989.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote: > @@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv, > reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS); > > /* Write the channel status */ > - buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT; > - buf[1] = 0x00; > - buf[2] = IEC958_AES3_CON_FS_NOTID; > - buf[3] = IEC958_AES4_CON_ORIGFS_NOTID | > - IEC958_AES4_CON_MAX_WORDLEN_24; > - reg_write_range(priv, REG_CH_STAT_B(0), buf, 4); > + reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4); Take a close look at the code you are replacing here - the buffer contents is not AES bytes 0, 1, 2, 3, 4, but AES bytes 0, 1, 3, 4 - byte 2 is not present in this array. I don't think you've noticed this as your second patch merely copies verboten the IEC status: + memcpy(audio.status, params->iec.status, + min(sizeof(audio.status), sizeof(params->iec.status))); assuming that it is bytes 0-3. Byte 2 is stored separately for each I2S channel in the following registers. > diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h > index 3e419d9..24be7aa 100644 > --- a/include/drm/i2c/tda998x.h > +++ b/include/drm/i2c/tda998x.h > @@ -1,6 +1,19 @@ > #ifndef __DRM_I2C_TDA998X_H__ > #define __DRM_I2C_TDA998X_H__ > > +#define AFMT_UNUSED 0 > +#define AFMT_SPDIF 1 > +#define AFMT_I2S 2 I'd prefer this to stay an enum please. > +struct tda998x_audio_params { > + u8 config; > + u8 format; > + unsigned sample_width; > + unsigned sample_rate; > + struct hdmi_audio_infoframe cea; With this addition, this file will need to include linux/hdmi.h. > + u8 status[4]; A comment here about the missing byte 2 would probably be a good idea.
On 08/04/16 16:31, Russell King - ARM Linux wrote: > On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote: >> @@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv, >> reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS); >> >> /* Write the channel status */ >> - buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT; >> - buf[1] = 0x00; >> - buf[2] = IEC958_AES3_CON_FS_NOTID; >> - buf[3] = IEC958_AES4_CON_ORIGFS_NOTID | >> - IEC958_AES4_CON_MAX_WORDLEN_24; >> - reg_write_range(priv, REG_CH_STAT_B(0), buf, 4); >> + reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4); > > Take a close look at the code you are replacing here - the buffer > contents is not AES bytes 0, 1, 2, 3, 4, but AES bytes 0, 1, 3, 4 > - byte 2 is not present in this array. I don't think you've noticed > this as your second patch merely copies verboten the IEC status: > > + memcpy(audio.status, params->iec.status, > + min(sizeof(audio.status), sizeof(params->iec.status))); > > assuming that it is bytes 0-3. Byte 2 is stored separately for each > I2S channel in the following registers. > Oh, yes. I missed that. I think it would be better to increase tda998x_audio_params status buffer length by one and keep it otherwise as it is. Then just write the bytes 0-1 and 3-4 separately into channel status registers, with appropriate comments. I guess the the AES byte 2 can remain 0 for now (source and channel unspecified, with assumption of consumer stream). >> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h >> index 3e419d9..24be7aa 100644 >> --- a/include/drm/i2c/tda998x.h >> +++ b/include/drm/i2c/tda998x.h >> @@ -1,6 +1,19 @@ >> #ifndef __DRM_I2C_TDA998X_H__ >> #define __DRM_I2C_TDA998X_H__ >> >> +#define AFMT_UNUSED 0 >> +#define AFMT_SPDIF 1 >> +#define AFMT_I2S 2 > > I'd prefer this to stay an enum please. > Ok. >> +struct tda998x_audio_params { >> + u8 config; >> + u8 format; >> + unsigned sample_width; >> + unsigned sample_rate; >> + struct hdmi_audio_infoframe cea; > > With this addition, this file will need to include linux/hdmi.h. > Oh yes, thanks. >> + u8 status[4]; > > A comment here about the missing byte 2 would probably be a good idea. > As I said earlier, I'd rather keep this as plain channel status buffer and just increase its size by one. Thanks a lot for the review! Best reagards, Jyri
On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote: > @@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv, > reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS); > > /* Write the channel status */ > - buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT; > - buf[1] = 0x00; > - buf[2] = IEC958_AES3_CON_FS_NOTID; > - buf[3] = IEC958_AES4_CON_ORIGFS_NOTID | > - IEC958_AES4_CON_MAX_WORDLEN_24; > - reg_write_range(priv, REG_CH_STAT_B(0), buf, 4); > + reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4); Another couple of points here: 1. I think we should only write the channel status if I2S mode is selected - channel status won't be used on SPDIF mode as SPDIF supplies its own channel status. 2. I think we should continue writing all the channel status in a single I2C transaction, and not breaking it into several different transactions to cater for the odd register order. I don't have any direct evidence that one way is better than the other in terms of atomicity of the update, but I think having it as a single transaction stands a better chance of the chip atomically updating the channel status.
On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote: > @@ -41,7 +41,7 @@ struct tda998x_priv { > u8 vip_cntrl_0; > u8 vip_cntrl_1; > u8 vip_cntrl_2; > - struct tda998x_encoder_params params; > + struct tda998x_audio_params audio_params; ... > @@ -820,7 +819,7 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv, > VIP_CNTRL_2_SWAP_F(p->swap_f) | > (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0); > > - priv->params = *p; > + priv->audio_params = p->audio; ... > @@ -15,16 +28,7 @@ struct tda998x_encoder_params { > u8 swap_e:3; > u8 mirr_e:1; > > - u8 audio_cfg; > - u8 audio_clk_cfg; > - u8 audio_frame[6]; > - > - enum { > - AFMT_SPDIF, > - AFMT_I2S > - } audio_format; > - > - unsigned audio_sample_rate; > + struct tda998x_audio_params audio; Another point - it would be nice if the name for both tda998x_audio_params structures was the same.
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index f4315bc..f97b748 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -41,7 +41,7 @@ struct tda998x_priv { u8 vip_cntrl_0; u8 vip_cntrl_1; u8 vip_cntrl_2; - struct tda998x_encoder_params params; + struct tda998x_audio_params audio_params; wait_queue_head_t wq_edid; volatile int wq_edid_wait; @@ -666,26 +666,16 @@ tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr, reg_set(priv, REG_DIP_IF_FLAGS, bit); } -static void -tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p) +static int tda998x_write_aif(struct tda998x_priv *priv, + struct hdmi_audio_infoframe *cea) { union hdmi_infoframe frame; - hdmi_audio_infoframe_init(&frame.audio); - - frame.audio.channels = p->audio_frame[1] & 0x07; - frame.audio.channel_allocation = p->audio_frame[4]; - frame.audio.level_shift_value = (p->audio_frame[5] & 0x78) >> 3; - frame.audio.downmix_inhibit = (p->audio_frame[5] & 0x80) >> 7; - - /* - * L-PCM and IEC61937 compressed audio shall always set sample - * frequency to "refer to stream". For others, see the HDMI - * specification. - */ - frame.audio.sample_frequency = (p->audio_frame[2] & 0x1c) >> 2; + frame.audio = *cea; tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, &frame); + + return 0; } static void @@ -710,20 +700,21 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on) } } -static void +static int tda998x_configure_audio(struct tda998x_priv *priv, - struct drm_display_mode *mode, struct tda998x_encoder_params *p) + struct tda998x_audio_params *params, + unsigned mode_clock) { u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv; u32 n; /* Enable audio ports */ - reg_write(priv, REG_ENA_AP, p->audio_cfg); - reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg); + reg_write(priv, REG_ENA_AP, params->config); /* Set audio input source */ - switch (p->audio_format) { + switch (params->format) { case AFMT_SPDIF: + reg_write(priv, REG_ENA_ACLK, 0); reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF); clksel_aip = AIP_CLKSEL_AIP_SPDIF; clksel_fs = AIP_CLKSEL_FS_FS64SPDIF; @@ -731,15 +722,29 @@ tda998x_configure_audio(struct tda998x_priv *priv, break; case AFMT_I2S: + reg_write(priv, REG_ENA_ACLK, 1); reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; - cts_n = CTS_N_M(3) | CTS_N_K(3); + switch (params->sample_width) { + case 16: + cts_n = CTS_N_M(3) | CTS_N_K(1); + break; + case 18: + case 20: + case 24: + cts_n = CTS_N_M(3) | CTS_N_K(2); + break; + default: + case 32: + cts_n = CTS_N_M(3) | CTS_N_K(3); + break; + } break; default: BUG(); - return; + return -EINVAL; } reg_write(priv, REG_AIP_CLKSEL, clksel_aip); @@ -755,11 +760,11 @@ tda998x_configure_audio(struct tda998x_priv *priv, * assume 100MHz requires larger divider. */ adiv = AUDIO_DIV_SERCLK_8; - if (mode->clock > 100000) + if (mode_clock > 100000) adiv++; /* AUDIO_DIV_SERCLK_16 */ /* S/PDIF asks for a larger divider */ - if (p->audio_format == AFMT_SPDIF) + if (params->format == AFMT_SPDIF) adiv++; /* AUDIO_DIV_SERCLK_16 or _32 */ reg_write(priv, REG_AUDIO_DIV, adiv); @@ -768,7 +773,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, * 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; + n = 128 * params->sample_rate / 1000; /* Write the CTS and N values */ buf[0] = 0x44; @@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS); /* Write the channel status */ - buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT; - buf[1] = 0x00; - buf[2] = IEC958_AES3_CON_FS_NOTID; - buf[3] = IEC958_AES4_CON_ORIGFS_NOTID | - IEC958_AES4_CON_MAX_WORDLEN_24; - reg_write_range(priv, REG_CH_STAT_B(0), buf, 4); + reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4); tda998x_audio_mute(priv, true); msleep(20); tda998x_audio_mute(priv, false); - /* Write the audio information packet */ - tda998x_write_aif(priv, p); + return tda998x_write_aif(priv, ¶ms->cea); } /* DRM encoder functions */ @@ -820,7 +819,7 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv, VIP_CNTRL_2_SWAP_F(p->swap_f) | (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0); - priv->params = *p; + priv->audio_params = p->audio; } static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) @@ -1057,9 +1056,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, tda998x_write_avi(priv, adjusted_mode); - if (priv->params.audio_cfg) - tda998x_configure_audio(priv, adjusted_mode, - &priv->params); + if (priv->audio_params.format != AFMT_UNUSED) { + tda998x_configure_audio(priv, + &priv->audio_params, + adjusted_mode->clock); + } } } diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3e419d9..24be7aa 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -1,6 +1,19 @@ #ifndef __DRM_I2C_TDA998X_H__ #define __DRM_I2C_TDA998X_H__ +#define AFMT_UNUSED 0 +#define AFMT_SPDIF 1 +#define AFMT_I2S 2 + +struct tda998x_audio_params { + u8 config; + u8 format; + unsigned sample_width; + unsigned sample_rate; + struct hdmi_audio_infoframe cea; + u8 status[4]; +}; + struct tda998x_encoder_params { u8 swap_b:3; u8 mirr_b:1; @@ -15,16 +28,7 @@ struct tda998x_encoder_params { u8 swap_e:3; u8 mirr_e:1; - u8 audio_cfg; - u8 audio_clk_cfg; - u8 audio_frame[6]; - - enum { - AFMT_SPDIF, - AFMT_I2S - } audio_format; - - unsigned audio_sample_rate; + struct tda998x_audio_params audio; }; #endif
Define struct tda998x_audio_params in include/drm/i2c/tda998x.h and use it in pdata and for tda998x_configure_audio() parameters. Also updates tda998x_write_aif() to take struct hdmi_audio_infoframe * directly as a parameter. Signed-off-by: Jyri Sarha <jsarha@ti.com> --- drivers/gpu/drm/i2c/tda998x_drv.c | 77 ++++++++++++++++++++------------------- include/drm/i2c/tda998x.h | 24 +++++++----- 2 files changed, 53 insertions(+), 48 deletions(-)