Re: [PATCH v2 1/5] iommu/amd - Add debugfs support

[Gary R Hook]

On 03/13/2018 03:23 PM, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 8:54 PM, Gary R Hook <gary.hook@xxxxxxx> 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@xxxxxxx> 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