[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

We register the ab8500 as an MFD device from db8500 code during Device Tree
boot in order to solve some limitations of DT. However, when Device Tree is
not enabled, we still want to allow platform code to register the ab8500 in
the normal way. Here we force MFD device registration of the ab8500 only
when booting with Device Tree enabled.

Solves this issue:
WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0x88/0xb0()
sysfs: cannot create duplicate filename '/bus/platform/devices/ab8500-core.0'

Reported-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/db8500-prcmu.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 80def6c..4ec0ed1 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -2964,6 +2964,9 @@ static struct mfd_cell db8500_prcmu_devs[] = {
 		.name = "cpufreq-u8500",
 		.of_compatible = "stericsson,cpufreq-u8500",
 	},
+};
+
+static struct mfd_cell db8500_of_prcmu_devs[] = {
 	{
 		.name = "ab8500-core",
 		.of_compatible = "stericsson,ab8500",
@@ -3014,6 +3017,15 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	if (np) {
+		err = mfd_add_devices(&pdev->dev, 0, db8500_of_prcmu_devs,
+				ARRAY_SIZE(db8500_of_prcmu_devs), NULL, 0);
+		if (err) {
+			pr_err("prcmu: Failed to add DT subdevices\n");
+			return err;
+		}
+	}
+
 	pr_info("DB8500 PRCMU initialized\n");
 
 no_irq_return:
-- 
1.7.9.5

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Tue, Jul 03, 2012 at 12:59:48PM +0100, Lee Jones wrote:
> We register the ab8500 as an MFD device from db8500 code during Device Tree
> boot in order to solve some limitations of DT. However, when Device Tree is
> not enabled, we still want to allow platform code to register the ab8500 in
> the normal way. Here we force MFD device registration of the ab8500 only
> when booting with Device Tree enabled.
> 
> Solves this issue:
> WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0x88/0xb0()
> sysfs: cannot create duplicate filename '/bus/platform/devices/ab8500-core.0'

Do we really want to create MFD cells like this (which are really Linux
internal things and might vary if another OS or another version of Linux
changes its internal abstractions) from the device tree?

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 03/07/12 13:35, Mark Brown wrote:
> On Tue, Jul 03, 2012 at 12:59:48PM +0100, Lee Jones wrote:
>> We register the ab8500 as an MFD device from db8500 code during Device Tree
>> boot in order to solve some limitations of DT. However, when Device Tree is
>> not enabled, we still want to allow platform code to register the ab8500 in
>> the normal way. Here we force MFD device registration of the ab8500 only
>> when booting with Device Tree enabled.
>>
>> Solves this issue:
>> WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0x88/0xb0()
>> sysfs: cannot create duplicate filename '/bus/platform/devices/ab8500-core.0'
>
> Do we really want to create MFD cells like this (which are really Linux
> internal things and might vary if another OS or another version of Linux
> changes its internal abstractions) from the device tree?

We're not creating them. We're merely using current infrastructure.

Before, when we probed each device from Device Tree we came up against 
some fairly major limitations of the Device Tree. As a result, Arnd and 
I agreed that this was the way to go.

See c5395e7ed8f16cc7bb72a783de68659db5aed515 for a short description of 
the troubles we faced.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Tue, Jul 03, 2012 at 02:07:45PM +0100, Lee Jones wrote:
> On 03/07/12 13:35, Mark Brown wrote:

> >Do we really want to create MFD cells like this (which are really Linux
> >internal things and might vary if another OS or another version of Linux
> >changes its internal abstractions) from the device tree?

> We're not creating them. We're merely using current infrastructure.

*Very* recently added infrastructure which caused you to notice this...

> Before, when we probed each device from Device Tree we came up
> against some fairly major limitations of the Device Tree. As a
> result, Arnd and I agreed that this was the way to go.

I'm really unconvinced that instnatiating the MFD cells from device tree
is in general a good idea.

> See c5395e7ed8f16cc7bb72a783de68659db5aed515 for a short description
> of the troubles we faced.

$ git show c5395e7ed8f16cc7bb72a783de68659db5aed515
fatal: bad object c5395e7ed8f16cc7bb72a783de68659db5aed515
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120703/8cfced81/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 03/07/12 14:24, Mark Brown wrote:
> On Tue, Jul 03, 2012 at 02:07:45PM +0100, Lee Jones wrote:
>> On 03/07/12 13:35, Mark Brown wrote:
>
>>> Do we really want to create MFD cells like this (which are really Linux
>>> internal things and might vary if another OS or another version of Linux
>>> changes its internal abstractions) from the device tree?
>
>> We're not creating them. We're merely using current infrastructure.
>
> *Very* recently added infrastructure which caused you to notice this...

No, I mean the MFD method is current infrastructure.

Using it with DT is new, yes.

>> Before, when we probed each device from Device Tree we came up
>> against some fairly major limitations of the Device Tree. As a
>> result, Arnd and I agreed that this was the way to go.
>
> I'm really unconvinced that instnatiating the MFD cells from device tree
> is in general a good idea.

Well it just doesn't work the other way.

>> See c5395e7ed8f16cc7bb72a783de68659db5aed515 for a short description
>> of the troubles we faced.
>
> $ git show c5395e7ed8f16cc7bb72a783de68659db5aed515
> fatal: bad object c5395e7ed8f16cc7bb72a783de68659db5aed515

Sorry, looks live I've rebased since.

5f3fc8adeec9bb12742fbfa777fa1947deda21a2

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Tue, Jul 03, 2012 at 02:48:29PM +0100, Lee Jones wrote:
> On 03/07/12 14:24, Mark Brown wrote:

> >I'm really unconvinced that instnatiating the MFD cells from device tree
> >is in general a good idea.

> Well it just doesn't work the other way.

In what way does it not work?

> >>See c5395e7ed8f16cc7bb72a783de68659db5aed515 for a short description
> >>of the troubles we faced.

> >$ git show c5395e7ed8f16cc7bb72a783de68659db5aed515
> >fatal: bad object c5395e7ed8f16cc7bb72a783de68659db5aed515

> Sorry, looks live I've rebased since.

> 5f3fc8adeec9bb12742fbfa777fa1947deda21a2

This doesn't explain any of the issues, it just says that they exist.
My best guess would be that at least some of the issue is due to
instantiating the MFD cells from the device tree but it's hard to say
clearly.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120703/feaa42d3/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 03/07/12 15:21, Mark Brown wrote:
> On Tue, Jul 03, 2012 at 02:48:29PM +0100, Lee Jones wrote:
>> On 03/07/12 14:24, Mark Brown wrote:
>
>>> I'm really unconvinced that instnatiating the MFD cells from device tree
>>> is in general a good idea.
>
>> Well it just doesn't work the other way.
>
> In what way does it not work?
>
>>>> See c5395e7ed8f16cc7bb72a783de68659db5aed515 for a short description
>>>> of the troubles we faced.
>
>>> $ git show c5395e7ed8f16cc7bb72a783de68659db5aed515
>>> fatal: bad object c5395e7ed8f16cc7bb72a783de68659db5aed515
>
>> Sorry, looks live I've rebased since.
>
>> 5f3fc8adeec9bb12742fbfa777fa1947deda21a2
>
> This doesn't explain any of the issues, it just says that they exist.
> My best guess would be that at least some of the issue is due to
> instantiating the MFD cells from the device tree but it's hard to say
> clearly.

I'm guessing Arnd's email answered some of the questions you had. Let me 
know of you would like me to explain it in any greater detail.

By the way, this patch has nothing to do with registering these devices 
when DT is enabled. The code already does that. This is a bug fix, to 
stop multiple registration of the ab8500 when DT is _not_ enabled.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 08:36:38AM +0100, Lee Jones wrote:
> On 03/07/12 15:21, Mark Brown wrote:

> >This doesn't explain any of the issues, it just says that they exist.
> >My best guess would be that at least some of the issue is due to
> >instantiating the MFD cells from the device tree but it's hard to say
> >clearly.

> I'm guessing Arnd's email answered some of the questions you had.
> Let me know of you would like me to explain it in any greater
> detail.

No, frankly.  It was just a general "why might we put things in DT"
answer which (especially given what you say below) isn't related to the
issue at all.

> By the way, this patch has nothing to do with registering these
> devices when DT is enabled. The code already does that. This is a
> bug fix, to stop multiple registration of the ab8500 when DT is
> _not_ enabled.

Really?  It seems really surprising that adding more DT support to the
MFD core would have any bearing on something like this...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/1c4ded73/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 10:45, Mark Brown wrote:
> On Thu, Jul 05, 2012 at 08:36:38AM +0100, Lee Jones wrote:
>> On 03/07/12 15:21, Mark Brown wrote:
> 
>>> This doesn't explain any of the issues, it just says that they exist.
>>> My best guess would be that at least some of the issue is due to
>>> instantiating the MFD cells from the device tree but it's hard to say
>>> clearly.
> 
>> I'm guessing Arnd's email answered some of the questions you had.
>> Let me know of you would like me to explain it in any greater
>> detail.
> 
> No, frankly.  It was just a general "why might we put things in DT"
> answer which (especially given what you say below) isn't related to the
> issue at all.

Okay, so currently we have something like this:

/ {
	soc-u9500 {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

/* 
 * The nodes below which have addresses associated with 
 * them all have correctly formed reg properties: 
 *   i.e. "reg = <0xa0411000 0x1000>" 
 */
		intc: interrupt-controller at a0411000
		L2: l2-cache
		pmu
		timer at a0410600
		rtc at 80154000
		gpio0: gpio at 8012e000
		pinctrl
		usb at a03e0000
		dma-controller at 801C0000

/* 
 * Then it becomes more interesting. We have the PRCMU
 * which has the same address space as above, but its
 * children have mixed address spaces. Some have their
 * own set of memory mapped registers, others are
 * communicated with by i2c. So:
 */

		prcmu at 80157000 {
			reg = <0x80157000 0x1000>;
			#address-cells = <1>;
			#size-cells = <1>;
			ranges;

/* The timer has its own memory mapped address space. */

			prcmu-timer-4 at 80157450

/* Ignore the regulators, no one really cares about those ;) */

			db8500-prcmu-regulators

/* 
 * Then the ab8500 communicates with the PRCMU via a selection
 * of i2c mailboxes. So we did have this:
 *         mailbox {
 *                 #address-cells = <1>;
 *                 #size-cells = <0>;
 *
 *                 ab8500 { <blah> };
 *        }
 * But then Device Tree complains at you because of this:

drivers/of/address.c:
> #define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && (ns) > 0)
> 
> 	/* Cound address cells & copy address locally */
> 	bus->count_cells(dev, &na, &ns);
> 	if (!OF_CHECK_COUNTS(na, ns)) {
> 		printk(KERN_ERR "prom_parse: Bad cell count for %s\n",
> 		       dev->full_name);
> 		goto bail;
> 	}

 * Device Tree doesn't allow you to have zero size cells, 
 * which we would require if we were to register all of the
 * AB8500 devices separately during a DT boot.
 */


			ab8500 at 5 {
				reg = <5>; /* mailbox 5 is i2c */

				ab8500-rtc
				ab8500-gpadc
				ab8500-usb
				        reg = <1>;
				ab8500-ponkey
				ab8500-sysctrl
				ab8500-pwm
				ab8500-debugfs
				ab8500-regulators
			};
		};

		i2c at 80004000
		ssp at 80002000
		uart at 80120000
		sdi at 80126000
		external-bus at 50000000
	};
};

Besides, the main role of Device Tree is to eradicate platform code.
Whereas the code in the MFD driver used to register the AB8500 devices
is not platform code.

Does that answer your question better?

>> By the way, this patch has nothing to do with registering these
>> devices when DT is enabled. The code already does that. This is a
>> bug fix, to stop multiple registration of the ab8500 when DT is
>> _not_ enabled.
> 
> Really?  It seems really surprising that adding more DT support to the
> MFD core would have any bearing on something like this...

I'm not really sure what you mean by this.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515 
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 12:46:49PM +0100, Lee Jones wrote:

> Besides, the main role of Device Tree is to eradicate platform code.
> Whereas the code in the MFD driver used to register the AB8500 devices
> is not platform code.

Right, this is a big part of what I'm saying.

> Does that answer your question better?

Not really, no.  

> >> By the way, this patch has nothing to do with registering these
> >> devices when DT is enabled. The code already does that. This is a
> >> bug fix, to stop multiple registration of the ab8500 when DT is
> >> _not_ enabled.

> > Really?  It seems really surprising that adding more DT support to the
> > MFD core would have any bearing on something like this...

> I'm not really sure what you mean by this.

You seemed to be suggesting that your fix was in some way related to the
DT changes in the MFD core.  I'm unsure as to the relationship here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/bef6b759/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 13:06, Mark Brown wrote:
> On Thu, Jul 05, 2012 at 12:46:49PM +0100, Lee Jones wrote:
>
>> Besides, the main role of Device Tree is to eradicate platform code.
>> Whereas the code in the MFD driver used to register the AB8500 devices
>> is not platform code.
>
> Right, this is a big part of what I'm saying.
>
>> Does that answer your question better?
>
> Not really, no.

Then you're not asking the right question. :)

>>>> By the way, this patch has nothing to do with registering these
>>>> devices when DT is enabled. The code already does that. This is a
>>>> bug fix, to stop multiple registration of the ab8500 when DT is
>>>> _not_ enabled.
>
>>> Really?  It seems really surprising that adding more DT support to the
>>> MFD core would have any bearing on something like this...
>
>> I'm not really sure what you mean by this.
>
> You seemed to be suggesting that your fix was in some way related to the
> DT changes in the MFD core.  I'm unsure as to the relationship here.

How is it not related? In English the patch would say; "Only register 
the AB8500 via the MFD API when we're booting with Device Tree. This 
allows AB8500 related devices to be registered in the normal way, rather 
than registering them individually using DT and prevents duplicate 
registration when we are not executing a Device Tree enabled boot."


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 01:15:45PM +0100, Lee Jones wrote:
> On 05/07/12 13:06, Mark Brown wrote:

> >You seemed to be suggesting that your fix was in some way related to the
> >DT changes in the MFD core.  I'm unsure as to the relationship here.

> How is it not related? In English the patch would say; "Only
> register the AB8500 via the MFD API when we're booting with Device
> Tree. This allows AB8500 related devices to be registered in the
> normal way, rather than registering them individually using DT and
> prevents duplicate registration when we are not executing a Device
> Tree enabled boot."

This is what you said before and it still doesn't make much sense to me.
I'd expect that if anything your first statement would be the opposite
of what happens - it seems like your non-DT code is doing something
really odd.  If anything I'd expect adding a DT to add duplicate
registrations, I'd not expect it to remove registrations.  

What I'd expect is that if we can figure out that we need to register
the AB8500 automatically without any information from DT then we should
be able to figure out exactly the same thing in the non-DT case.  I
would therefore expect that the change would instead be something which
removes the other source of registrations.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/a775c07d/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 13:29, Mark Brown wrote:
> On Thu, Jul 05, 2012 at 01:15:45PM +0100, Lee Jones wrote:
>> On 05/07/12 13:06, Mark Brown wrote:
>
>>> You seemed to be suggesting that your fix was in some way related to the
>>> DT changes in the MFD core.  I'm unsure as to the relationship here.
>
>> How is it not related? In English the patch would say; "Only
>> register the AB8500 via the MFD API when we're booting with Device
>> Tree. This allows AB8500 related devices to be registered in the
>> normal way, rather than registering them individually using DT and
>> prevents duplicate registration when we are not executing a Device
>> Tree enabled boot."
>
> This is what you said before and it still doesn't make much sense to me.
> I'd expect that if anything your first statement would be the opposite
> of what happens - it seems like your non-DT code is doing something
> really odd.  If anything I'd expect adding a DT to add duplicate
> registrations, I'd not expect it to remove registrations.
>
> What I'd expect is that if we can figure out that we need to register
> the AB8500 automatically without any information from DT then we should
> be able to figure out exactly the same thing in the non-DT case.  I
> would therefore expect that the change would instead be something which
> removes the other source of registrations.

Now you're confusing me. :)

If DT is _not_ enabled, we do:

   From platform code:
    - Register the DB8500-PRCMU
    - Register the AB8500

So you see the registration is separate.

If DT _is_ enabled, we do:

   From Device Tree:
    - Register the DB8500-PRCMU (which in turn registers the AB8500)

In this case we the DB8500-PRCMU goes on to register the AB8500 for us, 
so we need to ensure DT _is_ running before we go on to do that, because 
if we don't the DB8500-PRCMU will register it and so will platform code.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 01:41:12PM +0100, Lee Jones wrote:
> On 05/07/12 13:29, Mark Brown wrote:

> If DT is _not_ enabled, we do:

>   From platform code:
>    - Register the DB8500-PRCMU
>    - Register the AB8500

> So you see the registration is separate.

Right, so what I'm saying is that what I'd expect unless there's
something unusual going on is that you wouldn't be doing the separate
registration of the AB8500 here and would instead be passing the
platform data for the AB8500 through in the same way you pass the DT
data through.

DT and non-DT do have a very similar model for this stuff.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/3cfff40b/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 13:45, Mark Brown wrote:
> On Thu, Jul 05, 2012 at 01:41:12PM +0100, Lee Jones wrote:
>> On 05/07/12 13:29, Mark Brown wrote:
>
>> If DT is _not_ enabled, we do:
>
>>    From platform code:
>>     - Register the DB8500-PRCMU
>>     - Register the AB8500
>
>> So you see the registration is separate.
>
> Right, so what I'm saying is that what I'd expect unless there's
> something unusual going on is that you wouldn't be doing the separate
> registration of the AB8500 here and would instead be passing the
> platform data for the AB8500 through in the same way you pass the DT
> data through.

Then were would you register it, if not here?

> DT and non-DT do have a very similar model for this stuff.


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 01:55:50PM +0100, Lee Jones wrote:
> On 05/07/12 13:45, Mark Brown wrote:

> >Right, so what I'm saying is that what I'd expect unless there's
> >something unusual going on is that you wouldn't be doing the separate
> >registration of the AB8500 here and would instead be passing the
> >platform data for the AB8500 through in the same way you pass the DT
> >data through.

> Then were would you register it, if not here?

Same place as for DT.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/f66959b6/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 14:03, Mark Brown wrote:
> On Thu, Jul 05, 2012 at 01:55:50PM +0100, Lee Jones wrote:
>> On 05/07/12 13:45, Mark Brown wrote:
>
>>> Right, so what I'm saying is that what I'd expect unless there's
>>> something unusual going on is that you wouldn't be doing the separate
>>> registration of the AB8500 here and would instead be passing the
>>> platform data for the AB8500 through in the same way you pass the DT
>>> data through.
>
>> Then were would you register it, if not here?
>
> Same place as for DT.

That is a possibility, but the idea is to reduce code in the platform 
area, not add to it. We'd also need a completely separate platform_data 
structure to the one we use for platform registration, as much of it has 
now been moved into Device Tree. The regulators are a good example of 
this, but there's also GPIO information which is no longer relevant etc.

I do believe that registering the AB8500 from the DB8500 is appropriate 
though, for the simple reason that the AB8500 is a sub-device to the 
DB8500. I think this is the correct thing to do. But anyway, as I said 
before, that ship has sailed. We _already_ do this. All this patch does 
is prevent the AB8500 from being registered twice when DT is not enabled.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 02:12:09PM +0100, Lee Jones wrote:
> On 05/07/12 14:03, Mark Brown wrote:
> >On Thu, Jul 05, 2012 at 01:55:50PM +0100, Lee Jones wrote:

> >>Then were would you register it, if not here?

> >Same place as for DT.

> That is a possibility, but the idea is to reduce code in the
> platform area, not add to it. We'd also need a completely separate

But surely this would, if anything, remove code?  You already have the
code to do the registration in the MFD so all you're going to be doing
here is removing the code from 

> platform_data structure to the one we use for platform registration,
> as much of it has now been moved into Device Tree. The regulators
> are a good example of this, but there's also GPIO information which
> is no longer relevant etc.

Hrm, the usual pattern for this stuff is that the DT is parsed into
platform data so the DT code is isolated to the parser.  It sounds like
you've got a very different structure here?

> I do believe that registering the AB8500 from the DB8500 is
> appropriate though, for the simple reason that the AB8500 is a
> sub-device to the DB8500. I think this is the correct thing to do.
> But anyway, as I said before, that ship has sailed. We _already_ do
> this. All this patch does is prevent the AB8500 from being
> registered twice when DT is not enabled.

Well, it also introduces code into mainline which is likely to be used
as a template by other people - I'd be especially worried about the next
ST platform ending up repeating the same mistakes.  If the code is so
separate perhaps it's better to just remove the non-DT support?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/9489904b/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 14:20, Mark Brown wrote:
> On Thu, Jul 05, 2012 at 02:12:09PM +0100, Lee Jones wrote:
>> On 05/07/12 14:03, Mark Brown wrote:
>>> On Thu, Jul 05, 2012 at 01:55:50PM +0100, Lee Jones wrote:
>
>>>> Then were would you register it, if not here?
>
>>> Same place as for DT.
>
>> That is a possibility, but the idea is to reduce code in the
>> platform area, not add to it. We'd also need a completely separate
>
> But surely this would, if anything, remove code?  You already have the
> code to do the registration in the MFD so all you're going to be doing
> here is removing the code from

No, it will add platform code if we were to register the ab8500 from the 
platform area.

>> platform_data structure to the one we use for platform registration,
>> as much of it has now been moved into Device Tree. The regulators
>> are a good example of this, but there's also GPIO information which
>> is no longer relevant etc.
>
> Hrm, the usual pattern for this stuff is that the DT is parsed into
> platform data so the DT code is isolated to the parser.  It sounds like
> you've got a very different structure here?

Yes we do. Ref that commit ID I sent you you a few days ago:

5f3fc8adeec9bb12742fbfa777fa1947deda21a2

>> I do believe that registering the AB8500 from the DB8500 is
>> appropriate though, for the simple reason that the AB8500 is a
>> sub-device to the DB8500. I think this is the correct thing to do.
>> But anyway, as I said before, that ship has sailed. We _already_ do
>> this. All this patch does is prevent the AB8500 from being
>> registered twice when DT is not enabled.
>
> Well, it also introduces code into mainline which is likely to be used
> as a template by other people - I'd be especially worried about the next
> ST platform ending up repeating the same mistakes.

There are no mistakes. It would work for other platforms. :)

> If the code is so
> separate perhaps it's better to just remove the non-DT support?

That's the plan.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 02:54:04PM +0100, Lee Jones wrote:
> On 05/07/12 14:20, Mark Brown wrote:

[Moving registration to the MFD]
> >But surely this would, if anything, remove code?  You already have the
> >code to do the registration in the MFD so all you're going to be doing
> >here is removing the code from

> No, it will add platform code if we were to register the ab8500 from
> the platform area.

Why would this be the case?  You've already got registration code in
there for use in DT...

> >Hrm, the usual pattern for this stuff is that the DT is parsed into
> >platform data so the DT code is isolated to the parser.  It sounds like
> >you've got a very different structure here?

> Yes we do. Ref that commit ID I sent you you a few days ago:

> 5f3fc8adeec9bb12742fbfa777fa1947deda21a2

This doesn't appear to reference the platform data?

> >Well, it also introduces code into mainline which is likely to be used
> >as a template by other people - I'd be especially worried about the next
> >ST platform ending up repeating the same mistakes.

> There are no mistakes. It would work for other platforms. :)

