[PATCH] gianfar_ptp: use iomem, not ioports resource tree in probe

From: Wei Yang <redacted>

When using a 36 bit dtb file, the driver complains "resource busy".

Investigating the source of the message leads one to the
gianfar_ptp_probe function.

Since the type of the device resource requested in this function
is IORESOURCE_MEM, it should use "iomem_resource" instead of
"ioports_resource".

Signed-off-by: Wei Yang <redacted>
Cc: Claudiu Manoil <redacted>
Cc: Timur Tabi <redacted>
Signed-off-by: Paul Gortmaker <redacted>
---
 drivers/net/ethernet/freescale/gianfar_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ptp.c b/drivers/net/ethernet/freescale/gianfar_ptp.c
index b9db0e0..2e5daee 100644
--- a/drivers/net/ethernet/freescale/gianfar_ptp.c
+++ b/drivers/net/ethernet/freescale/gianfar_ptp.c

@@ -478,7 +478,7 @@ static int gianfar_ptp_probe(struct platform_device *dev)
 		pr_err("no resource\n");
 		goto no_resource;
 	}
-	if (request_resource(&ioport_resource, etsects->rsrc)) {
+	if (request_resource(&iomem_resource, etsects->rsrc)) {
 		pr_err("resource busy\n");
 		goto no_resource;
 	}

-- 
1.7.11.1

On Wed, Oct 24, 2012 at 10:21 AM, Paul Gortmaker
[off-list ref] wrote:

From: Wei Yang <redacted>

When using a 36 bit dtb file, the driver complains "resource busy".

Investigating the source of the message leads one to the
gianfar_ptp_probe function.

Since the type of the device resource requested in this function
is IORESOURCE_MEM, it should use "iomem_resource" instead of
"ioports_resource".

I can't comment on this patch, since I didn't write the driver, but I
am confused on one thing.  Why is the driver using platform_xxx calls
to get data?  Why isn't it using of_xxx calls to read properties from
the device tree?  For example, why is it using

	etsects->irq = platform_get_irq(dev, 0);

to get the IRQ?  Shouldn't it do this instead:

	etsects->irq = irq_of_parse_and_map(node, 0);

-- 
Timur Tabi
Linux kernel developer at Freescale

On 12-10-24 01:59 PM, Tabi Timur-B04825 wrote:

On Wed, Oct 24, 2012 at 10:21 AM, Paul Gortmaker
[off-list ref] wrote:

quoted

From: Wei Yang <redacted>

When using a 36 bit dtb file, the driver complains "resource busy".

Investigating the source of the message leads one to the
gianfar_ptp_probe function.

Since the type of the device resource requested in this function
is IORESOURCE_MEM, it should use "iomem_resource" instead of
"ioports_resource".

I can't comment on this patch, since I didn't write the driver, but I
am confused on one thing.  Why is the driver using platform_xxx calls

Even if it makes sense to convert the driver to of_xxx calls,
I think the obvious bug should be fixed as a separate commit,
so that the -stable folks have something to cherry pick.

P.
--

to get data?  Why isn't it using of_xxx calls to read properties from
the device tree?  For example, why is it using

	etsects->irq = platform_get_irq(dev, 0);

to get the IRQ?  Shouldn't it do this instead:

	etsects->irq = irq_of_parse_and_map(node, 0);

Paul Gortmaker wrote:

Even if it makes sense to convert the driver to of_xxx calls,
I think the obvious bug should be fixed as a separate commit,
so that the -stable folks have something to cherry pick.

Oh, I agree with that.  I was just wondering why an OF-enabled driver
would not use OF calls.  I've never seen that before.  My instinct is that
the original developer had no idea what he was doing, but perhaps there is
a very good reason for the way the driver is written.

-- 
Timur Tabi
Linux kernel developer at Freescale

On Wed, Oct 24, 2012 at 02:12:55PM -0500, Timur Tabi wrote:

Paul Gortmaker wrote:

quoted

Even if it makes sense to convert the driver to of_xxx calls,
I think the obvious bug should be fixed as a separate commit,
so that the -stable folks have something to cherry pick.

Oh, I agree with that.  I was just wondering why an OF-enabled driver
would not use OF calls.  I've never seen that before.  My instinct is that
the original developer had no idea what he was doing, but perhaps there is
a very good reason for the way the driver is written.

Instead of using your instinct, try using your brain instead.

Thanks,
Richard

Richard Cochran wrote:

quoted

quoted

Oh, I agree with that.  I was just wondering why an OF-enabled driver
would not use OF calls.  I've never seen that before.  My instinct is that
the original developer had no idea what he was doing, but perhaps there is
a very good reason for the way the driver is written.

Instead of using your instinct, try using your brain instead.

Well, as you are obviously so much smarter than I am, how about
enlightening me?  I do not see any explanations in the original commit,
and I do not know why someone would use non-OF calls to get data from the
device tree.  I didn't even know that you could use platform_get_irq() to
get the virtual IRQ from a device tree.

-- 
Timur Tabi
Linux kernel developer at Freescale

On Wed, Oct 24, 2012 at 03:11:09PM -0500, Timur Tabi wrote:

Richard Cochran wrote:

quoted

quoted

quoted

Oh, I agree with that.  I was just wondering why an OF-enabled driver
would not use OF calls.  I've never seen that before.  My instinct is that
the original developer had no idea what he was doing, but perhaps there is
a very good reason for the way the driver is written.

quoted

Instead of using your instinct, try using your brain instead.

Well, as you are obviously so much smarter than I am, how about
enlightening me?  I do not see any explanations in the original commit,
and I do not know why someone would use non-OF calls to get data from the
device tree.  I didn't even know that you could use platform_get_irq() to
get the virtual IRQ from a device tree.

And now you know.

From: Paul Gortmaker <redacted>
Date: Wed, 24 Oct 2012 11:21:36 -0400

From: Wei Yang <redacted>

When using a 36 bit dtb file, the driver complains "resource busy".

Investigating the source of the message leads one to the
gianfar_ptp_probe function.

Since the type of the device resource requested in this function
is IORESOURCE_MEM, it should use "iomem_resource" instead of
"ioports_resource".

Signed-off-by: Wei Yang <redacted>
Cc: Claudiu Manoil <redacted>
Cc: Timur Tabi <redacted>
Signed-off-by: Paul Gortmaker <redacted>

Applied, thanks everyone.

On 10/25/2012 03:12 AM, Timur Tabi wrote:

Paul Gortmaker wrote:

quoted

Even if it makes sense to convert the driver to of_xxx calls,
I think the obvious bug should be fixed as a separate commit,
so that the -stable folks have something to cherry pick.

Oh, I agree with that.  I was just wondering why an OF-enabled driver
would not use OF calls.  I've never seen that before.  My instinct is that
the original developer had no idea what he was doing, but perhaps there is
a very good reason for the way the driver is written.

okay, If have time, I will check the issue mentioned by you.:-)

