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

[Jean-Francois Moine <moinejf@free.fr>]

On Tue, 28 Jul 2015 11:24:10 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Tue, Jul 28, 2015 at 12:19:45PM +0200, Jean-Francois Moine wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > > +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?
> 
> What I said above - put them in the DRM driver.  They're just
> registering a device IIRC.

There is no CODEC device. The HDMI device supports both audio and video.

> > > > +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.
> 
> Just make this like any other driver with no default specified.

If I understand correctly, you want that the user might choose to use
the HDMI for video only and to have audio by some other mean.
No problem.

> > > > +/* 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.
> 
> Someone might put two chips on the board.

The pointer was from the tda998x codec to the tda998x driver.
There might be any number of chips on the board.

> > > > +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.
> 
> That's not the issue here - the issue is that the need to get the ELD
> out of the video subsystem is not specific to this driver, it's pretty
> common and something that it seems we should factor out.

Yes, but how?

The EDID arrives in the DRM connector when video starts. The built ELD
may be stored either in the connector itself (default), in the encoder
(TDA998x device) or in some DRM/encoder/connector private data.

On the audio side, the CODEC is supported by a driver which is either a
CODEC driver (as in the dummy HDMI codec) or the DRM encoder (TDA998x
device).

You may note that, in DRM, there is no relation between the encoder and
the connector except at video startup time, and no relation between the
DRM connector and the audio CODEC (no CODEC information is returned on
CODEC creation and the CODEC has no private data).

I tried many solutions to solve this problem, and the one of may latest
patchset (v14) seems the simpleset: set the audio/video common part at
the beginning of the TDA998x (HDMI) private data.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel