Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1368467 > unrolled thread

[PATCH 2/2] ti-adc081c: Initial triggered buffer support

Started byCrestez Dan Leonard <leonard.crestez@intel.com>
First post2016-03-31 19:30 +0200
Last post2016-04-01 13:20 +0200
Articles 5 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] ti-adc081c: Initial triggered buffer support Crestez Dan Leonard <leonard.crestez@intel.com> - 2016-03-31 19:30 +0200
    Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support Crt Mori <cmo@melexis.com> - 2016-04-01 10:10 +0200
      Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support Lars-Peter Clausen <lars@metafoo.de> - 2016-04-01 11:30 +0200
    Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support Peter Meerwald-Stadler <pmeerw@pmeerw.net> - 2016-04-01 10:40 +0200
      Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support Leonard Crestez <leonard.crestez@intel.com> - 2016-04-01 13:20 +0200

#1368467 — [PATCH 2/2] ti-adc081c: Initial triggered buffer support

FromCrestez Dan Leonard <leonard.crestez@intel.com>
Date2016-03-31 19:30 +0200
Subject[PATCH 2/2] ti-adc081c: Initial triggered buffer support
Message-ID<riOzU-8pW-13@gated-at.bofh.it>
Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.

The device can be configured to do internal periodic sampling but does
not appear to offer some sort of interrupt on data ready. It only offers
interrupts on values out of a specific range.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/adc/ti-adc081c.c | 99 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
index 9b2f26f..040e2aa 100644
--- a/drivers/iio/adc/ti-adc081c.c
+++ b/drivers/iio/adc/ti-adc081c.c
@@ -24,6 +24,9 @@
 #include <linux/of.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/regulator/consumer.h>
 
 struct adc081c {
@@ -69,27 +72,83 @@ static int adc081c_read_raw(struct iio_dev *iio,
 	return -EINVAL;
 }
 
-static const struct iio_chan_spec adc081c_channel = {
-	.type = IIO_VOLTAGE,
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-};
+static irqreturn_t adc081c_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adc081c *data = iio_priv(indio_dev);
+	s64 ts;
+	u16 buf[8];
+	int ret;
+
+	/* Otherwise iio_push_to_buffers will corrupt the stack. */
+	if (indio_dev->scan_bytes > sizeof(buf)) {
+		dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
+				indio_dev->scan_bytes, (int)sizeof(buf));
+		goto out;
+	}
+
+	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
+	ts = iio_get_time_ns();
+	if (ret < 0)
+		goto out;
+	buf[0] = ret;
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
 
 static const struct iio_info adc081c_info = {
 	.read_raw = adc081c_read_raw,
 	.driver_module = THIS_MODULE,
 };
 
+struct adcxx1c_model {
+	int bits;
+	const struct iio_chan_spec* channels;
+};
+
+#define ADCxx1C_CHAN(_bits) {					\
+	.type = IIO_VOLTAGE,					\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = (_bits),				\
+		.storagebits = 16,				\
+		.shift = 12 - (_bits),				\
+		.endianness = IIO_CPU,				\
+	},							\
+}
+
+#define DEFINE_ADCxx1C_MODEL(_name, _bits)				\
+	static const struct iio_chan_spec _name ## _channels[] = {	\
+		ADCxx1C_CHAN((_bits)),					\
+		IIO_CHAN_SOFT_TIMESTAMP(1),				\
+	};								\
+	static const struct adcxx1c_model _name ## _model = {		\
+		.bits = (_bits),					\
+		.channels = _name ## _channels,				\
+	}
+
+DEFINE_ADCxx1C_MODEL(adc081c,  8);
+DEFINE_ADCxx1C_MODEL(adc101c, 10);
+DEFINE_ADCxx1C_MODEL(adc121c, 12);
+
+struct adcxx1c_info {
+	int bits;
+	const struct adc081c_channels* channels;
+};
+
 static int adc081c_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct iio_dev *iio;
 	struct adc081c *adc;
+	struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
 	int err;
 
-	if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
-		return -EINVAL;
-
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
@@ -99,7 +158,7 @@ static int adc081c_probe(struct i2c_client *client,
 
 	adc = iio_priv(iio);
 	adc->i2c = client;
-	adc->bits = id->driver_data;
+	adc->bits = model->bits;
 
 	adc->ref = devm_regulator_get(&client->dev, "vref");
 	if (IS_ERR(adc->ref))
@@ -114,18 +173,26 @@ static int adc081c_probe(struct i2c_client *client,
 	iio->modes = INDIO_DIRECT_MODE;
 	iio->info = &adc081c_info;
 
-	iio->channels = &adc081c_channel;
-	iio->num_channels = 1;
+	iio->channels = model->channels;
+	iio->num_channels = 2;
+
+	err = iio_triggered_buffer_setup(iio, NULL, adc081c_trigger_handler, NULL);
+	if (err < 0) {
+		dev_err(&client->dev, "iio triggered buffer setup failed\n");
+		goto err_regulator_disable;
+	}
 
 	err = iio_device_register(iio);
 	if (err < 0)
-		goto regulator_disable;
+		goto err_buffer_cleanup;
 
 	i2c_set_clientdata(client, iio);
 
 	return 0;
 
-regulator_disable:
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(iio);
+err_regulator_disable:
 	regulator_disable(adc->ref);
 
 	return err;
@@ -143,9 +210,9 @@ static int adc081c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id adc081c_id[] = {
-	{ "adc081c",  8 },
-	{ "adc101c", 10 },
-	{ "adc121c", 12 },
+	{ "adc081c", (long)&adc081c_model },
+	{ "adc101c", (long)&adc101c_model },
+	{ "adc121c", (long)&adc121c_model },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
-- 
2.8.0.rc3

[toc] | [next] | [standalone]

#1369087

FromCrt Mori <cmo@melexis.com>
Date2016-04-01 10:10 +0200
Message-ID<rj2jw-1v4-19@gated-at.bofh.it>
In reply to#1368467
On 31 March 2016 at 19:20, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:
> Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.
Then we are missing DEPENDS in Kconfig...

>
> The device can be configured to do internal periodic sampling but does
> not appear to offer some sort of interrupt on data ready. It only offers
> interrupts on values out of a specific range.
>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/adc/ti-adc081c.c | 99 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 83 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
> index 9b2f26f..040e2aa 100644
> --- a/drivers/iio/adc/ti-adc081c.c
> +++ b/drivers/iio/adc/ti-adc081c.c
> @@ -24,6 +24,9 @@
>  #include <linux/of.h>
>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/regulator/consumer.h>
>
>  struct adc081c {
> @@ -69,27 +72,83 @@ static int adc081c_read_raw(struct iio_dev *iio,
>         return -EINVAL;
>  }
>
> -static const struct iio_chan_spec adc081c_channel = {
> -       .type = IIO_VOLTAGE,
> -       .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> -       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -};
> +static irqreturn_t adc081c_trigger_handler(int irq, void *p)
> +{
> +       struct iio_poll_func *pf = p;
> +       struct iio_dev *indio_dev = pf->indio_dev;
> +       struct adc081c *data = iio_priv(indio_dev);
> +       s64 ts;
> +       u16 buf[8];
> +       int ret;
> +
> +       /* Otherwise iio_push_to_buffers will corrupt the stack. */
> +       if (indio_dev->scan_bytes > sizeof(buf)) {
> +               dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
> +                               indio_dev->scan_bytes, (int)sizeof(buf));
> +               goto out;
> +       }
> +
> +       ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
> +       ts = iio_get_time_ns();
> +       if (ret < 0)
> +               goto out;
> +       buf[0] = ret;
> +       iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
> +out:
> +       iio_trigger_notify_done(indio_dev->trig);
> +       return IRQ_HANDLED;
> +}
>
>  static const struct iio_info adc081c_info = {
>         .read_raw = adc081c_read_raw,
>         .driver_module = THIS_MODULE,
>  };
>
> +struct adcxx1c_model {
> +       int bits;
> +       const struct iio_chan_spec* channels;
> +};
> +
> +#define ADCxx1C_CHAN(_bits) {                                  \
> +       .type = IIO_VOLTAGE,                                    \
> +       .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> +       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> +       .scan_type = {                                          \
> +               .sign = 'u',                                    \
> +               .realbits = (_bits),                            \
> +               .storagebits = 16,                              \
> +               .shift = 12 - (_bits),                          \
> +               .endianness = IIO_CPU,                          \
> +       },                                                      \
> +}
> +
> +#define DEFINE_ADCxx1C_MODEL(_name, _bits)                             \
> +       static const struct iio_chan_spec _name ## _channels[] = {      \
> +               ADCxx1C_CHAN((_bits)),                                  \
> +               IIO_CHAN_SOFT_TIMESTAMP(1),                             \
> +       };                                                              \
> +       static const struct adcxx1c_model _name ## _model = {           \
> +               .bits = (_bits),                                        \
> +               .channels = _name ## _channels,                         \
> +       }
> +
> +DEFINE_ADCxx1C_MODEL(adc081c,  8);
> +DEFINE_ADCxx1C_MODEL(adc101c, 10);
> +DEFINE_ADCxx1C_MODEL(adc121c, 12);
> +
> +struct adcxx1c_info {
> +       int bits;
> +       const struct adc081c_channels* channels;
> +};
> +
>  static int adc081c_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>  {
>         struct iio_dev *iio;
>         struct adc081c *adc;
> +       struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
>         int err;
>
> -       if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
> -               return -EINVAL;
> -
>         if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
>                 return -EOPNOTSUPP;
>
> @@ -99,7 +158,7 @@ static int adc081c_probe(struct i2c_client *client,
>
>         adc = iio_priv(iio);
>         adc->i2c = client;
> -       adc->bits = id->driver_data;
> +       adc->bits = model->bits;
>
>         adc->ref = devm_regulator_get(&client->dev, "vref");
>         if (IS_ERR(adc->ref))
> @@ -114,18 +173,26 @@ static int adc081c_probe(struct i2c_client *client,
>         iio->modes = INDIO_DIRECT_MODE;
>         iio->info = &adc081c_info;
>
> -       iio->channels = &adc081c_channel;
> -       iio->num_channels = 1;
> +       iio->channels = model->channels;
> +       iio->num_channels = 2;
> +
> +       err = iio_triggered_buffer_setup(iio, NULL, adc081c_trigger_handler, NULL);
> +       if (err < 0) {
> +               dev_err(&client->dev, "iio triggered buffer setup failed\n");
> +               goto err_regulator_disable;
> +       }
>
>         err = iio_device_register(iio);
>         if (err < 0)
> -               goto regulator_disable;
> +               goto err_buffer_cleanup;
>
>         i2c_set_clientdata(client, iio);
>
>         return 0;
>
> -regulator_disable:
> +err_buffer_cleanup:
> +       iio_triggered_buffer_cleanup(iio);
> +err_regulator_disable:
>         regulator_disable(adc->ref);
>
>         return err;
> @@ -143,9 +210,9 @@ static int adc081c_remove(struct i2c_client *client)
>  }
>
>  static const struct i2c_device_id adc081c_id[] = {
> -       { "adc081c",  8 },
> -       { "adc101c", 10 },
> -       { "adc121c", 12 },
> +       { "adc081c", (long)&adc081c_model },
> +       { "adc101c", (long)&adc101c_model },
> +       { "adc121c", (long)&adc121c_model },
>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, adc081c_id);
> --
> 2.8.0.rc3
>
> --
> 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

[toc] | [prev] | [next] | [standalone]

#1369128

FromLars-Peter Clausen <lars@metafoo.de>
Date2016-04-01 11:30 +0200
Message-ID<rj3yW-2hY-17@gated-at.bofh.it>
In reply to#1369087
On 04/01/2016 10:08 AM, Crt Mori wrote:
> On 31 March 2016 at 19:20, Crestez Dan Leonard
> <leonard.crestez@intel.com> wrote:
>> Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.
> Then we are missing DEPENDS in Kconfig...

The device could be used with any generic trigger. The device driver
shouldn't make the choice here which one it requires and which one not.

- Lars

[toc] | [prev] | [next] | [standalone]

#1369103

FromPeter Meerwald-Stadler <pmeerw@pmeerw.net>
Date2016-04-01 10:40 +0200
Message-ID<rj2Mx-1GR-1@gated-at.bofh.it>
In reply to#1368467
> Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.
> 
> The device can be configured to do internal periodic sampling but does
> not appear to offer some sort of interrupt on data ready. It only offers
> interrupts on values out of a specific range.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/adc/ti-adc081c.c | 99 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 83 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
> index 9b2f26f..040e2aa 100644
> --- a/drivers/iio/adc/ti-adc081c.c
> +++ b/drivers/iio/adc/ti-adc081c.c
> @@ -24,6 +24,9 @@
>  #include <linux/of.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/regulator/consumer.h>
>  
>  struct adc081c {
> @@ -69,27 +72,83 @@ static int adc081c_read_raw(struct iio_dev *iio,
>  	return -EINVAL;
>  }
>  
> -static const struct iio_chan_spec adc081c_channel = {
> -	.type = IIO_VOLTAGE,
> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -};

the patch would look cleaner/shorter if adc081c_channel won't get moved 
around

> +static irqreturn_t adc081c_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct adc081c *data = iio_priv(indio_dev);
> +	s64 ts;
> +	u16 buf[8];

comment: 2 bytes data + 6 bytes padding + 8 bytes timestamp

> +	int ret;
> +
> +	/* Otherwise iio_push_to_buffers will corrupt the stack. */
> +	if (indio_dev->scan_bytes > sizeof(buf)) {
> +		dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
> +				indio_dev->scan_bytes, (int)sizeof(buf));

rather than casting sizeof(buf), use the correct printf length modifier, 
i.e. %z

not sure if this check is needed

> +		goto out;
> +	}
> +
> +	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);

REG_CONV_RES should be called ADC081C_REG_CONV_RES, but that's a separate 
issue

> +	ts = iio_get_time_ns();

why is the timestamp taken here?, seems strange
often this is done together with iio_push_to_buffers_with_timestamp()

> +	if (ret < 0)
> +		goto out;
> +	buf[0] = ret;
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
>  
>  static const struct iio_info adc081c_info = {
>  	.read_raw = adc081c_read_raw,
>  	.driver_module = THIS_MODULE,
>  };
>  
> +struct adcxx1c_model {
> +	int bits;
> +	const struct iio_chan_spec* channels;
> +};
> +
> +#define ADCxx1C_CHAN(_bits) {					\
> +	.type = IIO_VOLTAGE,					\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.scan_type = {						\
> +		.sign = 'u',					\
> +		.realbits = (_bits),				\
> +		.storagebits = 16,				\
> +		.shift = 12 - (_bits),				\
> +		.endianness = IIO_CPU,				\
> +	},							\
> +}
> +
> +#define DEFINE_ADCxx1C_MODEL(_name, _bits)				\
> +	static const struct iio_chan_spec _name ## _channels[] = {	\
> +		ADCxx1C_CHAN((_bits)),					\
> +		IIO_CHAN_SOFT_TIMESTAMP(1),				\
> +	};								\
> +	static const struct adcxx1c_model _name ## _model = {		\
> +		.bits = (_bits),					\
> +		.channels = _name ## _channels,				\
> +	}
> +
> +DEFINE_ADCxx1C_MODEL(adc081c,  8);
> +DEFINE_ADCxx1C_MODEL(adc101c, 10);
> +DEFINE_ADCxx1C_MODEL(adc121c, 12);
> +
> +struct adcxx1c_info {
> +	int bits;
> +	const struct adc081c_channels* channels;
> +};
> +
>  static int adc081c_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
>  	struct iio_dev *iio;
>  	struct adc081c *adc;
> +	struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
>  	int err;
>  
> -	if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
> -		return -EINVAL;
> -
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
>  		return -EOPNOTSUPP;
>  
> @@ -99,7 +158,7 @@ static int adc081c_probe(struct i2c_client *client,
>  
>  	adc = iio_priv(iio);
>  	adc->i2c = client;
> -	adc->bits = id->driver_data;
> +	adc->bits = model->bits;
>  
>  	adc->ref = devm_regulator_get(&client->dev, "vref");
>  	if (IS_ERR(adc->ref))
> @@ -114,18 +173,26 @@ static int adc081c_probe(struct i2c_client *client,
>  	iio->modes = INDIO_DIRECT_MODE;
>  	iio->info = &adc081c_info;
>  
> -	iio->channels = &adc081c_channel;
> -	iio->num_channels = 1;
> +	iio->channels = model->channels;
> +	iio->num_channels = 2;

the number of channels could go into the adcxx1c_info struct

> +
> +	err = iio_triggered_buffer_setup(iio, NULL, adc081c_trigger_handler, NULL);
> +	if (err < 0) {
> +		dev_err(&client->dev, "iio triggered buffer setup failed\n");
> +		goto err_regulator_disable;
> +	}
>  
>  	err = iio_device_register(iio);
>  	if (err < 0)
> -		goto regulator_disable;
> +		goto err_buffer_cleanup;
>  
>  	i2c_set_clientdata(client, iio);
>  
>  	return 0;
>  
> -regulator_disable:
> +err_buffer_cleanup:
> +	iio_triggered_buffer_cleanup(iio);
> +err_regulator_disable:
>  	regulator_disable(adc->ref);
>  
>  	return err;
> @@ -143,9 +210,9 @@ static int adc081c_remove(struct i2c_client *client)
>  }

iio_triggered_buffer_cleanup() in _remove()?

  
>  static const struct i2c_device_id adc081c_id[] = {
> -	{ "adc081c",  8 },
> -	{ "adc101c", 10 },
> -	{ "adc121c", 12 },
> +	{ "adc081c", (long)&adc081c_model },

often an enum is used instead of a pointer

> +	{ "adc101c", (long)&adc101c_model },
> +	{ "adc121c", (long)&adc121c_model },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, adc081c_id);
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

[toc] | [prev] | [next] | [standalone]

#1369228

FromLeonard Crestez <leonard.crestez@intel.com>
Date2016-04-01 13:20 +0200
Message-ID<rj5ho-3uY-9@gated-at.bofh.it>
In reply to#1369103
On 04/01/2016 11:34 AM, Peter Meerwald-Stadler wrote:
>> -static const struct iio_chan_spec adc081c_channel = {
>> -	.type = IIO_VOLTAGE,
>> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> -};
>
> the patch would look cleaner/shorter if adc081c_channel won't get moved
> around

It was not moved around, it is now defined by a macro. Buffer support 
requires defining scan_type which contains a different number of bits.

The macros are now after iio_info adc081c_info instead of before, I can 
move that around. I also noticed that I declared both a struct 
adcxx1c_model and an unused struct adcxx1c_info. I will remove that.

The first patch in the series doesn't use any per-model data and just 
stores the number of bits in driver_data. I can change the series to 
introduce adcxx1c_model in the first patch.

>> +static irqreturn_t adc081c_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct adc081c *data = iio_priv(indio_dev);
>> +	s64 ts;
>> +	u16 buf[8];
>
> comment: 2 bytes data + 6 bytes padding + 8 bytes timestamp
>
>> +	int ret;
>> +
>> +	/* Otherwise iio_push_to_buffers will corrupt the stack. */
>> +	if (indio_dev->scan_bytes > sizeof(buf)) {
>> +		dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
>> +				indio_dev->scan_bytes, (int)sizeof(buf));
>
> rather than casting sizeof(buf), use the correct printf length modifier,
> i.e. %z
>
> not sure if this check is needed

I guess it's not needed. My first version defined the buffer incorrectly 
and caused a messy crash. Calculating manual buffer alignments seems 
very fragile.

It seems that C99 variable-length-arrays work fine, something like:
	u16 buf[indio_dev->scan_bytes / 2];

Would that be acceptable? It compiles without warnings and there some 
other places in the kernel where VLAs are used.

>> +	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
>
> REG_CONV_RES should be called ADC081C_REG_CONV_RES, but that's a separate
> issue

Yes, but that would be an entirely unrelated renaming.

>
>> +	ts = iio_get_time_ns();
>
> why is the timestamp taken here?, seems strange
> often this is done together with iio_push_to_buffers_with_timestamp()

I wanted to keep it as close to the read as possible. In this case it 
doesn't matter.

>> -	iio->channels = &adc081c_channel;
>> -	iio->num_channels = 1;
>> +	iio->channels = model->channels;
>> +	iio->num_channels = 2;
>
> the number of channels could go into the adcxx1c_info struct

But it's a constant, it does not vary between devices. I could make an 
ADC081C_NUM_CHANNELS define.

--
Regards,
Leonard

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web