Messages in this thread | | | Subject | Re: [PATCH 2/2] fpga: lattice machxo2: Add Lattice MachXO2 support | From | Joe Perches <> | Date | Wed, 21 Mar 2018 11:04:52 -0700 |
| |
On Wed, 2018-03-21 at 18:35 +0100, Paolo Pisati wrote: > This patch adds support to the FPGA manager for programming > MachXO2 device’s internal flash memory, via slave SPI.
style trivia:
> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c [] > +static int get_status(struct spi_device *spi, unsigned long *status) > +{ > + struct spi_message msg; > + struct spi_transfer rx, tx; > + u8 cmd[] = LSC_READ_STATUS;
static const u8 cmd[] here and everywhere else as all the tx_buf assignments of cmd are to const void *
> + int ret; > + > + memset(&rx, 0, sizeof(rx)); > + memset(&tx, 0, sizeof(tx)); > + tx.tx_buf = cmd;
[]
> +#ifdef DEBUG > +static void dump_status_reg(unsigned long *status) > +{
Instead of multiple declarations of dump_status_reg it's frequently nicer to use a style like
static void debug_func(args...) { #ifdef DEBUG [code...] #endif }
so if function arguments ever need to be changed it's only required to be changed in one spot and not multiply compilation tested with and without the DEBUG definition
> + char *ferr; > + > + switch (get_err(status)) { > + case ENOERR: > + ferr = "No Error"; > + break; > + case EID: > + ferr = "ID ERR"; > + break; > + case ECMD: > + ferr = "CMD ERR"; > + break; > + case ECRC: > + ferr = "CRC ERR"; > + break; > + case EPREAM: > + ferr = "Preamble ERR"; > + break; > + case EABRT: > + ferr = "Abort ERR"; > + break; > + case EOVERFL: > + ferr = "Overflow ERR"; > + break; > + case ESDMEOF: > + ferr = "SDM EOF"; > + break; > + default: > + ferr = "Default switch case"; > + }
It's frequently nicer to use a static function for these enum -> string conversions like:
static const char *get_err_string(unsigned long err) { switch (err) { case ENOERR: return "No Error"; case EID: return "ID ERR"; case ECMD: return "CMD ERR"; [...] } return "default switch case"; }
> + pr_debug("machxo2 status: 0x%08lX - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n", > + *status, test_bit(DONE, status), test_bit(ENAB, status), > + test_bit(BUSY, status), test_bit(FAIL, status), > + test_bit(DVER, status), ferr);
So instead of ferr, this could use get_err_string(*status)
And please try to keep a consistent alignment for indentation of multiple line statements
> +} > +#else > +static void dump_status_reg(unsigned long *status) {} > +#endif > + > +static int wait_until_not_busy(struct spi_device *spi) > +{ > + unsigned long status; > + int ret, loop = 0; > + > + do { > + ret = get_status(spi, &status); > + if (ret) > + break; > + if (++loop >= MACHXO2_MAX_BUSY_LOOP) { > + ret = -EBUSY; > + break; > + } > + } while (test_bit(BUSY, &status)); > + > + return ret; > +}
Direct returns are OK and would shorten the function line count.
| |