linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH 02/18] lirc serial port receiver/transmitter device driver
@ 2008-09-11 19:49 Stefan Bauer
  2008-09-12 16:24 ` Jarod Wilson
  0 siblings, 1 reply; 22+ messages in thread
From: Stefan Bauer @ 2008-09-11 19:49 UTC (permalink / raw)
  To: Jarod Wilson
  Cc: linux-kernel, Jarod Wilson, Janne Grunau, Christoph Bartelmus

Jarod Wilson wrote:

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

I just want to thank you very much for your work and give you my Tested-By. 
Todays git (b2e9c18a32423310a309f94ea5a659c4bb264378) works well here with 
lirc-0.8.3 userspace on a Pentium 3/i815-system.

But I've had a section mismatch in the lirc code, don't know if this is 
serious.

WARNING: drivers/input/lirc/lirc_serial.o(.init.text+0x11e): Section mismatch 
in reference from the function init_module() to the 
function .exit.text:lirc_serial_exit()
The function __init init_module() references
a function __exit lirc_serial_exit().
This is often seen when error handling in the init function
uses functionality in the exit path.
The fix is often to remove the __exit annotation of
lirc_serial_exit() so it may be used outside an exit section.


Regards,
Stefan

^ permalink raw reply	[flat|nested] 22+ messages in thread
* [PATCH 0/18] linux infrared remote control drivers
@ 2008-09-09  4:05 Jarod Wilson
  2008-09-09  4:05 ` [PATCH 01/18] lirc core device driver infrastructure Jarod Wilson
  0 siblings, 1 reply; 22+ messages in thread
From: Jarod Wilson @ 2008-09-09  4:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jarod Wilson, Janne Grunau, Christoph Bartelmus, Eric Sandeen,
	Mario Limonciello

The following patch series adds 17 new drivers for assorted infrared and/or RF
remote control receivers and/or transmitters. These drivers have long lived
out-of-tree at http://www.lirc.org/, packaged as 3rd-party modules by many
distributions, and more recently, patched into the kernels of at least Fedora
and Ubuntu. The primary maintainer of lirc, Christoph Bartelmus simply hasn't
had the time to send these bits upstream, and a few months back, gave me the
go-ahead to take on the task.

Most drivers are fairly widely tested, certainly within the MythTV community,
which relies upon this code quite a bit. However, note that in preparation
for this submission, a fair number of code changes have been made only
recently that may or may not have yet made it back into lirc cvs, and thus
might not be as widely tested. Any bugs found were probably introduced by
either myself or Janne Grunau, the primary folks working on the latest round
of whacking checkpatch.pl complaints to prepare for this submission.

Speaking of checkpatch... When I first added the lirc driver patch to the
Fedora kernels (oy, way back in August of 2007), there were tens of thousands
of lines of checkpatch warnings and errors. Through the efforts of myself,
Janne, Eric Sandeen, and misc contributions from others, I'm now quite
tickled to see the following:

--
$ scripts/checkpatch.pl /data/patches/lirc-for-upstream-combined.patch
total: 0 errors, 0 warnings, 17877 lines checked

/data/patches/lirc-for-upstream-combined.patch has no obvious style problems
and is ready for submission.
--

Earlier rounds of checkpatch cleanups have already been fed back into lirc
cvs, and Christoph has given me commit access there, so as to filter the
latest changes back in as well. For the time being, lirc cvs will
continue to be the canonical upstream for the lirc drivers, and all the
earlier kernel compat bits get stripped out via an export script[1],
though the script is still under a bit of development... I'll be taking
point on maintaining the git tree[2] and synchronizing changes back and
forth with cvs, at least until such time as everything is in the kernel,
and nobody has need for out-of-tree drivers anymore, at which time the
drivers will be dropped from the main lirc tarball.

Not all drivers have been tested with this codebase and certainly not all
devices they support, but its a solid place to start with these in-kernel.
Patches are against 2.6.27-rc5-git9 or so, and have been tested running
the same, at least in the cases where hardware was available. These patches
are also in the latest nightly Fedora rawhide kernel, for those who want
instant gratification[3]. Note that you also need the lirc userspace to
really do any meaningful testing...

Signed-off-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Janne Grunau <j@jannau.net>
CC: Christoph Bartelmus <lirc@bartelmus.de>
CC: Eric Sandeen <esandeen@redhat.com>
CC: Mario Limonciello <superm1@ubuntu.com>

[1] http://people.redhat.com/jwilson/lirc/, in a tarball atm
[2] http://git.wilsonet.com/linux-2.6-lirc.git/ (s/http/git/ to clone)
[3] http://kojipkgs.fedoraproject.org/packages/kernel/2.6.27/0.314.rc5.git9.fc10/
    is the latest build, at least as of this submission

