On Monday, February 23, 2015 11:58 AM, Ian Abbott wrote:
> On 20/02/15 23:05, H Hartley Sweeten wrote:
>> Convert this driver to use the comedi_8254 module to provide the 8254 timer
>> support.
>>
>> Add 'clock_src' and 'gate_src' members to the comedi_8254 data for
>> convienence.
>>
>> Signed-off-by: H Hartley Sweeten <[email protected]>
>> Cc: Ian Abbott <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> ---
>> drivers/staging/comedi/Kconfig | 1 +
>> .../staging/comedi/drivers/amplc_dio200_common.c | 263
>> ++++++---------------
>> drivers/staging/comedi/drivers/comedi_8254.h | 4 +
>> 3 files changed, 72 insertions(+), 196 deletions(-)
> [snip]
>> +static int dio200_subdev_8254_offset(struct comedi_device *dev,
>> + struct comedi_subdevice *s)
>> {
>> - const struct dio200_board *board = dev->board_ptr;
>> - struct dio200_subdev_8254 *subpriv = s->private;
>> + struct comedi_8254 *i8254 = s->private;
>>
>> - if (!board->has_clk_gat_sce)
>> - return -1;
>> + if (dev->mmio)
>> + return i8254->mmio - dev->mmio;
>>
>> - return subpriv->gate_src[counter_number];
>> + return i8254->iobase - dev->iobase;
>> }
>
> This will be wrong for the PCIe boards (board->is_pcie), where the
> registers are on 8-byte boundaries (regshift 3) instead of 1-byte
> boundaries (regshift 0). The result needs to be shifted right by 3 for
>the PCIe boards. The "offset" should be more of a "logical offset" as
> If the registers where on 1-byte boundaries. I.e. it's the "logical
> offset" that determines which clock and gate selection registers to use
> (and the 'which' bit setting within the register values).
This function is actually returning the original 'offset' that was passed
to dio200_subdev_8254_init(). That value was originally stored in the
subdevice private data (subpriv->ofs ). That value should be your
"logical offset".
> [snip]
>> static int dio200_subdev_8254_init(struct comedi_device *dev,
>> @@ -686,28 +551,34 @@ static int dio200_subdev_8254_init(struct
>> comedi_device *dev,
>> unsigned int offset)
>> {
>> const struct dio200_board *board = dev->board_ptr;
>> - struct dio200_subdev_8254 *subpriv;
>> - unsigned int chan;
>> + struct comedi_8254 *i8254;
>> + unsigned int regshift;
>> + int chan;
>>
>> - subpriv = comedi_alloc_spriv(s, sizeof(*subpriv));
>> - if (!subpriv)
>> + regshift = (board->is_pcie) ? 3 : 0;
>> +
>> + if (dev->mmio)
>> + i8254 = comedi_8254_mm_init(dev->mmio + offset,
>> + 0, I8254_IO8, regshift);
>> + else
>> + i8254 = comedi_8254_init(dev->iobase + offset,
>> + 0, I8254_IO8, regshift);
>
> The offsets to the 8254 will need to be shifted left by regshift, since
> the comedi_8254 module is reading and writing the registers itself
> rather than going through dio200_read8() and dio200_write8() which was
> previously responsible for the left shift.
I think this one is actually correct based on your review of patch 1/36.
The others you pointed out were incorrect and I have fixed them.
Regards,
Hartley
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
