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

[Kieran Bingham]

Hi Laurent,

On 12/08/2021 15:16, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> 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.


>> 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.


>>  #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 ...



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