[PATCH] drivers/misc: Altera Cyclone active serial implementation

[PATCH] drivers/misc: Altera Cyclone active serial implementation

[Baruch Siach]

From: Alex Gershgorin <agersh@rambler.ru>

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 <agersh@rambler.ru>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 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);
+	}
+}
+
+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);
+
+	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);
+
+	return 0;
+}
+
+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;
+
+	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);
+	}
+
+	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;
+		}
+
+		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);
+
+		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);
+		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;
+
+	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);
+	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";
+
+	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 <agersh@rambler.ru>");
+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 */
-- 
1.7.2.3

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Indan Zupancic]

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 <agersh@rambler.ru>
>
> 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 <agersh@rambler.ru>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  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 <agersh@rambler.ru>");
> +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

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Baruch Siach]

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 <agersh@rambler.ru>
> >
> > 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 <agersh@rambler.ru>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---

[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------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Indan Zupancic]

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

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Baruch Siach]

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------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Indan Zupancic]

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

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Baruch Siach]

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------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Greg KH]

On Wed, Nov 03, 2010 at 04:21:35PM +0200, Baruch Siach wrote:
> From: Alex Gershgorin <agersh@rambler.ru>
> 
> 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 <agersh@rambler.ru>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  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;

Please don't create your own class just for a single driver.  Just use
the misc class interface instead, as all you really want/need here is a
character device node, right?

And as discussed at the Plumbers conference this past week, we don't
want to add any new 'struct class' implementations to the kernel from
now on, as it's the overall wrong thing to do.


> --- /dev/null
> +++ b/include/linux/cyclone_as.h

Why do you need a .h file at all?

thanks,

greg k-h

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Baruch Siach]

Hi Greg,

On Sat, Nov 06, 2010 at 11:19:30AM -0700, Greg KH wrote:
> On Wed, Nov 03, 2010 at 04:21:35PM +0200, Baruch Siach wrote:
> > From: Alex Gershgorin <agersh@rambler.ru>
> > 
> > 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 <agersh@rambler.ru>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---

[snip]

> > +static struct class *cyclone_as_class;
> 
> Please don't create your own class just for a single driver.  Just use
> the misc class interface instead, as all you really want/need here is a
> character device node, right?

Searching for 'mist' under include/linux/ I couldn't find this "misc class 
interface". Can you enlighten me?

I did find the "miscdevices" interface in include/linux/miscdevice.h.  I also 
tried this one before going on to create a class of my own. However, this 
interface seems to be limited to singleton devices. Can I use 
MISC_DYNAMIC_MINOR to create multiple device nodes? Is there any example for 
this?

> And as discussed at the Plumbers conference this past week, we don't
> want to add any new 'struct class' implementations to the kernel from
> now on, as it's the overall wrong thing to do.
> 
> > --- /dev/null
> > +++ b/include/linux/cyclone_as.h
> 
> Why do you need a .h file at all?

Look at the content of this file. I need to pass GPIO numbers from the 
platform code under arch/ to the driver. For this I use a platform_data 
struct, which must be visible to the platform code. Is there a better way to 
do this?

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Greg KH]

On Mon, Nov 08, 2010 at 08:57:37AM +0200, Baruch Siach wrote:
> Hi Greg,
> 
> On Sat, Nov 06, 2010 at 11:19:30AM -0700, Greg KH wrote:
> > On Wed, Nov 03, 2010 at 04:21:35PM +0200, Baruch Siach wrote:
> > > From: Alex Gershgorin <agersh@rambler.ru>
> > > 
> > > 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 <agersh@rambler.ru>
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > ---
> 
> [snip]
> 
> > > +static struct class *cyclone_as_class;
> > 
> > Please don't create your own class just for a single driver.  Just use
> > the misc class interface instead, as all you really want/need here is a
> > character device node, right?
> 
> Searching for 'mist' under include/linux/ I couldn't find this "misc class 
> interface". Can you enlighten me?
> 
> I did find the "miscdevices" interface in include/linux/miscdevice.h.

Yes, that is the one.

> I also tried this one before going on to create a class of my own.
> However, this interface seems to be limited to singleton devices. Can
> I use MISC_DYNAMIC_MINOR to create multiple device nodes?

I didn't see where your code was handling multiple device nodes, is it
really doing that?

> Is there any example for this?

You can just increment the name for the miscdevice to do this based on a
driver-supplied unique number.

