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;bit_ch = 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));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);- }
- /*
* 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@samsung.com
Thanks for your review.