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
