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.
> + 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?
> +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.
> +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.
> +config SND_SOC_TDA998X
> + def_tristate y
> + select SND_PCM_ELD
> + depends on DRM_I2C_NXP_TDA998X
> +
def_tristate y? Why?
> +/* 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?
> +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.
> +/* 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.
> + 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.
> + 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.
Attachment:
signature.asc
Description: Digital signature
