[alsa-devel] [PATCH v2 06/13] OMAP4: hwmod data: Add McBSP

[Cousson, Benoit]

Hi Kishon,

I found a couple of weird stuff in this patch that does not look good to me and that must be fixed before merging that code.

On 1/31/2011 3:50 PM, ABRAHAM, KISHON VIJAY wrote:
> From: Benoit Cousson<b-cousson at ti.com>
> 
> Added a revision member inorder to facilitate the driver to
> differentiate between mcbsp in different omap.
> 
> Signed-off-by: Benoit Cousson<b-cousson at ti.com>
> Signed-off-by: Kishon Vijay Abraham I<kishon at ti.com>
> Signed-off-by: Charulatha V<charu at ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  321 ++++++++++++++++++++++++++++
>   arch/arm/plat-omap/include/plat/mcbsp.h    |    1 +
>   2 files changed, 322 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 c2806bd..a3860df 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -24,6 +24,7 @@
>   #include<plat/cpu.h>
>   #include<plat/gpio.h>
>   #include<plat/dma.h>
> +#include<plat/mcbsp.h>
> 
>   #include "omap_hwmod_common_data.h"
> 
> @@ -1435,6 +1436,320 @@ static struct omap_hwmod omap44xx_iva_hwmod = {
>   };
> 
>   /*
> + * 'mcbsp' class
> + * multi channel buffered serial port controller
> + */
> +
> +static struct omap_hwmod_class_sysconfig omap44xx_mcbsp_sysc = {
> +	.sysc_offs	= 0x008c,
> +	.sysc_flags	= (SYSC_HAS_CLOCKACTIVITY | SYSC_HAS_ENAWAKEUP |
> +			   SYSC_HAS_SIDLEMODE | SYSC_HAS_SOFTRESET),
> +	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
> +	.sysc_fields	=&omap_hwmod_sysc_type1,
> +};
> +
> +static struct omap_hwmod_class omap44xx_mcbsp_hwmod_class = {
> +	.name = "mcbsp",
> +	.sysc =&omap44xx_mcbsp_sysc,
> +	.rev  = MCBSP_CONFIG_TYPE4,
> +};
> +
> +/* mcbsp1 */
> +static struct omap_hwmod omap44xx_mcbsp1_hwmod;
> +static struct omap_hwmod_irq_info omap44xx_mcbsp1_irqs[] = {
> +	{ .name = "tx", .irq = 17 + OMAP44XX_IRQ_GIC_START },
> +	{ .name = "rx", .irq = 0 },

You should not do that! This is not even true. There is no more tx/rx lines in the HW anymore, so you should not try to emulate that in hwmod.
You should adapt your driver to handle the unique IRQ line, since this is the standard since OMAP3430.
These tx/rx lines were present but already mark as deprecated on OMAP3.
They are now removed on OMAP4 & OMAP5.

[...]

> +/* mcbsp4 */
> +static struct omap_hwmod omap44xx_mcbsp4_hwmod;
> +static struct omap_hwmod_irq_info omap44xx_mcbsp4_irqs[] = {
> +	{ .name = "tx", .irq = 16 + OMAP44XX_IRQ_GIC_START },
> +	{ .name = "rx", .irq = 0 },
> +};
> +
> +static struct omap_hwmod_dma_info omap44xx_mcbsp4_sdma_reqs[] = {
> +	{ .name = "tx", .dma_req = 30 + OMAP44XX_DMA_REQ_START },
> +	{ .name = "rx", .dma_req = 31 + OMAP44XX_DMA_REQ_START },
> +};
> +
> +static struct omap_hwmod_addr_space omap44xx_mcbsp4_addrs[] = {
> +	{
> +		.name		= "mpu",

This is not needed in this case.

> +		.pa_start	= 0x48096000,
> +		.pa_end		= 0x480960ff,
> +		.flags		= ADDR_TYPE_RT
> +	},
> +};
> +
> +/* l4_per ->  mcbsp4 */
> +static struct omap_hwmod_ocp_if omap44xx_l4_per__mcbsp4 = {
> +	.master		=&omap44xx_l4_per_hwmod,
> +	.slave		=&omap44xx_mcbsp4_hwmod,
> +	.clk		= "l4_div_ck",
> +	.addr		= omap44xx_mcbsp4_addrs,
> +	.addr_cnt	= ARRAY_SIZE(omap44xx_mcbsp4_addrs),
> +	.user		= OCP_USER_MPU,
> +};
> +
> +static struct omap_hwmod_addr_space omap44xx_mcbsp4_dma_addrs[] = {
> +	{
> +		.name           = "dma",
> +		.pa_start       = 0x48096000,
> +		.pa_end         = 0x480960ff,
> +		.flags          = ADDR_TYPE_RT

You should not do that. This is a duplication of the previous entry.
In general, you should not hack the hwmod data, which is a representation of the HW, to fit you specific driver needs.
You should request memory using name and if it failed you need to use the regular platform_get_resource method.
This is not ideal, but this is the best we can do with the current platform_get_XXX API.

Please find below the modified version of this patch rebased on top of the spinlock + mcspi series to avoid merge conflict.

Regards,
Benoit

---