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

[Sylwester Nawrocki]

On 10/23/2017 03:27 AM, Inki Dae wrote:

>> +static int hdmi_audio_digital_mute(struct device *dev, void *data, bool mute)
>> +{
>> +	struct hdmi_context *hdata = dev_get_drvdata(dev);
>> +
>> +	mutex_lock(&hdata->mutex);
>> +
>> +	hdata->audio.enable = !mute;
> 
> Wouldn't it be better to keep 'mute' instead of 'enable'? 'hdata->audio.enable' 
is used by only hdmi_adio_control function and whether hdmi is power on or nis already checked by 'hdata->powered'
Yes, makes sense, I'll change it.
 
>> +
>> +	if (hdata->powered)
>> +		hdmi_audio_control(hdata);
>> +
>> +	mutex_unlock(&hdata->mutex);
>> +
>> +	return 0;
>> +}


>> +static void hdmi_unregister_audio_device(struct hdmi_context *hdata)
>> +{
>> +	platform_device_unregister(hdata->audio.pdev);
>> +}
> 
> This function is unnecessary wrapper. You can use platform_device_unregister(hdata->audio.pdev) at probe callback.
> And you would need to unregister audio device at remove callback.

Fixed in v4.

>>  static int hdmi_bridge_init(struct hdmi_context *hdata)
>> @@ -1697,6 +1812,7 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
>>  	struct drm_device *drm_dev = data;
>>  	struct hdmi_context *hdata = dev_get_drvdata(dev);
>>  	struct drm_encoder *encoder = &hdata->encoder;
>> +	struct hdmi_audio_infoframe *audio_infoframe = &hdata->audio.infoframe;
>>  	struct exynos_drm_crtc *crtc;
>>  	int ret;
>>
>> @@ -1704,6 +1820,12 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
>>
>>  	hdata->phy_clk.enable = hdmiphy_clk_enable;
>>
>> +	hdmi_audio_infoframe_init(audio_infoframe);
>> +	audio_infoframe->coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
>> +	audio_infoframe->sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
>> +	audio_infoframe->sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
>> +	audio_infoframe->channels = 2;
> 
> Audio device is registered at probe callback so it would be better to move above code into probe callback.
> I coudln't see initializing audio infoframe should be done here.

Yes, it makes more sense indeed to have this initialization in probe().

Thanks for your review.

Regards,
Sylwester