Re: [PATCH v3 2/6] iio: pulse: add TI ECAP driver

2014-03-21 Thread Matt Ranostay
On Wed, Feb 5, 2014 at 11:01 AM, Matt Porter  wrote:
> Adds support for capturing PWM signals using the TI ECAP peripheral.
> This driver supports triggered buffer capture of pulses on multiple
> ECAP instances. In addition, the driver supports configurable polarity
> of the signal to be captured.
>
> Signed-off-by: Matt Porter 

Tested-by: Matt Ranostay 

> ---
>  drivers/iio/pulse/Kconfig  |  20 ++
>  drivers/iio/pulse/Makefile |   6 +
>  drivers/iio/pulse/tiecap.c | 491 
> +
>  3 files changed, 517 insertions(+)
>  create mode 100644 drivers/iio/pulse/Kconfig
>  create mode 100644 drivers/iio/pulse/Makefile
>  create mode 100644 drivers/iio/pulse/tiecap.c
>
> diff --git a/drivers/iio/pulse/Kconfig b/drivers/iio/pulse/Kconfig
> new file mode 100644
> index 000..9864d4b
> --- /dev/null
> +++ b/drivers/iio/pulse/Kconfig
> @@ -0,0 +1,20 @@
> +#
> +# Pulse Capture Devices
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Pulse Capture Devices"
> +
> +config IIO_TIECAP
> +   tristate "TI ECAP Pulse Capture"
> +   depends on SOC_AM33XX
> +   select IIO_BUFFER
> +   select IIO_TRIGGERED_BUFFER
> +   help
> +If you say yes here you get support for the TI ECAP peripheral
> +in pulse capture mode.
> +
> +This driver can also be built as a module.  If so, the module
> +will be called tiecap
> +
> +endmenu
> diff --git a/drivers/iio/pulse/Makefile b/drivers/iio/pulse/Makefile
> new file mode 100644
> index 000..94d4b00
> --- /dev/null
> +++ b/drivers/iio/pulse/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for IIO PWM Capture Devices
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_IIO_TIECAP)   += tiecap.o
> diff --git a/drivers/iio/pulse/tiecap.c b/drivers/iio/pulse/tiecap.c
> new file mode 100644
> index 000..fd96745
> --- /dev/null
> +++ b/drivers/iio/pulse/tiecap.c
> @@ -0,0 +1,491 @@
> +/*
> + * ECAP IIO pulse capture driver
> + *
> + * Copyright (C) 2014 Linaro Limited
> + * Author: Matt Porter 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "../../pwm/pwm-tipwmss.h"
> +
> +/* ECAP regs and bits */
> +#define CAP1   0x08
> +#define CAP2   0x0c
> +#define ECCTL1 0x28
> +#define ECCTL1_RUN_FREEBIT(15)
> +#define ECCTL1_CAPLDEN BIT(8)
> +#define ECCTL1_CAP2POL BIT(2)
> +#define ECCTL1_CTRRST1 BIT(1)
> +#define ECCTL1_CAP1POL BIT(0)
> +#define ECCTL2 0x2a
> +#define ECCTL2_SYNCO_SEL_DIS   BIT(7)
> +#define ECCTL2_TSCTR_FREERUN   BIT(4)
> +#define ECCTL2_REARM   BIT(3)
> +#define ECCTL2_STOP_WRAP_2 BIT(1)
> +#define ECEINT 0x2c
> +#define ECFLG  0x2e
> +#define ECCLR  0x30
> +#define ECINT_CTRCMP   BIT(7)
> +#define ECINT_CTRPRD   BIT(6)
> +#define ECINT_CTROVF   BIT(5)
> +#define ECINT_CEVT4BIT(4)
> +#define ECINT_CEVT3BIT(3)
> +#define ECINT_CEVT2BIT(2)
> +#define ECINT_CEVT1BIT(1)
> +#define ECINT_ALL  (ECINT_CTRCMP | \
> +   ECINT_CTRPRD |  \
> +   ECINT_CTROVF |  \
> +   ECINT_CEVT4 |   \
> +   ECINT_CEVT3 |   \
> +   ECINT_CEVT2 |   \
> +   ECINT_CEVT1)
> +
> +/* ECAP driver flags */
> +#define ECAP_POLARITY_HIGH BIT(1)
> +#define ECAP_ENABLED   BIT(0)
> +
> +struct ecap_context {
> +   u32 cap1;
> +   u32 cap2;
> +   u16 ecctl1;
> +   u16 ecctl2;
> +   u16 eceint;
> +};
> +
> +struct ecap_state {
> +   unsigned long   flags;
> +   unsigned intclk_rate;
> +   void __iomem*regs;
> +   u32 *buf;
> +   struct ecap_context ctx;
> +};
> +
> +#define dev_to_ecap_state(d)   iio_priv(dev_to_iio_dev(d))
> +
> +static const struct iio_chan_spec ecap_channels[] = {
> +   {
> +   .type   = IIO_PULSE,
> +   .info_mask_separate =
> +   BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +   .scan_index = 0,
> +   .scan_type = {
> +   .sign   = 'u',
> +   .realbits   = 32,
> +   .storagebits= 32,
> +  

Re: [PATCH v3 2/6] iio: pulse: add TI ECAP driver

2014-02-15 Thread Jonathan Cameron

On 05/02/14 19:01, Matt Porter wrote:

Adds support for capturing PWM signals using the TI ECAP peripheral.
This driver supports triggered buffer capture of pulses on multiple
ECAP instances. In addition, the driver supports configurable polarity
of the signal to be captured.

Signed-off-by: Matt Porter 

Ignoring the ABI questions - there are a few bits and bobs inline.

Sorry again that it took me so long to take a look at this.
Ouch, still far too much unread email in my inbox.

---
  drivers/iio/pulse/Kconfig  |  20 ++
  drivers/iio/pulse/Makefile |   6 +
  drivers/iio/pulse/tiecap.c | 491 +
  3 files changed, 517 insertions(+)
  create mode 100644 drivers/iio/pulse/Kconfig
  create mode 100644 drivers/iio/pulse/Makefile
  create mode 100644 drivers/iio/pulse/tiecap.c

diff --git a/drivers/iio/pulse/Kconfig b/drivers/iio/pulse/Kconfig
new file mode 100644
index 000..9864d4b
--- /dev/null
+++ b/drivers/iio/pulse/Kconfig
@@ -0,0 +1,20 @@
+#
+# Pulse Capture Devices
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Pulse Capture Devices"
+
+config IIO_TIECAP
+   tristate "TI ECAP Pulse Capture"
+   depends on SOC_AM33XX

Looks like it rather more specifically depends on the pwm supplied on
that soc.  I haven't checked but a dependency on that would seem
more logical to me.  Pitty it doesn't have a generic interface, because
then we could relax the dependency and it would allow you to get a lot
more build coverage.

+   select IIO_BUFFER
+   select IIO_TRIGGERED_BUFFER
+   help
+If you say yes here you get support for the TI ECAP peripheral
+in pulse capture mode.
+
+This driver can also be built as a module.  If so, the module
+will be called tiecap
+
+endmenu
diff --git a/drivers/iio/pulse/Makefile b/drivers/iio/pulse/Makefile
new file mode 100644
index 000..94d4b00
--- /dev/null
+++ b/drivers/iio/pulse/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for IIO PWM Capture Devices
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_IIO_TIECAP)   += tiecap.o
diff --git a/drivers/iio/pulse/tiecap.c b/drivers/iio/pulse/tiecap.c
new file mode 100644
index 000..fd96745
--- /dev/null
+++ b/drivers/iio/pulse/tiecap.c
@@ -0,0 +1,491 @@
+/*
+ * ECAP IIO pulse capture driver
+ *
+ * Copyright (C) 2014 Linaro Limited
+ * Author: Matt Porter 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "../../pwm/pwm-tipwmss.h"

If we need functions from in there, surely it should be in
include/linux/pwm instead of burried in the driver tree?

+
+/* ECAP regs and bits */

Please add prefixes to all of these.  i.e. #define ECAP_CAP1
etc.

This is to avoid the possibility of a naming clash if these rather
short names ever turn in in one of the headers included above.


+#define CAP1   0x08
+#define CAP2   0x0c
+#define ECCTL1 0x28
+#define ECCTL1_RUN_FREEBIT(15)
+#define ECCTL1_CAPLDEN BIT(8)
+#define ECCTL1_CAP2POL BIT(2)
+#define ECCTL1_CTRRST1 BIT(1)
+#define ECCTL1_CAP1POL BIT(0)
+#define ECCTL2 0x2a
+#define ECCTL2_SYNCO_SEL_DIS   BIT(7)
+#define ECCTL2_TSCTR_FREERUN   BIT(4)
+#define ECCTL2_REARM   BIT(3)
+#define ECCTL2_STOP_WRAP_2 BIT(1)
+#define ECEINT 0x2c
+#define ECFLG  0x2e
+#define ECCLR  0x30
+#define ECINT_CTRCMP   BIT(7)
+#define ECINT_CTRPRD   BIT(6)
+#define ECINT_CTROVF   BIT(5)
+#define ECINT_CEVT4BIT(4)
+#define ECINT_CEVT3BIT(3)
+#define ECINT_CEVT2BIT(2)
+#define ECINT_CEVT1BIT(1)
+#define ECINT_ALL  (ECINT_CTRCMP | \
+   ECINT_CTRPRD |  \
+   ECINT_CTROVF |  \
+   ECINT_CEVT4 |   \
+   ECINT_CEVT3 |   \
+   ECINT_CEVT2 |   \
+   ECINT_CEVT1)
+
+/* ECAP driver flags */
+#define ECAP_POLARITY_HIGH BIT(1)
+#define ECAP_ENABLED   BIT(0)
+
+struct ecap_context {
+   u32 cap1;
+   u32 cap2;
+   u16 ecctl1;
+   u16 ecctl2;
+   u16 eceint;
+};
+
+struct ecap_state {
+   unsigned long   flags;
+   unsigned intclk_rate;
+   void __iomem*regs;
+   u32 *buf;
+   struct ecap_context ctx;
+};
+
+#define dev_to_ecap_state(d)   iio_priv(d

[PATCH v3 2/6] iio: pulse: add TI ECAP driver

2014-02-05 Thread Matt Porter
Adds support for capturing PWM signals using the TI ECAP peripheral.
This driver supports triggered buffer capture of pulses on multiple
ECAP instances. In addition, the driver supports configurable polarity
of the signal to be captured.

Signed-off-by: Matt Porter 
---
 drivers/iio/pulse/Kconfig  |  20 ++
 drivers/iio/pulse/Makefile |   6 +
 drivers/iio/pulse/tiecap.c | 491 +
 3 files changed, 517 insertions(+)
 create mode 100644 drivers/iio/pulse/Kconfig
 create mode 100644 drivers/iio/pulse/Makefile
 create mode 100644 drivers/iio/pulse/tiecap.c

diff --git a/drivers/iio/pulse/Kconfig b/drivers/iio/pulse/Kconfig
new file mode 100644
index 000..9864d4b
--- /dev/null
+++ b/drivers/iio/pulse/Kconfig
@@ -0,0 +1,20 @@
+#
+# Pulse Capture Devices
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Pulse Capture Devices"
+
+config IIO_TIECAP
+   tristate "TI ECAP Pulse Capture"
+   depends on SOC_AM33XX
+   select IIO_BUFFER
+   select IIO_TRIGGERED_BUFFER
+   help
+If you say yes here you get support for the TI ECAP peripheral
+in pulse capture mode.
+
+This driver can also be built as a module.  If so, the module
+will be called tiecap
+
+endmenu
diff --git a/drivers/iio/pulse/Makefile b/drivers/iio/pulse/Makefile
new file mode 100644
index 000..94d4b00
--- /dev/null
+++ b/drivers/iio/pulse/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for IIO PWM Capture Devices
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_IIO_TIECAP)   += tiecap.o
diff --git a/drivers/iio/pulse/tiecap.c b/drivers/iio/pulse/tiecap.c
new file mode 100644
index 000..fd96745
--- /dev/null
+++ b/drivers/iio/pulse/tiecap.c
@@ -0,0 +1,491 @@
+/*
+ * ECAP IIO pulse capture driver
+ *
+ * Copyright (C) 2014 Linaro Limited
+ * Author: Matt Porter 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "../../pwm/pwm-tipwmss.h"
+
+/* ECAP regs and bits */
+#define CAP1   0x08
+#define CAP2   0x0c
+#define ECCTL1 0x28
+#define ECCTL1_RUN_FREEBIT(15)
+#define ECCTL1_CAPLDEN BIT(8)
+#define ECCTL1_CAP2POL BIT(2)
+#define ECCTL1_CTRRST1 BIT(1)
+#define ECCTL1_CAP1POL BIT(0)
+#define ECCTL2 0x2a
+#define ECCTL2_SYNCO_SEL_DIS   BIT(7)
+#define ECCTL2_TSCTR_FREERUN   BIT(4)
+#define ECCTL2_REARM   BIT(3)
+#define ECCTL2_STOP_WRAP_2 BIT(1)
+#define ECEINT 0x2c
+#define ECFLG  0x2e
+#define ECCLR  0x30
+#define ECINT_CTRCMP   BIT(7)
+#define ECINT_CTRPRD   BIT(6)
+#define ECINT_CTROVF   BIT(5)
+#define ECINT_CEVT4BIT(4)
+#define ECINT_CEVT3BIT(3)
+#define ECINT_CEVT2BIT(2)
+#define ECINT_CEVT1BIT(1)
+#define ECINT_ALL  (ECINT_CTRCMP | \
+   ECINT_CTRPRD |  \
+   ECINT_CTROVF |  \
+   ECINT_CEVT4 |   \
+   ECINT_CEVT3 |   \
+   ECINT_CEVT2 |   \
+   ECINT_CEVT1)
+
+/* ECAP driver flags */
+#define ECAP_POLARITY_HIGH BIT(1)
+#define ECAP_ENABLED   BIT(0)
+
+struct ecap_context {
+   u32 cap1;
+   u32 cap2;
+   u16 ecctl1;
+   u16 ecctl2;
+   u16 eceint;
+};
+
+struct ecap_state {
+   unsigned long   flags;
+   unsigned intclk_rate;
+   void __iomem*regs;
+   u32 *buf;
+   struct ecap_context ctx;
+};
+
+#define dev_to_ecap_state(d)   iio_priv(dev_to_iio_dev(d))
+
+static const struct iio_chan_spec ecap_channels[] = {
+   {
+   .type   = IIO_PULSE,
+   .info_mask_separate =
+   BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+   .scan_index = 0,
+   .scan_type = {
+   .sign   = 'u',
+   .realbits   = 32,
+   .storagebits= 32,
+   .endianness = IIO_LE,
+   },
+   },
+   IIO_CHAN_SOFT_TIMESTAMP(1)
+};
+
+static ssize_t ecap_attr_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+   struct ecap_state *state = dev_to_ecap_state(dev);
+
+   return sprintf(buf, "%d\n",
+  test_bit(ECAP_POLARITY_HIGH, &