lkml.org 
[lkml]   [2008]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 02/18] lirc serial port receiver/transmitter device driver
Date
On Tuesday 09 September 2008 12:14:22 Jonathan Corbet wrote:
> > +#ifdef LIRC_SERIAL_IRDEO
> > +static int type = LIRC_IRDEO;
> > +#elif defined(LIRC_SERIAL_IRDEO_REMOTE)
> > +static int type = LIRC_IRDEO_REMOTE;
> > +#elif defined(LIRC_SERIAL_ANIMAX)
> > +static int type = LIRC_ANIMAX;
> > +#elif defined(LIRC_SERIAL_IGOR)
> > +static int type = LIRC_IGOR;
> > +#elif defined(LIRC_SERIAL_NSLU2)
> > +static int type = LIRC_NSLU2;
> > +#else
> > +static int type = LIRC_HOMEBREW;
> > +#endif
>
> So where do all these LIRC_SERIAL_* macros come from? I can't really tell
> what this bunch of ifdeffery is doing or how one might influence it.

Bleah. I believe these get passed in when building lirc userspace and drivers
together, when manually selected the specific type of serial receiver you have
in lirc's menu-driven configuration tool thingy...

In other words, they do us absolutely no good here. We just build for the
default type (LIRC_HOMEBREW), and users can override the type as necessary via
the 'type' module parameter. I'll nuke that chunk.

