[alsa-devel] [PATCH v3 2/4] OMAP4: hwmod: add entries for DMIC driver

[Cousson, Benoit]

Hi David,

I have few comments about the modification you did to the original code.

On 1/25/2011 11:01 PM, Lambert, David wrote:
> From: Benoit Cousson<b-cousson at ti.com>
> 
> Adds HWMOD entries for the OMAP DMIC driver and creates
> a platform device.  The HWMOD entires define the system

The changelog does not really reflect what the patch is doing.
You are not creating a platform device in that patch.

> resource requirements for the drvier such as DMA addresses, channels,
> and IRQ's.  Placing this information in the HWMOD database allows
> for more generic drivers to be written and having the specific
> implementation details defined in HWMOD.
> 

We already discussed that, but my S-O-B is missing.

> Signed-off-by: David Lambert<dlambert at ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   91 ++++++++++++++++++++++++++++
>   1 files changed, 91 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index 7274db4..f9b2ad3 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -383,6 +383,95 @@ static struct omap_hwmod omap44xx_l4_wkup_hwmod = {
>   };
> 
>   /*
> + * 'dmic' class
> + * digital microphone controller
> + */
> +
> +static struct omap_hwmod_class_sysconfig omap44xx_dmic_sysc = {
> +	.rev_offs	= 0x0000,
> +	.sysc_offs	= 0x0010,
> +	.sysc_flags	= (SYSC_HAS_EMUFREE | SYSC_HAS_RESET_STATUS |
> +			   SYSC_HAS_SIDLEMODE | SYSC_HAS_SOFTRESET),
> +	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),

SIDLE_SMART_WKUP flag is missing.

> +	.sysc_fields	=&omap_hwmod_sysc_type2,
> +};
> +
> +static struct omap_hwmod_class omap44xx_dmic_hwmod_class = {
> +	.name = "omap-dmic",

This is not the right name. Please stick to the original one from the HW spec. You can rename it the way you want in the device only.

> +	.sysc =&omap44xx_dmic_sysc,
> +};
> +
> +/* dmic */
> +static struct omap_hwmod omap44xx_dmic_hwmod;
> +static struct omap_hwmod_irq_info omap44xx_dmic_irqs[] = {
> +	{ .irq = 114 + OMAP44XX_IRQ_GIC_START },
> +};
> +
> +static struct omap_hwmod_dma_info omap44xx_dmic_sdma_reqs[] = {
> +	{ .dma_req = 66 + OMAP44XX_DMA_REQ_START },
> +};
> +
> +static struct omap_hwmod_addr_space omap44xx_dmic_addrs[] = {
> +	{

it might not be useful for your driver, but we should use a name to differentiate the dual memory mapping here. It was introduce by Kishon in his McBSP series.

> +		.pa_start	= 0x4012e000,
> +		.pa_end		= 0x4012e07f,
> +		.flags		= ADDR_TYPE_RT
> +	},
> +};
> +
> +/* l4_abe ->  dmic */
> +static struct omap_hwmod_ocp_if omap44xx_l4_abe__dmic = {
> +	.master		=&omap44xx_l4_abe_hwmod,
> +	.slave		=&omap44xx_dmic_hwmod,
> +	.clk		= "ocp_abe_iclk",
> +	.addr		= omap44xx_dmic_addrs,
> +	.addr_cnt	= ARRAY_SIZE(omap44xx_dmic_addrs),
> +	.user		= OCP_USER_MPU,
> +};
> +
> +static struct omap_hwmod_addr_space omap44xx_dmic_dma_addrs[] = {
> +	{
> +		.pa_start	= 0x4902e000,
> +		.pa_end		= 0x4902e07f,
> +		.flags		= ADDR_TYPE_RT
> +	},
> +};
> +
> +/* l4_abe ->  dmic (dma) */
> +static struct omap_hwmod_ocp_if omap44xx_l4_abe__dmic_dma = {
> +	.master		=&omap44xx_l4_abe_hwmod,
> +	.slave		=&omap44xx_dmic_hwmod,
> +	.clk		= "ocp_abe_iclk",
> +	.addr		= omap44xx_dmic_dma_addrs,
> +	.addr_cnt	= ARRAY_SIZE(omap44xx_dmic_dma_addrs),
> +	.user		= OCP_USER_SDMA,
> +};
> +
> +/* dmic slave ports */
> +static struct omap_hwmod_ocp_if *omap44xx_dmic_slaves[] = {
> +	&omap44xx_l4_abe__dmic,
> +	&omap44xx_l4_abe__dmic_dma,
> +};
> +
> +static struct omap_hwmod omap44xx_dmic_hwmod = {
> +	.name		= "omap-dmic",
> +	.class		=&omap44xx_dmic_hwmod_class,
> +	.mpu_irqs	= omap44xx_dmic_irqs,
> +	.mpu_irqs_cnt	= ARRAY_SIZE(omap44xx_dmic_irqs),
> +	.sdma_reqs	= omap44xx_dmic_sdma_reqs,
> +	.sdma_reqs_cnt	= ARRAY_SIZE(omap44xx_dmic_sdma_reqs),
> +	.main_clk	= "dmic_fck",
> +	.prcm = {
> +		.omap4 = {
> +			.clkctrl_reg = OMAP4430_CM1_ABE_DMIC_CLKCTRL,
> +		},
> +	},
> +	.slaves		= omap44xx_dmic_slaves,
> +	.slaves_cnt	= ARRAY_SIZE(omap44xx_dmic_slaves),
> +	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> +};
> +
> +/*
>    * 'mpu_bus' class
>    * instance(s): mpu_private
>    */
> @@ -826,6 +915,8 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
>   	&omap44xx_l4_cfg_hwmod,
>   	&omap44xx_l4_per_hwmod,
>   	&omap44xx_l4_wkup_hwmod,
> +	/* dmic class */
> +	&omap44xx_dmic_hwmod,

This is not the right place, and a blank line is missing before and after.

Please find the fixed version below. This is based on top of the spinlock + mcspi + mcbsp to avoid conflict and to get the hwmod memory name support.

Regards,
Benoit

---