| Message ID | 20210812135750.165416-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
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, ... > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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 ? > #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. > static constexpr uint32_t kMaxCellWidthPerSet = 160; > static constexpr uint32_t kMaxCellHeightPerSet = 56;
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@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; >
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@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;
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> #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" static constexpr uint32_t kMaxCellWidthPerSet = 160; static constexpr uint32_t kMaxCellHeightPerSet = 56;
The ipu3 does not follow the current coding style practices for header inclusion. Re-order the headers to match the updated styles. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)