> > And as discussed at the Plumbers conference this past week, we don't
> > want to add any new 'struct class' implementations to the kernel from
> > now on, as it's the overall wrong thing to do.
> > 
> > > --- /dev/null
> > > +++ b/include/linux/cyclone_as.h
> > 
> > Why do you need a .h file at all?
> 
> Look at the content of this file. I need to pass GPIO numbers from the 
> platform code under arch/ to the driver. For this I use a platform_data 
> struct, which must be visible to the platform code. Is there a better way to 
> do this?

{sigh}

I'm getting really tired of these kinds of .h files cluttering up the
kernel for an interface that should be easier to handle some other way.

I'm really tempted to create something like:
	include/platform_crap/
to put all of these into.

Ok, that probably will not make people all that happy, so how about:
	include/platform_drivers/

instead?

thanks,

greg k-h

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Baruch Siach]

Hi Greg,

On Mon, Nov 08, 2010 at 07:58:51AM -0800, Greg KH wrote:
> On Mon, Nov 08, 2010 at 08:57:37AM +0200, Baruch Siach wrote:
> > On Sat, Nov 06, 2010 at 11:19:30AM -0700, Greg KH wrote:
> > > On Wed, Nov 03, 2010 at 04:21:35PM +0200, Baruch Siach wrote:
> > > > From: Alex Gershgorin <agersh@rambler.ru>
> > > > 
> > > > 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 <agersh@rambler.ru>
> > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > > ---
> > 
> > [snip]
> > 
> > > > +static struct class *cyclone_as_class;
> > > 
> > > Please don't create your own class just for a single driver.  Just use
> > > the misc class interface instead, as all you really want/need here is a
> > > character device node, right?
> > 
> > Searching for 'mist' under include/linux/ I couldn't find this "misc class 
> > interface". Can you enlighten me?
> > 
> > I did find the "miscdevices" interface in include/linux/miscdevice.h.
> 
> Yes, that is the one.
> 
> > I also tried this one before going on to create a class of my own.
> > However, this interface seems to be limited to singleton devices. Can
> > I use MISC_DYNAMIC_MINOR to create multiple device nodes?
> 
> I didn't see where your code was handling multiple device nodes, is it
> really doing that?

Of course. Quoting a .probe snippet from the original patch:

+	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);

> > Is there any example for this?
> 
> You can just increment the name for the miscdevice to do this based on a
> driver-supplied unique number.

OK. Will do.

> > > And as discussed at the Plumbers conference this past week, we don't
> > > want to add any new 'struct class' implementations to the kernel from
> > > now on, as it's the overall wrong thing to do.
> > > 
> > > > --- /dev/null
> > > > +++ b/include/linux/cyclone_as.h
> > > 
> > > Why do you need a .h file at all?
> > 
> > Look at the content of this file. I need to pass GPIO numbers from the 
> > platform code under arch/ to the driver. For this I use a platform_data 
> > struct, which must be visible to the platform code. Is there a better way to 
> > do this?
> 
> {sigh}
> 
> I'm getting really tired of these kinds of .h files cluttering up the
> kernel for an interface that should be easier to handle some other way.

Which easier way? Extending IORESOURCE in include/linux/ioport.h to include 
GPIOs (say, IORESOURCE_GPIO) may have solved the problem for me. Is it 
reasonable?

> I'm really tempted to create something like:
> 	include/platform_crap/
> to put all of these into.
> 
> Ok, that probably will not make people all that happy, so how about:
> 	include/platform_drivers/
> 
> instead?

OK. I'll create include/platform_drivers/ in my next patch. Quite a few header 
from include/ could move there.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[H. Peter Anvin]

On 11/03/2010 07:21 AM, Baruch Siach wrote:
> From: Alex Gershgorin <agersh@rambler.ru>
> 
> 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.
> 

Calling it "Cyclone" is a bit misleading.  This protocol is used by
Stratix, Cyclone and Arria devices (at least), and it probably depends
more on the generation than on the product line which exact version of
the protocol it is.

If anything it would probably be more accurate to call it a Statix
protocol, or just an Altera one.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] drivers/misc: Altera Cyclone active serial implementation

[Baruch Siach]

Hi hpa,

On Wed, Nov 10, 2010 at 09:11:49AM -0800, H. Peter Anvin wrote:
> On 11/03/2010 07:21 AM, Baruch Siach wrote:
> > From: Alex Gershgorin <agersh@rambler.ru>
> > 
> > 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.
> 
> Calling it "Cyclone" is a bit misleading.  This protocol is used by
> Stratix, Cyclone and Arria devices (at least), and it probably depends
> more on the generation than on the product line which exact version of
> the protocol it is.
> 
> If anything it would probably be more accurate to call it a Statix
> protocol, or just an Altera one.

I see. I think I'll rename it to altera_as, for Altera active serial.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -