Hello,
I'm in a code review mood, and you're the lucky person.
Feedback below.
On Wed, November 3, 2010 15:21, Baruch Siach wrote:
> From: Alex Gershgorin <[email protected]>
>
> The active serial protocol can be used to program Altera Cyclone FPGA devices.
> This driver uses the kernel gpio interface to implement the active serial
> protocol.
>
> Signed-off-by: Alex Gershgorin <[email protected]>
> Signed-off-by: Baruch Siach <[email protected]>
> ---
> drivers/misc/Kconfig | 10 ++
> drivers/misc/Makefile | 1 +
> drivers/misc/cyclone_as.c | 326++++++++++++++++++++++++++++++++++++++++++
> include/linux/cyclone_as.h | 23 +++
> 4 files changed, 360 insertions(+), 0 deletions(-)
> create mode 100644 drivers/misc/cyclone_as.c
> create mode 100644 include/linux/cyclone_as.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index b743312..182328e 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -390,6 +390,16 @@ config BMP085
> To compile this driver as a module, choose M here: the
> module will be called bmp085.
>
> +config CYCLONE_AS
> + tristate "Altera Cyclone Active Serial driver"
> + help
> + Provides support for active serial programming of Altera Cyclone
> + devices. For the active serial protocol details see the Altera
> + "Serial Configuration Devices" document (C51014).
> +
> + To compile this driver as a module, choose M here: the
> + module will be called cyclone_as.
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 42eab95..a3e0248 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -35,3 +35,4 @@ obj-y += eeprom/
> obj-y += cb710/
> obj-$(CONFIG_VMWARE_BALLOON) += vmw_balloon.o
> obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o
> +obj-$(CONFIG_CYCLONE_AS) += cyclone_as.o
> diff --git a/drivers/misc/cyclone_as.c b/drivers/misc/cyclone_as.c
> new file mode 100644
> index 0000000..bfaa60d
> --- /dev/null
> +++ b/drivers/misc/cyclone_as.c
> @@ -0,0 +1,326 @@
> +/*
> + * Copyright 2010 Alex Gershgorin, Orex Computed Radiography
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/err.h>
> +#include <linux/cdev.h>
> +#include <linux/gpio.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/uaccess.h>
> +#include <linux/cyclone_as.h>
> +#include <linux/platform_device.h>
> +
> +/* Active Serial Instruction Set */
> +#define AS_WRITE_ENABLE 0x06
> +#define AS_WRITE_DISABLE 0x04
> +#define AS_READ_STATUS 0x05
> +#define AS_WRITE_STATUS 0x01
> +#define AS_READ_BYTES 0x03
> +#define AS_FAST_READ_BYTES 0x0B
> +#define AS_PAGE_PROGRAM 0x02
> +#define AS_ERASE_SECTOR 0xD8
> +#define AS_ERASE_BULK 0xC7
> +#define AS_READ_SILICON_ID 0xAB
> +#define AS_CHECK_SILICON_ID 0x9F
> +
> +#define AS_PAGE_SIZE 256
> +
> +#define AS_MAX_DEVS 16
> +
> +#define ASIO_DATA 0
> +#define ASIO_NCONFIG 1
> +#define ASIO_DCLK 2
> +#define ASIO_NCS 3
> +#define ASIO_NCE 4
> +#define AS_GPIO_NUM 5
> +
> +#define AS_ALIGNED(x) IS_ALIGNED(x, AS_PAGE_SIZE)
> +
> +static struct class *cyclone_as_class;
> +static unsigned cyclone_as_major;
> +static DECLARE_BITMAP(cyclone_as_devs, AS_MAX_DEVS);
> +
> +struct cyclone_as_device {
> + struct cdev cdev;
> + struct device *dev;
> + struct mutex open_lock;
> + struct gpio gpios[AS_GPIO_NUM];
> +};
> +
> +static void xmit_byte(struct gpio *gpios, char c, char lsb_first)
> +{
> + int i;
> +
> + for (i = 0; i < 8; i++) {
> + gpio_set_value(gpios[ASIO_DATA].gpio,
> + lsb_first ? (c >> i) & 1 : (c << i) & 0x80);
> +
> + gpio_set_value(gpios[ASIO_DCLK].gpio, 0);
> + ndelay(40);
> + gpio_set_value(gpios[ASIO_DCLK].gpio, 1);
> + ndelay(20);
> + }
> +}
I think it would clear up the code a lot if you introduced a xmit_bytes()
function which does the above for variable lengths, as well as the ASIO_NCS
fiddling. Also pass in drvdata and get the gpios from there in this function,
you never have the one without the other.
Only thing preventing this is the lsb_first thing. That is only needed for
the page data itself, so I'd get rid of that option (always 0) and instead
introduce a function swap_page_byte_bits() or something and call that for
the page data before sending it all sequentially in one go. It can use
bitrev8() from bitrev.h for the actual bits swapping.
> +
> +static int cyclone_as_open(struct inode *inode, struct file *file)
> +{
> + int ret;
> + struct cyclone_as_device *drvdata;
> +
> + drvdata = container_of(inode->i_cdev, struct cyclone_as_device, cdev);
> + ret = mutex_trylock(&drvdata->open_lock);
> + if (ret == 0)
> + return -EBUSY;
> +
> + file->private_data = drvdata;
> +
> + ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
> + if (ret < 0) {
> + mutex_unlock(&drvdata->open_lock);
> + return ret;
> + }
> + ndelay(300);
This delay looks redundant.
> +
> + dev_dbg(drvdata->dev,
> + "data: %d, nconfig: %d, dclk: %d, ncs: %d, nce: %d\n",
> + drvdata->gpios[ASIO_DATA].gpio,
> + drvdata->gpios[ASIO_NCONFIG].gpio,
> + drvdata->gpios[ASIO_DCLK].gpio,
> + drvdata->gpios[ASIO_NCS].gpio,
> + drvdata->gpios[ASIO_NCE].gpio);
Can't you get the same info from somewhere in /sys, or from the gpio
debug prints? If so, consider leaving this away.
> +
> + return 0;
> +}
You don't unlock open_lock, so this supports only one user at the time?
> +
> +static ssize_t cyclone_as_write(struct file *file, const char *buf,
> + size_t count, loff_t *ppos)
> +{
> + int i, ret;
> + u8 *page_buf;
> + ssize_t len = count;
> + struct cyclone_as_device *drvdata = file->private_data;
> + unsigned page_count;
unsigned short?
> +
> + if (len < 0)
> + return -EFAULT;
> +
> + if (len == 0)
> + return 0;
> +
> + /* writes must be page aligned */
> + if (!AS_ALIGNED(*ppos) || !AS_ALIGNED(count))
> + return -EINVAL;
> + page_count = *ppos / AS_PAGE_SIZE;
> +
> + page_buf = kmalloc(AS_PAGE_SIZE, GFP_KERNEL);
> + if (page_buf == NULL)
> + return -ENOMEM;
> +
> + if (*ppos == 0) {
> + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> + xmit_byte(drvdata->gpios, AS_WRITE_ENABLE, 0);
> + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
> + ndelay(300);
> +
> + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> + xmit_byte(drvdata->gpios, AS_ERASE_BULK, 0);
> + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
> + msleep_interruptible(5000);
Is 5s always enough? Is there some way to check if it's really done?
> + }
> +
> + while (len) {
> + dev_dbg(drvdata->dev, "offset = 0x%p\n", buf);
> +
> + ret = copy_from_user(page_buf, buf, AS_PAGE_SIZE);
> + if (ret == 0) {
> + buf += AS_PAGE_SIZE;
> + *ppos += AS_PAGE_SIZE;
> + len -= AS_PAGE_SIZE;
> + } else {
> + kfree(page_buf);
> + return -EFAULT;
> + }
Maybe check the whole range before sending any data?
> +
> + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> + xmit_byte(drvdata->gpios, AS_WRITE_ENABLE, 0);
> + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
> + ndelay(300);
> +
> + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> + xmit_byte(drvdata->gpios, AS_PAGE_PROGRAM, 0);
> + xmit_byte(drvdata->gpios, (page_count >> 8) & 0xff, 0);
> + xmit_byte(drvdata->gpios, page_count & 0xff, 0);
> + xmit_byte(drvdata->gpios, 0, 0);
> + ndelay(300);
This delay looks redundant, sure it's needed when you're not fiddling
with ASIO_NCS?
> +
> + for (i = 0; i < AS_PAGE_SIZE; i++)
> + xmit_byte(drvdata->gpios, page_buf[i], 1);
> +
> + ndelay(300);
> + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
Everywhere else the delay is after setting the NCS. Just weird
protocol or a mistake?
> + page_count++;
> + mdelay(5);
> + }
> +
> + kfree(page_buf);
> + return count;
> +}
> +
> +static int cyclone_as_release(struct inode *inode, struct file *file)
> +{
> + struct cyclone_as_device *drvdata = file->private_data;
> + int i;
> +
> + gpio_set_value(drvdata->gpios[ASIO_NCONFIG].gpio, 1);
> + gpio_set_value(drvdata->gpios[ASIO_NCE].gpio, 0);
> + gpio_set_value(drvdata->gpios[ASIO_DCLK].gpio, 0);
> + ndelay(500);
> +
> + for (i = 0; i < ARRAY_SIZE(drvdata->gpios); i++)
> + gpio_direction_input(drvdata->gpios[i].gpio);
> + gpio_free_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
> + mutex_unlock(&drvdata->open_lock);
> +
> + return 0;
> +}
> +
> +static const struct file_operations cyclone_as_fops = {
> + .open = cyclone_as_open,
> + .write = cyclone_as_write,
> + .release = cyclone_as_release,
> +};
> +
> +static int __init cyclone_as_probe(struct platform_device *pdev)
> +{
> + int ret, minor;
> + struct cyclone_as_device *drvdata;
> + struct device *dev;
> + struct cyclone_as_platform_data *pdata = pdev->dev.platform_data;
> +
> + drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->dev = &pdev->dev;
> +
> + cdev_init(&drvdata->cdev, &cyclone_as_fops);
> + drvdata->cdev.owner = THIS_MODULE;
Isn't that already set by cdev_init? Hmm, apparently not, perhaps it should?
> +
> + minor = find_first_zero_bit(cyclone_as_devs, AS_MAX_DEVS);
> + if (minor >= AS_MAX_DEVS)
> + return -EBUSY;
> + ret = cdev_add(&drvdata->cdev, MKDEV(cyclone_as_major, minor), 1);
> + if (ret < 0)
> + return ret;
> + set_bit(minor, cyclone_as_devs);
> +
> + dev = device_create(cyclone_as_class, &pdev->dev,
> + MKDEV(cyclone_as_major, minor), drvdata,
> + "%s%d", "cyclone_as", minor);
Shouldn't you do this at the very end?
> + if (IS_ERR(dev)) {
> + ret = PTR_ERR(dev);
> + goto cdev_del;
> + }
> +
> + mutex_init(&drvdata->open_lock);
> +
> + drvdata->gpios[ASIO_DATA].gpio = pdata->data;
> + drvdata->gpios[ASIO_DATA].flags = GPIOF_OUT_INIT_LOW;
> + drvdata->gpios[ASIO_DATA].label = "as_data";
> + drvdata->gpios[ASIO_NCONFIG].gpio = pdata->nconfig;
> + drvdata->gpios[ASIO_NCONFIG].flags = GPIOF_OUT_INIT_LOW;
> + drvdata->gpios[ASIO_NCONFIG].label = "as_nconfig";
> + drvdata->gpios[ASIO_DCLK].gpio = pdata->dclk;
> + drvdata->gpios[ASIO_DCLK].flags = GPIOF_OUT_INIT_LOW;
> + drvdata->gpios[ASIO_DCLK].label = "as_dclk";
> + drvdata->gpios[ASIO_NCS].gpio = pdata->ncs;
> + drvdata->gpios[ASIO_NCS].flags = GPIOF_OUT_INIT_HIGH;
> + drvdata->gpios[ASIO_NCS].label = "as_ncs";
> + drvdata->gpios[ASIO_NCE].gpio = pdata->nce;
> + drvdata->gpios[ASIO_NCE].flags = GPIOF_OUT_INIT_HIGH;
> + drvdata->gpios[ASIO_NCE].label = "as_nce";
...at least after this bit?
> +
> + dev_info(drvdata->dev, "Altera Cyclone Active Serial driver\n");
> +
> + return 0;
> +
> +cdev_del:
> + clear_bit(minor, cyclone_as_devs);
> + cdev_del(&drvdata->cdev);
> + return ret;
> +}
> +
> +static int __devexit cyclone_as_remove(struct platform_device *pdev)
> +{
> + struct cyclone_as_device *drvdata = platform_get_drvdata(pdev);
> + int minor = MINOR(drvdata->cdev.dev);
> +
> + device_destroy(cyclone_as_class, MKDEV(cyclone_as_major, minor));
> + clear_bit(minor, cyclone_as_devs);
> + cdev_del(&drvdata->cdev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver cyclone_as_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "cyclone_as",
> + },
> + .remove = __devexit_p(cyclone_as_remove),
> +};
> +
> +int __init cyclone_as_init(void)
> +{
> + int err;
> + dev_t dev;
> +
> + cyclone_as_class = class_create(THIS_MODULE, "cyclone_as");
> + if (IS_ERR(cyclone_as_class)) {
> + err = PTR_ERR(cyclone_as_class);
> + goto out;
> + }
> +
> + err = alloc_chrdev_region(&dev, 0, AS_MAX_DEVS, "cyclone_as");
> + if (err < 0)
> + goto class_destroy;
> + cyclone_as_major = MAJOR(dev);
> +
> + err = platform_driver_probe(&cyclone_as_driver, cyclone_as_probe);
> + if (err < 0)
> + goto chr_remove;
> +
> + return 0;
> +
> +chr_remove:
> + unregister_chrdev_region(dev, AS_MAX_DEVS);
> +class_destroy:
> + class_destroy(cyclone_as_class);
> +out:
> + return err;
> +}
> +
> +void __exit cyclone_as_cleanup(void)
> +{
> + platform_driver_unregister(&cyclone_as_driver);
> + unregister_chrdev_region(MKDEV(cyclone_as_major, 0), AS_MAX_DEVS);
> + class_destroy(cyclone_as_class);
> +}
> +
> +module_init(cyclone_as_init);
> +module_exit(cyclone_as_cleanup);
> +
> +MODULE_AUTHOR("Alex Gershgorin <[email protected]>");
> +MODULE_DESCRIPTION("Altera Cyclone Active Serial driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/cyclone_as.h b/include/linux/cyclone_as.h
> new file mode 100644
> index 0000000..cf86955
> --- /dev/null
> +++ b/include/linux/cyclone_as.h
> @@ -0,0 +1,23 @@
> +/*
> + * Copyright 2010 Alex Gershgorin, Orex Computed Radiography
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
> + */
> +
> +#ifndef __ALTERA_PROG_H
> +#define __ALTERA_PROG_H
> +
> +struct cyclone_as_platform_data {
> + unsigned data;
> + unsigned nconfig;
> + unsigned dclk;
> + unsigned ncs;
> + unsigned nce;
> +};
> +
> +#endif /* __ALTERA_PROG_H */
I know other drivers put their header files in include/linux/ too,
but is there any reason to? This seems all internal to cyclone_as.c,
why not have no header file?
Probably not high on your list, but what about suspend support?
Preventing suspend from succeeding seems like a good idea when
stuff is going on.
Greetings,
Indan
Hi Indan,
On Wed, Nov 03, 2010 at 06:13:58PM +0100, Indan Zupancic wrote:
> Hello,
>
> I'm in a code review mood, and you're the lucky person.
> Feedback below.
>
> On Wed, November 3, 2010 15:21, Baruch Siach wrote:
> > From: Alex Gershgorin <[email protected]>
> >
> > The active serial protocol can be used to program Altera Cyclone FPGA devices.
> > This driver uses the kernel gpio interface to implement the active serial
> > protocol.
> >
> > Signed-off-by: Alex Gershgorin <[email protected]>
> > Signed-off-by: Baruch Siach <[email protected]>
> > ---
[snip]
> > +static void xmit_byte(struct gpio *gpios, char c, char lsb_first)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < 8; i++) {
> > + gpio_set_value(gpios[ASIO_DATA].gpio,
> > + lsb_first ? (c >> i) & 1 : (c << i) & 0x80);
> > +
> > + gpio_set_value(gpios[ASIO_DCLK].gpio, 0);
> > + ndelay(40);
> > + gpio_set_value(gpios[ASIO_DCLK].gpio, 1);
> > + ndelay(20);
> > + }
> > +}
>
> I think it would clear up the code a lot if you introduced a xmit_bytes()
> function which does the above for variable lengths, as well as the ASIO_NCS
> fiddling.
I'll consider this for the next version. Keep in mind that we send a whole
command during nCS assertion, not arbitrary buffers. So, in the case of the
PAGE_PROGRAM command, we'll need to prepend the instruction and its address
argument to the page data. I'm not sure we end up with something more clear
than we have now.
> Also pass in drvdata and get the gpios from there in this function,
> you never have the one without the other.
So? I want to avoid the 'drvdata->' prefix when it's not needed.
> Only thing preventing this is the lsb_first thing. That is only needed for
> the page data itself, so I'd get rid of that option (always 0) and instead
> introduce a function swap_page_byte_bits() or something and call that for
> the page data before sending it all sequentially in one go. It can use
> bitrev8() from bitrev.h for the actual bits swapping.
Thanks for the tip. I plan to use bitrev8() at the caller function and remove
this argument from xmit_byte().
> > +static int cyclone_as_open(struct inode *inode, struct file *file)
> > +{
> > + int ret;
> > + struct cyclone_as_device *drvdata;
> > +
> > + drvdata = container_of(inode->i_cdev, struct cyclone_as_device, cdev);
> > + ret = mutex_trylock(&drvdata->open_lock);
> > + if (ret == 0)
> > + return -EBUSY;
> > +
> > + file->private_data = drvdata;
> > +
> > + ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
> > + if (ret < 0) {
> > + mutex_unlock(&drvdata->open_lock);
> > + return ret;
> > + }
> > + ndelay(300);
>
> This delay looks redundant.
Right.
> > +
> > + dev_dbg(drvdata->dev,
> > + "data: %d, nconfig: %d, dclk: %d, ncs: %d, nce: %d\n",
> > + drvdata->gpios[ASIO_DATA].gpio,
> > + drvdata->gpios[ASIO_NCONFIG].gpio,
> > + drvdata->gpios[ASIO_DCLK].gpio,
> > + drvdata->gpios[ASIO_NCS].gpio,
> > + drvdata->gpios[ASIO_NCE].gpio);
>
> Can't you get the same info from somewhere in /sys, or from the gpio
> debug prints? If so, consider leaving this away.
The debugfs gpio file gives this information. I'll drop it.
> > +
> > + return 0;
> > +}
>
> You don't unlock open_lock, so this supports only one user at the time?
Correct. I don't think anything good will come out of concurrent writes to the
FPGA, and read is not supported.
> > +static ssize_t cyclone_as_write(struct file *file, const char *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + int i, ret;
> > + u8 *page_buf;
> > + ssize_t len = count;
> > + struct cyclone_as_device *drvdata = file->private_data;
> > + unsigned page_count;
>
> unsigned short?
The largest chip currently supporting this protocol (EPCS128) has 2^16 pages,
but the protocol allocates 3 address bytes. So why limit ourselves? Currently
the code only uses the 16 LSBs, I'll change this.
> > + if (len < 0)
> > + return -EFAULT;
> > +
> > + if (len == 0)
> > + return 0;
> > +
> > + /* writes must be page aligned */
> > + if (!AS_ALIGNED(*ppos) || !AS_ALIGNED(count))
> > + return -EINVAL;
> > + page_count = *ppos / AS_PAGE_SIZE;
> > +
> > + page_buf = kmalloc(AS_PAGE_SIZE, GFP_KERNEL);
> > + if (page_buf == NULL)
> > + return -ENOMEM;
> > +
> > + if (*ppos == 0) {
> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> > + xmit_byte(drvdata->gpios, AS_WRITE_ENABLE, 0);
> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
> > + ndelay(300);
> > +
> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> > + xmit_byte(drvdata->gpios, AS_ERASE_BULK, 0);
> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
> > + msleep_interruptible(5000);
>
> Is 5s always enough? Is there some way to check if it's really done?
You are right. 5 seconds are definitely not enough for the larger chips
(typical time of 105 seconds for EPCS128). I'll poll the "write in progress"
bit instead. More code to write :(.
> > + }
> > +
> > + while (len) {
> > + dev_dbg(drvdata->dev, "offset = 0x%p\n", buf);
> > +
> > + ret = copy_from_user(page_buf, buf, AS_PAGE_SIZE);
> > + if (ret == 0) {
> > + buf += AS_PAGE_SIZE;
> > + *ppos += AS_PAGE_SIZE;
> > + len -= AS_PAGE_SIZE;
> > + } else {
> > + kfree(page_buf);
> > + return -EFAULT;
> > + }
>
> Maybe check the whole range before sending any data?
What is there to check?
> > +
> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> > + xmit_byte(drvdata->gpios, AS_WRITE_ENABLE, 0);
> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
> > + ndelay(300);
> > +
> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> > + xmit_byte(drvdata->gpios, AS_PAGE_PROGRAM, 0);
> > + xmit_byte(drvdata->gpios, (page_count >> 8) & 0xff, 0);
> > + xmit_byte(drvdata->gpios, page_count & 0xff, 0);
> > + xmit_byte(drvdata->gpios, 0, 0);
> > + ndelay(300);
>
> This delay looks redundant, sure it's needed when you're not fiddling
> with ASIO_NCS?
Probably not.
> > + for (i = 0; i < AS_PAGE_SIZE; i++)
> > + xmit_byte(drvdata->gpios, page_buf[i], 1);
> > +
> > + ndelay(300);
> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
>
> Everywhere else the delay is after setting the NCS. Just weird
> protocol or a mistake?
Mistake.
> > + page_count++;
> > + mdelay(5);
> > + }
> > +
> > + kfree(page_buf);
> > + return count;
> > +}
[snip
> > +static int __init cyclone_as_probe(struct platform_device *pdev)
> > +{
> > + int ret, minor;
> > + struct cyclone_as_device *drvdata;
> > + struct device *dev;
> > + struct cyclone_as_platform_data *pdata = pdev->dev.platform_data;
> > +
> > + drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
> > + if (!drvdata)
> > + return -ENOMEM;
> > +
> > + drvdata->dev = &pdev->dev;
> > +
> > + cdev_init(&drvdata->cdev, &cyclone_as_fops);
> > + drvdata->cdev.owner = THIS_MODULE;
>
> Isn't that already set by cdev_init? Hmm, apparently not, perhaps it should?
>
> > +
> > + minor = find_first_zero_bit(cyclone_as_devs, AS_MAX_DEVS);
> > + if (minor >= AS_MAX_DEVS)
> > + return -EBUSY;
> > + ret = cdev_add(&drvdata->cdev, MKDEV(cyclone_as_major, minor), 1);
> > + if (ret < 0)
> > + return ret;
> > + set_bit(minor, cyclone_as_devs);
> > +
> > + dev = device_create(cyclone_as_class, &pdev->dev,
> > + MKDEV(cyclone_as_major, minor), drvdata,
> > + "%s%d", "cyclone_as", minor);
>
> Shouldn't you do this at the very end?
Sounds reasonable.
> > + if (IS_ERR(dev)) {
> > + ret = PTR_ERR(dev);
> > + goto cdev_del;
> > + }
> > +
> > + mutex_init(&drvdata->open_lock);
> > +
> > + drvdata->gpios[ASIO_DATA].gpio = pdata->data;
> > + drvdata->gpios[ASIO_DATA].flags = GPIOF_OUT_INIT_LOW;
> > + drvdata->gpios[ASIO_DATA].label = "as_data";
> > + drvdata->gpios[ASIO_NCONFIG].gpio = pdata->nconfig;
> > + drvdata->gpios[ASIO_NCONFIG].flags = GPIOF_OUT_INIT_LOW;
> > + drvdata->gpios[ASIO_NCONFIG].label = "as_nconfig";
> > + drvdata->gpios[ASIO_DCLK].gpio = pdata->dclk;
> > + drvdata->gpios[ASIO_DCLK].flags = GPIOF_OUT_INIT_LOW;
> > + drvdata->gpios[ASIO_DCLK].label = "as_dclk";
> > + drvdata->gpios[ASIO_NCS].gpio = pdata->ncs;
> > + drvdata->gpios[ASIO_NCS].flags = GPIOF_OUT_INIT_HIGH;
> > + drvdata->gpios[ASIO_NCS].label = "as_ncs";
> > + drvdata->gpios[ASIO_NCE].gpio = pdata->nce;
> > + drvdata->gpios[ASIO_NCE].flags = GPIOF_OUT_INIT_HIGH;
> > + drvdata->gpios[ASIO_NCE].label = "as_nce";
>
> ...at least after this bit?
>
> > +
> > + dev_info(drvdata->dev, "Altera Cyclone Active Serial driver\n");
> > +
> > + return 0;
> > +
> > +cdev_del:
> > + clear_bit(minor, cyclone_as_devs);
> > + cdev_del(&drvdata->cdev);
> > + return ret;
> > +}
[snip]
> > diff --git a/include/linux/cyclone_as.h b/include/linux/cyclone_as.h
> > new file mode 100644
> > index 0000000..cf86955
> > --- /dev/null
> > +++ b/include/linux/cyclone_as.h
> > @@ -0,0 +1,23 @@
> > +/*
> > + * Copyright 2010 Alex Gershgorin, Orex Computed Radiography
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
> > + */
> > +
> > +#ifndef __ALTERA_PROG_H
> > +#define __ALTERA_PROG_H
> > +
> > +struct cyclone_as_platform_data {
> > + unsigned data;
> > + unsigned nconfig;
> > + unsigned dclk;
> > + unsigned ncs;
> > + unsigned nce;
> > +};
> > +
> > +#endif /* __ALTERA_PROG_H */
>
> I know other drivers put their header files in include/linux/ too,
> but is there any reason to? This seems all internal to cyclone_as.c,
> why not have no header file?
This part is not internal at all. Using this struct the platform code (knowing
the actual machine specific GPIO wiring) passes this information to the
driver.
> Probably not high on your list, but what about suspend support?
> Preventing suspend from succeeding seems like a good idea when
> stuff is going on.
I'm not sure we should impose a suspend prevention in this case. The user
should bear the consequences if he/she decides to suspend the system in the
middle of FPGA programming. Maybe we should just warn.
baruch
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
Hi Baruch,
On Thu, November 4, 2010 07:46, Baruch Siach wrote:
>>
>> I think it would clear up the code a lot if you introduced a xmit_bytes()
>> function which does the above for variable lengths, as well as the ASIO_NCS
>> fiddling.
>
> I'll consider this for the next version. Keep in mind that we send a whole
> command during nCS assertion, not arbitrary buffers. So, in the case of the
> PAGE_PROGRAM command, we'll need to prepend the instruction and its address
> argument to the page data. I'm not sure we end up with something more clear
> than we have now.
You just have to make the buffer slightly bigger and do something like:
ret = copy_from_user(page_buf + 4, buf, AS_PAGE_SIZE);
...
page_buf[0] = AS_PAGE_PROGRAM;
page_buf[1] = (page_count >> 8) & 0xff;
page_buf[2] = page_count & 0xff;
page_buf[3] = 0;
Which doesn't look too bad.
(Btw, aren't [1] and [2] accidentally swapped? If it supports three
address bytes as you say below, then middle-low-high byte seems like
a weird order.)
>> Also pass in drvdata and get the gpios from there in this function,
>> you never have the one without the other.
>
> So? I want to avoid the 'drvdata->' prefix when it's not needed.
Good point. Perhaps add a temporary var holding the pointer if after
the changes there's still drvdata->gpios everywhere.
>> You don't unlock open_lock, so this supports only one user at the time?
>
> Correct. I don't think anything good will come out of concurrent writes to the
> FPGA, and read is not supported.
Seems sensible.
>> unsigned short?
>
> The largest chip currently supporting this protocol (EPCS128) has 2^16 pages,
> but the protocol allocates 3 address bytes. So why limit ourselves? Currently
> the code only uses the 16 LSBs, I'll change this.
I think you should add a check to see if page_count isn't too big,
or else things go silently wrong when the caller supplies a too big
value.
>> Is 5s always enough? Is there some way to check if it's really done?
>
> You are right. 5 seconds are definitely not enough for the larger chips
> (typical time of 105 seconds for EPCS128). I'll poll the "write in progress"
> bit instead. More code to write :(.
If it takes that long, just poll every second or so I guess.
>> Maybe check the whole range before sending any data?
>
> What is there to check?
The whole range of 'buf'. If the copy_from_user fails later on you end
up with a partially programmed FPGA. No big deal, but avoidable.
Maybe annotate buf with __user to keep Sparse happy?
static ssize_t cyclone_as_write(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
>> > diff --git a/include/linux/cyclone_as.h b/include/linux/cyclone_as.h
>> > new file mode 100644
>> > index 0000000..cf86955
>> > --- /dev/null
>> > +++ b/include/linux/cyclone_as.h
>> > @@ -0,0 +1,23 @@
>> > +/*
>> > + * Copyright 2010 Alex Gershgorin, Orex Computed Radiography
>> > + *
>> > + * The code contained herein is licensed under the GNU General Public
>> > + * License. You may obtain a copy of the GNU General Public License
>> > + * Version 2 or later at the following locations:
>> > + *
>> > + * http://www.opensource.org/licenses/gpl-license.html
>> > + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
>> > + */
>> > +
>> > +#ifndef __ALTERA_PROG_H
>> > +#define __ALTERA_PROG_H
>> > +
>> > +struct cyclone_as_platform_data {
>> > + unsigned data;
>> > + unsigned nconfig;
>> > + unsigned dclk;
>> > + unsigned ncs;
>> > + unsigned nce;
>> > +};
>> > +
>> > +#endif /* __ALTERA_PROG_H */
>>
>> I know other drivers put their header files in include/linux/ too,
>> but is there any reason to? This seems all internal to cyclone_as.c,
>> why not have no header file?
>
> This part is not internal at all. Using this struct the platform code (knowing
> the actual machine specific GPIO wiring) passes this information to the
> driver.
Except if I'm missing some build system voodoo, no, it's totally internal.
The driver sets it, no one else knows about it:
+static int __init cyclone_as_probe(struct platform_device *pdev)
+{
+ int ret, minor;
+ struct cyclone_as_device *drvdata;
+ struct device *dev;
+ struct cyclone_as_platform_data *pdata = pdev->dev.platform_data;
+
+ drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
I'm sure that dev.platform_data is void*.
>> Probably not high on your list, but what about suspend support?
>> Preventing suspend from succeeding seems like a good idea when
>> stuff is going on.
>
> I'm not sure we should impose a suspend prevention in this case. The user
> should bear the consequences if he/she decides to suspend the system in the
> middle of FPGA programming. Maybe we should just warn.
I don't think any user would ever want to suspend in the middle of
FPGA programming. So if it happens it's most likely a mistake or
some automatic suspend. Both should fail. The code to fail is
slightly simpler than the code to warn too. So I'd say either let
it fail or keep your code simpler and ignore it altogether.
Greetings,
Indan
Hi Indan,
On Thu, Nov 04, 2010 at 12:57:22PM +0100, Indan Zupancic wrote:
> On Thu, November 4, 2010 07:46, Baruch Siach wrote:
> >>
> >> I think it would clear up the code a lot if you introduced a xmit_bytes()
> >> function which does the above for variable lengths, as well as the ASIO_NCS
> >> fiddling.
> >
> > I'll consider this for the next version. Keep in mind that we send a whole
> > command during nCS assertion, not arbitrary buffers. So, in the case of the
> > PAGE_PROGRAM command, we'll need to prepend the instruction and its address
> > argument to the page data. I'm not sure we end up with something more clear
> > than we have now.
>
> You just have to make the buffer slightly bigger and do something like:
>
> ret = copy_from_user(page_buf + 4, buf, AS_PAGE_SIZE);
> ...
> page_buf[0] = AS_PAGE_PROGRAM;
> page_buf[1] = (page_count >> 8) & 0xff;
> page_buf[2] = page_count & 0xff;
> page_buf[3] = 0;
>
> Which doesn't look too bad.
OK.
> (Btw, aren't [1] and [2] accidentally swapped? If it supports three
> address bytes as you say below, then middle-low-high byte seems like
> a weird order.)
I was wrong about this. I checked the spec again, and it seems that the 24bit
PAGE_PROGRAM parameter is a byte pointer, not page pointer. So we can change
page_count to u16 after all.
>
> >> Also pass in drvdata and get the gpios from there in this function,
> >> you never have the one without the other.
> >
> > So? I want to avoid the 'drvdata->' prefix when it's not needed.
>
> Good point. Perhaps add a temporary var holding the pointer if after
> the changes there's still drvdata->gpios everywhere.
OK.
> >> You don't unlock open_lock, so this supports only one user at the time?
> >
> > Correct. I don't think anything good will come out of concurrent writes to the
> > FPGA, and read is not supported.
>
> Seems sensible.
>
> >> unsigned short?
> >
> > The largest chip currently supporting this protocol (EPCS128) has 2^16 pages,
> > but the protocol allocates 3 address bytes. So why limit ourselves? Currently
> > the code only uses the 16 LSBs, I'll change this.
>
> I think you should add a check to see if page_count isn't too big,
> or else things go silently wrong when the caller supplies a too big
> value.
>
> >> Is 5s always enough? Is there some way to check if it's really done?
> >
> > You are right. 5 seconds are definitely not enough for the larger chips
> > (typical time of 105 seconds for EPCS128). I'll poll the "write in progress"
> > bit instead. More code to write :(.
>
> If it takes that long, just poll every second or so I guess.
OK. I'm now implementing this. Also, ignoring the return value of
msleep_interruptible() is a terrible mistake. I've posted a patch earlier
today that makes it __must_check.
> >> Maybe check the whole range before sending any data?
> >
> > What is there to check?
>
> The whole range of 'buf'. If the copy_from_user fails later on you end
> up with a partially programmed FPGA. No big deal, but avoidable.
OK.
> Maybe annotate buf with __user to keep Sparse happy?
>
> static ssize_t cyclone_as_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
No problem.
> >> > diff --git a/include/linux/cyclone_as.h b/include/linux/cyclone_as.h
> >> > new file mode 100644
> >> > index 0000000..cf86955
> >> > --- /dev/null
> >> > +++ b/include/linux/cyclone_as.h
> >> > @@ -0,0 +1,23 @@
> >> > +/*
> >> > + * Copyright 2010 Alex Gershgorin, Orex Computed Radiography
> >> > + *
> >> > + * The code contained herein is licensed under the GNU General Public
> >> > + * License. You may obtain a copy of the GNU General Public License
> >> > + * Version 2 or later at the following locations:
> >> > + *
> >> > + * http://www.opensource.org/licenses/gpl-license.html
> >> > + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
> >> > + */
> >> > +
> >> > +#ifndef __ALTERA_PROG_H
> >> > +#define __ALTERA_PROG_H
> >> > +
> >> > +struct cyclone_as_platform_data {
> >> > + unsigned data;
> >> > + unsigned nconfig;
> >> > + unsigned dclk;
> >> > + unsigned ncs;
> >> > + unsigned nce;
> >> > +};
> >> > +
> >> > +#endif /* __ALTERA_PROG_H */
> >>
> >> I know other drivers put their header files in include/linux/ too,
> >> but is there any reason to? This seems all internal to cyclone_as.c,
> >> why not have no header file?
> >
> > This part is not internal at all. Using this struct the platform code (knowing
> > the actual machine specific GPIO wiring) passes this information to the
> > driver.
>
> Except if I'm missing some build system voodoo, no, it's totally internal.
> The driver sets it, no one else knows about it:
>
> +static int __init cyclone_as_probe(struct platform_device *pdev)
> +{
> + int ret, minor;
> + struct cyclone_as_device *drvdata;
> + struct device *dev;
> + struct cyclone_as_platform_data *pdata = pdev->dev.platform_data;
> +
> + drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
>
> I'm sure that dev.platform_data is void*.
I'm setting dev.platform_data somewhere under arch/ as follows:
static struct cyclone_as_platform_data altera_prog_info = {
.data = 102,
.nconfig = 105,
.dclk = 103,
.ncs = 106,
.nce = 104,
};
static struct platform_device fpga_prog_device = {
.name = "cyclone_as",
.dev = {
.platform_data = &altera_prog_info,
}
};
And then:
platform_device_register(&fpga_prog_device);
This is a very common patters in the arch/ underworld. I need struct
cyclone_as_platform_data to be visible for this.
> >> Probably not high on your list, but what about suspend support?
> >> Preventing suspend from succeeding seems like a good idea when
> >> stuff is going on.
> >
> > I'm not sure we should impose a suspend prevention in this case. The user
> > should bear the consequences if he/she decides to suspend the system in the
> > middle of FPGA programming. Maybe we should just warn.
>
> I don't think any user would ever want to suspend in the middle of
> FPGA programming. So if it happens it's most likely a mistake or
> some automatic suspend. Both should fail. The code to fail is
> slightly simpler than the code to warn too. So I'd say either let
> it fail or keep your code simpler and ignore it altogether.
I think I'll go for the latter for now. This can always be added later.
baruch
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
Hi Baruch,
On Thu, November 4, 2010 13:20, Baruch Siach wrote:
> I was wrong about this. I checked the spec again, and it seems that the 24bit
> PAGE_PROGRAM parameter is a byte pointer, not page pointer. So we can change
> page_count to u16 after all.
If it's a byte pointer, then isn't page_count totally wrong?
If it's not the page count, then it should be named differently.
For your next submission, can you test if this driver can successfully
program a FPGA? I mean, including a test to see if all the code made
it to the FPGA, to catch short writes. If the protocol doesn't allow
reads, how are you supposed to check if the programming was really
successful? Is there a CRC somewhere or anything?
I'd also get rid of "len" and use "count" directly. You have to
check if "count" and/or *ppos + count isn't too big anyway. Or
use len instead of page_count. I don't know. Maybe change the
while into a for loop?
+ ret = copy_from_user(page_buf, buf, AS_PAGE_SIZE);
+ if (ret == 0) {
+ buf += AS_PAGE_SIZE;
+ *ppos += AS_PAGE_SIZE;
+ len -= AS_PAGE_SIZE;
+ } else {
+ kfree(page_buf);
+ return -EFAULT;
+ }
I'd change that into:
+ ret = copy_from_user(page_buf, buf, AS_PAGE_SIZE);
+ if (ret) {
+ kfree(page_buf);
+ return -EFAULT;
+ }
+ buf += AS_PAGE_SIZE;
+ *ppos += AS_PAGE_SIZE;
+ len -= AS_PAGE_SIZE;
> OK. I'm now implementing this. Also, ignoring the return value of
> msleep_interruptible() is a terrible mistake. I've posted a patch
> earlier today that makes it __must_check.
Also if you don't care if it slept for shorter?
> I'm setting dev.platform_data somewhere under arch/ as follows:
>
> static struct cyclone_as_platform_data altera_prog_info = {
> .data = 102,
> .nconfig = 105,
> .dclk = 103,
> .ncs = 106,
> .nce = 104,
> };
>
> static struct platform_device fpga_prog_device = {
> .name = "cyclone_as",
> .dev = {
> .platform_data = &altera_prog_info,
> }
> };
>
> And then:
>
> platform_device_register(&fpga_prog_device);
>
> This is a very common patters in the arch/ underworld. I need struct
> cyclone_as_platform_data to be visible for this.
Thanks for the explanation.
I mixed up drvdata with pdata, so yeah, you're right.
Shouldn't you add this platform bit to the patch? A platform driver
with no users seems a bit useless.
>> >> Probably not high on your list, but what about suspend support?
>> >> Preventing suspend from succeeding seems like a good idea when
>> >> stuff is going on.
>> >
>> > I'm not sure we should impose a suspend prevention in this case. The user
>> > should bear the consequences if he/she decides to suspend the system in
>> the
>> > middle of FPGA programming. Maybe we should just warn.
>>
>> I don't think any user would ever want to suspend in the middle of
>> FPGA programming. So if it happens it's most likely a mistake or
>> some automatic suspend. Both should fail. The code to fail is
>> slightly simpler than the code to warn too. So I'd say either let
>> it fail or keep your code simpler and ignore it altogether.
>
> I think I'll go for the latter for now. This can always be added later.
As long as this isn't used for very expensive OTP FPGAs handling
suspend is not that important...
Greetings,
Indan
Hi Indan,
On Thu, Nov 04, 2010 at 02:09:41PM +0100, Indan Zupancic wrote:
> On Thu, November 4, 2010 13:20, Baruch Siach wrote:
> > I was wrong about this. I checked the spec again, and it seems that the 24bit
> > PAGE_PROGRAM parameter is a byte pointer, not page pointer. So we can change
> > page_count to u16 after all.
>
> If it's a byte pointer, then isn't page_count totally wrong?
> If it's not the page count, then it should be named differently.
No. It is a page_count. Appending one zero byte to page_count makes the page
pointer a byte pointer.
> For your next submission, can you test if this driver can successfully
> program a FPGA? I mean, including a test to see if all the code made
> it to the FPGA, to catch short writes.
The driver does program FPGAs successfully. I test it after every change using
two .rdp images. Each image makes the FPGA blink a LED is a slightly different
way.
> If the protocol doesn't allow
> reads, how are you supposed to check if the programming was really
> successful? Is there a CRC somewhere or anything?
The protocol does allow reads. I just haven't implemented it. This is going to
be the job of someone else.
> I'd also get rid of "len" and use "count" directly. You have to
> check if "count" and/or *ppos + count isn't too big anyway. Or
> use len instead of page_count. I don't know. Maybe change the
> while into a for loop?
I'll consider this.
> + ret = copy_from_user(page_buf, buf, AS_PAGE_SIZE);
> + if (ret == 0) {
> + buf += AS_PAGE_SIZE;
> + *ppos += AS_PAGE_SIZE;
> + len -= AS_PAGE_SIZE;
> + } else {
> + kfree(page_buf);
> + return -EFAULT;
> + }
>
> I'd change that into:
>
> + ret = copy_from_user(page_buf, buf, AS_PAGE_SIZE);
> + if (ret) {
> + kfree(page_buf);
> + return -EFAULT;
> + }
> + buf += AS_PAGE_SIZE;
> + *ppos += AS_PAGE_SIZE;
> + len -= AS_PAGE_SIZE;
Just did before seeing your reply :).
> > OK. I'm now implementing this. Also, ignoring the return value of
> > msleep_interruptible() is a terrible mistake. I've posted a patch
> > earlier today that makes it __must_check.
>
> Also if you don't care if it slept for shorter?
If you don't care than why sleep longer? If you need the delay you must make
sure it doesn't get shorter than expected.
> > I'm setting dev.platform_data somewhere under arch/ as follows:
> >
> > static struct cyclone_as_platform_data altera_prog_info = {
> > .data = 102,
> > .nconfig = 105,
> > .dclk = 103,
> > .ncs = 106,
> > .nce = 104,
> > };
> >
> > static struct platform_device fpga_prog_device = {
> > .name = "cyclone_as",
> > .dev = {
> > .platform_data = &altera_prog_info,
> > }
> > };
> >
> > And then:
> >
> > platform_device_register(&fpga_prog_device);
> >
> > This is a very common patters in the arch/ underworld. I need struct
> > cyclone_as_platform_data to be visible for this.
>
> Thanks for the explanation.
>
> I mixed up drvdata with pdata, so yeah, you're right.
>
> Shouldn't you add this platform bit to the patch? A platform driver
> with no users seems a bit useless.
I'm keeping my platform code private for now. May platform drivers have no
user under arch/, but it doesn't mean they are not being used. See, for
example, the DesignWare I2C bus driver (drivers/i2c/busses/i2c-designware.c).
Anyway, the custom is to put drivers code and platform code in separate
patches. They tend to go to different trees/maintainers.
> >> >> Probably not high on your list, but what about suspend support?
> >> >> Preventing suspend from succeeding seems like a good idea when
> >> >> stuff is going on.
> >> >
> >> > I'm not sure we should impose a suspend prevention in this case. The user
> >> > should bear the consequences if he/she decides to suspend the system in
> >> the
> >> > middle of FPGA programming. Maybe we should just warn.
> >>
> >> I don't think any user would ever want to suspend in the middle of
> >> FPGA programming. So if it happens it's most likely a mistake or
> >> some automatic suspend. Both should fail. The code to fail is
> >> slightly simpler than the code to warn too. So I'd say either let
> >> it fail or keep your code simpler and ignore it altogether.
> >
> > I think I'll go for the latter for now. This can always be added later.
>
> As long as this isn't used for very expensive OTP FPGAs handling
> suspend is not that important...
As far as I know this protocol is not for OTP FPGAs. Anyone doing expensive
expensive OTP FPGAs programming, should have the resources to either protect
the machine from being suspended, or write the code to make sure it doesn't
happen.
baruch
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -