Re: [RFC 3/5] ASoC: codec: hdmi drm codec driver

[Jyri Sarha <jsarha@ti.com>]

On 09/25/15 18:50, Arnaud Pouliquen wrote:
> Hello Jyri,
>
> Yes using or not DRM bridge we should be able to have a common
> implementation
>
> Please find,my answer belows
>
> BR,
> Arnaud
>
> On 09/25/2015 04:11 PM, Jyri Sarha wrote:
>> Despite my earlier comment this implementation and the related HW is
>> quite similar in all significant aspects to the patch set posted couple
>> of days ago [1] for Beaglebone-Black HDMI audio.
>>
>> [1] http://permalink.gmane.org/gmane.linux.alsa.devel/144144
> yes i trying to align my dev on it. to match with your development.
> Aim for me is to reuse it and adapt it using a DRM bridge interface.
> i hope to provide a V2 next week.
>>
>> I have not yet gotten to bottom of drm-side audio bride part, but I am
>> working on it. Bellow is couple of early comments to the ASoC part.
>>
>> Best regards,
>> Jyri
>>
>> On 09/21/15 16:19, Arnaud Pouliquen wrote:
>>> +
>>> +static int st_hdmi_dai_probe(struct snd_soc_dai *dai)
>>> +{
>>> +    struct hdmi_drm_dai_data *priv;
>>> +
>>> +    dev_err(dai->dev, "%s: enter\n", __func__);
>>> +    priv = devm_kzalloc(dai->dev, sizeof(*priv), GFP_KERNEL);
>>> +
>>> +    priv->bridge = of_drm_find_bridge(dai->dev->of_node);
>>> +
>>> +    dev_err(dai->dev, "%s: bridge %p\n", __func__, priv->bridge);
>>> +
>>> +    snd_soc_dai_set_drvdata(dai, priv);
>>> +
>>
>> The call above overwrites the private data pointer of the drivers that
>> registering the codec. This hardly works in general.
>>
>> A separate platform driver - with this already merged patch [2] - that I
>> use with my patch-set solves this issue quite nicely.
>>
>> [2] http://lists.freedesktop.org/archives/dri-devel/2015-May/083517.html
> Yes same dev,(but no crash...?).i need to define sub node.
>>> +    return 0;
>>> +}here are several important callbacks missing here
>>> +
>>> +static const struct snd_soc_dai_ops hdmi_drm_codec_ops = {
>>> +        .prepare =  hdmi_drm_dai_prepare,
>>> +        .trigger = hdmi_drm_dai_trigger,
>>> +};
>>
>>
>> At least set_daifmt() and hw_params() callbacks should be defined before
>> this could be generally usable. HDMI encoders do not usually support too
>> many daifmts, but the driver should be able the check that the selected
>> format is supported. But as you said this not complete code yet.
> I'm trying to match codec ops with following DRM audio bridge ops,
> that is similar to the existing drm_bridge_funcs structure.

I am not yet too familiar with drm way of doing things. My code is 
trying to follow the way how ALSA does things. I tried to survive with 
as few callback as possible, but if you think more is needed I can add 
those if there is a corresponding callback in ALSA.

> struct drm_audio_bridge_funcs {
>      void (*disable)(struct drm_bridge *bridge);

There is no such thing in my HDMI codec. However, there is digital_mute 
callback that is used by alsa before the streams are shut down to avoid 
undesired pops and clicks.

>      void (*post_disable)(struct drm_bridge *bridge);

Post_disable should map more or less directly to audio_shutdown() in my 
code.

>      void (*pre_enable)(struct drm_bridge *bridge);

audio_startup() and hw_params() should both be called at pre_enable() 
phase.

>      void (*enable)(struct drm_bridge *bridge);

... or one could see hw_params() to map to enable. And there is 
digital_mute which is toggled by ALSA at this phase.

>      int  (*mode_set)(struct drm_bridge *bridge,
>              struct hdmi_audio_mode *mode);

Actually hw_params() does pretty much the same thing as set_mode(), but 
it should be called after audio_startup() has been called.

>      uint8_t *(*mode_get)(struct drm_bridge *bridge); /*return eld*/
> };

For this there is get_eld() in my HDMI codec code.

> audio parameters should be part of struct hdmi_audio_mode that contains
> audio configurations ( info frame,iec, format, clk...)
>
>

BTW, the HDMI codec is made in such a way that one can get by with only 
hw_params() and audio_shutdown(). In such an implementation hw_params() 
sets the HDMI encoder ready for receiving i2s or spdif from CPU DAI and 
audio_shutdown() disables the audio stream.

Best regards,
Jyri