Working and good idea aren't the same thing!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/cb0022f5/attachment-0001.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Samuel Ortiz]

Hi Lee,

On Thu, Jul 05, 2012 at 02:12:09PM +0100, Lee Jones wrote:
> On 05/07/12 14:03, Mark Brown wrote:
> >On Thu, Jul 05, 2012 at 01:55:50PM +0100, Lee Jones wrote:
> >>On 05/07/12 13:45, Mark Brown wrote:
> >
> >>>Right, so what I'm saying is that what I'd expect unless there's
> >>>something unusual going on is that you wouldn't be doing the separate
> >>>registration of the AB8500 here and would instead be passing the
> >>>platform data for the AB8500 through in the same way you pass the DT
> >>>data through.
> >
> >>Then were would you register it, if not here?
> >
> >Same place as for DT.
> 
> That is a possibility, but the idea is to reduce code in the
> platform area, not add to it. We'd also need a completely separate
> platform_data structure to the one we use for platform registration,
> as much of it has now been moved into Device Tree. The regulators
> are a good example of this, but there's also GPIO information which
> is no longer relevant etc.
> 
> I do believe that registering the AB8500 from the DB8500 is
> appropriate though, for the simple reason that the AB8500 is a
> sub-device to the DB8500. I think this is the correct thing to do.
I agree with you here, and I thus see no reasons why we should have a
different code path for DT and !DT cases.


