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

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

[Paul Gortmaker]

From: Wei Yang <Wei.Yang@windriver.com>

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 <Wei.Yang@windriver.com>
Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
Cc: Timur Tabi <timur@freescale.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 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

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

[Tabi Timur-B04825]

On Wed, Oct 24, 2012 at 10:21 AM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> From: Wei Yang <Wei.Yang@windriver.com>
>
> 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

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

[Paul Gortmaker]

On 12-10-24 01:59 PM, Tabi Timur-B04825 wrote:
> On Wed, Oct 24, 2012 at 10:21 AM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
>> From: Wei Yang <Wei.Yang@windriver.com>
>>
>> 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);
> 

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

[Timur Tabi]

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

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

[Richard Cochran]

On Wed, Oct 24, 2012 at 02:12:55PM -0500, Timur Tabi wrote:
> 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.

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

Thanks,
Richard

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

[Timur Tabi]

Richard Cochran wrote:
>> > 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

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

[Richard Cochran]

On Wed, Oct 24, 2012 at 03:11:09PM -0500, Timur Tabi wrote:
> Richard Cochran wrote:
> >> > 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.

And now you know.

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

[David Miller]

From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Wed, 24 Oct 2012 11:21:36 -0400

> From: Wei Yang <Wei.Yang@windriver.com>
> 
> 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 <Wei.Yang@windriver.com>
> Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
> Cc: Timur Tabi <timur@freescale.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Applied, thanks everyone.

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

[wyang1]

On 10/25/2012 03:12 AM, Timur Tabi wrote:
> 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.
>
okay, If have time, I will check the issue mentioned by you.:-)

Thanks
Wei

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

[Richard Cochran]

On Wed, Oct 24, 2012 at 02:12:55PM -0500, Timur Tabi wrote:
> 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.

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

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

[Timur Tabi]

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

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

[Richard Cochran]

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