Andrew, thank you very much for your comments. On 12.02.2006 11:27, Andrew Morton wrote: > >>This patch adds the procfs interface to the gigaset module. > > sysfs, actually. Yes, sorry, the comment didn't get updated when we did the move from procfs to sysfs. > - The ringbuffer head and tail indexes are atomic_t's, but always seem to > be manipulated inside the lock. Perhaps they can become integers. We use ringbuffers in two places. I suppose you are talking about the one implementing our event queue through the ev_head and ev_tail members of the cardstate structure. You have a point there, although I'm not sure if it wouldn't be better to take advantage of the atomic_t and do a little less locking instead. The other one, within the inbuf_t structure, used for capturing characters received from the device, is accessed without locking from the gigaset_handle_event() tasklet and therefore needs atomic_t indexes, IMHO. > - You did the ringbuffer the wrong way. Don't constrain the head and > tail to be within 0..MAX_EVENTS. Instead, just let them wrap right up to > 0xffffffff. Apply the masking when you actually _use_ them. > > That way, empty is (head == tail) and full is (tail - head == MAX_EVENTS). Interesting idea. I have to admit it's rather new to me. I have always done ringbuffers the way they are done in the Gigaset driver now. Can you point me to some example code done the way you propose, so I can familiarize myself with its advantages? > - Could use kstrdup() in a few places. Thanks for the hint. Will watch out for these. > - A few unneeded casts of void*'s, but everyone does that. :-) > - There are a lot of global symbols in there. Perhaps they don't all > need to be global. Well, I *hope* there aren't any unnecessary ones, but we'll re-check. Regards Tilman -- Tilman Schmidt E-Mail: [email protected] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
Attachment:
signature.asc
Description: OpenPGP digital signature
- Follow-Ups:
- Re: [PATCH 6/9] isdn4linux: Siemens Gigaset drivers - procfs interface
- From: Andrew Morton <[email protected]>
- Re: [PATCH 6/9] isdn4linux: Siemens Gigaset drivers - procfs interface
- References:
- [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX
- From: Hansjoerg Lipp <[email protected]>
- [PATCH 1/9] isdn4linux: Siemens Gigaset drivers - Kconfigs and Makefiles
- From: Hansjoerg Lipp <[email protected]>
- [PATCH 2/9] isdn4linux: Siemens Gigaset drivers - common module
- From: Hansjoerg Lipp <[email protected]>
- [PATCH 3/9] isdn4linux: Siemens Gigaset drivers - event layer
- From: Hansjoerg Lipp <[email protected]>
- [PATCH 4/9] isdn4linux: Siemens Gigaset drivers - isdn4linux interface
- From: Hansjoerg Lipp <[email protected]>
- [PATCH 5/9] isdn4linux: Siemens Gigaset drivers - tty interface
- From: Hansjoerg Lipp <[email protected]>
- [PATCH 6/9] isdn4linux: Siemens Gigaset drivers - procfs interface
- From: Hansjoerg Lipp <[email protected]>
- Re: [PATCH 6/9] isdn4linux: Siemens Gigaset drivers - procfs interface
- From: Andrew Morton <[email protected]>
- [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX
- Prev by Date: Re: Device enumeration (was Re: CD writing in future Linux (stirring up a hornets' nest))
- Next by Date: Re: + isdn4linux-siemens-gigaset-drivers-common-module.patch added to -mm tree
- Previous by thread: Re: [PATCH 6/9] isdn4linux: Siemens Gigaset drivers - procfs interface
- Next by thread: Re: [PATCH 6/9] isdn4linux: Siemens Gigaset drivers - procfs interface
- Index(es):
 |