On Mon, 20 Jul 2015 19:06:06 +0100 Mark Brown broonie@kernel.org wrote:
On Fri, May 08, 2015 at 10:41:12AM +0200, Jean-Francois Moine wrote:
- if (!priv->is_hdmi_sink
|| !encoder->crtc)return NULL;That's weird indentation.
But the conditions are aligned... This sequence will be removed.
- list_for_each_entry(connector, &drm->mode_config.connector_list, head) {
if (connector->encoder == encoder)return connector->eld;- }
What guarantees that connector->eld stays valid after it's returned?
You are right, the ELD content may change. I will use a pointer to a stable content.
+struct tda998x_ops_s {
- u8 *(*get_eld)(struct device *dev);
Why _ops_s - what does the _s mean? If it's sound perhaps it makes sense to spell it out so people aren't guessing.
I will remove it.
+int tda9998x_codec_register(struct device *dev,
struct tda998x_audio_s *tda998x_audio_i,struct tda998x_ops_s *tda998x_ops);+void tda9998x_codec_unregister(struct device *dev);
I'd expect these to be internal to the DRM driver.
I don't see where. What is your idea?
+config SND_SOC_TDA998X
- def_tristate y
- select SND_PCM_ELD
- depends on DRM_I2C_NXP_TDA998X
def_tristate y? Why?
The TDA998x CODEC is always generated when the DRM TDA998x is generated and when audio is wanted. Its type, built-in or module, depends on the TDA998x driver type.
+/* functions in tda998x_drv */ +static struct tda998x_ops_s *tda998x_ops;
I'd expect this to be stored in the driver data rather than a static global, what if a system has two HDMI outputs?
Each TDA998x has only one HDMI output and there is only one driver.
I will put the pointer to the HDMI driver in the device private area.
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
struct snd_pcm_runtime *runtime = substream->runtime;u8 *eld;eld = tda998x_ops->get_eld(dai->dev);if (!eld)return -ENODEV;return snd_pcm_hw_constraint_eld(runtime, eld);+}
Do we really need a device specific mechanism for fishing the ELD out of the graphics code? I'd have expected this to be more generic.
I will put the ELD in the private data of the DRM driver.
+/* ask the HDMI transmitter to activate the audio input port */ +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- return tda998x_ops->set_audio_input(dai->dev, dai->id,
params_rate(params));+}
The set_audio_input() function doesn't appear to have anything that checks if the device is busy before enabling things, what happens if the user tries to switch between I2S and S/PDIF? It looks like only one DAI can be active at once.
Right. I will check this double streaming.
- for (i = 0, p_dai = dais; i < ndais ; i++, p_dai++) {
memcpy(p_dai, &tda998x_dai_i2s, sizeof(*p_dai));p_dai->id = i;if (tda998x_audio->port_types[i] == AFMT_SPDIF) {p_dai->name = "spdif-hifi";p_dai->playback.stream_name = "HDMI SPDIF Playback";p_dai->playback.channels_max = 2;p_dai->playback.rate_min = 22050;}- }
It would be a bit clearer if the template were just a template and this copying initialised both I2S and S/PDIF specific settings.
OK, I will change this.
- return snd_soc_register_codec(dev,
&tda998x_codec_drv,dais, ndais);+} +EXPORT_SYMBOL(tda9998x_codec_register);
ASoC is all EXPORT_SYMBOL_GPL, you shouldn't reexport functionality with plain EXPORT_SYMBOL.
Sorry, bug of mine.