[PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Jyri Sarha]

On 12/30/2014 07:00 PM, Mark Brown wrote:
> On Mon, Dec 29, 2014 at 07:37:21PM +0200, Jyri Sarha wrote:
>> On 12/29/2014 06:52 PM, Mark Brown wrote:
>
>>> So, I'm not seeing *any* interest here from any other HDMI users.  This
>>> is a continuing theme with HDMI patches and is really very concerning,
>>> everyone appears to be working in their own bubbles coming up with their
>>> own things and ignoring everyone else's work - what little review I'm
>
>> I have not seen any significant new development since v7 of these patches.
>> My comments for v6 were mostly[1] addressed and I can live with these
>> changes, even develop this approach further if it gets merged.
>
> OK, so this sort of feedback is really useful - even a qualified
> Reviwed-by is useful.  Total silence could mean anything.
>
>> However, as a general note I see a need for a generic ASoC hdmi codec
>> abstraction and I don't think this is generic enough. More of the audio
>> specific implementation and HDMI standard specific things should be pushed
>> away from the hdmi encoder driver (tda998x in this case) to the generic ASoC
>> side hdmi codec driver (or library).
>
> This is something I'm expecting, yes - what I don't have is a clear
> enough picture of how consistent the different hardware is in how it
> models these things.
>

After taking another look the patchesthere is not much functionality 
that could be moved away from tda998x. It is just that the ASoC side 
(and tda998x) could do more in terms of HDMI standard, but that is fine 
because all that stuff can be added later if/when needed. However, 
instantiating snd_soc_dai_driver etc. in DRM side makes a nasty 
dependency between DRM and ASoC. All that stuff could be in ASoC side 
and simple enum/bitfield parameters defining i2s/spdif selection could 
be passed between DRM and ASoC side.


>> [1] I personally do not like the hdmi_get_cdev() approach. I would rather go
>> with only a library for registering from ASoC codec component under the HDMI
>> encoder device or a completely separate device with only a reference to the
>> HDMI encoder.
>
> It does seem somewhat complicated, yes.  I don't know if matches idioms
> for DRM somehow?
>

Me neither. A more straight forward option would be adding an "audio 
header" to the beginning of hdmi encoders drvdata. However, it would be 
tempting to add a pointer for drvdata to snd_soc_dai_driver to add audio 
specific driver data to any device that provides a DAI in addition to 
other functionality. It could even simplify drivers for some audio only 
devices with multiple DAIs.

Best regards,
Jyri

[PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

On Mon, Dec 29, 2014 at 07:37:21PM +0200, Jyri Sarha wrote:
> On 12/29/2014 06:52 PM, Mark Brown wrote:

> >So, I'm not seeing *any* interest here from any other HDMI users.  This
> >is a continuing theme with HDMI patches and is really very concerning,
> >everyone appears to be working in their own bubbles coming up with their
> >own things and ignoring everyone else's work - what little review I'm

> I have not seen any significant new development since v7 of these patches.
> My comments for v6 were mostly[1] addressed and I can live with these
> changes, even develop this approach further if it gets merged.

OK, so this sort of feedback is really useful - even a qualified
Reviwed-by is useful.  Total silence could mean anything.

> However, as a general note I see a need for a generic ASoC hdmi codec
> abstraction and I don't think this is generic enough. More of the audio
> specific implementation and HDMI standard specific things should be pushed
> away from the hdmi encoder driver (tda998x in this case) to the generic ASoC
> side hdmi codec driver (or library).

This is something I'm expecting, yes - what I don't have is a clear
enough picture of how consistent the different hardware is in how it
models these things.

> [1] I personally do not like the hdmi_get_cdev() approach. I would rather go
> with only a library for registering from ASoC codec component under the HDMI
> encoder device or a completely separate device with only a reference to the
> HDMI encoder.

It does seem somewhat complicated, yes.  I don't know if matches idioms
for DRM somehow?
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: 

[PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Jyri Sarha]

On 12/29/2014 06:52 PM, Mark Brown wrote:
> On Thu, Oct 23, 2014 at 10:32:49AM +0200, Jean-Francois Moine wrote:
>> The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
>> from 2 different sources, I2S and S/PDIF.
>
> So, I'm not seeing *any* interest here from any other HDMI users.  This
> is a continuing theme with HDMI patches and is really very concerning,
> everyone appears to be working in their own bubbles coming up with their
> own things and ignoring everyone else's work - what little review I'm
> seeing seems to be happening only after I explicitly prompt it.  I'm
> following up to Jean-Francois' patches here but this isn't specific to
> them, it seems like a general thing with HDMI code.
>
> This in turn makes me think there's some abstraction problems with
> what's going on and we're going to have to go through yet more
> refactorings to fix things up as we do manage to come up with better
> abstractions.  What I'd really like to see as a bare minimum is some
> visible conversation about what we're doing and sign that people are
> at least keeping in mind generic solutions when working on HDMI code.
> Some commentary on the similarities and differences between hardware
> and which abstractions work with which devices would also be really
> helpful in working out if we're going in the right direction.
>
> Basically at the minute I'm worried that we may be making the problems
> we've got here worse not better, I've not personally had the time to sit
> down and study the hardware sufficiently to form a firm impression
> myself which isn't helping.
>

I have not seen any significant new development since v7 of these 
patches. My comments for v6 were mostly[1] addressed and I can live with 
these changes, even develop this approach further if it gets merged.

However, as a general note I see a need for a generic ASoC hdmi codec 
abstraction and I don't think this is generic enough. More of the audio 
specific implementation and HDMI standard specific things should be 
pushed away from the hdmi encoder driver (tda998x in this case) to the 
generic ASoC side hdmi codec driver (or library).

I am currently working on something that tries to be a generic solution 
that should be usable for different encoders and platforms, but 
unfortunately I have been busy with other things lately and have not 
been able to put anything working together yet. As soon as I have at 
least a bare bone API and and a proof of concept implementation working, 
I send RFC/POF patches out.

Best regards,
Jyri

[1] I personally do not like the hdmi_get_cdev() approach. I would 
rather go with only a library for registering from ASoC codec component 
under the HDMI encoder device or a completely separate device with only 
a reference to the HDMI encoder.

[PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

On Thu, Oct 23, 2014 at 10:32:49AM +0200, Jean-Francois Moine wrote:
> The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
> from 2 different sources, I2S and S/PDIF.

So, I'm not seeing *any* interest here from any other HDMI users.  This
is a continuing theme with HDMI patches and is really very concerning,
everyone appears to be working in their own bubbles coming up with their
own things and ignoring everyone else's work - what little review I'm
seeing seems to be happening only after I explicitly prompt it.  I'm
following up to Jean-Francois' patches here but this isn't specific to
them, it seems like a general thing with HDMI code.

This in turn makes me think there's some abstraction problems with
what's going on and we're going to have to go through yet more
refactorings to fix things up as we do manage to come up with better
abstractions.  What I'd really like to see as a bare minimum is some
visible conversation about what we're doing and sign that people are
at least keeping in mind generic solutions when working on HDMI code.
Some commentary on the similarities and differences between hardware
and which abstractions work with which devices would also be really
helpful in working out if we're going in the right direction.

Basically at the minute I'm worried that we may be making the problems
we've got here worse not better, I've not personally had the time to sit
down and study the hardware sufficiently to form a firm impression
myself which isn't helping.
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: 

[PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
from 2 different sources, I2S and S/PDIF.

This patch set first adds an interface between a HDMI transmitter and
the HDMI CODEC.

The interface is then used by the TDA998x driver to describe its audio
capabilities (DAIs), to give the audio constraints of the HDMI
device (EDID) to the audio subsystem, and to connect the chosen audio
source to the HDMI link.

v8:
- change some comments about the patches
v7:
- remove the change of the K predivider (Jyri Sarha)
- add S24_3LE and S32_LE as possible audio formats (Jyri Sarha)
- don't move the struct priv2 definition and use the
  slave encoder private data as the device private data
  (Russell King)
- remove the useless request_module (Russell King/Mark Brown)
- don't lock the HDMI module (Russell King)
- use platform_device_unregister to remove the codec
  (Russell King)
v6:
- extend the HDMI CODEC instead of using a specific CODEC
v5:
- use the TDA998x private data instead of a specific area
  for the CODEC interface
- the CODEC is TDA998x specific (Mark Brown)
v4:
- remove all the TDA998x specific stuff from the CODEC
- move the EDID scan from the CODEC to the TDA998x
- move the CODEC to sound/soc (Mark Brown)
- update the audio_sample_rate from the EDID (Andrew Jackson)
v3: fix bad rate (Andrew Jackson)
v2: check double stream start (Mark Brown)

Jean-Francois Moine (2):
  ASoC: hdmi: Add a transmitter interface to the HDMI CODEC
  drm/i2c: tda998x: Use the HDMI audio CODEC interface

 .../devicetree/bindings/drm/i2c/tda998x.txt|  18 ++
 drivers/gpu/drm/i2c/Kconfig|   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c  | 264 -
 include/sound/hdmi.h   |  20 ++
 sound/soc/codecs/hdmi.c| 176 +-
 5 files changed, 460 insertions(+), 19 deletions(-)
 create mode 100644 include/sound/hdmi.h

-- 
2.1.1