All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/7] Create platform device for audio support
@ 2012-11-03  0:31 Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 1/7] OMAPDSS: HDMI: Rename resource variable at probe Ricardo Neri
                   ` (6 more replies)
  0 siblings, 7 replies; 10+ messages in thread
From: Ricardo Neri @ 2012-11-03  0:31 UTC (permalink / raw)
  To: tomi.valkeinen
  Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap, Ricardo Neri

Hi Tomi, l-o list,

The main purpose of this patch set is to create a platform device for audio
support from the OMAPDSS HDMI driver. This tries to follow an approach similar
to MFD drivers in which a core driver creates domain-specific devices. Under
this approach, the OMAPDSS HDMI drivers acts as the core driver, retrieves its
resources (from DT or hwmod) and passes the relevant audio resources for
other drivers to use.

This is also beneficial for future DT boot support, as the OMAP HDMI IP will be
represented by a single node in the tree.

Before creating the platform device for audio, I also did minor cleanup to the
OMAPDSS HDMI driver probe function.

Changes from v1:
*Simplify ioremap further by using devm_request_and_ioremap.
*Pass to the audio driver only the DMA port and not the whole register space.
*Use a local array for the audio resources and init them with DEFINE_RES_MEM.
*Obtain the DMA port address offset and size from the HDMI IP-specific library.

v1 is accessible here:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg77861.html

BR,

Ricardo

Ricardo Neri (7):
  OMAPDSS: HDMI: Rename resource variable at probe.
  OMAPDSS: HDMI: Convert to devm_request_and_ioremap
  OMAPDSS: HDMI: Make panel return dssdev register errors
  OMAPDSS: HDMI: Handle panel init error at probe
  OMAPDSS: HDMI: Uninit display on device add error
  OMAPDSS: HDMI: Add op to get audio DMA port address offset
  OMAPDSS: HDMI: Create platform device for audio support

 drivers/video/omap2/dss/dss_features.c    |    1 +
 drivers/video/omap2/dss/hdmi.c            |   90 +++++++++++++++++++++++++----
 drivers/video/omap2/dss/hdmi_panel.c      |    4 +-
 drivers/video/omap2/dss/ti_hdmi.h         |    3 +
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |    9 +++
 5 files changed, 93 insertions(+), 14 deletions(-)

-- 
1.7.5.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/7] OMAPDSS: HDMI: Rename resource variable at probe.
  2012-11-03  0:31 [PATCH v2 0/7] Create platform device for audio support Ricardo Neri
@ 2012-11-03  0:31 ` Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 2/7] OMAPDSS: HDMI: Convert to devm_request_and_ioremap Ricardo Neri
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Ricardo Neri @ 2012-11-03  0:31 UTC (permalink / raw)
  To: tomi.valkeinen
  Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap, Ricardo Neri