> > +
> > +static struct lirc_serial hardware[] = {
> > + /* home-brew receiver/transmitter */
> > + {
> > + UART_MSR_DCD,
> > + UART_MSR_DDCD,
> > + UART_MCR_RTS|UART_MCR_OUT2|UART_MCR_DTR,
> > + UART_MCR_RTS|UART_MCR_OUT2,
> > + send_pulse_homebrew,
> > + send_space_homebrew,
> > + (
> > +#ifdef LIRC_SERIAL_TRANSMITTER
> > + LIRC_CAN_SET_SEND_DUTY_CYCLE|
> > + LIRC_CAN_SET_SEND_CARRIER|
> > + LIRC_CAN_SEND_PULSE|
> > +#endif
> > + LIRC_CAN_REC_MODE2)
> > + },
>
> It would be really nice to use the .field=value structure initialization
> conventions here.

Indeed. Done.

> > +#if defined(__i386__)
> > +/*
> > + From:
> > + Linux I/O port programming mini-HOWTO
> > + Author: Riku Saikkonen <Riku.Saikkonen@hut.fi>
> > + v, 28 December 1997
> > +
> > + [...]
> > + Actually, a port I/O instruction on most ports in the 0-0x3ff range
> > + takes almost exactly 1 microsecond, so if you're, for example,using
> > + the parallel port directly, just do additional inb()s from that port
> > + to delay.
> > + [...]
> > +*/
> > +/* transmitter latency 1.5625us 0x1.90 - this figure arrived at from
> > + * comment above plus trimming to match actual measured frequency.
> > + * This will be sensitive to cpu speed, though hopefully most of the
> > 1.5us + * is spent in the uart access. Still - for reference test
> > machine was a + * 1.13GHz Athlon system - Steve
> > + */
> > +
> > +/* changed from 400 to 450 as this works better on slower machines;
> > + faster machines will use the rdtsc code anyway */
> > +
> > +#define LIRC_SERIAL_TRANSMITTER_LATENCY 450
> > +
> > +#else
> > +
> > +/* does anybody have information on other platforms ? */
> > +/* 256 = 1<<8 */
> > +#define LIRC_SERIAL_TRANSMITTER_LATENCY 256
> > +
> > +#endif /* __i386__ */
>
> This is a little scary. Maybe hrtimers would be a better way of handling
> your timing issues?

Sounds plausible, will look into it. Of course, this partially hinges on the
USE_RDTSC bits, more comments just a little ways down...

> > +static inline unsigned int sinp(int offset)
> > +{
> > +#if defined(LIRC_ALLOW_MMAPPED_IO)
> > + if (iommap != 0) {
> > + /* the register is memory-mapped */
> > + offset <<= ioshift;
> > + return readb(io + offset);
> > + }
> > +#endif
> > + return inb(io + offset);
> > +}
>
> This all looks like a reimplementation of ioport_map() and the associated
> ioread*() and iowrite*() functions...?

Probably. Will see about using those instead.

> > +#ifdef USE_RDTSC
> > +/* Version that uses Pentium rdtsc instruction to measure clocks */
> > +
> > +/* This version does sub-microsecond timing using rdtsc instruction,
> > + * and does away with the fudged LIRC_SERIAL_TRANSMITTER_LATENCY
> > + * Implicitly i586 architecture... - Steve
> > + */
> > +
> > +static inline long send_pulse_homebrew_softcarrier(unsigned long length)
> > +{
> > + int flag;
> > + unsigned long target, start, now;
> > +
> > + /* Get going quick as we can */
> > + rdtscl(start); on();
> > + /* Convert length from microseconds to clocks */
> > + length *= conv_us_to_clocks;
> > + /* And loop till time is up - flipping at right intervals */
> > + now = start;
> > + target = pulse_width;
> > + flag = 1;
> > + while ((now-start) < length) {
> > + /* Delay till flip time */
> > + do {
> > + rdtscl(now);
> > + } while ((now-start) < target);
>
> This looks like a hard busy wait, without even an occasional, polite,
> cpu_relax() call. There's got to be a better way?
>
> The i2c code has the result of a lot of bit-banging work, I wonder if
> there's something there which could be helpful here.

Hrm... So at some point in the past, there was an "#if defined(rdtscl)" in the
lirc_serial.c code that triggered USE_RDTSC being defined. At the moment,
there's nothing defining it (I probably overzealously nuked it during clean-
up), so we're not even touching the above code. However, this is supposed to
be the "better" code path wrt producing a reliable waveform, at least on
platforms with rdtscl... Will have to do some investigating... This actually
affects whether or not we bother with hrtimers as suggested above too, as
LIRC_SERIAL_TRANSMITTER_LATENCY is not used at all in the USE_RDTSC case.

> > +static irqreturn_t irq_handler(int i, void *blah)
> > +{
> > + struct timeval tv;
> > + int status, counter, dcd;
> > + long deltv;
> > + int data;
> > + static int last_dcd = -1;
> > +
> > + if ((sinp(UART_IIR) & UART_IIR_NO_INT)) {
> > + /* not our interrupt */
> > + return IRQ_RETVAL(IRQ_NONE);
> > + }
>
> That should just be IRQ_NONE, no?

Yeah, looks like it. Done.

> > +static void hardware_init_port(void)
> > +{
> > + unsigned long flags;
> > + local_irq_save(flags);
>
> That won't help you if an interrupt is handled by another processor. This
> needs proper locking, methinks.

Yeah, working on implementing locking right now.

> Nothing in this function does anything to assure itself that the port
> actually exists and is the right kind of hardware. Maybe that can't really
> be done with this kind of device?

We should probably try to make sure the port actually exists, but I don't
think there's a whole lot (if anything) we can do as far as verifying the
device itself.

> > +static int init_port(void)
> > +{
>
> ...
>
> > + if (sense == -1) {
> > + /* wait 1/2 sec for the power supply */
> > +
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + schedule_timeout(HZ/2);
>
> msleep(), maybe?

Yeah, looks like it.

> > +static int set_use_inc(void *data)
> > +{
> > + int result;
> > + unsigned long flags;
> > +
> > + /* Init read buffer. */
> > + if (lirc_buffer_init(&rbuf, sizeof(int), RBUF_LEN) < 0)
> > + return -ENOMEM;
> > +
> > + /* initialize timestamp */
> > + do_gettimeofday(&lasttv);
> > +
> > + result = request_irq(irq, irq_handler,
> > + IRQF_DISABLED | (share_irq ? IRQF_SHARED : 0),
> > + LIRC_DRIVER_NAME, (void *)&hardware);
> > +
> > + switch (result) {
> > + case -EBUSY:
> > + printk(KERN_ERR LIRC_DRIVER_NAME ": IRQ %d busy\n", irq);
> > + lirc_buffer_free(&rbuf);
> > + return -EBUSY;
> > + case -EINVAL:
> > + printk(KERN_ERR LIRC_DRIVER_NAME
> > + ": Bad irq number or handler\n");
> > + lirc_buffer_free(&rbuf);
> > + return -EINVAL;
> > + default:
> > + dprintk("Interrupt %d, port %04x obtained\n", irq,
> > io);
> > + break;
> > + };
> > +
> > + local_irq_save(flags);
> > +
> > + /* Set DLAB 0. */
> > + soutp(UART_LCR, sinp(UART_LCR) & (~UART_LCR_DLAB));
> > +
> > + soutp(UART_IER, sinp(UART_IER)|UART_IER_MSI);
> > +
> > + local_irq_restore(flags);
> > +
> > + return 0;
> > +}
>
> OK, so set_use_inc() really is just an open() function. It still seems
> like a strange duplication.

Going to let the duplication be for the moment, since I don't know the history
behind why there's duplication, and there are enough other mountains to climb
first... :)

> Again, local_irq_save() seems insufficient here. You need a lock to
> serialize access to the hardware.

Will do.

--
Jarod Wilson
jarod@redhat.com



\
 
 \ /
  Last update: 2008-09-10 19:45    [W:0.376 / U:0.784 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site