> But anyway, as I said before, that ship has sailed. We _already_ do
> this. All this patch does is prevent the AB8500 from being
> registered twice when DT is not enabled.
Sure, and it does exactly that. But this does look like a workaround rather
than an actual fix. And Mark is right about showing the wrong example with
this kind of patches for other MFD drivers.
I'd prefer seeig the !DT support removed for now until you can find a proper
and common solution for both DT and !DT cases.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Arnd Bergmann]

On Thursday 05 July 2012, Mark Brown wrote:
> On Thu, Jul 05, 2012 at 01:41:12PM +0100, Lee Jones wrote:
> > On 05/07/12 13:29, Mark Brown wrote:
> 
> > If DT is not enabled, we do:
> 
> >   From platform code:
> >    - Register the DB8500-PRCMU
> >    - Register the AB8500
> 
> > So you see the registration is separate.
> 
> Right, so what I'm saying is that what I'd expect unless there's
> something unusual going on is that you wouldn't be doing the separate
> registration of the AB8500 here and would instead be passing the
> platform data for the AB8500 through in the same way you pass the DT
> data through.
> 
> DT and non-DT do have a very similar model for this stuff.

The non-DT path for this is a huge mess, I'd rather focus on making
it obsolete than trying to fix it. Other than that, I agree that
we should be registering the ab8500 from the prcmu from both the
DT and the non-DT case.