Minor cleanup to give to the resource variable a more proper name.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index adcc906..cdb043d 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -998,22 +998,21 @@ static void __exit hdmi_uninit_output(struct platform_device *pdev)
 /* HDMI HW IP initialisation */
 static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
 {
-	struct resource *hdmi_mem;
+	struct resource *res;
 	int r;
 
 	hdmi.pdev = pdev;
 
 	mutex_init(&hdmi.lock);
 
-	hdmi_mem = platform_get_resource(hdmi.pdev, IORESOURCE_MEM, 0);
-	if (!hdmi_mem) {
+	res = platform_get_resource(hdmi.pdev, IORESOURCE_MEM, 0);
+	if (!res) {
 		DSSERR("can't get IORESOURCE_MEM HDMI\n");
 		return -EINVAL;
 	}
 
 	/* Base address taken from platform */
-	hdmi.ip_data.base_wp = ioremap(hdmi_mem->start,
-						resource_size(hdmi_mem));
+	hdmi.ip_data.base_wp = ioremap(res->start, resource_size(res));
 	if (!hdmi.ip_data.base_wp) {
 		DSSERR("can't ioremap WP\n");
 		return -ENOMEM;
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/7] OMAPDSS: HDMI: Convert to devm_request_and_ioremap
  2012-11-03  0:31 [PATCH v2 0/7] Create platform device for audio support Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 1/7] OMAPDSS: HDMI: Rename resource variable at probe Ricardo Neri
@ 2012-11-03  0:31 ` Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 3/7] OMAPDSS: HDMI: Make panel return dssdev register errors Ricardo Neri
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Ricardo Neri @ 2012-11-03  0:31 UTC (permalink / raw)
  To: tomi.valkeinen
  Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap, Ricardo Neri

Using devm_request_and_ioremap provides better memory handling and
improves readability.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index cdb043d..51ee0a6 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -1012,7 +1012,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
 	}
 
 	/* Base address taken from platform */
-	hdmi.ip_data.base_wp = ioremap(res->start, resource_size(res));
+	hdmi.ip_data.base_wp = devm_request_and_ioremap(&pdev->dev, res);
 	if (!hdmi.ip_data.base_wp) {
 		DSSERR("can't ioremap WP\n");
 		return -ENOMEM;
@@ -1020,7 +1020,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
 
 	r = hdmi_get_clocks(pdev);
 	if (r) {
-		iounmap(hdmi.ip_data.base_wp);
+		DSSERR("can't get clocks\n");
 		return r;
 	}
 
@@ -1065,8 +1065,6 @@ static int __exit omapdss_hdmihw_remove(struct platform_device *pdev)
 
 	hdmi_put_clocks();
 
-	iounmap(hdmi.ip_data.base_wp);
-
 	return 0;
 }
 
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/7] OMAPDSS: HDMI: Make panel return dssdev register errors
  2012-11-03  0:31 [PATCH v2 0/7] Create platform device for audio support Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 1/7] OMAPDSS: HDMI: Rename resource variable at probe Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 2/7] OMAPDSS: HDMI: Convert to devm_request_and_ioremap Ricardo Neri
@ 2012-11-03  0:31 ` Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 4/7] OMAPDSS: HDMI: Handle panel init error at probe Ricardo Neri
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Ricardo Neri @ 2012-11-03  0:31 UTC (permalink / raw)
  To: tomi.valkeinen
  Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap, Ricardo Neri

Do not assume blindly that the DSS driver was registered successfully.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi_panel.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi_panel.c b/drivers/video/omap2/dss/hdmi_panel.c
index 69fb115..129107b 100644
--- a/drivers/video/omap2/dss/hdmi_panel.c
+++ b/drivers/video/omap2/dss/hdmi_panel.c
@@ -454,9 +454,7 @@ int hdmi_panel_init(void)
 	spin_lock_init(&hdmi.audio_lock);
 #endif
 
-	omap_dss_register_driver(&hdmi_driver);
-
-	return 0;
+	return omap_dss_register_driver(&hdmi_driver);
 }
 
 void hdmi_panel_exit(void)
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/7] OMAPDSS: HDMI: Handle panel init error at probe
  2012-11-03  0:31 [PATCH v2 0/7] Create platform device for audio support Ricardo Neri
                   ` (2 preceding siblings ...)
  2012-11-03  0:31 ` [PATCH v2 3/7] OMAPDSS: HDMI: Make panel return dssdev register errors Ricardo Neri
@ 2012-11-03  0:31 ` Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 5/7] OMAPDSS: HDMI: Uninit display on device add error Ricardo Neri
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Ricardo Neri @ 2012-11-03  0:31 UTC (permalink / raw)
  To: tomi.valkeinen
  Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap, Ricardo Neri

Do not blindly assume that the panel could be initialized.

While there, group mutex initialization at a single place.
Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 51ee0a6..696386c 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -60,6 +60,7 @@
 static struct {
 	struct mutex lock;
 	struct platform_device *pdev;
+
 	struct hdmi_ip_data ip_data;
 
 	struct clk *sys_clk;
@@ -1004,6 +1005,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
 	hdmi.pdev = pdev;
 
 	mutex_init(&hdmi.lock);
+	mutex_init(&hdmi.ip_data.lock);
 
 	res = platform_get_resource(hdmi.pdev, IORESOURCE_MEM, 0);
 	if (!res) {
@@ -1031,9 +1033,11 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
 	hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
 	hdmi.ip_data.phy_offset = HDMI_PHY;
 
-	mutex_init(&hdmi.ip_data.lock);
-
-	hdmi_panel_init();
+	r = hdmi_panel_init();
+	if (r) {
+		DSSERR("can't init panel\n");
+		goto err_panel_init;
+	}
 
 	dss_debugfs_create_file("hdmi", hdmi_dump_regs);
 
@@ -1042,6 +1046,10 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
 	hdmi_probe_pdata(pdev);
 
 	return 0;
+
+err_panel_init:
+	hdmi_put_clocks();
+	return r;
 }
 
 static int __exit hdmi_remove_child(struct device *dev, void *data)
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 5/7] OMAPDSS: HDMI: Uninit display on device add error
  2012-11-03  0:31 [PATCH v2 0/7] Create platform device for audio support Ricardo Neri
                   ` (3 preceding siblings ...)
  2012-11-03  0:31 ` [PATCH v2 4/7] OMAPDSS: HDMI: Handle panel init error at probe Ricardo Neri
@ 2012-11-03  0:31 ` Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 6/7] OMAPDSS: HDMI: Add op to get audio DMA port address offset Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 7/7] OMAPDSS: HDMI: Create platform device for audio support Ricardo Neri
  6 siblings, 0 replies; 10+ messages in thread
