Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support

[Zubair Lutfullah :]

On Wed, Aug 28, 2013 at 03:01:24PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
> 
> I am mostly happy with it. There are just two things I pointed out.
> Besides that, it looks good from my side.
Sign-off?

> >+static irqreturn_t tiadc_irq(int irq, void *private)
> >+{
...
> >+	/* If any IRQ flags left, return none. So TSC can handle its IRQs */
> >+	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> >+	if (status == false)
> >+		return IRQ_HANDLED;
> >+	else
> >+		return IRQ_NONE;
> 
> As I said in 1/2 of this series, you shouldn't do this. Both handlers of a
> shared line are invoked once an interrupt is detected.
> 
> â
I somehow missed sending these patches to Dmitry.. 
I'll do it next series.

He originally suggested this way.
An alternative was to handle irqs in mfd core which would 
seriously complicate code and patches..

Whenever I change the handlers to the way you suggest.
i.e. Handle what each irq handler does and return IRQ_HANDLED
if the irq handler handled nothing then return IRQ_NONE.

The driver hangs when I'm brutal while touching the TSC and 
continuously sampling the ADC. Not always. But happens.

When I check /proc/interrupts the hardware irqs seem to be firing.
But for some reason the whole thing messes up..

> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> >+{
> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+	struct iio_buffer *buffer = indio_dev->buffer;
> >+	unsigned int enb = 0, stepnum;
> >+	u8 bit;
> >+
> >+	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> >+	if (adc_dev->data == NULL)
> >+		return -ENOMEM;
> >+
> >+	tiadc_step_config(indio_dev);
> >+	for_each_set_bit(bit, buffer->scan_mask,
> >+			adc_dev->channels) {
> >+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> >+		/*
> >+		 * There are a total of 16 steps available
> >+		 * that are shared between ADC and touchscreen.
> >+		 * We start configuring from step 16 to 0 incase of
> >+		 * ADC. Hence the relation between input channel
> >+		 * and step for ADC would be as below.
> >+		 */
> >+		stepnum = chan->channel + 9;
> >+		enb |= (1 << stepnum);
> 
> Hmm. This looks odd.
>  
> In tiadc_step_config() we do:
> | steps = TOTAL_STEPS - adc_dev->channels;
> |â
> | for (i = 0; i < adc_dev->channels; i++) {
> |         chan = adc_dev->channel_line[i];
> |         tiadc_writel(adc_dev,
> |                         REG_STEPCONFIG(steps),
> |                         stepconfig
> |                         |
> |                         STEPCONFIG_INP(chan));
> |         tiadc_writel(adc_dev,
> |                         REG_STEPDELAY(steps),
> |                         STEPCONFIG_OPENDLY);
> |         adc_dev->channel_step[i]
> |                 =
> |                 steps;
> |         steps++;
> | }
> 
> That means if we have only one channel we enable the last step bit /
> topmost that is bit 16. For the "default" four channels we get 0x1e000
> as mask which means bit 13 to 16. 
> If you do have only one channel with the number 4 you then
> get_adc_step_mask() will compute the correct step mask but here enable
> step 14 instead of step 16.
> 
> What about the following as replacement?
I sense a mismatch. Probably because of different styles of coding
these bit handling :).

I'll look into it and the alternative way you suggested as well.

Thanks
ZubairLK

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