Re: [PATCH 02/18] lirc serial port receiver/transmitter device driver

[Jarod Wilson]

On Thursday 11 September 2008 15:49:25 Stefan Bauer wrote:
> Jarod Wilson wrote:
[...]
> >> > +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.

>From what I've been able to ascertain, reducing the above to...

static u8 sinp(int offset)
{
        if (iommap != 0)
                /* the register is memory-mapped */
                offset <<= ioshift;

        return inb(io + offset);
}

...should be sane. inb() either calls ioport_map() and ioread8() as needed, or 
defines inb() as readb() (or __raw_readb()) on platforms where its 
appropriate. (and similar for lirc_serial.c::soutp()).

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

I've borrowed the simple existence test from 
drivers/serial/8250.c::autoconfig(), which tries a few reads and writes from 
UART_IER. I think this is probably sufficient for verifying the port is legit.

Christoph B., you're definitely much more familiar with serial IR devices than 
I am... Is there any sort of test you can think of that we could use to try to 
verify the existence of an IR device hooked to the port? Or should we be happy 
we at least know there's a port and just assume the IR device is there?


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

Oh good! I haven't broken anything further w/my changes up to b2e9c18... ;) 
I've got another slew of updates to lirc_serial still pending though -- a few 
that I'm about to push, and at least one more chunk I still need to figure out 
(the hrtimers/rdtscl bits).

Continued testing would be much appreciated, since I still have no serial IR 
hardware myself (couldn't find the thingy that shipped with my Technisat card, 
but I do now have hardware on the way).

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

Ah, I believe you're absolutely correct. Done.

-- 
Jarod Wilson
jarod@xxxxxxxxxx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/