Attention is currently required from: ChrisEric1 CECL, Nicholas Chin.

View Change

14 comments:

• Patchset:

  • Patch Set #3:

    Hi Cristopher,
    fist of all: Thank you for contributing! This looks already quite good. Nevertheless I've a few thinks (most of them are quite small):

    * Can you move the changes in `flashchips.c` into an own commit
    * The entry for the meson buildsystem ist missing (meson.build:148ff, in alphabetic order)
    ```
    'vl805' : {
      'systems' : systems_hwaccess,
      'deps'    : [ libpci ],
      'groups'  : [ group_pci, group_internal ],
      'srcs'    : files('vl805.c', 'pcidev.c'),
      'flags'   : [ '-DCONFIG_VL805=1' ],
     },
    ```
    * Can you share a little bit about the 'original driver' and the 'logs' you mentioned in the comments?

• File flashrom.8.tmpl:

  • Patch Set #3, Line 1371: VL806

    Is there a different between VL805 and VL806? This is the only mention of the second chip

• File vl805.c:

  • Patch Set #3, Line 4: * Copyright (C) 2019, 2020 Carl-Daniel Hailfinger

    This is your copyright

  • Patch Set #3, Line 27: //#include "hwaccess.h"

    Please remove this comment.

  • Patch Set #3, Line 28: #include "hwaccess_x86_io.h"

    This header is not needed, because the programmer doesn't do anything with I/O ports.
    (see line 137)

  • Patch Set #3, Line 31: extern const struct dev_entry devs_vl805[];

    This is not needed.

  • Patch Set #3, Line 33: const struct dev_entry devs_vl805[] = {

    Make this `static`. It is only used in this file.

  • Patch Set #3, Line 38: static struct pci_dev *dev = NULL;

    Can you please move this global into a programmer data struct, like it is done on e.g. the it8212.c programmer.

  • Patch Set #3, Line 53: /* Some of the registers have unknown purpose and are just used inside the init sequence replay. */

    Can you put these defines below the includes, ontop of all functions?

  • Patch Set #3, Line 72:

    uint32_t indata = 0;
    	unsigned int curwritecnt = 0;
    	unsigned int curreadcnt = 0;
    

    These variables don't need to be initialized.

  • Patch Set #3, Line 107: static const struct spi_master spi_master_vl805 = {

    Can this controller handle SPI flashes with 4-byte addresses? Then you should add `.features = SPI_MASTER_4BA`

  • Patch Set #3, Line 131: int vl805_init(const struct programmer_cfg *cfg);

    This prototype is not needed if the function is static.

  • Patch Set #3, Line 133: int vl805_init(const struct programmer_cfg *cfg)

    this should be `static`

  • Patch Set #3, Line 137:

    #if defined(__i386__) || defined(__x86_64__)
    	if (rget_io_perms())
    		return 1;
    	#endif
    

    This can be removed since the programmer doesn't deal with I/O ports

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