[libcamera-devel] [PATCH] ipa: ipu3: fix headers order

[Laurent Pinchart]

Hi Kieran,

On Thu, Aug 12, 2021 at 03:50:12PM +0100, Kieran Bingham wrote:
> On 12/08/2021 15:16, Laurent Pinchart wrote:
> > On Thu, Aug 12, 2021 at 03:57:50PM +0200, Jean-Michel Hautbois wrote:
> >> The ipu3 does not follow the current coding style practices for header inclusion.
> >> Re-order the headers to match the updated styles.
> > 
> > A library-wide patch may be better to be done with this once and for
> > all. However, before that, ...
> 
> I sort of disagree, these can be handled as they arise. This patch is
> split from the upcoming series which likely highlighted this, so it's
> just a cleanup along the way.
> 
> Header sort orders are not ... critical, or bug fixes (for the most
> part). It's just style, and the tool is updated to make it consistent as
> we go.

It's not critical, but the longer we stay in the space between the old
and new styles, the more annoying it will be. I'd rather write a big
patch and get used to the new order than having to deal with a slow
migration for a long time.

> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> ---
> >>  src/ipa/ipu3/ipu3.cpp | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index c34fa460..21b9db64 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -22,16 +22,18 @@
> >>  
> >>  #include <libcamera/control_ids.h>
> >>  #include <libcamera/framebuffer.h>
> >> +#include <libcamera/request.h>
> >> +
> >>  #include <libcamera/ipa/ipa_interface.h>
> >>  #include <libcamera/ipa/ipa_module_info.h>
> >>  #include <libcamera/ipa/ipu3_ipa_interface.h>
> >> -#include <libcamera/request.h>
> > 
> > Kieran, is the IPA headers separation something we want to carry
> > forward, or should we update the clang-format rules ?
> 
> I think this is much better.
> 
> The IPA interface is a part of libcamera, but is separate from the
> public API.
> 
> To me ... sorting folders in alphabetically among files is ... wrong.
> 
> We already have base and internal as distinct groups.
> Merging libcamera/ipa/* in libcamera/* would be an inconsistency if we
> didn't give it a group of it's own.

Fine with me.

> >>  #include "libcamera/internal/mapped_framebuffer.h"
> >>  
> >> +#include "libipa/camera_sensor_helper.h"
> >> +
> >>  #include "ipu3_agc.h"
> >>  #include "ipu3_awb.h"
> >> -#include "libipa/camera_sensor_helper.h"
> > 
> > The split here doesn't bother me much, but we could also do without it
> > if desired.
> 
> Same here, libipa is an internal helper library. It should be included
> before any local includes.
> 
> #include "ipu3_agc.h"
> #include "ipu3_awb.h"
> #include "libipa/camera_sensor_helper.h"
> #include "helpers.h"
> #include "xylophones.h"
> 
> would be wrong to me ...

OK. I have a feeling we'll rework libipa to move from "" to <> at some
point :-)

> >>  static constexpr uint32_t kMaxCellWidthPerSet = 160;
> >>  static constexpr uint32_t kMaxCellHeightPerSet = 56;

-- 
Regards,

Laurent Pinchart