[{"id":17530,"web_url":"https://patchwork.libcamera.org/comment/17530/","msgid":"","date":"2021-06-14T16:55:20","subject":"Re: [libcamera-devel] [PATCH 4/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser_smia.cpp","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for this patch.\n\nOn Mon, 14 Jun 2021 at 10:53, Naushir Patuck wrote:\n>\n> Adjust source formatting to closer match libcamera guidelines:\n>\n> - Switch to C style comments.\n> - Adjust whitespace for readability.\n> - Remove retcode local variable usage.\n>\n> There are no functional changes in this commit.\n>\n> Signed-off-by: Naushir Patuck \n\nAll looks good to me! If we did decide to get rid of the whole line\nlength thing, I think that would be a separate commit anyway.\n\nReviewed-by: David Plowman \n\nThanks\nDavid\n\n> ---\n> src/ipa/raspberrypi/md_parser_smia.cpp | 70 ++++++++++++++------------\n> 1 file changed, 38 insertions(+), 32 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp\n> index 852a1d347d11..65ffbe00c76e 100644\n> --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> @@ -1,31 +1,32 @@\n> /* SPDX-License-Identifier: BSD-2-Clause */\n> /*\n> - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited\n> *\n> - * md_parser.cpp - image sensor metadata parsers\n> + * md_parser_smia.cpp - SMIA specification based embedded data parser\n> */\n> -\n> #include \n> #include \n> -#include \n> +#include \n>\n> #include \"md_parser.hpp\"\n>\n> using namespace RPiController;\n>\n> -// This function goes through the embedded data to find the offsets (not\n> -// values!), in the data block, where the values of the given registers can\n> -// subsequently be found.\n> -\n> -// Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA\n> -// sensors, I think.\n> +/*\n> + * This function goes through the embedded data to find the offsets (not\n> + * values!), in the data block, where the values of the given registers can\n> + * subsequently be found.\n> + *\n> + * Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA\n> + * sensors, I think.\n> + */\n>\n> -#define LINE_START 0x0a\n> -#define LINE_END_TAG 0x07\n> -#define REG_HI_BITS 0xaa\n> -#define REG_LOW_BITS 0xa5\n> -#define REG_VALUE 0x5a\n> -#define REG_SKIP 0x55\n> +constexpr unsigned int LINE_START = 0x0a;\n> +constexpr unsigned int LINE_END_TAG = 0x07;\n> +constexpr unsigned int REG_HI_BITS = 0xaa;\n> +constexpr unsigned int REG_LOW_BITS = 0xa5;\n> +constexpr unsigned int REG_VALUE = 0x5a;\n> +constexpr unsigned int REG_SKIP = 0x55;\n>\n> MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer,\n> uint32_t regs[], int offsets[],\n> @@ -36,12 +37,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> if (buffer[0] != LINE_START)\n> return NO_LINE_START;\n>\n> - unsigned int current_offset = 1; // after the LINE_START\n> + unsigned int current_offset = 1; /* after the LINE_START */\n> unsigned int current_line_start = 0, current_line = 0;\n> unsigned int reg_num = 0, first_reg = 0;\n> - ParseStatus retcode = PARSE_OK;\n> +\n> while (1) {\n> int tag = buffer[current_offset++];\n> +\n> if ((bits_per_pixel_ == 10 &&\n> (current_offset + 1 - current_line_start) % 5 == 0) ||\n> (bits_per_pixel_ == 12 &&\n> @@ -49,34 +51,38 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> if (buffer[current_offset++] != REG_SKIP)\n> return BAD_DUMMY;\n> }\n> +\n> int data_byte = buffer[current_offset++];\n> - //printf(\"Offset %u, tag 0x%02x data_byte 0x%02x\\n\", current_offset-1, tag, data_byte);\n> +\n> if (tag == LINE_END_TAG) {\n> if (data_byte != LINE_END_TAG)\n> return BAD_LINE_END;\n> +\n> if (num_lines_ && ++current_line == num_lines_)\n> return MISSING_REGS;\n> +\n> if (line_length_bytes_) {\n> - current_offset =\n> - current_line_start + line_length_bytes_;\n> - // Require whole line to be in the buffer (if buffer size set).\n> + current_offset = current_line_start + line_length_bytes_;\n> +\n> + /* Require whole line to be in the buffer (if buffer size set). */\n> if (buffer.size() &&\n> - current_offset + line_length_bytes_ >\n> - buffer.size())\n> + current_offset + line_length_bytes_ > buffer.size())\n> return MISSING_REGS;\n> +\n> if (buffer[current_offset] != LINE_START)\n> return NO_LINE_START;\n> } else {\n> - // allow a zero line length to mean \"hunt for the next line\"\n> + /* allow a zero line length to mean \"hunt for the next line\" */\n> while (buffer[current_offset] != LINE_START &&\n> current_offset < buffer.size())\n> current_offset++;\n> +\n> if (current_offset == buffer.size())\n> return NO_LINE_START;\n> }\n> - // inc current_offset to after LINE_START\n> - current_line_start =\n> - current_offset++;\n> +\n> + /* inc current_offset to after LINE_START */\n> + current_line_start = current_offset++;\n> } else {\n> if (tag == REG_HI_BITS)\n> reg_num = (reg_num & 0xff) | (data_byte << 8);\n> @@ -86,13 +92,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> reg_num++;\n> else if (tag == REG_VALUE) {\n> while (reg_num >=\n> - // assumes registers are in order...\n> + /* assumes registers are in order... */\n> regs[first_reg]) {\n> if (reg_num == regs[first_reg])\n> - offsets[first_reg] =\n> - current_offset - 1;\n> + offsets[first_reg] = current_offset - 1;\n> +\n> if (++first_reg == num_regs)\n> - return retcode;\n> + return PARSE_OK;\n> }\n> reg_num++;\n> } else\n> --\n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7D3E2C3218\n\tfor ;\n\tMon, 14 Jun 2021 16:55:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C708368932;\n\tMon, 14 Jun 2021 18:55:33 +0200 (CEST)","from mail-wr1-x434.google.com (mail-wr1-x434.google.com\n\t[IPv6:2a00:1450:4864:20::434])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3976E602A1\n\tfor ;\n\tMon, 14 Jun 2021 18:55:32 +0200 (CEST)","by mail-wr1-x434.google.com with SMTP id l2so15322134wrw.6\n\tfor ;\n\tMon, 14 Jun 2021 09:55:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"G0e1rFHA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Yf7A3/wmmyxR1ofsapPyeZmBZeBtXHEsvMiKLGYsiOE=;\n\tb=G0e1rFHA2cQBCM5PNMXovuQgiF5Bk8wggLz3vTl9b+k8J9j04ZlGphmKxVHdc0/2wH\n\tpB1PATm73iarsNeMXJw4eDTO2my3I/morJvjzL1c5PzLIWnfNLe7MP3B102TPufHbNQS\n\tE5PoSo2B0IaHmUBJN03OqXJjlX6AT2roGW8x9U0p8ECAb0Q9ykxyS57eGocIhZhbXyrE\n\tDuVwlE+qUD2kCqAh2IUj3vobmlUkuc3CibC6ydDkjG9KnE8RgELPu3ItovGwUOiXL4qh\n\tpD9O0LK5BeGGp9kjAlC8QwgYAjoI3ai5jQuYY0hfDPp2zhSR+WV/Q/NN5PMF54hlUO9U\n\tVBhQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Yf7A3/wmmyxR1ofsapPyeZmBZeBtXHEsvMiKLGYsiOE=;\n\tb=gso6Z/d6wraXHaAa1zr7pTzov9XWmmKxM2uBt+aGoD6VF5DZRyF95ObeWrcimPocRF\n\thCGgXz3+xSO8jjFSmV5j+m5NHlmSSUzngxkxC+2fr1mOrtdv0AX1lgdQFBdg05+43suW\n\tytiotWonFXpdO0h4Fklhtew2JMf3dgem81o0yOaTnfbZD53ebIL56BEkGFpGbo/RtgV9\n\t+QAAc1Kah2x9Isd5bSZaKvcoufIrxCU9OPOOC2LIV43v92/VOX2IAKd7Lan5tMF4KWIY\n\tWHqSouPYU9tu24lsBExB5hQh7WeCQFWeU+U+egsWQiSzVONKVfgGtaB3O/5LEr7jb2To\n\t03Mg==","X-Gm-Message-State":"AOAM531MDrobKkmYINLqsp/WoluhcnRH2I4v2eKD3am5G3Yb4+JNLfD5\n\tQ/Ica+DBKbntQz3EvvdEkpzxSo1x/5Hh9Wjt7G5X6Q==","X-Google-Smtp-Source":"ABdhPJziUgoBnUV+aO78VUJc4abk3kkdApcXjGgRzCtSqFf0fcoWvUyDQtSZAQjhSogiBm5MVq6XDLqR0MeS8Bkky9E=","X-Received":"by 2002:a5d:5192:: with SMTP id\n\tk18mr19755264wrv.163.1623689731876; \n\tMon, 14 Jun 2021 09:55:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-5-naush@raspberrypi.com>","In-Reply-To":"<20210614095340.3051816-5-naush@raspberrypi.com>","From":"David Plowman ","Date":"Mon, 14 Jun 2021 17:55:20 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser_smia.cpp","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17545,"web_url":"https://patchwork.libcamera.org/comment/17545/","msgid":"","date":"2021-06-15T02:48:33","subject":"Re: [libcamera-devel] [PATCH 4/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser_smia.cpp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Jun 14, 2021 at 10:53:38AM +0100, Naushir Patuck wrote:\n> Adjust source formatting to closer match libcamera guidelines:\n> \n> - Switch to C style comments.\n> - Adjust whitespace for readability.\n> - Remove retcode local variable usage.\n> \n> There are no functional changes in this commit.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/md_parser_smia.cpp | 70 ++++++++++++++------------\n> 1 file changed, 38 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp\n> index 852a1d347d11..65ffbe00c76e 100644\n> --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> @@ -1,31 +1,32 @@\n> /* SPDX-License-Identifier: BSD-2-Clause */\n> /*\n> - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited\n> *\n> - * md_parser.cpp - image sensor metadata parsers\n> + * md_parser_smia.cpp - SMIA specification based embedded data parser\n> */\n> -\n> #include \n> #include \n> -#include \n> +#include \n\nstring doesn't seem to be needed, and neither is map.\n\n> \n> #include \"md_parser.hpp\"\n> \n> using namespace RPiController;\n> \n> -// This function goes through the embedded data to find the offsets (not\n> -// values!), in the data block, where the values of the given registers can\n> -// subsequently be found.\n> -\n> -// Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA\n> -// sensors, I think.\n> +/*\n> + * This function goes through the embedded data to find the offsets (not\n> + * values!), in the data block, where the values of the given registers can\n> + * subsequently be found.\n> + *\n> + * Embedded data tag bytes, from Sony IMX219 datasheet but general to all SMIA\n> + * sensors, I think.\n\nAre they identical to the values used in the MIPI CCS specification ? If\nso, maybe we should rename the parser from SMIA to CCS, as the latter is\na public specification (SMIA is as well but SMIA++ isn't, if I recall\ncorrectly). This can be done later.\n\nReviewed-by: Laurent Pinchart \n\nI can handle the small change above when applying.\n\n> + */\n> \n> -#define LINE_START 0x0a\n> -#define LINE_END_TAG 0x07\n> -#define REG_HI_BITS 0xaa\n> -#define REG_LOW_BITS 0xa5\n> -#define REG_VALUE 0x5a\n> -#define REG_SKIP 0x55\n> +constexpr unsigned int LINE_START = 0x0a;\n> +constexpr unsigned int LINE_END_TAG = 0x07;\n> +constexpr unsigned int REG_HI_BITS = 0xaa;\n> +constexpr unsigned int REG_LOW_BITS = 0xa5;\n> +constexpr unsigned int REG_VALUE = 0x5a;\n> +constexpr unsigned int REG_SKIP = 0x55;\n> \n> MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer,\n> \t\t\t\t\t\t uint32_t regs[], int offsets[],\n> @@ -36,12 +37,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> \tif (buffer[0] != LINE_START)\n> \t\treturn NO_LINE_START;\n> \n> -\tunsigned int current_offset = 1; // after the LINE_START\n> +\tunsigned int current_offset = 1; /* after the LINE_START */\n> \tunsigned int current_line_start = 0, current_line = 0;\n> \tunsigned int reg_num = 0, first_reg = 0;\n> -\tParseStatus retcode = PARSE_OK;\n> +\n> \twhile (1) {\n> \t\tint tag = buffer[current_offset++];\n> +\n> \t\tif ((bits_per_pixel_ == 10 &&\n> \t\t (current_offset + 1 - current_line_start) % 5 == 0) ||\n> \t\t (bits_per_pixel_ == 12 &&\n> @@ -49,34 +51,38 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> \t\t\tif (buffer[current_offset++] != REG_SKIP)\n> \t\t\t\treturn BAD_DUMMY;\n> \t\t}\n> +\n> \t\tint data_byte = buffer[current_offset++];\n> -\t\t//printf(\"Offset %u, tag 0x%02x data_byte 0x%02x\\n\", current_offset-1, tag, data_byte);\n> +\n> \t\tif (tag == LINE_END_TAG) {\n> \t\t\tif (data_byte != LINE_END_TAG)\n> \t\t\t\treturn BAD_LINE_END;\n> +\n> \t\t\tif (num_lines_ && ++current_line == num_lines_)\n> \t\t\t\treturn MISSING_REGS;\n> +\n> \t\t\tif (line_length_bytes_) {\n> -\t\t\t\tcurrent_offset =\n> -\t\t\t\t\tcurrent_line_start + line_length_bytes_;\n> -\t\t\t\t// Require whole line to be in the buffer (if buffer size set).\n> +\t\t\t\tcurrent_offset = current_line_start + line_length_bytes_;\n> +\n> +\t\t\t\t/* Require whole line to be in the buffer (if buffer size set). */\n> \t\t\t\tif (buffer.size() &&\n> -\t\t\t\t current_offset + line_length_bytes_ >\n> -\t\t\t\t\t buffer.size())\n> +\t\t\t\t current_offset + line_length_bytes_ > buffer.size())\n> \t\t\t\t\treturn MISSING_REGS;\n> +\n> \t\t\t\tif (buffer[current_offset] != LINE_START)\n> \t\t\t\t\treturn NO_LINE_START;\n> \t\t\t} else {\n> -\t\t\t\t// allow a zero line length to mean \"hunt for the next line\"\n> +\t\t\t\t/* allow a zero line length to mean \"hunt for the next line\" */\n> \t\t\t\twhile (buffer[current_offset] != LINE_START &&\n> \t\t\t\t current_offset < buffer.size())\n> \t\t\t\t\tcurrent_offset++;\n> +\n> \t\t\t\tif (current_offset == buffer.size())\n> \t\t\t\t\treturn NO_LINE_START;\n> \t\t\t}\n> -\t\t\t// inc current_offset to after LINE_START\n> -\t\t\tcurrent_line_start =\n> -\t\t\t\tcurrent_offset++;\n> +\n> +\t\t\t/* inc current_offset to after LINE_START */\n> +\t\t\tcurrent_line_start = current_offset++;\n> \t\t} else {\n> \t\t\tif (tag == REG_HI_BITS)\n> \t\t\t\treg_num = (reg_num & 0xff) | (data_byte << 8);\n> @@ -86,13 +92,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> \t\t\t\treg_num++;\n> \t\t\telse if (tag == REG_VALUE) {\n> \t\t\t\twhile (reg_num >=\n> -\t\t\t\t // assumes registers are in order...\n> +\t\t\t\t /* assumes registers are in order... */\n> \t\t\t\t regs[first_reg]) {\n> \t\t\t\t\tif (reg_num == regs[first_reg])\n> -\t\t\t\t\t\toffsets[first_reg] =\n> -\t\t\t\t\t\t\tcurrent_offset - 1;\n> +\t\t\t\t\t\toffsets[first_reg] = current_offset - 1;\n> +\n> \t\t\t\t\tif (++first_reg == num_regs)\n> -\t\t\t\t\t\treturn retcode;\n> +\t\t\t\t\t\treturn PARSE_OK;\n> \t\t\t\t}\n> \t\t\t\treg_num++;\n> \t\t\t} else","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A5A9BBD78E\n\tfor ;\n\tTue, 15 Jun 2021 02:48:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28BB568944;\n\tTue, 15 Jun 2021 04:48:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B64D16050D\n\tfor ;\n\tTue, 15 Jun 2021 04:48:54 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35A38E9;\n\tTue, 15 Jun 2021 04:48:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kV4vYskh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623725334;\n\tbh=drJF8wDE6CPbF8CDDy61neCyvDbXbxhC0rw6m6UDtjQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kV4vYskhjdlv6oukwjTf9ZUV0UF1Yi01LJlI5zsgXbVOF7YK1gYnu/uk0WqFbrs8T\n\tFYl+GZxFO0DQJIHiMvLm/LKoASgBM56fQY7tFq2dSOBZqn6AhKSArQYmAVySMTfSw7\n\t00kW2Ofh6keJ12g+STnS1v6OSymzYurwuMO0at0g=","Date":"Tue, 15 Jun 2021 05:48:33 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210614095340.3051816-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 4/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser_smia.cpp","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17557,"web_url":"https://patchwork.libcamera.org/comment/17557/","msgid":"","date":"2021-06-15T08:54:41","subject":"Re: [libcamera-devel] [PATCH 4/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser_smia.cpp","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 15 Jun 2021 at 03:48, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Jun 14, 2021 at 10:53:38AM +0100, Naushir Patuck wrote:\n> > Adjust source formatting to closer match libcamera guidelines:\n> >\n> > - Switch to C style comments.\n> > - Adjust whitespace for readability.\n> > - Remove retcode local variable usage.\n> >\n> > There are no functional changes in this commit.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/md_parser_smia.cpp | 70 ++++++++++++++------------\n> > 1 file changed, 38 insertions(+), 32 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp\n> b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > index 852a1d347d11..65ffbe00c76e 100644\n> > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > @@ -1,31 +1,32 @@\n> > /* SPDX-License-Identifier: BSD-2-Clause */\n> > /*\n> > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited\n> > *\n> > - * md_parser.cpp - image sensor metadata parsers\n> > + * md_parser_smia.cpp - SMIA specification based embedded data parser\n> > */\n> > -\n> > #include \n> > #include \n> > -#include \n> > +#include \n>\n> string doesn't seem to be needed, and neither is map.\n>\n> >\n> > #include \"md_parser.hpp\"\n> >\n> > using namespace RPiController;\n> >\n> > -// This function goes through the embedded data to find the offsets (not\n> > -// values!), in the data block, where the values of the given registers\n> can\n> > -// subsequently be found.\n> > -\n> > -// Embedded data tag bytes, from Sony IMX219 datasheet but general to\n> all SMIA\n> > -// sensors, I think.\n> > +/*\n> > + * This function goes through the embedded data to find the offsets (not\n> > + * values!), in the data block, where the values of the given registers\n> can\n> > + * subsequently be found.\n> > + *\n> > + * Embedded data tag bytes, from Sony IMX219 datasheet but general to\n> all SMIA\n> > + * sensors, I think.\n>\n> Are they identical to the values used in the MIPI CCS specification ? If\n> so, maybe we should rename the parser from SMIA to CCS, as the latter is\n> a public specification (SMIA is as well but SMIA++ isn't, if I recall\n> correctly). This can be done later.\n>\n\nSure, renaming it to CCS seems sensible here.\n\nThanks,\nNaush\n\n\n> Reviewed-by: Laurent Pinchart \n>\n> I can handle the small change above when applying.\n>\n> > + */\n> >\n> > -#define LINE_START 0x0a\n> > -#define LINE_END_TAG 0x07\n> > -#define REG_HI_BITS 0xaa\n> > -#define REG_LOW_BITS 0xa5\n> > -#define REG_VALUE 0x5a\n> > -#define REG_SKIP 0x55\n> > +constexpr unsigned int LINE_START = 0x0a;\n> > +constexpr unsigned int LINE_END_TAG = 0x07;\n> > +constexpr unsigned int REG_HI_BITS = 0xaa;\n> > +constexpr unsigned int REG_LOW_BITS = 0xa5;\n> > +constexpr unsigned int REG_VALUE = 0x5a;\n> > +constexpr unsigned int REG_SKIP = 0x55;\n> >\n> > MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span uint8_t> buffer,\n> > uint32_t regs[], int\n> offsets[],\n> > @@ -36,12 +37,13 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> > if (buffer[0] != LINE_START)\n> > return NO_LINE_START;\n> >\n> > - unsigned int current_offset = 1; // after the LINE_START\n> > + unsigned int current_offset = 1; /* after the LINE_START */\n> > unsigned int current_line_start = 0, current_line = 0;\n> > unsigned int reg_num = 0, first_reg = 0;\n> > - ParseStatus retcode = PARSE_OK;\n> > +\n> > while (1) {\n> > int tag = buffer[current_offset++];\n> > +\n> > if ((bits_per_pixel_ == 10 &&\n> > (current_offset + 1 - current_line_start) % 5 == 0) ||\n> > (bits_per_pixel_ == 12 &&\n> > @@ -49,34 +51,38 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> > if (buffer[current_offset++] != REG_SKIP)\n> > return BAD_DUMMY;\n> > }\n> > +\n> > int data_byte = buffer[current_offset++];\n> > - //printf(\"Offset %u, tag 0x%02x data_byte 0x%02x\\n\",\n> current_offset-1, tag, data_byte);\n> > +\n> > if (tag == LINE_END_TAG) {\n> > if (data_byte != LINE_END_TAG)\n> > return BAD_LINE_END;\n> > +\n> > if (num_lines_ && ++current_line == num_lines_)\n> > return MISSING_REGS;\n> > +\n> > if (line_length_bytes_) {\n> > - current_offset =\n> > - current_line_start +\n> line_length_bytes_;\n> > - // Require whole line to be in the buffer\n> (if buffer size set).\n> > + current_offset = current_line_start +\n> line_length_bytes_;\n> > +\n> > + /* Require whole line to be in the buffer\n> (if buffer size set). */\n> > if (buffer.size() &&\n> > - current_offset + line_length_bytes_ >\n> > - buffer.size())\n> > + current_offset + line_length_bytes_ >\n> buffer.size())\n> > return MISSING_REGS;\n> > +\n> > if (buffer[current_offset] != LINE_START)\n> > return NO_LINE_START;\n> > } else {\n> > - // allow a zero line length to mean \"hunt\n> for the next line\"\n> > + /* allow a zero line length to mean \"hunt\n> for the next line\" */\n> > while (buffer[current_offset] !=\n> LINE_START &&\n> > current_offset < buffer.size())\n> > current_offset++;\n> > +\n> > if (current_offset == buffer.size())\n> > return NO_LINE_START;\n> > }\n> > - // inc current_offset to after LINE_START\n> > - current_line_start =\n> > - current_offset++;\n> > +\n> > + /* inc current_offset to after LINE_START */\n> > + current_line_start = current_offset++;\n> > } else {\n> > if (tag == REG_HI_BITS)\n> > reg_num = (reg_num & 0xff) | (data_byte <<\n> 8);\n> > @@ -86,13 +92,13 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> > reg_num++;\n> > else if (tag == REG_VALUE) {\n> > while (reg_num >=\n> > - // assumes registers are in order...\n> > + /* assumes registers are in\n> order... */\n> > regs[first_reg]) {\n> > if (reg_num == regs[first_reg])\n> > - offsets[first_reg] =\n> > - current_offset - 1;\n> > + offsets[first_reg] =\n> current_offset - 1;\n> > +\n> > if (++first_reg == num_regs)\n> > - return retcode;\n> > + return PARSE_OK;\n> > }\n> > reg_num++;\n> > } else\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6CA0FBD78E\n\tfor ;\n\tTue, 15 Jun 2021 08:55:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8576568943;\n\tTue, 15 Jun 2021 10:54:59 +0200 (CEST)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F011A6892D\n\tfor ;\n\tTue, 15 Jun 2021 10:54:57 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id x14so23901854ljp.7\n\tfor ;\n\tTue, 15 Jun 2021 01:54:57 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"C3fjhDm7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=toDPR5LHhzs0pF/Ad7gg/rMUuQUx/RIjEYwkyvfJbUE=;\n\tb=C3fjhDm7dxb2KJPsKLqbH/wpw80xlPUyjY73mjK92hpY1IYczKHtRfyAgE1pcNXOLm\n\tctjX/nYZrk373w3Yak0cM0z7b2LBQ1yP+lbcMPfxfxTTUNWbo8ItdvkRm8MEDr0NI8wI\n\tjFdBUA/NXET/ISuD0VPh4Yqgkeppm+QWSSX9wm8QiCtrJVvqGL9pqYMbDjmxY/DB2+dR\n\tDg37dlJjdCLbLi+WEDUC+PBkqxpJG85GKJh/AKoQCiy3OYI2zyY4ndYLnzpggJ39L9Zh\n\tHLcLbtLjCHM6nCXNdSSSEHa6kozyPuVAR9R3dlQIetST1UT5jM/nAxn2rlHAaDKnsdNo\n\thVSQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=toDPR5LHhzs0pF/Ad7gg/rMUuQUx/RIjEYwkyvfJbUE=;\n\tb=iNY2Navw0kbptxVha+B5LX3an3ZT2brg6rTYvnnQOCU7GveKplw6fboZuL9GsbUNm0\n\tdTJpDyBMJwwSj5dsXVHK5zAp0CNn5YTIT565uPnmBq9evTmZIj8pDq98xT8rObpEnwL5\n\tR0uIoqwyKQHqk0+DL8wDgCPhcvhqjFlBa1iTI4IZYh7PNjHFceRYDayq+b51uoPmNNx/\n\tRHLDcBmzXC5wqg2SOi2LwenH2IMw6kDG/HJcd0YBH+bRDdwAPNYNy4pYcYVKA3sPau5X\n\tO3kpp+296kmjtNYdyqNilzwENTOPTpcerAZQsQdLgKnqkamR1i7WRe7qjRovBnz/dlVI\n\t5j6A==","X-Gm-Message-State":"AOAM530hd7XDla9YM0ok63+NI5n1vtc4Ia3ZXqSPFyo1Pq+nukQz0eNH\n\tvwDzkCEO0aCC+CuQiWte6rBL+/5DjhL6OSIyAmJYKLX10k4=","X-Google-Smtp-Source":"ABdhPJwBnXAyv4FDtVKxW/j2tEnCi5MOH451nrhRlRxIlyWhYfKrNCp5QBYxndUDudjnCsIgr99QXNtLMml/R29as38=","X-Received":"by 2002:a2e:a230:: with SMTP id\n\ti16mr17257129ljm.169.1623747297304; \n\tTue, 15 Jun 2021 01:54:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-5-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 15 Jun 2021 09:54:41 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000dc737c05c4ca20c7\"","Subject":"Re: [libcamera-devel] [PATCH 4/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser_smia.cpp","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]