Re: [PATCH 00/12] iio:accel: Header Cleanups.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 11, 2021 at 9:21 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 11 Jun 2021 20:48:08 +0300
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> > On Fri, Jun 11, 2021 at 8:14 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > >
> > > 0-day recently started running the include-what-you-use checker with LLVM builds.
> > > After it identified a header we should have dropped in a particular patch,
> > > I decided to experiment with it a little and see if it was useful for tidying
> > > up includes that have gotten rather out of sync with the code over the years.
> > >
> > > Note the tool is something I'd only advocate using to give you hints on what
> > > might want adjusting so each of these was done by hand inspection.
> > >
> > > I've grouped them by manufacturer as otherwise we would have a lot of patches.
> > > Note that the big 'many device / device type' drivers have been done separately
> > > so you won't see them in this series.
> > >
> > > I'm rather hoping this approach may ease getting reviews of these, but we
> > > shall see.  If people have time to look at ones I haven't directly cc'd them
> > > on that would be great. There are some drivers touched in here where I don't
> > > know of a current maintainer.
> >
> > Same comment as per staging series.
> >
> > I know that kernel.h provides some crucial everywhere used macros /
> > helpers which are in the TODO list to be split.
> >
> > I would recommend dropping kernel.h from the drivers and see what's missed.
> >
> > These series probably need to be based on splitting out container_of()
> > and ARRAY_SIZE() first.
>
> Tool obligingly tells me at least one item from kernel.h and you have
> the correct pair (occasionally it's the kernel specific string routines).
> e.g.
>  #include "linux/kernel.h"                         // for kstrtouint, ARRAY_SIZE

Oh yes, kstrtox.h should be born (I have even somewhere in my tree the
preliminary split).

> > So, no tag from me for now (but I like the idea in general, and thanks
> > for looking into this).
> >
>
> The tool is rather a blunderbuss for this job which makes it rather time
> consuming to use.
>
> Splitting kernel.h up is a step beyond where I'm interested in going
> at this stage because it's sure to involve a lot of bike shedding.

Not really. There is a kind of consensus about the mentioned
container_of() and ARRAY_SIZE() and similar to be split out from
kernel.h.

But I understand you and not insisting that you have to go with it. My
point is that...

> I 'could' just drop the kernel.h where added on the basis we were clearly
> getting it indirectly. I've not included a whole bunch of other suggestions
> on that basis.

(Which is probably not a good idea, because explicit in this case is
better than implicit, i.e. kernel.h is not guaranteed to be included
by other headers and I have long standing work to actually make sure
that most of the headers won't require kernel.h!)

> Note I didn't include a whole bunch of other headers on the basis
> they were a bit more esoteric.
>
> To give an idea of how noisy this is here's the output another file...

...the tool simply doesn't know anything about kernel and header
guarantees. That's why it tries play dumb.

If you would like to continue with this, please drop the removal of
the headers that are not guaranteed to be included by others
(excluding kernel.h from the equation). Otherwise it will become
someone else's problem to _reinstantiate_ all those headers, and since
I already had a headache with panic.h, I won't repeat it. Still no tag
from me, although no explicit NAK (actually opposite, no explicit ACK
because of the dependencies), you just really need to spend more time
on this.

-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux