[PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Jyri Sarha]

On 09/28/15 12:01, Arnaud Pouliquen wrote:
> few questions/remarks
> BR,
> Arnaud
>
>> +static void hdmi_codec_abort(struct device *dev)
>> +{
>> +    struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
>> +
>> +    dev_dbg(dev, "%s()\n", __func__);
>> +
>> +    mutex_lock(&hcp->current_stream_lock);
>> +    if (hcp->current_stream && hcp->current_stream->runtime &&
>> +        snd_pcm_running(hcp->current_stream)) {
>> +        dev_info(dev, "HDMI audio playback aborted\n");
>> +        snd_pcm_stream_lock_irq(hcp->current_stream);
>> +        snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
>> +        snd_pcm_stream_unlock_irq(hcp->current_stream);
>> +    }
>> +    mutex_unlock(&hcp->current_stream_lock);
>> +}
> Does driver should stop the stream in case of unplug?
> This could generate unexpected behavior at application level.
> Perhaps should be better if this part is managed in DRM driver. if HDMI
> master, I2S bus should be maintained ON to consume the audio stream
> until application action.
>

The whole point of this abort callback is to provide the means for the 
video side to stop the audio playback in a clean way.

The abort callback is given to the video side in startup() callback 
(ASoC side calls video side). If the video side sees that the playback 
can not go on it can call the abort callback and the audio playback will 
abort in standard ALSA way and a properly written applications should 
not get confused.

Nothing is forcing the video side to call the callback ever, if so 
decided. But if for instance power management stops the i2s clocks when 
connector is unplugged, then the audio can not go on any more and it 
should be aborted (actually it would abort in a moment when ALSA 
realizes that the DMA is not running).

>> +
>> +static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
>> +                struct snd_pcm_hw_params *params,
>> +                struct snd_soc_dai *dai)
>> +{
>> +    struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
>> +    struct hdmi_codec_params hp = {
>> +        .iec = {
>> +            .status = { 0 },
>> +            .subcode = { 0 },
>> +            .pad = 0,
>> +            .dig_subframe = { 0 },
>> +        }
>> +    };
>> +    int ret;
>> +
>> +    dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
>> +        params_width(params), params_rate(params),
>> +        params_channels(params));
>> +
>> +    ret = snd_pcm_create_iec958_consumer_hw_params(params,
>> hp.iec.status,
>> +                               sizeof(hp.iec.status));
> Tell me if i am wrong, but in case of SPDIF format, IEC status is
> managed by cpu_dai not by the codec.
> To manage IEC61937 a control should be needed to update IEC status bits...
>

AFAIK yes. However, the video side implementation is free to ignore 
status bits in the struct hdmi_codec_params and it should normally do so 
if the format is SPDIF. Of course I could save couple CPU cycles in the 
codec code if would not even produce the bits when the format is SPDIF.

Best regards,
Jyri

>> +    if (ret < 0) {
>> +        dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",
>> +            ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = hdmi_codec_new_stream(substream, dai);
>> +    if (ret)
>> +        return ret;
>> +
>> +    hdmi_audio_infoframe_init(&hp.cea);
>> +    hp.cea.channels = params_channels(params);
>> +    hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
>> +    hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
>> +    hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
>> +
>> +    hp.sample_width = params_width(params);
>> +    hp.sample_rate = params_rate(params);
>> +    hp.channels = params_channels(params);
>> +
>> +    return hcp->hcd.ops->hw_params(dai->dev->parent,
>> &hcp->daifmt[dai->id],
>> +                       &hp);
>> +}
>> +