[PATCHv3] drivers/misc: Altera active serial implementation

[PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach]

From: Alex Gershgorin <agersh@rambler.ru>

The active serial protocol can be used to program Altera serial configuration
devices. This driver uses the kernel gpio interface to implement the active
serial protocol.

This patch also introduces the include/platform_drivers/ directory, which is
the new home for platform data headers. This is per Greg's suggestion.

Reviewed-by: Indan Zupancic <indan@nul.nu>
Signed-off-by: Alex Gershgorin <agersh@rambler.ru>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
Changes in v3:

    * Rename to altera_as for a better description of the driver scope

    * Mention ESPC devices in the Kconfig help text

    * Add a comment that explains why the static altera_as_devs arrays doesn't
      need locking protection

    * Shorten too long delays

    * Move the erase operation to a separate function

    * Eliminate page_count in .write, use *ppos instead

Changes in v2:

    * Don't create a new class, use the misc class instead

    * Depend on GENERIC_GPIO

    * Simplify xmit_byte(), use bitrev8() to flip data bytes

    * Add xmit_cmd() to simplify send of simple commands

    * Remove unnecessary debug prints

    * Check msleep_interruptible() return value

    * Remove unneeded ndelay() calls

    * Poll the write-in-progress indicator instead of waiting for a fixed (and 
      too short) period of time after erase.

 drivers/misc/Kconfig                 |   12 ++
 drivers/misc/Makefile                |    1 +
 drivers/misc/altera_as.c             |  349 ++++++++++++++++++++++++++++++++++
 include/platform_drivers/altera_as.h |   28 +++
 4 files changed, 390 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/altera_as.c
 create mode 100644 include/platform_drivers/altera_as.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 4d073f1..6dbe59b 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -452,6 +452,18 @@ config PCH_PHUB
 	  To compile this driver as a module, choose M here: the module will
 	  be called pch_phub.
 
+config ALTERA_AS
+	depends on GENERIC_GPIO
+	tristate "Altera Active Serial driver"
+	help
+	  Provides support for active serial programming of Altera serial
+	  configuration devices (EPCS1, EPCS4, EPCS16, EPCS128). 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 altera_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 98009cc..ea03995 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -42,3 +42,4 @@ obj-$(CONFIG_ARM_CHARLCD)	+= arm-charlcd.o
 obj-$(CONFIG_PCH_PHUB)		+= pch_phub.o
 obj-y				+= ti-st/
 obj-$(CONFIG_AB8500_PWM)	+= ab8500-pwm.o
