Messages in this thread | | | Subject | Re: [PATCH] net: ethernet: davicom: fix devicetree irq resource | From | Sergei Shtylyov <> | Date | Thu, 4 Feb 2016 23:56:38 +0300 |
| |
On 02/04/2016 11:42 PM, Robert Jarzmik wrote:
>> Your patch summary prefixes are too verbose, it was enough to say only >> "dm9000: ".
Or "davicom: dm9000: ". Missing the driver name itself doesn't look very consistent. :-)
> Well, I don't agree here. The subsystem should be fully specified, at least this > is something I require in pxa, something that is also required in sound/*, etc > ... If David doesn't object, I'll keep it that way. As it's his tree, his > decision in the end, so let's have him decide.
I expect that he disagrees with you. Let's wait...
>>> - /* If there is no IRQ type specified, default to something that >>> - * may work, and tell the user that this is a problem */ >>> - >>> - if (irqflags == IRQF_TRIGGER_NONE) >>> - irqflags = irq_get_trigger_type(dev->irq); >>> - >>> - if (irqflags == IRQF_TRIGGER_NONE) >>> + /* If there is no IRQ type specified, tell the user that this is a >>> + * problem */ >> >> The networking code formats comments this way: >> >> /* foo >> * bar >> */ > May I know where this is documented ?
Documentation/CodingStyle, chapter 8. Have you run your patch thru scripts/checkpatch.pl?
> I'm asking because I didn't find it, because I parsed drivers/net/*.c files, and > the standard kernel comment style was there, ie: > /* > * foo > * bar > */
But you didn't follow it as well?
> I was reusing the previous comment style,
Ah...
> but I will change it for the standard > kernel style if you wish.
Yes, I think checkpatch.pl checks for that, --strict is forced for the networking code.
>>> + ndev->irq = platform_get_irq(pdev, 0); >>> + if (ndev->irq <= 0) { >> >> I don't recommend checking for 0 and returning early in this case -- >> you'll signal a probe success this way. Either ignore 0 or return -E<smth> >> in this case. Unfortunately, platform_get_irq() is so sloppily coded now that it >> *can* return 0 on error. :-(
I'll try looking into this issue once I get more free time...
> Ah we had that discussion not very long ago, didn't we ? :)
Yeah, I remembered that just after hitting <Send>. :-)
> And I think I'll reuse the "if (ndev->irq < 0) {" solution to be consistent with > myself.
> Thanks for the review.
My pleasure. :-)
MBR, Sergei
| |