On Monday, February 23, 2015 11:06 AM, Ian Abbott wrote:
> On 20/02/15 23:04, H Hartley Sweeten wrote:
>> A 8254 timer/counter is commonly used on data acquisition boards to provide
>> the internal pacer clock used to acquire analog input samples. Some boards
>> also to allow the timers to be used externally.
>>
>> Currently the 8254 timers are supported by comedi using the 8253.h header
>> and a number of inline functions. This works for the internal pacer clock
>> but requires the drivers to implement subdevice code necessary to use the
>> timers externally.
>>
>> Introduce a new module to support both the internal pacer clock and the
>> external counter subdevice. This will allow removing a bunch of duplicated
>> code in the drivers and standardizes the comedi 8254 timer support.
>>
>> This implementation is based on the 8253.h inline functions and the various
>> subdevice functionality in the comedi drivers.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/staging/comedi/Kconfig | 3 +
>> drivers/staging/comedi/drivers/Makefile | 1 +
>> drivers/staging/comedi/drivers/comedi_8254.c | 665 +++++++++++++++++++++++++++
>> drivers/staging/comedi/drivers/comedi_8254.h | 129 ++++++
>> 4 files changed, 798 insertions(+)
>> create mode 100644 drivers/staging/comedi/drivers/comedi_8254.c
>> create mode 100644 drivers/staging/comedi/drivers/comedi_8254.h
>>
> [snip]
>> diff --git a/drivers/staging/comedi/drivers/comedi_8254.c b/drivers/staging/comedi/drivers/comedi_8254.c
>> new file mode 100644
>> index 0000000..51fe043
>> --- /dev/null
>> +++ b/drivers/staging/comedi/drivers/comedi_8254.c
> [snip]
>> +static unsigned int __i8254_read(struct comedi_8254 *i8254, unsigned int reg)
>> +{
>> + unsigned int reg_offset = (reg * i8254->iosize) << i8254->regshift;
>
> How is the regshift meant to be used when iosize is 2 (I8254_IO16) or 4
> (I8254_IO32)? The usage above suggests that regshift should be 0 when
> there are no gaps in the registers, but the sanity check in
> __i8254_init() suggests otherwise.
Grrr...
I added this based on an earlier comment from you and didn't think
the regshift all the way thru.
"reg" will be always 0-3 and "iosize" will always be 1, 2, or 4
The (reg * iosze) will automatically calculate the correct offset based on
no gaps between registers.
"regshift" values > 0 account for gaps between registers.
I'll need to rework all the init calls to have the correct regshift values.
>> + unsigned int val;
>> +
>> + switch (i8254->iosize) {
>> + default:
>> + case I8254_IO8:
>> + if (i8254->mmio)
>> + val = readb(i8254->mmio + reg_offset);
>> + else
>> + val = inb(i8254->iobase + reg_offset);
>> + break;
>> + case I8254_IO16:
>> + if (i8254->mmio)
>> + val = readw(i8254->mmio + reg_offset);
>> + else
>> + val = inw(i8254->iobase + reg_offset);
>> + break;
>> + case I8254_IO32:
>> + if (i8254->mmio)
>> + val = readl(i8254->mmio + reg_offset);
>> + else
>> + val = inl(i8254->iobase + reg_offset);
>> + break;
>> + }
>> + return val;
>
> Only the bottom 8 bits of the returned value are valid, so it ought to
> be ANDed with 0xff.
True. None of the current implementations do the masking but it
wouldn't hurt. I'll add this in the next spin.
> [snip]
>> +static struct comedi_8254 *__i8254_init(unsigned long iobase,
>> + void __iomem *mmio,
>> + unsigned int osc_base,
>> + unsigned int iosize,
>> + unsigned int regshift)
>> +{
>> + struct comedi_8254 *i8254;
>> + int i;
>> +
>> + /* sanity check the iosize and that the regshift is valid */
>> + if (!(iosize == I8254_IO8 ||
>> + (iosize == I8254_IO16 && regshift >= 1) ||
>> + (iosize == I8254_IO32 && regshift >= 2)))
>> + return NULL;
>
> (This snippet referred to above.)
I'll fix this. It just needs to verify that the iosize is valid.
Thanks,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
