[PATCH] kill mutex in lis

Sean McNeil sean at mcneil.com
Sun Oct 12 17:50:00 CEST 2008


A couple of comments:

1) Your disable/enable is commented out. If you use this route, then you
better use level triggered interrupts.
2) If using level triggering, then you don't need to kick off the reading.
3) Please test the driver under heavy load. When using edge triggered
interrupts, we have seen cases where more data was available than was
read. This would prevent the interrupt source from resetting and we'd
never get another edge trigger. That in turn would cause the driver to
stop functioning.

Simon Kagstrom wrote:
> Thanks for the discussion, everyone!
>
> On Sat, 11 Oct 2008 10:03:41 -0700
> Sean McNeil <sean at mcneil.com> wrote:
>
>   
>> For a SPI implementation, I'm pretty sure you will want to go back to
>> an edge triggered interrupt as you will not be clearing out the cause
>> during the ISR. I don't know if you started with the previous code or
>> the more recent code that changes it to level trigger. You will also
>> want to make sure your work function loops reading all the data until
>> the status says no more data or you will end up with no further
>> interrupts being generated when the system is too busy to keep up.
>>     
>
> I started with an earlier version. However, I've used the "old method"
> of disabling the interrupt until the completion handle has been run. If
> a level-triggered interrupt should be used that's a must, I believe.
>
> If anyone is interested, the current (non-working) patch is below.
> Lot's of hacks there and since I did a mistake, also mixed with the
> debug patch I sent earlier :-)
>
> On Sat, 11 Oct 2008 14:04:30 -0200
> Werner Almesberger <werner at openmoko.org> wrote:
>
>   
>> One technique MMC-SPI uses may be useful here: instead of doing N
>> individual operations, string them together in a single transaction
>> and pick the results from the byte stream returned.
>>
>> E.g., if you have an interface where you send an address byte and
>> then the content of that address is returned in the next transfer,
>> you'd send one buffer with
>>
>> <addr1> 0xff <addr2> 0xff ...
>>
>> and receive a buffer with
>>
>> ??? <data1> ??? <data2>
>>     
>
> Thanks for the hints. I thought this would be more or less what the
> spi_async() interface does, but I'll look more into it.
>
>
> Anyway, I should say that if someone else wants to take this up, feel
> free to do so. I unfortunately have other work to do, so I'll only do
> some short hacks on this every once in a while. That said, I have a
> train trip tonight when I'll take a look at this again.
>
> // Simon
> ---
>
>  drivers/input/misc/Kconfig    |    7 ++
>  drivers/input/misc/lis302dl.c |  162 ++++++++++++++++++++++++++++++++++++++++-
>  include/linux/lis302dl.h      |    5 +
>  3 files changed, 172 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 5a15edd..f4e4c5b 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -192,4 +192,11 @@ config INPUT_LIS302DL
>  	  The userspece interface is a 3-axis (X/Y/Z) relative movement
>  	  Linux input device, reporting REL_[XYZ] events.
>  
> +config INPUT_LIS302DL_DEBUG
> +	boolean "Debug files in /sys for LIS302DL"
> +	depends on INPUT_LIS302DL
> +	help
> +	  Select this if you want to be able to read and set register
> +	  values of the LIS302DL in the /sys filesystem
> +
>  endif
> diff --git a/drivers/input/misc/lis302dl.c b/drivers/input/misc/lis302dl.c
> index b01ca04..3afb22c 100644
> --- a/drivers/input/misc/lis302dl.c
> +++ b/drivers/input/misc/lis302dl.c
> @@ -160,12 +160,69 @@ static void _report_btn_double(struct input_dev *inp, int btn)
>  }
>  #endif
>  
> +#define MG_PER_SAMPLE 18
> +
> +static void lis302dl_complete(void *_lis)
> +{
> +	struct lis302dl_info *lis = _lis;
> +	int x,y,z;
> +
> +	x = (s8)lis->rx_buf[0];
> +	y = (s8)lis->rx_buf[1];
> +	z = (s8)lis->rx_buf[2];
> +
> +	printk(KERN_ERR "rxbuf/txbuf: %02x:%02x:%02x  %02x:%02x:%02x\n",
> +			lis->rx_buf[0], lis->rx_buf[1], lis->rx_buf[2],
> +			lis->tx_buf[0], lis->tx_buf[1], lis->tx_buf[2]);
> +
> +	input_report_rel(lis->input_dev, REL_X, MG_PER_SAMPLE * x);
> +	input_report_rel(lis->input_dev, REL_Y, MG_PER_SAMPLE * y);
> +	input_report_rel(lis->input_dev, REL_Z, MG_PER_SAMPLE * z);
> +
> +	input_sync(lis->input_dev);
> +
> +	/* Reenable the IRQ again (another transfer might start here) */
> +//	enable_irq(lis->spi_dev->irq);
> +}
>  
>  static irqreturn_t lis302dl_interrupt(int irq, void *_lis)
>  {
>  	struct lis302dl_info *lis = _lis;
> +	struct spi_message *m = &lis->msg;
> +	struct spi_transfer *x_out = &lis->xfer[0];
> +	struct spi_transfer *x_in = &lis->xfer[1];
> +	static u8 buf[8];
> +
> +	/* Disable the IRQ until this message has been handled */
> +	printk(KERN_ERR "Intorrupt\n");
> +//	disable_irq(lis->spi_dev->irq);
> +
> +	spi_message_init(m);
> +	memset(lis->xfer, 0, sizeof(lis->xfer));
> +	memset(lis->rx_buf, 0, sizeof(lis->rx_buf));
> +	memset(lis->tx_buf, 0, sizeof(lis->tx_buf));
> +
> +	m->complete = lis302dl_complete;
> +	m->context = lis;
> +
> +	/* Create the send and receive messages */
> +	x_out->tx_buf = buf;
> +	x_out->len = 4;
> +	x_in->rx_buf = lis->rx_buf;
> +	x_in->len = sizeof(lis->rx_buf);
> +
> +	buf[0] = LIS302DL_REG_OUT_X | READ_BIT;
> +	buf[1] = LIS302DL_REG_OUT_Y | READ_BIT;
> +	buf[2] = LIS302DL_REG_OUT_Z | READ_BIT;
> +	buf[2] = LIS302DL_REG_STATUS | READ_BIT;
> +
> +	spi_message_add_tail(x_out, m);
> +	spi_message_add_tail(x_in, m);
> +
> +	/* Send the message and wait for the best */
> +	if (spi_async(lis->spi_dev, m) < 0)
> +		printk(KERN_ERR "lis302dl: Sending spi message failed\n");
>  
> -	(lis->pdata->lis302dl_bitbang_read)(lis);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -455,12 +512,109 @@ static DEVICE_ATTR(freefall_wakeup_1, S_IRUGO | S_IWUSR, show_freefall_1,
>  static DEVICE_ATTR(freefall_wakeup_2, S_IRUGO | S_IWUSR, show_freefall_2,
>  								set_freefall_2);
>  
> +#ifdef CONFIG_INPUT_LIS302DL_DEBUG
> +static ssize_t show_register(struct lis302dl_info *lis, u8 reg,
> +		char *buf)
> +{
> +	return sprintf(buf, "0x%x\n", reg_read(lis, reg));
> +}
> +
> +static ssize_t set_register(struct lis302dl_info *lis, u8 reg,
> +		const char *buf, size_t count)
> +{
> +	u32 val;
> +	int r;
> +
> +	if (sscanf(buf, "0x%x\n", &val) != 1)
> +		return -EINVAL;
> +	r = reg_write(lis, reg, val);
> +
> +	return count;
> +}
> +
> +#define DEBUG_REG_WHICH(name, which_reg)                  \
> +static ssize_t show_register_##name(struct device *dev,   \
> +		struct device_attribute *attr, char *buf) \
> +{                                                         \
> +	return show_register(dev_get_drvdata(dev),        \
> +			which_reg, buf);                  \
> +}                                                         \
> +							  \
> +static ssize_t set_register_##name(struct device *dev,    \
> +		struct device_attribute *attr, const char *buf, \
> +		size_t count)                             \
> +{                                                         \
> +	return set_register(dev_get_drvdata(dev),         \
> +			which_reg, buf, count);           \
> +}
> +
> +DEBUG_REG_WHICH(ctrl1, LIS302DL_REG_CTRL1)
> +DEBUG_REG_WHICH(ctrl2, LIS302DL_REG_CTRL2)
> +DEBUG_REG_WHICH(ctrl3, LIS302DL_REG_CTRL3)
> +DEBUG_REG_WHICH(hp_filter_reset, LIS302DL_REG_HP_FILTER_RESET)
> +DEBUG_REG_WHICH(who_am_i, LIS302DL_REG_WHO_AM_I)
> +DEBUG_REG_WHICH(status, LIS302DL_REG_STATUS)
> +DEBUG_REG_WHICH(ff_wu_cfg1, LIS302DL_REG_FF_WU_CFG_1)
> +DEBUG_REG_WHICH(ff_wu_cfg2, LIS302DL_REG_FF_WU_CFG_2)
> +DEBUG_REG_WHICH(ff_wu_src1, LIS302DL_REG_FF_WU_SRC_1)
> +DEBUG_REG_WHICH(ff_wu_src2, LIS302DL_REG_FF_WU_SRC_2)
> +DEBUG_REG_WHICH(ff_wu_ths1, LIS302DL_REG_FF_WU_THS_1)
> +DEBUG_REG_WHICH(ff_wu_ths2, LIS302DL_REG_FF_WU_THS_2)
> +DEBUG_REG_WHICH(ff_wu_duration1, LIS302DL_REG_FF_WU_DURATION_1)
> +DEBUG_REG_WHICH(ff_wu_duration2, LIS302DL_REG_FF_WU_DURATION_2)
> +
> +static DEVICE_ATTR(debug_reg_ctrl1, S_IRUGO | S_IWUSR,
> +		show_register_ctrl1, set_register_ctrl1);
> +static DEVICE_ATTR(debug_reg_ctrl2, S_IRUGO | S_IWUSR,
> +		show_register_ctrl2, set_register_ctrl2);
> +static DEVICE_ATTR(debug_reg_ctrl3, S_IRUGO | S_IWUSR,
> +		show_register_ctrl3, set_register_ctrl3);
> +static DEVICE_ATTR(debug_reg_hp_filter_reset, S_IRUGO,
> +		show_register_hp_filter_reset, set_register_hp_filter_reset);
> +static DEVICE_ATTR(debug_reg_who_am_i, S_IRUGO,
> +		show_register_who_am_i, set_register_who_am_i);
> +static DEVICE_ATTR(debug_reg_status, S_IRUGO,
> +		show_register_status, set_register_status);
> +static DEVICE_ATTR(debug_reg_ff_wu_cfg1, S_IRUGO | S_IWUSR,
> +		show_register_ff_wu_cfg1, set_register_ff_wu_cfg1);
> +static DEVICE_ATTR(debug_reg_ff_wu_cfg2, S_IRUGO | S_IWUSR,
> +		show_register_ff_wu_cfg2, set_register_ff_wu_cfg2);
> +static DEVICE_ATTR(debug_reg_ff_wu_src1, S_IRUGO,
> +		show_register_ff_wu_src1, set_register_ff_wu_src1);
> +static DEVICE_ATTR(debug_reg_ff_wu_src2, S_IRUGO,
> +		show_register_ff_wu_src2, set_register_ff_wu_src2);
> +static DEVICE_ATTR(debug_reg_ff_wu_ths1, S_IRUGO | S_IWUSR,
> +		show_register_ff_wu_ths1, set_register_ff_wu_ths1);
> +static DEVICE_ATTR(debug_reg_ff_wu_ths2, S_IRUGO | S_IWUSR,
> +		show_register_ff_wu_ths2, set_register_ff_wu_ths2);
> +static DEVICE_ATTR(debug_reg_ff_wu_duration1, S_IRUGO | S_IWUSR,
> +		show_register_ff_wu_duration1, set_register_ff_wu_duration1);
> +static DEVICE_ATTR(debug_reg_ff_wu_duration2, S_IRUGO | S_IWUSR,
> +		show_register_ff_wu_duration2, set_register_ff_wu_duration2);
> +#endif
> +
>  static struct attribute *lis302dl_sysfs_entries[] = {
>  	&dev_attr_sample_rate.attr,
>  	&dev_attr_full_scale.attr,
>  	&dev_attr_dump.attr,
>  	&dev_attr_freefall_wakeup_1.attr,
>  	&dev_attr_freefall_wakeup_2.attr,
> +#ifdef CONFIG_INPUT_LIS302DL_DEBUG
> +    &dev_attr_debug_reg_ctrl1.attr,
> +    &dev_attr_debug_reg_ctrl2.attr,
> +    &dev_attr_debug_reg_ctrl3.attr,
> +    &dev_attr_debug_reg_hp_filter_reset.attr,
> +    &dev_attr_debug_reg_who_am_i.attr,
> +    &dev_attr_debug_reg_status.attr,
> +    &dev_attr_debug_reg_ff_wu_cfg1.attr,
> +    &dev_attr_debug_reg_ff_wu_cfg2.attr,
> +    &dev_attr_debug_reg_ff_wu_src1.attr,
> +    &dev_attr_debug_reg_ff_wu_src2.attr,
> +    &dev_attr_debug_reg_ff_wu_ths1.attr,
> +    &dev_attr_debug_reg_ff_wu_ths2.attr,
> +    &dev_attr_debug_reg_ff_wu_duration1.attr,
> +    &dev_attr_debug_reg_ff_wu_duration2.attr,
> +#endif
>  	NULL
>  };
>  
> @@ -487,7 +641,11 @@ static int lis302dl_input_open(struct input_dev *inp)
>  
>  	/* kick it off -- since we are edge triggered, if we missed the edge
>  	 * permanent low interrupt is death for us */
> -	(lis->pdata->lis302dl_bitbang_read)(lis);
> +	//(lis->pdata->lis302dl_bitbang_read)(lis);
> +	reg_read(lis, LIS302DL_REG_OUT_X);
> +	reg_read(lis, LIS302DL_REG_OUT_Y);
> +	reg_read(lis, LIS302DL_REG_OUT_Z);
> +	reg_read(lis, LIS302DL_REG_STATUS);
>  
>  	return 0;
>  }
> diff --git a/include/linux/lis302dl.h b/include/linux/lis302dl.h
> index 7daa8b3..8776571 100644
> --- a/include/linux/lis302dl.h
> +++ b/include/linux/lis302dl.h
> @@ -23,6 +23,11 @@ struct lis302dl_info {
>  	struct lis302dl_platform_data *pdata;
>  	struct spi_device *spi_dev;
>  	struct input_dev *input_dev;
> +	struct spi_transfer xfer[2];
> +	struct spi_message msg;
> +	u8 rx_buf[3];
> +	u8 tx_buf[3];
> +
>  	struct mutex lock;
>  	unsigned int flags;
>  	u_int8_t regs[0x40];
>
>
>
>   



More information about the openmoko-kernel mailing list