c2port (Version 2): support for Silicon C2 Interface.

Signed-off-by Rodolfo Giometti <[email protected]>

C2port implements a two wire serial communication protocol (bit banging) designed to enable in-system programming, debugging, and boundary-scan testing on low pin-count Silicon Labs devices.

Currently this code supports only flash programming through sysfs interface but extensions shoud be easy to add.

Signed-off-by Rodolfo Giometti <[email protected]>

Quoted 8 lines · Rodolfo Giometti

C2port implements a two wire serial communication protocol (bit banging) designed to enable in-system programming, debugging, and boundary-scan testing on low pin-count Silicon Labs devices.

Currently this code supports only flash programming through sysfs interface but extensions shoud be easy to add.

Signed-off-by Rodolfo Giometti <[email protected]>

Acked-by Greg Kroah-Hartman <[email protected]>

Nice job.

Andrew, no reason why these 2 patches can't go into 2.6.28, they are self-contained and are new drivers.

Do you want to send them to Linus, or want me to?

No opinion yet - I haven't review them.

<looks>

<wonders why c2port_idr_lock and c2port_idr are kernel-wide symbols>

<wonders what local_irq_disable() is supposed to achieve on SMP>

<spots an error-path memory leak in c2port_device_register>

<spots a stray semicolon in c2port_device_register>

<suggests that update_lock be initialised at compile-time, not at runtime>

What I haven't yet got my head around is this:

Currently this code supports only flash programming through sysfs interface but extensions shoud be easy to add.

is that really what we want to use sysfs for? As the prime interface to a device driver (or is it a bus driver?) Didn't we used to use device nodes for that sort of thing?

Quoted 5 lines · Andrew Morton

No opinion yet - I haven't review them.

<looks>

<wonders why c2port_idr_lock and c2port_idr are kernel-wide symbols>

Fixed.

Quoted 1 line

<wonders what local_irq_disable() is supposed to achieve on SMP>

The SMP is achieved by mutex_lock(), the local_irq_disable() is used to avoid interructions in code execution. C2 port has very stringent timings.

Quoted 3 lines

<spots an error-path memory leak in c2port_device_register>

<spots a stray semicolon in c2port_device_register>

Can you please explain these two points?

drivers/misc/c2port/core.c new file +8 -0