Right now, for non-DT, we register ab8500 as a platform device
with board specific platform_data from arch/arm/mach-ux500/board-mop500.c
and the device just accesses the prcmu driver through its exported
functions.

Making it registered through the prcmu sounds like the right thing
to do, but it requires funneling the board specific ab8500 platform
data through to the prcmu device registration, something like the
patch below, which is not really making things nicer overall.

	Arnd

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 1509a3c..f8fae8c 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -197,24 +197,6 @@ static struct ab8500_platform_data ab8500_platdata = {
 	.gpio		= &ab8500_gpio_pdata,
 };
 
-static struct resource ab8500_resources[] = {
-	[0] = {
-		.start	= IRQ_DB8500_AB8500,
-		.end	= IRQ_DB8500_AB8500,
-		.flags	= IORESOURCE_IRQ
-	}
-};
-
-struct platform_device ab8500_device = {
-	.name = "ab8500-core",
-	.id = 0,
-	.dev = {
-		.platform_data = &ab8500_platdata,
-	},
-	.num_resources = 1,
-	.resource = ab8500_resources,
-};
-
 /*
  * TPS61052
  */
@@ -460,7 +442,6 @@ static struct hash_platform_data u8500_hash1_platform_data = {
 /* add any platform devices here - TODO */
 static struct platform_device *mop500_platform_devs[] __initdata = {
 	&mop500_gpio_keys_device,
-	&ab8500_device,
 };
 
 #ifdef CONFIG_STE_DMA40
@@ -622,7 +603,6 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
 	&snowball_led_dev,
 	&snowball_key_dev,
 	&snowball_sbnet_dev,
-	&ab8500_device,
 };
 
 static struct platform_device *snowball_of_platform_devs[] __initdata = {
@@ -639,9 +619,8 @@ static void __init mop500_init_machine(void)
 	mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
 
 	mop500_pinmaps_init();
-	parent = u8500_init_devices();
+	parent = u8500_init_devices(&ab8500_platform_data);
 
-	/* FIXME: parent of ab8500 should be prcmu */
 	for (i = 0; i < ARRAY_SIZE(mop500_platform_devs); i++)
 		mop500_platform_devs[i]->dev.parent = parent;
 
@@ -674,7 +653,8 @@ static void __init snowball_init_machine(void)
 	int i;
 
 	snowball_pinmaps_init();
-	parent = u8500_init_devices();
+
+	parent = u8500_init_devices(&ab8500_platform_data);
 
 	for (i = 0; i < ARRAY_SIZE(snowball_platform_devs); i++)
 		snowball_platform_devs[i]->dev.parent = parent;
@@ -706,7 +686,7 @@ static void __init hrefv60_init_machine(void)
 	mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
 
 	hrefv60_pinmaps_init();
-	parent = u8500_init_devices();
+	parent = u8500_init_devices(&ab8500_platform_data);
 
 	for (i = 0; i < ARRAY_SIZE(mop500_platform_devs); i++)
 		mop500_platform_devs[i]->dev.parent = parent;
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 33275eb..6cc247c 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -207,7 +207,7 @@ static struct device * __init db8500_soc_device_init(void)
 /*
  * This function is called from the board init
  */
-struct device * __init u8500_init_devices(void)
+struct device * __init u8500_init_devices(struct ab8500_platform_data *ab8500)
 {
 	struct device *parent;
 	int i;
@@ -224,6 +224,8 @@ struct device * __init u8500_init_devices(void)
 	for (i = 0; i < ARRAY_SIZE(platform_devs); i++)
 		platform_devs[i]->dev.parent = parent;
 
+	db8500_prcmu_device.dev.platform_data = ab8500;
+
 	platform_add_devices(platform_devs, ARRAY_SIZE(platform_devs));
 
 	return parent;
diff --git a/arch/arm/mach-ux500/include/mach/setup.h b/arch/arm/mach-ux500/include/mach/setup.h
index 8b7ed82..7940615 100644
--- a/arch/arm/mach-ux500/include/mach/setup.h
+++ b/arch/arm/mach-ux500/include/mach/setup.h
@@ -17,7 +17,7 @@
 void __init ux500_map_io(void);
 extern void __init u8500_map_io(void);
 
-extern struct device * __init u8500_init_devices(void);
+extern struct device * __init u8500_init_devices(struct ab8500_platform_data *ab8500);
 
 extern void __init ux500_init_irq(void);
 extern void __init ux500_init_late(void);
diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 50e83dc5..fc0bd4e 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -2987,6 +2987,8 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
 		goto no_irq_return;
 	}
 
+	db8500_prcmu_devs[AB8500].platform_data = pdev->dev.platform_data;
+
 	if (cpu_is_u8500v20_or_later())
 		prcmu_config_esram0_deep_sleep(ESRAM0_DEEP_SLEEP_STATE_RET);
 

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 01:57:06PM +0000, Arnd Bergmann wrote:

> The non-DT path for this is a huge mess, I'd rather focus on making
> it obsolete than trying to fix it. Other than that, I agree that
> we should be registering the ab8500 from the prcmu from both the
> DT and the non-DT case.

If that's the decision then we should probably have a comment in the
code saying that this is what's going on to warn off anyone who's
inclined to cut'n'paste.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/8f8a8547/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 14:57, Arnd Bergmann wrote:
> On Thursday 05 July 2012, Mark Brown wrote:
>> On Thu, Jul 05, 2012 at 01:41:12PM +0100, Lee Jones wrote:
>>> On 05/07/12 13:29, Mark Brown wrote:
>>
>>> If DT is not enabled, we do:
>>
>>>    From platform code:
>>>     - Register the DB8500-PRCMU
>>>     - Register the AB8500
>>
>>> So you see the registration is separate.
>>
>> Right, so what I'm saying is that what I'd expect unless there's
>> something unusual going on is that you wouldn't be doing the separate
>> registration of the AB8500 here and would instead be passing the
>> platform data for the AB8500 through in the same way you pass the DT
>> data through.
>>
>> DT and non-DT do have a very similar model for this stuff.
>
> The non-DT path for this is a huge mess, I'd rather focus on making
> it obsolete than trying to fix it. Other than that, I agree that
> we should be registering the ab8500 from the prcmu from both the
> DT and the non-DT case.

Ah, is that what you were saying Mark?

If so, I apologise. I thought you meant register both from platform 
code. I'm happy to register the AB8500 from the DB8500 for _both_ DT and 
!DT.

> Right now, for non-DT, we register ab8500 as a platform device
> with board specific platform_data from arch/arm/mach-ux500/board-mop500.c
> and the device just accesses the prcmu driver through its exported
> functions.
>
> Making it registered through the prcmu sounds like the right thing
> to do, but it requires funneling the board specific ab8500 platform
> data through to the prcmu device registration, something like the
> patch below, which is not really making things nicer overall.

The patch doesn't look awful. There are more '-' than '+', and it's only 
a temporary solution, as the plan is to go solely DT once it's been 
proven viable and ST-Ericsson's delta has been DT:ed and upstreamed anyway.

> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
> index 1509a3c..f8fae8c 100644
> --- a/arch/arm/mach-ux500/board-mop500.c
> +++ b/arch/arm/mach-ux500/board-mop500.c
> @@ -197,24 +197,6 @@ static struct ab8500_platform_data ab8500_platdata = {
>   	.gpio		= &ab8500_gpio_pdata,
>   };
>
> -static struct resource ab8500_resources[] = {
> -	[0] = {
> -		.start	= IRQ_DB8500_AB8500,
> -		.end	= IRQ_DB8500_AB8500,
> -		.flags	= IORESOURCE_IRQ
> -	}
> -};
> -
> -struct platform_device ab8500_device = {
> -	.name = "ab8500-core",
> -	.id = 0,
> -	.dev = {
> -		.platform_data = &ab8500_platdata,
> -	},
> -	.num_resources = 1,
> -	.resource = ab8500_resources,
> -};
> -
>   /*
>    * TPS61052
>    */
> @@ -460,7 +442,6 @@ static struct hash_platform_data u8500_hash1_platform_data = {
>   /* add any platform devices here - TODO */
>   static struct platform_device *mop500_platform_devs[] __initdata = {
>   	&mop500_gpio_keys_device,
> -	&ab8500_device,
>   };
>
>   #ifdef CONFIG_STE_DMA40
> @@ -622,7 +603,6 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
>   	&snowball_led_dev,
>   	&snowball_key_dev,
>   	&snowball_sbnet_dev,
> -	&ab8500_device,
>   };
>
>   static struct platform_device *snowball_of_platform_devs[] __initdata = {
> @@ -639,9 +619,8 @@ static void __init mop500_init_machine(void)
>   	mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
>
>   	mop500_pinmaps_init();
> -	parent = u8500_init_devices();
> +	parent = u8500_init_devices(&ab8500_platform_data);
>
> -	/* FIXME: parent of ab8500 should be prcmu */
>   	for (i = 0; i < ARRAY_SIZE(mop500_platform_devs); i++)
>   		mop500_platform_devs[i]->dev.parent = parent;
>
> @@ -674,7 +653,8 @@ static void __init snowball_init_machine(void)
>   	int i;
>
>   	snowball_pinmaps_init();
> -	parent = u8500_init_devices();
> +
> +	parent = u8500_init_devices(&ab8500_platform_data);
>
>   	for (i = 0; i < ARRAY_SIZE(snowball_platform_devs); i++)
>   		snowball_platform_devs[i]->dev.parent = parent;
> @@ -706,7 +686,7 @@ static void __init hrefv60_init_machine(void)
>   	mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
>
>   	hrefv60_pinmaps_init();
> -	parent = u8500_init_devices();
> +	parent = u8500_init_devices(&ab8500_platform_data);
>
>   	for (i = 0; i < ARRAY_SIZE(mop500_platform_devs); i++)
>   		mop500_platform_devs[i]->dev.parent = parent;
> diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
> index 33275eb..6cc247c 100644
> --- a/arch/arm/mach-ux500/cpu-db8500.c
> +++ b/arch/arm/mach-ux500/cpu-db8500.c
> @@ -207,7 +207,7 @@ static struct device * __init db8500_soc_device_init(void)
>   /*
>    * This function is called from the board init
>    */
> -struct device * __init u8500_init_devices(void)
> +struct device * __init u8500_init_devices(struct ab8500_platform_data *ab8500)
>   {
>   	struct device *parent;
>   	int i;
> @@ -224,6 +224,8 @@ struct device * __init u8500_init_devices(void)
>   	for (i = 0; i < ARRAY_SIZE(platform_devs); i++)
>   		platform_devs[i]->dev.parent = parent;
>
> +	db8500_prcmu_device.dev.platform_data = ab8500;
> +
>   	platform_add_devices(platform_devs, ARRAY_SIZE(platform_devs));
>
>   	return parent;
> diff --git a/arch/arm/mach-ux500/include/mach/setup.h b/arch/arm/mach-ux500/include/mach/setup.h
> index 8b7ed82..7940615 100644
> --- a/arch/arm/mach-ux500/include/mach/setup.h
> +++ b/arch/arm/mach-ux500/include/mach/setup.h
> @@ -17,7 +17,7 @@
>   void __init ux500_map_io(void);
>   extern void __init u8500_map_io(void);
>
> -extern struct device * __init u8500_init_devices(void);
> +extern struct device * __init u8500_init_devices(struct ab8500_platform_data *ab8500);
>
>   extern void __init ux500_init_irq(void);
>   extern void __init ux500_init_late(void);
> diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
> index 50e83dc5..fc0bd4e 100644
> --- a/drivers/mfd/db8500-prcmu.c
> +++ b/drivers/mfd/db8500-prcmu.c
> @@ -2987,6 +2987,8 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
>   		goto no_irq_return;
>   	}
>
> +	db8500_prcmu_devs[AB8500].platform_data = pdev->dev.platform_data;
> +
>   	if (cpu_is_u8500v20_or_later())
>   		prcmu_config_esram0_deep_sleep(ESRAM0_DEEP_SLEEP_STATE_RET);
>
>


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Thu, Jul 05, 2012 at 03:06:20PM +0100, Lee Jones wrote:
> On 05/07/12 14:57, Arnd Bergmann wrote:

> >The non-DT path for this is a huge mess, I'd rather focus on making
> >it obsolete than trying to fix it. Other than that, I agree that
> >we should be registering the ab8500 from the prcmu from both the
> >DT and the non-DT case.

> Ah, is that what you were saying Mark?

> If so, I apologise. I thought you meant register both from platform
> code. I'm happy to register the AB8500 from the DB8500 for _both_ DT
> and !DT.

Yes, that's what I was trying to say - sorry that I wasn't clear!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120705/33b512ca/attachment-0001.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 15:13, Mark Brown wrote:
> On Thu, Jul 05, 2012 at 03:06:20PM +0100, Lee Jones wrote:
>> On 05/07/12 14:57, Arnd Bergmann wrote:
>
>>> The non-DT path for this is a huge mess, I'd rather focus on making
>>> it obsolete than trying to fix it. Other than that, I agree that
>>> we should be registering the ab8500 from the prcmu from both the
>>> DT and the non-DT case.
>
>> Ah, is that what you were saying Mark?
>
>> If so, I apologise. I thought you meant register both from platform
>> code. I'm happy to register the AB8500 from the DB8500 for _both_ DT
>> and !DT.
>
> Yes, that's what I was trying to say - sorry that I wasn't clear!

Well it seems silly of me just to copy Arnd's work. Do you want to 
author and submit the patch to do this Arnd?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Arnd Bergmann]

On Thursday 05 July 2012, Lee Jones wrote:
> 
> On 05/07/12 15:13, Mark Brown wrote:
> > On Thu, Jul 05, 2012 at 03:06:20PM +0100, Lee Jones wrote:
> >> On 05/07/12 14:57, Arnd Bergmann wrote:
> >
> >>> The non-DT path for this is a huge mess, I'd rather focus on making
> >>> it obsolete than trying to fix it. Other than that, I agree that
> >>> we should be registering the ab8500 from the prcmu from both the
> >>> DT and the non-DT case.
> >
> >> Ah, is that what you were saying Mark?
> >
> >> If so, I apologise. I thought you meant register both from platform
> >> code. I'm happy to register the AB8500 from the DB8500 for both DT
> >> and !DT.
> >
> > Yes, that's what I was trying to say - sorry that I wasn't clear!
> 
> Well it seems silly of me just to copy Arnd's work. Do you want to 
> author and submit the patch to do this Arnd?

No, just assume ownership of it. I haven't tried building the
patch, so I assume you will have to change it some more. Just
list me as Suggested-by:.

	Arnd

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

On 05/07/12 16:41, Arnd Bergmann wrote:
> On Thursday 05 July 2012, Lee Jones wrote:
>>
>> On 05/07/12 15:13, Mark Brown wrote:
>>> On Thu, Jul 05, 2012 at 03:06:20PM +0100, Lee Jones wrote:
>>>> On 05/07/12 14:57, Arnd Bergmann wrote:
>>>
>>>>> The non-DT path for this is a huge mess, I'd rather focus on making
>>>>> it obsolete than trying to fix it. Other than that, I agree that
>>>>> we should be registering the ab8500 from the prcmu from both the
>>>>> DT and the non-DT case.
>>>
>>>> Ah, is that what you were saying Mark?
>>>
>>>> If so, I apologise. I thought you meant register both from platform
>>>> code. I'm happy to register the AB8500 from the DB8500 for both DT
>>>> and !DT.
>>>
>>> Yes, that's what I was trying to say - sorry that I wasn't clear!
>>
>> Well it seems silly of me just to copy Arnd's work. Do you want to
>> author and submit the patch to do this Arnd?
>
> No, just assume ownership of it. I haven't tried building the
> patch, so I assume you will have to change it some more. Just
> list me as Suggested-by:.

It'll have to wait until tomorrow now, but okay, I'll take it on.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Arnd Bergmann]

