[v6,2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

This patch interfaces the HDMI transmitter with the audio system.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/drm/i2c/tda998x.txt        |  18 ++
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 299 +++++++++++++++++++--
 3 files changed, 300 insertions(+), 18 deletions(-)

On Wed, Sep 24, 2014 at 10:11:21AM +0200, Jean-Francois Moine wrote:
> This patch interfaces the HDMI transmitter with the audio system.

Please use subject lines corresponding to the style for the subsystem.

> +	request_module("snd-soc-hdmi-codec");
> +	pdev = platform_device_register_resndata(&priv->hdmi->dev,
> +						 "hdmi-audio-codec",
> +						  PLATFORM_DEVID_NONE,
> +						  NULL, 0,
> +						  &tda998x_hdmi_data,
> +						  sizeof tda998x_hdmi_data);

Why is this request_module() needed?  If there is a good reason for it
we should have some documentation.

On Tue, 30 Sep 2014 20:25:40 +0100
Mark Brown <broonie@kernel.org> wrote:

> > +	request_module("snd-soc-hdmi-codec");
> > +	pdev = platform_device_register_resndata(&priv->hdmi->dev,
> > +						 "hdmi-audio-codec",
> > +						  PLATFORM_DEVID_NONE,
> > +						  NULL, 0,
> > +						  &tda998x_hdmi_data,
> > +						  sizeof tda998x_hdmi_data);  
> 
> Why is this request_module() needed?  If there is a good reason for it
> we should have some documentation.

The reason is simple: as the HDMI CODEC is not declared in the DT, the
associated module must be loaded in memory.

On Wed, Oct 01, 2014 at 11:28:46AM +0200, Jean-Francois Moine wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > > +	request_module("snd-soc-hdmi-codec");
> > > +	pdev = platform_device_register_resndata(&priv->hdmi->dev,
> > > +						 "hdmi-audio-codec",
> > > +						  PLATFORM_DEVID_NONE,
> > > +						  NULL, 0,
> > > +						  &tda998x_hdmi_data,
> > > +						  sizeof tda998x_hdmi_data);  

> > Why is this request_module() needed?  If there is a good reason for it
> > we should have some documentation.

> The reason is simple: as the HDMI CODEC is not declared in the DT, the
> associated module must be loaded in memory.

No, that doesn't make sense - there's absolutely no dependency on DT for
module loading, non-DT boards work perfectly fine.  Think about this for
a minute, we don't have lots of request_module() calls in board files.
You're working around some other bug here.

On Wed, Oct 01, 2014 at 11:28:46AM +0200, Jean-Francois Moine wrote:
> On Tue, 30 Sep 2014 20:25:40 +0100
> Mark Brown <broonie@kernel.org> wrote:
> 
> > > +	request_module("snd-soc-hdmi-codec");
> > > +	pdev = platform_device_register_resndata(&priv->hdmi->dev,
> > > +						 "hdmi-audio-codec",
> > > +						  PLATFORM_DEVID_NONE,
> > > +						  NULL, 0,
> > > +						  &tda998x_hdmi_data,
> > > +						  sizeof tda998x_hdmi_data);  
> > 
> > Why is this request_module() needed?  If there is a good reason for it
> > we should have some documentation.
> 
> The reason is simple: as the HDMI CODEC is not declared in the DT, the
> associated module must be loaded in memory.

Module auto-loading works in non-DT environments too.  Unlike the
direction that the arm port is going, the core kernel features, such
as driver autoloading, are coded *not* to require DT (or indeed any
particular firmware.)

There are circumstances where this has been lost sight of (such as
the gpiod stuff), but as a general rule, features do not rely on DT.

Platform drivers will be auto-loaded if they have:

MODULE_ALIAS(PLATFORM_MODULE_PREFIX DRIVER_NAME);

and DRIVER_NAME matches the non-id part of the platform device name.
In the case of platform driver ID tables:

MODULE_DEVICE_TABLE(platform, id-table);

On 09/24/2014 11:11 AM, Jean-Francois Moine wrote:
 > This patch interfaces the HDMI transmitter with the audio system.
 >
 > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
 > ---
 >   .../devicetree/bindings/drm/i2c/tda998x.txt        |  18 ++
 >   drivers/gpu/drm/i2c/Kconfig                        |   1 +
 >   drivers/gpu/drm/i2c/tda998x_drv.c                  | 299 
+++++++++++++++++++--
 >   3 files changed, 300 insertions(+), 18 deletions(-)
 >
 > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt 
b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
 > index e9e4bce..e50e7cd 100644
 > --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
 > +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
 > @@ -17,6 +17,20 @@ Optional properties:
 >     - video-ports: 24 bits value which defines how the video controller
 >   	output is wired to the TDA998x input - default: <0x230145>
 >
 > +  - audio-ports: must contain one or two values selecting the source
 > +	in the audio port.
 > +	The source type is given by the corresponding entry in
 > +	the audio-port-names property.
 > +
 > +  - audio-port-names: must contain entries matching the entries in
 > +	the audio-ports property.
 > +	Each value may be "i2s" or "spdif", giving the type of
 > +	the audio source.
 > +
 > +  - #sound-dai-cells: must be set to <1> for use with the simple-card.
 > +	The TDA998x audio CODEC always defines two DAIs.
 > +	The DAI 0 is the S/PDIF input and the DAI 1 is the I2S input.
 > +
 >   Example:
 >
 >   	tda998x: hdmi-encoder {
 > @@ -26,4 +40,8 @@ Example:
 >   		interrupts = <27 2>;		/* falling edge */
 >   		pinctrl-0 = <&pmx_camera>;
 >   		pinctrl-names = "default";
 > +
 > +		audio-ports = <0x04>, <0x03>;
 > +		audio-port-names = "spdif", "i2s";
 > +		#sound-dai-cells = <1>;
 >   	};
 > diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
 > index 4d341db..42ca744 100644
 > --- a/drivers/gpu/drm/i2c/Kconfig
 > +++ b/drivers/gpu/drm/i2c/Kconfig
 > @@ -22,6 +22,7 @@ config DRM_I2C_SIL164
 >   config DRM_I2C_NXP_TDA998X
 >   	tristate "NXP Semiconductors TDA998X HDMI encoder"
 >   	default m if DRM_TILCDC
 > +	select SND_SOC_HDMI_CODEC
 >   	help
 >   	  Support for NXP Semiconductors TDA998X HDMI encoders.
 >
 > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
 > index d476279..66c41c0 100644
 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
 > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
 > @@ -20,12 +20,14 @@
 >   #include <linux/module.h>
 >   #include <linux/irq.h>
 >   #include <sound/asoundef.h>
 > +#include <linux/platform_device.h>
 >
 >   #include <drm/drmP.h>
 >   #include <drm/drm_crtc_helper.h>
 >   #include <drm/drm_encoder_slave.h>
 >   #include <drm/drm_edid.h>
 >   #include <drm/i2c/tda998x.h>
 > +#include <sound/hdmi.h>
 >
 >   #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 >
 > @@ -44,6 +46,22 @@ struct tda998x_priv {
 >   	wait_queue_head_t wq_edid;
 >   	volatile int wq_edid_wait;
 >   	struct drm_encoder *encoder;
 > +
 > +	/* audio variables */
 > +	struct platform_device *pdev_codec;
 > +	u8 audio_ports[2];
 > +
 > +	u8 max_channels;		/* EDID parameters */
 > +	u8 rate_mask;
 > +	u8 fmt;
 > +
 > +	int audio_sample_format;
 > +};
 > +
 > +struct tda998x_priv2 {
 > +	struct tda998x_priv base;
 > +	struct drm_encoder encoder;
 > +	struct drm_connector connector;
 >   };
 >
 >   #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)->slave_priv)
 > @@ -624,6 +642,8 @@ tda998x_write_avi(struct tda998x_priv *priv, 
struct drm_display_mode *mode)
 >   			 sizeof(buf));
 >   }
 >
 > +/* audio functions */
 > +
 >   static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 >   {
 >   	if (on) {
 > @@ -639,12 +659,11 @@ static void
 >   tda998x_configure_audio(struct tda998x_priv *priv,
 >   		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
 >   {
 > -	uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv;
 > +	uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv, aclk;
 >   	uint32_t n;
 >
 >   	/* Enable audio ports */
 >   	reg_write(priv, REG_ENA_AP, p->audio_cfg);
 > -	reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);
 >
 >   	/* Set audio input source */
 >   	switch (p->audio_format) {
 > @@ -653,13 +672,29 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 >   		clksel_aip = AIP_CLKSEL_AIP_SPDIF;
 >   		clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
 >   		cts_n = CTS_N_M(3) | CTS_N_K(3);
 > +		aclk = 0;				/* no clock */
 >   		break;
 >
 >   	case AFMT_I2S:
 >   		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
 >   		clksel_aip = AIP_CLKSEL_AIP_I2S;
 >   		clksel_fs = AIP_CLKSEL_FS_ACLK;
 > -		cts_n = CTS_N_M(3) | CTS_N_K(3);
 > +
 > +		/* with I2S input, the CTS_N predivider depends on
 > +		 * the sample width */
 > +		switch (priv->audio_sample_format) {
 > +		case SNDRV_PCM_FORMAT_S16_LE:
 > +			cts_n = CTS_N_M(3) | CTS_N_K(1);
 > +			break;
 > +		case SNDRV_PCM_FORMAT_S24_LE:
 > +			cts_n = CTS_N_M(3) | CTS_N_K(2);
 > +			break;
 > +		default:

Setting the default here does not really help, because
priv->audio_sample_format is initialized to SNDRV_PCM_FORMAT_S24_LE in
tda998x_encoder_set_config(). But I am Ok with the default being
changed for 24 bit samples on i2s interface.

 > +		case SNDRV_PCM_FORMAT_S32_LE:
 > +			cts_n = CTS_N_M(3) | CTS_N_K(3);
 > +			break;
 > +		}
 > +		aclk = 1;				/* clock enable */
 >   		break;
 >
 >   	default:
 > @@ -671,6 +706,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 >   	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT |
 >   					AIP_CNTRL_0_ACR_MAN);	/* auto CTS */
 >   	reg_write(priv, REG_CTS_N, cts_n);
 > +	reg_write(priv, REG_ENA_ACLK, aclk);
 >
 >   	/*
 >   	 * Audio input somehow depends on HDMI line rate which is
 > @@ -727,6 +763,144 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 >   	tda998x_write_aif(priv, p);
 >   }
 >
 > +/* audio codec interface */
 > +
 > +/* return the audio parameters extracted from the last EDID */
 > +static int tda998x_get_audio(struct device *dev,
 > +			int *max_channels,
 > +			int *rate_mask,
 > +			int *fmt)
 > +{
 > +	struct tda998x_priv2 *priv2 = dev_get_drvdata(dev);
 > +	struct tda998x_priv *priv = &priv2->base;
 > +
 > +	if (!priv->encoder->crtc)
 > +		return -ENODEV;
 > +
 > +	*max_channels = priv->max_channels;
 > +	*rate_mask = priv->rate_mask;
 > +	*fmt = priv->fmt;
 > +	return 0;
 > +}
 > +
 > +/* switch the audio port and initialize the audio parameters for 
streaming */
 > +static void tda998x_audio_switch(struct device *dev,
 > +				 int port_index,
 > +				 unsigned sample_rate,
 > +				 int sample_format)
 > +{
 > +	struct tda998x_priv2 *priv2 = dev_get_drvdata(dev);
 > +	struct tda998x_priv *priv = &priv2->base;
 > +	struct tda998x_encoder_params *p = &priv->params;
 > +
 > +	if (!priv->encoder->crtc)
 > +		return;
 > +
 > +	/*
 > +	 * if port_index is negative (streaming stop),
 > +	 * disable the audio port
 > +	 */
 > +	if (port_index < 0) {
 > +		reg_write(priv, REG_ENA_AP, 0);
 > +		return;
 > +	}
 > +
 > +	/* if same audio parameters, just enable the audio port */
 > +	if (p->audio_cfg == priv->audio_ports[port_index] &&
 > +	    p->audio_sample_rate == sample_rate &&
 > +	    priv->audio_sample_format == sample_format) {
 > +		reg_write(priv, REG_ENA_AP, p->audio_cfg);
 > +		return;
 > +	}
 > +
 > +	p->audio_format = port_index;
 > +	p->audio_cfg = priv->audio_ports[port_index];
 > +	p->audio_sample_rate = sample_rate;
 > +	priv->audio_sample_format = sample_format;
 > +	tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p);
 > +}
 > +
 > +#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_dais[] = {
 > +	{
 > +		.name = "spdif-hifi",
 > +		.id = AFMT_SPDIF,
 > +		.playback = {
 > +			.stream_name	= "HDMI SPDIF Playback",
 > +			.channels_min	= 1,
 > +			.channels_max	= 2,
 > +			.rates		= SNDRV_PCM_RATE_CONTINUOUS,
 > +			.rate_min	= 22050,
 > +			.rate_max	= 192000,
 > +			.formats	= TDA998X_FORMATS,
 > +		},
 > +	},
 > +	{
 > +		.name = "i2s-hifi",
 > +		.id = AFMT_I2S,
 > +		.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,
 > +		},
 > +	},
 > +};
 > +
 > +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_driver = {
 > +	.dapm_widgets = tda998x_widgets,
 > +	.num_dapm_widgets = ARRAY_SIZE(tda998x_widgets),
 > +	.dapm_routes = tda998x_routes,
 > +	.num_dapm_routes = ARRAY_SIZE(tda998x_routes),
 > +};
 > +
 > +static struct hdmi_data tda998x_hdmi_data = {
 > +	.get_audio = tda998x_get_audio,
 > +	.audio_switch = tda998x_audio_switch,
 > +	.ndais = ARRAY_SIZE(tda998x_dais),
 > +	.dais = tda998x_dais,
 > +	.driver = &tda998x_codec_driver,
 > +};
 > +
 > +static void tda998x_create_audio_codec(struct tda998x_priv *priv)
 > +{
 > +	struct platform_device *pdev;
 > +	struct module *module;
 > +
 > +	request_module("snd-soc-hdmi-codec");
 > +	pdev = platform_device_register_resndata(&priv->hdmi->dev,
 > +						 "hdmi-audio-codec",
 > +						  PLATFORM_DEVID_NONE,
 > +						  NULL, 0,
 > +						  &tda998x_hdmi_data,
 > +						  sizeof tda998x_hdmi_data);
 > +	if (IS_ERR(pdev)) {
 > +		dev_err(&priv->hdmi->dev, "cannot create codec: %ld\n",
 > +			PTR_ERR(pdev));
 > +		return;
 > +	}
 > +
 > +	priv->pdev_codec = pdev;
 > +	module = pdev->dev.driver->owner;
 > +	if (module)
 > +		try_module_get(module);
 > +}
 > +
 >   /* DRM encoder functions */
 >
 >   static void tda998x_encoder_set_config(struct tda998x_priv *priv,
 > @@ -746,6 +920,8 @@ static void tda998x_encoder_set_config(struct 
tda998x_priv *priv,
 >   			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 >
 >   	priv->params = *p;
 > +	priv->audio_ports[p->audio_format] = p->audio_cfg;
 > +	priv->audio_sample_format = SNDRV_PCM_FORMAT_S24_LE;

See the previous comment.

 >   }
 >
 >   static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)
 > @@ -1128,6 +1304,47 @@ fail:
 >   	return NULL;
 >   }
 >
 > +static void tda998x_set_audio(struct tda998x_priv *priv,
 > +			      struct drm_connector *connector)
 > +{
 > +	u8 *eld = connector->eld;
 > +	u8 *sad;
 > +	int sad_count;
 > +	unsigned eld_ver, mnl, rate_mask;
 > +	unsigned max_channels, fmt;
 > +
 > +	/* adjust the hw params from the ELD (EDID) */
 > +	eld_ver = eld[0] >> 3;
 > +	if (eld_ver != 2 && eld_ver != 31)
 > +		return;
 > +
 > +	mnl = eld[4] & 0x1f;
 > +	if (mnl > 16)
 > +		return;
 > +
 > +	sad_count = eld[5] >> 4;
 > +	sad = eld + 20 + mnl;
 > +
 > +	/* Start from the basic audio settings */
 > +	max_channels = 2;
 > +	rate_mask = 0;
 > +	fmt = 0;
 > +	while (sad_count--) {
 > +		switch (sad[0] & 0x78) {
 > +		case 0x08: /* PCM */
 > +			max_channels = max(max_channels, (sad[0] & 7) + 1u);
 > +			rate_mask |= sad[1];
 > +			fmt |= sad[2] & 0x07;
 > +			break;
 > +		}
 > +		sad += 3;
 > +	}
 > +
 > +	priv->max_channels = max_channels;
 > +	priv->rate_mask = rate_mask;
 > +	priv->fmt = fmt;
 > +}
 > +
 >   static int
 >   tda998x_encoder_get_modes(struct tda998x_priv *priv,
 >   			  struct drm_connector *connector)
 > @@ -1139,6 +1356,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);
 > +
 > +		/* set the audio parameters from the EDID */
 > +		if (priv->is_hdmi_sink) {
 > +			drm_edid_to_eld(connector, edid);
 > +			tda998x_set_audio(priv, connector);
 > +		}
 >   		kfree(edid);
 >   	}
 >
 > @@ -1167,12 +1390,19 @@ tda998x_encoder_set_property(struct 
drm_encoder *encoder,
 >
 >   static void tda998x_destroy(struct tda998x_priv *priv)
 >   {
 > +	struct module *module;
 > +
 >   	/* disable all IRQs and free the IRQ handler */
 >   	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
 >   	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 >   	if (priv->hdmi->irq)
 >   		free_irq(priv->hdmi->irq, priv);
 >
 > +	if (priv->pdev_codec) {
 > +		module = priv->pdev_codec->dev.driver->owner;
 > +		module_put(module);
 > +		platform_device_del(priv->pdev_codec);
 > +	}
 >   	i2c_unregister_device(priv->cec);
 >   }
 >
 > @@ -1254,12 +1484,16 @@ static int tda998x_create(struct i2c_client 
*client, struct tda998x_priv *priv)
 >   {
 >   	struct device_node *np = client->dev.of_node;
 >   	u32 video;
 > -	int rev_lo, rev_hi, ret;
 > +	int i, j, rev_lo, rev_hi, ret;
 > +	const char *p;
 >
 >   	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
 >   	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;
 >   	priv->cec = i2c_new_dummy(client->adapter, 0x34);
 > @@ -1351,17 +1585,48 @@ static int tda998x_create(struct i2c_client 
*client, struct tda998x_priv *priv)
 >   	/* enable EDID read irq: */
 >   	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 >
 > -	if (!np)
 > -		return 0;		/* non-DT */
 > +	/* get the device tree parameters */
 > +	if (np) {
 >
 > -	/* get the optional video properties */
 > -	ret = of_property_read_u32(np, "video-ports", &video);
 > -	if (ret == 0) {
 > -		priv->vip_cntrl_0 = video >> 16;
 > -		priv->vip_cntrl_1 = video >> 8;
 > -		priv->vip_cntrl_2 = video;
 > +		/* optional video properties */
 > +		ret = of_property_read_u32(np, "video-ports", &video);
 > +		if (ret == 0) {
 > +			priv->vip_cntrl_0 = video >> 16;
 > +			priv->vip_cntrl_1 = video >> 8;
 > +			priv->vip_cntrl_2 = video;
 > +		}
 > +
 > +		/* audio properties */
 > +		for (i = 0; i < 2; i++) {
 > +			u32 port;
 > +
 > +			ret = of_property_read_u32_index(np, "audio-ports", i, &port);
 > +			if (ret)
 > +				break;
 > +			ret = of_property_read_string_index(np, "audio-port-names",
 > +							i, &p);
 > +			if (ret) {
 > +				dev_err(&client->dev,
 > +					"missing audio-port-names[%d]\n", i);
 > +				break;
 > +			}
 > +			if (strcmp(p, "spdif") == 0) {
 > +				j = AFMT_SPDIF;
 > +			} else if (strcmp(p, "i2s") == 0) {
 > +				j = AFMT_I2S;
 > +			} else {
 > +				dev_err(&client->dev,
 > +					"bad audio-port-names '%s'\n", p);
 > +				break;
 > +			}
 > +			priv->audio_ports[j] = port;
 > +		}
 >   	}
 >
 > +	/* create the audio CODEC */
 > +	if (priv->audio_ports[AFMT_SPDIF] || priv->audio_ports[AFMT_I2S])
 > +		tda998x_create_audio_codec(priv);
 > +
 >   	return 0;
 >
 >   fail:
 > @@ -1395,15 +1660,13 @@ static int tda998x_encoder_init(struct 
i2c_client *client,
 >   	encoder_slave->slave_priv = priv;
 >   	encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;
 >
 > +	/* set the drvdata pointer to priv2 for CODEC calls */
 > +	dev_set_drvdata(&client->dev,
 > +			container_of(priv, struct tda998x_priv2, base));
 > +
 >   	return 0;
 >   }
 >
 > -struct tda998x_priv2 {
 > -	struct tda998x_priv base;
 > -	struct drm_encoder encoder;
 > -	struct drm_connector connector;
 > -};
 > -
 >   #define conn_to_tda998x_priv2(x) \
 >   	container_of(x, struct tda998x_priv2, connector);
 >
 >

The only audio side change in the platform data usage of tda998x_drv I
can see is the change in the default value of CTS_N.

Best regards,
Jyri
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Sep 24, 2014 at 10:11:21AM +0200, Jean-Francois Moine wrote:
> This patch interfaces the HDMI transmitter with the audio system.
...

> +struct tda998x_priv2 {
> +	struct tda998x_priv base;
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
>  };

NAK on moving this up here.  It was specifically placed below the core
and below the older drm_slave_encoder code because nothing but the new
component based interfaces to TDA998x have any business knowing that
these are packaged up in this way.

> +static void tda998x_create_audio_codec(struct tda998x_priv *priv)
> +{
> +	struct platform_device *pdev;
> +	struct module *module;
> +
> +	request_module("snd-soc-hdmi-codec");
> +	pdev = platform_device_register_resndata(&priv->hdmi->dev,
> +						 "hdmi-audio-codec",
> +						  PLATFORM_DEVID_NONE,
> +						  NULL, 0,
> +						  &tda998x_hdmi_data,
> +						  sizeof tda998x_hdmi_data);
> +	if (IS_ERR(pdev)) {
> +		dev_err(&priv->hdmi->dev, "cannot create codec: %ld\n",
> +			PTR_ERR(pdev));
> +		return;
> +	}
> +
> +	priv->pdev_codec = pdev;
> +	module = pdev->dev.driver->owner;
> +	if (module)
> +		try_module_get(module);

This is really not on.  Firstly, registering a platform device has no
guarantee that it will bind to a driver.  So, pdev->dev.driver may be
NULL here -> kernel oops.

Secondly, what's the purpose of the unchecked try_module_get() there?
You can't stop the device being unbound from its driver, so all you're
doing is possibly locking the module into module space for no other
benefit.

> @@ -1167,12 +1390,19 @@ tda998x_encoder_set_property(struct drm_encoder *encoder,
>  
>  static void tda998x_destroy(struct tda998x_priv *priv)
>  {
> +	struct module *module;
> +
>  	/* disable all IRQs and free the IRQ handler */
>  	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
>  	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
>  	if (priv->hdmi->irq)
>  		free_irq(priv->hdmi->irq, priv);
>  
> +	if (priv->pdev_codec) {
> +		module = priv->pdev_codec->dev.driver->owner;
> +		module_put(module);

This is wrong for all the reasons the other part above is wrong.
Userspace can decide to unbind the platform device from the associated
platform driver whenever it wishes.  At that point,
priv->pdev_codec->dev.driver becomes NULL.

So, please get rid of this.

> +		platform_device_del(priv->pdev_codec);

This leaks the memory associated with the platform device.

> @@ -1395,15 +1660,13 @@ static int tda998x_encoder_init(struct i2c_client *client,
>  	encoder_slave->slave_priv = priv;
>  	encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;
>  
> +	/* set the drvdata pointer to priv2 for CODEC calls */
> +	dev_set_drvdata(&client->dev,
> +			container_of(priv, struct tda998x_priv2, base));
> +
>  	return 0;
>  }
>  
> -struct tda998x_priv2 {
> -	struct tda998x_priv base;
> -	struct drm_encoder encoder;
> -	struct drm_connector connector;
> -};
> -

I would prefer this structure to stay here, as code above this point should
have no business knowing how these are packaged together.  I would suggest
either:

- moving the audio codec code below this point, or
- storing struct tda998x_priv in the device private pointer, and
  converting it to struct tda998x_priv2 via container_of() where
  necessary below this point.

On Wed, 1 Oct 2014 15:23:41 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> I would prefer this structure to stay here, as code above this point should
> have no business knowing how these are packaged together.  I would suggest
> either:
> 
> - moving the audio codec code below this point, or
> - storing struct tda998x_priv in the device private pointer, and
>   converting it to struct tda998x_priv2 via container_of() where
>   necessary below this point.

The second option seems easier for use as a slave encoder.

Thanks for all your remarks. I will send an other version.

On Wed, 1 Oct 2014 17:05:32 +0300
Jyri Sarha <jsarha@ti.com> wrote:

> >   	case AFMT_I2S:
>  >   		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
>  >   		clksel_aip = AIP_CLKSEL_AIP_I2S;
>  >   		clksel_fs = AIP_CLKSEL_FS_ACLK;
>  > -		cts_n = CTS_N_M(3) | CTS_N_K(3);
>  > +
>  > +		/* with I2S input, the CTS_N predivider depends on
>  > +		 * the sample width */
>  > +		switch (priv->audio_sample_format) {
>  > +		case SNDRV_PCM_FORMAT_S16_LE:
>  > +			cts_n = CTS_N_M(3) | CTS_N_K(1);
>  > +			break;
>  > +		case SNDRV_PCM_FORMAT_S24_LE:
>  > +			cts_n = CTS_N_M(3) | CTS_N_K(2);
>  > +			break;
>  > +		default:  
> 
> Setting the default here does not really help, because
> priv->audio_sample_format is initialized to SNDRV_PCM_FORMAT_S24_LE in
> tda998x_encoder_set_config(). But I am Ok with the default being
> changed for 24 bit samples on i2s interface.
> 
>  > +		case SNDRV_PCM_FORMAT_S32_LE:
>  > +			cts_n = CTS_N_M(3) | CTS_N_K(3);
>  > +			break;
>  > +		}

I looked again at the original driver and they set K = 1 for 16 bits
and K = 3 for 24 or 32 bits.

Anyway, letting K = 3 for 16 bits works for me, so, I will not change
this code in the next version.

Thanks.