+obj-$(CONFIG_ALTERA_AS)		+= altera_as.o
diff --git a/drivers/misc/altera_as.c b/drivers/misc/altera_as.c
new file mode 100644
index 0000000..b6806a6
--- /dev/null
+++ b/drivers/misc/altera_as.c
@@ -0,0 +1,349 @@
+/*
+ * 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/gpio.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/bitrev.h>
+#include <linux/device.h>
+#include <linux/uaccess.h>
+#include <linux/miscdevice.h>
+#include <linux/platform_device.h>
+
+#include <platform_drivers/altera_as.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_STATUS_WIP		(1 << 0)
+#define AS_STATUS_WEL		(1 << 1)
+#define AS_STATUS_BP0		(1 << 2)
+#define AS_STATUS_BP1		(1 << 3)
+#define AS_STATUS_BP2		(1 << 4)
+
+#define AS_ERASE_TIMEOUT	250 /* seconds, max for EPCS128 */
+
+#define AS_PAGE_SIZE		256
+
+#define AS_MAX_DEVS		16
+
+#define ASIO_DATA		0
+#define ASIO_ASDI		1
+#define ASIO_NCONFIG		2
+#define ASIO_DCLK		3
+#define ASIO_NCS		4
+#define ASIO_NCE		5
+#define AS_GPIO_NUM		6
+
+#define AS_ALIGNED(x)	IS_ALIGNED(x, AS_PAGE_SIZE)
+
+struct altera_as_device {
+	unsigned id;
+	struct device *dev;
+	struct miscdevice miscdev;
+	struct mutex open_lock;
+	struct gpio gpios[AS_GPIO_NUM];
+};
+
+/*
+ * The only functions updating this array are .probe/.remove, which are
+ * serialized. Hence, no locking is needed here.
+ */
+static struct {
+	int minor;
+	struct altera_as_device *drvdata;
+} altera_as_devs[AS_MAX_DEVS];
+
+static struct altera_as_device *get_as_dev(int minor)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(altera_as_devs); i++)
+		if (altera_as_devs[i].minor == minor)
+			return altera_as_devs[i].drvdata;
+
+	return NULL;
+}
+
+static void as_clk_tick(struct gpio *gpios)
+{
+	gpio_set_value(gpios[ASIO_DCLK].gpio, 0);
+	ndelay(20);
+	gpio_set_value(gpios[ASIO_DCLK].gpio, 1);
+	ndelay(20);
+}
+
+static void xmit_byte(struct gpio *gpios, u8 c)
+{
+	int i;
+
+	for (i = 0; i < 8; i++) {
+		gpio_set_value(gpios[ASIO_ASDI].gpio, (c << i) & 0x80);
+		as_clk_tick(gpios);
+	}
+}
+
+static void xmit_cmd(struct gpio *gpios, u8 cmd)
+{
+	gpio_set_value(gpios[ASIO_NCS].gpio, 0);
+	xmit_byte(gpios, cmd);
+	gpio_set_value(gpios[ASIO_NCS].gpio, 1);
+	ndelay(100);
+}
+
+static u8 recv_byte(struct gpio *gpios)
+{
+	int i;
+	u8 val = 0;
+
+	for (i = 0; i < 8; i++) {
+		as_clk_tick(gpios);
+		val |= (!!gpio_get_value(gpios[ASIO_DATA].gpio) << (7 - i));
+	}
+
+	return val;
+}
+
+static int as_erase(struct gpio *gpios)
+{
+	u8 as_status;
+	int i, err = 0;
+
+	xmit_cmd(gpios, AS_WRITE_ENABLE);
+	xmit_cmd(gpios, AS_ERASE_BULK);
+
+	gpio_set_value(gpios[ASIO_NCS].gpio, 0);
+	xmit_byte(gpios, AS_READ_STATUS);
+	for (i = 0; i < AS_ERASE_TIMEOUT; i++) {
+		as_status = recv_byte(gpios);
+		if ((as_status & AS_STATUS_WIP) == 0)
+			break; /* erase done */
+		if (msleep_interruptible(1000) > 0) {
+			err = -EINTR;
+			break;
+		}
+	}
+	gpio_set_value(gpios[ASIO_NCS].gpio, 1);
+	ndelay(100);
+	if ((as_status & AS_STATUS_WIP) && err == 0)
+		err = -EIO; /* erase timeout */
+
+	return err;
+}
+
+static int altera_as_open(struct inode *inode, struct file *file)
+{
+	int ret;
+	struct altera_as_device *drvdata = get_as_dev(iminor(inode));
+
+	if (drvdata == NULL)
+		return -ENODEV;
+	file->private_data = drvdata;
+
+	ret = mutex_trylock(&drvdata->open_lock);
+	if (ret == 0)
+		return -EBUSY;
+
+	ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
+	if (ret < 0) {
+		mutex_unlock(&drvdata->open_lock);
+		return ret;
+	}
+
+	return 0;
+}
+
+static ssize_t altera_as_write(struct file *file, const char __user *buf,
+		size_t count, loff_t *ppos)
+{
+	int	i, err = 0;
+	u8	*page_buf;
+	ssize_t written = 0;
+	struct altera_as_device *drvdata = file->private_data;
+
+	if (count == 0)
+		return 0;
+
+	/* writes must be page aligned */
+	if (!AS_ALIGNED(*ppos) || !AS_ALIGNED(count))
+		return -EINVAL;
+
+	page_buf = kmalloc(AS_PAGE_SIZE, GFP_KERNEL);
+	if (page_buf == NULL)
+		return -ENOMEM;
+
+	if (*ppos == 0) {
+		err = as_erase(drvdata->gpios);
+		if (err < 0)
+			goto out;
+	}
+
+	while ((count - written) > 0) {
+		err = copy_from_user(page_buf, buf+written, AS_PAGE_SIZE);
+		if (err < 0) {
+			err = -EFAULT;
+			goto out;
+		}
+
+		xmit_cmd(drvdata->gpios, AS_WRITE_ENABLE);
+
+		gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
+		/* op code */
+		xmit_byte(drvdata->gpios, AS_PAGE_PROGRAM);
+		/* address */
+		xmit_byte(drvdata->gpios, (*ppos >> 16) & 0xff);
+		xmit_byte(drvdata->gpios, (*ppos >> 8) & 0xff);
+		xmit_byte(drvdata->gpios, *ppos & 0xff);
+		/* page data (LSB first) */
+		for (i = 0; i < AS_PAGE_SIZE; i++)
+			xmit_byte(drvdata->gpios, bitrev8(page_buf[i]));
+		gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
+		mdelay(7);
+
+		*ppos += AS_PAGE_SIZE;
+		written += AS_PAGE_SIZE;
+	}
+
+out:
+	kfree(page_buf);
+	return err ?: written;
+}
+
+static int altera_as_release(struct inode *inode, struct file *file)
+{
+	struct altera_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 altera_as_fops = {
+	.open		= altera_as_open,
+	.write		= altera_as_write,
+	.release	= altera_as_release,
+};
+
+static int __init altera_as_probe(struct platform_device *pdev)
+{
+	struct altera_as_device *drvdata;
+	struct altera_as_platform_data *pdata = pdev->dev.platform_data;
+
+	if (pdata->id >= AS_MAX_DEVS)
+		return -ENODEV;
+
+	if (altera_as_devs[pdata->id].drvdata)
+		return -EBUSY;
+
+	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->dev = &pdev->dev;
+	platform_set_drvdata(pdev, drvdata);
+
+	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
+	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
+			pdata->id);
+	if (drvdata->miscdev.name == NULL)
+		return -ENOMEM;
+	drvdata->miscdev.fops = &altera_as_fops;
+	if (misc_register(&drvdata->miscdev) < 0) {
+		kfree(drvdata->miscdev.name);
+		return -ENODEV;
+	}
+	altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
+	altera_as_devs[pdata->id].drvdata = drvdata;
+
+	mutex_init(&drvdata->open_lock);
+
+	drvdata->id = pdata->id;
+
+	drvdata->gpios[ASIO_DATA].gpio		= pdata->data;
+	drvdata->gpios[ASIO_DATA].flags		= GPIOF_IN;
+	drvdata->gpios[ASIO_DATA].label		= "as_data";
+	drvdata->gpios[ASIO_ASDI].gpio		= pdata->asdi;
+	drvdata->gpios[ASIO_ASDI].flags		= GPIOF_OUT_INIT_LOW;
+	drvdata->gpios[ASIO_ASDI].label		= "as_asdi";
+	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;
+}
+
+static int __devexit altera_as_remove(struct platform_device *pdev)
+{
+	struct altera_as_device *drvdata = platform_get_drvdata(pdev);
+
+	altera_as_devs[drvdata->id].drvdata = NULL;
+	kfree(drvdata->miscdev.name);
+
+	return misc_deregister(&drvdata->miscdev);
+}
+
+static struct platform_driver altera_as_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "altera_as",
+	},
+	.remove = __devexit_p(altera_as_remove),
+};
+
+int __init altera_as_init(void)
+{
+	return platform_driver_probe(&altera_as_driver, altera_as_probe);
+}
+
+void __exit altera_as_cleanup(void)
+{
+	platform_driver_unregister(&altera_as_driver);
+}
+
+module_init(altera_as_init);
+module_exit(altera_as_cleanup);
+
+MODULE_AUTHOR("Alex Gershgorin <agersh@rambler.ru>");
+MODULE_DESCRIPTION("Altera Active Serial driver");
+MODULE_LICENSE("GPL");
diff --git a/include/platform_drivers/altera_as.h b/include/platform_drivers/altera_as.h
new file mode 100644
index 0000000..788bab5
--- /dev/null
+++ b/include/platform_drivers/altera_as.h
@@ -0,0 +1,28 @@
+/*
+ * include/platform_drivers/altera_as.h
+ *
+ * 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 __PLATFORM_DRIVERS_ALTERA_AS_H__
+#define __PLATFORM_DRIVERS_ALTERA_AS_H__
+
+struct altera_as_platform_data {
+	unsigned id; /* instance number */
+
+	unsigned data;
+	unsigned asdi;
+	unsigned nconfig;
+	unsigned dclk;
+	unsigned ncs;
+	unsigned nce;
+};
+
+#endif /* __PLATFORM_DRIVERS_ALTERA_AS_H__ */
-- 
1.7.2.3

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Jiri Slaby]

On 11/15/2010 09:23 AM, Baruch Siach wrote:
> --- /dev/null
> +++ b/drivers/misc/altera_as.c
> @@ -0,0 +1,349 @@
...
> +struct altera_as_device {
> +	unsigned id;
> +	struct device *dev;
> +	struct miscdevice miscdev;
> +	struct mutex open_lock;
> +	struct gpio gpios[AS_GPIO_NUM];
> +};
> +
> +/*
> + * The only functions updating this array are .probe/.remove, which are
> + * serialized. Hence, no locking is needed here.
> + */
> +static struct {
> +	int minor;

Why you need minor here? It's in drvdata->miscdev.minor.

> +	struct altera_as_device *drvdata;
> +} altera_as_devs[AS_MAX_DEVS];

You don't need the struct at all.
static struct altera_as_device *drvdata[AS_MAX_DEVS];
should be enough.

> +static int altera_as_open(struct inode *inode, struct file *file)
> +{
> +	int ret;
> +	struct altera_as_device *drvdata = get_as_dev(iminor(inode));
> +
> +	if (drvdata == NULL)
> +		return -ENODEV;
> +	file->private_data = drvdata;
> +
> +	ret = mutex_trylock(&drvdata->open_lock);
> +	if (ret == 0)
> +		return -EBUSY;
> +
> +	ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
> +	if (ret < 0) {
> +		mutex_unlock(&drvdata->open_lock);
> +		return ret;
> +	}

So leaving to userspace with mutex held. This is *wrong*. use
test_and_set_bit or alike instead.

> +	return 0;
> +}
> +
> +static ssize_t altera_as_write(struct file *file, const char __user *buf,
> +		size_t count, loff_t *ppos)
> +{
...
> +	while ((count - written) > 0) {
> +		err = copy_from_user(page_buf, buf+written, AS_PAGE_SIZE);
> +		if (err < 0) {
> +			err = -EFAULT;
> +			goto out;
> +		}
> +
> +		xmit_cmd(drvdata->gpios, AS_WRITE_ENABLE);
> +
> +		gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> +		/* op code */
> +		xmit_byte(drvdata->gpios, AS_PAGE_PROGRAM);
> +		/* address */
> +		xmit_byte(drvdata->gpios, (*ppos >> 16) & 0xff);
> +		xmit_byte(drvdata->gpios, (*ppos >> 8) & 0xff);
> +		xmit_byte(drvdata->gpios, *ppos & 0xff);
> +		/* page data (LSB first) */
> +		for (i = 0; i < AS_PAGE_SIZE; i++)
> +			xmit_byte(drvdata->gpios, bitrev8(page_buf[i]));
> +		gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
> +		mdelay(7);

So if I write 100 pages, I will _spin_ for 700 ms. You should rather
(m)sleep here.

> +
> +		*ppos += AS_PAGE_SIZE;
> +		written += AS_PAGE_SIZE;
> +	}
> +
> +out:
> +	kfree(page_buf);
> +	return err ?: written;
> +}
...
> +static int __init altera_as_probe(struct platform_device *pdev)
> +{
...
> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
> +			pdata->id);
> +	if (drvdata->miscdev.name == NULL)
> +		return -ENOMEM;
> +	drvdata->miscdev.fops = &altera_as_fops;
> +	if (misc_register(&drvdata->miscdev) < 0) {

Hmm, how many devices can be in the system? This doesn't look like the
right way.

> +		kfree(drvdata->miscdev.name);
> +		return -ENODEV;
> +	}
> +	altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
> +	altera_as_devs[pdata->id].drvdata = drvdata;

