[RFC/PATCH 00/14] dt: omap hwmod-dt binding and omap3 i2c1 dt support

[RFC/PATCH 00/14] dt: omap hwmod-dt binding and omap3 i2c1 dt support

[Rajendra Nayak]

On 8/9/2011 7:40 PM, G, Manjunath Kondaiah wrote:
>
> This is in continuation of patch series posted at:
> http://lwn.net/Articles/451917/
>
> Patches from Kevin Hilman and others are consolidated along with this
> series since it will be part of ongoing work of decoupling pdev from
> omap_device.
> [There is latest version posted which will be rebased later]
>
> Apart from the above, the major feature included in this series is
> dt-hwmod binding and preparing omap hwmod framework for adapting omap
> drivers to support omap dt after aligning with Grant Likely
> <grant.likely@secretlab.ca>
>
> However, current hwmod-dt binding will be replaced with notifiers in
> generic board file and hwmod device pointer will be scanned and attached
> in notifier call.
>
> Used 3.0 stable kernel version for this patch series since latest mainline
> was broken for beagle boot last week.
>
> git://git.secretlab.ca/git/linux-2.6.git
> Branch: devicetree/test-3.0

Is there a reason why these are based on devicetree/test-3.0 and
not devicetree/test?

>
> G, Manjunath Kondaiah (6):
>    dt: omap: prepare hwmod to support dt
>    dt: Add pd_size to AUXDATA structure
>    dt: omap3: add soc file for handling i2c controllers
>    dt: omap3: beagle board: set clock freq for i2c devices
>    dt: omap3: add generic board file for dt support
>    dt: omap3: enable dt support for i2c1 controller
>
> Kevin Hilman (7):
>    OMAP: omap_device: replace _find_by_pdev() with to_omap_device()
>    OMAP: omap_device: replace debug/warning/error prints with dev_*
>      macros
>    OMAP3: beagle: don't touch omap_device internals
>    OMAP: McBSP: use existing macros for converting between devices
>    OMAP: omap_device: remove internal functions from omap_device.h
>    OMAP: omap_device: when building return platform_device instead of
>      omap_device
>    ARM: platform_device: pdev_archdata: add omap_device pointer
>
> Tony Lindgren (1):
>    omap2+: Use Kconfig symbol in Makefile instead of obj-y
>
>   arch/arm/boot/dts/omap3-beagle-nunchuck.dts   |   11 +--
>   arch/arm/boot/dts/omap3-beagle.dts            |   18 +++-
>   arch/arm/boot/dts/omap3-soc.dtsi              |   62 ++++++++++
>   arch/arm/include/asm/device.h                 |    5 +
>   arch/arm/mach-omap2/Kconfig                   |   11 ++
>   arch/arm/mach-omap2/Makefile                  |   77 ++++++-------
>   arch/arm/mach-omap2/board-omap3-dt.c          |   93 +++++++++++++++
>   arch/arm/mach-omap2/board-omap3beagle.c       |   40 ++-----
>   arch/arm/mach-omap2/devices.c                 |   44 ++++----
>   arch/arm/mach-omap2/display.c                 |    6 +-
>   arch/arm/mach-omap2/dma.c                     |   16 ++--
>   arch/arm/mach-omap2/gpio.c                    |    8 +-
>   arch/arm/mach-omap2/hsmmc.c                   |    8 +-
>   arch/arm/mach-omap2/hwspinlock.c              |    8 +-
>   arch/arm/mach-omap2/mcbsp.c                   |    8 +-
>   arch/arm/mach-omap2/pm.c                      |    8 +-
>   arch/arm/mach-omap2/serial.c                  |   12 +-
>   arch/arm/plat-omap/i2c.c                      |    8 +-
>   arch/arm/plat-omap/include/plat/omap_device.h |   17 ++-
>   arch/arm/plat-omap/mcbsp.c                    |    6 +-
>   arch/arm/plat-omap/omap_device.c              |  150 ++++++++++++++++---------
>   drivers/i2c/busses/i2c-omap.c                 |   23 ++++-
>   drivers/of/platform.c                         |   41 +++++++
>   include/linux/of_platform.h                   |    5 +
>   24 files changed, 469 insertions(+), 216 deletions(-)
>   create mode 100644 arch/arm/boot/dts/omap3-soc.dtsi
>   create mode 100644 arch/arm/mach-omap2/board-omap3-dt.c
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

[RFC/PATCH 00/14] dt: omap hwmod-dt binding and omap3 i2c1 dt support

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 10:56 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> On 8/9/2011 7:40 PM, G, Manjunath Kondaiah wrote:
>>
>> This is in continuation of patch series posted at:
>> http://lwn.net/Articles/451917/
>>
>> Patches from Kevin Hilman and others are consolidated along with this
>> series since it will be part of ongoing work of decoupling pdev from
>> omap_device.
>> [There is latest version posted which will be rebased later]
>>
>> Apart from the above, the major feature included in this series is
>> dt-hwmod binding and preparing omap hwmod framework for adapting omap
>> drivers to support omap dt after aligning with Grant Likely
>> <grant.likely@secretlab.ca>
>>
>> However, current hwmod-dt binding will be replaced with notifiers in
>> generic board file and hwmod device pointer will be scanned and attached
>> in notifier call.
>>
>> Used 3.0 stable kernel version for this patch series since latest mainline
>> was broken for beagle boot last week.
>>
>> git://git.secretlab.ca/git/linux-2.6.git
>> Branch: devicetree/test-3.0
>
> Is there a reason why these are based on devicetree/test-3.0 and
> not devicetree/test?

devicetree/test was broken last week since grant has pulled latest
mainline changes hence selected stable version.

-M

[...]

[RFC/PATCH 00/14] dt: omap hwmod-dt binding and omap3 i2c1 dt support

[Rajendra Nayak]

On 8/10/2011 11:00 AM, G, Manjunath Kondaiah wrote:
> On Wed, Aug 10, 2011 at 10:56 AM, Rajendra Nayak<rnayak@ti.com>  wrote:
>> On 8/9/2011 7:40 PM, G, Manjunath Kondaiah wrote:
>>>
>>> This is in continuation of patch series posted at:
>>> http://lwn.net/Articles/451917/
>>>
>>> Patches from Kevin Hilman and others are consolidated along with this
>>> series since it will be part of ongoing work of decoupling pdev from
>>> omap_device.
>>> [There is latest version posted which will be rebased later]
>>>
>>> Apart from the above, the major feature included in this series is
>>> dt-hwmod binding and preparing omap hwmod framework for adapting omap
>>> drivers to support omap dt after aligning with Grant Likely
>>> <grant.likely@secretlab.ca>
>>>
>>> However, current hwmod-dt binding will be replaced with notifiers in
>>> generic board file and hwmod device pointer will be scanned and attached
>>> in notifier call.
>>>
>>> Used 3.0 stable kernel version for this patch series since latest mainline
>>> was broken for beagle boot last week.
>>>
>>> git://git.secretlab.ca/git/linux-2.6.git
>>> Branch: devicetree/test-3.0
>>
>> Is there a reason why these are based on devicetree/test-3.0 and
>> not devicetree/test?
>
> devicetree/test was broken last week since grant has pulled latest
> mainline changes hence selected stable version.

