[PATCH v6 2/2] ARM: davinci: Remoteproc platform device creation data/code

[Tivy, Robert]

Hi Sergei,

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov at mvista.com]
> Sent: Saturday, January 26, 2013 6:43 AM
> 
> Hello.
> 
> On 26-01-2013 6:45, Robert Tivy wrote:
> 
> > Added a new remoteproc platform device for DA8XX.  Contains CMA-based
> > reservation of physical memory block.  A new kernel command-line
> > parameter has been added to allow boot-time specification of the
> > physical memory block.
> 
>     No signoff again.

Thanks, I will fix this.

> 
> > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-
> davinci/devices-da8xx.c
> > index fb2f51b..a455e5c 100644
> > --- a/arch/arm/mach-davinci/devices-da8xx.c
> > +++ b/arch/arm/mach-davinci/devices-da8xx.c
> [...]
> > @@ -706,6 +706,96 @@ int __init da850_register_mmcsd1(struct
> davinci_mmc_config *config)
> >   }
> >   #endif
> >
> > +static struct resource da8xx_rproc_resources[] = {
> > +	{ /* DSP boot address */
> > +		.start		= DA8XX_SYSCFG0_BASE +
> DA8XX_HOST1CFG_REG,
> > +		.end		= DA8XX_SYSCFG0_BASE + DA8XX_HOST1CFG_REG + 3,
> > +		.flags		= IORESOURCE_MEM,
> > +	},
> > +	{ /* DSP interrupt registers */
> > +		.start		= DA8XX_SYSCFG0_BASE + DA8XX_CHIPSIG_REG,
> > +		.end		= DA8XX_SYSCFG0_BASE + DA8XX_CHIPSIG_REG + 7,
> > +		.flags		= IORESOURCE_MEM,
> 
>     Does it really make sense to pass these as 2 resources -- they have
> the
> same base address?

I didn't want to impart that knowledge on the remoteproc driver.  As far as the driver is concerned, there is a boot address register and some interrupt-related registers.  That the boot address and interrupt-related registers share the same base address is a device-specific fact.

> 
> > +int __init da8xx_register_rproc(void)
> > +{
> > +	int ret;
> > +
> > +	ret = platform_device_register(&da8xx_dsp);
> > +	if (ret) {
> > +		pr_err("%s: platform_device_register: %d\n", __func__,
> ret);
> 
>     Better message would be "can't register DSP device".

Agreed, I will change this.

> 
> > +
> 
>     Empty line hardly needed here.
> 
> > +		return ret;
> 
>     Not needed here, just move it outside the {} to replace 'return 0'.

Thanks, I will change this.

Regards,

- Rob

> 
> > +	}
> > +
> > +	return 0;
> > +};
> > +
> 
> WBR, Sergei