So now the device is usable without the mutex and gpios inited?

> +	mutex_init(&drvdata->open_lock);
> +
> +	drvdata->id = pdata->id;
> +
> +	drvdata->gpios[ASIO_DATA].gpio		= pdata->data;
> +	drvdata->gpios[ASIO_DATA].flags		= GPIOF_IN;
> +	drvdata->gpios[ASIO_DATA].label		= "as_data";
...
> +	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;
> +}

regards,
-- 
js
suse labs

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach]

Hi Jiri,

Thanks for reviewing. See my response to your comments below.

On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
> On 11/15/2010 09:23 AM, Baruch Siach wrote:
> > --- /dev/null
> > +++ b/drivers/misc/altera_as.c
> > @@ -0,0 +1,349 @@
> ...
> > +struct altera_as_device {
> > +	unsigned id;
> > +	struct device *dev;
> > +	struct miscdevice miscdev;
> > +	struct mutex open_lock;
> > +	struct gpio gpios[AS_GPIO_NUM];
> > +};
> > +
> > +/*
> > + * The only functions updating this array are .probe/.remove, which are
> > + * serialized. Hence, no locking is needed here.
> > + */
> > +static struct {
> > +	int minor;
> 
> Why you need minor here? It's in drvdata->miscdev.minor.

I use the minor field to lookup the drvdata pointer in get_as_dev(), which I 
use is altera_as_open().  I do this because I have no access to the 'struct 
device' pointer from the .open method. Do you know a better way?

> > +	struct altera_as_device *drvdata;
> > +} altera_as_devs[AS_MAX_DEVS];
> 
> You don't need the struct at all.
> static struct altera_as_device *drvdata[AS_MAX_DEVS];
> should be enough.

See above.

> > +static int altera_as_open(struct inode *inode, struct file *file)
> > +{
> > +	int ret;
> > +	struct altera_as_device *drvdata = get_as_dev(iminor(inode));
> > +
> > +	if (drvdata == NULL)
> > +		return -ENODEV;
> > +	file->private_data = drvdata;
> > +
> > +	ret = mutex_trylock(&drvdata->open_lock);
> > +	if (ret == 0)
> > +		return -EBUSY;
> > +
> > +	ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
> > +	if (ret < 0) {
> > +		mutex_unlock(&drvdata->open_lock);
> > +		return ret;
> > +	}
> 
> So leaving to userspace with mutex held. This is *wrong*. use
> test_and_set_bit or alike instead.

Can you explain why this is wrong? I don't mind doing test_and_set_bit 
instead, I'm just curious.

> > +	return 0;
> > +}
> > +
> > +static ssize_t altera_as_write(struct file *file, const char __user *buf,
> > +		size_t count, loff_t *ppos)
> > +{
> ...
> > +	while ((count - written) > 0) {
> > +		err = copy_from_user(page_buf, buf+written, AS_PAGE_SIZE);
> > +		if (err < 0) {
> > +			err = -EFAULT;
> > +			goto out;
> > +		}
> > +
> > +		xmit_cmd(drvdata->gpios, AS_WRITE_ENABLE);
> > +
> > +		gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
> > +		/* op code */
> > +		xmit_byte(drvdata->gpios, AS_PAGE_PROGRAM);
> > +		/* address */
> > +		xmit_byte(drvdata->gpios, (*ppos >> 16) & 0xff);
> > +		xmit_byte(drvdata->gpios, (*ppos >> 8) & 0xff);
> > +		xmit_byte(drvdata->gpios, *ppos & 0xff);
> > +		/* page data (LSB first) */
> > +		for (i = 0; i < AS_PAGE_SIZE; i++)
> > +			xmit_byte(drvdata->gpios, bitrev8(page_buf[i]));
> > +		gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
> > +		mdelay(7);
> 
> So if I write 100 pages, I will _spin_ for 700 ms. You should rather
> (m)sleep here.

OK.

> > +		*ppos += AS_PAGE_SIZE;
> > +		written += AS_PAGE_SIZE;
> > +	}
> > +
> > +out:
> > +	kfree(page_buf);
> > +	return err ?: written;
> > +}
> ...
> > +static int __init altera_as_probe(struct platform_device *pdev)
> > +{
> ...
> > +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
> > +			pdata->id);
> > +	if (drvdata->miscdev.name == NULL)
> > +		return -ENOMEM;
> > +	drvdata->miscdev.fops = &altera_as_fops;
> > +	if (misc_register(&drvdata->miscdev) < 0) {
> 
> Hmm, how many devices can be in the system?

Up to AS_MAX_DEVS, currently 16.

> This doesn't look like the right way.

What is the right way then?

> > +		kfree(drvdata->miscdev.name);
> > +		return -ENODEV;
> > +	}
> > +	altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
> > +	altera_as_devs[pdata->id].drvdata = drvdata;
> 
> So now the device is usable without the mutex and gpios inited?

I was thinking that the device lock which is being held during the .probe run, 
prevents the device from being open. After all I can still return -EGOAWAY at 
this point. Am I wrong?

If so, I'll reorder this code.

> > +	mutex_init(&drvdata->open_lock);
> > +
> > +	drvdata->id = pdata->id;
> > +
> > +	drvdata->gpios[ASIO_DATA].gpio		= pdata->data;
> > +	drvdata->gpios[ASIO_DATA].flags		= GPIOF_IN;
> > +	drvdata->gpios[ASIO_DATA].label		= "as_data";
> ...
> > +	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;
> > +}
> 
> regards,
> -- 
> js
> suse labs

Thanks,

baruch

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

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Jiri Slaby]

On 11/15/2010 11:08 AM, Baruch Siach wrote:
> Hi Jiri,
> 
> Thanks for reviewing. See my response to your comments below.
> 
> On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
>> On 11/15/2010 09:23 AM, Baruch Siach wrote:
>>> --- /dev/null
>>> +++ b/drivers/misc/altera_as.c
>>> @@ -0,0 +1,349 @@
>> ...
>>> +struct altera_as_device {
>>> +	unsigned id;
>>> +	struct device *dev;
>>> +	struct miscdevice miscdev;
>>> +	struct mutex open_lock;
>>> +	struct gpio gpios[AS_GPIO_NUM];
>>> +};
>>> +
>>> +/*
>>> + * The only functions updating this array are .probe/.remove, which are
>>> + * serialized. Hence, no locking is needed here.
>>> + */
>>> +static struct {
>>> +	int minor;
>>
>> Why you need minor here? It's in drvdata->miscdev.minor.
> 
> I use the minor field to lookup the drvdata pointer in get_as_dev(), which I 
> use is altera_as_open().  I do this because I have no access to the 'struct 
> device' pointer from the .open method. Do you know a better way?
> 
>>> +	struct altera_as_device *drvdata;
>>> +} altera_as_devs[AS_MAX_DEVS];
>>
>> You don't need the struct at all.
>> static struct altera_as_device *drvdata[AS_MAX_DEVS];
>> should be enough.
> 
> See above.

