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

[Ricardo Neri <ricardo.neri@ti.com>]

Hi Tomi,

Thanks for reviewing!

On 10/22/2012 02:40 AM, Tomi Valkeinen wrote:
> 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.

I just thought it could make the code more readable; but if the 
resources array is going to be local, then they are not helpful.
>
>> +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.

Thanks! I checked, platform_device_register does the copy. I will put it 
as a local variable.
>
>> +#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?
That could be a new approach, but the idea here is to continue having an 
omapdss audio interface for audio drivers to use.

The root problem that I am trying to address is that the omapdss audio 
interface does not have functionality for DMA transfer of audio samples 
to the HDMI IP. Also, I am not sure how that could be done without 
duplicating the functionality that ASoC already provides.
>
> 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.

Fwiw, the audio drivers (at least my audio drivers) will not ioremap. 
They will just take the DMA request number and port. Maybe spliting the 
register space into audio and video is not practical as we would endup 
having many tiny address spaces.

BR,

Ricardo
>
>   Tomi
>
>