+struct c2port_device *c2port_device_register(char *name,
+					struct c2port_ops *ops, void *devdata)
+{
+	struct c2port_device *c2dev;
+	int id, ret;
+
+	if (unlikely(!ops) || unlikely(!ops->access) || \
+		unlikely(!ops->c2d_dir) || unlikely(!ops->c2ck_set) || \

drivers/misc/c2port/core.c new file +4 -0

+	spin_lock_irq(&c2port_idr_lock);
+	ret = idr_get_new(&c2port_idr, c2dev, &id);
+	spin_unlock_irq(&c2port_idr_lock);
+

Quoted 1 line

Quoted 1 line

<suggests that update_lock be initialised at compile-time, not at runtime>

Fixed.

Quoted 6 lines

What I haven't yet got my head around is this:

Currently this code supports only flash programming through sysfs interface but extensions shoud be easy to add.

Quoted 3 lines

is that really what we want to use sysfs for? As the prime interface to a device driver (or is it a bus driver?) Didn't we used to use device nodes for that sort of thing?

I decided to use this interface since the C2 ports are simple and very easy to manage with such files abstraction.

Whould you like I repropose the code with the fixed issues for your review?

Quoted 5 lines

Quoted 1 line

<wonders what local_irq_disable() is supposed to achieve on SMP>

The SMP is achieved by mutex_lock(), the local_irq_disable() is used to avoid interructions in code execution. C2 port has very stringent timings.

Please add code comments explaning this (unless I missed them).

drivers/misc/c2port/core.c line 894

Quoted 2 lines

drivers/misc/c2port/core.c new file +2 -0

+	if (!ret)
+		return ERR_PTR(-ENOMEM);

^^ leaks c2dev

drivers/misc/c2port/core.c line 909

Quoted 4 lines

drivers/misc/c2port/core.c new file +4 -0

+	if (unlikely(!c2dev->dev)) {
+		ret = -ENOMEM;
+		goto error_device_create;
+	};

         ^

Quoted 9 lines

Quoted 8 lines

What I haven't yet got my head around is this:

Currently this code supports only flash programming through sysfs interface but extensions shoud be easy to add.

is that really what we want to use sysfs for? As the prime interface to a device driver (or is it a bus driver?) Didn't we used to use device nodes for that sort of thing?

Quoted 2 lines

I decided to use this interface since the C2 ports are simple and very easy to manage with such files abstraction.

well.. what _is_ the implemented interface? What alternatives were considered and why were they judged inappropriate? What interfaces do any similar in-tree drivers use?

Quoted 2 lines

Whould you like I repropose the code with the fixed issues for your review?

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

What I haven't yet got my head around is this:

Currently this code supports only flash programming through sysfs interface but extensions shoud be easy to add.

is that really what we want to use sysfs for? As the prime interface to a device driver (or is it a bus driver?) Didn't we used to use device nodes for that sort of thing?

I decided to use this interface since the C2 ports are simple and very easy to manage with such files abstraction.

Quoted 3 lines

well.. what _is_ the implemented interface? What alternatives were considered and why were they judged inappropriate? What interfaces do any similar in-tree drivers use?

Well... main reasons in using sysfs support are:

* no more char devices and new ioctls to define,

* the microcontrollers on-board flash can be easily programmed by using only the "echo" and "cat" commands, so no more tools are needed and even simply busybox can be used.

Quoted 12 lines · On Wed, 22 Oct 2008 22:48:01 +0200 Rodolfo Giometti <[email protected]>…

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

What I haven't yet got my head around is this:

Currently this code supports only flash programming through sysfs interface but extensions shoud be easy to add.

is that really what we want to use sysfs for? As the prime interface to a device driver (or is it a bus driver?) Didn't we used to use device nodes for that sort of thing?

I decided to use this interface since the C2 ports are simple and very easy to manage with such files abstraction.

Quoted 8 lines

Quoted 3 lines

well.. what _is_ the implemented interface? What alternatives were considered and why were they judged inappropriate? What interfaces do any similar in-tree drivers use?

Well... main reasons in using sysfs support are:

* no more char devices and new ioctls to define,

Why is that good? We have hundreds and hundreds of drivers which use a device node interface and they work OK.

sysfs was never intended for use as the sole interface between device drivers and userspace. Using it that way is somewhat unconventional. At the very least I do think it needs more justification and explanation than we're being provded here.

Device nodes and ioctls and such do have a number of problems, and perhaps we should sit down and invent a new way of wiring drivers up to userspace. But whether that would end up being identical to the present sysfs code is not obvious.

Quoted 3 lines

* the microcontrollers on-board flash can be easily programmed by using only the "echo" and "cat" commands, so no more tools are needed and even simply busybox can be used.

A system which is using busybox might want to set CONFIG_SYSFS=n for memory savings. Now what?

Quoted 7 lines

Why is that good? We have hundreds and hundreds of drivers which use a device node interface and they work OK.

sysfs was never intended for use as the sole interface between device drivers and userspace. Using it that way is somewhat unconventional. At the very least I do think it needs more justification and explanation than we're being provded here.

For firmware download to devices, using sysfs is the "normal" interface these days, that is what the firmware class is for.

Which reminds me, Rodolfo, why didn't you just use that interface for your firmware downloading instead of rolling your own here?

Quoted 8 lines · Greg KH

Quoted 7 lines

Why is that good? We have hundreds and hundreds of drivers which use a device node interface and they work OK.

sysfs was never intended for use as the sole interface between device drivers and userspace. Using it that way is somewhat unconventional. At the very least I do think it needs more justification and explanation than we're being provded here.

Quoted 5 lines

For firmware download to devices, using sysfs is the "normal" interface these days, that is what the firmware class is for.

Which reminds me, Rodolfo, why didn't you just use that interface for your firmware downloading instead of rolling your own here?

Because I need also to know revision ID, device ID and other minor info from the microcontroller.

Also I can reset the microcontroller when I need it without downloading any new firmware.

And again, you can still use sysfs for that as well. I was just referring to the binary sysfs file that you use for the firmware download, it would make more sense for you to use the standard kernel firmware download interface instead of rolling your own here.

Quoted 4 lines · Greg KH

And again, you can still use sysfs for that as well. I was just referring to the binary sysfs file that you use for the firmware download, it would make more sense for you to use the standard kernel firmware download interface instead of rolling your own here.

If I well understand the firmware class it works starting the firmware download from the kernel side, that is into the kernel we should call request_firmware() and then the files will /sys/class/firmware/xxx/{loading,data} appear.

This solution doesn't fit C2 port needs since the firmware download is decided from userland erasing and then writing into the microcontroller on-board flash.