[alsa-devel] [PATCH 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Jyri Sarha]

On 08/04/16 17:07, Russell King - ARM Linux wrote:
> On Tue, Aug 02, 2016 at 03:05:08PM +0300, Jyri Sarha wrote:
>> +	memcpy(audio.status, params->iec.status,
>> +	       min(sizeof(audio.status), sizeof(params->iec.status)));
> 
> As mentioned in the other patch, the audio status does not directly
> correspond with the AES bytes, so this ends up causing the driver to
> write the wrong bytes to the wrong registers.
> 

As I said earlier, I'd rather have it as plain AES/IEC958 channel status
bits buffer.

>> +	ret = tda998x_configure_audio(priv,
>> +				      &audio,
>> +				      priv->encoder.crtc->hwmode.clock);
> 
> What happens if audio changes at the same time that the video mode
> changes?  What protection is there against two threads concurrently
> executing tda998x_configure_audio() ?
> 

Oh, yes. I definitely need some locking here. The same lock could
protect the atomicity of tda998x_audio_params update and usage.

>> +	priv->audio_pdev = platform_device_register_data(
>> +		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
>> +		&codec_data, sizeof(codec_data));
> 
> I'd much prefer that this was a child of the I2C device, which will
> show the relationship between this virtual platform device and the
> device which it's associated with.  That can be done via
> platform_device_register_full().
> 

That may be a problem. The ASoC card device tree binding current looks
for the ASoC DAI's parent's of-node if it can not find the node for the
DAI-device itself. Indicating ASoC DAI-link by referencing the parent
i2c device simply won't work, because there may be other ASoC codecs on
the same i2c bus. Adding a separate DT-node for a virtual audio device
should be possible, but it needs some thinking and may have its own
problems. I can not follow the reasoning behind this, is this really
necessary?

Best regards,
Jyri