[libcamera-devel] [PATCH 4/6] ipa: raspberrypi: Non-functional formatting fixes to md_parser_smia.cpp

[David Plowman]

Hi Naush

Thanks for this patch.

On Mon, 14 Jun 2021 at 10:53, Naushir Patuck <naush at 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 at 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 at 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
>