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