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