The answer to you previous question is here. You can just have a global
array of struct altera_as_device.

>>> +static int altera_as_open(struct inode *inode, struct file *file)
>>> +{
>>> +	int ret;
>>> +	struct altera_as_device *drvdata = get_as_dev(iminor(inode));
>>> +
>>> +	if (drvdata == NULL)
>>> +		return -ENODEV;
>>> +	file->private_data = drvdata;
>>> +
>>> +	ret = mutex_trylock(&drvdata->open_lock);
>>> +	if (ret == 0)
>>> +		return -EBUSY;
>>> +
>>> +	ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
>>> +	if (ret < 0) {
>>> +		mutex_unlock(&drvdata->open_lock);
>>> +		return ret;
>>> +	}
>>
>> So leaving to userspace with mutex held. This is *wrong*. use
>> test_and_set_bit or alike instead.
> 
> Can you explain why this is wrong? I don't mind doing test_and_set_bit 
> instead, I'm just curious.

The mutex has an owner which it expects to unlock it. For example if you
fork a process which already opened the device, you have a problem. This
is a technical point. Another point is that it's ugly to leave to
userspace with any lock.

>>> +static int __init altera_as_probe(struct platform_device *pdev)
>>> +{
>> ...
>>> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
>>> +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
>>> +			pdata->id);
>>> +	if (drvdata->miscdev.name == NULL)
>>> +		return -ENOMEM;
>>> +	drvdata->miscdev.fops = &altera_as_fops;
>>> +	if (misc_register(&drvdata->miscdev) < 0) {
>>
>> Hmm, how many devices can be in the system?
> 
> Up to AS_MAX_DEVS, currently 16.
> 
>> This doesn't look like the right way.
> 
> What is the right way then?

Ok, so for that count it definitely deserves its own major to not eat up
misc device space.

>>> +		kfree(drvdata->miscdev.name);
>>> +		return -ENODEV;
>>> +	}
>>> +	altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
>>> +	altera_as_devs[pdata->id].drvdata = drvdata;
>>
>> So now the device is usable without the mutex and gpios inited?
> 
> I was thinking that the device lock which is being held during the .probe run, 
> prevents the device from being open. After all I can still return -EGOAWAY at 
> this point. Am I wrong?
> 
> If so, I'll reorder this code.

This cannot be done easily. You need to set drvdata prior to minor and
after all the assignments here. The former because in get_as_dev you
test minor and return drvdata. The latter because you use open_lock &
gpios after playing with minor & drvdata.

Technically, it can be done with a use of barriers, but I don't
recommend it as drivers should not use barriers at all. You should
introduce some locking here (it introduces barriers on its own).

So move the assignment to altera_as_devs below the mutex_init & gpios
setup and lock it appropriately. Then add a lock to get_as_dev.

>>> +	mutex_init(&drvdata->open_lock);
>>> +
>>> +	drvdata->id = pdata->id;
>>> +
>>> +	drvdata->gpios[ASIO_DATA].gpio		= pdata->data;
>>> +	drvdata->gpios[ASIO_DATA].flags		= GPIOF_IN;
>>> +	drvdata->gpios[ASIO_DATA].label		= "as_data";
>> ...
>>> +	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;
>>> +}

regards,
-- 
js
suse labs

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach]

Hi Jiri,

On Mon, Nov 15, 2010 at 11:28:02AM +0100, Jiri Slaby wrote:
> On 11/15/2010 11:08 AM, Baruch Siach wrote:
> > Thanks for reviewing. See my response to your comments below.
> > 
> > On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
> >> On 11/15/2010 09:23 AM, Baruch Siach wrote:
> >>> --- /dev/null
> >>> +++ b/drivers/misc/altera_as.c
> >>> @@ -0,0 +1,349 @@
> >> ...
> >>> +struct altera_as_device {
> >>> +	unsigned id;
> >>> +	struct device *dev;
> >>> +	struct miscdevice miscdev;
> >>> +	struct mutex open_lock;
> >>> +	struct gpio gpios[AS_GPIO_NUM];
> >>> +};
> >>> +
> >>> +/*
> >>> + * The only functions updating this array are .probe/.remove, which are
> >>> + * serialized. Hence, no locking is needed here.
> >>> + */
> >>> +static struct {
> >>> +	int minor;
> >>
> >> Why you need minor here? It's in drvdata->miscdev.minor.
> > 
> > I use the minor field to lookup the drvdata pointer in get_as_dev(), which I 
> > use is altera_as_open().  I do this because I have no access to the 'struct 
> > device' pointer from the .open method. Do you know a better way?
> > 
> >>> +	struct altera_as_device *drvdata;
> >>> +} altera_as_devs[AS_MAX_DEVS];
> >>
> >> You don't need the struct at all.
> >> static struct altera_as_device *drvdata[AS_MAX_DEVS];
> >> should be enough.
> > 
> > See above.
> 
> The answer to you previous question is here. You can just have a global
> array of struct altera_as_device.

This static array would occupy sizeof(struct altera_as_device) * AS_MAX_DEVS 
== 128 (for 32bit) * 16 == 2KB unconditionally. This seems unreasonable to me.  
IMO struct altera_as_device should be allocated per device at run time.

> >>> +static int altera_as_open(struct inode *inode, struct file *file)
> >>> +{
> >>> +	int ret;
> >>> +	struct altera_as_device *drvdata = get_as_dev(iminor(inode));
> >>> +
> >>> +	if (drvdata == NULL)
> >>> +		return -ENODEV;
> >>> +	file->private_data = drvdata;
> >>> +
> >>> +	ret = mutex_trylock(&drvdata->open_lock);
> >>> +	if (ret == 0)
> >>> +		return -EBUSY;
> >>> +
> >>> +	ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
> >>> +	if (ret < 0) {
> >>> +		mutex_unlock(&drvdata->open_lock);
> >>> +		return ret;
> >>> +	}
> >>
> >> So leaving to userspace with mutex held. This is *wrong*. use
> >> test_and_set_bit or alike instead.
> > 
> > Can you explain why this is wrong? I don't mind doing test_and_set_bit 
> > instead, I'm just curious.
> 
> The mutex has an owner which it expects to unlock it. For example if you
> fork a process which already opened the device, you have a problem. This
> is a technical point. Another point is that it's ugly to leave to
> userspace with any lock.
> 
> >>> +static int __init altera_as_probe(struct platform_device *pdev)
> >>> +{
> >> ...
> >>> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> >>> +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
> >>> +			pdata->id);
> >>> +	if (drvdata->miscdev.name == NULL)
> >>> +		return -ENOMEM;
> >>> +	drvdata->miscdev.fops = &altera_as_fops;
> >>> +	if (misc_register(&drvdata->miscdev) < 0) {
> >>
> >> Hmm, how many devices can be in the system?
> > 
> > Up to AS_MAX_DEVS, currently 16.
> > 
> >> This doesn't look like the right way.
> > 
> > What is the right way then?
> 
> Ok, so for that count it definitely deserves its own major to not eat up
> misc device space.

A previous version of this driver[1] did just that. It was Greg KH who 
suggested[2] to use the misc class.

[1] http://article.gmane.org/gmane.linux.kernel/1057434
[2] http://article.gmane.org/gmane.linux.kernel/1058627

> >>> +		kfree(drvdata->miscdev.name);
> >>> +		return -ENODEV;
> >>> +	}
> >>> +	altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
> >>> +	altera_as_devs[pdata->id].drvdata = drvdata;
> >>
> >> So now the device is usable without the mutex and gpios inited?
> > 
> > I was thinking that the device lock which is being held during the .probe run, 
> > prevents the device from being open. After all I can still return -EGOAWAY at 
> > this point. Am I wrong?
> > 
> > If so, I'll reorder this code.
> 
> This cannot be done easily. You need to set drvdata prior to minor and
> after all the assignments here. The former because in get_as_dev you
> test minor and return drvdata.

When drvdata is not set it contains NULL. The .open method then just returns 
-ENODEV. So moving the drvdata assignment to the end of .probe should plug all 
race scenarios without any additional locking, isn't it?

> The latter because you use open_lock &
> gpios after playing with minor & drvdata.
> 
> Technically, it can be done with a use of barriers, but I don't
> recommend it as drivers should not use barriers at all. You should
> introduce some locking here (it introduces barriers on its own).
> 
> So move the assignment to altera_as_devs below the mutex_init & gpios
> setup and lock it appropriately. Then add a lock to get_as_dev.
> 
> >>> +	mutex_init(&drvdata->open_lock);
> >>> +
> >>> +	drvdata->id = pdata->id;
> >>> +
> >>> +	drvdata->gpios[ASIO_DATA].gpio		= pdata->data;
> >>> +	drvdata->gpios[ASIO_DATA].flags		= GPIOF_IN;
> >>> +	drvdata->gpios[ASIO_DATA].label		= "as_data";
> >> ...
> >>> +	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;
> >>> +}
> 
> regards,
> -- 
> js
> suse labs

