[RFC/PATCH 09/14] dt: omap: prepare hwmod to support dt
Cousson, Benoit
b-cousson at ti.com
Thu Aug 11 03:11:27 EST 2011
On 8/10/2011 6:28 PM, G, Manjunath Kondaiah wrote:
> On Wed, Aug 10, 2011 at 01:51:47PM +0200, Cousson, Benoit wrote:
>> Hi Manju,
>>
>> On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
>>>
>>> The omap dt requires new omap hwmod api to be added for in order
>>> to support omap dt.
>>
>> Both the subject and the changelog are misleading. You are not doing
>> any hwmod stuff in it.
>> You are just passing an "of_node" pointer during omap_device_build_ss.
>>
>> The subject should start with OMAP: omap_device: because it does not
>> belong to the DT subsystem.
>> The same comment apply to most patches in that series.
>
> The goal of this patch is to make omap-hwmod ready for dt adaptation hence
> I used the title "dt: omap: prepare hwmod to support dt" and "of_node" pointer
> is passed from dt and it is required for dt build.
I think that the goal of this patch is to update the
omap_device_build_ss API in order to provide a device tree node pointer
to pdev. For the moment there is nothing related to hwmod.
> And as you mentioned, patch does not do anything related to omap_device.
You meant omap_hwmod?
Benoit
>>> The new api is added and new parameter "np" is added for existing
>>> api.
>>
>> I don't think np is not a super meaningful name. Some more
>> explanation are needed as well.
>
> ok. I can expand it.
>
>>
>>> The users of hwmod api is changed accrodingly.
>>
>> omap_device API + typo.
>>
>>> Build and boot tested on omap3 beagle for both dt and not dt build.
>>>
>>> Signed-off-by: G, Manjunath Kondaiah<manjugk at ti.com>
>>> ---
>>> arch/arm/mach-omap2/devices.c | 2 +-
>>> arch/arm/mach-omap2/mcbsp.c | 2 +-
>>> arch/arm/plat-omap/include/plat/omap_device.h | 11 +++++-
>>> arch/arm/plat-omap/omap_device.c | 53 ++++++++++++++++++++++---
>>> 4 files changed, 59 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
>>> index 458f7be..d7ff1ae 100644
>>> --- a/arch/arm/mach-omap2/devices.c
>>> +++ b/arch/arm/mach-omap2/devices.c
>>> @@ -92,7 +92,7 @@ static int __init omap4_l3_init(void)
>>> pr_err("could not look up %s\n", oh_name);
>>> }
>>>
>>> - pdev = omap_device_build_ss("omap_l3_noc", 0, oh, 3, NULL,
>>> + pdev = omap_device_build_ss(NULL, "omap_l3_noc", 0, oh, 3, NULL,
>>> 0, NULL, 0, 0);
>>
>> OK, maybe that is just me, but in order to extend an API, I'd rather
>> add the new parameter at the end.
>
> I feel it's fine since node pointer is first parameter is all dt api's.
>
>>
>>> WARN(IS_ERR(pdev), "could not build omap_device for %s\n", oh_name);
>>> diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
>>> index 7a42f32..98eb95d 100644
>>> --- a/arch/arm/mach-omap2/mcbsp.c
>>> +++ b/arch/arm/mach-omap2/mcbsp.c
>>> @@ -144,7 +144,7 @@ static int omap_init_mcbsp(struct omap_hwmod *oh, void *unused)
>>> (struct omap_mcbsp_dev_attr *)(oh->dev_attr))->sidetone);
>>> count++;
>>> }
>>> - pdev = omap_device_build_ss(name, id, oh_device, count, pdata,
>>> + pdev = omap_device_build_ss(NULL, name, id, oh_device, count, pdata,
>>> sizeof(*pdata), omap2_mcbsp_latency,
>>> ARRAY_SIZE(omap2_mcbsp_latency), false);
>>> kfree(pdata);
>>> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
>>> index 7a3ec4f..5e2424b 100644
>>> --- a/arch/arm/plat-omap/include/plat/omap_device.h
>>> +++ b/arch/arm/plat-omap/include/plat/omap_device.h
>>> @@ -33,6 +33,7 @@
>>>
>>> #include<linux/kernel.h>
>>> #include<linux/platform_device.h>
>>> +#include<linux/of.h>
>>>
>>> #include<plat/omap_hwmod.h>
>>>
>>> @@ -89,12 +90,20 @@ struct platform_device *omap_device_build(const char *pdev_name, int pdev_id,
>>> struct omap_device_pm_latency *pm_lats,
>>> int pm_lats_cnt, int is_early_device);
>>>
>>> -struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
>>> +struct platform_device *omap_device_build_ss(struct device_node *np,
>>> + const char *pdev_name, int pdev_id,
>>> struct omap_hwmod **oh, int oh_cnt,
>>> void *pdata, int pdata_len,
>>> struct omap_device_pm_latency *pm_lats,
>>> int pm_lats_cnt, int is_early_device);
>>>
>>> +struct platform_device *omap_device_build_dt(struct device_node *np,
>>> + const char *pdev_name, int pdev_id,
>>> + struct omap_hwmod *oh, void *pdata,
>>> + int pdata_len,
>>> + struct omap_device_pm_latency *pm_lats,
>>> + int pm_lats_cnt, int is_early_device);
>>> +
>>> void __iomem *omap_device_get_rt_va(struct omap_device *od);
>>>
>>> /* OMAP PM interface */
>>> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>>> index 7d5e76b..fa49168 100644
>>> --- a/arch/arm/plat-omap/omap_device.c
>>> +++ b/arch/arm/plat-omap/omap_device.c
>>> @@ -85,6 +85,7 @@
>>> #include<linux/clk.h>
>>> #include<linux/clkdev.h>
>>> #include<linux/pm_runtime.h>
>>> +#include<linux/of_device.h>
>>>
>>> #include<plat/omap_device.h>
>>> #include<plat/omap_hwmod.h>
>>> @@ -377,6 +378,7 @@ static int omap_device_fill_resources(struct omap_device *od,
>>> /**
>>> * omap_device_build - build and register an omap_device with one omap_hwmod
>>
>> Need to update the kerneldoc.
> ok.
>>
>>> * @pdev_name: name of the platform_device driver to use
>>> + * @np: device node pointer for attaching it to of_node pointer
>>> * @pdev_id: this platform_device's connection ID
>>> * @oh: ptr to the single omap_hwmod that backs this omap_device
>>> * @pdata: platform_data ptr to associate with the platform_device
>>> @@ -391,7 +393,8 @@ static int omap_device_fill_resources(struct omap_device *od,
>>> * information. Returns ERR_PTR(-EINVAL) if @oh is NULL; otherwise,
>>> * passes along the return value of omap_device_build_ss().
>>> */
>>> -struct platform_device *omap_device_build(const char *pdev_name, int pdev_id,
>>> +struct platform_device *omap_device_build_dt(struct device_node *np,
>>> + const char *pdev_name, int pdev_id,
>>> struct omap_hwmod *oh, void *pdata,
>>> int pdata_len,
>>> struct omap_device_pm_latency *pm_lats,
>>
>> That function should not be needed. You have to export
>> omap_device_build_ss, otherwise you will not build any device with
>> multiple hwmods.
> ok.
>>
>>> @@ -402,12 +405,44 @@ struct platform_device *omap_device_build(const char *pdev_name, int pdev_id,
>>> if (!oh)
>>> return ERR_PTR(-EINVAL);
>>>
>>> - return omap_device_build_ss(pdev_name, pdev_id, ohs, 1, pdata,
>>> + return omap_device_build_ss(np, pdev_name, pdev_id, ohs, 1, pdata,
>>> pdata_len, pm_lats, pm_lats_cnt,
>>> is_early_device);
>>> }
>>>
>>> /**
>>> + * omap_device_build - build and register an omap_device with one omap_hwmod
>>> + * @pdev_name: name of the platform_device driver to use
>>> + * @pdev_id: this platform_device's connection ID
>>> + * @oh: ptr to the single omap_hwmod that backs this omap_device
>>> + * @pdata: platform_data ptr to associate with the platform_device
>>> + * @pdata_len: amount of memory pointed to by @pdata
>>> + * @pm_lats: pointer to a omap_device_pm_latency array for this device
>>> + * @pm_lats_cnt: ARRAY_SIZE() of @pm_lats
>>> + * @is_early_device: should the device be registered as an early device or not
>>> + *
>>> + * Convenience function for building and registering a single
>>> + * omap_device record, which in turn builds and registers a
>>> + * platform_device record. See omap_device_build_ss() for more
>>> + * information. Returns ERR_PTR(-EINVAL) if @oh is NULL; otherwise,
>>> + * passes along the return value of omap_device_build_ss().
>>> + */
>>> +struct platform_device *omap_device_build(const char *pdev_name, int pdev_id,
>>> + struct omap_hwmod *oh, void *pdata,
>>> + int pdata_len,
>>> + struct omap_device_pm_latency *pm_lats,
>>> + int pm_lats_cnt, int is_early_device)
>>> +{
>>> + struct omap_hwmod *ohs[] = { oh };
>>> +
>>> + if (!oh)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + return omap_device_build_ss(NULL, pdev_name, pdev_id, ohs, 1, pdata,
>>> + pdata_len, pm_lats, pm_lats_cnt, is_early_device);
>>> +}
>>> +
>>> +/**
>>> * omap_device_build_ss - build and register an omap_device with multiple hwmods
>>> * @pdev_name: name of the platform_device driver to use
>>> * @pdev_id: this platform_device's connection ID
>>> @@ -424,7 +459,8 @@ struct platform_device *omap_device_build(const char *pdev_name, int pdev_id,
>>> * platform_device record. Returns an ERR_PTR() on error, or passes
>>> * along the return value of omap_device_register().
>>> */
>>> -struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
>>> +struct platform_device *omap_device_build_ss(struct device_node *np,
>>> + const char *pdev_name, int pdev_id,
>>> struct omap_hwmod **ohs, int oh_cnt,
>>> void *pdata, int pdata_len,
>>> struct omap_device_pm_latency *pm_lats,
>>> @@ -436,6 +472,7 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
>>> struct resource *res = NULL;
>>> int i, res_count;
>>> struct omap_hwmod **hwmods;
>>> + struct platform_device *pdev;
>>>
>>> if (!ohs || oh_cnt == 0 || !pdev_name)
>>> return ERR_PTR(-EINVAL);
>>> @@ -450,6 +487,7 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
>>> if (!od)
>>> return ERR_PTR(-ENOMEM);
>>>
>>> + pdev =&od->pdev;
>>
>> Why do you need that? You are just adding one more user of the pdev.
>
> just improve readability otherwise we need to have&od->pdev at multiple places
> below.
>
>>
>>> od->hwmods_cnt = oh_cnt;
>>>
>>> hwmods = kzalloc(sizeof(struct omap_hwmod *) * oh_cnt,
>>> @@ -465,8 +503,11 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
>>> goto odbs_exit2;
>>> strcpy(pdev_name2, pdev_name);
>>>
>>> - od->pdev.name = pdev_name2;
>>> - od->pdev.id = pdev_id;
>>> +#ifdef CONFIG_OF_DEVICE
>>> + pdev->dev.of_node = of_node_get(np);
>>> +#endif
>>> + pdev->name = pdev_name2;
>>> + pdev->id = pdev_id;
>>>
>>> res_count = omap_device_count_resources(od);
>>> if (res_count> 0) {
>>> @@ -499,7 +540,7 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
>>> if (ret)
>>> goto odbs_exit4;
>>>
>>> - return&od->pdev;
>>> + return pdev;
>>>
>>> odbs_exit4:
>>> kfree(res);
>>
>> Regards,
>> Benoit
>>
>>
>> _______________________________________________
>> devicetree-discuss mailing list
>> devicetree-discuss at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/devicetree-discuss
More information about the devicetree-discuss
mailing list