On Tuesday 03 July 2012, Mark Brown wrote:
> > Before, when we probed each device from Device Tree we came up
> > against some fairly major limitations of the Device Tree. As a
> > result, Arnd and I agreed that this was the way to go.
> 
> I'm really unconvinced that instnatiating the MFD cells from device tree
> is in general a good idea.

In general it's not, e.g. it makes no sense when the MFD is just
a bunch of registers that are mapped to various distinct Linux
devices. The two mfd devices on ux500 (prcmu and ab8500) are really
buses by themselves that happen to be implemented using the MFD
framework on Linux.

I don't see how we would get around representing the buses in
the device tree because we have to refer to nodes under them
from off-chip devices. Simplified we have something like


  db9500 {
	prcmu {
		opp at 1 {
		};

		regulator at 2 {
		};

		watchdog at 4 {
		};

		ab8500 at 5 {
			regulator at 3 {
			};
			regulator at 4 {
			};
			adc at a {
			};
			rtc at f {
			};
			irq at e {
			};
			gpio at 10 {
			};
		};
	};
  };

Most of the stuff on the board is connected to the ab8500 regulators and
gpio pins, which are also used as interrupts. In order to hook up
the external devices in the device tree, we need to have something that
we can point to as their interrupt-parent or the phandle in the gpio
description.

	Arnd

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Mark Brown]

