Re: [PATCH v2] drm: exynos: Add driver for HDMI audio interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 09/19/2017 10:30 AM, Andrzej Hajda wrote:
On 18.09.2017 18:19, Sylwester Nawrocki wrote:

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 214fa5e..dc254b7 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -40,7 +40,7 @@

+struct hdmi_audio {
+	struct platform_device *pdev;
+	struct hdmi_audio_infoframe infoframe;
+	unsigned int sample_rate;
+	unsigned int sample_width;
+	u8 enable;

Why not bool ?

Yes, I also asked myself that question after submitting the patch.
-static void hdmi_audio_init(struct hdmi_context *hdata)
+static void hdmi_audio_config(struct hdmi_context *hdata)
  {
-	u32 sample_rate, bits_per_sample;
-	u32 data_num, bit_ch, sample_frq;
-	u32 val;
+	u32 data_num, sample_freq, val;
+	u32 bit_ch = 1;

-	sample_rate = 44100;
-	bits_per_sample = 16;

-	switch (bits_per_sample) {
+	switch (hdata->audio.sample_width) {
  	case 20:
  		data_num = 2;
-		bit_ch = 1;
  		break;
  	case 24:
  		data_num = 3;
-		bit_ch = 1;
  		break;
  	default:
  		data_num = 1;
@@ -1019,7 +1034,7 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
  		break;
  	}

-	hdmi_reg_acr(hdata, sample_rate);
+	hdmi_reg_acr(hdata, hdata->audio.sample_rate);

  	hdmi_reg_writeb(hdata, HDMI_I2S_MUX_CON, HDMI_I2S_IN_DISABLE
  				| HDMI_I2S_AUD_I2S | HDMI_I2S_CUV_I2S_ENABLE
@@ -1030,10 +1045,21 @@ static void hdmi_audio_init(struct hdmi_context *hdata)

  	hdmi_reg_writeb(hdata, HDMI_I2S_MUX_CUV, HDMI_I2S_CUV_RL_EN);

-	sample_frq = (sample_rate == 44100) ? 0 :
-			(sample_rate == 48000) ? 2 :
-			(sample_rate == 32000) ? 3 :
-			(sample_rate == 96000) ? 0xa : 0x0;
+	switch (hdata->audio.sample_rate) {
+	case 32000:
+		sample_freq = 0x3;
+		break;
+	case 48000:
+		sample_freq = 0x2;
+		break;
+	case 96000:
+		sample_freq = 0xa;
+		break;
+	case 44100:
+	default:
+		sample_freq = 0;
+		break;
+	}

  	hdmi_reg_writeb(hdata, HDMI_I2S_CLK_CON, HDMI_I2S_CLK_DIS);
  	hdmi_reg_writeb(hdata, HDMI_I2S_CLK_CON, HDMI_I2S_CLK_EN);
@@ -1067,7 +1093,7 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
  	hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_1, HDMI_I2S_CD_PLAYER);
  	hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_2, HDMI_I2S_SET_SOURCE_NUM(0));
  	hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_3, HDMI_I2S_CLK_ACCUR_LEVEL_2
-			| HDMI_I2S_SET_SMP_FREQ(sample_frq));
+			| HDMI_I2S_SET_SMP_FREQ(sample_freq));
  	hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_4,
  			HDMI_I2S_ORG_SMP_FREQ_44_1
  			| HDMI_I2S_WORD_LEN_MAX24_24BITS

I am not audio expert, so maybe I miss something but I wonder if it
wouldn't be good to fill HDMI_I2S_CH_ST_* with content of
hdmi_codec_params.iec.status?
This way you can remove some magic code above, but maybe it could be
done in separate patch.

Yes, makes sense, I will try and see if it works properly with
the HDMI_I2S_CH_ST_? registers written directly with values from
the IEC status arrray.

@@ -1076,13 +1102,15 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
  	hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_CON, HDMI_I2S_CH_STATUS_RELOAD);
  }

-static void hdmi_audio_control(struct hdmi_context *hdata, bool onoff)
+static void hdmi_audio_control(struct hdmi_context *hdata)
  {
+	bool enable = hdata->audio.enable;
+
  	if (hdata->dvi_mode)
  		return;

-	hdmi_reg_writeb(hdata, HDMI_AUI_CON, onoff ? 2 : 0);
-	hdmi_reg_writemask(hdata, HDMI_CON_0, onoff ?
+	hdmi_reg_writeb(hdata, HDMI_AUI_CON, enable ? 2 : 0);

While at it you can replace magic '2' to HDMI_AUI_CON_EVERY_VSYNC.

Changed.

+	hdmi_reg_writemask(hdata, HDMI_CON_0, enable ?
  			HDMI_ASP_EN : HDMI_ASP_DIS, HDMI_ASP_MASK);
  }


  static void hdmi_disable(struct drm_encoder *encoder)
  {
  	struct hdmi_context *hdata = encoder_to_hdmi(encoder);

-	if (!hdata->powered)
+	mutex_lock(&hdata->mutex);
+
+	if (hdata->powered) {
+		/*
+		 * The SFRs of VP and Mixer are updated by Vertical Sync of
+		 * Timing generator which is a part of HDMI so the sequence
+		 * to disable TV Subsystem should be as following,
+		 *	VP -> Mixer -> HDMI
+		 *
+		 * To achieve such sequence HDMI is disabled together with
+		 * HDMI PHY, via pipe clock callback.
+		 */
+		mutex_unlock(&hdata->mutex);
+		cancel_delayed_work(&hdata->hotplug_work);
+		cec_notifier_set_phys_addr(hdata->notifier,
+					   CEC_PHYS_ADDR_INVALID);
  		return;
+	}

-	/*
-	 * The SFRs of VP and Mixer are updated by Vertical Sync of
-	 * Timing generator which is a part of HDMI so the sequence
-	 * to disable TV Subsystem should be as following,
-	 *	VP -> Mixer -> HDMI
-	 *
-	 * To achieve such sequence HDMI is disabled together with HDMI PHY, via
-	 * pipe clock callback.
-	 */
-	cancel_delayed_work(&hdata->hotplug_work);
-	cec_notifier_set_phys_addr(hdata->notifier, CEC_PHYS_ADDR_INVALID);
+	mutex_unlock(&hdata->mutex);

Wouldn't be enough to change it to:
mutex_lock
powered = hdata->powered;
mutex_unlock
if (!powered)
     return

It would, I tried it but current version looks better to me so I would
prefer to keep it that way.

  }

  static const struct drm_encoder_helper_funcs exynos_hdmi_encoder_helper_funcs = {
@@ -1513,6 +1554,109 @@ static void hdmi_disable(struct drm_encoder *encoder)
  	.destroy = drm_encoder_cleanup,
  };

+static int hdmi_register_audio_device(struct hdmi_context *hdata)
+{
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &audio_codec_ops,
+		.max_i2s_channels = 6,
+		.i2s = 1,
+	};
+
+	hdata->audio.pdev = platform_device_register_data(
+		hdata->dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));
+
+	if (IS_ERR(hdata->audio.pdev))
+		return PTR_ERR(hdata->audio.pdev);
+
+	return 0;

return PTR_ERR_OR_ZERO(...) ?

Changed.

Beside these nitpicks:
Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>

Thanks for your review.

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux