Hi Hugo,
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 4e84e90..6c2c3c8 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -500,4 +500,14 @@ config SGI_GRU_DEBUG
> This option enables addition debugging code for the SGI GRU driver. If
> you are unsure, say N.
>
> +config FPGALOAD
> + tristate "FPGA bitstream loader support"
As per the commit text description this is very much specific to
Xilinx, please change the config option name to XILINX_FPGALOAD
please.
> + This option enables support for the FPGA bitstream loader.
> +
Better if we write "Xilinx FPGA bitstream loader".
> diff --git a/drivers/misc/fpgaload.c b/drivers/misc/fpgaload.c
How about renaming C file to xilinx_fpgaload.c?
> new file mode 100644
> index 0000000..d4f2408
> --- /dev/null
> +++ b/drivers/misc/fpgaload.c
> @@ -0,0 +1,430 @@
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/string.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/fpgaload.h>
Ditto. Prefer xilinux_fpgaload.h.
> +#include <linux/delay.h>
> +
> +#include <asm/gpio.h>
> +
> +#define MODULE_NAME "fpgaload"
Here too...
> +
> +/*
> + * Return value:
> + * 0: Error
> + * 1: Full bitstream
> + * 2: Partial bitstream
> + */
May be you want to document this function with kernel-doc style?
> +static int bitstream_parse_header(const u8 *buffer, size_t length,
> + int fpga_family, size_t payload_full_size)
> +{
> +
> +/*
> + * Bitstreams supported: Full or Partial.
> + * Note: Full bitstream that supports partial bitstream must be generated
> with
> + * option Persist = true.
> + */
Ditto ?
> +int fpgaload_bitstream_load(const struct fpgaload_t *fpgaload,
> + const u8 *data, size_t size,
> + int *bitstream_mode)
> +{
> + int k;
> + int retval;
> + int timeout_counter;
> +
> + *bitstream_mode = bitstream_parse_header(data, size,
> + fpgaload->fpga_family,
> + fpgaload->payload_full_size);
> + switch (*bitstream_mode) {
> + case BITSTREAM_MODE_FULL:
> + DBGMSG(" Bitstream type: FULL");
> + /* Toggle PROG_B Pin and wait 300nS before proceeding. */
> + gpio_set_value(fpgaload->program_b, 0);
> + udelay(1);
> +
> + /* Confirm that INIT_B is low */
> + if (gpio_get_value(fpgaload->init_b) != 0) {
> + FAILMSG("Error: INIT_B not LOW when PROG is LOW.");
> + return -EIO;
> + }
> +
> + break;
> + case BITSTREAM_MODE_PARTIAL:
> + DBGMSG(" Bitstream type: PARTIAL");
> + break;
> + case BITSTREAM_MODE_UNKNOWN:
> + default:
> + DBGMSG(" Bitstream type: UNKNOWN");
> + return -EINVAL;
> + break;
> + }
> +
> + /* For partial bitstream, PROGRAM_B is already high. */
> + retval = bitstr_load_make_clock(fpgaload, 3);
> + if (retval < 0)
> + return retval;
> +
> + gpio_set_value(fpgaload->program_b, 1);
> +
> + /* Wait for INIT_B pin to go high. */
> + timeout_counter = 0;
> + while ((gpio_get_value(fpgaload->init_b) == 0) &&
> + (timeout_counter < FPGA_WAIT_TIMEOUT)) {
> + retval = bitstr_load_make_clock(fpgaload, 3);
> + if (retval < 0)
> + return retval;
> +
> + timeout_counter++;
> + }
> +
> + if (timeout_counter == FPGA_WAIT_TIMEOUT) {
> + /* Timeout error. */
> + FAILMSG("Error: timeout while waiting for INIT_B to go
> HIGH.");
> + return -EIO;
Better to return -ETIMEDOUT.
> + }
> +
> + /* Send actual bitstream data to FPGA one byte at a time. */
> + for (k = 0; k < size; k += XFER_SIZE) {
> + retval = fpgaload->write_byte((u8 *) &data[k],
> + XFER_SIZE);
> + if (retval < 0)
> + return retval;
> +
> +#ifdef CHECK_INIT_LOW_DURING_PROG
> + if (gpio_get_value(fpgaload->init_b) == 0) {
> + /* Error if INIT_B goes low during programming. */
> + FAILMSG("Error: INIT_B LOW during programming.");
> + return -EIO;
Ditto.
> + }
> +#endif
> + }
> +
> + /* Pulse the clock line ten times at the end. */
> + retval = bitstr_load_make_clock(fpgaload, 10);
> + if (retval < 0)
> + return retval;
> +
> + /* FPGA DONE pin must go high. */
> + timeout_counter = 0;
> + while ((gpio_get_value(fpgaload->done) == 0) &&
> + (timeout_counter < FPGA_WAIT_TIMEOUT))
> + timeout_counter++;
> +
> + if (gpio_get_value(fpgaload->done) == 0) {
> + /* Timeout error. */
> + FAILMSG("Error: timeout while waiting for DONE to go HIGH.");
> + return -EIO;
Whatever comes after Ditto.
> +
> +int fpgaload_register(const struct fpgaload_t *fpgaload)
> +{
> + int retval;
> +
> + if (fpgaload->write_byte == NULL) {
> + FAILMSG("Error, fpgaload->write_byte() callback not
> defined.");
> + return -EINVAL;
> + }
> +
> + /* Configure FPGA PROGRAM_B GPIO. */
> + retval = gpio_request(fpgaload->program_b, "fpga_program_b");
> + if (retval == 0) /* FPGA_PROGRAM_B must be initially HIGH. */
> + retval = gpio_direction_output(fpgaload->program_b, 1);
> + if (retval != 0)
> + goto gpio_error;
> +
> + /* Configure FPGA INIT_B GPIO. */
> + retval = gpio_request(fpgaload->init_b, "fpga_init_b");
> + if (retval == 0)
> + retval = gpio_direction_input(fpgaload->init_b);
> + if (retval != 0)
> + goto gpio_error;
> +
> + /* Configure FPGA DONE GPIO. */
> + retval = gpio_request(fpgaload->done, "fpga_done");
> + if (retval == 0)
> + retval = gpio_direction_input(fpgaload->done);
> + if (retval != 0)
> + goto gpio_error;
> +
> + return 0;
> +
> +gpio_error:
> + fpgaload_release_gpio(fpgaload);
> + return retval;
> +}
> +EXPORT_SYMBOL(fpgaload_register);
I think it is not register API, but fpgaload_request_gpio, right? I
don't see typical registration stuff in above API.
> +
> +void fpgaload_unregister(const struct fpgaload_t *fpgaload)
> +{
> + fpgaload_release_gpio(fpgaload);
> +}
> +EXPORT_SYMBOL(fpgaload_unregister);
Same here...I don't know why this API is needed.
> +
> +int __init fpgaload_init(void)
> +{
This function should be static.
> + DBGMSG("fpgaload_init()");
> +
> + return 0;
> +}
> +module_init(fpgaload_init);
> +
> +void __exit fpgaload_exit(void)
> +{
Ditto.
Code looks clean otherwise.
--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