From: Ricardo Neri @ 2012-11-03  0:31 UTC (permalink / raw)
  To: tomi.valkeinen
  Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap, Ricardo Neri

The display must be uninitialized in order to free the requested GPIOs.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 696386c..4adf830 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -973,6 +973,7 @@ static void __init hdmi_probe_pdata(struct platform_device *pdev)
 	r = dss_add_device(dssdev);
 	if (r) {
 		DSSERR("device %s register failed: %d\n", dssdev->name, r);
+		hdmi_uninit_display(dssdev);
 		dss_put_device(dssdev);
 		return;
 	}
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 6/7] OMAPDSS: HDMI: Add op to get audio DMA port address offset
  2012-11-03  0:31 [PATCH v2 0/7] Create platform device for audio support Ricardo Neri
                   ` (4 preceding siblings ...)
  2012-11-03  0:31 ` [PATCH v2 5/7] OMAPDSS: HDMI: Uninit display on device add error Ricardo Neri
@ 2012-11-03  0:31 ` Ricardo Neri
  2012-11-03  0:31 ` [PATCH v2 7/7] OMAPDSS: HDMI: Create platform device for audio support Ricardo Neri
  6 siblings, 0 replies; 10+ messages in thread
From: Ricardo Neri @ 2012-11-03  0:31 UTC (permalink / raw)
  To: tomi.valkeinen
  Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap, Ricardo Neri

It could be possible that the DMA port differs accross diferent HDMI IPs. Thus,
add an IP-specific function to obtain the address offset and size of the DMA
data port.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/dss_features.c    |    1 +
 drivers/video/omap2/dss/ti_hdmi.h         |    3 +++
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |    9 +++++++++
 3 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index 3e8287c..8dcecbc 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -820,6 +820,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
 	.audio_start		=       ti_hdmi_4xxx_audio_start,
 	.audio_stop		=       ti_hdmi_4xxx_audio_stop,
 	.audio_config		=	ti_hdmi_4xxx_audio_config,
+	.audio_get_dma_port	=	ti_hdmi_4xxx_audio_get_dma_port,
 #endif
 
 };
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index b046c20..216aa70 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -102,6 +102,8 @@ struct ti_hdmi_ip_ops {
 
 	int (*audio_config)(struct hdmi_ip_data *ip_data,
 		struct omap_dss_audio *audio);
+
+	int (*audio_get_dma_port)(u32 *offset, u32 *size);
 #endif
 
 };
@@ -183,5 +185,6 @@ int ti_hdmi_4xxx_audio_start(struct hdmi_ip_data *ip_data);
 void ti_hdmi_4xxx_audio_stop(struct hdmi_ip_data *ip_data);
 int ti_hdmi_4xxx_audio_config(struct hdmi_ip_data *ip_data,
 		struct omap_dss_audio *audio);
+int ti_hdmi_4xxx_audio_get_dma_port(u32 *offset, u32 *size);
 #endif
 #endif
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index a6efff2..e18b222 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -1418,4 +1418,13 @@ void ti_hdmi_4xxx_audio_stop(struct hdmi_ip_data *ip_data)
 	REG_FLD_MOD(hdmi_wp_base(ip_data),
 		    HDMI_WP_AUDIO_CTRL, false, 30, 30);
 }
