Hi Alan, On 11/30/2016 09:45 AM, atull wrote: > On Wed, 30 Nov 2016, Joshua Clayton wrote: > > Hi Clayton, > > I just have a few minor one line changes below. Only one > is operational, I should have caught that earlier. > Thanks for the speedy review. >> +}; >> +MODULE_DEVICE_TABLE(of, of_ef_match); >> + >> +static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr) >> +{ >> + return mgr->state; >> +} > This function gets called once to initialize mgr->state in > fpga_mgr_register(). So it should at least return the state the FPGA > is at then. If it is unknown, it can just return > FPGA_MGR_STATE_UNKNOWN. > I guess I didn't understand the purpose of this function. The driver has access to the status pin at this phase, so I can return FPGA_MGR_STATE_UNKNOWN or FPGA_MGR_STATE_RESET depending on the state of that pin. >> + >> +static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags, >> + const char *buf, size_t count) > Minor, but please fix the indentation of 'const' to match that of > 'struct' above. checkpatch.pl is probably issuing warnings > about that. I double checked. The indentation is correct here. It only has The appearance of being off by one due to the diff format. >> +{ >> + struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; >> + int i; >> + >> + if (flags & FPGA_MGR_PARTIAL_RECONFIG) { >> + dev_err(&mgr->dev, "Partial reconfiguration not supported.\n"); >> + return -EINVAL; >> + } >> + >> + gpiod_set_value(conf->config, 0); >> + usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20); >> + if (gpiod_get_value(conf->status) == 1) { >> + dev_err(&mgr->dev, "Status pin should be low.\n"); >> + return -EIO; >> + } >> + >> + gpiod_set_value(conf->config, 1); >> + for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) { >> + usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20); >> + if (gpiod_get_value(conf->status)) >> + return 0; >> + } >> + >> + dev_err(&mgr->dev, "Status pin not ready.\n"); >> + return -EIO; >> +} >> + >> +static void rev_buf(void *buf, size_t len) >> +{ >> + u32 *fw32 = (u32 *)buf; >> + const u32 *fw_end = (u32 *)(buf + len); >> + >> + /* set buffer to lsb first */ >> + while (fw32 < fw_end) { >> + *fw32 = bitrev8x4(*fw32); >> + fw32++; >> + } >> +} >> + >> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf, >> + size_t count) > Please fix alignment here also. Same as above. Indentation is OK. I'll get a v4 turned around soon. Thanks, Joshua -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html