Re: [PATCH 6/6] OMAPDSS: HDMI: Create platform device to support audio

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

On 2012-10-16 04:27, 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 accesory
> 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 |   68 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index e5be0a5..c62c5ab 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;
>  
> @@ -73,6 +76,13 @@ static struct {
>  	struct omap_dss_output output;
>  } hdmi;
>  
> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
> +#define HDMI_AUDIO_MEM_RESOURCE 0
> +#define HDMI_AUDIO_DMA_RESOURCE 1

I don't see much point with these definitions. They are hdmi.c internal,
so the audio driver can't use them, and so they aren't really fixed.

> +static struct resource hdmi_aud_res[2];

Did you check if the platform_device_register does a copy of these? If
it does, this can be local to the probe function.

> +#endif
> +
> +
>  /*
>   * Logic for the below structure :
>   * user enters the CEA or VESA timings by specifying the HDMI/DVI code.
> @@ -765,6 +775,50 @@ 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;
> +
> +	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 this resource to audio_pdev.
> +	 * Audio drivers should not remap it
> +	 */
> +	hdmi_aud_res[HDMI_AUDIO_MEM_RESOURCE].start = res->start;
> +	hdmi_aud_res[HDMI_AUDIO_MEM_RESOURCE].end = res->end;
> +	hdmi_aud_res[HDMI_AUDIO_MEM_RESOURCE].flags = IORESOURCE_MEM;
> +
> +	res = platform_get_resource(hdmi.pdev, IORESOURCE_DMA, 0);
> +	if (!res) {
> +		DSSERR("can't get IORESOURCE_DMA HDMI\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Pass this resource to audio_pdev */
> +	hdmi_aud_res[HDMI_AUDIO_DMA_RESOURCE].start = res->start;
> +	hdmi_aud_res[HDMI_AUDIO_DMA_RESOURCE].end = res->end;
> +	hdmi_aud_res[HDMI_AUDIO_DMA_RESOURCE].flags = IORESOURCE_DMA;
> +
> +	/* create platform device for HDMI audio driver */
> +	hdmi.audio_pdev = platform_device_register_simple(
> +							  "omap_hdmi_audio",
> +							  -1, hdmi_aud_res,
> +							   ARRAY_SIZE(hdmi_aud_res));
> +	if (IS_ERR(hdmi.audio_pdev)) {
> +		DSSERR("Can't instantiate hdmi-audio\n");
> +		return PTR_ERR(hdmi.audio_pdev);
> +	}
> +
> +	return 0;
> +}

So, how will this work? All the audio related functions will be removed
from the (video) hdmi driver, and the audio driver will access the
registers independently? The audio driver will still need to access the
video parts, right?

I feel a bit uneasy about giving the same ioremapped register space to
two independent drivers... If we could split the registers to video and
audio parts, each driver only ioremapping their respective registers,
it'd be much better.

 Tomi



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