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

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]

On Tue, Jul 28, 2015 at 12:19:45PM +0200, Jean-Francois Moine wrote:
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

> > > +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.

> > > +/* 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.

> > > +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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]