* [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-10 14:11 ` Stefan Mavrodiev
0 siblings, 0 replies; 42+ messages in thread
From: Stefan Mavrodiev @ 2020-01-10 14:11 UTC (permalink / raw)
To: Dan Williams, Vinod Koul, Maxime Ripard, Chen-Yu Tsai,
David Airlie, Daniel Vetter, open list,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
moderated list:ARM/Allwinner sunXi SoC support,
open list:DRM DRIVERS FOR ALLWINNER A10
Cc: linux-sunxi, Stefan Mavrodiev
Add HDMI audio support for the sun4i-hdmi encoder, used on
the older Allwinner chips - A10, A20, A31.
Most of the code is based on the BSP implementation. In it
dditional formats are supported (S20_3LE and S24_LE), however
there where some problems with them and only S16_LE is left.
Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
---
drivers/gpu/drm/sun4i/Kconfig | 1 +
drivers/gpu/drm/sun4i/Makefile | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
5 files changed, 411 insertions(+)
create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
index 37e90e42943f..192b732b10cd 100644
--- a/drivers/gpu/drm/sun4i/Kconfig
+++ b/drivers/gpu/drm/sun4i/Kconfig
@@ -19,6 +19,7 @@ if DRM_SUN4I
config DRM_SUN4I_HDMI
tristate "Allwinner A10 HDMI Controller Support"
default DRM_SUN4I
+ select SND_PCM_ELD
help
Choose this option if you have an Allwinner SoC with an HDMI
controller.
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index 0d04f2447b01..e2d82b451c36 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
sun4i-drm-y += sun4i_drv.o
sun4i-drm-y += sun4i_framebuffer.o
+sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 7ad3f06c127e..456964e681b0 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -42,7 +42,32 @@
#define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
#define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
+#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
+#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
+#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
+
+#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
+#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
+#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
+#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
+#define SUN4I_HDMI_AUDIO_FMT_CH_CFG_MASK GENMASK(2, 0)
+
+#define SUN4I_HDMI_AUDIO_PCM_REG 0x4c
+#define SUN4I_HDMI_AUDIO_PCM_CH_MAP(n, m) ((m - 1) << (n * 4))
+#define SUN4I_HDMI_AUDIO_PCM_CH_MAP_MASK(n) (GENMASK(2, 0) << (n * 4))
+
+#define SUN4I_HDMI_AUDIO_CTS_REG 0x050
+#define SUN4I_HDMI_AUDIO_CTS(n) (n & GENMASK(19, 0))
+
+#define SUN4I_HDMI_AUDIO_N_REG 0x054
+#define SUN4I_HDMI_AUDIO_N(n) (n & GENMASK(19, 0))
+
+#define SUN4I_HDMI_AUDIO_STAT0_REG 0x58
+#define SUN4I_HDMI_AUDIO_STAT0_FREQ(n) (n << 24)
+#define SUN4I_HDMI_AUDIO_STAT0_FREQ_MASK GENMASK(27, 24)
+
#define SUN4I_HDMI_AVI_INFOFRAME_REG(n) (0x080 + (n))
+#define SUN4I_HDMI_AUDIO_INFOFRAME_REG(n) (0x0a0 + (n))
#define SUN4I_HDMI_PAD_CTRL0_REG 0x200
#define SUN4I_HDMI_PAD_CTRL0_BIASEN BIT(31)
@@ -283,9 +308,13 @@ struct sun4i_hdmi {
struct regmap_field *field_ddc_sda_en;
struct regmap_field *field_ddc_sck_en;
+ u8 hdmi_audio_channels;
+
struct sun4i_drv *drv;
bool hdmi_monitor;
+ bool hdmi_audio;
+
struct cec_adapter *cec_adap;
const struct sun4i_hdmi_variant *variant;
@@ -294,5 +323,6 @@ struct sun4i_hdmi {
int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
+int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi);
#endif /* _SUN4I_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
new file mode 100644
index 000000000000..b6d4199d15ce
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
@@ -0,0 +1,375 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020 Olimex Ltd.
+ * Author: Stefan Mavrodiev <stefan@olimex.com>
+ */
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/module.h>
+#include <linux/of_dma.h>
+#include <linux/regmap.h>
+
+#include <drm/drm_print.h>
+
+#include <sound/dmaengine_pcm.h>
+#include <sound/pcm_drm_eld.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include "sun4i_hdmi.h"
+
+static const struct snd_soc_dapm_widget sun4i_hdmi_audio_widgets[] = {
+ SND_SOC_DAPM_OUTPUT("TX"),
+};
+
+static const struct snd_soc_dapm_route sun4i_hdmi_audio_routes[] = {
+ { "TX", NULL, "Playback" },
+};
+
+static const struct snd_soc_component_driver sun4i_hdmi_audio_component = {
+ .dapm_widgets = sun4i_hdmi_audio_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(sun4i_hdmi_audio_widgets),
+ .dapm_routes = sun4i_hdmi_audio_routes,
+ .num_dapm_routes = ARRAY_SIZE(sun4i_hdmi_audio_routes),
+};
+
+static int sun4i_hdmi_audio_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_card *card = snd_soc_dai_get_drvdata(dai);
+ struct sun4i_hdmi *hdmi = snd_soc_card_get_drvdata(card);
+ u32 reg;
+ int ret;
+
+ regmap_write(hdmi->regmap, SUN4I_HDMI_AUDIO_CTRL_REG, 0);
+ regmap_write(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_CTRL_REG,
+ SUN4I_HDMI_AUDIO_CTRL_RESET);
+ ret = regmap_read_poll_timeout(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_CTRL_REG,
+ reg, !reg, 100, 50000);
+ if (ret < 0) {
+ DRM_ERROR("Failed to reset HDMI Audio\n");
+ return ret;
+ }
+
+ regmap_write(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_CTRL_REG,
+ SUN4I_HDMI_AUDIO_CTRL_ENABLE);
+
+ return snd_pcm_hw_constraint_eld(substream->runtime,
+ hdmi->connector.eld);
+}
+
+static void sun4i_hdmi_audio_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_card *card = snd_soc_dai_get_drvdata(dai);
+ struct sun4i_hdmi *hdmi = snd_soc_card_get_drvdata(card);
+
+ regmap_write(hdmi->regmap, SUN4I_HDMI_AUDIO_CTRL_REG, 0);
+}
+
+static int sun4i_hdmi_setup_audio_infoframes(struct sun4i_hdmi *hdmi)
+{
+ union hdmi_infoframe frame;
+ u8 buffer[14];
+ int i, ret;
+
+ ret = hdmi_audio_infoframe_init(&frame.audio);
+ if (ret < 0) {
+ DRM_ERROR("Failed to init HDMI audio infoframe\n");
+ return ret;
+ }
+
+ frame.audio.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
+ frame.audio.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
+ frame.audio.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
+ frame.audio.channels = hdmi->hdmi_audio_channels;
+
+ ret = hdmi_infoframe_pack(&frame, buffer, sizeof(buffer));
+ if (ret < 0) {
+ DRM_ERROR("Failed to pack HDMI audio infoframe\n");
+ return ret;
+ }
+
+ for (i = 0; i < sizeof(buffer); i++)
+ writeb(buffer[i],
+ hdmi->base + SUN4I_HDMI_AUDIO_INFOFRAME_REG(i));
+
+ return 0;
+}
+
+static void sun4i_hdmi_audio_set_cts_n(struct sun4i_hdmi *hdmi,
+ struct snd_pcm_hw_params *params)
+{
+ struct drm_encoder *encoder = &hdmi->encoder;
+ struct drm_crtc *crtc = encoder->crtc;
+ const struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+ u32 rate = params_rate(params);
+ u32 n, cts;
+ u64 tmp;
+
+ /**
+ * Calculate Cycle Time Stamp (CTS) and Numerator (N):
+ *
+ * N = 128 * Samplerate / 1000
+ * CTS = (Ftdms * N) / (128 * Samplerate)
+ */
+
+ n = 128 * rate / 1000;
+ tmp = (u64)(mode->clock * 1000) * n;
+ do_div(tmp, 128 * rate);
+ cts = tmp;
+
+ regmap_write(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_CTS_REG,
+ SUN4I_HDMI_AUDIO_CTS(cts));
+
+ regmap_write(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_N_REG,
+ SUN4I_HDMI_AUDIO_N(n));
+}
+
+static int sun4i_hdmi_audio_set_hw_rate(struct sun4i_hdmi *hdmi,
+ struct snd_pcm_hw_params *params)
+{
+ u32 rate = params_rate(params);
+ u32 val;
+
+ switch (rate) {
+ case 44100:
+ val = 0x0;
+ break;
+ case 48000:
+ val = 0x2;
+ break;
+ case 32000:
+ val = 0x3;
+ break;
+ case 88200:
+ val = 0x8;
+ break;
+ case 96000:
+ val = 0x9;
+ break;
+ case 176400:
+ val = 0xc;
+ break;
+ case 192000:
+ val = 0xe;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ regmap_update_bits(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_STAT0_REG,
+ SUN4I_HDMI_AUDIO_STAT0_FREQ_MASK,
+ SUN4I_HDMI_AUDIO_STAT0_FREQ(val));
+
+ return 0;
+}
+
+static int sun4i_hdmi_audio_set_hw_channels(struct sun4i_hdmi *hdmi,
+ struct snd_pcm_hw_params *params)
+{
+ u32 channels = params_channels(params);
+
+ if (channels > 8)
+ return -EINVAL;
+
+ hdmi->hdmi_audio_channels = channels;
+
+ regmap_update_bits(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_FMT_REG,
+ SUN4I_HDMI_AUDIO_FMT_LAYOUT,
+ (channels > 2) ? SUN4I_HDMI_AUDIO_FMT_LAYOUT : 0);
+
+ regmap_update_bits(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_FMT_REG,
+ SUN4I_HDMI_AUDIO_FMT_CH_CFG_MASK,
+ SUN4I_HDMI_AUDIO_FMT_CH_CFG(channels));
+
+ regmap_write(hdmi->regmap, SUN4I_HDMI_AUDIO_PCM_REG, 0x76543210);
+
+ /**
+ * If only one channel is required, send the same sample
+ * to the sink device as a left and right channel.
+ */
+ if (channels == 1)
+ regmap_update_bits(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_PCM_REG,
+ SUN4I_HDMI_AUDIO_PCM_CH_MAP_MASK(1),
+ SUN4I_HDMI_AUDIO_PCM_CH_MAP(1, 1));
+
+ return 0;
+}
+
+static int sun4i_hdmi_audio_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_card *card = snd_soc_dai_get_drvdata(dai);
+ struct sun4i_hdmi *hdmi = snd_soc_card_get_drvdata(card);
+ int ret;
+
+ ret = sun4i_hdmi_audio_set_hw_rate(hdmi, params);
+ if (ret)
+ return ret;
+
+ ret = sun4i_hdmi_audio_set_hw_channels(hdmi, params);
+ if (ret)
+ return ret;
+
+ sun4i_hdmi_audio_set_cts_n(hdmi, params);
+
+ return 0;
+}
+
+static int sun4i_hdmi_audio_trigger(struct snd_pcm_substream *substream,
+ int cmd,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_card *card = snd_soc_dai_get_drvdata(dai);
+ struct sun4i_hdmi *hdmi = snd_soc_card_get_drvdata(card);
+ int ret = 0;
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ ret = sun4i_hdmi_setup_audio_infoframes(hdmi);
+ break;
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+static const struct snd_soc_dai_ops sun4i_hdmi_audio_dai_ops = {
+ .startup = sun4i_hdmi_audio_startup,
+ .shutdown = sun4i_hdmi_audio_shutdown,
+ .hw_params = sun4i_hdmi_audio_hw_params,
+ .trigger = sun4i_hdmi_audio_trigger,
+};
+
+static int sun4i_hdmi_audio_dai_probe(struct snd_soc_dai *dai)
+{
+ struct snd_dmaengine_dai_dma_data *dma_data;
+
+ dma_data = devm_kzalloc(dai->dev, sizeof(*dma_data), GFP_KERNEL);
+ if (!dma_data)
+ return -ENOMEM;
+
+ dma_data->addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ dma_data->maxburst = 8;
+
+ snd_soc_dai_init_dma_data(dai, dma_data, NULL);
+
+ return 0;
+}
+
+static struct snd_soc_dai_driver sun4i_hdmi_audio_dai = {
+ .name = "HDMI",
+ .ops = &sun4i_hdmi_audio_dai_ops,
+ .probe = sun4i_hdmi_audio_dai_probe,
+ .playback = {
+ .stream_name = "Playback",
+ .channels_min = 1,
+ .channels_max = 8,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE,
+ .rates = SNDRV_PCM_RATE_8000_192000,
+ },
+};
+
+static const struct snd_pcm_hardware sun4i_hdmi_audio_pcm_hardware = {
+ .info = SNDRV_PCM_INFO_INTERLEAVED |
+ SNDRV_PCM_INFO_BLOCK_TRANSFER |
+ SNDRV_PCM_INFO_MMAP |
+ SNDRV_PCM_INFO_MMAP_VALID |
+ SNDRV_PCM_INFO_PAUSE |
+ SNDRV_PCM_INFO_RESUME,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE,
+ .rates = SNDRV_PCM_RATE_8000_192000,
+ .rate_min = 8000,
+ .rate_max = 192000,
+ .channels_min = 1,
+ .channels_max = 8,
+ .buffer_bytes_max = 128 * 1024,
+ .period_bytes_min = 4 * 1024,
+ .period_bytes_max = 32 * 1024,
+ .periods_min = 2,
+ .periods_max = 8,
+ .fifo_size = 128,
+};
+
+static const struct snd_dmaengine_pcm_config sun4i_hdmi_audio_pcm_config = {
+ .chan_names[SNDRV_PCM_STREAM_PLAYBACK] = "audio-tx",
+ .pcm_hardware = &sun4i_hdmi_audio_pcm_hardware,
+ .prealloc_buffer_size = 128 * 1024,
+ .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+};
+
+struct snd_soc_card sun4i_hdmi_audio_card = {
+ .name = "sun4i-hdmi",
+};
+
+int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
+{
+ struct snd_soc_card *card = &sun4i_hdmi_audio_card;
+ struct snd_soc_dai_link_component *comp;
+ struct snd_soc_dai_link *link;
+ int ret;
+
+ ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
+ &sun4i_hdmi_audio_pcm_config, 0);
+ if (ret) {
+ DRM_ERROR("Could not register PCM\n");
+ return ret;
+ }
+
+ ret = devm_snd_soc_register_component(hdmi->dev,
+ &sun4i_hdmi_audio_component,
+ &sun4i_hdmi_audio_dai, 1);
+ if (ret) {
+ DRM_ERROR("Could not register DAI\n");
+ return ret;
+ }
+
+ link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
+ if (!link)
+ return -ENOMEM;
+
+ comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
+ if (!comp)
+ return -ENOMEM;
+
+ link->cpus = &comp[0];
+ link->codecs = &comp[1];
+ link->platforms = &comp[2];
+
+ link->num_cpus = 1;
+ link->num_codecs = 1;
+ link->num_platforms = 1;
+
+ link->playback_only = 1;
+
+ link->name = "SUN4I-HDMI";
+ link->stream_name = "SUN4I-HDMI PCM";
+
+ link->codecs->name = dev_name(hdmi->dev);
+ link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
+
+ link->cpus->dai_name = dev_name(hdmi->dev);
+
+ link->platforms->name = dev_name(hdmi->dev);
+
+ link->dai_fmt = SND_SOC_DAIFMT_I2S;
+
+ card->dai_link = link;
+ card->num_links = 1;
+ card->dev = hdmi->dev;
+
+ snd_soc_card_set_drvdata(card, hdmi);
+ return devm_snd_soc_register_card(hdmi->dev, card);
+}
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index a7c4654445c7..79ecd89fb705 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
+
+ if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
+ DRM_ERROR("Couldn't create the HDMI audio adapter\n");
}
static void sun4i_hdmi_mode_set(struct drm_encoder *encoder,
@@ -218,6 +221,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
if (!edid)
return 0;
+ hdmi->hdmi_audio = drm_detect_monitor_audio(edid);
hdmi->hdmi_monitor = drm_detect_hdmi_monitor(edid);
DRM_DEBUG_DRIVER("Monitor is %s monitor\n",
hdmi->hdmi_monitor ? "an HDMI" : "a DVI");
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-10 14:11 ` Stefan Mavrodiev
0 siblings, 0 replies; 42+ messages in thread
From: Stefan Mavrodiev @ 2020-01-10 14:11 UTC (permalink / raw)
To: Dan Williams, Vinod Koul, Maxime Ripard, Chen-Yu Tsai,
David Airlie, Daniel Vetter, open list,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
moderated list:ARM/Allwinner sunXi SoC support,
open list:DRM DRIVERS FOR ALLWINNER A10
Cc: linux-sunxi, Stefan Mavrodiev
Add HDMI audio support for the sun4i-hdmi encoder, used on
the older Allwinner chips - A10, A20, A31.
Most of the code is based on the BSP implementation. In it
dditional formats are supported (S20_3LE and S24_LE), however
there where some problems with them and only S16_LE is left.
Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
---
drivers/gpu/drm/sun4i/Kconfig | 1 +
drivers/gpu/drm/sun4i/Makefile | 1 +
drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
5 files changed, 411 insertions(+)
create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
index 37e90e42943f..192b732b10cd 100644
--- a/drivers/gpu/drm/sun4i/Kconfig
+++ b/drivers/gpu/drm/sun4i/Kconfig
@@ -19,6 +19,7 @@ if DRM_SUN4I
config DRM_SUN4I_HDMI
tristate "Allwinner A10 HDMI Controller Support"
default DRM_SUN4I
+ select SND_PCM_ELD
help
Choose this option if you have an Allwinner SoC with an HDMI
controller.
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index 0d04f2447b01..e2d82b451c36 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
sun4i-drm-y += sun4i_drv.o
sun4i-drm-y += sun4i_framebuffer.o
+sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 7ad3f06c127e..456964e681b0 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -42,7 +42,32 @@
#define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
#define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
+#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
+#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
+#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
+
+#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
+#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
+#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
+#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
+#define SUN4I_HDMI_AUDIO_FMT_CH_CFG_MASK GENMASK(2, 0)
+
+#define SUN4I_HDMI_AUDIO_PCM_REG 0x4c
+#define SUN4I_HDMI_AUDIO_PCM_CH_MAP(n, m) ((m - 1) << (n * 4))
+#define SUN4I_HDMI_AUDIO_PCM_CH_MAP_MASK(n) (GENMASK(2, 0) << (n * 4))
+
+#define SUN4I_HDMI_AUDIO_CTS_REG 0x050
+#define SUN4I_HDMI_AUDIO_CTS(n) (n & GENMASK(19, 0))
+
+#define SUN4I_HDMI_AUDIO_N_REG 0x054
+#define SUN4I_HDMI_AUDIO_N(n) (n & GENMASK(19, 0))
+
+#define SUN4I_HDMI_AUDIO_STAT0_REG 0x58
+#define SUN4I_HDMI_AUDIO_STAT0_FREQ(n) (n << 24)
+#define SUN4I_HDMI_AUDIO_STAT0_FREQ_MASK GENMASK(27, 24)
+
#define SUN4I_HDMI_AVI_INFOFRAME_REG(n) (0x080 + (n))
+#define SUN4I_HDMI_AUDIO_INFOFRAME_REG(n) (0x0a0 + (n))
#define SUN4I_HDMI_PAD_CTRL0_REG 0x200
#define SUN4I_HDMI_PAD_CTRL0_BIASEN BIT(31)
@@ -283,9 +308,13 @@ struct sun4i_hdmi {
struct regmap_field *field_ddc_sda_en;
struct regmap_field *field_ddc_sck_en;
+ u8 hdmi_audio_channels;
+
struct sun4i_drv *drv;
bool hdmi_monitor;
+ bool hdmi_audio;
+
struct cec_adapter *cec_adap;
const struct sun4i_hdmi_variant *variant;
@@ -294,5 +323,6 @@ struct sun4i_hdmi {
int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
+int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi);
#endif /* _SUN4I_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
new file mode 100644
index 000000000000..b6d4199d15ce
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
@@ -0,0 +1,375 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020 Olimex Ltd.
+ * Author: Stefan Mavrodiev <stefan@olimex.com>
+ */
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/module.h>
+#include <linux/of_dma.h>
+#include <linux/regmap.h>
+
+#include <drm/drm_print.h>
+
+#include <sound/dmaengine_pcm.h>
+#include <sound/pcm_drm_eld.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include "sun4i_hdmi.h"
+
+static const struct snd_soc_dapm_widget sun4i_hdmi_audio_widgets[] = {
+ SND_SOC_DAPM_OUTPUT("TX"),
+};
+
+static const struct snd_soc_dapm_route sun4i_hdmi_audio_routes[] = {
+ { "TX", NULL, "Playback" },
+};
+
+static const struct snd_soc_component_driver sun4i_hdmi_audio_component = {
+ .dapm_widgets = sun4i_hdmi_audio_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(sun4i_hdmi_audio_widgets),
+ .dapm_routes = sun4i_hdmi_audio_routes,
+ .num_dapm_routes = ARRAY_SIZE(sun4i_hdmi_audio_routes),
+};
+
+static int sun4i_hdmi_audio_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_card *card = snd_soc_dai_get_drvdata(dai);
+ struct sun4i_hdmi *hdmi = snd_soc_card_get_drvdata(card);
+ u32 reg;
+ int ret;
+
+ regmap_write(hdmi->regmap, SUN4I_HDMI_AUDIO_CTRL_REG, 0);
+ regmap_write(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_CTRL_REG,
+ SUN4I_HDMI_AUDIO_CTRL_RESET);
+ ret = regmap_read_poll_timeout(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_CTRL_REG,
+ reg, !reg, 100, 50000);
+ if (ret < 0) {
+ DRM_ERROR("Failed to reset HDMI Audio\n");
+ return ret;
+ }
+
+ regmap_write(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_CTRL_REG,
+ SUN4I_HDMI_AUDIO_CTRL_ENABLE);
+
+ return snd_pcm_hw_constraint_eld(substream->runtime,
+ hdmi->connector.eld);
+}
+
+static void sun4i_hdmi_audio_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_card *card = snd_soc_dai_get_drvdata(dai);
+ struct sun4i_hdmi *hdmi = snd_soc_card_get_drvdata(card);
+
+ regmap_write(hdmi->regmap, SUN4I_HDMI_AUDIO_CTRL_REG, 0);
+}
+
+static int sun4i_hdmi_setup_audio_infoframes(struct sun4i_hdmi *hdmi)
+{
+ union hdmi_infoframe frame;
+ u8 buffer[14];
+ int i, ret;
+
+ ret = hdmi_audio_infoframe_init(&frame.audio);
+ if (ret < 0) {
+ DRM_ERROR("Failed to init HDMI audio infoframe\n");
+ return ret;
+ }
+
+ frame.audio.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
+ frame.audio.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
+ frame.audio.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
+ frame.audio.channels = hdmi->hdmi_audio_channels;
+
+ ret = hdmi_infoframe_pack(&frame, buffer, sizeof(buffer));
+ if (ret < 0) {
+ DRM_ERROR("Failed to pack HDMI audio infoframe\n");
+ return ret;
+ }
+
+ for (i = 0; i < sizeof(buffer); i++)
+ writeb(buffer[i],
+ hdmi->base + SUN4I_HDMI_AUDIO_INFOFRAME_REG(i));
+
+ return 0;
+}
+
+static void sun4i_hdmi_audio_set_cts_n(struct sun4i_hdmi *hdmi,
+ struct snd_pcm_hw_params *params)
+{
+ struct drm_encoder *encoder = &hdmi->encoder;
+ struct drm_crtc *crtc = encoder->crtc;
+ const struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+ u32 rate = params_rate(params);
+ u32 n, cts;
+ u64 tmp;
+
+ /**
+ * Calculate Cycle Time Stamp (CTS) and Numerator (N):
+ *
+ * N = 128 * Samplerate / 1000
+ * CTS = (Ftdms * N) / (128 * Samplerate)
+ */
+
+ n = 128 * rate / 1000;
+ tmp = (u64)(mode->clock * 1000) * n;
+ do_div(tmp, 128 * rate);
+ cts = tmp;
+
+ regmap_write(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_CTS_REG,
+ SUN4I_HDMI_AUDIO_CTS(cts));
+
+ regmap_write(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_N_REG,
+ SUN4I_HDMI_AUDIO_N(n));
+}
+
+static int sun4i_hdmi_audio_set_hw_rate(struct sun4i_hdmi *hdmi,
+ struct snd_pcm_hw_params *params)
+{
+ u32 rate = params_rate(params);
+ u32 val;
+
+ switch (rate) {
+ case 44100:
+ val = 0x0;
+ break;
+ case 48000:
+ val = 0x2;
+ break;
+ case 32000:
+ val = 0x3;
+ break;
+ case 88200:
+ val = 0x8;
+ break;
+ case 96000:
+ val = 0x9;
+ break;
+ case 176400:
+ val = 0xc;
+ break;
+ case 192000:
+ val = 0xe;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ regmap_update_bits(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_STAT0_REG,
+ SUN4I_HDMI_AUDIO_STAT0_FREQ_MASK,
+ SUN4I_HDMI_AUDIO_STAT0_FREQ(val));
+
+ return 0;
+}
+
+static int sun4i_hdmi_audio_set_hw_channels(struct sun4i_hdmi *hdmi,
+ struct snd_pcm_hw_params *params)
+{
+ u32 channels = params_channels(params);
+
+ if (channels > 8)
+ return -EINVAL;
+
+ hdmi->hdmi_audio_channels = channels;
+
+ regmap_update_bits(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_FMT_REG,
+ SUN4I_HDMI_AUDIO_FMT_LAYOUT,
+ (channels > 2) ? SUN4I_HDMI_AUDIO_FMT_LAYOUT : 0);
+
+ regmap_update_bits(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_FMT_REG,
+ SUN4I_HDMI_AUDIO_FMT_CH_CFG_MASK,
+ SUN4I_HDMI_AUDIO_FMT_CH_CFG(channels));
+
+ regmap_write(hdmi->regmap, SUN4I_HDMI_AUDIO_PCM_REG, 0x76543210);
+
+ /**
+ * If only one channel is required, send the same sample
+ * to the sink device as a left and right channel.
+ */
+ if (channels == 1)
+ regmap_update_bits(hdmi->regmap,
+ SUN4I_HDMI_AUDIO_PCM_REG,
+ SUN4I_HDMI_AUDIO_PCM_CH_MAP_MASK(1),
+ SUN4I_HDMI_AUDIO_PCM_CH_MAP(1, 1));
+
+ return 0;
+}
+
+static int sun4i_hdmi_audio_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_card *card = snd_soc_dai_get_drvdata(dai);
+ struct sun4i_hdmi *hdmi = snd_soc_card_get_drvdata(card);
+ int ret;
+
+ ret = sun4i_hdmi_audio_set_hw_rate(hdmi, params);
+ if (ret)
+ return ret;
+
+ ret = sun4i_hdmi_audio_set_hw_channels(hdmi, params);
+ if (ret)
+ return ret;
+
+ sun4i_hdmi_audio_set_cts_n(hdmi, params);
+
+ return 0;
+}
+
+static int sun4i_hdmi_audio_trigger(struct snd_pcm_substream *substream,
+ int cmd,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_card *card = snd_soc_dai_get_drvdata(dai);
+ struct sun4i_hdmi *hdmi = snd_soc_card_get_drvdata(card);
+ int ret = 0;
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ ret = sun4i_hdmi_setup_audio_infoframes(hdmi);
+ break;
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+static const struct snd_soc_dai_ops sun4i_hdmi_audio_dai_ops = {
+ .startup = sun4i_hdmi_audio_startup,
+ .shutdown = sun4i_hdmi_audio_shutdown,
+ .hw_params = sun4i_hdmi_audio_hw_params,
+ .trigger = sun4i_hdmi_audio_trigger,
+};
+
+static int sun4i_hdmi_audio_dai_probe(struct snd_soc_dai *dai)
+{
+ struct snd_dmaengine_dai_dma_data *dma_data;
+
+ dma_data = devm_kzalloc(dai->dev, sizeof(*dma_data), GFP_KERNEL);
+ if (!dma_data)
+ return -ENOMEM;
+
+ dma_data->addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ dma_data->maxburst = 8;
+
+ snd_soc_dai_init_dma_data(dai, dma_data, NULL);
+
+ return 0;
+}
+
+static struct snd_soc_dai_driver sun4i_hdmi_audio_dai = {
+ .name = "HDMI",
+ .ops = &sun4i_hdmi_audio_dai_ops,
+ .probe = sun4i_hdmi_audio_dai_probe,
+ .playback = {
+ .stream_name = "Playback",
+ .channels_min = 1,
+ .channels_max = 8,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE,
+ .rates = SNDRV_PCM_RATE_8000_192000,
+ },
+};
+
+static const struct snd_pcm_hardware sun4i_hdmi_audio_pcm_hardware = {
+ .info = SNDRV_PCM_INFO_INTERLEAVED |
+ SNDRV_PCM_INFO_BLOCK_TRANSFER |
+ SNDRV_PCM_INFO_MMAP |
+ SNDRV_PCM_INFO_MMAP_VALID |
+ SNDRV_PCM_INFO_PAUSE |
+ SNDRV_PCM_INFO_RESUME,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE,
+ .rates = SNDRV_PCM_RATE_8000_192000,
+ .rate_min = 8000,
+ .rate_max = 192000,
+ .channels_min = 1,
+ .channels_max = 8,
+ .buffer_bytes_max = 128 * 1024,
+ .period_bytes_min = 4 * 1024,
+ .period_bytes_max = 32 * 1024,
+ .periods_min = 2,
+ .periods_max = 8,
+ .fifo_size = 128,
+};
+
+static const struct snd_dmaengine_pcm_config sun4i_hdmi_audio_pcm_config = {
+ .chan_names[SNDRV_PCM_STREAM_PLAYBACK] = "audio-tx",
+ .pcm_hardware = &sun4i_hdmi_audio_pcm_hardware,
+ .prealloc_buffer_size = 128 * 1024,
+ .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+};
+
+struct snd_soc_card sun4i_hdmi_audio_card = {
+ .name = "sun4i-hdmi",
+};
+
+int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
+{
+ struct snd_soc_card *card = &sun4i_hdmi_audio_card;
+ struct snd_soc_dai_link_component *comp;
+ struct snd_soc_dai_link *link;
+ int ret;
+
+ ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
+ &sun4i_hdmi_audio_pcm_config, 0);
+ if (ret) {
+ DRM_ERROR("Could not register PCM\n");
+ return ret;
+ }
+
+ ret = devm_snd_soc_register_component(hdmi->dev,
+ &sun4i_hdmi_audio_component,
+ &sun4i_hdmi_audio_dai, 1);
+ if (ret) {
+ DRM_ERROR("Could not register DAI\n");
+ return ret;
+ }
+
+ link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
+ if (!link)
+ return -ENOMEM;
+
+ comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
+ if (!comp)
+ return -ENOMEM;
+
+ link->cpus = &comp[0];
+ link->codecs = &comp[1];
+ link->platforms = &comp[2];
+
+ link->num_cpus = 1;
+ link->num_codecs = 1;
+ link->num_platforms = 1;
+
+ link->playback_only = 1;
+
+ link->name = "SUN4I-HDMI";
+ link->stream_name = "SUN4I-HDMI PCM";
+
+ link->codecs->name = dev_name(hdmi->dev);
+ link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
+
+ link->cpus->dai_name = dev_name(hdmi->dev);
+
+ link->platforms->name = dev_name(hdmi->dev);
+
+ link->dai_fmt = SND_SOC_DAIFMT_I2S;
+
+ card->dai_link = link;
+ card->num_links = 1;
+ card->dev = hdmi->dev;
+
+ snd_soc_card_set_drvdata(card, hdmi);
+ return devm_snd_soc_register_card(hdmi->dev, card);
+}
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index a7c4654445c7..79ecd89fb705 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
+
+ if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
+ DRM_ERROR("Couldn't create the HDMI audio adapter\n");
}
static void sun4i_hdmi_mode_set(struct drm_encoder *encoder,
@@ -218,6 +221,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector)
if (!edid)
return 0;
+ hdmi->hdmi_audio = drm_detect_monitor_audio(edid);
hdmi->hdmi_monitor = drm_detect_hdmi_monitor(edid);
DRM_DEBUG_DRIVER("Monitor is %s monitor\n",
hdmi->hdmi_monitor ? "an HDMI" : "a DVI");
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
2020-01-10 14:11 ` Stefan Mavrodiev
(?)
@ 2020-01-10 16:26 ` Maxime Ripard
-1 siblings, 0 replies; 42+ messages in thread
From: Maxime Ripard @ 2020-01-10 16:26 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: Dan Williams, Vinod Koul, Chen-Yu Tsai, David Airlie,
Daniel Vetter, open list,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
moderated list:ARM/Allwinner sunXi SoC support,
open list:DRM DRIVERS FOR ALLWINNER A10, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 5381 bytes --]
Hi,
On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
> Add HDMI audio support for the sun4i-hdmi encoder, used on
> the older Allwinner chips - A10, A20, A31.
>
> Most of the code is based on the BSP implementation. In it
> dditional formats are supported (S20_3LE and S24_LE), however
> there where some problems with them and only S16_LE is left.
>
> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> ---
> drivers/gpu/drm/sun4i/Kconfig | 1 +
> drivers/gpu/drm/sun4i/Makefile | 1 +
> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> 5 files changed, 411 insertions(+)
> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index 37e90e42943f..192b732b10cd 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -19,6 +19,7 @@ if DRM_SUN4I
> config DRM_SUN4I_HDMI
> tristate "Allwinner A10 HDMI Controller Support"
> default DRM_SUN4I
> + select SND_PCM_ELD
> help
> Choose this option if you have an Allwinner SoC with an HDMI
> controller.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 0d04f2447b01..e2d82b451c36 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> sun4i-drm-y += sun4i_drv.o
> sun4i-drm-y += sun4i_framebuffer.o
>
> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> index 7ad3f06c127e..456964e681b0 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -42,7 +42,32 @@
> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>
> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> +
> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
There's the issue multiple times in the headers, but you should wrap n
in parentheses to make sure we have no issue with precedence when
calling the macro.
> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
> +{
> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
> + struct snd_soc_dai_link_component *comp;
> + struct snd_soc_dai_link *link;
> + int ret;
> +
> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
> + &sun4i_hdmi_audio_pcm_config, 0);
> + if (ret) {
> + DRM_ERROR("Could not register PCM\n");
> + return ret;
> + }
> +
> + ret = devm_snd_soc_register_component(hdmi->dev,
> + &sun4i_hdmi_audio_component,
> + &sun4i_hdmi_audio_dai, 1);
> + if (ret) {
> + DRM_ERROR("Could not register DAI\n");
> + return ret;
> + }
> +
> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
> + if (!link)
> + return -ENOMEM;
> +
> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
> + if (!comp)
> + return -ENOMEM;
> +
> + link->cpus = &comp[0];
> + link->codecs = &comp[1];
> + link->platforms = &comp[2];
> +
> + link->num_cpus = 1;
> + link->num_codecs = 1;
> + link->num_platforms = 1;
> +
> + link->playback_only = 1;
> +
> + link->name = "SUN4I-HDMI";
> + link->stream_name = "SUN4I-HDMI PCM";
> +
> + link->codecs->name = dev_name(hdmi->dev);
> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
> +
> + link->cpus->dai_name = dev_name(hdmi->dev);
> +
> + link->platforms->name = dev_name(hdmi->dev);
> +
> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
> +
> + card->dai_link = link;
> + card->num_links = 1;
> + card->dev = hdmi->dev;
> +
> + snd_soc_card_set_drvdata(card, hdmi);
> + return devm_snd_soc_register_card(hdmi->dev, card);
Out of curiosity, did you try to remove the module with that patch
applied? IIRC, these functions will overwrite the device drvdata, and
we will try to access them in unbind / remove.
> +}
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> index a7c4654445c7..79ecd89fb705 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>
> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +
> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
So you create the audio card each time the display is enabled? I guess
this is to deal with the hotplug?
I'm not sure this is the right thing to do. If I remember well, the
ELD are here precisely to let userspace know that the display is
plugged (and audio-capable) or not.
Also, you don't remove that card in the disable, which mean that if
you end up in a situation where you would enable the display, disable
it and then enable it again, you have two audio cards now.
Thanks!
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-10 16:26 ` Maxime Ripard
0 siblings, 0 replies; 42+ messages in thread
From: Maxime Ripard @ 2020-01-10 16:26 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, open list, Chen-Yu Tsai,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Dan Williams,
moderated list:ARM/Allwinner sunXi SoC support
[-- Attachment #1.1: Type: text/plain, Size: 5381 bytes --]
Hi,
On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
> Add HDMI audio support for the sun4i-hdmi encoder, used on
> the older Allwinner chips - A10, A20, A31.
>
> Most of the code is based on the BSP implementation. In it
> dditional formats are supported (S20_3LE and S24_LE), however
> there where some problems with them and only S16_LE is left.
>
> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> ---
> drivers/gpu/drm/sun4i/Kconfig | 1 +
> drivers/gpu/drm/sun4i/Makefile | 1 +
> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> 5 files changed, 411 insertions(+)
> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index 37e90e42943f..192b732b10cd 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -19,6 +19,7 @@ if DRM_SUN4I
> config DRM_SUN4I_HDMI
> tristate "Allwinner A10 HDMI Controller Support"
> default DRM_SUN4I
> + select SND_PCM_ELD
> help
> Choose this option if you have an Allwinner SoC with an HDMI
> controller.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 0d04f2447b01..e2d82b451c36 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> sun4i-drm-y += sun4i_drv.o
> sun4i-drm-y += sun4i_framebuffer.o
>
> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> index 7ad3f06c127e..456964e681b0 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -42,7 +42,32 @@
> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>
> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> +
> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
There's the issue multiple times in the headers, but you should wrap n
in parentheses to make sure we have no issue with precedence when
calling the macro.
> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
> +{
> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
> + struct snd_soc_dai_link_component *comp;
> + struct snd_soc_dai_link *link;
> + int ret;
> +
> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
> + &sun4i_hdmi_audio_pcm_config, 0);
> + if (ret) {
> + DRM_ERROR("Could not register PCM\n");
> + return ret;
> + }
> +
> + ret = devm_snd_soc_register_component(hdmi->dev,
> + &sun4i_hdmi_audio_component,
> + &sun4i_hdmi_audio_dai, 1);
> + if (ret) {
> + DRM_ERROR("Could not register DAI\n");
> + return ret;
> + }
> +
> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
> + if (!link)
> + return -ENOMEM;
> +
> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
> + if (!comp)
> + return -ENOMEM;
> +
> + link->cpus = &comp[0];
> + link->codecs = &comp[1];
> + link->platforms = &comp[2];
> +
> + link->num_cpus = 1;
> + link->num_codecs = 1;
> + link->num_platforms = 1;
> +
> + link->playback_only = 1;
> +
> + link->name = "SUN4I-HDMI";
> + link->stream_name = "SUN4I-HDMI PCM";
> +
> + link->codecs->name = dev_name(hdmi->dev);
> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
> +
> + link->cpus->dai_name = dev_name(hdmi->dev);
> +
> + link->platforms->name = dev_name(hdmi->dev);
> +
> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
> +
> + card->dai_link = link;
> + card->num_links = 1;
> + card->dev = hdmi->dev;
> +
> + snd_soc_card_set_drvdata(card, hdmi);
> + return devm_snd_soc_register_card(hdmi->dev, card);
Out of curiosity, did you try to remove the module with that patch
applied? IIRC, these functions will overwrite the device drvdata, and
we will try to access them in unbind / remove.
> +}
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> index a7c4654445c7..79ecd89fb705 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>
> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +
> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
So you create the audio card each time the display is enabled? I guess
this is to deal with the hotplug?
I'm not sure this is the right thing to do. If I remember well, the
ELD are here precisely to let userspace know that the display is
plugged (and audio-capable) or not.
Also, you don't remove that card in the disable, which mean that if
you end up in a situation where you would enable the display, disable
it and then enable it again, you have two audio cards now.
Thanks!
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-10 16:26 ` Maxime Ripard
0 siblings, 0 replies; 42+ messages in thread
From: Maxime Ripard @ 2020-01-10 16:26 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, open list, Chen-Yu Tsai,
Daniel Vetter, open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
Dan Williams, moderated list:ARM/Allwinner sunXi SoC support
[-- Attachment #1.1: Type: text/plain, Size: 5381 bytes --]
Hi,
On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
> Add HDMI audio support for the sun4i-hdmi encoder, used on
> the older Allwinner chips - A10, A20, A31.
>
> Most of the code is based on the BSP implementation. In it
> dditional formats are supported (S20_3LE and S24_LE), however
> there where some problems with them and only S16_LE is left.
>
> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> ---
> drivers/gpu/drm/sun4i/Kconfig | 1 +
> drivers/gpu/drm/sun4i/Makefile | 1 +
> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> 5 files changed, 411 insertions(+)
> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index 37e90e42943f..192b732b10cd 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -19,6 +19,7 @@ if DRM_SUN4I
> config DRM_SUN4I_HDMI
> tristate "Allwinner A10 HDMI Controller Support"
> default DRM_SUN4I
> + select SND_PCM_ELD
> help
> Choose this option if you have an Allwinner SoC with an HDMI
> controller.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 0d04f2447b01..e2d82b451c36 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> sun4i-drm-y += sun4i_drv.o
> sun4i-drm-y += sun4i_framebuffer.o
>
> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> index 7ad3f06c127e..456964e681b0 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -42,7 +42,32 @@
> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>
> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> +
> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
There's the issue multiple times in the headers, but you should wrap n
in parentheses to make sure we have no issue with precedence when
calling the macro.
> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
> +{
> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
> + struct snd_soc_dai_link_component *comp;
> + struct snd_soc_dai_link *link;
> + int ret;
> +
> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
> + &sun4i_hdmi_audio_pcm_config, 0);
> + if (ret) {
> + DRM_ERROR("Could not register PCM\n");
> + return ret;
> + }
> +
> + ret = devm_snd_soc_register_component(hdmi->dev,
> + &sun4i_hdmi_audio_component,
> + &sun4i_hdmi_audio_dai, 1);
> + if (ret) {
> + DRM_ERROR("Could not register DAI\n");
> + return ret;
> + }
> +
> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
> + if (!link)
> + return -ENOMEM;
> +
> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
> + if (!comp)
> + return -ENOMEM;
> +
> + link->cpus = &comp[0];
> + link->codecs = &comp[1];
> + link->platforms = &comp[2];
> +
> + link->num_cpus = 1;
> + link->num_codecs = 1;
> + link->num_platforms = 1;
> +
> + link->playback_only = 1;
> +
> + link->name = "SUN4I-HDMI";
> + link->stream_name = "SUN4I-HDMI PCM";
> +
> + link->codecs->name = dev_name(hdmi->dev);
> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
> +
> + link->cpus->dai_name = dev_name(hdmi->dev);
> +
> + link->platforms->name = dev_name(hdmi->dev);
> +
> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
> +
> + card->dai_link = link;
> + card->num_links = 1;
> + card->dev = hdmi->dev;
> +
> + snd_soc_card_set_drvdata(card, hdmi);
> + return devm_snd_soc_register_card(hdmi->dev, card);
Out of curiosity, did you try to remove the module with that patch
applied? IIRC, these functions will overwrite the device drvdata, and
we will try to access them in unbind / remove.
> +}
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> index a7c4654445c7..79ecd89fb705 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>
> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +
> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
So you create the audio card each time the display is enabled? I guess
this is to deal with the hotplug?
I'm not sure this is the right thing to do. If I remember well, the
ELD are here precisely to let userspace know that the display is
plugged (and audio-capable) or not.
Also, you don't remove that card in the disable, which mean that if
you end up in a situation where you would enable the display, disable
it and then enable it again, you have two audio cards now.
Thanks!
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
2020-01-10 16:26 ` Maxime Ripard
(?)
@ 2020-01-14 9:04 ` Stefan Mavrodiev
-1 siblings, 0 replies; 42+ messages in thread
From: Stefan Mavrodiev @ 2020-01-14 9:04 UTC (permalink / raw)
To: Maxime Ripard, Stefan Mavrodiev
Cc: Dan Williams, Vinod Koul, Chen-Yu Tsai, David Airlie,
Daniel Vetter, open list,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
moderated list:ARM/Allwinner sunXi SoC support,
open list:DRM DRIVERS FOR ALLWINNER A10, linux-sunxi
Hi,
On 1/10/20 6:26 PM, Maxime Ripard wrote:
> Hi,
>
> On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
>> Add HDMI audio support for the sun4i-hdmi encoder, used on
>> the older Allwinner chips - A10, A20, A31.
>>
>> Most of the code is based on the BSP implementation. In it
>> dditional formats are supported (S20_3LE and S24_LE), however
>> there where some problems with them and only S16_LE is left.
>>
>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>> ---
>> drivers/gpu/drm/sun4i/Kconfig | 1 +
>> drivers/gpu/drm/sun4i/Makefile | 1 +
>> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
>> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
>> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
>> 5 files changed, 411 insertions(+)
>> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>>
>> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
>> index 37e90e42943f..192b732b10cd 100644
>> --- a/drivers/gpu/drm/sun4i/Kconfig
>> +++ b/drivers/gpu/drm/sun4i/Kconfig
>> @@ -19,6 +19,7 @@ if DRM_SUN4I
>> config DRM_SUN4I_HDMI
>> tristate "Allwinner A10 HDMI Controller Support"
>> default DRM_SUN4I
>> + select SND_PCM_ELD
>> help
>> Choose this option if you have an Allwinner SoC with an HDMI
>> controller.
>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>> index 0d04f2447b01..e2d82b451c36 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
>> sun4i-drm-y += sun4i_drv.o
>> sun4i-drm-y += sun4i_framebuffer.o
>>
>> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index 7ad3f06c127e..456964e681b0 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -42,7 +42,32 @@
>> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
>> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>>
>> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
>> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
>> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
>> +
>> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
>> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
>> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
>> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> There's the issue multiple times in the headers, but you should wrap n
> in parentheses to make sure we have no issue with precedence when
> calling the macro.
>
>> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
>> +{
>> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
>> + struct snd_soc_dai_link_component *comp;
>> + struct snd_soc_dai_link *link;
>> + int ret;
>> +
>> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
>> + &sun4i_hdmi_audio_pcm_config, 0);
>> + if (ret) {
>> + DRM_ERROR("Could not register PCM\n");
>> + return ret;
>> + }
>> +
>> + ret = devm_snd_soc_register_component(hdmi->dev,
>> + &sun4i_hdmi_audio_component,
>> + &sun4i_hdmi_audio_dai, 1);
>> + if (ret) {
>> + DRM_ERROR("Could not register DAI\n");
>> + return ret;
>> + }
>> +
>> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
>> + if (!link)
>> + return -ENOMEM;
>> +
>> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
>> + if (!comp)
>> + return -ENOMEM;
>> +
>> + link->cpus = &comp[0];
>> + link->codecs = &comp[1];
>> + link->platforms = &comp[2];
>> +
>> + link->num_cpus = 1;
>> + link->num_codecs = 1;
>> + link->num_platforms = 1;
>> +
>> + link->playback_only = 1;
>> +
>> + link->name = "SUN4I-HDMI";
>> + link->stream_name = "SUN4I-HDMI PCM";
>> +
>> + link->codecs->name = dev_name(hdmi->dev);
>> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
>> +
>> + link->cpus->dai_name = dev_name(hdmi->dev);
>> +
>> + link->platforms->name = dev_name(hdmi->dev);
>> +
>> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
>> +
>> + card->dai_link = link;
>> + card->num_links = 1;
>> + card->dev = hdmi->dev;
>> +
>> + snd_soc_card_set_drvdata(card, hdmi);
>> + return devm_snd_soc_register_card(hdmi->dev, card);
> Out of curiosity, did you try to remove the module with that patch
> applied? IIRC, these functions will overwrite the device drvdata, and
> we will try to access them in unbind / remove.
Actually I did not. Just tried that and you're right. The module crashes
at the unbind call.
I use sun4i_hdmi struct only for regmap. Maybe create separate private
structure and copy only
regmap?
>
>> +}
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index a7c4654445c7..79ecd89fb705 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
>> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>>
>> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
>> +
>> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
>> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
> So you create the audio card each time the display is enabled? I guess
> this is to deal with the hotplug?
Yes. See below.
>
> I'm not sure this is the right thing to do. If I remember well, the
> ELD are here precisely to let userspace know that the display is
> plugged (and audio-capable) or not.
>
> Also, you don't remove that card in the disable, which mean that if
> you end up in a situation where you would enable the display, disable
> it and then enable it again, you have two audio cards now.
There is issue with the hotplug. When inserting the cable, the event is
detected
and the hdmi encoder is enabled. Thus the card is created. However
further removal and
insertions are not detected. This is why I don't remove the card.
Also I count on devm_snd_soc_register_card() to release the card.
>
> Thanks!
> Maxime
Best regards,
Stefan
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-14 9:04 ` Stefan Mavrodiev
0 siblings, 0 replies; 42+ messages in thread
From: Stefan Mavrodiev @ 2020-01-14 9:04 UTC (permalink / raw)
To: Maxime Ripard, Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, open list, Chen-Yu Tsai,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Dan Williams,
moderated list:ARM/Allwinner sunXi SoC support
Hi,
On 1/10/20 6:26 PM, Maxime Ripard wrote:
> Hi,
>
> On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
>> Add HDMI audio support for the sun4i-hdmi encoder, used on
>> the older Allwinner chips - A10, A20, A31.
>>
>> Most of the code is based on the BSP implementation. In it
>> dditional formats are supported (S20_3LE and S24_LE), however
>> there where some problems with them and only S16_LE is left.
>>
>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>> ---
>> drivers/gpu/drm/sun4i/Kconfig | 1 +
>> drivers/gpu/drm/sun4i/Makefile | 1 +
>> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
>> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
>> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
>> 5 files changed, 411 insertions(+)
>> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>>
>> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
>> index 37e90e42943f..192b732b10cd 100644
>> --- a/drivers/gpu/drm/sun4i/Kconfig
>> +++ b/drivers/gpu/drm/sun4i/Kconfig
>> @@ -19,6 +19,7 @@ if DRM_SUN4I
>> config DRM_SUN4I_HDMI
>> tristate "Allwinner A10 HDMI Controller Support"
>> default DRM_SUN4I
>> + select SND_PCM_ELD
>> help
>> Choose this option if you have an Allwinner SoC with an HDMI
>> controller.
>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>> index 0d04f2447b01..e2d82b451c36 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
>> sun4i-drm-y += sun4i_drv.o
>> sun4i-drm-y += sun4i_framebuffer.o
>>
>> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index 7ad3f06c127e..456964e681b0 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -42,7 +42,32 @@
>> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
>> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>>
>> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
>> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
>> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
>> +
>> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
>> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
>> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
>> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> There's the issue multiple times in the headers, but you should wrap n
> in parentheses to make sure we have no issue with precedence when
> calling the macro.
>
>> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
>> +{
>> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
>> + struct snd_soc_dai_link_component *comp;
>> + struct snd_soc_dai_link *link;
>> + int ret;
>> +
>> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
>> + &sun4i_hdmi_audio_pcm_config, 0);
>> + if (ret) {
>> + DRM_ERROR("Could not register PCM\n");
>> + return ret;
>> + }
>> +
>> + ret = devm_snd_soc_register_component(hdmi->dev,
>> + &sun4i_hdmi_audio_component,
>> + &sun4i_hdmi_audio_dai, 1);
>> + if (ret) {
>> + DRM_ERROR("Could not register DAI\n");
>> + return ret;
>> + }
>> +
>> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
>> + if (!link)
>> + return -ENOMEM;
>> +
>> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
>> + if (!comp)
>> + return -ENOMEM;
>> +
>> + link->cpus = &comp[0];
>> + link->codecs = &comp[1];
>> + link->platforms = &comp[2];
>> +
>> + link->num_cpus = 1;
>> + link->num_codecs = 1;
>> + link->num_platforms = 1;
>> +
>> + link->playback_only = 1;
>> +
>> + link->name = "SUN4I-HDMI";
>> + link->stream_name = "SUN4I-HDMI PCM";
>> +
>> + link->codecs->name = dev_name(hdmi->dev);
>> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
>> +
>> + link->cpus->dai_name = dev_name(hdmi->dev);
>> +
>> + link->platforms->name = dev_name(hdmi->dev);
>> +
>> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
>> +
>> + card->dai_link = link;
>> + card->num_links = 1;
>> + card->dev = hdmi->dev;
>> +
>> + snd_soc_card_set_drvdata(card, hdmi);
>> + return devm_snd_soc_register_card(hdmi->dev, card);
> Out of curiosity, did you try to remove the module with that patch
> applied? IIRC, these functions will overwrite the device drvdata, and
> we will try to access them in unbind / remove.
Actually I did not. Just tried that and you're right. The module crashes
at the unbind call.
I use sun4i_hdmi struct only for regmap. Maybe create separate private
structure and copy only
regmap?
>
>> +}
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index a7c4654445c7..79ecd89fb705 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
>> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>>
>> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
>> +
>> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
>> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
> So you create the audio card each time the display is enabled? I guess
> this is to deal with the hotplug?
Yes. See below.
>
> I'm not sure this is the right thing to do. If I remember well, the
> ELD are here precisely to let userspace know that the display is
> plugged (and audio-capable) or not.
>
> Also, you don't remove that card in the disable, which mean that if
> you end up in a situation where you would enable the display, disable
> it and then enable it again, you have two audio cards now.
There is issue with the hotplug. When inserting the cable, the event is
detected
and the hdmi encoder is enabled. Thus the card is created. However
further removal and
insertions are not detected. This is why I don't remove the card.
Also I count on devm_snd_soc_register_card() to release the card.
>
> Thanks!
> Maxime
Best regards,
Stefan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-14 9:04 ` Stefan Mavrodiev
0 siblings, 0 replies; 42+ messages in thread
From: Stefan Mavrodiev @ 2020-01-14 9:04 UTC (permalink / raw)
To: Maxime Ripard, Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, open list, Chen-Yu Tsai,
Daniel Vetter, open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
Dan Williams, moderated list:ARM/Allwinner sunXi SoC support
Hi,
On 1/10/20 6:26 PM, Maxime Ripard wrote:
> Hi,
>
> On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
>> Add HDMI audio support for the sun4i-hdmi encoder, used on
>> the older Allwinner chips - A10, A20, A31.
>>
>> Most of the code is based on the BSP implementation. In it
>> dditional formats are supported (S20_3LE and S24_LE), however
>> there where some problems with them and only S16_LE is left.
>>
>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>> ---
>> drivers/gpu/drm/sun4i/Kconfig | 1 +
>> drivers/gpu/drm/sun4i/Makefile | 1 +
>> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
>> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
>> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
>> 5 files changed, 411 insertions(+)
>> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>>
>> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
>> index 37e90e42943f..192b732b10cd 100644
>> --- a/drivers/gpu/drm/sun4i/Kconfig
>> +++ b/drivers/gpu/drm/sun4i/Kconfig
>> @@ -19,6 +19,7 @@ if DRM_SUN4I
>> config DRM_SUN4I_HDMI
>> tristate "Allwinner A10 HDMI Controller Support"
>> default DRM_SUN4I
>> + select SND_PCM_ELD
>> help
>> Choose this option if you have an Allwinner SoC with an HDMI
>> controller.
>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>> index 0d04f2447b01..e2d82b451c36 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
>> sun4i-drm-y += sun4i_drv.o
>> sun4i-drm-y += sun4i_framebuffer.o
>>
>> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index 7ad3f06c127e..456964e681b0 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -42,7 +42,32 @@
>> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
>> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>>
>> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
>> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
>> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
>> +
>> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
>> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
>> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
>> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> There's the issue multiple times in the headers, but you should wrap n
> in parentheses to make sure we have no issue with precedence when
> calling the macro.
>
>> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
>> +{
>> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
>> + struct snd_soc_dai_link_component *comp;
>> + struct snd_soc_dai_link *link;
>> + int ret;
>> +
>> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
>> + &sun4i_hdmi_audio_pcm_config, 0);
>> + if (ret) {
>> + DRM_ERROR("Could not register PCM\n");
>> + return ret;
>> + }
>> +
>> + ret = devm_snd_soc_register_component(hdmi->dev,
>> + &sun4i_hdmi_audio_component,
>> + &sun4i_hdmi_audio_dai, 1);
>> + if (ret) {
>> + DRM_ERROR("Could not register DAI\n");
>> + return ret;
>> + }
>> +
>> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
>> + if (!link)
>> + return -ENOMEM;
>> +
>> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
>> + if (!comp)
>> + return -ENOMEM;
>> +
>> + link->cpus = &comp[0];
>> + link->codecs = &comp[1];
>> + link->platforms = &comp[2];
>> +
>> + link->num_cpus = 1;
>> + link->num_codecs = 1;
>> + link->num_platforms = 1;
>> +
>> + link->playback_only = 1;
>> +
>> + link->name = "SUN4I-HDMI";
>> + link->stream_name = "SUN4I-HDMI PCM";
>> +
>> + link->codecs->name = dev_name(hdmi->dev);
>> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
>> +
>> + link->cpus->dai_name = dev_name(hdmi->dev);
>> +
>> + link->platforms->name = dev_name(hdmi->dev);
>> +
>> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
>> +
>> + card->dai_link = link;
>> + card->num_links = 1;
>> + card->dev = hdmi->dev;
>> +
>> + snd_soc_card_set_drvdata(card, hdmi);
>> + return devm_snd_soc_register_card(hdmi->dev, card);
> Out of curiosity, did you try to remove the module with that patch
> applied? IIRC, these functions will overwrite the device drvdata, and
> we will try to access them in unbind / remove.
Actually I did not. Just tried that and you're right. The module crashes
at the unbind call.
I use sun4i_hdmi struct only for regmap. Maybe create separate private
structure and copy only
regmap?
>
>> +}
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index a7c4654445c7..79ecd89fb705 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
>> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>>
>> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
>> +
>> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
>> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
> So you create the audio card each time the display is enabled? I guess
> this is to deal with the hotplug?
Yes. See below.
>
> I'm not sure this is the right thing to do. If I remember well, the
> ELD are here precisely to let userspace know that the display is
> plugged (and audio-capable) or not.
>
> Also, you don't remove that card in the disable, which mean that if
> you end up in a situation where you would enable the display, disable
> it and then enable it again, you have two audio cards now.
There is issue with the hotplug. When inserting the cable, the event is
detected
and the hdmi encoder is enabled. Thus the card is created. However
further removal and
insertions are not detected. This is why I don't remove the card.
Also I count on devm_snd_soc_register_card() to release the card.
>
> Thanks!
> Maxime
Best regards,
Stefan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
2020-01-14 9:04 ` Stefan Mavrodiev
(?)
@ 2020-01-15 8:32 ` Maxime Ripard
-1 siblings, 0 replies; 42+ messages in thread
From: Maxime Ripard @ 2020-01-15 8:32 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: Dan Williams, Vinod Koul, Chen-Yu Tsai, David Airlie,
Daniel Vetter, open list,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
moderated list:ARM/Allwinner sunXi SoC support,
open list:DRM DRIVERS FOR ALLWINNER A10, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 8563 bytes --]
Hi Stefan,
On Tue, Jan 14, 2020 at 11:04:55AM +0200, Stefan Mavrodiev wrote:
> On 1/10/20 6:26 PM, Maxime Ripard wrote:
> > Hi,
> >
> > On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
> > > Add HDMI audio support for the sun4i-hdmi encoder, used on
> > > the older Allwinner chips - A10, A20, A31.
> > >
> > > Most of the code is based on the BSP implementation. In it
> > > dditional formats are supported (S20_3LE and S24_LE), however
> > > there where some problems with them and only S16_LE is left.
> > >
> > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> > > ---
> > > drivers/gpu/drm/sun4i/Kconfig | 1 +
> > > drivers/gpu/drm/sun4i/Makefile | 1 +
> > > drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> > > drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> > > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> > > 5 files changed, 411 insertions(+)
> > > create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> > > index 37e90e42943f..192b732b10cd 100644
> > > --- a/drivers/gpu/drm/sun4i/Kconfig
> > > +++ b/drivers/gpu/drm/sun4i/Kconfig
> > > @@ -19,6 +19,7 @@ if DRM_SUN4I
> > > config DRM_SUN4I_HDMI
> > > tristate "Allwinner A10 HDMI Controller Support"
> > > default DRM_SUN4I
> > > + select SND_PCM_ELD
> > > help
> > > Choose this option if you have an Allwinner SoC with an HDMI
> > > controller.
> > > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> > > index 0d04f2447b01..e2d82b451c36 100644
> > > --- a/drivers/gpu/drm/sun4i/Makefile
> > > +++ b/drivers/gpu/drm/sun4i/Makefile
> > > @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> > > sun4i-drm-y += sun4i_drv.o
> > > sun4i-drm-y += sun4i_framebuffer.o
> > >
> > > +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > index 7ad3f06c127e..456964e681b0 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > @@ -42,7 +42,32 @@
> > > #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> > > #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
> > >
> > > +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> > > +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> > > +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> > > +
> > > +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> > > +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> > > +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> > > +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> > There's the issue multiple times in the headers, but you should wrap n
> > in parentheses to make sure we have no issue with precedence when
> > calling the macro.
> >
> > > +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
> > > +{
> > > + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
> > > + struct snd_soc_dai_link_component *comp;
> > > + struct snd_soc_dai_link *link;
> > > + int ret;
> > > +
> > > + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
> > > + &sun4i_hdmi_audio_pcm_config, 0);
> > > + if (ret) {
> > > + DRM_ERROR("Could not register PCM\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = devm_snd_soc_register_component(hdmi->dev,
> > > + &sun4i_hdmi_audio_component,
> > > + &sun4i_hdmi_audio_dai, 1);
> > > + if (ret) {
> > > + DRM_ERROR("Could not register DAI\n");
> > > + return ret;
> > > + }
> > > +
> > > + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
> > > + if (!link)
> > > + return -ENOMEM;
> > > +
> > > + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
> > > + if (!comp)
> > > + return -ENOMEM;
> > > +
> > > + link->cpus = &comp[0];
> > > + link->codecs = &comp[1];
> > > + link->platforms = &comp[2];
> > > +
> > > + link->num_cpus = 1;
> > > + link->num_codecs = 1;
> > > + link->num_platforms = 1;
> > > +
> > > + link->playback_only = 1;
> > > +
> > > + link->name = "SUN4I-HDMI";
> > > + link->stream_name = "SUN4I-HDMI PCM";
> > > +
> > > + link->codecs->name = dev_name(hdmi->dev);
> > > + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
> > > +
> > > + link->cpus->dai_name = dev_name(hdmi->dev);
> > > +
> > > + link->platforms->name = dev_name(hdmi->dev);
> > > +
> > > + link->dai_fmt = SND_SOC_DAIFMT_I2S;
> > > +
> > > + card->dai_link = link;
> > > + card->num_links = 1;
> > > + card->dev = hdmi->dev;
> > > +
> > > + snd_soc_card_set_drvdata(card, hdmi);
> > > + return devm_snd_soc_register_card(hdmi->dev, card);
> >
> > Out of curiosity, did you try to remove the module with that patch
> > applied? IIRC, these functions will overwrite the device drvdata, and
> > we will try to access them in unbind / remove.
>
> Actually I did not. Just tried that and you're right. The module
> crashes at the unbind call. I use sun4i_hdmi struct only for
> regmap. Maybe create separate private structure and copy only
> regmap?
I think the issue is that:
- In bind, we first call dev_set_drvdata on the bound device, with a
pointer to struct sun4i_hdmi as the value. The driver_data field
in struct device is now a pointer to our instance of struct
sun4i_hdmi.
- In audio create, you then call snd_soc_card_set_drvdata with a
pointer to struct sun4i_hdmi as the value. The drvdata field in
the struct snd_soc_card is now a pointer to our instance of struct
sun4i_hdmi (so far so good).
- Then you call (devm_)snd_soc_register_card. One of the thing that
it will do is call drv_set_drvdata on the card->dev device,
setting it to our pointer to the struct snd_soc_card we provided.
However, since you set card->dev to the same device than the one
initially bound, this means that you just overwrote the struct
sun4i_hdmi pointer with a pointer to struct snd_soc_card.
- The driver will operate properly, since we never really use the
driver_data field, in the HDMI driver, except when...
- At unbind, you retrieve the driver_data field, expecting a struct
sun4i_hdmi pointer, except you have a pointer to struct
snd_soc_card, and everything explodes.
I think the way to work around that would be to create a new
(platform_)device for the HDMI audio component, so that ASoC can work
on that device instead.
This seems to be what dw-hdmi is doing here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2812
(Except that they are also using platform_data, since they have
multiple drivers, we wouldn't, so we can just lookup sun4i_hdmi using
the parent's device driver_data).
> > > +}
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > index a7c4654445c7..79ecd89fb705 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
> > > val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
> > >
> > > writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> > > +
> > > + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
> > > + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
> >
> > So you create the audio card each time the display is enabled? I guess
> > this is to deal with the hotplug?
>
> Yes. See below.
>
> > I'm not sure this is the right thing to do. If I remember well, the
> > ELD are here precisely to let userspace know that the display is
> > plugged (and audio-capable) or not.
> >
> > Also, you don't remove that card in the disable, which mean that if
> > you end up in a situation where you would enable the display, disable
> > it and then enable it again, you have two audio cards now.
>
> There is issue with the hotplug. When inserting the cable, the event
> is detected and the hdmi encoder is enabled. Thus the card is
> created. However further removal and insertions are not
> detected.
I guess we would need to fix that then?
> This is why I don't remove the card.
>
> Also I count on devm_snd_soc_register_card() to release the card.
I think you should really create the card all the time, and just
update the ELD to let the userspace know when something has been
created.
And yeah, we should have a working hotplug, but that's a separate
story :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-15 8:32 ` Maxime Ripard
0 siblings, 0 replies; 42+ messages in thread
From: Maxime Ripard @ 2020-01-15 8:32 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, open list, Chen-Yu Tsai,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Dan Williams,
moderated list:ARM/Allwinner sunXi SoC support
[-- Attachment #1.1: Type: text/plain, Size: 8563 bytes --]
Hi Stefan,
On Tue, Jan 14, 2020 at 11:04:55AM +0200, Stefan Mavrodiev wrote:
> On 1/10/20 6:26 PM, Maxime Ripard wrote:
> > Hi,
> >
> > On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
> > > Add HDMI audio support for the sun4i-hdmi encoder, used on
> > > the older Allwinner chips - A10, A20, A31.
> > >
> > > Most of the code is based on the BSP implementation. In it
> > > dditional formats are supported (S20_3LE and S24_LE), however
> > > there where some problems with them and only S16_LE is left.
> > >
> > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> > > ---
> > > drivers/gpu/drm/sun4i/Kconfig | 1 +
> > > drivers/gpu/drm/sun4i/Makefile | 1 +
> > > drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> > > drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> > > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> > > 5 files changed, 411 insertions(+)
> > > create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> > > index 37e90e42943f..192b732b10cd 100644
> > > --- a/drivers/gpu/drm/sun4i/Kconfig
> > > +++ b/drivers/gpu/drm/sun4i/Kconfig
> > > @@ -19,6 +19,7 @@ if DRM_SUN4I
> > > config DRM_SUN4I_HDMI
> > > tristate "Allwinner A10 HDMI Controller Support"
> > > default DRM_SUN4I
> > > + select SND_PCM_ELD
> > > help
> > > Choose this option if you have an Allwinner SoC with an HDMI
> > > controller.
> > > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> > > index 0d04f2447b01..e2d82b451c36 100644
> > > --- a/drivers/gpu/drm/sun4i/Makefile
> > > +++ b/drivers/gpu/drm/sun4i/Makefile
> > > @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> > > sun4i-drm-y += sun4i_drv.o
> > > sun4i-drm-y += sun4i_framebuffer.o
> > >
> > > +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > index 7ad3f06c127e..456964e681b0 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > @@ -42,7 +42,32 @@
> > > #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> > > #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
> > >
> > > +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> > > +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> > > +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> > > +
> > > +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> > > +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> > > +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> > > +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> > There's the issue multiple times in the headers, but you should wrap n
> > in parentheses to make sure we have no issue with precedence when
> > calling the macro.
> >
> > > +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
> > > +{
> > > + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
> > > + struct snd_soc_dai_link_component *comp;
> > > + struct snd_soc_dai_link *link;
> > > + int ret;
> > > +
> > > + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
> > > + &sun4i_hdmi_audio_pcm_config, 0);
> > > + if (ret) {
> > > + DRM_ERROR("Could not register PCM\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = devm_snd_soc_register_component(hdmi->dev,
> > > + &sun4i_hdmi_audio_component,
> > > + &sun4i_hdmi_audio_dai, 1);
> > > + if (ret) {
> > > + DRM_ERROR("Could not register DAI\n");
> > > + return ret;
> > > + }
> > > +
> > > + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
> > > + if (!link)
> > > + return -ENOMEM;
> > > +
> > > + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
> > > + if (!comp)
> > > + return -ENOMEM;
> > > +
> > > + link->cpus = &comp[0];
> > > + link->codecs = &comp[1];
> > > + link->platforms = &comp[2];
> > > +
> > > + link->num_cpus = 1;
> > > + link->num_codecs = 1;
> > > + link->num_platforms = 1;
> > > +
> > > + link->playback_only = 1;
> > > +
> > > + link->name = "SUN4I-HDMI";
> > > + link->stream_name = "SUN4I-HDMI PCM";
> > > +
> > > + link->codecs->name = dev_name(hdmi->dev);
> > > + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
> > > +
> > > + link->cpus->dai_name = dev_name(hdmi->dev);
> > > +
> > > + link->platforms->name = dev_name(hdmi->dev);
> > > +
> > > + link->dai_fmt = SND_SOC_DAIFMT_I2S;
> > > +
> > > + card->dai_link = link;
> > > + card->num_links = 1;
> > > + card->dev = hdmi->dev;
> > > +
> > > + snd_soc_card_set_drvdata(card, hdmi);
> > > + return devm_snd_soc_register_card(hdmi->dev, card);
> >
> > Out of curiosity, did you try to remove the module with that patch
> > applied? IIRC, these functions will overwrite the device drvdata, and
> > we will try to access them in unbind / remove.
>
> Actually I did not. Just tried that and you're right. The module
> crashes at the unbind call. I use sun4i_hdmi struct only for
> regmap. Maybe create separate private structure and copy only
> regmap?
I think the issue is that:
- In bind, we first call dev_set_drvdata on the bound device, with a
pointer to struct sun4i_hdmi as the value. The driver_data field
in struct device is now a pointer to our instance of struct
sun4i_hdmi.
- In audio create, you then call snd_soc_card_set_drvdata with a
pointer to struct sun4i_hdmi as the value. The drvdata field in
the struct snd_soc_card is now a pointer to our instance of struct
sun4i_hdmi (so far so good).
- Then you call (devm_)snd_soc_register_card. One of the thing that
it will do is call drv_set_drvdata on the card->dev device,
setting it to our pointer to the struct snd_soc_card we provided.
However, since you set card->dev to the same device than the one
initially bound, this means that you just overwrote the struct
sun4i_hdmi pointer with a pointer to struct snd_soc_card.
- The driver will operate properly, since we never really use the
driver_data field, in the HDMI driver, except when...
- At unbind, you retrieve the driver_data field, expecting a struct
sun4i_hdmi pointer, except you have a pointer to struct
snd_soc_card, and everything explodes.
I think the way to work around that would be to create a new
(platform_)device for the HDMI audio component, so that ASoC can work
on that device instead.
This seems to be what dw-hdmi is doing here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2812
(Except that they are also using platform_data, since they have
multiple drivers, we wouldn't, so we can just lookup sun4i_hdmi using
the parent's device driver_data).
> > > +}
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > index a7c4654445c7..79ecd89fb705 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
> > > val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
> > >
> > > writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> > > +
> > > + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
> > > + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
> >
> > So you create the audio card each time the display is enabled? I guess
> > this is to deal with the hotplug?
>
> Yes. See below.
>
> > I'm not sure this is the right thing to do. If I remember well, the
> > ELD are here precisely to let userspace know that the display is
> > plugged (and audio-capable) or not.
> >
> > Also, you don't remove that card in the disable, which mean that if
> > you end up in a situation where you would enable the display, disable
> > it and then enable it again, you have two audio cards now.
>
> There is issue with the hotplug. When inserting the cable, the event
> is detected and the hdmi encoder is enabled. Thus the card is
> created. However further removal and insertions are not
> detected.
I guess we would need to fix that then?
> This is why I don't remove the card.
>
> Also I count on devm_snd_soc_register_card() to release the card.
I think you should really create the card all the time, and just
update the ELD to let the userspace know when something has been
created.
And yeah, we should have a working hotplug, but that's a separate
story :)
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-15 8:32 ` Maxime Ripard
0 siblings, 0 replies; 42+ messages in thread
From: Maxime Ripard @ 2020-01-15 8:32 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, open list, Chen-Yu Tsai,
Daniel Vetter, open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
Dan Williams, moderated list:ARM/Allwinner sunXi SoC support
[-- Attachment #1.1: Type: text/plain, Size: 8563 bytes --]
Hi Stefan,
On Tue, Jan 14, 2020 at 11:04:55AM +0200, Stefan Mavrodiev wrote:
> On 1/10/20 6:26 PM, Maxime Ripard wrote:
> > Hi,
> >
> > On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
> > > Add HDMI audio support for the sun4i-hdmi encoder, used on
> > > the older Allwinner chips - A10, A20, A31.
> > >
> > > Most of the code is based on the BSP implementation. In it
> > > dditional formats are supported (S20_3LE and S24_LE), however
> > > there where some problems with them and only S16_LE is left.
> > >
> > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> > > ---
> > > drivers/gpu/drm/sun4i/Kconfig | 1 +
> > > drivers/gpu/drm/sun4i/Makefile | 1 +
> > > drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> > > drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> > > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> > > 5 files changed, 411 insertions(+)
> > > create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> > > index 37e90e42943f..192b732b10cd 100644
> > > --- a/drivers/gpu/drm/sun4i/Kconfig
> > > +++ b/drivers/gpu/drm/sun4i/Kconfig
> > > @@ -19,6 +19,7 @@ if DRM_SUN4I
> > > config DRM_SUN4I_HDMI
> > > tristate "Allwinner A10 HDMI Controller Support"
> > > default DRM_SUN4I
> > > + select SND_PCM_ELD
> > > help
> > > Choose this option if you have an Allwinner SoC with an HDMI
> > > controller.
> > > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> > > index 0d04f2447b01..e2d82b451c36 100644
> > > --- a/drivers/gpu/drm/sun4i/Makefile
> > > +++ b/drivers/gpu/drm/sun4i/Makefile
> > > @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> > > sun4i-drm-y += sun4i_drv.o
> > > sun4i-drm-y += sun4i_framebuffer.o
> > >
> > > +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> > > sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > index 7ad3f06c127e..456964e681b0 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> > > @@ -42,7 +42,32 @@
> > > #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> > > #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
> > >
> > > +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> > > +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> > > +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> > > +
> > > +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> > > +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> > > +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> > > +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> > There's the issue multiple times in the headers, but you should wrap n
> > in parentheses to make sure we have no issue with precedence when
> > calling the macro.
> >
> > > +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
> > > +{
> > > + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
> > > + struct snd_soc_dai_link_component *comp;
> > > + struct snd_soc_dai_link *link;
> > > + int ret;
> > > +
> > > + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
> > > + &sun4i_hdmi_audio_pcm_config, 0);
> > > + if (ret) {
> > > + DRM_ERROR("Could not register PCM\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = devm_snd_soc_register_component(hdmi->dev,
> > > + &sun4i_hdmi_audio_component,
> > > + &sun4i_hdmi_audio_dai, 1);
> > > + if (ret) {
> > > + DRM_ERROR("Could not register DAI\n");
> > > + return ret;
> > > + }
> > > +
> > > + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
> > > + if (!link)
> > > + return -ENOMEM;
> > > +
> > > + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
> > > + if (!comp)
> > > + return -ENOMEM;
> > > +
> > > + link->cpus = &comp[0];
> > > + link->codecs = &comp[1];
> > > + link->platforms = &comp[2];
> > > +
> > > + link->num_cpus = 1;
> > > + link->num_codecs = 1;
> > > + link->num_platforms = 1;
> > > +
> > > + link->playback_only = 1;
> > > +
> > > + link->name = "SUN4I-HDMI";
> > > + link->stream_name = "SUN4I-HDMI PCM";
> > > +
> > > + link->codecs->name = dev_name(hdmi->dev);
> > > + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
> > > +
> > > + link->cpus->dai_name = dev_name(hdmi->dev);
> > > +
> > > + link->platforms->name = dev_name(hdmi->dev);
> > > +
> > > + link->dai_fmt = SND_SOC_DAIFMT_I2S;
> > > +
> > > + card->dai_link = link;
> > > + card->num_links = 1;
> > > + card->dev = hdmi->dev;
> > > +
> > > + snd_soc_card_set_drvdata(card, hdmi);
> > > + return devm_snd_soc_register_card(hdmi->dev, card);
> >
> > Out of curiosity, did you try to remove the module with that patch
> > applied? IIRC, these functions will overwrite the device drvdata, and
> > we will try to access them in unbind / remove.
>
> Actually I did not. Just tried that and you're right. The module
> crashes at the unbind call. I use sun4i_hdmi struct only for
> regmap. Maybe create separate private structure and copy only
> regmap?
I think the issue is that:
- In bind, we first call dev_set_drvdata on the bound device, with a
pointer to struct sun4i_hdmi as the value. The driver_data field
in struct device is now a pointer to our instance of struct
sun4i_hdmi.
- In audio create, you then call snd_soc_card_set_drvdata with a
pointer to struct sun4i_hdmi as the value. The drvdata field in
the struct snd_soc_card is now a pointer to our instance of struct
sun4i_hdmi (so far so good).
- Then you call (devm_)snd_soc_register_card. One of the thing that
it will do is call drv_set_drvdata on the card->dev device,
setting it to our pointer to the struct snd_soc_card we provided.
However, since you set card->dev to the same device than the one
initially bound, this means that you just overwrote the struct
sun4i_hdmi pointer with a pointer to struct snd_soc_card.
- The driver will operate properly, since we never really use the
driver_data field, in the HDMI driver, except when...
- At unbind, you retrieve the driver_data field, expecting a struct
sun4i_hdmi pointer, except you have a pointer to struct
snd_soc_card, and everything explodes.
I think the way to work around that would be to create a new
(platform_)device for the HDMI audio component, so that ASoC can work
on that device instead.
This seems to be what dw-hdmi is doing here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2812
(Except that they are also using platform_data, since they have
multiple drivers, we wouldn't, so we can just lookup sun4i_hdmi using
the parent's device driver_data).
> > > +}
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > index a7c4654445c7..79ecd89fb705 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > > @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
> > > val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
> > >
> > > writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> > > +
> > > + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
> > > + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
> >
> > So you create the audio card each time the display is enabled? I guess
> > this is to deal with the hotplug?
>
> Yes. See below.
>
> > I'm not sure this is the right thing to do. If I remember well, the
> > ELD are here precisely to let userspace know that the display is
> > plugged (and audio-capable) or not.
> >
> > Also, you don't remove that card in the disable, which mean that if
> > you end up in a situation where you would enable the display, disable
> > it and then enable it again, you have two audio cards now.
>
> There is issue with the hotplug. When inserting the cable, the event
> is detected and the hdmi encoder is enabled. Thus the card is
> created. However further removal and insertions are not
> detected.
I guess we would need to fix that then?
> This is why I don't remove the card.
>
> Also I count on devm_snd_soc_register_card() to release the card.
I think you should really create the card all the time, and just
update the ELD to let the userspace know when something has been
created.
And yeah, we should have a working hotplug, but that's a separate
story :)
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
2020-01-15 8:32 ` Maxime Ripard
(?)
@ 2020-01-15 12:23 ` Stefan Mavrodiev
-1 siblings, 0 replies; 42+ messages in thread
From: Stefan Mavrodiev @ 2020-01-15 12:23 UTC (permalink / raw)
To: Maxime Ripard, Stefan Mavrodiev
Cc: Dan Williams, Vinod Koul, Chen-Yu Tsai, David Airlie,
Daniel Vetter, open list,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
moderated list:ARM/Allwinner sunXi SoC support,
open list:DRM DRIVERS FOR ALLWINNER A10, linux-sunxi
Hi,
On 1/15/20 10:32 AM, Maxime Ripard wrote:
> Hi Stefan,
>
> On Tue, Jan 14, 2020 at 11:04:55AM +0200, Stefan Mavrodiev wrote:
>> On 1/10/20 6:26 PM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
>>>> Add HDMI audio support for the sun4i-hdmi encoder, used on
>>>> the older Allwinner chips - A10, A20, A31.
>>>>
>>>> Most of the code is based on the BSP implementation. In it
>>>> dditional formats are supported (S20_3LE and S24_LE), however
>>>> there where some problems with them and only S16_LE is left.
>>>>
>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>>> ---
>>>> drivers/gpu/drm/sun4i/Kconfig | 1 +
>>>> drivers/gpu/drm/sun4i/Makefile | 1 +
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
>>>> 5 files changed, 411 insertions(+)
>>>> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>>>>
>>>> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
>>>> index 37e90e42943f..192b732b10cd 100644
>>>> --- a/drivers/gpu/drm/sun4i/Kconfig
>>>> +++ b/drivers/gpu/drm/sun4i/Kconfig
>>>> @@ -19,6 +19,7 @@ if DRM_SUN4I
>>>> config DRM_SUN4I_HDMI
>>>> tristate "Allwinner A10 HDMI Controller Support"
>>>> default DRM_SUN4I
>>>> + select SND_PCM_ELD
>>>> help
>>>> Choose this option if you have an Allwinner SoC with an HDMI
>>>> controller.
>>>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>>>> index 0d04f2447b01..e2d82b451c36 100644
>>>> --- a/drivers/gpu/drm/sun4i/Makefile
>>>> +++ b/drivers/gpu/drm/sun4i/Makefile
>>>> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
>>>> sun4i-drm-y += sun4i_drv.o
>>>> sun4i-drm-y += sun4i_framebuffer.o
>>>>
>>>> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> index 7ad3f06c127e..456964e681b0 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> @@ -42,7 +42,32 @@
>>>> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
>>>> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>>>>
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
>>>> +
>>>> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
>>>> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
>>>> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
>>>> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
>>> There's the issue multiple times in the headers, but you should wrap n
>>> in parentheses to make sure we have no issue with precedence when
>>> calling the macro.
>>>
>>>> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
>>>> +{
>>>> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
>>>> + struct snd_soc_dai_link_component *comp;
>>>> + struct snd_soc_dai_link *link;
>>>> + int ret;
>>>> +
>>>> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
>>>> + &sun4i_hdmi_audio_pcm_config, 0);
>>>> + if (ret) {
>>>> + DRM_ERROR("Could not register PCM\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = devm_snd_soc_register_component(hdmi->dev,
>>>> + &sun4i_hdmi_audio_component,
>>>> + &sun4i_hdmi_audio_dai, 1);
>>>> + if (ret) {
>>>> + DRM_ERROR("Could not register DAI\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
>>>> + if (!link)
>>>> + return -ENOMEM;
>>>> +
>>>> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
>>>> + if (!comp)
>>>> + return -ENOMEM;
>>>> +
>>>> + link->cpus = &comp[0];
>>>> + link->codecs = &comp[1];
>>>> + link->platforms = &comp[2];
>>>> +
>>>> + link->num_cpus = 1;
>>>> + link->num_codecs = 1;
>>>> + link->num_platforms = 1;
>>>> +
>>>> + link->playback_only = 1;
>>>> +
>>>> + link->name = "SUN4I-HDMI";
>>>> + link->stream_name = "SUN4I-HDMI PCM";
>>>> +
>>>> + link->codecs->name = dev_name(hdmi->dev);
>>>> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
>>>> +
>>>> + link->cpus->dai_name = dev_name(hdmi->dev);
>>>> +
>>>> + link->platforms->name = dev_name(hdmi->dev);
>>>> +
>>>> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
>>>> +
>>>> + card->dai_link = link;
>>>> + card->num_links = 1;
>>>> + card->dev = hdmi->dev;
>>>> +
>>>> + snd_soc_card_set_drvdata(card, hdmi);
>>>> + return devm_snd_soc_register_card(hdmi->dev, card);
>>> Out of curiosity, did you try to remove the module with that patch
>>> applied? IIRC, these functions will overwrite the device drvdata, and
>>> we will try to access them in unbind / remove.
>> Actually I did not. Just tried that and you're right. The module
>> crashes at the unbind call. I use sun4i_hdmi struct only for
>> regmap. Maybe create separate private structure and copy only
>> regmap?
> I think the issue is that:
>
> - In bind, we first call dev_set_drvdata on the bound device, with a
> pointer to struct sun4i_hdmi as the value. The driver_data field
> in struct device is now a pointer to our instance of struct
> sun4i_hdmi.
>
> - In audio create, you then call snd_soc_card_set_drvdata with a
> pointer to struct sun4i_hdmi as the value. The drvdata field in
> the struct snd_soc_card is now a pointer to our instance of struct
> sun4i_hdmi (so far so good).
>
> - Then you call (devm_)snd_soc_register_card. One of the thing that
> it will do is call drv_set_drvdata on the card->dev device,
> setting it to our pointer to the struct snd_soc_card we provided.
> However, since you set card->dev to the same device than the one
> initially bound, this means that you just overwrote the struct
> sun4i_hdmi pointer with a pointer to struct snd_soc_card.
>
> - The driver will operate properly, since we never really use the
> driver_data field, in the HDMI driver, except when...
>
> - At unbind, you retrieve the driver_data field, expecting a struct
> sun4i_hdmi pointer, except you have a pointer to struct
> snd_soc_card, and everything explodes.
>
> I think the way to work around that would be to create a new
> (platform_)device for the HDMI audio component, so that ASoC can work
> on that device instead.
>
> This seems to be what dw-hdmi is doing here:
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2812
>
> (Except that they are also using platform_data, since they have
> multiple drivers, we wouldn't, so we can just lookup sun4i_hdmi using
> the parent's device driver_data).
>
>>>> +}
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> index a7c4654445c7..79ecd89fb705 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
>>>> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>>>>
>>>> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
>>>> +
>>>> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
>>>> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
>>> So you create the audio card each time the display is enabled? I guess
>>> this is to deal with the hotplug?
>> Yes. See below.
>>
>>> I'm not sure this is the right thing to do. If I remember well, the
>>> ELD are here precisely to let userspace know that the display is
>>> plugged (and audio-capable) or not.
>>>
>>> Also, you don't remove that card in the disable, which mean that if
>>> you end up in a situation where you would enable the display, disable
>>> it and then enable it again, you have two audio cards now.
>> There is issue with the hotplug. When inserting the cable, the event
>> is detected and the hdmi encoder is enabled. Thus the card is
>> created. However further removal and insertions are not
>> detected.
> I guess we would need to fix that then?
>
>> This is why I don't remove the card.
>>
>> Also I count on devm_snd_soc_register_card() to release the card.
> I think you should really create the card all the time, and just
> update the ELD to let the userspace know when something has been
> created.
>
> And yeah, we should have a working hotplug, but that's a separate
> story :)
Thank you for the review. Soon I'll prepare v2.
Also I'll check the hotplug issue.
>
> Maxime
Best regards,
Stefan
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-15 12:23 ` Stefan Mavrodiev
0 siblings, 0 replies; 42+ messages in thread
From: Stefan Mavrodiev @ 2020-01-15 12:23 UTC (permalink / raw)
To: Maxime Ripard, Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, open list, Chen-Yu Tsai,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Dan Williams,
moderated list:ARM/Allwinner sunXi SoC support
Hi,
On 1/15/20 10:32 AM, Maxime Ripard wrote:
> Hi Stefan,
>
> On Tue, Jan 14, 2020 at 11:04:55AM +0200, Stefan Mavrodiev wrote:
>> On 1/10/20 6:26 PM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
>>>> Add HDMI audio support for the sun4i-hdmi encoder, used on
>>>> the older Allwinner chips - A10, A20, A31.
>>>>
>>>> Most of the code is based on the BSP implementation. In it
>>>> dditional formats are supported (S20_3LE and S24_LE), however
>>>> there where some problems with them and only S16_LE is left.
>>>>
>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>>> ---
>>>> drivers/gpu/drm/sun4i/Kconfig | 1 +
>>>> drivers/gpu/drm/sun4i/Makefile | 1 +
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
>>>> 5 files changed, 411 insertions(+)
>>>> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>>>>
>>>> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
>>>> index 37e90e42943f..192b732b10cd 100644
>>>> --- a/drivers/gpu/drm/sun4i/Kconfig
>>>> +++ b/drivers/gpu/drm/sun4i/Kconfig
>>>> @@ -19,6 +19,7 @@ if DRM_SUN4I
>>>> config DRM_SUN4I_HDMI
>>>> tristate "Allwinner A10 HDMI Controller Support"
>>>> default DRM_SUN4I
>>>> + select SND_PCM_ELD
>>>> help
>>>> Choose this option if you have an Allwinner SoC with an HDMI
>>>> controller.
>>>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>>>> index 0d04f2447b01..e2d82b451c36 100644
>>>> --- a/drivers/gpu/drm/sun4i/Makefile
>>>> +++ b/drivers/gpu/drm/sun4i/Makefile
>>>> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
>>>> sun4i-drm-y += sun4i_drv.o
>>>> sun4i-drm-y += sun4i_framebuffer.o
>>>>
>>>> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> index 7ad3f06c127e..456964e681b0 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> @@ -42,7 +42,32 @@
>>>> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
>>>> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>>>>
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
>>>> +
>>>> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
>>>> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
>>>> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
>>>> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
>>> There's the issue multiple times in the headers, but you should wrap n
>>> in parentheses to make sure we have no issue with precedence when
>>> calling the macro.
>>>
>>>> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
>>>> +{
>>>> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
>>>> + struct snd_soc_dai_link_component *comp;
>>>> + struct snd_soc_dai_link *link;
>>>> + int ret;
>>>> +
>>>> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
>>>> + &sun4i_hdmi_audio_pcm_config, 0);
>>>> + if (ret) {
>>>> + DRM_ERROR("Could not register PCM\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = devm_snd_soc_register_component(hdmi->dev,
>>>> + &sun4i_hdmi_audio_component,
>>>> + &sun4i_hdmi_audio_dai, 1);
>>>> + if (ret) {
>>>> + DRM_ERROR("Could not register DAI\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
>>>> + if (!link)
>>>> + return -ENOMEM;
>>>> +
>>>> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
>>>> + if (!comp)
>>>> + return -ENOMEM;
>>>> +
>>>> + link->cpus = &comp[0];
>>>> + link->codecs = &comp[1];
>>>> + link->platforms = &comp[2];
>>>> +
>>>> + link->num_cpus = 1;
>>>> + link->num_codecs = 1;
>>>> + link->num_platforms = 1;
>>>> +
>>>> + link->playback_only = 1;
>>>> +
>>>> + link->name = "SUN4I-HDMI";
>>>> + link->stream_name = "SUN4I-HDMI PCM";
>>>> +
>>>> + link->codecs->name = dev_name(hdmi->dev);
>>>> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
>>>> +
>>>> + link->cpus->dai_name = dev_name(hdmi->dev);
>>>> +
>>>> + link->platforms->name = dev_name(hdmi->dev);
>>>> +
>>>> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
>>>> +
>>>> + card->dai_link = link;
>>>> + card->num_links = 1;
>>>> + card->dev = hdmi->dev;
>>>> +
>>>> + snd_soc_card_set_drvdata(card, hdmi);
>>>> + return devm_snd_soc_register_card(hdmi->dev, card);
>>> Out of curiosity, did you try to remove the module with that patch
>>> applied? IIRC, these functions will overwrite the device drvdata, and
>>> we will try to access them in unbind / remove.
>> Actually I did not. Just tried that and you're right. The module
>> crashes at the unbind call. I use sun4i_hdmi struct only for
>> regmap. Maybe create separate private structure and copy only
>> regmap?
> I think the issue is that:
>
> - In bind, we first call dev_set_drvdata on the bound device, with a
> pointer to struct sun4i_hdmi as the value. The driver_data field
> in struct device is now a pointer to our instance of struct
> sun4i_hdmi.
>
> - In audio create, you then call snd_soc_card_set_drvdata with a
> pointer to struct sun4i_hdmi as the value. The drvdata field in
> the struct snd_soc_card is now a pointer to our instance of struct
> sun4i_hdmi (so far so good).
>
> - Then you call (devm_)snd_soc_register_card. One of the thing that
> it will do is call drv_set_drvdata on the card->dev device,
> setting it to our pointer to the struct snd_soc_card we provided.
> However, since you set card->dev to the same device than the one
> initially bound, this means that you just overwrote the struct
> sun4i_hdmi pointer with a pointer to struct snd_soc_card.
>
> - The driver will operate properly, since we never really use the
> driver_data field, in the HDMI driver, except when...
>
> - At unbind, you retrieve the driver_data field, expecting a struct
> sun4i_hdmi pointer, except you have a pointer to struct
> snd_soc_card, and everything explodes.
>
> I think the way to work around that would be to create a new
> (platform_)device for the HDMI audio component, so that ASoC can work
> on that device instead.
>
> This seems to be what dw-hdmi is doing here:
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2812
>
> (Except that they are also using platform_data, since they have
> multiple drivers, we wouldn't, so we can just lookup sun4i_hdmi using
> the parent's device driver_data).
>
>>>> +}
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> index a7c4654445c7..79ecd89fb705 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
>>>> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>>>>
>>>> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
>>>> +
>>>> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
>>>> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
>>> So you create the audio card each time the display is enabled? I guess
>>> this is to deal with the hotplug?
>> Yes. See below.
>>
>>> I'm not sure this is the right thing to do. If I remember well, the
>>> ELD are here precisely to let userspace know that the display is
>>> plugged (and audio-capable) or not.
>>>
>>> Also, you don't remove that card in the disable, which mean that if
>>> you end up in a situation where you would enable the display, disable
>>> it and then enable it again, you have two audio cards now.
>> There is issue with the hotplug. When inserting the cable, the event
>> is detected and the hdmi encoder is enabled. Thus the card is
>> created. However further removal and insertions are not
>> detected.
> I guess we would need to fix that then?
>
>> This is why I don't remove the card.
>>
>> Also I count on devm_snd_soc_register_card() to release the card.
> I think you should really create the card all the time, and just
> update the ELD to let the userspace know when something has been
> created.
>
> And yeah, we should have a working hotplug, but that's a separate
> story :)
Thank you for the review. Soon I'll prepare v2.
Also I'll check the hotplug issue.
>
> Maxime
Best regards,
Stefan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-15 12:23 ` Stefan Mavrodiev
0 siblings, 0 replies; 42+ messages in thread
From: Stefan Mavrodiev @ 2020-01-15 12:23 UTC (permalink / raw)
To: Maxime Ripard, Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, open list, Chen-Yu Tsai,
Daniel Vetter, open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
Dan Williams, moderated list:ARM/Allwinner sunXi SoC support
Hi,
On 1/15/20 10:32 AM, Maxime Ripard wrote:
> Hi Stefan,
>
> On Tue, Jan 14, 2020 at 11:04:55AM +0200, Stefan Mavrodiev wrote:
>> On 1/10/20 6:26 PM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Fri, Jan 10, 2020 at 04:11:40PM +0200, Stefan Mavrodiev wrote:
>>>> Add HDMI audio support for the sun4i-hdmi encoder, used on
>>>> the older Allwinner chips - A10, A20, A31.
>>>>
>>>> Most of the code is based on the BSP implementation. In it
>>>> dditional formats are supported (S20_3LE and S24_LE), however
>>>> there where some problems with them and only S16_LE is left.
>>>>
>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>>> ---
>>>> drivers/gpu/drm/sun4i/Kconfig | 1 +
>>>> drivers/gpu/drm/sun4i/Makefile | 1 +
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
>>>> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
>>>> 5 files changed, 411 insertions(+)
>>>> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>>>>
>>>> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
>>>> index 37e90e42943f..192b732b10cd 100644
>>>> --- a/drivers/gpu/drm/sun4i/Kconfig
>>>> +++ b/drivers/gpu/drm/sun4i/Kconfig
>>>> @@ -19,6 +19,7 @@ if DRM_SUN4I
>>>> config DRM_SUN4I_HDMI
>>>> tristate "Allwinner A10 HDMI Controller Support"
>>>> default DRM_SUN4I
>>>> + select SND_PCM_ELD
>>>> help
>>>> Choose this option if you have an Allwinner SoC with an HDMI
>>>> controller.
>>>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>>>> index 0d04f2447b01..e2d82b451c36 100644
>>>> --- a/drivers/gpu/drm/sun4i/Makefile
>>>> +++ b/drivers/gpu/drm/sun4i/Makefile
>>>> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
>>>> sun4i-drm-y += sun4i_drv.o
>>>> sun4i-drm-y += sun4i_framebuffer.o
>>>>
>>>> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>>>> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> index 7ad3f06c127e..456964e681b0 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>>>> @@ -42,7 +42,32 @@
>>>> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
>>>> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>>>>
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
>>>> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
>>>> +
>>>> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
>>>> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
>>>> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
>>>> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
>>> There's the issue multiple times in the headers, but you should wrap n
>>> in parentheses to make sure we have no issue with precedence when
>>> calling the macro.
>>>
>>>> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi)
>>>> +{
>>>> + struct snd_soc_card *card = &sun4i_hdmi_audio_card;
>>>> + struct snd_soc_dai_link_component *comp;
>>>> + struct snd_soc_dai_link *link;
>>>> + int ret;
>>>> +
>>>> + ret = devm_snd_dmaengine_pcm_register(hdmi->dev,
>>>> + &sun4i_hdmi_audio_pcm_config, 0);
>>>> + if (ret) {
>>>> + DRM_ERROR("Could not register PCM\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = devm_snd_soc_register_component(hdmi->dev,
>>>> + &sun4i_hdmi_audio_component,
>>>> + &sun4i_hdmi_audio_dai, 1);
>>>> + if (ret) {
>>>> + DRM_ERROR("Could not register DAI\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + link = devm_kzalloc(hdmi->dev, sizeof(*link), GFP_KERNEL);
>>>> + if (!link)
>>>> + return -ENOMEM;
>>>> +
>>>> + comp = devm_kzalloc(hdmi->dev, sizeof(*comp) * 3, GFP_KERNEL);
>>>> + if (!comp)
>>>> + return -ENOMEM;
>>>> +
>>>> + link->cpus = &comp[0];
>>>> + link->codecs = &comp[1];
>>>> + link->platforms = &comp[2];
>>>> +
>>>> + link->num_cpus = 1;
>>>> + link->num_codecs = 1;
>>>> + link->num_platforms = 1;
>>>> +
>>>> + link->playback_only = 1;
>>>> +
>>>> + link->name = "SUN4I-HDMI";
>>>> + link->stream_name = "SUN4I-HDMI PCM";
>>>> +
>>>> + link->codecs->name = dev_name(hdmi->dev);
>>>> + link->codecs->dai_name = sun4i_hdmi_audio_dai.name;
>>>> +
>>>> + link->cpus->dai_name = dev_name(hdmi->dev);
>>>> +
>>>> + link->platforms->name = dev_name(hdmi->dev);
>>>> +
>>>> + link->dai_fmt = SND_SOC_DAIFMT_I2S;
>>>> +
>>>> + card->dai_link = link;
>>>> + card->num_links = 1;
>>>> + card->dev = hdmi->dev;
>>>> +
>>>> + snd_soc_card_set_drvdata(card, hdmi);
>>>> + return devm_snd_soc_register_card(hdmi->dev, card);
>>> Out of curiosity, did you try to remove the module with that patch
>>> applied? IIRC, these functions will overwrite the device drvdata, and
>>> we will try to access them in unbind / remove.
>> Actually I did not. Just tried that and you're right. The module
>> crashes at the unbind call. I use sun4i_hdmi struct only for
>> regmap. Maybe create separate private structure and copy only
>> regmap?
> I think the issue is that:
>
> - In bind, we first call dev_set_drvdata on the bound device, with a
> pointer to struct sun4i_hdmi as the value. The driver_data field
> in struct device is now a pointer to our instance of struct
> sun4i_hdmi.
>
> - In audio create, you then call snd_soc_card_set_drvdata with a
> pointer to struct sun4i_hdmi as the value. The drvdata field in
> the struct snd_soc_card is now a pointer to our instance of struct
> sun4i_hdmi (so far so good).
>
> - Then you call (devm_)snd_soc_register_card. One of the thing that
> it will do is call drv_set_drvdata on the card->dev device,
> setting it to our pointer to the struct snd_soc_card we provided.
> However, since you set card->dev to the same device than the one
> initially bound, this means that you just overwrote the struct
> sun4i_hdmi pointer with a pointer to struct snd_soc_card.
>
> - The driver will operate properly, since we never really use the
> driver_data field, in the HDMI driver, except when...
>
> - At unbind, you retrieve the driver_data field, expecting a struct
> sun4i_hdmi pointer, except you have a pointer to struct
> snd_soc_card, and everything explodes.
>
> I think the way to work around that would be to create a new
> (platform_)device for the HDMI audio component, so that ASoC can work
> on that device instead.
>
> This seems to be what dw-hdmi is doing here:
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2812
>
> (Except that they are also using platform_data, since they have
> multiple drivers, we wouldn't, so we can just lookup sun4i_hdmi using
> the parent's device driver_data).
>
>>>> +}
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> index a7c4654445c7..79ecd89fb705 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>>>> @@ -114,6 +114,9 @@ static void sun4i_hdmi_enable(struct drm_encoder *encoder)
>>>> val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
>>>>
>>>> writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
>>>> +
>>>> + if (hdmi->hdmi_audio && sun4i_hdmi_audio_create(hdmi))
>>>> + DRM_ERROR("Couldn't create the HDMI audio adapter\n");
>>> So you create the audio card each time the display is enabled? I guess
>>> this is to deal with the hotplug?
>> Yes. See below.
>>
>>> I'm not sure this is the right thing to do. If I remember well, the
>>> ELD are here precisely to let userspace know that the display is
>>> plugged (and audio-capable) or not.
>>>
>>> Also, you don't remove that card in the disable, which mean that if
>>> you end up in a situation where you would enable the display, disable
>>> it and then enable it again, you have two audio cards now.
>> There is issue with the hotplug. When inserting the cable, the event
>> is detected and the hdmi encoder is enabled. Thus the card is
>> created. However further removal and insertions are not
>> detected.
> I guess we would need to fix that then?
>
>> This is why I don't remove the card.
>>
>> Also I count on devm_snd_soc_register_card() to release the card.
> I think you should really create the card all the time, and just
> update the ELD to let the userspace know when something has been
> created.
>
> And yeah, we should have a working hotplug, but that's a separate
> story :)
Thank you for the review. Soon I'll prepare v2.
Also I'll check the hotplug issue.
>
> Maxime
Best regards,
Stefan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [linux-sunxi] [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
2020-01-10 14:11 ` Stefan Mavrodiev
(?)
@ 2020-01-10 16:30 ` Chen-Yu Tsai
-1 siblings, 0 replies; 42+ messages in thread
From: Chen-Yu Tsai @ 2020-01-10 16:30 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: Dan Williams, Vinod Koul, Maxime Ripard, David Airlie,
Daniel Vetter, open list,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM,
moderated list:ARM/Allwinner sunXi SoC support,
open list:DRM DRIVERS FOR ALLWINNER A10, linux-sunxi
On Fri, Jan 10, 2020 at 10:12 PM Stefan Mavrodiev <stefan@olimex.com> wrote:
>
> Add HDMI audio support for the sun4i-hdmi encoder, used on
> the older Allwinner chips - A10, A20, A31.
>
> Most of the code is based on the BSP implementation. In it
> dditional formats are supported (S20_3LE and S24_LE), however
> there where some problems with them and only S16_LE is left.
>
> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> ---
> drivers/gpu/drm/sun4i/Kconfig | 1 +
> drivers/gpu/drm/sun4i/Makefile | 1 +
> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> 5 files changed, 411 insertions(+)
> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index 37e90e42943f..192b732b10cd 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -19,6 +19,7 @@ if DRM_SUN4I
> config DRM_SUN4I_HDMI
> tristate "Allwinner A10 HDMI Controller Support"
> default DRM_SUN4I
> + select SND_PCM_ELD
> help
> Choose this option if you have an Allwinner SoC with an HDMI
> controller.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 0d04f2447b01..e2d82b451c36 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> sun4i-drm-y += sun4i_drv.o
> sun4i-drm-y += sun4i_framebuffer.o
>
> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> index 7ad3f06c127e..456964e681b0 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -42,7 +42,32 @@
> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>
> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> +
> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG_MASK GENMASK(2, 0)
> +
> +#define SUN4I_HDMI_AUDIO_PCM_REG 0x4c
> +#define SUN4I_HDMI_AUDIO_PCM_CH_MAP(n, m) ((m - 1) << (n * 4))
> +#define SUN4I_HDMI_AUDIO_PCM_CH_MAP_MASK(n) (GENMASK(2, 0) << (n * 4))
> +
> +#define SUN4I_HDMI_AUDIO_CTS_REG 0x050
> +#define SUN4I_HDMI_AUDIO_CTS(n) (n & GENMASK(19, 0))
> +
> +#define SUN4I_HDMI_AUDIO_N_REG 0x054
> +#define SUN4I_HDMI_AUDIO_N(n) (n & GENMASK(19, 0))
> +
> +#define SUN4I_HDMI_AUDIO_STAT0_REG 0x58
> +#define SUN4I_HDMI_AUDIO_STAT0_FREQ(n) (n << 24)
> +#define SUN4I_HDMI_AUDIO_STAT0_FREQ_MASK GENMASK(27, 24)
> +
> #define SUN4I_HDMI_AVI_INFOFRAME_REG(n) (0x080 + (n))
> +#define SUN4I_HDMI_AUDIO_INFOFRAME_REG(n) (0x0a0 + (n))
>
> #define SUN4I_HDMI_PAD_CTRL0_REG 0x200
> #define SUN4I_HDMI_PAD_CTRL0_BIASEN BIT(31)
> @@ -283,9 +308,13 @@ struct sun4i_hdmi {
> struct regmap_field *field_ddc_sda_en;
> struct regmap_field *field_ddc_sck_en;
>
> + u8 hdmi_audio_channels;
> +
> struct sun4i_drv *drv;
>
> bool hdmi_monitor;
> + bool hdmi_audio;
> +
> struct cec_adapter *cec_adap;
>
> const struct sun4i_hdmi_variant *variant;
> @@ -294,5 +323,6 @@ struct sun4i_hdmi {
> int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
> int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
> int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi);
>
> #endif /* _SUN4I_HDMI_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> new file mode 100644
> index 000000000000..b6d4199d15ce
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Olimex Ltd.
> + * Author: Stefan Mavrodiev <stefan@olimex.com>
> + */
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/module.h>
> +#include <linux/of_dma.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_print.h>
> +
> +#include <sound/dmaengine_pcm.h>
> +#include <sound/pcm_drm_eld.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
I would drop the ASoC stuff and just do a standard ALSA driver.
You really don't gain anything from using ASoC, since this is
just a really standard PCM DMA interface plus some controls.
There aren't multiple components that need to be strung together.
ChenYu
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [linux-sunxi] [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-10 16:30 ` Chen-Yu Tsai
0 siblings, 0 replies; 42+ messages in thread
From: Chen-Yu Tsai @ 2020-01-10 16:30 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, open list, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Dan Williams,
moderated list:ARM/Allwinner sunXi SoC support
On Fri, Jan 10, 2020 at 10:12 PM Stefan Mavrodiev <stefan@olimex.com> wrote:
>
> Add HDMI audio support for the sun4i-hdmi encoder, used on
> the older Allwinner chips - A10, A20, A31.
>
> Most of the code is based on the BSP implementation. In it
> dditional formats are supported (S20_3LE and S24_LE), however
> there where some problems with them and only S16_LE is left.
>
> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> ---
> drivers/gpu/drm/sun4i/Kconfig | 1 +
> drivers/gpu/drm/sun4i/Makefile | 1 +
> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> 5 files changed, 411 insertions(+)
> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index 37e90e42943f..192b732b10cd 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -19,6 +19,7 @@ if DRM_SUN4I
> config DRM_SUN4I_HDMI
> tristate "Allwinner A10 HDMI Controller Support"
> default DRM_SUN4I
> + select SND_PCM_ELD
> help
> Choose this option if you have an Allwinner SoC with an HDMI
> controller.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 0d04f2447b01..e2d82b451c36 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> sun4i-drm-y += sun4i_drv.o
> sun4i-drm-y += sun4i_framebuffer.o
>
> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> index 7ad3f06c127e..456964e681b0 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -42,7 +42,32 @@
> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>
> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> +
> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG_MASK GENMASK(2, 0)
> +
> +#define SUN4I_HDMI_AUDIO_PCM_REG 0x4c
> +#define SUN4I_HDMI_AUDIO_PCM_CH_MAP(n, m) ((m - 1) << (n * 4))
> +#define SUN4I_HDMI_AUDIO_PCM_CH_MAP_MASK(n) (GENMASK(2, 0) << (n * 4))
> +
> +#define SUN4I_HDMI_AUDIO_CTS_REG 0x050
> +#define SUN4I_HDMI_AUDIO_CTS(n) (n & GENMASK(19, 0))
> +
> +#define SUN4I_HDMI_AUDIO_N_REG 0x054
> +#define SUN4I_HDMI_AUDIO_N(n) (n & GENMASK(19, 0))
> +
> +#define SUN4I_HDMI_AUDIO_STAT0_REG 0x58
> +#define SUN4I_HDMI_AUDIO_STAT0_FREQ(n) (n << 24)
> +#define SUN4I_HDMI_AUDIO_STAT0_FREQ_MASK GENMASK(27, 24)
> +
> #define SUN4I_HDMI_AVI_INFOFRAME_REG(n) (0x080 + (n))
> +#define SUN4I_HDMI_AUDIO_INFOFRAME_REG(n) (0x0a0 + (n))
>
> #define SUN4I_HDMI_PAD_CTRL0_REG 0x200
> #define SUN4I_HDMI_PAD_CTRL0_BIASEN BIT(31)
> @@ -283,9 +308,13 @@ struct sun4i_hdmi {
> struct regmap_field *field_ddc_sda_en;
> struct regmap_field *field_ddc_sck_en;
>
> + u8 hdmi_audio_channels;
> +
> struct sun4i_drv *drv;
>
> bool hdmi_monitor;
> + bool hdmi_audio;
> +
> struct cec_adapter *cec_adap;
>
> const struct sun4i_hdmi_variant *variant;
> @@ -294,5 +323,6 @@ struct sun4i_hdmi {
> int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
> int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
> int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi);
>
> #endif /* _SUN4I_HDMI_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> new file mode 100644
> index 000000000000..b6d4199d15ce
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Olimex Ltd.
> + * Author: Stefan Mavrodiev <stefan@olimex.com>
> + */
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/module.h>
> +#include <linux/of_dma.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_print.h>
> +
> +#include <sound/dmaengine_pcm.h>
> +#include <sound/pcm_drm_eld.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
I would drop the ASoC stuff and just do a standard ALSA driver.
You really don't gain anything from using ASoC, since this is
just a really standard PCM DMA interface plus some controls.
There aren't multiple components that need to be strung together.
ChenYu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [linux-sunxi] [PATCH 2/2] drm: sun4i: hdmi: Add support for sun4i HDMI encoder audio
@ 2020-01-10 16:30 ` Chen-Yu Tsai
0 siblings, 0 replies; 42+ messages in thread
From: Chen-Yu Tsai @ 2020-01-10 16:30 UTC (permalink / raw)
To: Stefan Mavrodiev
Cc: David Airlie, linux-sunxi, open list, Maxime Ripard, Vinod Koul,
open list:DRM DRIVERS FOR ALLWINNER A10, Daniel Vetter,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Dan Williams,
moderated list:ARM/Allwinner sunXi SoC support
On Fri, Jan 10, 2020 at 10:12 PM Stefan Mavrodiev <stefan@olimex.com> wrote:
>
> Add HDMI audio support for the sun4i-hdmi encoder, used on
> the older Allwinner chips - A10, A20, A31.
>
> Most of the code is based on the BSP implementation. In it
> dditional formats are supported (S20_3LE and S24_LE), however
> there where some problems with them and only S16_LE is left.
>
> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> ---
> drivers/gpu/drm/sun4i/Kconfig | 1 +
> drivers/gpu/drm/sun4i/Makefile | 1 +
> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 30 ++
> drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c | 375 +++++++++++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 4 +
> 5 files changed, 411 insertions(+)
> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
>
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index 37e90e42943f..192b732b10cd 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -19,6 +19,7 @@ if DRM_SUN4I
> config DRM_SUN4I_HDMI
> tristate "Allwinner A10 HDMI Controller Support"
> default DRM_SUN4I
> + select SND_PCM_ELD
> help
> Choose this option if you have an Allwinner SoC with an HDMI
> controller.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 0d04f2447b01..e2d82b451c36 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -5,6 +5,7 @@ sun4i-frontend-y += sun4i_frontend.o
> sun4i-drm-y += sun4i_drv.o
> sun4i-drm-y += sun4i_framebuffer.o
>
> +sun4i-drm-hdmi-y += sun4i_hdmi_audio.o
> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> index 7ad3f06c127e..456964e681b0 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -42,7 +42,32 @@
> #define SUN4I_HDMI_VID_TIMING_POL_VSYNC BIT(1)
> #define SUN4I_HDMI_VID_TIMING_POL_HSYNC BIT(0)
>
> +#define SUN4I_HDMI_AUDIO_CTRL_REG 0x040
> +#define SUN4I_HDMI_AUDIO_CTRL_ENABLE BIT(31)
> +#define SUN4I_HDMI_AUDIO_CTRL_RESET BIT(30)
> +
> +#define SUN4I_HDMI_AUDIO_FMT_REG 0x048
> +#define SUN4I_HDMI_AUDIO_FMT_SRC BIT(31)
> +#define SUN4I_HDMI_AUDIO_FMT_LAYOUT BIT(3)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG(n) (n - 1)
> +#define SUN4I_HDMI_AUDIO_FMT_CH_CFG_MASK GENMASK(2, 0)
> +
> +#define SUN4I_HDMI_AUDIO_PCM_REG 0x4c
> +#define SUN4I_HDMI_AUDIO_PCM_CH_MAP(n, m) ((m - 1) << (n * 4))
> +#define SUN4I_HDMI_AUDIO_PCM_CH_MAP_MASK(n) (GENMASK(2, 0) << (n * 4))
> +
> +#define SUN4I_HDMI_AUDIO_CTS_REG 0x050
> +#define SUN4I_HDMI_AUDIO_CTS(n) (n & GENMASK(19, 0))
> +
> +#define SUN4I_HDMI_AUDIO_N_REG 0x054
> +#define SUN4I_HDMI_AUDIO_N(n) (n & GENMASK(19, 0))
> +
> +#define SUN4I_HDMI_AUDIO_STAT0_REG 0x58
> +#define SUN4I_HDMI_AUDIO_STAT0_FREQ(n) (n << 24)
> +#define SUN4I_HDMI_AUDIO_STAT0_FREQ_MASK GENMASK(27, 24)
> +
> #define SUN4I_HDMI_AVI_INFOFRAME_REG(n) (0x080 + (n))
> +#define SUN4I_HDMI_AUDIO_INFOFRAME_REG(n) (0x0a0 + (n))
>
> #define SUN4I_HDMI_PAD_CTRL0_REG 0x200
> #define SUN4I_HDMI_PAD_CTRL0_BIASEN BIT(31)
> @@ -283,9 +308,13 @@ struct sun4i_hdmi {
> struct regmap_field *field_ddc_sda_en;
> struct regmap_field *field_ddc_sck_en;
>
> + u8 hdmi_audio_channels;
> +
> struct sun4i_drv *drv;
>
> bool hdmi_monitor;
> + bool hdmi_audio;
> +
> struct cec_adapter *cec_adap;
>
> const struct sun4i_hdmi_variant *variant;
> @@ -294,5 +323,6 @@ struct sun4i_hdmi {
> int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
> int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
> int sun4i_hdmi_i2c_create(struct device *dev, struct sun4i_hdmi *hdmi);
> +int sun4i_hdmi_audio_create(struct sun4i_hdmi *hdmi);
>
> #endif /* _SUN4I_HDMI_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> new file mode 100644
> index 000000000000..b6d4199d15ce
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_audio.c
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Olimex Ltd.
> + * Author: Stefan Mavrodiev <stefan@olimex.com>
> + */
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/module.h>
> +#include <linux/of_dma.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_print.h>
> +
> +#include <sound/dmaengine_pcm.h>
> +#include <sound/pcm_drm_eld.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
I would drop the ASoC stuff and just do a standard ALSA driver.
You really don't gain anything from using ASoC, since this is
just a really standard PCM DMA interface plus some controls.
There aren't multiple components that need to be strung together.
ChenYu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread