[edid-decode] [PATCH 2/2] Calculate DisplayID checksums. Refactor do_checksum.

[walter harms]

Am 10.12.2016 20:44, schrieb Mark Ferry:
> ---
>  edid-decode.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/edid-decode.c b/edid-decode.c
> index c18697f..6df2b6e 100644
> --- a/edid-decode.c
> +++ b/edid-decode.c
> @@ -47,6 +47,7 @@ static int has_range_descriptor = 0;
>  static int has_preferred_timing = 0;
>  static int has_valid_checksum = 1;
>  static int has_valid_cvt = 1;
> +static int has_valid_displayid_checksum = 1;
>  static int has_valid_dummy_block = 1;
>  static int has_valid_week = 0;
>  static int has_valid_year = 0;
> @@ -560,23 +561,27 @@ detailed_block(unsigned char *x, int in_extension)
>      return 1;
>  }
>  
> -static void
> -do_checksum(unsigned char *x)
> +static unsigned char
> +do_checksum(unsigned char *x, size_t len)
>  {
> -    printf("Checksum: 0x%hx", x[0x7f]);
> -    {
> -	unsigned char sum = 0;
> -	int i;
> -	for (i = 0; i < 128; i++)
> -	    sum += x[i];
> -	if (sum) {
> -	    printf(" (should be 0x%hx)", (unsigned char)(x[0x7f] - sum));
> -	    has_valid_checksum = 0;
> -	} else printf(" (valid)");
> -    }
> +    unsigned char sum = 0;
> +    int i;
> +
> +    printf("Checksum: 0x%hx", x[len -1]);
> +
> +    for (i = 0; i < len; i++)
> +        sum += x[i];
> +
> +    if (sum) {
> +        printf(" (should be 0x%hx)", (unsigned char)(x[len-1] - sum));
> +    } else printf(" (valid)");
> +
>      printf("\n");

	any reason not to move the \n to the printf in front ?
	just thinking of it ..
	perhaps this can be reduced to invalid/valid ? i do not know anyone
	seeing more in that information.
	then you can simply return 0/-1.	

> +
> +    return sum;
>  }
>  
> +
>  /* CEA extension */
>  
>  static const char *
> @@ -1281,7 +1286,7 @@ parse_cea(unsigned char *x)
>  		detailed_block(detailed, 1);
>      } while (0);
>  
> -    do_checksum(x);
> +    (void) do_checksum(x, 128);
>  
>      return ret;
>  }
> @@ -1371,6 +1376,9 @@ parse_displayid(unsigned char *x)
>      int ext_count = x[4];
>      int i;
>      printf("Length %d, version %d, extension count %d\n", length, version, ext_count);
> +
> +    has_valid_displayid_checksum = (do_checksum(x+1, length + 5) == 0x0);
> +
>      int offset = 5;
>      while (length > 0) {
>         int tag = x[offset];
> @@ -2037,7 +2045,7 @@ int main(int argc, char **argv)
>  	has_valid_extension_count = 1;
>      }
>  
> -    do_checksum(edid);
> +    (void) do_checksum(edid, 128);
>  
in General i am a fan of checking return values,
why not here ?
and why the magic 128 ?
Perhaps we can avoid the global ?

hope that helps,
re,
 wh

>      x = edid;
>      for (edid_lines /= 8; edid_lines > 1; edid_lines--) {
> @@ -2127,6 +2135,8 @@ int main(int argc, char **argv)
>  	    printf("\tInvalid detailed timing descriptor ordering\n");
>  	if (!has_valid_range_descriptor)
>  	    printf("\tRange descriptor contains garbage\n");
> +	if (!has_valid_displayid_checksum)
> +	    printf("\tBlock has broken DisplayID checksum\n");
>  	if (!has_valid_max_dotclock)
>  	    printf("\tEDID 1.4 block does not set max dotclock\n");
>      }