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

[Laurent Pinchart]

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 at 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 at 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