Ok, atleast mainline is fixed now.

>
> -M
>
> [...]

[RFC/PATCH 00/14] dt: omap hwmod-dt binding and omap3 i2c1 dt support

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 11:09 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> On 8/10/2011 11:00 AM, G, Manjunath Kondaiah wrote:
>>
>> On Wed, Aug 10, 2011 at 10:56 AM, Rajendra Nayak<rnayak@ti.com> ?wrote:
>>>
>>> On 8/9/2011 7:40 PM, G, Manjunath Kondaiah wrote:
>>>>
>>>> This is in continuation of patch series posted at:
>>>> http://lwn.net/Articles/451917/
>>>>
>>>> Patches from Kevin Hilman and others are consolidated along with this
>>>> series since it will be part of ongoing work of decoupling pdev from
>>>> omap_device.
>>>> [There is latest version posted which will be rebased later]
>>>>
>>>> Apart from the above, the major feature included in this series is
>>>> dt-hwmod binding and preparing omap hwmod framework for adapting omap
>>>> drivers to support omap dt after aligning with Grant Likely
>>>> <grant.likely@secretlab.ca>
>>>>
>>>> However, current hwmod-dt binding will be replaced with notifiers in
>>>> generic board file and hwmod device pointer will be scanned and attached
>>>> in notifier call.
>>>>
>>>> Used 3.0 stable kernel version for this patch series since latest
>>>> mainline
>>>> was broken for beagle boot last week.
>>>>
>>>> git://git.secretlab.ca/git/linux-2.6.git
>>>> Branch: devicetree/test-3.0
>>>
>>> Is there a reason why these are based on devicetree/test-3.0 and
>>> not devicetree/test?
>>
>> devicetree/test was broken last week since grant has pulled latest
>> mainline changes hence selected stable version.
>
> Ok, atleast mainline is fixed now.

These changes are for v3.2 merge window and it will be rebased later
after the review.

-M

[RFC/PATCH 04/14] OMAP: McBSP: use existing macros for converting between devices

[Jarkko Nikula]

Hi

On Tue, 09 Aug 2011 19:10:22 +0500
"G, Manjunath Kondaiah" <manjugk@ti.com> wrote:

> From: Kevin Hilman <khilman@ti.com>
> 
> For converting from struct device to platform_device, and from
> platform_device to struct device, there are existing macros.  Use
> them instead of manual use of container_of().
> 
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
>  arch/arm/plat-omap/mcbsp.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
Kevin dropped this patch and used mine that does more cleanups:

http://marc.info/?l=linux-omap&m=131255627918065&w=2

Worth to check also other patches since Kevin's updated set seems to
have other differences too:

http://marc.info/?l=linux-omap&m=131258997416300&w=2

-- 
Jarkko

[RFC/PATCH 04/14] OMAP: McBSP: use existing macros for converting between devices

[Cousson, Benoit]

Manju,

On 8/10/2011 9:07 AM, Jarkko Nikula wrote:
> Hi
>
> On Tue, 09 Aug 2011 19:10:22 +0500
> "G, Manjunath Kondaiah"<manjugk@ti.com>  wrote:
>
>> From: Kevin Hilman<khilman@ti.com>
>>
>> For converting from struct device to platform_device, and from
>> platform_device to struct device, there are existing macros.  Use
>> them instead of manual use of container_of().
>>
>> Signed-off-by: Kevin Hilman<khilman@ti.com>
>> ---
>>   arch/arm/plat-omap/mcbsp.c |    6 +++---
>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>
> Kevin dropped this patch and used mine that does more cleanups:
>
> http://marc.info/?l=linux-omap&m=131255627918065&w=2
>
> Worth to check also other patches since Kevin's updated set seems to
> have other differences too:
>
> http://marc.info/?l=linux-omap&m=131258997416300&w=2

In fact, you'd better point to the lastest Kevin's tree and not included 
the patches in this series.

Benoit

[RFC/PATCH 04/14] OMAP: McBSP: use existing macros for converting between devices

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 12:15:50PM +0200, Cousson, Benoit wrote:
> Manju,
> 
> On 8/10/2011 9:07 AM, Jarkko Nikula wrote:
> >Hi
> >
> >On Tue, 09 Aug 2011 19:10:22 +0500
> >"G, Manjunath Kondaiah"<manjugk@ti.com>  wrote:
> >
> >>From: Kevin Hilman<khilman@ti.com>
> >>
> >>For converting from struct device to platform_device, and from
> >>platform_device to struct device, there are existing macros.  Use
> >>them instead of manual use of container_of().
> >>
> >>Signed-off-by: Kevin Hilman<khilman@ti.com>
> >>---
> >>  arch/arm/plat-omap/mcbsp.c |    6 +++---
> >>  1 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >Kevin dropped this patch and used mine that does more cleanups:
> >
> >http://marc.info/?l=linux-omap&m=131255627918065&w=2
> >
> >Worth to check also other patches since Kevin's updated set seems to
> >have other differences too:
> >
> >http://marc.info/?l=linux-omap&m=131258997416300&w=2
> 
> In fact, you'd better point to the lastest Kevin's tree and not
> included the patches in this series.

Yes. I have already mentioned this in PATCH 00/14 and it will be rebased
during rework for review comments.

Main focus here is on dt-hwmod binding and omap-i2c dt adaptation.

-M

[RFC/PATCH 09/14] dt: omap: prepare hwmod to support dt

[Cousson, Benoit]

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 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.

> 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@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.

>   	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.

>    * @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.

> @@ -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.

>   	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

[RFC/PATCH 09/14] dt: omap: prepare hwmod to support dt

[G, Manjunath Kondaiah]

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.

And as you mentioned, patch does not do anything related to omap_device.

> 
> >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@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

[RFC/PATCH 09/14] dt: omap: prepare hwmod to support dt

[Cousson, Benoit]

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@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

[RFC/PATCH 09/14] dt: omap: prepare hwmod to support dt

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 10:41 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> 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.
yes. it is to update  *_ss api with device node pointer and introduce new
api exported for dt builds.  I can update with this description.
>
>> And as you mentioned, patch does not do anything related to omap_device.
>
> You meant omap_hwmod?

Yes. It should be "omap: omap_hwmod: add device node pointer"

-M

[RFC/PATCH 09/14] dt: omap: prepare hwmod to support dt

[Cousson, Benoit]

On 8/10/2011 8:03 PM, G, Manjunath Kondaiah wrote:
> On Wed, Aug 10, 2011 at 10:41 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> 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.
> yes. it is to update  *_ss api with device node pointer and introduce new
> api exported for dt builds.  I can update with this description.
>>
>>> And as you mentioned, patch does not do anything related to omap_device.
>>
>> You meant omap_hwmod?
>
> Yes. It should be "omap: omap_hwmod: add device node pointer"

... "OMAP: omap_device: Add device tree node pointer"

