[alsa-devel] [PATCH v13 3/3] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

On Mon, 20 Jul 2015 19:06:06 +0100
Mark Brown <broonie at 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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/