On Tue, Jul 03, 2012 at 02:01:35PM +0000, Arnd Bergmann wrote:
> On Tuesday 03 July 2012, Mark Brown wrote:

> > I'm really unconvinced that instnatiating the MFD cells from device tree
> > is in general a good idea.

> In general it's not, e.g. it makes no sense when the MFD is just
> a bunch of registers that are mapped to various distinct Linux
> devices. The two mfd devices on ux500 (prcmu and ab8500) are really
> buses by themselves that happen to be implemented using the MFD
> framework on Linux.

This is true for pretty much all MFDs except the pure MMIO ones.

> I don't see how we would get around representing the buses in
> the device tree because we have to refer to nodes under them
> from off-chip devices. Simplified we have something like

We can always refer to the parent device itself; there's two separate
things going on here which we can resolve separately.  One is
instantiating the children and the other is referencing the various
configuration that's needed which we don't need to do by mapping MFD
cells directly onto device tree nodes.  The MFD children can always look
at the DT bindings of their parents.

> Most of the stuff on the board is connected to the ab8500 regulators and
> gpio pins, which are also used as interrupts. In order to hook up
> the external devices in the device tree, we need to have something that
> we can point to as their interrupt-parent or the phandle in the gpio
> description.

Right, but there's no reason we have to instantiate a dedicated node for
each of these things and we should think carefully about what we're
doing.  One of the problems I've seen with people doing this stuff is
that the split we do for some of the cells in Linux is often not really
a good generic representation of the hardware (and some of them are actually
churning right now) so encoding it in the device tree isn't so helpful.
You also seem to get stuff where the MFD cells are all full of hard
coding for their parent so there's no way you could reuse them and again
we don't gain anything.