baruch

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

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Jiri Slaby]

On 11/15/2010 02:40 PM, Baruch Siach wrote:
> On Mon, Nov 15, 2010 at 11:28:02AM +0100, Jiri Slaby wrote:
>> On 11/15/2010 11:08 AM, Baruch Siach wrote:
>>> Thanks for reviewing. See my response to your comments below.
>>>
>>> On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
>>>> On 11/15/2010 09:23 AM, Baruch Siach wrote:
>>>>> --- /dev/null
>>>>> +++ b/drivers/misc/altera_as.c
>>>>> @@ -0,0 +1,349 @@
>>>> ...
>>>>> +struct altera_as_device {
>>>>> +	unsigned id;
>>>>> +	struct device *dev;
>>>>> +	struct miscdevice miscdev;
>>>>> +	struct mutex open_lock;
>>>>> +	struct gpio gpios[AS_GPIO_NUM];
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * The only functions updating this array are .probe/.remove, which are
>>>>> + * serialized. Hence, no locking is needed here.
>>>>> + */
>>>>> +static struct {
>>>>> +	int minor;
>>>>
>>>> Why you need minor here? It's in drvdata->miscdev.minor.
>>>
>>> I use the minor field to lookup the drvdata pointer in get_as_dev(), which I 
>>> use is altera_as_open().  I do this because I have no access to the 'struct 
>>> device' pointer from the .open method. Do you know a better way?
>>>
>>>>> +	struct altera_as_device *drvdata;
>>>>> +} altera_as_devs[AS_MAX_DEVS];
>>>>
>>>> You don't need the struct at all.
>>>> static struct altera_as_device *drvdata[AS_MAX_DEVS];
>>>> should be enough.
>>>
>>> See above.
>>
>> The answer to you previous question is here. You can just have a global
>> array of struct altera_as_device.
> 
> This static array would occupy sizeof(struct altera_as_device) * AS_MAX_DEVS 
> == 128 (for 32bit) * 16 == 2KB unconditionally.

No, it is an array of pointers.

>>>>> +static int __init altera_as_probe(struct platform_device *pdev)
>>>>> +{
>>>> ...
>>>>> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
>>>>> +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
>>>>> +			pdata->id);
>>>>> +	if (drvdata->miscdev.name == NULL)
>>>>> +		return -ENOMEM;
>>>>> +	drvdata->miscdev.fops = &altera_as_fops;
>>>>> +	if (misc_register(&drvdata->miscdev) < 0) {
>>>>
>>>> Hmm, how many devices can be in the system?
>>>
>>> Up to AS_MAX_DEVS, currently 16.
>>>
>>>> This doesn't look like the right way.
>>>
>>> What is the right way then?
>>
>> Ok, so for that count it definitely deserves its own major to not eat up
>> misc device space.
> 
> A previous version of this driver[1] did just that. It was Greg KH who 
> suggested[2] to use the misc class.
> 
> [1] http://article.gmane.org/gmane.linux.kernel/1057434
> [2] http://article.gmane.org/gmane.linux.kernel/1058627

Ok, citing in-place:
<cite1>
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?
</cite1>

The "miscdevice" is intended for people who want only a single device
per system (singleton) and it is designed that way. There can be only up
to 64 dynamic minors. Here you allow up to 16 devices out of box...

<cite2>
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.
</cite2>

Well, how do people notify udev about their character devices then? Any
pointers to the discussion?

>>>>> +		kfree(drvdata->miscdev.name);
>>>>> +		return -ENODEV;
>>>>> +	}
>>>>> +	altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
>>>>> +	altera_as_devs[pdata->id].drvdata = drvdata;
>>>>
>>>> So now the device is usable without the mutex and gpios inited?
>>>
>>> I was thinking that the device lock which is being held during the .probe run, 
>>> prevents the device from being open. After all I can still return -EGOAWAY at 
>>> this point. Am I wrong?
>>>
>>> If so, I'll reorder this code.
>>
>> This cannot be done easily. You need to set drvdata prior to minor and
>> after all the assignments here. The former because in get_as_dev you
>> test minor and return drvdata.
> 
> When drvdata is not set it contains NULL. The .open method then just returns 
> -ENODEV. So moving the drvdata assignment to the end of .probe should plug all 
> race scenarios without any additional locking, isn't it?

How do you ensure gpios and drvdata to be in the memory before minor?
(Compilers and processors may reorder.)

regards,
-- 
js
suse labs

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Greg KH]

On Mon, Nov 15, 2010 at 03:24:56PM +0100, Jiri Slaby wrote:
> On 11/15/2010 02:40 PM, Baruch Siach wrote:
> > On Mon, Nov 15, 2010 at 11:28:02AM +0100, Jiri Slaby wrote:
> >> On 11/15/2010 11:08 AM, Baruch Siach wrote:
> >>> Thanks for reviewing. See my response to your comments below.
> >>>
> >>> On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
> >>>> On 11/15/2010 09:23 AM, Baruch Siach wrote:
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/misc/altera_as.c
> >>>>> @@ -0,0 +1,349 @@
> >>>> ...
> >>>>> +struct altera_as_device {
> >>>>> +	unsigned id;
> >>>>> +	struct device *dev;
> >>>>> +	struct miscdevice miscdev;
> >>>>> +	struct mutex open_lock;
> >>>>> +	struct gpio gpios[AS_GPIO_NUM];
> >>>>> +};
> >>>>> +
> >>>>> +/*
> >>>>> + * The only functions updating this array are .probe/.remove, which are
> >>>>> + * serialized. Hence, no locking is needed here.
> >>>>> + */
> >>>>> +static struct {
> >>>>> +	int minor;
> >>>>
> >>>> Why you need minor here? It's in drvdata->miscdev.minor.
> >>>
> >>> I use the minor field to lookup the drvdata pointer in get_as_dev(), which I 
> >>> use is altera_as_open().  I do this because I have no access to the 'struct 
> >>> device' pointer from the .open method. Do you know a better way?
> >>>
> >>>>> +	struct altera_as_device *drvdata;
> >>>>> +} altera_as_devs[AS_MAX_DEVS];
> >>>>
> >>>> You don't need the struct at all.
> >>>> static struct altera_as_device *drvdata[AS_MAX_DEVS];
> >>>> should be enough.
> >>>
> >>> See above.
> >>
> >> The answer to you previous question is here. You can just have a global
> >> array of struct altera_as_device.
> > 
> > This static array would occupy sizeof(struct altera_as_device) * AS_MAX_DEVS 
> > == 128 (for 32bit) * 16 == 2KB unconditionally.
> 
> No, it is an array of pointers.
> 
> >>>>> +static int __init altera_as_probe(struct platform_device *pdev)
> >>>>> +{
> >>>> ...
> >>>>> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> >>>>> +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
> >>>>> +			pdata->id);
> >>>>> +	if (drvdata->miscdev.name == NULL)
> >>>>> +		return -ENOMEM;
> >>>>> +	drvdata->miscdev.fops = &altera_as_fops;
> >>>>> +	if (misc_register(&drvdata->miscdev) < 0) {
> >>>>
> >>>> Hmm, how many devices can be in the system?
> >>>
> >>> Up to AS_MAX_DEVS, currently 16.
> >>>
> >>>> This doesn't look like the right way.
> >>>
> >>> What is the right way then?
> >>
> >> Ok, so for that count it definitely deserves its own major to not eat up
> >> misc device space.
> > 
> > A previous version of this driver[1] did just that. It was Greg KH who 
> > suggested[2] to use the misc class.
> > 
> > [1] http://article.gmane.org/gmane.linux.kernel/1057434
> > [2] http://article.gmane.org/gmane.linux.kernel/1058627
> 
> Ok, citing in-place:
> <cite1>
> 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?
> </cite1>
> 
> The "miscdevice" is intended for people who want only a single device
> per system (singleton) and it is designed that way. There can be only up
> to 64 dynamic minors. Here you allow up to 16 devices out of box...

I thought this driver only allocated one single minor number.  If it is
doing this for 16 all the time, then it should get its own major, sorry
my fault.

> <cite2>
> 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.
> </cite2>
> 
> Well, how do people notify udev about their character devices then? Any
> pointers to the discussion?

Use a struct bus_type instead.

thanks,

greg k-h

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Indan Zupancic]

Hello,

On Mon, November 15, 2010 11:28, Jiri Slaby wrote:
> On 11/15/2010 11:08 AM, Baruch Siach wrote:
>> Hi Jiri,
>>
>> Thanks for reviewing. See my response to your comments below.
>>
>> On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
>>> On 11/15/2010 09:23 AM, Baruch Siach wrote:
>>>> --- /dev/null
>>>> +++ b/drivers/misc/altera_as.c
>>>> @@ -0,0 +1,349 @@
>>> ...
>>>> +struct altera_as_device {
>>>> +	unsigned id;
>>>> +	struct device *dev;
>>>> +	struct miscdevice miscdev;
>>>> +	struct mutex open_lock;
>>>> +	struct gpio gpios[AS_GPIO_NUM];
>>>> +};
>>>> +
>>>> +/*
>>>> + * The only functions updating this array are .probe/.remove, which are
>>>> + * serialized. Hence, no locking is needed here.
>>>> + */
>>>> +static struct {
>>>> +	int minor;
>>>
>>> Why you need minor here? It's in drvdata->miscdev.minor.
>>
>> I use the minor field to lookup the drvdata pointer in get_as_dev(), which I
>> use is altera_as_open().  I do this because I have no access to the 'struct
>> device' pointer from the .open method. Do you know a better way?

Yes.

Get rid of the array and minor lookup, and instead use container_of()
to get struct altera_as_device directly from file->private_data, which
is a pointer to miscdev, set by misc_open().

Sorry for not noticing this before.

>>
>>>> +	struct altera_as_device *drvdata;
>>>> +} altera_as_devs[AS_MAX_DEVS];
>>>
>>> You don't need the struct at all.
>>> static struct altera_as_device *drvdata[AS_MAX_DEVS];
>>> should be enough.
>>
>> See above.
>
> The answer to you previous question is here. You can just have a global
> array of struct altera_as_device.
>
>>>> +static int altera_as_open(struct inode *inode, struct file *file)
>>>> +{
>>>> +	int ret;
>>>> +	struct altera_as_device *drvdata = get_as_dev(iminor(inode));
>>>> +
>>>> +	if (drvdata == NULL)
>>>> +		return -ENODEV;
>>>> +	file->private_data = drvdata;
>>>> +
>>>> +	ret = mutex_trylock(&drvdata->open_lock);
>>>> +	if (ret == 0)
>>>> +		return -EBUSY;
>>>> +
>>>> +	ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
>>>> +	if (ret < 0) {
>>>> +		mutex_unlock(&drvdata->open_lock);
>>>> +		return ret;
>>>> +	}
>>>
>>> So leaving to userspace with mutex held. This is *wrong*. use
>>> test_and_set_bit or alike instead.
>>
>> Can you explain why this is wrong? I don't mind doing test_and_set_bit
>> instead, I'm just curious.
>
> The mutex has an owner which it expects to unlock it. For example if you
> fork a process which already opened the device, you have a problem. This
> is a technical point. Another point is that it's ugly to leave to
> userspace with any lock.
>
>>>> +static int __init altera_as_probe(struct platform_device *pdev)
>>>> +{
>>> ...
>>>> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
>>>> +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
>>>> +			pdata->id);
>>>> +	if (drvdata->miscdev.name == NULL)
>>>> +		return -ENOMEM;
>>>> +	drvdata->miscdev.fops = &altera_as_fops;
>>>> +	if (misc_register(&drvdata->miscdev) < 0) {
>>>
>>> Hmm, how many devices can be in the system?
>>
>> Up to AS_MAX_DEVS, currently 16.
>>
>>> This doesn't look like the right way.
>>
>> What is the right way then?
>
> Ok, so for that count it definitely deserves its own major to not eat up
> misc device space.

Ther are only as many minors allocates as needed, not always 16.
Almost always there will be only one FPGA, maybe a couple, but
more is very rare. And when that's the case there are probably
not many other misc devices around either, or they use a more
scalable way to program many FPGAs.

This is niche enough, don't make it more complicated than needed.
Having its own major seems overkill.

>>>> +		kfree(drvdata->miscdev.name);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +	altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
>>>> +	altera_as_devs[pdata->id].drvdata = drvdata;
>>>
>>> So now the device is usable without the mutex and gpios inited?
>>
>> I was thinking that the device lock which is being held during the .probe
>> run,
>> prevents the device from being open. After all I can still return -EGOAWAY
>> at
>> this point. Am I wrong?
>>
>> If so, I'll reorder this code.
>
> This cannot be done easily. You need to set drvdata prior to minor and
> after all the assignments here. The former because in get_as_dev you
> test minor and return drvdata. The latter because you use open_lock &
> gpios after playing with minor & drvdata.
>
> Technically, it can be done with a use of barriers, but I don't
> recommend it as drivers should not use barriers at all. You should
> introduce some locking here (it introduces barriers on its own).
>
> So move the assignment to altera_as_devs below the mutex_init & gpios
> setup and lock it appropriately. Then add a lock to get_as_dev.

Just put the misc_register() call at the end and it should be fine,
once the array is gone.

>>>> +	mutex_init(&drvdata->open_lock);
>>>> +
>>>> +	drvdata->id = pdata->id;
>>>> +
>>>> +	drvdata->gpios[ASIO_DATA].gpio		= pdata->data;
>>>> +	drvdata->gpios[ASIO_DATA].flags		= GPIOF_IN;
>>>> +	drvdata->gpios[ASIO_DATA].label		= "as_data";
>>> ...
>>>> +	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;
>>>> +}
>
> regards,
> --
> js
> suse labs
>

Greetings,

Indan

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach]

Hi Indan,

On Tue, Nov 16, 2010 at 01:47:52AM +0100, Indan Zupancic wrote:
> On Mon, November 15, 2010 11:28, Jiri Slaby wrote:
> > On 11/15/2010 11:08 AM, Baruch Siach wrote:
> >> Hi Jiri,
> >>
> >> Thanks for reviewing. See my response to your comments below.
> >>
> >> On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
> >>> On 11/15/2010 09:23 AM, Baruch Siach wrote:
> >>>> --- /dev/null
> >>>> +++ b/drivers/misc/altera_as.c
> >>>> @@ -0,0 +1,349 @@
> >>> ...
> >>>> +struct altera_as_device {
> >>>> +	unsigned id;
> >>>> +	struct device *dev;
> >>>> +	struct miscdevice miscdev;
> >>>> +	struct mutex open_lock;
> >>>> +	struct gpio gpios[AS_GPIO_NUM];
> >>>> +};
> >>>> +
> >>>> +/*
> >>>> + * The only functions updating this array are .probe/.remove, which are
> >>>> + * serialized. Hence, no locking is needed here.
> >>>> + */
> >>>> +static struct {
> >>>> +	int minor;
> >>>
> >>> Why you need minor here? It's in drvdata->miscdev.minor.
> >>
> >> I use the minor field to lookup the drvdata pointer in get_as_dev(), which I
> >> use is altera_as_open().  I do this because I have no access to the 'struct
> >> device' pointer from the .open method. Do you know a better way?
> 
> Yes.
> 
> Get rid of the array and minor lookup, and instead use container_of()
> to get struct altera_as_device directly from file->private_data, which
> is a pointer to miscdev, set by misc_open().

Thanks. This was exactly what I was looking for.

> Sorry for not noticing this before.
> 
> >>
> >>>> +	struct altera_as_device *drvdata;
> >>>> +} altera_as_devs[AS_MAX_DEVS];
> >>>
> >>> You don't need the struct at all.
> >>> static struct altera_as_device *drvdata[AS_MAX_DEVS];
> >>> should be enough.
> >>
> >> See above.
> >
> > The answer to you previous question is here. You can just have a global
> > array of struct altera_as_device.
> >
> >>>> +static int altera_as_open(struct inode *inode, struct file *file)
> >>>> +{
> >>>> +	int ret;
> >>>> +	struct altera_as_device *drvdata = get_as_dev(iminor(inode));
> >>>> +
> >>>> +	if (drvdata == NULL)
> >>>> +		return -ENODEV;
> >>>> +	file->private_data = drvdata;
> >>>> +
> >>>> +	ret = mutex_trylock(&drvdata->open_lock);
> >>>> +	if (ret == 0)
> >>>> +		return -EBUSY;
> >>>> +
> >>>> +	ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
> >>>> +	if (ret < 0) {
> >>>> +		mutex_unlock(&drvdata->open_lock);
> >>>> +		return ret;
> >>>> +	}
> >>>
> >>> So leaving to userspace with mutex held. This is *wrong*. use
> >>> test_and_set_bit or alike instead.
> >>
> >> Can you explain why this is wrong? I don't mind doing test_and_set_bit
> >> instead, I'm just curious.
> >
> > The mutex has an owner which it expects to unlock it. For example if you
> > fork a process which already opened the device, you have a problem. This
> > is a technical point. Another point is that it's ugly to leave to
> > userspace with any lock.
> >
> >>>> +static int __init altera_as_probe(struct platform_device *pdev)
> >>>> +{
> >>> ...
> >>>> +	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> >>>> +	drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
> >>>> +			pdata->id);
> >>>> +	if (drvdata->miscdev.name == NULL)
> >>>> +		return -ENOMEM;
> >>>> +	drvdata->miscdev.fops = &altera_as_fops;
> >>>> +	if (misc_register(&drvdata->miscdev) < 0) {
> >>>
> >>> Hmm, how many devices can be in the system?
> >>
> >> Up to AS_MAX_DEVS, currently 16.
> >>
> >>> This doesn't look like the right way.
> >>
> >> What is the right way then?
> >
> > Ok, so for that count it definitely deserves its own major to not eat up
> > misc device space.
> 
> Ther are only as many minors allocates as needed, not always 16.
> Almost always there will be only one FPGA, maybe a couple, but
> more is very rare. And when that's the case there are probably
> not many other misc devices around either, or they use a more
> scalable way to program many FPGAs.
> 
> This is niche enough, don't make it more complicated than needed.
> Having its own major seems overkill.

I agree.

> >>>> +		kfree(drvdata->miscdev.name);
> >>>> +		return -ENODEV;
> >>>> +	}
> >>>> +	altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
> >>>> +	altera_as_devs[pdata->id].drvdata = drvdata;
> >>>
> >>> So now the device is usable without the mutex and gpios inited?
> >>
> >> I was thinking that the device lock which is being held during the .probe
> >> run,
> >> prevents the device from being open. After all I can still return -EGOAWAY
> >> at
> >> this point. Am I wrong?
> >>
> >> If so, I'll reorder this code.
> >
> > This cannot be done easily. You need to set drvdata prior to minor and
> > after all the assignments here. The former because in get_as_dev you
> > test minor and return drvdata. The latter because you use open_lock &
> > gpios after playing with minor & drvdata.
> >
> > Technically, it can be done with a use of barriers, but I don't
> > recommend it as drivers should not use barriers at all. You should
> > introduce some locking here (it introduces barriers on its own).
> >
> > So move the assignment to altera_as_devs below the mutex_init & gpios
> > setup and lock it appropriately. Then add a lock to get_as_dev.
> 
> Just put the misc_register() call at the end and it should be fine,
> once the array is gone.

Thanks again. You were immensely helpful.

Going to remove some lines of code. I love the sound it makes.

baruch

> >>>> +	mutex_init(&drvdata->open_lock);
> >>>> +
> >>>> +	drvdata->id = pdata->id;
> >>>> +
> >>>> +	drvdata->gpios[ASIO_DATA].gpio		= pdata->data;
> >>>> +	drvdata->gpios[ASIO_DATA].flags		= GPIOF_IN;
> >>>> +	drvdata->gpios[ASIO_DATA].label		= "as_data";
> >>> ...
> >>>> +	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;
> >>>> +}

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

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[H. Peter Anvin]

On 11/15/2010 09:56 PM, Baruch Siach wrote:
>>
>> This is niche enough, don't make it more complicated than needed.
>> Having its own major seems overkill.
> 
> I agree.
> 

Numbers are cheap, especially these days when we have 12-bit dynamic
majors.  Of course, we also have 20-bit dynamic minors.

	-hpa

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

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Alan Cox]

Sixteen devices is way too many for drivers/misc - it only has 256 total
and only half of those are dynamic.

NAK use of drivers/misc.

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Alan Cox]

Or to be more clear

NAK use of misc_register etc.

I don't care if it ends up in drivers/misc as a directory but it should
allocate its own dynamic device major.

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Jiri Slaby]

On 11/16/2010 12:04 PM, Alan Cox wrote:
> Sixteen devices is way too many for drivers/misc - it only has 256 total
> and only half of those are dynamic.

Seconded. (And it's a quarter, not even a half like I already wrote.)

regards,
-- 
js
suse labs

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach]

Hi Jiri, Alan,

On Tue, Nov 16, 2010 at 02:09:45PM +0100, Jiri Slaby wrote:
> On 11/16/2010 12:04 PM, Alan Cox wrote:
> > Sixteen devices is way too many for drivers/misc - it only has 256 total
> > and only half of those are dynamic.
> 
> Seconded. (And it's a quarter, not even a half like I already wrote.)

If I reduce the maximal number of devices to 4, will it make any difference? I 
don't mind even having a limit of 2 by default, as long as a simple define 
change is enough to raise this limit.

baruch

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

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[H. Peter Anvin]

On 11/16/2010 09:32 PM, Baruch Siach wrote:
> Hi Jiri, Alan,
> 
> On Tue, Nov 16, 2010 at 02:09:45PM +0100, Jiri Slaby wrote:
>> On 11/16/2010 12:04 PM, Alan Cox wrote:
>>> Sixteen devices is way too many for drivers/misc - it only has 256 total
>>> and only half of those are dynamic.
>>
>> Seconded. (And it's a quarter, not even a half like I already wrote.)
> 
> If I reduce the maximal number of devices to 4, will it make any difference? I 
> don't mind even having a limit of 2 by default, as long as a simple define 
> change is enough to raise this limit.
> 
Just use a major.  No biggie.

	-hpa


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

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach]

Hi H. Peter,

On Tue, Nov 16, 2010 at 09:43:39PM -0800, H. Peter Anvin wrote:
> On 11/16/2010 09:32 PM, Baruch Siach wrote:
> > On Tue, Nov 16, 2010 at 02:09:45PM +0100, Jiri Slaby wrote:
> >> On 11/16/2010 12:04 PM, Alan Cox wrote:
> >>> Sixteen devices is way too many for drivers/misc - it only has 256 total
> >>> and only half of those are dynamic.
> >>
> >> Seconded. (And it's a quarter, not even a half like I already wrote.)
> > 
> > If I reduce the maximal number of devices to 4, will it make any difference? I 
> > don't mind even having a limit of 2 by default, as long as a simple define 
> > change is enough to raise this limit.
> > 
> Just use a major.  No biggie.

And create my own 'struct class'?

baruch

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

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Thomas Chou]

On 11/15/2010 04:23 PM, Baruch Siach wrote:
> From: Alex Gershgorin<agersh@rambler.ru>
>
> The active serial protocol can be used to program Altera serial configuration
> devices. This driver uses the kernel gpio interface to implement the active
> serial protocol.
>
> This patch also introduces the include/platform_drivers/ directory, which is
> the new home for platform data headers. This is per Greg's suggestion.
>

Hi Alex,

Altera EPCS is normal SPI serial flash, which is exactly the same as 
STmicro part. Please consider existing drivers/mtd/devices/m25p80.c and 
drivers/spi/spi_gpio.c.

Best regards,
Thomas

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Jiri Slaby]

On 11/17/2010 06:48 AM, Baruch Siach wrote:
>> Just use a major.  No biggie.
> 
> And create my own 'struct class'?

No, use bus_type struct as Greg suggested.

regards,
-- 
js
suse labs

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Alan Cox]

On Wed, 17 Nov 2010 07:32:51 +0200
Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Jiri, Alan,
> 
> On Tue, Nov 16, 2010 at 02:09:45PM +0100, Jiri Slaby wrote:
> > On 11/16/2010 12:04 PM, Alan Cox wrote:
> > > Sixteen devices is way too many for drivers/misc - it only has 256 total
> > > and only half of those are dynamic.
> > 
> > Seconded. (And it's a quarter, not even a half like I already wrote.)
> 
> If I reduce the maximal number of devices to 4, will it make any difference? I 
> don't mind even having a limit of 2 by default, as long as a simple define 
> change is enough to raise this limit.

No look you can need more than two, its not a one off special device it
doesn't use the misc minor space.

Allocate some dynamic device space.

Alan

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach]

Hi Thomas,

On Wed, Nov 17, 2010 at 02:18:36PM +0800, Thomas Chou wrote:
> On 11/15/2010 04:23 PM, Baruch Siach wrote:
> >From: Alex Gershgorin<agersh@rambler.ru>
> >
> >The active serial protocol can be used to program Altera serial configuration
> >devices. This driver uses the kernel gpio interface to implement the active
> >serial protocol.
> >
> >This patch also introduces the include/platform_drivers/ directory, which is
> >the new home for platform data headers. This is per Greg's suggestion.
> 
> Altera EPCS is normal SPI serial flash, which is exactly the same as
> STmicro part. Please consider existing drivers/mtd/devices/m25p80.c
> and drivers/spi/spi_gpio.c.

Thanks for the pointer. Looking at the op codes and status bits it seems you 
are right. The 38 pages Altera datasheet doesn't mention the terms SPI or 
m25p80. Go figure.

I'll test you suggestion.

baruch

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

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Thomas Chou]

Hi hpa,

On 11/18/2010 02:52 AM, H. Peter Anvin wrote:
> Do you happen to have a sample STmicro part number?  Also, are you
> saying that EPCS is *only* serial flash, or only for the purpose of
> being programmed (as opposed to programming the FPGA afterwards)?  It
> just seems a bit surprising to me, since Xilinx right now seem to have a
> bit of an advantage over Altera in being able to boot from stock SPI
> flash...

EPCS16 = M25P16
EPCS64 = M25P64

Yes, EPCS is ONLY serial flash. The Altera EPCS controller is only SPI 
controller. It is well-known on Altera wiki and Altera forum.

You may use any stock SPI flash to replace EPCS as long as the access 
time met. Though Altera flash programming utility supports only limited 
brands, more SPI flash can be programmed using u-boot or Linux MTD. We 
often boot right out of SPI/EPCS, as it saves cost and space.

There is no difference between Altera and Xilinx in this aspect, except 
for their marketing approach.

Cheers,
Thomas
http://sopc.et.ntust.edu.tw/

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[H. Peter Anvin]

On 11/17/2010 06:15 PM, Thomas Chou wrote:
> 
> EPCS16 = M25P16
> EPCS64 = M25P64
> 
> Yes, EPCS is ONLY serial flash. The Altera EPCS controller is only SPI 
> controller. It is well-known on Altera wiki and Altera forum.
> 
> You may use any stock SPI flash to replace EPCS as long as the access 
> time met. Though Altera flash programming utility supports only limited 
> brands, more SPI flash can be programmed using u-boot or Linux MTD. We 
> often boot right out of SPI/EPCS, as it saves cost and space.
> 
> There is no difference between Altera and Xilinx in this aspect, except 
> for their marketing approach.
> 

OK, that's rather funny.

	-hpa

Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach]

Hi Thomas,

On Wed, Nov 17, 2010 at 02:18:36PM +0800, Thomas Chou wrote:
> On 11/15/2010 04:23 PM, Baruch Siach wrote:
> >From: Alex Gershgorin<agersh@rambler.ru>
> >
> >The active serial protocol can be used to program Altera serial configuration
> >devices. This driver uses the kernel gpio interface to implement the active
> >serial protocol.
> >
> >This patch also introduces the include/platform_drivers/ directory, which is
> >the new home for platform data headers. This is per Greg's suggestion.
> 
> Altera EPCS is normal SPI serial flash, which is exactly the same as
> STmicro part. Please consider existing drivers/mtd/devices/m25p80.c
> and drivers/spi/spi_gpio.c.

For the record, I tested your suggestion and it works perfectly. Thanks.

Thank you everyone for your helpful suggestion regarding the, now abandoned, 
Altera active serial implementation driver.

baruch

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