Combined diffstat:

 MAINTAINERS                           |    9 +
 drivers/input/Kconfig                 |    2 +
 drivers/input/Makefile                |    2 +
 drivers/input/lirc/Kconfig            |  142 ++++
 drivers/input/lirc/Makefile           |   25 +
 drivers/input/lirc/commandir.c        |  982 +++++++++++++++++++++++
 drivers/input/lirc/commandir.h        |   68 ++
 drivers/input/lirc/lirc.h             |  103 +++
 drivers/input/lirc/lirc_atiusb.c      | 1321 +++++++++++++++++++++++++++++++
 drivers/input/lirc/lirc_bt829.c       |  388 +++++++++
 drivers/input/lirc/lirc_cmdir.c       |  596 ++++++++++++++
 drivers/input/lirc/lirc_cmdir.h       |   25 +
 drivers/input/lirc/lirc_dev.c         |  809 +++++++++++++++++++
 drivers/input/lirc/lirc_dev.h         |  262 ++++++
 drivers/input/lirc/lirc_i2c.c         |  639 +++++++++++++++
 drivers/input/lirc/lirc_igorplugusb.c |  619 +++++++++++++++
 drivers/input/lirc/lirc_imon.c        | 1280 ++++++++++++++++++++++++++++++
 drivers/input/lirc/lirc_it87.c        |  999 +++++++++++++++++++++++
 drivers/input/lirc/lirc_it87.h        |  116 +++
 drivers/input/lirc/lirc_ite8709.c     |  545 +++++++++++++
 drivers/input/lirc/lirc_mceusb.c      |  890 +++++++++++++++++++++
 drivers/input/lirc/lirc_mceusb2.c     | 1119 ++++++++++++++++++++++++++
 drivers/input/lirc/lirc_parallel.c    |  728 +++++++++++++++++
 drivers/input/lirc/lirc_parallel.h    |   26 +
 drivers/input/lirc/lirc_sasem.c       |  969 +++++++++++++++++++++++
 drivers/input/lirc/lirc_serial.c      | 1312 +++++++++++++++++++++++++++++++
 drivers/input/lirc/lirc_sir.c         | 1302 ++++++++++++++++++++++++++++++
 drivers/input/lirc/lirc_streamzap.c   |  795 +++++++++++++++++++
 drivers/input/lirc/lirc_ttusbir.c     |  400 ++++++++++
 drivers/input/lirc/lirc_zilog.c       | 1395 +++++++++++++++++++++++++++++++++
 30 files changed, 17868 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/lirc/Kconfig
 create mode 100644 drivers/input/lirc/Makefile
 create mode 100644 drivers/input/lirc/commandir.c
 create mode 100644 drivers/input/lirc/commandir.h
 create mode 100644 drivers/input/lirc/lirc.h
 create mode 100644 drivers/input/lirc/lirc_atiusb.c
 create mode 100644 drivers/input/lirc/lirc_bt829.c
 create mode 100644 drivers/input/lirc/lirc_cmdir.c
 create mode 100644 drivers/input/lirc/lirc_cmdir.h
 create mode 100644 drivers/input/lirc/lirc_dev.c
 create mode 100644 drivers/input/lirc/lirc_dev.h
 create mode 100644 drivers/input/lirc/lirc_i2c.c
 create mode 100644 drivers/input/lirc/lirc_igorplugusb.c
 create mode 100644 drivers/input/lirc/lirc_imon.c
 create mode 100644 drivers/input/lirc/lirc_it87.c
 create mode 100644 drivers/input/lirc/lirc_it87.h
 create mode 100644 drivers/input/lirc/lirc_ite8709.c
 create mode 100644 drivers/input/lirc/lirc_mceusb.c
 create mode 100644 drivers/input/lirc/lirc_mceusb2.c
 create mode 100644 drivers/input/lirc/lirc_parallel.c
 create mode 100644 drivers/input/lirc/lirc_parallel.h
 create mode 100644 drivers/input/lirc/lirc_sasem.c
 create mode 100644 drivers/input/lirc/lirc_serial.c
 create mode 100644 drivers/input/lirc/lirc_sir.c
 create mode 100644 drivers/input/lirc/lirc_streamzap.c
 create mode 100644 drivers/input/lirc/lirc_ttusbir.c
 create mode 100644 drivers/input/lirc/lirc_zilog.c

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2008-09-26 19:42 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-09-11 19:49 [PATCH 02/18] lirc serial port receiver/transmitter device driver Stefan Bauer
2008-09-12 16:24 ` Jarod Wilson
2008-09-13  0:26   ` Janne Grunau
2008-09-13  8:41     ` Stefan Bauer
2008-09-15  3:55       ` Jarod Wilson
2008-09-15 18:20         ` Jarod Wilson
2008-09-16  4:08           ` Jarod Wilson
2008-09-18 14:00             ` Jarod Wilson
2008-09-19 18:05             ` Stefan Bauer
2008-09-19 18:26               ` Janne Grunau
2008-09-19 18:53                 ` Stefan Bauer
2008-09-19 19:24                   ` Janne Grunau
2008-09-20  0:10                   ` Janne Grunau
2008-09-26 19:42                     ` Stefan Bauer
2008-09-19 18:54                 ` Jarod Wilson
2008-09-19 19:12                   ` Stefan Bauer
2008-09-13  7:09   ` Christoph Bartelmus
  -- strict thread matches above, loose matches on Subject: below --
2008-09-09  4:05 [PATCH 0/18] linux infrared remote control drivers Jarod Wilson
2008-09-09  4:05 ` [PATCH 01/18] lirc core device driver infrastructure Jarod Wilson
2008-09-09  4:05   ` [PATCH 02/18] lirc serial port receiver/transmitter device driver Jarod Wilson
2008-09-09 16:14     ` Jonathan Corbet
2008-09-09 19:51       ` Stefan Lippers-Hollmann
2008-09-09 19:56         ` Jarod Wilson
2008-09-10 17:40       ` Jarod Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).