+
+int ti_hdmi_4xxx_audio_get_dma_port(u32 *offset, u32 *size)
+{
+	if (!offset || !size)
+		return -EINVAL;
+	*offset = HDMI_WP_AUDIO_DATA;
+	*size = 4;
+	return 0;
+}
 #endif
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 7/7] OMAPDSS: HDMI: Create platform device for audio support
  2012-11-03  0:31 [PATCH v2 0/7] Create platform device for audio support Ricardo Neri
                   ` (5 preceding siblings ...)
  2012-11-03  0:31 ` [PATCH v2 6/7] OMAPDSS: HDMI: Add op to get audio DMA port address offset Ricardo Neri
@ 2012-11-03  0:31 ` Ricardo Neri
  2012-11-05  8:46   ` Tomi Valkeinen
  6 siblings, 1 reply; 10+ messages in thread
From: Ricardo Neri @ 2012-11-03  0:31 UTC (permalink / raw)
  To: tomi.valkeinen
  Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap, Ricardo Neri

Creating the accessory devices, such as audio, from the HDMI driver
allows to regard HDMI as a single entity with audio an display
functionality. This intends to follow the design of drivers such
as MFD, in which a single entity handles the creation of the accessory
devices. Such devices are then used by domain-specific drivers; audio in
this case.

Also, this is in line with the DT implementation of HDMI, in which we will
have a single node to describe this feature of the OMAP SoC.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi.c |   62 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 4adf830..d6ce4c6 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -60,6 +60,9 @@
 static struct {
 	struct mutex lock;
 	struct platform_device *pdev;
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+	struct platform_device *audio_pdev;
+#endif
 
 	struct hdmi_ip_data ip_data;
 
@@ -766,6 +769,54 @@ static void hdmi_put_clocks(void)
 }
 
 #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+static int hdmi_probe_audio(struct platform_device *pdev)
