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

From: Jyri Sarha <jsarha@ti.com>
To: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
	peter.ujfalusi@ti.com, tomi.valkeinen@ti.com,
	dri-devel@lists.freedesktop.org, liam.r.girdwood@linux.intel.com,
	tony@atomide.com, broonie@kernel.org, bcousson@baylibre.com,
	linux-omap@vger.kernel.org
Subject: Re: [PATCH 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding
Date: Fri, 5 Aug 2016 12:02:39 +0300	[thread overview]
Message-ID: <3a3dcfa4-cec8-53f8-eddb-9a0e04c10ac9@ti.com> (raw)
In-Reply-To: <20160804140712.GO5783@n2100.arm.linux.org.uk>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel