[Xenomai-core] Re: [Xenomai-help] Draft for a RTDM I2C driver

[Dirk Eibach <eibach@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 4695 bytes --]

>> At the moment the devicename is defined by the caller of
>> rti2c_adapter_register().
>> When the device is registered a number is assigned to the adapter. Maybe
>> the devicename could be made up of some fixed text like "rti2c" followed
>> by the number that is assigned to the adapter.
> 
> That was the idea behind it. Given the RTI2C is generic (and it looks
> like it is), you can than write a generic application, opening "rti2c0"
> more or less blindly without knowing the adapter behind it.

Fixed.

>>>>   // set the address of the device on the bus
>>>>   rt_dev_ioctl(fd, RTI2C_SLAVE, addr);
>>> Is the typical use case to not change the slave address that often,
>>> rather to use separate device instances for accessing different slaves?
>>> I'm wondering if a combined address+request command may make sense.
>>> Maybe even a socket-based protocol device would match as well, maybe
>>> even better... (needs more thinking, I guess)
>>>
>>> Does Linux expose a similar API via some character devices? Keeping the
>>> distance to Linux low might be a reason to not go the socket path.
>> The linux API is exactly the same. I had the same thoughts concerning
>> combined commands (address+request) but maybe we could offer such
>> commands as wrappers.
> 
> My concern was rather about performance than convenience. If a usage
> pattern consists of almost as many address-set driver invocations as
> actual requests, then you would benefit quite a lot from a combined
> service call. But the question is, given that Linux uses the same API,
> if that is really an expected use case and worth an optimisation effort.

For the moment I'll keep it the way it is.

> ...
>>> A few thoughts on this:
>>>
>>>  - What are typical delays between 4. and 5.? Does it makes sense for
>>>    short requests to busy-wait (to avoid the scheduler/IRQ overhead)?
>>>    I've seen that there is some polling path included in the code, but
>>>    that would break as it calls into Linux.
>> Oops. I missed that schedule() call. Regarding typical delays I have to
>> admit that I have not measured yet, but I would estimate about 500 us
>> for the above usecase. What would you estimate the scheduler/IRQ overhead?
> 
> Oh, 500 us is far more than you should see as "suspend-me +
> switch-to-someone-else + raise-irq-and-switch-back-to-me" overhead even
> on low-end PPC. Busy-waiting is something for a few microseconds.

Ah, all right, so I leave this as is too.

>>>  - Will a request always return? Or does it make sense to establish an
>>>    (optional) timeout mechanism here?
>> A timeout mechanism is already there: rti2c_ppc4xx_wait_for_tc() uses
>> rtdm_event_timedwait to wait for the interrupt to complete. Certainly
>> that is left to the implementation of the adapter.
> 
> True, I missed this.
> 
>>>  - Buffer allocation for short requests may also happen on the stack, I
>>>    think.
>> That is already done. Have a look at the RTI2C_SMBUS IOCTL. Do you think
>> it should also be possible to do this in the read/write calls denpending
>> on the requested size?
> 
> I currently see allocations in RTI2C_RDWR (BTW, there is a forgotten
> kmalloc) and in read/write. As I don't know the typical sizes of those
> requests, I cannot judge on this. So take it as some food to think about.
> 
> In contrast, the rtdm_malloc on adapter registration is overkill - this
> will not happen in RT context, will it?

Fixed. Still not sure what to do about the memory allocations in the
other places.

>>>  - Buffer allocation for large requests may (optionally) happen
>>>    ahead-of-time via some special IOCTL. This would make a device
>>>    independent of the current system heap usage/fragmentation.
>>>
>>>  - During concurrent use, the latency of an user is defined by its
>>>    priority, of course, and the number and lengths of potentially issued
>>>    request of some lower priority user, right? Is there a way one could
>>>    intercept a pending request list? Or is this list handled in toto to
>>>    the hardware? Melts down to "how to manage the bandwidth according to
>>>    the user's priority".
>> Concurrent use means that single RTI2C requests are called from
>> different tasks. Each request is atomic (and usually quite small). So
>> when a low-pri thread does a lot of requests a high-pri thread can get
>> inbetween anytime. I think this way bandwidth is already managed properly.
> 
> So there is no interface where you can submit several requests as an
> atomic chunk? Then I'm fine with what we have.

As far as I can see there is no atomic chunk support.

..

I attached the changes as a patch.
I'll have a closer look at documentation and api next week.

Dirk



[-- Attachment #2: rti2c.patch --]
[-- Type: application/octet-stream, Size: 3663 bytes --]

Index: busses/rti2c-ppc4xx.c
===================================================================
--- busses/rti2c-ppc4xx.c	(Revision 5)
+++ busses/rti2c-ppc4xx.c	(Arbeitskopie)
@@ -62,7 +62,7 @@
 module_param(rti2c_ppc4xx_force_fast, bool, 0);
 MODULE_PARM_DESC(rti2c_ppc4xx_fast_poll, "Force fast mode (400 kHz)");
 
-#define DBG_LEVEL 3
+#define DBG_LEVEL 0
 
 #ifdef DBG
 #undef DBG
@@ -405,7 +405,7 @@
 			rti2c_ppc4xx_dev_reset(dev);
 			return;
 		}
-		schedule();
+		rtdm_task_sleep(1);
 	}
 
 	/* Just to clear errors */
@@ -764,7 +764,7 @@
 	adap->timeout = 1;
 	adap->retries = 1;
 	
-	ret = rti2c_adapter_register(adap, "rti2cppc4xx");
+	ret = rti2c_adapter_register(adap);
 	rtdm_printk("ibm-iic%d: using %s mode\n", dev->idx,
 		dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
 
Index: rti2c.h
===================================================================
--- rti2c.h	(Revision 5)
+++ rti2c.h	(Arbeitskopie)
@@ -100,8 +100,7 @@
 /* Adapter registration */
 struct rti2c_adapter *rti2c_adapter_alloc(void);
 void rti2c_adapter_free(struct rti2c_adapter *);
-int rti2c_adapter_register(struct rti2c_adapter *adapter, 
-	const char* rtdm_device_name);
+int rti2c_adapter_register(struct rti2c_adapter *adapter);
 int rti2c_adapter_unregister(struct rti2c_adapter *adapter);
 
 /* Return the functionality mask */
Index: rti2c-core.c
===================================================================
--- rti2c-core.c	(Revision 5)
+++ rti2c-core.c	(Arbeitskopie)
@@ -26,6 +26,8 @@
  */
 
 #include "rti2c.h"
+#include <asm/io.h>
+#include <linux/slab.h>
 
 #define RTI2C_MAX_ADAPTERS 5
 
@@ -43,16 +45,12 @@
 {
 	struct rti2c_adapter *adapter;
 	
-	adapter = (struct rti2c_adapter *)rtdm_malloc(sizeof(*adapter));
+	adapter = kzalloc(sizeof(*adapter), GFP_KERNEL);
 	if (!adapter) {
 		rtdm_printk("rti2c-core: cannot allocate rti2c_adapter\n");
 		return NULL;
 	}
-	memset(adapter, 0, sizeof(*adapter));
 	memcpy(&adapter->dev, &rti2c_device, sizeof(adapter->dev));
-	rtdm_printk("adapter->dev.proc_name = %s,"
-		" rti2c_device.proc_name = %s\n", 
-		adapter->dev.proc_name, rti2c_device.proc_name);
 	rtdm_mutex_init(&adapter->bus_lock);
 	
 	return adapter;
@@ -61,11 +59,10 @@
 void rti2c_adapter_free(struct rti2c_adapter *adapter)
 {
 	rtdm_mutex_destroy(&adapter->bus_lock);
-	rtdm_free(adapter);
+	kfree(adapter);
 }
 
-int rti2c_adapter_register(struct rti2c_adapter *adapter, 
-	const char* rtdm_device_name)
+int rti2c_adapter_register(struct rti2c_adapter *adapter)
 {
 	unsigned int i;
 	int n = -1;
@@ -76,6 +73,7 @@
 	for (i = 0; i < RTI2C_MAX_ADAPTERS; i++) {
 		if (rti2c_adapters[i] == NULL) {
 			n = i;
+			break;
 		}
 	}
 	if (n < 0) {
@@ -85,8 +83,8 @@
 
 	rtdm_mutex_unlock(&rti2c_mut_adapters);
 	
-	memcpy(adapter->dev.device_name, rtdm_device_name, 
-		sizeof(adapter->dev.device_name));
+        snprintf(adapter->dev.device_name, sizeof(adapter->dev.device_name),
+		"rti2c%d", n);
 	adapter->dev.proc_name = adapter->dev.device_name;
 	adapter->dev.device_id = n;
 	
@@ -94,8 +92,10 @@
 	if (ret) {
 		rtdm_printk("rti2c-core: cannot register rtdm-device %s\n",
 			adapter->dev.device_name);
+	} else {
+		rtdm_printk("rti2c-core: adapter %s registered\n",
+			adapter->dev.device_name);
 	}
-	
 	return ret;
 }
 
@@ -632,7 +632,7 @@
 				break;
 			}
 			data_ptrs[i] = (u8 __user *)rdwr_pa[i].buf;
-			rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL);
+			rdwr_pa[i].buf = rtdm_malloc(rdwr_pa[i].len);
 			if(rdwr_pa[i].buf == NULL) {
 				res = -ENOMEM;
 				break;