[RFC/PATCH 09/14] dt: omap: prepare hwmod to support dt

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 09:58:46PM +0530, 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.
> 
> And as you mentioned, patch does not do anything related to omap_device.
> 
> > 
> > >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@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.

As these API's are interim API's and we might have alternate setup to handle
hwmod in the coming days, the documentation part will be taken care along with
final solution for hwmod issue.

> > 
> > >   * @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.

confused here. All the three API's *_dt/*_ss/*_dt are exported and can be
accessed for single or multiple hwmods. Am I missing anything?

-M

[RFC/PATCH 10/14] dt: Add pd_size to AUXDATA structure

[Cousson, Benoit]

On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
>
> Add pd_size in the AUXDATA structure so that device drivers which require
> platform_data size can pass along with AUXDATA.

It is really needed by device driver? Or is it because omap_device_build 
is using platform_device_add_data that is doing a copy of the pdata and 
thus require the size?

> Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> ---
>   drivers/of/platform.c       |    2 ++
>   include/linux/of_platform.h |    5 +++++
>   2 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ebbbf42..4b27286 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -565,6 +565,7 @@ static int of_platform_bus_create(struct device_node *bus,
>   	struct platform_device *dev;
>   	const char *bus_id = NULL;
>   	void *platform_data = NULL;
> +	int pd_size;

Maybe platform_data_size will be a little bit more consistent?

Regards,
Benoit

>   	int id = -1;
>   	int rc = 0;
>
> @@ -588,6 +589,7 @@ static int of_platform_bus_create(struct device_node *bus,
>   		bus_id = auxdata->name;
>   		id = auxdata->id;
>   		platform_data = auxdata->platform_data;
> +		pd_size = auxdata->pd_size;
>   	}
>
>   	if (of_device_is_compatible(bus, "arm,primecell")) {
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 252246c..a3be980 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -47,6 +47,7 @@ struct of_dev_auxdata {
>   	char *name;
>   	int id;
>   	void *platform_data;
> +	int pd_size;
>   };
>
>   /* Macro to simplify populating a lookup table */
> @@ -58,6 +59,10 @@ struct of_dev_auxdata {
>   	{ .compatible = _compat, .phys_addr = _phys, .name = _name, \
>   	  .id = _id, .platform_data = _pdata }
>
> +#define OF_DEV_AUXDATA_ID_PDSIZE(_compat,_phys,_name,_id,_pdata,_pd_size) \
> +	{ .compatible = _compat, .phys_addr = _phys, .name = _name, \
> +	  .id = _id, .platform_data = _pdata, .pd_size = _pd_size }
> +
>   /**
>    * of_platform_driver - Legacy of-aware driver for platform devices.
>    *

[RFC/PATCH 10/14] dt: Add pd_size to AUXDATA structure

[Grant Likely]

On Wed, Aug 10, 2011 at 5:57 AM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
>>
>> Add pd_size in the AUXDATA structure so that device drivers which require
>> platform_data size can pass along with AUXDATA.
>
> It is really needed by device driver? Or is it because omap_device_build is
> using platform_device_add_data that is doing a copy of the pdata and thus
> require the size?

Yes, I have the same question.  What is the reason for needing the
platform data size?

g.

>
>> Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
>> ---
>> ?drivers/of/platform.c ? ? ? | ? ?2 ++
>> ?include/linux/of_platform.h | ? ?5 +++++
>> ?2 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index ebbbf42..4b27286 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -565,6 +565,7 @@ static int of_platform_bus_create(struct device_node
>> *bus,
>> ? ? ? ?struct platform_device *dev;
>> ? ? ? ?const char *bus_id = NULL;
>> ? ? ? ?void *platform_data = NULL;
>> + ? ? ? int pd_size;
>
> Maybe platform_data_size will be a little bit more consistent?
>
> Regards,
> Benoit
>
>> ? ? ? ?int id = -1;
>> ? ? ? ?int rc = 0;
>>
>> @@ -588,6 +589,7 @@ static int of_platform_bus_create(struct device_node
>> *bus,
>> ? ? ? ? ? ? ? ?bus_id = auxdata->name;
>> ? ? ? ? ? ? ? ?id = auxdata->id;
>> ? ? ? ? ? ? ? ?platform_data = auxdata->platform_data;
>> + ? ? ? ? ? ? ? pd_size = auxdata->pd_size;
>> ? ? ? ?}
>>
>> ? ? ? ?if (of_device_is_compatible(bus, "arm,primecell")) {
>> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
>> index 252246c..a3be980 100644
>> --- a/include/linux/of_platform.h
>> +++ b/include/linux/of_platform.h
>> @@ -47,6 +47,7 @@ struct of_dev_auxdata {
>> ? ? ? ?char *name;
>> ? ? ? ?int id;
>> ? ? ? ?void *platform_data;
>> + ? ? ? int pd_size;
>> ?};
>>
>> ?/* Macro to simplify populating a lookup table */
>> @@ -58,6 +59,10 @@ struct of_dev_auxdata {
>> ? ? ? ?{ .compatible = _compat, .phys_addr = _phys, .name = _name, \
>> ? ? ? ? ?.id = _id, .platform_data = _pdata }
>>
>> +#define OF_DEV_AUXDATA_ID_PDSIZE(_compat,_phys,_name,_id,_pdata,_pd_size)
>> \
>> + ? ? ? { .compatible = _compat, .phys_addr = _phys, .name = _name, \
>> + ? ? ? ? .id = _id, .platform_data = _pdata, .pd_size = _pd_size }
>> +
>> ?/**
>> ? * of_platform_driver - Legacy of-aware driver for platform devices.
>> ? *
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[RFC/PATCH 10/14] dt: Add pd_size to AUXDATA structure

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 07:16:30AM -0600, Grant Likely wrote:
> On Wed, Aug 10, 2011 at 5:57 AM, Cousson, Benoit <b-cousson@ti.com> wrote:
> > On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
> >>
> >> Add pd_size in the AUXDATA structure so that device drivers which require
> >> platform_data size can pass along with AUXDATA.
> >
> > It is really needed by device driver? Or is it because omap_device_build is
> > using platform_device_add_data that is doing a copy of the pdata and thus
> > require the size?
> 
> Yes, I have the same question.  What is the reason for needing the
> platform data size?

Yes. It is required by "omap_device_build" which in turn calls 
"platform_device_add_data" and copies using "kmemdup" and original
pdata pointer memory will get freed.

It is required by hwmod and not device driver. I can change description.

> 
> g.
> 
> >
> >> Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> >> ---
> >> ?drivers/of/platform.c ? ? ? | ? ?2 ++
> >> ?include/linux/of_platform.h | ? ?5 +++++
> >> ?2 files changed, 7 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index ebbbf42..4b27286 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -565,6 +565,7 @@ static int of_platform_bus_create(struct device_node
> >> *bus,
> >> ? ? ? ?struct platform_device *dev;
> >> ? ? ? ?const char *bus_id = NULL;
> >> ? ? ? ?void *platform_data = NULL;
> >> + ? ? ? int pd_size;
> >
> > Maybe platform_data_size will be a little bit more consistent?

Thought shorter name is better and still it should be readable.

-M

[RFC/PATCH 11/14] dt: omap3: add soc file for handling i2c controllers

[Cousson, Benoit]

On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
>
> Add omap3 soc file for handling omap3 soc i2c controllers existing
> on l4-core bus.
>
> Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> ---
>   arch/arm/boot/dts/omap3-soc.dtsi |   62 ++++++++++++++++++++++++++++++++++++++
>   1 files changed, 62 insertions(+), 0 deletions(-)
>   create mode 100644 arch/arm/boot/dts/omap3-soc.dtsi
>
> diff --git a/arch/arm/boot/dts/omap3-soc.dtsi b/arch/arm/boot/dts/omap3-soc.dtsi
> new file mode 100644
> index 0000000..85de92f
> --- /dev/null
> +++ b/arch/arm/boot/dts/omap3-soc.dtsi
> @@ -0,0 +1,62 @@
> +/*
> + * Device Tree Source for OMAP3 SoC
> + *
> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2.  This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
> +
> +/dts-v1/;
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	#address-cells =<1>;
> +	#size-cells =<1>;
> +	model = "ti,omap3";
> +
> +	aliases {
> +		i2c1 =&i2c1;
> +		i2c2 =&i2c2;
> +		i2c3 =&i2c3;
> +	};
> +
> +	l4-core {

That comment is probably subject to discussion, but even if this 
interconnect is there physically, I'm not sure of the added value to add it.
It will add an extra level of indentation and that all. Moreover, it 
will mess up the physical address that are expressed using absolute 
value in the TRM with a less readable offset value.
In fact, most DTS files in the ARM directory are using a purely flat 
representation of the interconnect.

> +		compatible = "ti,omap3-l4-core";

Assuming we will keep that, you should probably add a more generic 
compatible name after that one. Like "ti,s3220" or even "sonics,s3220", 
assuming that this IP is generic enough for other SoC.

> +		#address-cells =<1>;
> +		#size-cells =<1>;
> +		ranges =<0 0x48000000 0x1000000>;
> +
> +		i2c1: i2c at 70000 {
> +			#address-cells =<1>;
> +			#size-cells =<0>;
> +			compatible = "ti,omap3-i2c";

The I2C IP and thus the driver is generic across OMAP generations and is 
even potentially used by other non-OMAP TI chips like DSP or Davinci. So 
having an extra compatible entry with "ti,i2c" or "ti, omap-i2c" seems 
mandatory.

Regards,
Benoit

[RFC/PATCH 11/14] dt: omap3: add soc file for handling i2c controllers

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 02:36:50PM +0200, Cousson, Benoit wrote:
> On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
> >
> >Add omap3 soc file for handling omap3 soc i2c controllers existing
> >on l4-core bus.
> >
> >Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> >---
> >  arch/arm/boot/dts/omap3-soc.dtsi |   62 ++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 62 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/boot/dts/omap3-soc.dtsi
> >
> >diff --git a/arch/arm/boot/dts/omap3-soc.dtsi b/arch/arm/boot/dts/omap3-soc.dtsi
> >new file mode 100644
> >index 0000000..85de92f
> >--- /dev/null
> >+++ b/arch/arm/boot/dts/omap3-soc.dtsi
> >@@ -0,0 +1,62 @@
> >+/*
> >+ * Device Tree Source for OMAP3 SoC
> >+ *
> >+ * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
> >+ *
> >+ * This file is licensed under the terms of the GNU General Public License
> >+ * version 2.  This program is licensed "as is" without any warranty of any
> >+ * kind, whether express or implied.
> >+ */
> >+
> >+/dts-v1/;
> >+/include/ "skeleton.dtsi"
> >+
> >+/ {
> >+	#address-cells =<1>;
> >+	#size-cells =<1>;
> >+	model = "ti,omap3";
> >+
> >+	aliases {
> >+		i2c1 =&i2c1;
> >+		i2c2 =&i2c2;
> >+		i2c3 =&i2c3;
> >+	};
> >+
> >+	l4-core {
> 
> That comment is probably subject to discussion, but even if this
> interconnect is there physically, I'm not sure of the added value to
> add it.
> It will add an extra level of indentation and that all. Moreover, it
> will mess up the physical address that are expressed using absolute
> value in the TRM with a less readable offset value.
> In fact, most DTS files in the ARM directory are using a purely flat
> representation of the interconnect.
This is as per alignment with Tony and Grant:
http://permalink.gmane.org/gmane.linux.ports.arm.omap/60391

> 
> >+		compatible = "ti,omap3-l4-core";
> 
> Assuming we will keep that, you should probably add a more generic
> compatible name after that one. Like "ti,s3220" or even
> "sonics,s3220", assuming that this IP is generic enough for other
> SoC.
will check this. I don't remember any generic names.
> 
> >+		#address-cells =<1>;
> >+		#size-cells =<1>;
> >+		ranges =<0 0x48000000 0x1000000>;
> >+
> >+		i2c1: i2c at 70000 {
> >+			#address-cells =<1>;
> >+			#size-cells =<0>;
> >+			compatible = "ti,omap3-i2c";
> 
> The I2C IP and thus the driver is generic across OMAP generations
> and is even potentially used by other non-OMAP TI chips like DSP or
> Davinci. So having an extra compatible entry with "ti,i2c" or "ti,
> omap-i2c" seems mandatory.
This can be updated as and when new soc/board adaptations are done. 
As of now, it is omap3 and when we have omap4 it will be appended with
"ti,omap4-i2c" etc

-M

[RFC/PATCH 11/14] dt: omap3: add soc file for handling i2c controllers

[Cousson, Benoit]

+ Tony

On 8/10/2011 6:57 PM, G, Manjunath Kondaiah wrote:
> On Wed, Aug 10, 2011 at 02:36:50PM +0200, Cousson, Benoit wrote:
>> On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
>>>
>>> Add omap3 soc file for handling omap3 soc i2c controllers existing
>>> on l4-core bus.
>>>
>>> Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
>>> ---
>>>   arch/arm/boot/dts/omap3-soc.dtsi |   62 ++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 62 insertions(+), 0 deletions(-)
>>>   create mode 100644 arch/arm/boot/dts/omap3-soc.dtsi
>>>
>>> diff --git a/arch/arm/boot/dts/omap3-soc.dtsi b/arch/arm/boot/dts/omap3-soc.dtsi
>>> new file mode 100644
>>> index 0000000..85de92f
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/omap3-soc.dtsi
>>> @@ -0,0 +1,62 @@
>>> +/*
>>> + * Device Tree Source for OMAP3 SoC
>>> + *
>>> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
>>> + *
>>> + * This file is licensed under the terms of the GNU General Public License
>>> + * version 2.  This program is licensed "as is" without any warranty of any
>>> + * kind, whether express or implied.
>>> + */
>>> +
>>> +/dts-v1/;
>>> +/include/ "skeleton.dtsi"
>>> +
>>> +/ {
>>> +	#address-cells =<1>;
>>> +	#size-cells =<1>;
>>> +	model = "ti,omap3";
>>> +
>>> +	aliases {
>>> +		i2c1 =&i2c1;
>>> +		i2c2 =&i2c2;
>>> +		i2c3 =&i2c3;
>>> +	};
>>> +
>>> +	l4-core {
>>
>> That comment is probably subject to discussion, but even if this
>> interconnect is there physically, I'm not sure of the added value to
>> add it.
>> It will add an extra level of indentation and that all. Moreover, it
>> will mess up the physical address that are expressed using absolute
>> value in the TRM with a less readable offset value.
>> In fact, most DTS files in the ARM directory are using a purely flat
>> representation of the interconnect.
> This is as per alignment with Tony and Grant:
> http://permalink.gmane.org/gmane.linux.ports.arm.omap/60391

So I'm asking the same question to Grant and Tony then:-)

>>> +		compatible = "ti,omap3-l4-core";
>>
>> Assuming we will keep that, you should probably add a more generic
>> compatible name after that one. Like "ti,s3220" or even
>> "sonics,s3220", assuming that this IP is generic enough for other
>> SoC.
> will check this. I don't remember any generic names.
>>
>>> +		#address-cells =<1>;
>>> +		#size-cells =<1>;
>>> +		ranges =<0 0x48000000 0x1000000>;
>>> +
>>> +		i2c1: i2c at 70000 {
>>> +			#address-cells =<1>;
>>> +			#size-cells =<0>;
>>> +			compatible = "ti,omap3-i2c";
>>
>> The I2C IP and thus the driver is generic across OMAP generations
>> and is even potentially used by other non-OMAP TI chips like DSP or
>> Davinci. So having an extra compatible entry with "ti,i2c" or "ti,
>> omap-i2c" seems mandatory.
> This can be updated as and when new soc/board adaptations are done.
> As of now, it is omap3 and when we have omap4 it will be appended with
> "ti,omap4-i2c" etc

To infinity and beyond?

There is no need, and we should absolutely not update the driver each 
time we introduce a new SoC version/revision.
The driver should match with the generic device name in that case. Until 
we change the IP and potentially introduce a new driver.

BTW, even omap3 should not be in the name in theory. It should be 
potentially "ti,i2c-v3" or whatever HW version the IP is using.
But for some reason, most devices are using such convention in existing 
DTS:-(
This is probably due to the lack of well identified IP version in most 
SoC... including OMAP:-)


Benoit

[RFC/PATCH 11/14] dt: omap3: add soc file for handling i2c controllers

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 07:45:42PM +0200, Cousson, Benoit wrote:
> + Tony
> 
> On 8/10/2011 6:57 PM, G, Manjunath Kondaiah wrote:
> >On Wed, Aug 10, 2011 at 02:36:50PM +0200, Cousson, Benoit wrote:
> >>On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
> >>>
> >>>Add omap3 soc file for handling omap3 soc i2c controllers existing
> >>>on l4-core bus.
> >>>
> >>>Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> >>>---
> >>>  arch/arm/boot/dts/omap3-soc.dtsi |   62 ++++++++++++++++++++++++++++++++++++++
> >>>  1 files changed, 62 insertions(+), 0 deletions(-)
> >>>  create mode 100644 arch/arm/boot/dts/omap3-soc.dtsi
> >>>
> >>>diff --git a/arch/arm/boot/dts/omap3-soc.dtsi b/arch/arm/boot/dts/omap3-soc.dtsi
> >>>new file mode 100644
> >>>index 0000000..85de92f
> >>>--- /dev/null
> >>>+++ b/arch/arm/boot/dts/omap3-soc.dtsi
> >>>@@ -0,0 +1,62 @@
> >>>+/*
> >>>+ * Device Tree Source for OMAP3 SoC
> >>>+ *
> >>>+ * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
> >>>+ *
> >>>+ * This file is licensed under the terms of the GNU General Public License
> >>>+ * version 2.  This program is licensed "as is" without any warranty of any
> >>>+ * kind, whether express or implied.
> >>>+ */
> >>>+
> >>>+/dts-v1/;
> >>>+/include/ "skeleton.dtsi"
> >>>+
> >>>+/ {
> >>>+	#address-cells =<1>;
> >>>+	#size-cells =<1>;
> >>>+	model = "ti,omap3";
> >>>+
> >>>+	aliases {
> >>>+		i2c1 =&i2c1;
> >>>+		i2c2 =&i2c2;
> >>>+		i2c3 =&i2c3;
> >>>+	};
> >>>+
> >>>+	l4-core {
> >>
> >>That comment is probably subject to discussion, but even if this
> >>interconnect is there physically, I'm not sure of the added value to
> >>add it.
> >>It will add an extra level of indentation and that all. Moreover, it
> >>will mess up the physical address that are expressed using absolute
> >>value in the TRM with a less readable offset value.
> >>In fact, most DTS files in the ARM directory are using a purely flat
> >>representation of the interconnect.
> >This is as per alignment with Tony and Grant:
> >http://permalink.gmane.org/gmane.linux.ports.arm.omap/60391
> 
> So I'm asking the same question to Grant and Tony then:-)

What is the conclusion here? I will go ahead with current approach since
tony and grant has voted for it.

-M

> 
> >>>+		compatible = "ti,omap3-l4-core";
> >>
> >>Assuming we will keep that, you should probably add a more generic
> >>compatible name after that one. Like "ti,s3220" or even
> >>"sonics,s3220", assuming that this IP is generic enough for other
> >>SoC.
> >will check this. I don't remember any generic names.
> >>
> >>>+		#address-cells =<1>;
> >>>+		#size-cells =<1>;
> >>>+		ranges =<0 0x48000000 0x1000000>;
> >>>+
> >>>+		i2c1: i2c at 70000 {
> >>>+			#address-cells =<1>;
> >>>+			#size-cells =<0>;
> >>>+			compatible = "ti,omap3-i2c";
> >>
> >>The I2C IP and thus the driver is generic across OMAP generations
> >>and is even potentially used by other non-OMAP TI chips like DSP or
> >>Davinci. So having an extra compatible entry with "ti,i2c" or "ti,
> >>omap-i2c" seems mandatory.
> >This can be updated as and when new soc/board adaptations are done.
> >As of now, it is omap3 and when we have omap4 it will be appended with
> >"ti,omap4-i2c" etc
> 
> To infinity and beyond?
> 
> There is no need, and we should absolutely not update the driver
> each time we introduce a new SoC version/revision.
> The driver should match with the generic device name in that case.
> Until we change the IP and potentially introduce a new driver.
> 
> BTW, even omap3 should not be in the name in theory. It should be
> potentially "ti,i2c-v3" or whatever HW version the IP is using.
> But for some reason, most devices are using such convention in
> existing DTS:-(
> This is probably due to the lack of well identified IP version in
> most SoC... including OMAP:-)
> 
> 
> Benoit

[RFC/PATCH 12/14] dt: omap3: beagle board: set clock freq for i2c devices

[Cousson, Benoit]

On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
>
> Update omap3 beagle dts file with required clock frequencies for the i2c
> client devices existing on beagle board.
>
> Beagle custom board dts file is cleaned up so that it can coexist with omap3
> soc dts file.
>
> Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> ---
>   arch/arm/boot/dts/omap3-beagle-nunchuck.dts |   11 +----------
>   arch/arm/boot/dts/omap3-beagle.dts          |   18 +++++++++++++++---
>   2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> index 2607be5..324ff86 100644
> --- a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> +++ b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> @@ -1,16 +1,7 @@
>   /include/ "omap3-beagle.dts"
>
>   / {
> -	i2c at 48072000 {
> -		compatible = "ti,omap3-i2c";
> -		reg =<0x48072000 0x80>;
> -		#address-cells =<1>;
> -		#size-cells =<0>;
> -
> -		eeprom at 50 {
> -			compatible = "at,at24c01";
> -			reg =<  0x50>;
> -		};

This change should probably not be there.

> +	i2c at 2 {
>   		joystick at 52 {
>   			compatible = "sparkfun,wiichuck";

It looks like someone is having fun with a beagle connected to a Wii 
nunchuck:-)

>   			reg =<  0x52>;
> diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts
> index 4439466..49a5ac7 100644
> --- a/arch/arm/boot/dts/omap3-beagle.dts
> +++ b/arch/arm/boot/dts/omap3-beagle.dts
> @@ -1,7 +1,19 @@
> -/dts-v1/;
> -/include/ "skeleton.dtsi"
> +/include/ "omap3-soc.dtsi"

There is no need for the "-soc" postfix, otherwise all the other SoCs 
inside DTS directory should have it.

Benoit

[RFC/PATCH 12/14] dt: omap3: beagle board: set clock freq for i2c devices

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 02:42:56PM +0200, Cousson, Benoit wrote:
> On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
> >
> >Update omap3 beagle dts file with required clock frequencies for the i2c
> >client devices existing on beagle board.
> >
> >Beagle custom board dts file is cleaned up so that it can coexist with omap3
> >soc dts file.
> >
> >Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> >---
> >  arch/arm/boot/dts/omap3-beagle-nunchuck.dts |   11 +----------
> >  arch/arm/boot/dts/omap3-beagle.dts          |   18 +++++++++++++++---
> >  2 files changed, 16 insertions(+), 13 deletions(-)
> >
> >diff --git a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> >index 2607be5..324ff86 100644
> >--- a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> >+++ b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts
> >@@ -1,16 +1,7 @@
> >  /include/ "omap3-beagle.dts"
> >
> >  / {
> >-	i2c at 48072000 {
> >-		compatible = "ti,omap3-i2c";
> >-		reg =<0x48072000 0x80>;
> >-		#address-cells =<1>;
> >-		#size-cells =<0>;
> >-
> >-		eeprom at 50 {
> >-			compatible = "at,at24c01";
> >-			reg =<  0x50>;
> >-		};
> 
> This change should probably not be there.
> 
> >+	i2c at 2 {
> >  		joystick at 52 {
> >  			compatible = "sparkfun,wiichuck";
> 
> It looks like someone is having fun with a beagle connected to a Wii
> nunchuck:-)
It's custom beagle belong to grant :)

> 
> >  			reg =<  0x52>;
> >diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts
> >index 4439466..49a5ac7 100644
> >--- a/arch/arm/boot/dts/omap3-beagle.dts
> >+++ b/arch/arm/boot/dts/omap3-beagle.dts
> >@@ -1,7 +1,19 @@
> >-/dts-v1/;
> >-/include/ "skeleton.dtsi"
> >+/include/ "omap3-soc.dtsi"
> 
> There is no need for the "-soc" postfix, otherwise all the other
> SoCs inside DTS directory should have it.
ok.

-M

[RFC/PATCH 14/14] dt: omap3: enable dt support for i2c1 controller

[Cousson, Benoit]

On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
>
> Adapt dt for omap i2c1 controller and remove legacy i2c
> initilization in omap3 generic board file.
>
> Tested on omap3 beagle board for dt and non-dt builds.
>
> Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> ---
>   arch/arm/boot/dts/omap3-soc.dtsi     |    6 ++--
>   arch/arm/mach-omap2/board-omap3-dt.c |   14 +++++-----
>   drivers/i2c/busses/i2c-omap.c        |   23 ++++++++++++++++--
>   drivers/of/platform.c                |   41 +++++++++++++++++++++++++++++++++-

It looks like this patch is doing a lot of things. You should probably 
hack the DT core first and then update the driver and the DTS.

>   4 files changed, 70 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/boot/dts/omap3-soc.dtsi b/arch/arm/boot/dts/omap3-soc.dtsi
> index 85de92f..bcff63b 100644
> --- a/arch/arm/boot/dts/omap3-soc.dtsi
> +++ b/arch/arm/boot/dts/omap3-soc.dtsi
> @@ -31,7 +31,7 @@
>   		i2c1: i2c at 70000 {
>   			#address-cells =<1>;
>   			#size-cells =<0>;
> -			compatible = "ti,omap3-i2c";
> +			compatible = "ti,omap3-i2c", "ti,omap3-device";

In that case the compatible is just a tagto identify an OMAP type of 
devices. You should use a more generic "ti,omap-device" entry.
This is similar to the "arm,primecell" used to tag the primecell IPs.

>   			reg =<0x70000 0x100>;
>   			interrupts =<  88>;
>   		};
> @@ -39,7 +39,7 @@
>   		i2c2: i2c at 72000 {
>   			#address-cells =<1>;
>   			#size-cells =<0>;
> -			compatible = "ti,omap3-i2c";
> +			compatible = "ti,omap3-i2c", "ti,omap3-device";
>   			reg =<0x72000 0x100>;
>   			interrupts =<  89>;
>   		};
> @@ -47,7 +47,7 @@
>   		i2c3: i2c at 60000 {
>   			#address-cells =<1>;
>   			#size-cells =<0>;
> -			compatible = "ti,omap3-i2c";
> +			compatible = "ti,omap3-i2c", "ti,omap3-device";
>   			reg =<0x60000 0x100>;
>   			interrupts =<  93>;
>   		};
> diff --git a/arch/arm/mach-omap2/board-omap3-dt.c b/arch/arm/mach-omap2/board-omap3-dt.c
> index 4b76e19..16cf283 100644
> --- a/arch/arm/mach-omap2/board-omap3-dt.c
> +++ b/arch/arm/mach-omap2/board-omap3-dt.c
> @@ -36,11 +36,11 @@ static struct twl4030_platform_data beagle_twldata = {
>   	/* platform_data for children goes here */
>   };
>
> -static int __init omap3_beagle_i2c_init(void)
> -{
> -	omap3_pmic_init("twl4030",&beagle_twldata);
> -	return 0;
> -}
> +struct of_dev_auxdata omap3_auxdata_lookup[] __initdata = {
> +	OF_DEV_AUXDATA_ID_PDSIZE("ti,omap3-i2c", 0x48070000, "i2c1", 1,\
> +				&beagle_twldata, sizeof(beagle_twldata)),
> +	{}
> +};
>
>   static void __init omap3_init_early(void)
>   {
> @@ -70,11 +70,11 @@ static struct of_device_id omap_dt_match_table[] __initdata = {
>
>   static void __init omap3_init(void)
>   {
> -	omap3_beagle_i2c_init();
>   	omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
>   	omap_serial_init();
>
> -	of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
> +	of_platform_populate(NULL, omap_dt_match_table, omap3_auxdata_lookup,
> +									 NULL);
>   }
>
>   static const char *omap3_dt_match[] __initdata = {
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ae1545b..5167737 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -38,6 +38,7 @@
>   #include<linux/clk.h>
>   #include<linux/io.h>
>   #include<linux/of_i2c.h>
> +#include<linux/of_device.h>
>   #include<linux/slab.h>
>   #include<linux/i2c-omap.h>
>   #include<linux/pm_runtime.h>
> @@ -972,6 +973,16 @@ static const struct i2c_algorithm omap_i2c_algo = {
>   	.functionality	= omap_i2c_func,
>   };
>
> +#if defined(CONFIG_OF)
> +static const struct of_device_id omap_i2c_of_match[] = {
> +	{.compatible = "ti,omap3-i2c", },.

This is a generic OMAP driver, so a "ti,omap-i2c" is probably much more 
appropriate. We should always avoid adding OMAP version information into 
a generic driver. Only the IP version should matter for a driver.

> +	{},
> +}
> +MODULE_DEVICE_TABLE(of, omap_i2c_of_match);
> +#else
> +#define omap_i2c_of_match NULL
> +#endif
> +
>   static int __devinit
>   omap_i2c_probe(struct platform_device *pdev)
>   {
> @@ -1008,12 +1019,17 @@ omap_i2c_probe(struct platform_device *pdev)
>   		goto err_release_region;
>   	}
>
> +	speed = 100;	/* Default speed */
>   	if (pdata != NULL) {
>   		speed = pdata->clkrate;
>   		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> -	} else {
> -		speed = 100;	/* Default speed */
> -		dev->set_mpu_wkup_lat = NULL;
> +#if defined(CONFIG_OF)
> +	} else if (pdev->dev.of_node) {
> +		u32 prop;
> +		if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +									&prop))
> +			speed = prop/100;
> +#endif
>   	}
>
>   	dev->speed = speed;
> @@ -1178,6 +1194,7 @@ static struct platform_driver omap_i2c_driver = {
>   		.name	= "omap_i2c",
>   		.owner	= THIS_MODULE,
>   		.pm	= OMAP_I2C_PM_OPS,
> +		.of_match_table = omap_i2c_of_match,
>   	},
>   };
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 4b27286..4d8a2fa 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -24,6 +24,10 @@
>   #include<linux/of_platform.h>
>   #include<linux/platform_device.h>
>
> +#ifdef CONFIG_ARCH_OMAP2PLUS
> +#include<plat/omap_device.h>
> +#endif
> +
>   const struct of_device_id of_default_bus_match_table[] = {
>   	{ .compatible = "simple-bus", },
>   #ifdef CONFIG_ARM_AMBA
> @@ -544,6 +548,36 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
>   	return NULL;
>   }
>
> +static struct omap_device_pm_latency omap_device_latency[] = {
> +	[0] = {
> +		.deactivate_func	= omap_device_idle_hwmods,
> +		.activate_func		= omap_device_enable_hwmods,
> +		.flags			= OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> +	},
> +};
> +
> +int of_omap_device_create(struct device_node *np, const char *name, int id,
> +							void *platform_data,
> +							int pd_size)
> +{
> +	struct omap_hwmod *oh;
> +	struct platform_device *pdev;
> +
> +	oh = omap_hwmod_lookup(name);

This is not enough to handle multiple hwmod entries. Moreover you will 
force the device to be named like the hwmod. This is not necessarily 
bad, but we should be able to have a distinct name if needed.

Benoit

[RFC/PATCH 14/14] dt: omap3: enable dt support for i2c1 controller

[G, Manjunath Kondaiah]

On Wed, Aug 10, 2011 at 02:57:21PM +0200, Cousson, Benoit wrote:
> On 8/9/2011 4:10 PM, G, Manjunath Kondaiah wrote:
> >
> >Adapt dt for omap i2c1 controller and remove legacy i2c
> >initilization in omap3 generic board file.
> >
> >Tested on omap3 beagle board for dt and non-dt builds.
> >
> >Signed-off-by: G, Manjunath Kondaiah<manjugk@ti.com>
> >---
> >  arch/arm/boot/dts/omap3-soc.dtsi     |    6 ++--
> >  arch/arm/mach-omap2/board-omap3-dt.c |   14 +++++-----
> >  drivers/i2c/busses/i2c-omap.c        |   23 ++++++++++++++++--
> >  drivers/of/platform.c                |   41 +++++++++++++++++++++++++++++++++-
> 
> It looks like this patch is doing a lot of things. You should
> probably hack the DT core first and then update the driver and the
> DTS.
> 
> >  4 files changed, 70 insertions(+), 14 deletions(-)
> >
> >diff --git a/arch/arm/boot/dts/omap3-soc.dtsi b/arch/arm/boot/dts/omap3-soc.dtsi
> >index 85de92f..bcff63b 100644
> >--- a/arch/arm/boot/dts/omap3-soc.dtsi
> >+++ b/arch/arm/boot/dts/omap3-soc.dtsi
> >@@ -31,7 +31,7 @@
> >  		i2c1: i2c at 70000 {
> >  			#address-cells =<1>;
> >  			#size-cells =<0>;
> >-			compatible = "ti,omap3-i2c";
> >+			compatible = "ti,omap3-i2c", "ti,omap3-device";
> 
> In that case the compatible is just a tagto identify an OMAP type of
> devices. You should use a more generic "ti,omap-device" entry.
> This is similar to the "arm,primecell" used to tag the primecell IPs.
> 
> >  			reg =<0x70000 0x100>;
> >  			interrupts =<  88>;
> >  		};
> >@@ -39,7 +39,7 @@
> >  		i2c2: i2c at 72000 {
> >  			#address-cells =<1>;
> >  			#size-cells =<0>;
> >-			compatible = "ti,omap3-i2c";
> >+			compatible = "ti,omap3-i2c", "ti,omap3-device";
> >  			reg =<0x72000 0x100>;
> >  			interrupts =<  89>;
> >  		};
> >@@ -47,7 +47,7 @@
> >  		i2c3: i2c at 60000 {
> >  			#address-cells =<1>;
> >  			#size-cells =<0>;
> >-			compatible = "ti,omap3-i2c";
> >+			compatible = "ti,omap3-i2c", "ti,omap3-device";
> >  			reg =<0x60000 0x100>;
> >  			interrupts =<  93>;
> >  		};
> >diff --git a/arch/arm/mach-omap2/board-omap3-dt.c b/arch/arm/mach-omap2/board-omap3-dt.c
> >index 4b76e19..16cf283 100644
> >--- a/arch/arm/mach-omap2/board-omap3-dt.c
> >+++ b/arch/arm/mach-omap2/board-omap3-dt.c
> >@@ -36,11 +36,11 @@ static struct twl4030_platform_data beagle_twldata = {
> >  	/* platform_data for children goes here */
> >  };
> >
> >-static int __init omap3_beagle_i2c_init(void)
> >-{
> >-	omap3_pmic_init("twl4030",&beagle_twldata);
> >-	return 0;
> >-}
> >+struct of_dev_auxdata omap3_auxdata_lookup[] __initdata = {
> >+	OF_DEV_AUXDATA_ID_PDSIZE("ti,omap3-i2c", 0x48070000, "i2c1", 1,\
> >+				&beagle_twldata, sizeof(beagle_twldata)),
> >+	{}
> >+};
> >
> >  static void __init omap3_init_early(void)
> >  {
> >@@ -70,11 +70,11 @@ static struct of_device_id omap_dt_match_table[] __initdata = {
> >
> >  static void __init omap3_init(void)
> >  {
> >-	omap3_beagle_i2c_init();
> >  	omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
> >  	omap_serial_init();
> >
> >-	of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
> >+	of_platform_populate(NULL, omap_dt_match_table, omap3_auxdata_lookup,
> >+									 NULL);
> >  }
> >
> >  static const char *omap3_dt_match[] __initdata = {
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index ae1545b..5167737 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -38,6 +38,7 @@
> >  #include<linux/clk.h>
> >  #include<linux/io.h>
> >  #include<linux/of_i2c.h>
> >+#include<linux/of_device.h>
> >  #include<linux/slab.h>
> >  #include<linux/i2c-omap.h>
> >  #include<linux/pm_runtime.h>
> >@@ -972,6 +973,16 @@ static const struct i2c_algorithm omap_i2c_algo = {
> >  	.functionality	= omap_i2c_func,
> >  };
> >
> >+#if defined(CONFIG_OF)
> >+static const struct of_device_id omap_i2c_of_match[] = {
> >+	{.compatible = "ti,omap3-i2c", },.
> 
> This is a generic OMAP driver, so a "ti,omap-i2c" is probably much
> more appropriate. We should always avoid adding OMAP version
> information into a generic driver. Only the IP version should matter
> for a driver.
> 
> >+	{},
> >+}
> >+MODULE_DEVICE_TABLE(of, omap_i2c_of_match);
> >+#else
> >+#define omap_i2c_of_match NULL
> >+#endif
> >+
> >  static int __devinit
> >  omap_i2c_probe(struct platform_device *pdev)
> >  {
> >@@ -1008,12 +1019,17 @@ omap_i2c_probe(struct platform_device *pdev)
> >  		goto err_release_region;
> >  	}
> >
> >+	speed = 100;	/* Default speed */
> >  	if (pdata != NULL) {
> >  		speed = pdata->clkrate;
> >  		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> >-	} else {
> >-		speed = 100;	/* Default speed */
> >-		dev->set_mpu_wkup_lat = NULL;
> >+#if defined(CONFIG_OF)
> >+	} else if (pdev->dev.of_node) {
> >+		u32 prop;
> >+		if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> >+									&prop))
> >+			speed = prop/100;
> >+#endif
> >  	}
> >
> >  	dev->speed = speed;
> >@@ -1178,6 +1194,7 @@ static struct platform_driver omap_i2c_driver = {
> >  		.name	= "omap_i2c",
> >  		.owner	= THIS_MODULE,
> >  		.pm	= OMAP_I2C_PM_OPS,
> >+		.of_match_table = omap_i2c_of_match,
> >  	},
> >  };
> >
> >diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >index 4b27286..4d8a2fa 100644
> >--- a/drivers/of/platform.c
> >+++ b/drivers/of/platform.c
> >@@ -24,6 +24,10 @@
> >  #include<linux/of_platform.h>
> >  #include<linux/platform_device.h>
> >
> >+#ifdef CONFIG_ARCH_OMAP2PLUS
> >+#include<plat/omap_device.h>
> >+#endif
> >+
> >  const struct of_device_id of_default_bus_match_table[] = {
> >  	{ .compatible = "simple-bus", },
> >  #ifdef CONFIG_ARM_AMBA
> >@@ -544,6 +548,36 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
> >  	return NULL;
> >  }
> >
> >+static struct omap_device_pm_latency omap_device_latency[] = {
> >+	[0] = {
> >+		.deactivate_func	= omap_device_idle_hwmods,
> >+		.activate_func		= omap_device_enable_hwmods,
> >+		.flags			= OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> >+	},
> >+};
> >+
> >+int of_omap_device_create(struct device_node *np, const char *name, int id,
> >+							void *platform_data,
> >+							int pd_size)
> >+{
> >+	struct omap_hwmod *oh;
> >+	struct platform_device *pdev;
> >+
> >+	oh = omap_hwmod_lookup(name);
> 
> This is not enough to handle multiple hwmod entries. Moreover you
> will force the device to be named like the hwmod. This is not
> necessarily bad, but we should be able to have a distinct name if
> needed.

I agree it supports only single hwmod entries. As of now, only mcbsp uses
multiple hwmod entries. With dt enabled, most of the drivers can use single
hwmod entry for omap device creation and we can upgrade this api for handling
mutiple hwmod entries.

#>git grep omap_device_build_ss
arch/arm/mach-omap2/devices.c:  pdev = omap_device_build_ss(NULL, "omap_l3_noc",
0, oh, 3, NULL,
arch/arm/mach-omap2/mcbsp.c:    pdev = omap_device_build_ss(NULL, name, id,
oh_device, count, pdata,
arch/arm/plat-omap/include/plat/omap_device.h:struct platform_device
*omap_device_build_ss(struct device_node *np,
arch/arm/plat-omap/omap_device.c: * The function is called from inside
omap_device_build_ss(), after
arch/arm/plat-omap/omap_device.c: * omap_device @od.  Used by
omap_device_build_ss() to determine how
arch/arm/plat-omap/omap_device.c: * omap_device_build_ss() after calling
omap_device_count_resources().
arch/arm/plat-omap/omap_device.c: * platform_device record.  See
omap_device_build_ss() for more
arch/arm/plat-omap/omap_device.c: * passes along the return value of
omap_device_build_ss().
arch/arm/plat-omap/omap_device.c:       return omap_device_build_ss(np,
pdev_name, pdev_id, ohs, 1, pdata,
arch/arm/plat-omap/omap_device.c: * platform_device record.  See
omap_device_build_ss() for more
arch/arm/plat-omap/omap_device.c: * passes along the return value of
omap_device_build_ss().
arch/arm/plat-omap/omap_device.c:       return omap_device_build_ss(NULL,
pdev_name, pdev_id, ohs, 1, pdata,
arch/arm/plat-omap/omap_device.c: * omap_device_build_ss - build and register an
omap_device with multiple hwmods
arch/arm/plat-omap/omap_device.c:struct platform_device
*omap_device_build_ss(struct device_node *np,
#>

-M