There will be some places where representing things directly in the DT
makes sense but we should think carefully before we go and do it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120703/058ad2f7/attachment.sig>

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

Hi Sam,

On 03/07/12 12:59, Lee Jones wrote:
> We register the ab8500 as an MFD device from db8500 code during Device Tree
> boot in order to solve some limitations of DT. However, when Device Tree is
> not enabled, we still want to allow platform code to register the ab8500 in
> the normal way. Here we force MFD device registration of the ab8500 only
> when booting with Device Tree enabled.
>
> Solves this issue:
> WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0x88/0xb0()
> sysfs: cannot create duplicate filename '/bus/platform/devices/ab8500-core.0'
>
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Are you planning on taking this in any time soon? I'd really like to fix 
this bug for Linus in -next.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Fabio Estevam]

Hi Lee,

On Tue, Jul 3, 2012 at 8:59 AM, Lee Jones <lee.jones@linaro.org> wrote:
> We register the ab8500 as an MFD device from db8500 code during Device Tree
> boot in order to solve some limitations of DT. However, when Device Tree is
> not enabled, we still want to allow platform code to register the ab8500 in
> the normal way. Here we force MFD device registration of the ab8500 only
> when booting with Device Tree enabled.
>
> Solves this issue:
> WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0x88/0xb0()
> sysfs: cannot create duplicate filename '/bus/platform/devices/ab8500-core.0'

Does the following patch fix your issue?
http://www.spinics.net/lists/arm-kernel/msg182827.html

Regards,

Fabio Estevam

[PATCH 1/1] mfd: Fix runtime warning caused by duplicate device registration

[Lee Jones]

Hi Fabio,

> On Tue, Jul 3, 2012 at 8:59 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> We register the ab8500 as an MFD device from db8500 code during Device Tree
>> boot in order to solve some limitations of DT. However, when Device Tree is
>> not enabled, we still want to allow platform code to register the ab8500 in
>> the normal way. Here we force MFD device registration of the ab8500 only
>> when booting with Device Tree enabled.
>>
>> Solves this issue:
>> WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0x88/0xb0()
>> sysfs: cannot create duplicate filename '/bus/platform/devices/ab8500-core.0'
>
> Does the following patch fix your issue?
> http://www.spinics.net/lists/arm-kernel/msg182827.html

No, it's unrelated.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog