[RFC PATCH 01/36] staging: comedi: comedi_8254: introduce module for 8254 timer support

[Ian Abbott]

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 at visionengravers.com>
> Cc: Ian Abbott <abbotti at mev.co.uk>
> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> ---
>   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.

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

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

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-