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_24BITSI 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
