| Message ID | e036c88aa945e72b40ec965c9358dacf564e79f2.1437397272.git.moinejf@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Mon, 20 Jul 2015 19:06:06 +0100 Mark Brown <broonie@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.
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. > > > +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.
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.
On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote: > 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. The ELD is stored in the DRM connector, and there _should_ be a system of locking which ensures that you can get at that information safely. The ELD is only updated when the connector's get_modes() method is called, and the driver itself calls drm_edid_to_eld(). This all happens under the drm_device's struct_mutex. So, to safely read the ELD from outside DRM, you need to take the top-level drm_device's struct_mutex. That could be fraught, as that's quite a big lock, so an alternative would be to introduce an 'eld' lock to the driver, which protects the call to drm_edid_to_eld() and the reading of the ELD. However, that doesn't solve the problem of passing the data to an audio driver. What's needed there is a notification system which allows a video driver to broadcast HDMI events such as: * connected * new EDID available * new ELD available * disconnected With such a system, different components driving the facilities of a HDMI connector can make decisions on what to do - which not only includes things like the audio driver, but also a driver for the CEC interface (which also needs to see the EDID to get at its "address".) This can be done in a safe manner as the HDMI video driver would have control over the generation of these messages. The point that Mark is trying to get you to see is that this problem is not specific to TDA998x. The same problem exists for many other HDMI interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware mechanism of passing the ELD data from the video driver, through the hardware to the audio driver.) It needs solving in a driver-agnostic way, and the normal way that happens is when someone comes along, wanting to add support in that area, they get to do the hard work to propose that infrastructure. > 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). In the case of the TDA998x, there is a 1:1 fixed relationship between the connector and the encoder. The connector part can't be used with any other encoder, and the encoder part can't be used with any other connector. The same is true of many other HDMI implementations (such as the Synopsis Designware HDMI interface found on iMX6 and Rockchip.)
On Tue, 28 Jul 2015 15:53:58 +0200, Russell King - ARM Linux wrote: > > On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote: > > 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. > > The ELD is stored in the DRM connector, and there _should_ be a system > of locking which ensures that you can get at that information safely. > > The ELD is only updated when the connector's get_modes() method is called, > and the driver itself calls drm_edid_to_eld(). This all happens under > the drm_device's struct_mutex. > > So, to safely read the ELD from outside DRM, you need to take the top-level > drm_device's struct_mutex. That could be fraught, as that's quite a big > lock, so an alternative would be to introduce an 'eld' lock to the driver, > which protects the call to drm_edid_to_eld() and the reading of the ELD. > > However, that doesn't solve the problem of passing the data to an audio > driver. What's needed there is a notification system which allows a video > driver to broadcast HDMI events such as: > > * connected > * new EDID available > * new ELD available > * disconnected > > With such a system, different components driving the facilities of a HDMI > connector can make decisions on what to do - which not only includes things > like the audio driver, but also a driver for the CEC interface (which also > needs to see the EDID to get at its "address".) This can be done in a safe > manner as the HDMI video driver would have control over the generation of > these messages. > > The point that Mark is trying to get you to see is that this problem is > not specific to TDA998x. The same problem exists for many other HDMI > interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware > mechanism of passing the ELD data from the video driver, through the > hardware to the audio driver.) FWIW, we're currently discussing about extending the i915/hda component binding so that the audio driver gets a notification and queries the ELD via callbacks instead of the flaky hardware access. It'd be best if we have a common infrastructure for that, of course. But currently it's a start, just a bit step forward there. Takashi
On Tue, Jul 28, 2015 at 03:59:45PM +0200, Takashi Iwai wrote: > FWIW, we're currently discussing about extending the i915/hda > component binding so that the audio driver gets a notification and > queries the ELD via callbacks instead of the flaky hardware access. > > It'd be best if we have a common infrastructure for that, of course. > But currently it's a start, just a bit step forward there. Okay, so it sounds like i915/hda also needs this. I think in addition to what I said above, we need whatever provides the notification system to generate state notifications for new "listeners" that would otherwise be unaware of the current state. I'm thinking at the moment that something along these lines to go into drivers/video/hdmi.c / include/linux/hdmi.h: struct hdmi_state { struct mutex mutex; struct raw_notifier_head head; bool connected; bool edid_valid; u8 edid[MAX_EDID_SIZE]; }; enum { HDMI_CONNECTED, HDMI_DISCONNECTED, HDMI_NEW_EDID, }; int hdmi_register_notifier(struct notifier_block *nb) { struct hdmi_state *state = ...; int ret; mutex_lock(&state->mutex); if (state->connected) { nb->notifier_call(nb, HDMI_CONNECTED, NULL); if (state->edid_valid) nb->notifier_call(nb, HDMI_NEW_EDID, state->edid); } else { nb->notifier_call(nb, HDMI_DISCONNECTED, NULL); } ret = raw_notifier_chain_register(&state->head, nb); mutex_unlock(&state->mutex); return ret; } int hdmi_unregister_notifier(struct notifier_block *nb) { struct hdmi_state *state = ...; int ret; mutex_lock(&state->mutex); ret = raw_notifier_chain_unregister(&state->head, nb); mutex_unlock(&state->mutex); return ret; } void hdmi_event_connect(struct device *dev) { struct hdmi_state *state = lookup_state_from_dev(dev); mutex_lock(&state->mutex); state->connected = true; raw_notifier_call_chain(&state->head, HDMI_CONNECTED, NULL); mutex_unlock(&state->mutex); } void hdmi_event_disconnect(struct device *dev) { struct hdmi_state *state = lookup_state_from_dev(dev); mutex_lock(&state->mutex); state->connected = false; state->valid_edid = false; raw_notifier_call_chain(&state->head, HDMI_DISCONNECTED, NULL); mutex_unlock(&state->mutex); } int hdmi_event_new_edid(struct device *dev, const void *edid, size_t size) { struct hdmi_state *state = lookup_state_from_dev(dev); if (size > sizeof(state->edid)) return -EINVAL; mutex_lock(&state->mutex); state->valid_edid = true; memcpy(state->edid, edid, size); raw_notifier_call_chain(&state->head, HDMI_NEW_EDID, state->edid); mutex_unlock(&state->mutex); return 0; } The problems here are: how to go from a struct device to the state (iow, the implementation of lookup_state_from_dev()) and how to convey on the client side which 'state' to attach to. I'd rather not involve DT at this level: DT is not the world's only firmware system, we have to cater for x86 too here, so we need something that is firmware agnostic. This is something I've just banged out in this email...
On Tue, 28 Jul 2015 14:53:58 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote: > > 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. > > The ELD is stored in the DRM connector, and there _should_ be a system > of locking which ensures that you can get at that information safely. > > The ELD is only updated when the connector's get_modes() method is called, > and the driver itself calls drm_edid_to_eld(). This all happens under > the drm_device's struct_mutex. > > So, to safely read the ELD from outside DRM, you need to take the top-level > drm_device's struct_mutex. That could be fraught, as that's quite a big > lock, so an alternative would be to introduce an 'eld' lock to the driver, > which protects the call to drm_edid_to_eld() and the reading of the ELD. I think that my solution should work: - the CODEC uses a pointer in the driver private data to the ELD. - when get_modes() is called, this pointer is reset to NULL. (no audio streaming is permitted). - this function reads the EDID and this asks for hardware exchanges with the HDMI device. - the EDID is then translated to ELD and the ELD pointer is set (audio streaming is permitted again). Then, if audio streaming is started before get_modes() is called, the memory treatment of the ELD may be safely done during the harware exchanges. > However, that doesn't solve the problem of passing the data to an audio > driver. What's needed there is a notification system which allows a video > driver to broadcast HDMI events such as: > > * connected > * new EDID available > * new ELD available > * disconnected My patch just wants to offer basic audio to the TDA998x. Especially, the display device state is of no importance: audio streaming may continue while there is no connected device. > With such a system, different components driving the facilities of a HDMI > connector can make decisions on what to do - which not only includes things > like the audio driver, but also a driver for the CEC interface (which also > needs to see the EDID to get at its "address".) This can be done in a safe > manner as the HDMI video driver would have control over the generation of > these messages. > > The point that Mark is trying to get you to see is that this problem is > not specific to TDA998x. The same problem exists for many other HDMI > interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware > mechanism of passing the ELD data from the video driver, through the > hardware to the audio driver.) I know that, but I don't know enough about all the DRM and ASoC drivers to propose a global mechanism. > It needs solving in a driver-agnostic way, and the normal way that happens > is when someone comes along, wanting to add support in that area, they get > to do the hard work to propose that infrastructure. OK. I'll stop here. > > 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). > > In the case of the TDA998x, there is a 1:1 fixed relationship between the > connector and the encoder. The connector part can't be used with any other > encoder, and the encoder part can't be used with any other connector. The > same is true of many other HDMI implementations (such as the Synopsis > Designware HDMI interface found on iMX6 and Rockchip.) But there is no direct link (pointer) between the encoder and the connector.
On Tue, Jul 28, 2015 at 04:39:12PM +0200, Jean-Francois Moine wrote: > On Tue, 28 Jul 2015 14:53:58 +0100 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > In the case of the TDA998x, there is a 1:1 fixed relationship between the > > connector and the encoder. The connector part can't be used with any other > > encoder, and the encoder part can't be used with any other connector. The > > same is true of many other HDMI implementations (such as the Synopsis > > Designware HDMI interface found on iMX6 and Rockchip.) > > But there is no direct link (pointer) between the encoder and the > connector. There's no direct link from an encoder to a connector because the DRM model allows a single encoder to be associated with multiple connectors. Adding a link from the encoder to a connector breaks that ability (an encoder would then need an array of connectors.) If we kill the old drm_slave_encoder support in TDA998x, then this would solve the problem - we can then get at the TDA998x's drm_connector easily as it then becomes part of TDA998x's private data. However, this doesn't matter. All that we need from the DRM side of the TDA998x is to know is the EDID or ELD. That can be broadcast via a notification (using something like the code I illustrated in a follow up email) to any listeners, whether they be an audio driver or a CEC bus driver. The audio driver may not care about HDMI connect/disconnect events, but that's no reason not to include them. Some consumers of these events need to know (CEC certainly needs to know.)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 655ebb0..90a508f 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -758,6 +758,66 @@ tda998x_configure_audio(struct tda998x_priv *priv, tda998x_write_aif(priv, p); } +#if IS_ENABLED(CONFIG_SND_SOC_TDA998X) +/* tda998x audio codec interface */ + +/* return the ELD to the CODEC */ +static u8 *tda998x_get_eld(struct device *dev) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + struct drm_encoder *encoder = priv->encoder; + struct drm_device *drm = encoder->dev; + struct drm_connector *connector; + + if (!priv->is_hdmi_sink + || !encoder->crtc) + return NULL; + + list_for_each_entry(connector, &drm->mode_config.connector_list, head) { + if (connector->encoder == encoder) + return connector->eld; + } + return NULL; +} + +/* switch the audio port and initialize the audio parameters for streaming */ +static int tda998x_set_audio_input(struct device *dev, + int port_index, + unsigned sample_rate) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + struct tda998x_encoder_params *p = &priv->params; + + if (!priv->encoder->crtc) + return -ENODEV; + + /* if no port, just disable the audio port */ + if (port_index == PORT_NONE) { + reg_write(priv, REG_ENA_AP, 0); + return 0; + } + + /* if same audio parameters, just enable the audio port */ + if (p->audio_cfg == priv->audio.ports[port_index] && + p->audio_sample_rate == sample_rate) { + reg_write(priv, REG_ENA_AP, p->audio_cfg); + return 0; + } + + p->audio_format = priv->audio.port_types[port_index]; + p->audio_clk_cfg = p->audio_format == AFMT_SPDIF ? 0 : 1; + p->audio_cfg = priv->audio.ports[port_index]; + p->audio_sample_rate = sample_rate; + tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p); + return 0; +} + +static struct tda998x_ops_s tda998x_codec_ops = { + .get_eld = tda998x_get_eld, + .set_audio_input = tda998x_set_audio_input, +}; +#endif /* SND_SOC */ + /* DRM encoder functions */ static void tda998x_encoder_set_config(struct tda998x_priv *priv, @@ -1123,6 +1183,12 @@ tda998x_encoder_get_modes(struct tda998x_priv *priv, drm_mode_connector_update_edid_property(connector, edid); n = drm_add_edid_modes(connector, edid); priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); + +#if IS_ENABLED(CONFIG_SND_SOC_TDA998X) + if (priv->is_hdmi_sink) + drm_edid_to_eld(connector, edid); +#endif + kfree(edid); return n; @@ -1158,6 +1224,10 @@ static void tda998x_destroy(struct tda998x_priv *priv) cancel_delayed_work_sync(&priv->dwork); } +#if IS_ENABLED(CONFIG_SND_SOC_TDA998X) + if (priv->audio.ports[0]) + tda9998x_codec_unregister(&priv->hdmi->dev); +#endif i2c_unregister_device(priv->cec); } @@ -1294,6 +1364,9 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5); + priv->params.audio_frame[1] = 1; /* channels - 1 */ + priv->params.audio_sample_rate = 48000; /* 48kHz */ + priv->current_page = 0xff; priv->hdmi = client; /* CEC I2C address bound to TDA998x I2C addr by configuration pins */ @@ -1407,6 +1480,13 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) priv->params.audio_clk_cfg = priv->params.audio_format == AFMT_SPDIF ? 0 : 1; + +#if IS_ENABLED(CONFIG_SND_SOC_TDA998X) + /* and create the audio CODEC */ + tda9998x_codec_register(&client->dev, + &priv->audio, + &tda998x_codec_ops); +#endif } } else { diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h index 487a809..8e89db4 100644 --- a/include/sound/tda998x.h +++ b/include/sound/tda998x.h @@ -1,8 +1,23 @@ #ifndef SND_TDA998X_H #define SND_TDA998X_H +/* port index for audio stream stop */ +#define PORT_NONE (-1) + struct tda998x_audio_s { u8 ports[2]; /* AP value */ u8 port_types[2]; /* AFMT_xxx */ }; + +struct tda998x_ops_s { + u8 *(*get_eld)(struct device *dev); + int (*set_audio_input)(struct device *dev, + int port_index, + unsigned sample_rate); +}; + +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); #endif diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index efaafce..08b73bd 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -105,6 +105,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_TAS2552 if I2C select SND_SOC_TAS5086 if I2C select SND_SOC_TAS571X if I2C + select SND_SOC_TDA998X if DRM_I2C_NXP_TDA998X select SND_SOC_TFA9879 if I2C select SND_SOC_TLV320AIC23_I2C if I2C select SND_SOC_TLV320AIC23_SPI if SPI_MASTER @@ -622,6 +623,11 @@ config SND_SOC_TAS571X tristate "Texas Instruments TAS5711/TAS5717/TAS5719 power amplifiers" depends on I2C +config SND_SOC_TDA998X + def_tristate y + select SND_PCM_ELD + depends on DRM_I2C_NXP_TDA998X + config SND_SOC_TFA9879 tristate "NXP Semiconductors TFA9879 amplifier" depends on I2C diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index cf160d9..819d689 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -108,6 +108,7 @@ snd-soc-sta529-objs := sta529.o snd-soc-stac9766-objs := stac9766.o snd-soc-tas5086-objs := tas5086.o snd-soc-tas571x-objs := tas571x.o +snd-soc-tda998x-objs := tda998x.o snd-soc-tfa9879-objs := tfa9879.o snd-soc-tlv320aic23-objs := tlv320aic23.o snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o @@ -292,6 +293,7 @@ obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o +obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c new file mode 100644 index 0000000..f588a75 --- /dev/null +++ b/sound/soc/codecs/tda998x.c @@ -0,0 +1,132 @@ +/* + * ALSA SoC TDA998x CODEC + * + * Copyright (C) 2015 Jean-Francois Moine + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/module.h> +#include <sound/soc.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <sound/pcm_drm_eld.h> +#include <drm/i2c/tda998x.h> +#include <sound/tda998x.h> + +/* functions in tda998x_drv */ +static struct tda998x_ops_s *tda998x_ops; + +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); +} + +/* 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)); +} + +static void tda998x_codec_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + tda998x_ops->set_audio_input(dai->dev, PORT_NONE, 0); +} + +static const struct snd_soc_dai_ops tda998x_codec_ops = { + .startup = tda998x_codec_startup, + .hw_params = tda998x_codec_hw_params, + .shutdown = tda998x_codec_shutdown, +}; + +#define TDA998X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S20_3LE | \ + SNDRV_PCM_FMTBIT_S24_LE | \ + SNDRV_PCM_FMTBIT_S32_LE) + +static struct snd_soc_dai_driver tda998x_dai_i2s = { + .name = "i2s-hifi", + .playback = { + .stream_name = "HDMI I2S Playback", + .channels_min = 1, + .channels_max = 8, + .rates = SNDRV_PCM_RATE_CONTINUOUS, + .rate_min = 5512, + .rate_max = 192000, + .formats = TDA998X_FORMATS, + }, + .ops = &tda998x_codec_ops, +}; + +static const struct snd_soc_dapm_widget tda998x_widgets[] = { + SND_SOC_DAPM_OUTPUT("hdmi-out"), +}; +static const struct snd_soc_dapm_route tda998x_routes[] = { + { "hdmi-out", NULL, "HDMI I2S Playback" }, + { "hdmi-out", NULL, "HDMI SPDIF Playback" }, +}; + +static struct snd_soc_codec_driver tda998x_codec_drv = { + .dapm_widgets = tda998x_widgets, + .num_dapm_widgets = ARRAY_SIZE(tda998x_widgets), + .dapm_routes = tda998x_routes, + .num_dapm_routes = ARRAY_SIZE(tda998x_routes), + .ignore_pmdown_time = true, +}; + +int tda9998x_codec_register(struct device *dev, + struct tda998x_audio_s *tda998x_audio, + struct tda998x_ops_s *tda998x_ops_i) +{ + struct snd_soc_dai_driver *dais, *p_dai; + int i, ndais; + + tda998x_ops = tda998x_ops_i; + + /* build the DAIs */ + for (ndais = 0; ndais < ARRAY_SIZE(tda998x_audio->ports); ndais++) { + if (!tda998x_audio->ports[ndais]) + break; + } + dais = devm_kzalloc(dev, sizeof(*dais) * ndais, GFP_KERNEL); + if (!dais) + return -ENOMEM; + 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; + } + } + + return snd_soc_register_codec(dev, + &tda998x_codec_drv, + dais, ndais); +} +EXPORT_SYMBOL(tda9998x_codec_register); + +void tda9998x_codec_unregister(struct device *dev) +{ + snd_soc_unregister_codec(dev); +} +EXPORT_SYMBOL(tda9998x_codec_unregister); + +MODULE_AUTHOR("Jean-Francois Moine <moinejf@free.fr>"); +MODULE_DESCRIPTION("TDA998X CODEC"); +MODULE_LICENSE("GPL");
The tda998x CODEC maintains the audio constraints according to the HDMI device parameters (EDID) and sets dynamically the input ports in the TDA998x I2C driver on start/stop audio streaming. Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- drivers/gpu/drm/i2c/tda998x_drv.c | 80 +++++++++++++++++++++++ include/sound/tda998x.h | 15 +++++ sound/soc/codecs/Kconfig | 6 ++ sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tda998x.c | 132 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 235 insertions(+) create mode 100644 sound/soc/codecs/tda998x.c