| Message ID | 20210614095340.3051816-5-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch. On Mon, 14 Jun 2021 at 10:53, Naushir Patuck <naush@raspberrypi.com> wrote: > > Adjust source formatting to closer match libcamera guidelines: > > - Switch to C style comments. > - Adjust whitespace for readability. > - Remove retcode local variable usage. > > There are no functional changes in this commit. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> All looks good to me! If we did decide to get rid of the whole line length thing, I think that would be a separate commit anyway. Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > --- > src/ipa/raspberrypi/md_parser_smia.cpp | 70 ++++++++++++++------------ > 1 file changed, 38 insertions(+), 32 deletions(-) > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp > index 852a1d347d11..65ffbe00c76e 100644 > --- a/src/ipa/raspberrypi/md_parser_smia.cpp > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp > @@ -1,31 +1,32 @@ > /* SPDX-License-Identifier: BSD-2-Clause */ > /* > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited > * > - * md_parser.cpp - image sensor metadata parsers > + * md_parser_smia.cpp - SMIA specification based embedded data parser > */ > - > #include <assert.h> > #include <map> > -#include <string.h> > +#include <string> > > #include "md_parser.hpp" > > using namespace RPiController; > > -// This function goes through the embedded data to find the offsets (not > -// values!), in the data block, where the values of the given registers can > -// subsequently be found. > - > -// Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA > -// sensors, I think. > +/* > + * This function goes through the embedded data to find the offsets (not > + * values!), in the data block, where the values of the given registers can > + * subsequently be found. > + * > + * Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA > + * sensors, I think. > + */ > > -#define LINE_START 0x0a > -#define LINE_END_TAG 0x07 > -#define REG_HI_BITS 0xaa > -#define REG_LOW_BITS 0xa5 > -#define REG_VALUE 0x5a > -#define REG_SKIP 0x55 > +constexpr unsigned int LINE_START = 0x0a; > +constexpr unsigned int LINE_END_TAG = 0x07; > +constexpr unsigned int REG_HI_BITS = 0xaa; > +constexpr unsigned int REG_LOW_BITS = 0xa5; > +constexpr unsigned int REG_VALUE = 0x5a; > +constexpr unsigned int REG_SKIP = 0x55; > > MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer, > uint32_t regs[], int offsets[], > @@ -36,12 +37,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> > if (buffer[0] != LINE_START) > return NO_LINE_START; > > - unsigned int current_offset = 1; // after the LINE_START > + unsigned int current_offset = 1; /* after the LINE_START */ > unsigned int current_line_start = 0, current_line = 0; > unsigned int reg_num = 0, first_reg = 0; > - ParseStatus retcode = PARSE_OK; > + > while (1) { > int tag = buffer[current_offset++]; > + > if ((bits_per_pixel_ == 10 && > (current_offset + 1 - current_line_start) % 5 == 0) || > (bits_per_pixel_ == 12 && > @@ -49,34 +51,38 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> > if (buffer[current_offset++] != REG_SKIP) > return BAD_DUMMY; > } > + > int data_byte = buffer[current_offset++]; > - //printf("Offset %u, tag 0x%02x data_byte 0x%02x\n", current_offset-1, tag, data_byte); > + > if (tag == LINE_END_TAG) { > if (data_byte != LINE_END_TAG) > return BAD_LINE_END; > + > if (num_lines_ && ++current_line == num_lines_) > return MISSING_REGS; > + > if (line_length_bytes_) { > - current_offset = > - current_line_start + line_length_bytes_; > - // Require whole line to be in the buffer (if buffer size set). > + current_offset = current_line_start + line_length_bytes_; > + > + /* Require whole line to be in the buffer (if buffer size set). */ > if (buffer.size() && > - current_offset + line_length_bytes_ > > - buffer.size()) > + current_offset + line_length_bytes_ > buffer.size()) > return MISSING_REGS; > + > if (buffer[current_offset] != LINE_START) > return NO_LINE_START; > } else { > - // allow a zero line length to mean "hunt for the next line" > + /* allow a zero line length to mean "hunt for the next line" */ > while (buffer[current_offset] != LINE_START && > current_offset < buffer.size()) > current_offset++; > + > if (current_offset == buffer.size()) > return NO_LINE_START; > } > - // inc current_offset to after LINE_START > - current_line_start = > - current_offset++; > + > + /* inc current_offset to after LINE_START */ > + current_line_start = current_offset++; > } else { > if (tag == REG_HI_BITS) > reg_num = (reg_num & 0xff) | (data_byte << 8); > @@ -86,13 +92,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> > reg_num++; > else if (tag == REG_VALUE) { > while (reg_num >= > - // assumes registers are in order... > + /* assumes registers are in order... */ > regs[first_reg]) { > if (reg_num == regs[first_reg]) > - offsets[first_reg] = > - current_offset - 1; > + offsets[first_reg] = current_offset - 1; > + > if (++first_reg == num_regs) > - return retcode; > + return PARSE_OK; > } > reg_num++; > } else > -- > 2.25.1 >
Hi Naush, Thank you for the patch. On Mon, Jun 14, 2021 at 10:53:38AM +0100, Naushir Patuck wrote: > Adjust source formatting to closer match libcamera guidelines: > > - Switch to C style comments. > - Adjust whitespace for readability. > - Remove retcode local variable usage. > > There are no functional changes in this commit. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/md_parser_smia.cpp | 70 ++++++++++++++------------ > 1 file changed, 38 insertions(+), 32 deletions(-) > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp > index 852a1d347d11..65ffbe00c76e 100644 > --- a/src/ipa/raspberrypi/md_parser_smia.cpp > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp > @@ -1,31 +1,32 @@ > /* SPDX-License-Identifier: BSD-2-Clause */ > /* > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited > * > - * md_parser.cpp - image sensor metadata parsers > + * md_parser_smia.cpp - SMIA specification based embedded data parser > */ > - > #include <assert.h> > #include <map> > -#include <string.h> > +#include <string> string doesn't seem to be needed, and neither is map. > > #include "md_parser.hpp" > > using namespace RPiController; > > -// This function goes through the embedded data to find the offsets (not > -// values!), in the data block, where the values of the given registers can > -// subsequently be found. > - > -// Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA > -// sensors, I think. > +/* > + * This function goes through the embedded data to find the offsets (not > + * values!), in the data block, where the values of the given registers can > + * subsequently be found. > + * > + * Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA > + * sensors, I think. Are they identical to the values used in the MIPI CCS specification ? If so, maybe we should rename the parser from SMIA to CCS, as the latter is a public specification (SMIA is as well but SMIA++ isn't, if I recall correctly). This can be done later. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I can handle the small change above when applying. > + */ > > -#define LINE_START 0x0a > -#define LINE_END_TAG 0x07 > -#define REG_HI_BITS 0xaa > -#define REG_LOW_BITS 0xa5 > -#define REG_VALUE 0x5a > -#define REG_SKIP 0x55 > +constexpr unsigned int LINE_START = 0x0a; > +constexpr unsigned int LINE_END_TAG = 0x07; > +constexpr unsigned int REG_HI_BITS = 0xaa; > +constexpr unsigned int REG_LOW_BITS = 0xa5; > +constexpr unsigned int REG_VALUE = 0x5a; > +constexpr unsigned int REG_SKIP = 0x55; > > MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer, > uint32_t regs[], int offsets[], > @@ -36,12 +37,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> > if (buffer[0] != LINE_START) > return NO_LINE_START; > > - unsigned int current_offset = 1; // after the LINE_START > + unsigned int current_offset = 1; /* after the LINE_START */ > unsigned int current_line_start = 0, current_line = 0; > unsigned int reg_num = 0, first_reg = 0; > - ParseStatus retcode = PARSE_OK; > + > while (1) { > int tag = buffer[current_offset++]; > + > if ((bits_per_pixel_ == 10 && > (current_offset + 1 - current_line_start) % 5 == 0) || > (bits_per_pixel_ == 12 && > @@ -49,34 +51,38 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> > if (buffer[current_offset++] != REG_SKIP) > return BAD_DUMMY; > } > + > int data_byte = buffer[current_offset++]; > - //printf("Offset %u, tag 0x%02x data_byte 0x%02x\n", current_offset-1, tag, data_byte); > + > if (tag == LINE_END_TAG) { > if (data_byte != LINE_END_TAG) > return BAD_LINE_END; > + > if (num_lines_ && ++current_line == num_lines_) > return MISSING_REGS; > + > if (line_length_bytes_) { > - current_offset = > - current_line_start + line_length_bytes_; > - // Require whole line to be in the buffer (if buffer size set). > + current_offset = current_line_start + line_length_bytes_; > + > + /* Require whole line to be in the buffer (if buffer size set). */ > if (buffer.size() && > - current_offset + line_length_bytes_ > > - buffer.size()) > + current_offset + line_length_bytes_ > buffer.size()) > return MISSING_REGS; > + > if (buffer[current_offset] != LINE_START) > return NO_LINE_START; > } else { > - // allow a zero line length to mean "hunt for the next line" > + /* allow a zero line length to mean "hunt for the next line" */ > while (buffer[current_offset] != LINE_START && > current_offset < buffer.size()) > current_offset++; > + > if (current_offset == buffer.size()) > return NO_LINE_START; > } > - // inc current_offset to after LINE_START > - current_line_start = > - current_offset++; > + > + /* inc current_offset to after LINE_START */ > + current_line_start = current_offset++; > } else { > if (tag == REG_HI_BITS) > reg_num = (reg_num & 0xff) | (data_byte << 8); > @@ -86,13 +92,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> > reg_num++; > else if (tag == REG_VALUE) { > while (reg_num >= > - // assumes registers are in order... > + /* assumes registers are in order... */ > regs[first_reg]) { > if (reg_num == regs[first_reg]) > - offsets[first_reg] = > - current_offset - 1; > + offsets[first_reg] = current_offset - 1; > + > if (++first_reg == num_regs) > - return retcode; > + return PARSE_OK; > } > reg_num++; > } else
Hi Laurent, On Tue, 15 Jun 2021 at 03:48, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Mon, Jun 14, 2021 at 10:53:38AM +0100, Naushir Patuck wrote: > > Adjust source formatting to closer match libcamera guidelines: > > > > - Switch to C style comments. > > - Adjust whitespace for readability. > > - Remove retcode local variable usage. > > > > There are no functional changes in this commit. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/md_parser_smia.cpp | 70 ++++++++++++++------------ > > 1 file changed, 38 insertions(+), 32 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp > b/src/ipa/raspberrypi/md_parser_smia.cpp > > index 852a1d347d11..65ffbe00c76e 100644 > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp > > @@ -1,31 +1,32 @@ > > /* SPDX-License-Identifier: BSD-2-Clause */ > > /* > > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited > > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited > > * > > - * md_parser.cpp - image sensor metadata parsers > > + * md_parser_smia.cpp - SMIA specification based embedded data parser > > */ > > - > > #include <assert.h> > > #include <map> > > -#include <string.h> > > +#include <string> > > string doesn't seem to be needed, and neither is map. > > > > > #include "md_parser.hpp" > > > > using namespace RPiController; > > > > -// This function goes through the embedded data to find the offsets (not > > -// values!), in the data block, where the values of the given registers > can > > -// subsequently be found. > > - > > -// Embedded data tag bytes, from Sony IMX219 datasheet but general to > all SMIA > > -// sensors, I think. > > +/* > > + * This function goes through the embedded data to find the offsets (not > > + * values!), in the data block, where the values of the given registers > can > > + * subsequently be found. > > + * > > + * Embedded data tag bytes, from Sony IMX219 datasheet but general to > all SMIA > > + * sensors, I think. > > Are they identical to the values used in the MIPI CCS specification ? If > so, maybe we should rename the parser from SMIA to CCS, as the latter is > a public specification (SMIA is as well but SMIA++ isn't, if I recall > correctly). This can be done later. > Sure, renaming it to CCS seems sensible here. Thanks, Naush > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I can handle the small change above when applying. > > > + */ > > > > -#define LINE_START 0x0a > > -#define LINE_END_TAG 0x07 > > -#define REG_HI_BITS 0xaa > > -#define REG_LOW_BITS 0xa5 > > -#define REG_VALUE 0x5a > > -#define REG_SKIP 0x55 > > +constexpr unsigned int LINE_START = 0x0a; > > +constexpr unsigned int LINE_END_TAG = 0x07; > > +constexpr unsigned int REG_HI_BITS = 0xaa; > > +constexpr unsigned int REG_LOW_BITS = 0xa5; > > +constexpr unsigned int REG_VALUE = 0x5a; > > +constexpr unsigned int REG_SKIP = 0x55; > > > > MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const > uint8_t> buffer, > > uint32_t regs[], int > offsets[], > > @@ -36,12 +37,13 @@ MdParserSmia::ParseStatus > MdParserSmia::findRegs(libcamera::Span<const uint8_t> > > if (buffer[0] != LINE_START) > > return NO_LINE_START; > > > > - unsigned int current_offset = 1; // after the LINE_START > > + unsigned int current_offset = 1; /* after the LINE_START */ > > unsigned int current_line_start = 0, current_line = 0; > > unsigned int reg_num = 0, first_reg = 0; > > - ParseStatus retcode = PARSE_OK; > > + > > while (1) { > > int tag = buffer[current_offset++]; > > + > > if ((bits_per_pixel_ == 10 && > > (current_offset + 1 - current_line_start) % 5 == 0) || > > (bits_per_pixel_ == 12 && > > @@ -49,34 +51,38 @@ MdParserSmia::ParseStatus > MdParserSmia::findRegs(libcamera::Span<const uint8_t> > > if (buffer[current_offset++] != REG_SKIP) > > return BAD_DUMMY; > > } > > + > > int data_byte = buffer[current_offset++]; > > - //printf("Offset %u, tag 0x%02x data_byte 0x%02x\n", > current_offset-1, tag, data_byte); > > + > > if (tag == LINE_END_TAG) { > > if (data_byte != LINE_END_TAG) > > return BAD_LINE_END; > > + > > if (num_lines_ && ++current_line == num_lines_) > > return MISSING_REGS; > > + > > if (line_length_bytes_) { > > - current_offset = > > - current_line_start + > line_length_bytes_; > > - // Require whole line to be in the buffer > (if buffer size set). > > + current_offset = current_line_start + > line_length_bytes_; > > + > > + /* Require whole line to be in the buffer > (if buffer size set). */ > > if (buffer.size() && > > - current_offset + line_length_bytes_ > > > - buffer.size()) > > + current_offset + line_length_bytes_ > > buffer.size()) > > return MISSING_REGS; > > + > > if (buffer[current_offset] != LINE_START) > > return NO_LINE_START; > > } else { > > - // allow a zero line length to mean "hunt > for the next line" > > + /* allow a zero line length to mean "hunt > for the next line" */ > > while (buffer[current_offset] != > LINE_START && > > current_offset < buffer.size()) > > current_offset++; > > + > > if (current_offset == buffer.size()) > > return NO_LINE_START; > > } > > - // inc current_offset to after LINE_START > > - current_line_start = > > - current_offset++; > > + > > + /* inc current_offset to after LINE_START */ > > + current_line_start = current_offset++; > > } else { > > if (tag == REG_HI_BITS) > > reg_num = (reg_num & 0xff) | (data_byte << > 8); > > @@ -86,13 +92,13 @@ MdParserSmia::ParseStatus > MdParserSmia::findRegs(libcamera::Span<const uint8_t> > > reg_num++; > > else if (tag == REG_VALUE) { > > while (reg_num >= > > - // assumes registers are in order... > > + /* assumes registers are in > order... */ > > regs[first_reg]) { > > if (reg_num == regs[first_reg]) > > - offsets[first_reg] = > > - current_offset - 1; > > + offsets[first_reg] = > current_offset - 1; > > + > > if (++first_reg == num_regs) > > - return retcode; > > + return PARSE_OK; > > } > > reg_num++; > > } else > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp index 852a1d347d11..65ffbe00c76e 100644 --- a/src/ipa/raspberrypi/md_parser_smia.cpp +++ b/src/ipa/raspberrypi/md_parser_smia.cpp @@ -1,31 +1,32 @@ /* SPDX-License-Identifier: BSD-2-Clause */ /* - * Copyright (C) 2019, Raspberry Pi (Trading) Limited + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited * - * md_parser.cpp - image sensor metadata parsers + * md_parser_smia.cpp - SMIA specification based embedded data parser */ - #include <assert.h> #include <map> -#include <string.h> +#include <string> #include "md_parser.hpp" using namespace RPiController; -// This function goes through the embedded data to find the offsets (not -// values!), in the data block, where the values of the given registers can -// subsequently be found. - -// Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA -// sensors, I think. +/* + * This function goes through the embedded data to find the offsets (not + * values!), in the data block, where the values of the given registers can + * subsequently be found. + * + * Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA + * sensors, I think. + */ -#define LINE_START 0x0a -#define LINE_END_TAG 0x07 -#define REG_HI_BITS 0xaa -#define REG_LOW_BITS 0xa5 -#define REG_VALUE 0x5a -#define REG_SKIP 0x55 +constexpr unsigned int LINE_START = 0x0a; +constexpr unsigned int LINE_END_TAG = 0x07; +constexpr unsigned int REG_HI_BITS = 0xaa; +constexpr unsigned int REG_LOW_BITS = 0xa5; +constexpr unsigned int REG_VALUE = 0x5a; +constexpr unsigned int REG_SKIP = 0x55; MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer, uint32_t regs[], int offsets[], @@ -36,12 +37,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> if (buffer[0] != LINE_START) return NO_LINE_START; - unsigned int current_offset = 1; // after the LINE_START + unsigned int current_offset = 1; /* after the LINE_START */ unsigned int current_line_start = 0, current_line = 0; unsigned int reg_num = 0, first_reg = 0; - ParseStatus retcode = PARSE_OK; + while (1) { int tag = buffer[current_offset++]; + if ((bits_per_pixel_ == 10 && (current_offset + 1 - current_line_start) % 5 == 0) || (bits_per_pixel_ == 12 && @@ -49,34 +51,38 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> if (buffer[current_offset++] != REG_SKIP) return BAD_DUMMY; } + int data_byte = buffer[current_offset++]; - //printf("Offset %u, tag 0x%02x data_byte 0x%02x\n", current_offset-1, tag, data_byte); + if (tag == LINE_END_TAG) { if (data_byte != LINE_END_TAG) return BAD_LINE_END; + if (num_lines_ && ++current_line == num_lines_) return MISSING_REGS; + if (line_length_bytes_) { - current_offset = - current_line_start + line_length_bytes_; - // Require whole line to be in the buffer (if buffer size set). + current_offset = current_line_start + line_length_bytes_; + + /* Require whole line to be in the buffer (if buffer size set). */ if (buffer.size() && - current_offset + line_length_bytes_ > - buffer.size()) + current_offset + line_length_bytes_ > buffer.size()) return MISSING_REGS; + if (buffer[current_offset] != LINE_START) return NO_LINE_START; } else { - // allow a zero line length to mean "hunt for the next line" + /* allow a zero line length to mean "hunt for the next line" */ while (buffer[current_offset] != LINE_START && current_offset < buffer.size()) current_offset++; + if (current_offset == buffer.size()) return NO_LINE_START; } - // inc current_offset to after LINE_START - current_line_start = - current_offset++; + + /* inc current_offset to after LINE_START */ + current_line_start = current_offset++; } else { if (tag == REG_HI_BITS) reg_num = (reg_num & 0xff) | (data_byte << 8); @@ -86,13 +92,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> reg_num++; else if (tag == REG_VALUE) { while (reg_num >= - // assumes registers are in order... + /* assumes registers are in order... */ regs[first_reg]) { if (reg_num == regs[first_reg]) - offsets[first_reg] = - current_offset - 1; + offsets[first_reg] = current_offset - 1; + if (++first_reg == num_regs) - return retcode; + return PARSE_OK; } reg_num++; } else
Adjust source formatting to closer match libcamera guidelines: - Switch to C style comments. - Adjust whitespace for readability. - Remove retcode local variable usage. There are no functional changes in this commit. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/md_parser_smia.cpp | 70 ++++++++++++++------------ 1 file changed, 38 insertions(+), 32 deletions(-)