Hi Ohad
Thanks for reviewing.
On Sun, May 6, 2012 at 6:21 AM, Ohad Ben-Cohen <ohad@xxxxxxxxxx> wrote:
> Hi Juan,
>
> Thanks for the patches ! it's exciting to see that support for OMAP's
> DSP is (relatively) easily achieved now. I hope your work can be a
> good basis for an OMAP3 port, too.
>
> Overall the patches look good, I have only some minor comments inline.
>
> And - sorry for the late response. I've just left for a (somewhat
> long) business trip when you posted this.
>
> On Sat, Apr 14, 2012 at 2:39 AM, Juan Gutierrez <jgutierrez@xxxxxx> wrote:
>> Some remote processors (like Omap4's DSP) need to explicitly
>> configure a boot address in a special register or memory
>> location
>
> Let's just slightly rephrase this to prevent confusion: some remote
> processors "need to have the boot address configured" in a special
> register (as opposed to "need to configure").
>
Ok, I will do it.
>> @@ -30,6 +30,7 @@ struct platform_device;
>> * @ops: start/stop rproc handlers
>> * @device_enable: omap-specific handler for enabling a device
>> * @device_shutdown: omap-specific handler for shutting down a device
>> + * @boot_reg: physical address of the control register for storing boot address
>> */
>> struct omap_rproc_pdata {
>> const char *name;
>> @@ -40,6 +41,7 @@ struct omap_rproc_pdata {
>> const struct rproc_ops *ops;
>> int (*device_enable) (struct platform_device *pdev);
>> int (*device_shutdown) (struct platform_device *pdev);
>> + u32 boot_reg;
>
> We might want to use an IORESOURCE_MEM resource instead, since we're
> dealing with a platform device anyway.
>
> The driver can then fetch the address using platform_get_resource.
>
Ok, I will investigate about this, and include it in next series
>> @@ -203,6 +215,9 @@ static int __devinit omap_rproc_probe(struct platform_device *pdev)
>> return 0;
>>
>> free_rproc:
>> + if (oproc->boot_reg)
>> + iounmap(oproc->boot_reg);
>> +err_ioremap:
>> rproc_free(rproc);
>> return ret;
>> }
>
> I tend to call those cleanup snippets with names that indicate what they do.
>
> In this case I'd slightly prefer to keep calling the lower snippet
> with "free_rproc" and just name the new snippet with something like
> "unmap_bootreg". It's then a bit easier to read the code that calls
> into those snippets (the reader easily know which cleanup snippet is
> invoked by each 'goto').
>
Agree. I will do the change.
> Thanks,
> Ohad.
--
Thanks
juan gutierrez
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
