lkml.org 
[lkml]   [2018]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/5] iommu/amd - Add debugfs support
From
Date
On 03/13/2018 03:23 PM, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook <gary.hook@amd.com> wrote:
>> On 03/13/2018 12:16 PM, Andy Shevchenko wrote:
>>> On Fri, Mar 9, 2018 at 2:50 AM, Gary R Hook <gary.hook@amd.com> wrote:
>
>>>> +#include <linux/pci.h>
>>>> +#include <linux/iommu.h>
>>>> +#include <linux/debugfs.h>
>>>
>>>
>>> Keep in order?
>
>> What order would that be? These few needed files are listed in the same
>> order as which they appear in amd_iommu.c. I'm gonna need a preference
>> spelled out, please (and a rationale, so I may better understand).
>
> To increase readability and avoid potential header duplication (here
> is can bus protocol implementation where the problem exists for real,
> even in new code!)

With all due respect, I don't find that you clearly answered my
question. I will hazard a guess that you mean to -alphabetize- them?
Which I am happy to do, and will do so in the next version.

If that is not your meaning, I'll have to ask you to use small words,
and not presume any understanding on my (or anyone's) part about
preferences that are not documented in the style guide. I don't mean to
be thick, but I have to ask for clarity.

Given that this is a preference, and that there are reasons for -not-
doing so, I would also like to hear other comments on this suggestionn.


>>>> + for (i = start ; i <= end ; i++)
>
>>> Missed {}
>
>> Wasn't sure about the M.O. given that the body of this loop is a single if
>> statement. And I don't see anywhere in
>> https://www.kernel.org/doc/html/latest/process/coding-style.html
>> in section 3.1 where curly braces are called for in this situation. May I
>> ask for clarification on the style rule, please?
>
> You can do nothing, though I'm guided by the end of section 3.0
> (though it tells only about 'if' case).

Fixed this.

>
>>>> @@ -89,6 +89,7 @@
>>>> #define ACPI_DEVFLAG_ATSDIS 0x10000000
>>>>
>>>> #define LOOP_TIMEOUT 100000
>>>> +
>>>> /*
>>>> * ACPI table definitions
>>>> *
>
>>> Doesn't belong to the patch.
>
>> I'm sorry, I don't understand. The added blank line doesn't belong to the
>> patch?
>
> Correct.
>
Fixed this.

Thanks,
Gary

\
 
 \ /
  Last update: 2018-03-14 16:25    [W:0.114 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site