Thanks
Wei

On Wed, Oct 24, 2012 at 02:12:55PM -0500, Timur Tabi wrote:

Paul Gortmaker wrote:

quoted

Even if it makes sense to convert the driver to of_xxx calls,
I think the obvious bug should be fixed as a separate commit,
so that the -stable folks have something to cherry pick.

Oh, I agree with that.  I was just wondering why an OF-enabled driver
would not use OF calls.  I've never seen that before.  My instinct is that
the original developer had no idea what he was doing, but perhaps there is
a very good reason for the way the driver is written.

Getting back to your really ignorant comment, I suggest that you look
at this review. It was made by Grant Likely. Perhaps you have heard of
him?

  https://lkml.org/lkml/2011/2/23/281

I was the original developer of the PTP code, and my code went through
fifteen rounds of review. And guess what - I actually listened to the
reviewer's comments and changed my work accordingly.

You can read all about what happened, but you will have to find v15
yourself. Be sure to pay special attention to the history of
irq_of_parse_and_map() verses platform_get_irq().

Or maybe your instinct was right, and I don't know what I am doing.

   - [V14] http://lkml.org/lkml/2011/4/18/16
   - [V13] http://lkml.org/lkml/2011/3/27/2
   - [V12] http://lkml.org/lkml/2011/2/28/53
   - [V11] http://lkml.org/lkml/2011/2/23/107
   - [V10] http://lkml.org/lkml/2011/1/27/71
   - [V9]  http://lkml.org/lkml/2011/1/13/65
   - [V8]  http://lkml.org/lkml/2010/12/31/128
   - [V7]  http://lkml.org/lkml/2010/12/16/195
   - [V6]  http://lkml.org/lkml/2010/9/23/310
   - [V5]  http://lkml.org/lkml/2010/8/16/90
   - Thomas Gleixner: Rework of the PTP support series core code
     http://lkml.org/lkml/2011/2/1/137
   - Dynamic clock devices [RFC]
     http://lkml.org/lkml/2010/11/4/290
   - POSIX clock tuning syscall with dynamic clock ids
     http://lkml.org/lkml/2010/9/3/119
   - POSIX clock tuning syscall with static clock ids
     http://lkml.org/lkml/2010/8/23/49
   - Versions 1-4 appeared on the netdev list.

Thanks,
Richard

Richard Cochran wrote:

Getting back to your really ignorant comment, I suggest that you look
at this review. It was made by Grant Likely. Perhaps you have heard of
him?

  https://lkml.org/lkml/2011/2/23/281

I was the original developer of the PTP code, and my code went through
fifteen rounds of review. And guess what - I actually listened to the
reviewer's comments and changed my work accordingly.

I'm sorry for what I said.  It was inappropriate.  I deal with crappy code
from co-workers on a daily basis, and sometimes I forgot that just because
something is done differently than I would have done it, that doesn't mean
it's wrong.

You can read all about what happened, but you will have to find v15
yourself. Be sure to pay special attention to the history of
irq_of_parse_and_map() verses platform_get_irq().

I actually did that research and saw Grant's comments.  I asked him about
it on IRC, and although I understand his reasoning, I'm not sure I agree
with all of it.  In particular, I think
platform_get_resource/request_resource/ioremap is less elegant than just
calling of_iomap.


-- 
Timur Tabi
Linux kernel developer at Freescale

On Thu, Oct 25, 2012 at 03:17:48PM -0500, Timur Tabi wrote:

I'm sorry for what I said.  It was inappropriate.  I deal with crappy code
from co-workers on a daily basis, and sometimes I forgot that just because
something is done differently than I would have done it, that doesn't mean
it's wrong.

Thanks for that apology. Now my wounded pride has recovered :)

I actually did that research and saw Grant's comments.  I asked him about
it on IRC, and although I understand his reasoning, I'm not sure I agree
with all of it.  In particular, I think
platform_get_resource/request_resource/ioremap is less elegant than just
calling of_iomap.

No comment from me, since I always seem to lose at the DT game.

Thanks,
Richard