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