+{
+	struct resource *res;
+	u32 port_offset, port_size;
+	struct resource aud_res[2] = {
+		DEFINE_RES_MEM(-1, -1),
+		DEFINE_RES_DMA(-1),
+	};
+
+	hdmi.audio_pdev = ERR_PTR(-EINVAL);
+
+	res = platform_get_resource(hdmi.pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		DSSERR("can't get IORESOURCE_MEM HDMI\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Pass DMA audio port to audio drivers.
+	 * Audio drivers should not ioremap it.
+	 */
+	hdmi.ip_data.ops->audio_get_dma_port(&port_offset, &port_size);
+
+	aud_res[0].start = res->start + port_offset;
+	aud_res[0].end = aud_res[0].start + port_size - 1;
+
+	res = platform_get_resource(hdmi.pdev, IORESOURCE_DMA, 0);
+	if (!res) {
+		DSSERR("can't get IORESOURCE_DMA HDMI\n");
+		return -EINVAL;
+	}
+
+	/* Pass the audio DMA request resource to audio drivers. */
+	aud_res[1].start = res->start;
+
+	/* create platform device for HDMI audio driver */
+	hdmi.audio_pdev = platform_device_register_simple(
+							  "omap_hdmi_audio",
+							  -1, aud_res,
+							   ARRAY_SIZE(aud_res));
+	if (IS_ERR(hdmi.audio_pdev)) {
+		DSSERR("Can't instantiate hdmi-audio\n");
+		return PTR_ERR(hdmi.audio_pdev);
+	}
+
+	return 0;
+}
+
 int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
 {
 	u32 deep_color;
@@ -1046,6 +1097,12 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
 
 	hdmi_probe_pdata(pdev);
 
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+	r = hdmi_probe_audio(pdev);
+	if (r)
+		DSSWARN("could not create platform device for audio");
+#endif
+
 	return 0;
 
 err_panel_init:
@@ -1062,6 +1119,11 @@ static int __exit hdmi_remove_child(struct device *dev, void *data)
 
 static int __exit omapdss_hdmihw_remove(struct platform_device *pdev)
 {
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+	if (!IS_ERR(hdmi.audio_pdev))
+		platform_device_unregister(hdmi.audio_pdev);
+#endif
+
 	device_for_each_child(&pdev->dev, NULL, hdmi_remove_child);
 
 	dss_unregister_child_devices(&pdev->dev);
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 7/7] OMAPDSS: HDMI: Create platform device for audio support
  2012-11-03  0:31 ` [PATCH v2 7/7] OMAPDSS: HDMI: Create platform device for audio support Ricardo Neri
@ 2012-11-05  8:46   ` Tomi Valkeinen
  2012-11-06  5:27     ` Ricardo Neri
  0 siblings, 1 reply; 10+ messages in thread
From: Tomi Valkeinen @ 2012-11-05  8:46 UTC (permalink / raw)
  To: Ricardo Neri; +Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap

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

On 2012-11-03 02:31, Ricardo Neri wrote:
> Creating the accessory devices, such as audio, from the HDMI driver
> allows to regard HDMI as a single entity with audio an display
> functionality. This intends to follow the design of drivers such
> as MFD, in which a single entity handles the creation of the accessory
> devices. Such devices are then used by domain-specific drivers; audio in
> this case.
> 
> Also, this is in line with the DT implementation of HDMI, in which we will
> have a single node to describe this feature of the OMAP SoC.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
> ---
>  drivers/video/omap2/dss/hdmi.c |   62 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 62 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 4adf830..d6ce4c6 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -60,6 +60,9 @@
>  static struct {
>  	struct mutex lock;
>  	struct platform_device *pdev;
> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
> +	struct platform_device *audio_pdev;
> +#endif
>  
>  	struct hdmi_ip_data ip_data;
>  
> @@ -766,6 +769,54 @@ static void hdmi_put_clocks(void)
>  }
>  
>  #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
> +static int hdmi_probe_audio(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	u32 port_offset, port_size;
> +	struct resource aud_res[2] = {
> +		DEFINE_RES_MEM(-1, -1),
> +		DEFINE_RES_DMA(-1),
> +	};
> +
> +	hdmi.audio_pdev = ERR_PTR(-EINVAL);

I don't like this. I think ERR_PTR stuff should be used only for return
values, not when storing pointers. I think it's much more natural and
less error prone to do if (hdmi.audio_pdev == NULL) than if
(IS_ERR(hdmi.audio_pdev)).

So store the return value from platform_dev_register first to a local
variable, check IS_ERR for that, and only then store it to hdmi.audio_pdev.

> +	res = platform_get_resource(hdmi.pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		DSSERR("can't get IORESOURCE_MEM HDMI\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Pass DMA audio port to audio drivers.
> +	 * Audio drivers should not ioremap it.
> +	 */
> +	hdmi.ip_data.ops->audio_get_dma_port(&port_offset, &port_size);
> +
> +	aud_res[0].start = res->start + port_offset;
> +	aud_res[0].end = aud_res[0].start + port_size - 1;
> +
> +	res = platform_get_resource(hdmi.pdev, IORESOURCE_DMA, 0);
> +	if (!res) {
> +		DSSERR("can't get IORESOURCE_DMA HDMI\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Pass the audio DMA request resource to audio drivers. */
> +	aud_res[1].start = res->start;
> +
> +	/* create platform device for HDMI audio driver */
> +	hdmi.audio_pdev = platform_device_register_simple(
> +							  "omap_hdmi_audio",
> +							  -1, aud_res,
> +							   ARRAY_SIZE(aud_res));

Not a problem for the time being, but the above prevents us from having
two HDMI outputs. Perhaps you could use hdmi pdev's ID instead of -1 above?

Otherwise I think the series is ok.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 7/7] OMAPDSS: HDMI: Create platform device for audio support
  2012-11-05  8:46   ` Tomi Valkeinen
@ 2012-11-06  5:27     ` Ricardo Neri
  0 siblings, 0 replies; 10+ messages in thread
From: Ricardo Neri @ 2012-11-06  5:27 UTC (permalink / raw)
  To: Tomi Valkeinen; +Cc: peter.ujfalusi, s-guiriec, b-cousson, linux-omap

Hi Tomi,

Thanks for reviewing.

On 11/05/2012 02:46 AM, Tomi Valkeinen wrote:
> On 2012-11-03 02:31, Ricardo Neri wrote:
>> Creating the accessory devices, such as audio, from the HDMI driver
>> allows to regard HDMI as a single entity with audio an display
>> functionality. This intends to follow the design of drivers such
>> as MFD, in which a single entity handles the creation of the accessory
>> devices. Such devices are then used by domain-specific drivers; audio in
>> this case.
>>
>> Also, this is in line with the DT implementation of HDMI, in which we will
>> have a single node to describe this feature of the OMAP SoC.
>>
>> Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
>> ---
>>   drivers/video/omap2/dss/hdmi.c |   62 ++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 62 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
>> index 4adf830..d6ce4c6 100644
>> --- a/drivers/video/omap2/dss/hdmi.c
>> +++ b/drivers/video/omap2/dss/hdmi.c
>> @@ -60,6 +60,9 @@
>>   static struct {
>>   	struct mutex lock;
>>   	struct platform_device *pdev;
>> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>> +	struct platform_device *audio_pdev;
>> +#endif
>>
>>   	struct hdmi_ip_data ip_data;
>>
>> @@ -766,6 +769,54 @@ static void hdmi_put_clocks(void)
>>   }
>>
>>   #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>> +static int hdmi_probe_audio(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	u32 port_offset, port_size;
>> +	struct resource aud_res[2] = {
>> +		DEFINE_RES_MEM(-1, -1),
>> +		DEFINE_RES_DMA(-1),
>> +	};
>> +
>> +	hdmi.audio_pdev = ERR_PTR(-EINVAL);
>
> I don't like this. I think ERR_PTR stuff should be used only for return
> values, not when storing pointers. I think it's much more natural and
> less error prone to do if (hdmi.audio_pdev == NULL) than if
> (IS_ERR(hdmi.audio_pdev)).
>
> So store the return value from platform_dev_register first to a local
> variable, check IS_ERR for that, and only then store it to hdmi.audio_pdev.

OK. I'll implement the change.
>
>> +	res = platform_get_resource(hdmi.pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		DSSERR("can't get IORESOURCE_MEM HDMI\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Pass DMA audio port to audio drivers.
>> +	 * Audio drivers should not ioremap it.
>> +	 */
>> +	hdmi.ip_data.ops->audio_get_dma_port(&port_offset, &port_size);
>> +
>> +	aud_res[0].start = res->start + port_offset;
>> +	aud_res[0].end = aud_res[0].start + port_size - 1;
>> +
>> +	res = platform_get_resource(hdmi.pdev, IORESOURCE_DMA, 0);
>> +	if (!res) {
>> +		DSSERR("can't get IORESOURCE_DMA HDMI\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Pass the audio DMA request resource to audio drivers. */
>> +	aud_res[1].start = res->start;
>> +
>> +	/* create platform device for HDMI audio driver */
>> +	hdmi.audio_pdev = platform_device_register_simple(
>> +							  "omap_hdmi_audio",
>> +							  -1, aud_res,
>> +							   ARRAY_SIZE(aud_res));
>
> Not a problem for the time being, but the above prevents us from having
> two HDMI outputs. Perhaps you could use hdmi pdev's ID instead of -1 above?

I guess it is fine as you suggest. I will have to work on the ASoC side 
to support more than one HDMI outputs as well.

BR,

Ricardo
>
> Otherwise I think the series is ok.
>
>   Tomi
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-11-06  4:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-03  0:31 [PATCH v2 0/7] Create platform device for audio support Ricardo Neri
2012-11-03  0:31 ` [PATCH v2 1/7] OMAPDSS: HDMI: Rename resource variable at probe Ricardo Neri
2012-11-03  0:31 ` [PATCH v2 2/7] OMAPDSS: HDMI: Convert to devm_request_and_ioremap Ricardo Neri
2012-11-03  0:31 ` [PATCH v2 3/7] OMAPDSS: HDMI: Make panel return dssdev register errors Ricardo Neri
2012-11-03  0:31 ` [PATCH v2 4/7] OMAPDSS: HDMI: Handle panel init error at probe Ricardo Neri
2012-11-03  0:31 ` [PATCH v2 5/7] OMAPDSS: HDMI: Uninit display on device add error Ricardo Neri
2012-11-03  0:31 ` [PATCH v2 6/7] OMAPDSS: HDMI: Add op to get audio DMA port address offset Ricardo Neri
2012-11-03  0:31 ` [PATCH v2 7/7] OMAPDSS: HDMI: Create platform device for audio support Ricardo Neri
2012-11-05  8:46   ` Tomi Valkeinen
2012-11-06  5:27     ` Ricardo Neri

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.