linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 3/5] iio: srf08: add triggered buffer support
@ 2017-08-16 19:34 Andreas Klinger
  2017-08-18  6:56 ` Jonathan Cameron
  0 siblings, 1 reply; 3+ messages in thread
From: Andreas Klinger @ 2017-08-16 19:34 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, linux-iio
  Cc: wsa, robh+dt, mark.rutland, linux-i2c, devicetree, linux-kernel

Add support for triggered buffers.

Data format is quite simple:
  distance     16 Bit
  alignment    48 Bit
  timestamp    64 Bit

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/proximity/srf08.c | 60 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/proximity/srf08.c b/drivers/iio/proximity/srf08.c
index 3f19536f215f..de699d67310a 100644
--- a/drivers/iio/proximity/srf08.c
+++ b/drivers/iio/proximity/srf08.c
@@ -18,6 +18,9 @@
 #include <linux/bitops.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 /* registers of SRF08 device */
 #define SRF08_WRITE_COMMAND	0x00	/* Command Register */
@@ -35,9 +38,22 @@
 
 struct srf08_data {
 	struct i2c_client	*client;
-	int			sensitivity;		/* Gain */
-	int			range_mm;		/* max. Range in mm */
+
+	/*
+	 * Gain in the datasheet is called sensitivity here to distinct it
+	 * from the gain used with amplifiers of adc's
+	 */
+	int			sensitivity;
+
+	/* max. Range in mm */
+	int			range_mm;
 	struct mutex		lock;
+
+	/*
+	 * triggered buffer
+	 * 1x16-bit channel + 3x16 padding + 4x16 timestamp
+	 */
+	s16			buffer[8];
 };
 
 /*
@@ -110,6 +126,29 @@ static int srf08_read_ranging(struct srf08_data *data)
 	return ret;
 }
 
+static irqreturn_t srf08_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct srf08_data *data = iio_priv(indio_dev);
+	s16 sensor_data;
+
+	sensor_data = srf08_read_ranging(data);
+	if (sensor_data < 0)
+		goto err;
+
+	mutex_lock(&data->lock);
+
+	data->buffer[0] = sensor_data;
+	iio_push_to_buffers_with_timestamp(indio_dev,
+						data->buffer, pf->timestamp);
+
+	mutex_unlock(&data->lock);
+err:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 static int srf08_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *channel, int *val,
 			    int *val2, long mask)
@@ -323,7 +362,15 @@ static const struct iio_chan_spec srf08_channels[] = {
 		.info_mask_separate =
 				BIT(IIO_CHAN_INFO_RAW) |
 				BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
 };
 
 static const struct iio_info srf08_info = {
@@ -362,6 +409,13 @@ static int srf08_probe(struct i2c_client *client,
 
 	mutex_init(&data->lock);
 
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
+			iio_pollfunc_store_time, srf08_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(&client->dev, "setup of iio triggered buffer failed\n");
+		return ret;
+	}
+
 	/*
 	 * set default values of device here
 	 * these register values cannot be read from the hardware
@@ -402,5 +456,5 @@ static struct i2c_driver srf08_driver = {
 module_i2c_driver(srf08_driver);
 
 MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
-MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
+MODULE_DESCRIPTION("Devantech SRF08/SRF10 ultrasonic ranger driver");
 MODULE_LICENSE("GPL");
-- 
2.1.4


-- 

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v3 3/5] iio: srf08: add triggered buffer support
  2017-08-16 19:34 [PATCH v3 3/5] iio: srf08: add triggered buffer support Andreas Klinger
@ 2017-08-18  6:56 ` Jonathan Cameron
  2017-08-18  7:01   ` Jonathan Cameron
  0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2017-08-18  6:56 UTC (permalink / raw)
  To: Andreas Klinger
  Cc: knaack.h, lars, pmeerw, linux-iio, wsa, robh+dt, mark.rutland,
	linux-i2c, devicetree, linux-kernel

On Wed, 16 Aug 2017 21:34:06 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Add support for triggered buffers.
> 
> Data format is quite simple:
>   distance     16 Bit
>   alignment    48 Bit
>   timestamp    64 Bit
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

One little improvement I noticed inline that an additional unlocked
reading function would tidy up.

Thanks,

Jonathan
> ---
>  drivers/iio/proximity/srf08.c | 60 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/proximity/srf08.c b/drivers/iio/proximity/srf08.c
> index 3f19536f215f..de699d67310a 100644
> --- a/drivers/iio/proximity/srf08.c
> +++ b/drivers/iio/proximity/srf08.c
> @@ -18,6 +18,9 @@
>  #include <linux/bitops.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  /* registers of SRF08 device */
>  #define SRF08_WRITE_COMMAND	0x00	/* Command Register */
> @@ -35,9 +38,22 @@
>  
>  struct srf08_data {
>  	struct i2c_client	*client;
> -	int			sensitivity;		/* Gain */
> -	int			range_mm;		/* max. Range in mm */
> +
> +	/*
> +	 * Gain in the datasheet is called sensitivity here to distinct it
> +	 * from the gain used with amplifiers of adc's
> +	 */
> +	int			sensitivity;
> +
> +	/* max. Range in mm */
> +	int			range_mm;
>  	struct mutex		lock;
> +
> +	/*
> +	 * triggered buffer
> +	 * 1x16-bit channel + 3x16 padding + 4x16 timestamp
> +	 */
> +	s16			buffer[8];
>  };
>  
>  /*
> @@ -110,6 +126,29 @@ static int srf08_read_ranging(struct srf08_data *data)
>  	return ret;
>  }
>  
> +static irqreturn_t srf08_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct srf08_data *data = iio_priv(indio_dev);
> +	s16 sensor_data;
> +

There is a bit of lock bouncing going on here where you take the lock
inside srf08_read_ranging drop it at the end and immediately
retake it.  

Introduce an unlocked version of read_ranging and hold the lock over
the whole cycle.
> +	sensor_data = srf08_read_ranging(data);
> +	if (sensor_data < 0)
> +		goto err;
> +
> +	mutex_lock(&data->lock);
> +
> +	data->buffer[0] = sensor_data;
> +	iio_push_to_buffers_with_timestamp(indio_dev,
> +						data->buffer, pf->timestamp);
> +
> +	mutex_unlock(&data->lock);
> +err:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static int srf08_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *channel, int *val,
>  			    int *val2, long mask)
> @@ -323,7 +362,15 @@ static const struct iio_chan_spec srf08_channels[] = {
>  		.info_mask_separate =
>  				BIT(IIO_CHAN_INFO_RAW) |
>  				BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
>  	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
>  };
>  
>  static const struct iio_info srf08_info = {
> @@ -362,6 +409,13 @@ static int srf08_probe(struct i2c_client *client,
>  
>  	mutex_init(&data->lock);
>  
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
> +			iio_pollfunc_store_time, srf08_trigger_handler, NULL);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "setup of iio triggered buffer failed\n");
> +		return ret;
> +	}
> +
>  	/*
>  	 * set default values of device here
>  	 * these register values cannot be read from the hardware
> @@ -402,5 +456,5 @@ static struct i2c_driver srf08_driver = {
>  module_i2c_driver(srf08_driver);
>  
>  MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
> -MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
> +MODULE_DESCRIPTION("Devantech SRF08/SRF10 ultrasonic ranger driver");
>  MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v3 3/5] iio: srf08: add triggered buffer support
  2017-08-18  6:56 ` Jonathan Cameron
@ 2017-08-18  7:01   ` Jonathan Cameron
  0 siblings, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2017-08-18  7:01 UTC (permalink / raw)
  To: Andreas Klinger
  Cc: knaack.h, lars, pmeerw, linux-iio, wsa, robh+dt, mark.rutland,
	linux-i2c, devicetree, linux-kernel

On Fri, 18 Aug 2017 07:56:05 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 16 Aug 2017 21:34:06 +0200
> Andreas Klinger <ak@it-klinger.de> wrote:
> 
> > Add support for triggered buffers.
> > 
> > Data format is quite simple:
> >   distance     16 Bit
> >   alignment    48 Bit
> >   timestamp    64 Bit
> > 
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>  
> 
> One little improvement I noticed inline that an additional unlocked
> reading function would tidy up.

Let us treat this as a suggestion for a follow up patch.
It isn't substantial enough to prevent me takin the patch in it's
current state.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> 
> Thanks,
> 
> Jonathan
> > ---
> >  drivers/iio/proximity/srf08.c | 60 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 57 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/proximity/srf08.c b/drivers/iio/proximity/srf08.c
> > index 3f19536f215f..de699d67310a 100644
> > --- a/drivers/iio/proximity/srf08.c
> > +++ b/drivers/iio/proximity/srf08.c
> > @@ -18,6 +18,9 @@
> >  #include <linux/bitops.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> >  
> >  /* registers of SRF08 device */
> >  #define SRF08_WRITE_COMMAND	0x00	/* Command Register */
> > @@ -35,9 +38,22 @@
> >  
> >  struct srf08_data {
> >  	struct i2c_client	*client;
> > -	int			sensitivity;		/* Gain */
> > -	int			range_mm;		/* max. Range in mm */
> > +
> > +	/*
> > +	 * Gain in the datasheet is called sensitivity here to distinct it
> > +	 * from the gain used with amplifiers of adc's
> > +	 */
> > +	int			sensitivity;
> > +
> > +	/* max. Range in mm */
> > +	int			range_mm;
> >  	struct mutex		lock;
> > +
> > +	/*
> > +	 * triggered buffer
> > +	 * 1x16-bit channel + 3x16 padding + 4x16 timestamp
> > +	 */
> > +	s16			buffer[8];
> >  };
> >  
> >  /*
> > @@ -110,6 +126,29 @@ static int srf08_read_ranging(struct srf08_data *data)
> >  	return ret;
> >  }
> >  
> > +static irqreturn_t srf08_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct srf08_data *data = iio_priv(indio_dev);
> > +	s16 sensor_data;
> > +  
> 
> There is a bit of lock bouncing going on here where you take the lock
> inside srf08_read_ranging drop it at the end and immediately
> retake it.  
> 
> Introduce an unlocked version of read_ranging and hold the lock over
> the whole cycle.
> > +	sensor_data = srf08_read_ranging(data);
> > +	if (sensor_data < 0)
> > +		goto err;
> > +
> > +	mutex_lock(&data->lock);
> > +
> > +	data->buffer[0] = sensor_data;
> > +	iio_push_to_buffers_with_timestamp(indio_dev,
> > +						data->buffer, pf->timestamp);
> > +
> > +	mutex_unlock(&data->lock);
> > +err:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int srf08_read_raw(struct iio_dev *indio_dev,
> >  			    struct iio_chan_spec const *channel, int *val,
> >  			    int *val2, long mask)
> > @@ -323,7 +362,15 @@ static const struct iio_chan_spec srf08_channels[] = {
> >  		.info_mask_separate =
> >  				BIT(IIO_CHAN_INFO_RAW) |
> >  				BIT(IIO_CHAN_INFO_SCALE),
> > +		.scan_index = 0,
> > +		.scan_type = {
> > +			.sign = 's',
> > +			.realbits = 16,
> > +			.storagebits = 16,
> > +			.endianness = IIO_CPU,
> > +		},
> >  	},
> > +	IIO_CHAN_SOFT_TIMESTAMP(1),
> >  };
> >  
> >  static const struct iio_info srf08_info = {
> > @@ -362,6 +409,13 @@ static int srf08_probe(struct i2c_client *client,
> >  
> >  	mutex_init(&data->lock);
> >  
> > +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
> > +			iio_pollfunc_store_time, srf08_trigger_handler, NULL);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "setup of iio triggered buffer failed\n");
> > +		return ret;
> > +	}
> > +
> >  	/*
> >  	 * set default values of device here
> >  	 * these register values cannot be read from the hardware
> > @@ -402,5 +456,5 @@ static struct i2c_driver srf08_driver = {
> >  module_i2c_driver(srf08_driver);
> >  
> >  MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
> > -MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
> > +MODULE_DESCRIPTION("Devantech SRF08/SRF10 ultrasonic ranger driver");
> >  MODULE_LICENSE("GPL");  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-08-18  7:04 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-16 19:34 [PATCH v3 3/5] iio: srf08: add triggered buffer support Andreas Klinger
2017-08-18  6:56 ` Jonathan Cameron
2017-08-18  7:01   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).