[PATCH v2 13/14] staging: comedi: multiq3: add 8254 counter/timer subdevice support

[Hartley Sweeten]

On Tuesday, October 06, 2015 4:06 AM, Ian Abbott wrote:
> On 05/10/15 23:33, H Hartley Sweeten wrote:
>> The board has an 8254 timer/counter. Add support for this subdevice.
>>
>> 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/drivers/multiq3.c | 147 ++++++++++++++++++++++++++++++-
>>   1 file changed, 146 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/multiq3.c b/drivers/staging/comedi/drivers/multiq3.c
>> index fc743df..50efc7c 100644
>> --- a/drivers/staging/comedi/drivers/multiq3.c
>> +++ b/drivers/staging/comedi/drivers/multiq3.c
>> @@ -38,6 +38,8 @@
>>
>>   #include "../comedidev.h"
>>
>> +#include "comedi_8254.h"	/* just for the register map */
>> +
>>   /*
>>    * Register map
>>    */
>> @@ -47,6 +49,8 @@
>>   #define MULTIQ3_AI_REG			0x04
>>   #define MULTIQ3_AI_CONV_REG		0x04
>>   #define MULTIQ3_STATUS_REG		0x06
>> +#define MULTIQ3_STATUS_CT(x)		(((x) == 0) ? BIT(0) :	\
>> +					 ((x) == 1) ? BIT(2) : BIT(1))
>
> Which manual are you looking at, Hartley?  The first copy I found online 
> was this one:
>
> http://engineering.nyu.edu/mechatronics/Control_Lab/mq3_manual.pdf

Same manual.

> I suspect the strange ordering of the CTx bits in the diagram in section 
> 3.2.4.2 is probably a typo.  Underneath the diagram it says, "Bits 0-2 
>  (CT0,CT1,CT2): Counter timeout states for the three timers."  There's a 
> discrepancy between that and the diagram, so I think I'd stick with the 
> one that is less stupid!

I also thought the order was a bit strange.

> These CTx status bits are more than likely connected to the OUT0, OUT1 
> and OUT2 pins of the 82C54 and will behave according to the mode of the 
> respective counter channels.
>
>>   #define MULTIQ3_STATUS_EOC		BIT(3)
>>   #define MULTIQ3_STATUS_EOC_I		BIT(4)
>>   #define MULTIQ3_CTRL_REG		0x06
>> @@ -257,6 +261,126 @@ static int multiq3_encoder_insn_config(struct comedi_device *dev,
>   	return insn->n;
>   }
>
> +static unsigned int multiq3_i8254_read(struct comedi_device *dev,
> +				       unsigned int reg)
> +{
> +	/* select the 8254 register then read the value */
> +	multiq3_set_ctrl(dev, MULTIQ3_CTRL_RC(reg));
> +	return inb(dev->iobase + MULTIQ3_CLK_REG);
> +}
>
> According to the manual I linked to, sections 3.2, 3.2.5.1 and 3.2.5.2 
> indicate that CLK_DATA (your MULTIQ3_CLK_REG) is write-only.  This means 
> the INSN_CONFIG_8254_READ_STATUS config instruction and the insn_read 
> handler won't work, although INSN_CONFIG_8254_READ_STATUS could be 
> partially emulated (see below).

Ugh.. Missed that part. It does appear that the CLK_DATA register is write
only. Kind of a silly design choice...

<snip>

> As mentioned above, the insn_read handler this won't work, unfortunately!

<snip>

>> +static int multiq3_8254_insn_config(struct comedi_device *dev,
>> +				    struct comedi_subdevice *s,
>> +				    struct comedi_insn *insn,
>> +				    unsigned int *data)
>> +{
>> +	unsigned int chan = CR_CHAN(insn->chanspec);
>> +	unsigned int status;
>> +	int ret;
>> +
>> +	switch (data[0]) {
>> +	case INSN_CONFIG_RESET:
>> +		ret = multiq3_8254_set_mode(dev, chan,
>> +					    I8254_MODE0 | I8254_BINARY);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case INSN_CONFIG_SET_COUNTER_MODE:
>> +		ret = multiq3_8254_set_mode(dev, chan, data[1]);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case INSN_CONFIG_GET_COUNTER_STATUS:
>> +		data[1] = 0;
>> +		status = inw(dev->iobase + MULTIQ3_STATUS_REG);
>> +		if (status & MULTIQ3_STATUS_CT(chan))
>> +			data[1] |= COMEDI_COUNTER_TERMINAL_COUNT;
>> +		data[2] = COMEDI_COUNTER_TERMINAL_COUNT;
>> +		break;
>
> I think this would only indicate a terminal count if the counter channel 
> is in mode 0.

Or possibly the state of the 'out' signal for each counter.

>> +	case INSN_CONFIG_8254_READ_STATUS:
>> +		multiq3_i8254_write(dev, I8254_CTRL_READBACK_STATUS |
>> +					 I8254_CTRL_READBACK_SEL_CTR(chan),
>> +				    I8254_CTRL_REG);
>> +
>> +		data[1] = multiq3_i8254_read(dev, chan);
>> +		break;
>
> As mentioned above, this won't work, unfortunately, due to the register 
> being write-only.  Most of the function can be emulated by remembering 
> the mode the channel is set to, and by reading the output status of the 
> channel from MULTIQ3_STATUS_REG.  The format of the 8254 status is:

I think just dropping this patch is probably the right choice.

As you mentioned in the follow up email, the counter/timers don't appear
to be very useful on this board. It doesn't appear that any of the signals (clk,
gate, out) are available to the user and they can only really be used to generate
an interrupt with the output of the timer.

Are you ok with the rest of the series if this one is dropped?

Thanks,
Hartley