First time I looked through all the code. I know it's
already been lying around for more than a year, sorry
for that.

It looks nearly ready, although I added some serious
comments. Also, there is still a lot of CamelCase. And
generally, we try to follow the coding style of the
Linux kernel. There is a nice tool to help with that
(checkpatch.pl [1]). That is, if you have perl installed.
I don't use it myself, but let me try...

    ./checkpath.pl --no-tree --max-line-length=112 --file ni845x_spi.c

The first warning is spurious, but the errors should be
checked. And the other warnings might give some nice
hints.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl

View Change

19 comments:

• File flashrom.8.tmpl:

  • Patch Set #17, Line 1144: 1230A2

    Nit, example has only 6 digits.

• File ni845x_spi.c:

  • Patch Set #10, Line 123: %ld

    int32 is coming from the NI's header where it is defined as a long: […]

    Stefan is correct. You shouldn't know what the header file defines. The
    header file could change, for instance if NI would issue a 64-bit version
    of the library. Please either cast `tmp` to the expected type or use
    the `inttypes.h` definitions as Stefan suggested.

• File ni845x_spi.c:

  • Patch Set #17, Line 43: static char error_string_buffer[1024];

    Writing a function around this would probably save a lot lines below
    and the buffer could be local. e.g.

        void ni845x_report_error(const char *const func, const int32 err)
        {
                static char buf[1024];
                ni845xStatusToString(err, sizeof(buf), buf);
    	    msg_perr("%s failed with: %s (%d)\n", func, buf, (int)err);
        }

    Callers could use `__func__` to reduce copy-paste errors:

        ni845x_report_error(__func__, tmp);

  • Patch Set #17, Line 107: * @brief ni845x_spi_open

    Nit, adds no information.

  • Patch Set #17, Line 109: competition

    completion?

  • Patch Set #17, Line 134: "USB%d::0x%04X::0x%04X::%08X::RAW",

    %d expects an `int`, %X an `unsigned int`. If the pointers you pass
    are compatible, is implementation specific.

    A generally portable version would be (assuming bus numbers are >= 0):

        unsigned int usb_bus, vid, device_pid;
        unsigned long int serial_as_number;
        sccanf(resource_name, "USB%u::0x%04X::0x%04X::%08lX::RAW",
               &usb_bus, &vid, &device_pid, &serial_as_number);

  • Patch Set #17, Line 207: IO_voltage_100mV = ((float)IOVoltage_mV / 100.0f);

    Doing the calculation as `float` shouldn't make a difference.

  • Patch Set #17, Line 219: msg_pwarn

    Shouldn't be a warning, msg_pinfo() maybe?

  • Patch Set #17, Line 226: if (selected_voltage_100mV == 0) {

    This looks odd. I would have expect it to select 1.2V automatically here.
    The loop above doesn't look like it's possible to select 1.2V.

  • Patch Set #17, Line 266: Set

    Get

  • Patch Set #17, Line 301: &usb_bus, &vid, &pid, &serial_as_number);

    Check return code?

  • Patch Set #17, Line 317: ni845xFindDeviceNext(device_find_handle, resource_handle);

    Check return code?

  • Patch Set #17, Line 342: CS_number = atoi(CS_str);

    atoi() isn't a good choice, it would return a valid `0` on any error.

    One good pattern is to use strtoul() and check `endptr`, e.g.:

        cs_number = strtoul(cs_str, &endptr, 0);
        if (!*cs_str || *endptr || cs_number < 0 || 7 < cs_number) {
                return 1;
        }

    In this case, however, as there is only a single digit to parse, this
    should be safe too:

        cs_number = cs_str[0] - '0';
        if (cs_str[1] || cs_number < 0 || 7 < cs_number) {
                return 1;
        }

  • Patch Set #17, Line 345: return 1;

    leaking `CS_str`

  • Patch Set #17, Line 382: if (serial_number)

    Not needed, as `free(NULL);` is valid.

  • Patch Set #17, Line 391: atoi

    atoi(), see above.

  • Patch Set #17, Line 399:

    None of the error paths (early `return`s) after opening the device
    closes the device. Maybe just call ni845x_spi_shutdown() directly,
    in case.

    It would be nice to sort the code blocks here: 1. do all the parameter
    parsing, 2. open the device and apply the settings. This should also ease
    the error path handling.

  • Patch Set #17, Line 441: msg_cinfo("FAILED.\n");

    Why the second message?

  • Patch Set #17, Line 449: (uInt32)

    the cast seems unnecessary

To view, visit change 25683. To unsubscribe, or for help writing mail filters, visit settings.