[PATCH 2/2] pps: new client driver using IRQs

[PATCH 2/2] pps: new client driver using IRQs

[James Nuss]

This client driver allows you to use raw IRQs as a source for PPS
signals.

This driver is based on the work by Ricardo Martins <rasm@fe.up.pt> who
submitted an initial implementation [1] of a PPS IRQ client driver to
the linuxpps mailing-list on Dec 3 2010.

[1] http://ml.enneenne.com/pipermail/linuxpps/2010-December/004155.html

Signed-off-by: James Nuss <jamesnuss@nanometrics.ca>
Reviewed-by: Ben Gardiner <bengardiner@nanometrics.ca>
CC: Ricardo Martins <rasm@fe.up.pt>
---
NOTE: drivers/pps/clients/Kconfig does not have the obligatory statements
regarding building the drivers as modules, even though they are all tristates.
This was purposefully omitted from the new config item for consistency.
This could be addressed in a separate patch.

 drivers/pps/clients/Kconfig   |    8 ++
 drivers/pps/clients/Makefile  |    1 +
 drivers/pps/clients/pps-irq.c |  169 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pps/clients/pps-irq.c

diff --git a/drivers/pps/clients/Kconfig b/drivers/pps/clients/Kconfig
index 8520a7f..94b2cc6 100644
--- a/drivers/pps/clients/Kconfig
+++ b/drivers/pps/clients/Kconfig
@@ -29,4 +29,12 @@ config PPS_CLIENT_PARPORT
 	  If you say yes here you get support for a PPS source connected
 	  with the interrupt pin of your parallel port.
 
+config PPS_CLIENT_IRQ
+	tristate "PPS client based on raw IRQs"
+	depends on PPS
+	help
+	  If you say yes here you get support for a PPS source based
+	  on raw IRQs. To be useful, you must also register a platform
+	  device with an IRQ resource, usually in your board setup.
+
 endif
diff --git a/drivers/pps/clients/Makefile b/drivers/pps/clients/Makefile
index 42517da..609a332 100644
--- a/drivers/pps/clients/Makefile
+++ b/drivers/pps/clients/Makefile
@@ -5,6 +5,7 @@
 obj-$(CONFIG_PPS_CLIENT_KTIMER)	+= pps-ktimer.o
 obj-$(CONFIG_PPS_CLIENT_LDISC)	+= pps-ldisc.o
 obj-$(CONFIG_PPS_CLIENT_PARPORT) += pps_parport.o
+obj-$(CONFIG_PPS_CLIENT_IRQ)	+= pps-irq.o
 
 ifeq ($(CONFIG_PPS_DEBUG),y)
 EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/pps/clients/pps-irq.c b/drivers/pps/clients/pps-irq.c
new file mode 100644
index 0000000..33c1bf4
--- /dev/null
+++ b/drivers/pps/clients/pps-irq.c
@@ -0,0 +1,169 @@
+/*
+ * pps-irq.c -- PPS IRQ client
+ *
+ *
+ * Copyright (C) 2010 Ricardo Martins <rasm@fe.up.pt>
+ * Copyright (C) 2011 James Nuss <jamesnuss@nanometrics.ca>
+ *
+ *   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.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/pps_kernel.h>
+#include <linux/gpio.h>
+#include <linux/list.h>
+
+#define PPS_IRQ_NAME            "pps-irq"
+#define PPS_IRQ_VERSION         "1.1.0"
+
+/* Info for each registered platform device */
+struct pps_irq_data {
+	int irq;			/* IRQ used as PPS source */
+	struct pps_device *pps;		/* PPS source device */
+	struct pps_source_info info;	/* PPS source information */
+};
+
+/*
+ * Report the PPS event
+ */
+
+static irqreturn_t pps_irq_irq_handler(int irq, void *data)
+{
+	struct pps_irq_data *info;
+	struct pps_event_time ts;
+
+	/* Get the time stamp first */
+	pps_get_ts(&ts);
+
+	info = (struct pps_irq_data *)data;
+	pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL);
+
+	return IRQ_HANDLED;
+}
+
+static int pps_irq_probe(struct platform_device *pdev)
+{
+	struct pps_irq_data *data;
+	struct resource *res;
+	int irq;
+	int ret;
+
+	/* Validate resource. */
+	if (pdev->num_resources != 1) {
+		pr_err(PPS_IRQ_NAME ": this driver handles only one IRQ resource");
+		return -EINVAL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (res == NULL) {
+		pr_err(PPS_IRQ_NAME ": no IRQ resource was given");
+		return -EINVAL;
+	}
+
+	if (!(res->flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
+		pr_err(PPS_IRQ_NAME ": given IRQ resource must be edge triggered");
+		return -EINVAL;
+	}
+
+	/* Allocate space for device info */
+	data = kzalloc(sizeof(struct pps_irq_data), GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	/* Initialize PPS specific parts of the bookkeeping data structure. */
+	data->info.mode = PPS_CAPTUREASSERT | PPS_OFFSETASSERT |
+			PPS_ECHOASSERT | PPS_CANWAIT | PPS_TSFMT_TSPEC;
+	data->info.owner = THIS_MODULE;
+	snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d",
+		 pdev->name, pdev->id);
+
+	/* Register PPS source. */
+	irq = platform_get_irq(pdev, 0);
+	pr_info(PPS_IRQ_NAME ": registering IRQ %d as PPS source", irq);
+	data->pps = pps_register_source(&data->info, PPS_CAPTUREASSERT
+					| PPS_OFFSETASSERT);
+	if (data->pps == NULL) {
+		kfree(data);
+		pr_err(PPS_IRQ_NAME ": failed to register IRQ %d as PPS source",
+				irq);
+		return -1;
+	}
+
+	/* Request IRQ. */
+	pr_info(PPS_IRQ_NAME ": requesting IRQ %d", irq);
+	ret = request_irq(irq, pps_irq_irq_handler,
+			 res->flags & IRQF_TRIGGER_MASK,
+			 data->info.name, data);
+	if (ret) {
+		pps_unregister_source(data->pps);
+		kfree(data);
+		pr_err(PPS_IRQ_NAME ": failed to acquire IRQ %d\n", irq);
+		return ret;
+	}
+	data->irq = irq;
+
+	platform_set_drvdata(pdev, data);
+	dev_info(data->pps->dev, "Registered IRQ %d as PPS source", data->irq);
+
+	return 0;
+}
+
+static int pps_irq_remove(struct platform_device *pdev)
+{
+	struct pps_irq_data *data = platform_get_drvdata(pdev);
+	platform_set_drvdata(pdev, NULL);
+	free_irq(data->irq, data);
+	pps_unregister_source(data->pps);
+	pr_info(PPS_IRQ_NAME ": removed IRQ %d as PPS source", data->irq);
+	kfree(data);
+	return 0;
+}
+
+static struct platform_driver pps_irq_driver = {
+	.probe		= pps_irq_probe,
+	.remove		=  __devexit_p(pps_irq_remove),
+	.driver		= {
+		.name	= PPS_IRQ_NAME,
+		.owner	= THIS_MODULE
+	},
+};
+
+static int __init pps_irq_init(void)
+{
+	int ret = platform_driver_register(&pps_irq_driver);
+	if (ret < 0)
+		pr_err(PPS_IRQ_NAME ": failed to register platform driver");
+	return ret;
+}
+
+static void __exit pps_irq_exit(void)
+{
+	platform_driver_unregister(&pps_irq_driver);
+	pr_debug(PPS_IRQ_NAME ": unregistered platform driver");
+}
+
+module_init(pps_irq_init);
+module_exit(pps_irq_exit);
+
+MODULE_AUTHOR("Ricardo Martins <rasm@fe.up.pt>");
+MODULE_AUTHOR("James Nuss <jamesnuss@nanometrics.ca>");
+MODULE_DESCRIPTION("Use raw IRQs as PPS source");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(PPS_IRQ_VERSION);
-- 
1.7.1

Re: [PATCH 2/2] pps: new client driver using IRQs

[Rodolfo Giometti]

On Wed, Apr 27, 2011 at 02:14:14PM -0400, James Nuss wrote:
> This client driver allows you to use raw IRQs as a source for PPS
> signals.
> 
> This driver is based on the work by Ricardo Martins <rasm@fe.up.pt> who
> submitted an initial implementation [1] of a PPS IRQ client driver to
> the linuxpps mailing-list on Dec 3 2010.
> 
> [1] http://ml.enneenne.com/pipermail/linuxpps/2010-December/004155.html
> 
> Signed-off-by: James Nuss <jamesnuss@nanometrics.ca>
> Reviewed-by: Ben Gardiner <bengardiner@nanometrics.ca>
> CC: Ricardo Martins <rasm@fe.up.pt>

Acked-by: Rodolfo Giometti <giometti@linux.it>

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[Alexander Gordeev]

[-- Attachment #1: Type: text/plain, Size: 8340 bytes --]

Hi James,

В Wed, 27 Apr 2011 14:14:14 -0400
James Nuss <jamesnuss@nanometrics.ca> пишет:

> This client driver allows you to use raw IRQs as a source for PPS
> signals.
> 
> This driver is based on the work by Ricardo Martins <rasm@fe.up.pt> who
> submitted an initial implementation [1] of a PPS IRQ client driver to
> the linuxpps mailing-list on Dec 3 2010.
> 
> [1] http://ml.enneenne.com/pipermail/linuxpps/2010-December/004155.html
> 
> Signed-off-by: James Nuss <jamesnuss@nanometrics.ca>
> Reviewed-by: Ben Gardiner <bengardiner@nanometrics.ca>
> CC: Ricardo Martins <rasm@fe.up.pt>
> ---
> NOTE: drivers/pps/clients/Kconfig does not have the obligatory statements
> regarding building the drivers as modules, even though they are all tristates.
> This was purposefully omitted from the new config item for consistency.
> This could be addressed in a separate patch.
> 
>  drivers/pps/clients/Kconfig   |    8 ++
>  drivers/pps/clients/Makefile  |    1 +
>  drivers/pps/clients/pps-irq.c |  169 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/pps/clients/pps-irq.c
> 
> diff --git a/drivers/pps/clients/Kconfig b/drivers/pps/clients/Kconfig
> index 8520a7f..94b2cc6 100644
> --- a/drivers/pps/clients/Kconfig
> +++ b/drivers/pps/clients/Kconfig
> @@ -29,4 +29,12 @@ config PPS_CLIENT_PARPORT
>  	  If you say yes here you get support for a PPS source connected
>  	  with the interrupt pin of your parallel port.
>  
> +config PPS_CLIENT_IRQ
> +	tristate "PPS client based on raw IRQs"
> +	depends on PPS
> +	help
> +	  If you say yes here you get support for a PPS source based
> +	  on raw IRQs. To be useful, you must also register a platform
> +	  device with an IRQ resource, usually in your board setup.
> +
>  endif
> diff --git a/drivers/pps/clients/Makefile b/drivers/pps/clients/Makefile
> index 42517da..609a332 100644
> --- a/drivers/pps/clients/Makefile
> +++ b/drivers/pps/clients/Makefile
> @@ -5,6 +5,7 @@
>  obj-$(CONFIG_PPS_CLIENT_KTIMER)	+= pps-ktimer.o
>  obj-$(CONFIG_PPS_CLIENT_LDISC)	+= pps-ldisc.o
>  obj-$(CONFIG_PPS_CLIENT_PARPORT) += pps_parport.o
> +obj-$(CONFIG_PPS_CLIENT_IRQ)	+= pps-irq.o
>  
>  ifeq ($(CONFIG_PPS_DEBUG),y)
>  EXTRA_CFLAGS += -DDEBUG
> diff --git a/drivers/pps/clients/pps-irq.c b/drivers/pps/clients/pps-irq.c
> new file mode 100644
> index 0000000..33c1bf4
> --- /dev/null
> +++ b/drivers/pps/clients/pps-irq.c
> @@ -0,0 +1,169 @@
> +/*
> + * pps-irq.c -- PPS IRQ client
> + *
> + *
> + * Copyright (C) 2010 Ricardo Martins <rasm@fe.up.pt>
> + * Copyright (C) 2011 James Nuss <jamesnuss@nanometrics.ca>
> + *
> + *   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.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program; if not, write to the Free Software
> + *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/pps_kernel.h>
> +#include <linux/gpio.h>
> +#include <linux/list.h>
> +
> +#define PPS_IRQ_NAME            "pps-irq"
> +#define PPS_IRQ_VERSION         "1.1.0"
> +
> +/* Info for each registered platform device */
> +struct pps_irq_data {
> +	int irq;			/* IRQ used as PPS source */
> +	struct pps_device *pps;		/* PPS source device */
> +	struct pps_source_info info;	/* PPS source information */
> +};
> +
> +/*
> + * Report the PPS event
> + */
> +
> +static irqreturn_t pps_irq_irq_handler(int irq, void *data)
> +{
> +	struct pps_irq_data *info;
> +	struct pps_event_time ts;
> +
> +	/* Get the time stamp first */
> +	pps_get_ts(&ts);
> +
> +	info = (struct pps_irq_data *)data;
> +	pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int pps_irq_probe(struct platform_device *pdev)
> +{
> +	struct pps_irq_data *data;
> +	struct resource *res;
> +	int irq;
> +	int ret;
> +
> +	/* Validate resource. */
> +	if (pdev->num_resources != 1) {
> +		pr_err(PPS_IRQ_NAME ": this driver handles only one IRQ resource");

Shouldn't the format string in pr_*/dev_* call be ended with a '\n' or I
missed something?
Also I suggest you what I was once suggested: to define pr_fmt() macro
with a module name as in drivers/pps/clients/pps_parport.c to omit
it's use in every pr_* call. :)

> +		return -EINVAL;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (res == NULL) {
> +		pr_err(PPS_IRQ_NAME ": no IRQ resource was given");
> +		return -EINVAL;
> +	}
> +
> +	if (!(res->flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
> +		pr_err(PPS_IRQ_NAME ": given IRQ resource must be edge triggered");
> +		return -EINVAL;
> +	}

I think it doesn't actually expect that both flags are set because it
always treats it as assert in the irq handler. What does your signal
look like?

> +	/* Allocate space for device info */
> +	data = kzalloc(sizeof(struct pps_irq_data), GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	/* Initialize PPS specific parts of the bookkeeping data structure. */
> +	data->info.mode = PPS_CAPTUREASSERT | PPS_OFFSETASSERT |
> +			PPS_ECHOASSERT | PPS_CANWAIT | PPS_TSFMT_TSPEC;
> +	data->info.owner = THIS_MODULE;
> +	snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d",
> +		 pdev->name, pdev->id);
> +
> +	/* Register PPS source. */
> +	irq = platform_get_irq(pdev, 0);
> +	pr_info(PPS_IRQ_NAME ": registering IRQ %d as PPS source", irq);
> +	data->pps = pps_register_source(&data->info, PPS_CAPTUREASSERT
> +					| PPS_OFFSETASSERT);
> +	if (data->pps == NULL) {
> +		kfree(data);
> +		pr_err(PPS_IRQ_NAME ": failed to register IRQ %d as PPS source",
> +				irq);
> +		return -1;
> +	}
> +
> +	/* Request IRQ. */
> +	pr_info(PPS_IRQ_NAME ": requesting IRQ %d", irq);
> +	ret = request_irq(irq, pps_irq_irq_handler,
> +			 res->flags & IRQF_TRIGGER_MASK,
> +			 data->info.name, data);
> +	if (ret) {
> +		pps_unregister_source(data->pps);
> +		kfree(data);
> +		pr_err(PPS_IRQ_NAME ": failed to acquire IRQ %d\n", irq);
> +		return ret;
> +	}
> +	data->irq = irq;
> +
> +	platform_set_drvdata(pdev, data);
> +	dev_info(data->pps->dev, "Registered IRQ %d as PPS source", data->irq);
> +
> +	return 0;
> +}
> +
> +static int pps_irq_remove(struct platform_device *pdev)
> +{
> +	struct pps_irq_data *data = platform_get_drvdata(pdev);
> +	platform_set_drvdata(pdev, NULL);
> +	free_irq(data->irq, data);
> +	pps_unregister_source(data->pps);
> +	pr_info(PPS_IRQ_NAME ": removed IRQ %d as PPS source", data->irq);
> +	kfree(data);
> +	return 0;
> +}
> +
> +static struct platform_driver pps_irq_driver = {
> +	.probe		= pps_irq_probe,
> +	.remove		=  __devexit_p(pps_irq_remove),
> +	.driver		= {
> +		.name	= PPS_IRQ_NAME,
> +		.owner	= THIS_MODULE
> +	},
> +};
> +
> +static int __init pps_irq_init(void)
> +{
> +	int ret = platform_driver_register(&pps_irq_driver);
> +	if (ret < 0)
> +		pr_err(PPS_IRQ_NAME ": failed to register platform driver");
> +	return ret;
> +}
> +
> +static void __exit pps_irq_exit(void)
> +{
> +	platform_driver_unregister(&pps_irq_driver);
> +	pr_debug(PPS_IRQ_NAME ": unregistered platform driver");
> +}
> +
> +module_init(pps_irq_init);
> +module_exit(pps_irq_exit);
> +
> +MODULE_AUTHOR("Ricardo Martins <rasm@fe.up.pt>");
> +MODULE_AUTHOR("James Nuss <jamesnuss@nanometrics.ca>");
> +MODULE_DESCRIPTION("Use raw IRQs as PPS source");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(PPS_IRQ_VERSION);


-- 
  Alexander

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[James Nuss]

Hi Alexander,

Thanks for reviewing the code.

On Thu, Apr 28, 2011 at 7:22 AM, Alexander Gordeev
<lasaine@lvk.cs.msu.su> wrote:
> Hi James,
>
> В Wed, 27 Apr 2011 14:14:14 -0400
> James Nuss <jamesnuss@nanometrics.ca> пишет:
>
>> This client driver allows you to use raw IRQs as a source for PPS
>> signals.
>>
>> This driver is based on the work by Ricardo Martins <rasm@fe.up.pt> who
>> submitted an initial implementation [1] of a PPS IRQ client driver to
>> the linuxpps mailing-list on Dec 3 2010.
>>
>> [1] http://ml.enneenne.com/pipermail/linuxpps/2010-December/004155.html
>>
>> Signed-off-by: James Nuss <jamesnuss@nanometrics.ca>
>> Reviewed-by: Ben Gardiner <bengardiner@nanometrics.ca>
>> CC: Ricardo Martins <rasm@fe.up.pt>
>> ---
>> NOTE: drivers/pps/clients/Kconfig does not have the obligatory statements
>> regarding building the drivers as modules, even though they are all tristates.
>> This was purposefully omitted from the new config item for consistency.
>> This could be addressed in a separate patch.
>>
>>  drivers/pps/clients/Kconfig   |    8 ++
>>  drivers/pps/clients/Makefile  |    1 +
>>  drivers/pps/clients/pps-irq.c |  169 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 178 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/pps/clients/pps-irq.c
>>
>> diff --git a/drivers/pps/clients/Kconfig b/drivers/pps/clients/Kconfig
>> index 8520a7f..94b2cc6 100644
>> --- a/drivers/pps/clients/Kconfig
>> +++ b/drivers/pps/clients/Kconfig
>> @@ -29,4 +29,12 @@ config PPS_CLIENT_PARPORT
>>         If you say yes here you get support for a PPS source connected
>>         with the interrupt pin of your parallel port.
>>
>> +config PPS_CLIENT_IRQ
>> +     tristate "PPS client based on raw IRQs"
>> +     depends on PPS
>> +     help
>> +       If you say yes here you get support for a PPS source based
>> +       on raw IRQs. To be useful, you must also register a platform
>> +       device with an IRQ resource, usually in your board setup.
>> +
>>  endif
>> diff --git a/drivers/pps/clients/Makefile b/drivers/pps/clients/Makefile
>> index 42517da..609a332 100644
>> --- a/drivers/pps/clients/Makefile
>> +++ b/drivers/pps/clients/Makefile
>> @@ -5,6 +5,7 @@
>>  obj-$(CONFIG_PPS_CLIENT_KTIMER)      += pps-ktimer.o
>>  obj-$(CONFIG_PPS_CLIENT_LDISC)       += pps-ldisc.o
>>  obj-$(CONFIG_PPS_CLIENT_PARPORT) += pps_parport.o
>> +obj-$(CONFIG_PPS_CLIENT_IRQ) += pps-irq.o
>>
>>  ifeq ($(CONFIG_PPS_DEBUG),y)
>>  EXTRA_CFLAGS += -DDEBUG
>> diff --git a/drivers/pps/clients/pps-irq.c b/drivers/pps/clients/pps-irq.c
>> new file mode 100644
>> index 0000000..33c1bf4
>> --- /dev/null
>> +++ b/drivers/pps/clients/pps-irq.c
>> @@ -0,0 +1,169 @@
>> +/*
>> + * pps-irq.c -- PPS IRQ client
>> + *
>> + *
>> + * Copyright (C) 2010 Ricardo Martins <rasm@fe.up.pt>
>> + * Copyright (C) 2011 James Nuss <jamesnuss@nanometrics.ca>
>> + *
>> + *   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.
>> + *
>> + *   This program is distributed in the hope that it will be useful,
>> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *   GNU General Public License for more details.
>> + *
>> + *   You should have received a copy of the GNU General Public License
>> + *   along with this program; if not, write to the Free Software
>> + *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/pps_kernel.h>
>> +#include <linux/gpio.h>
>> +#include <linux/list.h>
>> +
>> +#define PPS_IRQ_NAME            "pps-irq"
>> +#define PPS_IRQ_VERSION         "1.1.0"
>> +
>> +/* Info for each registered platform device */
>> +struct pps_irq_data {
>> +     int irq;                        /* IRQ used as PPS source */
>> +     struct pps_device *pps;         /* PPS source device */
>> +     struct pps_source_info info;    /* PPS source information */
>> +};
>> +
>> +/*
>> + * Report the PPS event
>> + */
>> +
>> +static irqreturn_t pps_irq_irq_handler(int irq, void *data)
>> +{
>> +     struct pps_irq_data *info;
>> +     struct pps_event_time ts;
>> +
>> +     /* Get the time stamp first */
>> +     pps_get_ts(&ts);
>> +
>> +     info = (struct pps_irq_data *)data;
>> +     pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int pps_irq_probe(struct platform_device *pdev)
>> +{
>> +     struct pps_irq_data *data;
>> +     struct resource *res;
>> +     int irq;
>> +     int ret;
>> +
>> +     /* Validate resource. */
>> +     if (pdev->num_resources != 1) {
>> +             pr_err(PPS_IRQ_NAME ": this driver handles only one IRQ resource");
>
> Shouldn't the format string in pr_*/dev_* call be ended with a '\n' or I
> missed something?
> Also I suggest you what I was once suggested: to define pr_fmt() macro
> with a module name as in drivers/pps/clients/pps_parport.c to omit
> it's use in every pr_* call. :)

You are right with the '\n'. My mistake.
Good suggestion on the pr_fmt() macro - that's much cleaner.

>
>> +             return -EINVAL;
>> +     }
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> +     if (res == NULL) {
>> +             pr_err(PPS_IRQ_NAME ": no IRQ resource was given");
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (!(res->flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
>> +             pr_err(PPS_IRQ_NAME ": given IRQ resource must be edge triggered");
>> +             return -EINVAL;
>> +     }
>
> I think it doesn't actually expect that both flags are set because it
> always treats it as assert in the irq handler. What does your signal
> look like?

The conditional logic is that one of either IRQF_TRIGGER_RISING or
IRQF_TRIGGER_FALLING must be set. It doesn't make much sense to have
neither set for PPS signals.
My intention is that the driver is generic enough so you can register
an IRQ resource with either IRQF_TRIGGER_RISING or
IRQF_TRIGGER_FALLING and you will get and assert event for that edge.
Clear events are not generated as you suggest but I believe this is
OK.
My signal is a simple low-to-high transition indicating the PPS. But I
believe you could register a device using this driver referencing the
other edge if required.

>
>> +     /* Allocate space for device info */
>> +     data = kzalloc(sizeof(struct pps_irq_data), GFP_KERNEL);
>> +     if (data == NULL)
>> +             return -ENOMEM;
>> +
>> +     /* Initialize PPS specific parts of the bookkeeping data structure. */
>> +     data->info.mode = PPS_CAPTUREASSERT | PPS_OFFSETASSERT |
>> +                     PPS_ECHOASSERT | PPS_CANWAIT | PPS_TSFMT_TSPEC;
>> +     data->info.owner = THIS_MODULE;
>> +     snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d",
>> +              pdev->name, pdev->id);
>> +
>> +     /* Register PPS source. */
>> +     irq = platform_get_irq(pdev, 0);
>> +     pr_info(PPS_IRQ_NAME ": registering IRQ %d as PPS source", irq);
>> +     data->pps = pps_register_source(&data->info, PPS_CAPTUREASSERT
>> +                                     | PPS_OFFSETASSERT);
>> +     if (data->pps == NULL) {
>> +             kfree(data);
>> +             pr_err(PPS_IRQ_NAME ": failed to register IRQ %d as PPS source",
>> +                             irq);
>> +             return -1;
>> +     }
>> +
>> +     /* Request IRQ. */
>> +     pr_info(PPS_IRQ_NAME ": requesting IRQ %d", irq);
>> +     ret = request_irq(irq, pps_irq_irq_handler,
>> +                      res->flags & IRQF_TRIGGER_MASK,
>> +                      data->info.name, data);
>> +     if (ret) {
>> +             pps_unregister_source(data->pps);
>> +             kfree(data);
>> +             pr_err(PPS_IRQ_NAME ": failed to acquire IRQ %d\n", irq);
>> +             return ret;
>> +     }
>> +     data->irq = irq;
>> +
>> +     platform_set_drvdata(pdev, data);
>> +     dev_info(data->pps->dev, "Registered IRQ %d as PPS source", data->irq);
>> +
>> +     return 0;
>> +}
>> +
>> +static int pps_irq_remove(struct platform_device *pdev)
>> +{
>> +     struct pps_irq_data *data = platform_get_drvdata(pdev);
>> +     platform_set_drvdata(pdev, NULL);
>> +     free_irq(data->irq, data);
>> +     pps_unregister_source(data->pps);
>> +     pr_info(PPS_IRQ_NAME ": removed IRQ %d as PPS source", data->irq);
>> +     kfree(data);
>> +     return 0;
>> +}
>> +
>> +static struct platform_driver pps_irq_driver = {
>> +     .probe          = pps_irq_probe,
>> +     .remove         =  __devexit_p(pps_irq_remove),
>> +     .driver         = {
>> +             .name   = PPS_IRQ_NAME,
>> +             .owner  = THIS_MODULE
>> +     },
>> +};
>> +
>> +static int __init pps_irq_init(void)
>> +{
>> +     int ret = platform_driver_register(&pps_irq_driver);
>> +     if (ret < 0)
>> +             pr_err(PPS_IRQ_NAME ": failed to register platform driver");
>> +     return ret;
>> +}
>> +
>> +static void __exit pps_irq_exit(void)
>> +{
>> +     platform_driver_unregister(&pps_irq_driver);
>> +     pr_debug(PPS_IRQ_NAME ": unregistered platform driver");
>> +}
>> +
>> +module_init(pps_irq_init);
>> +module_exit(pps_irq_exit);
>> +
>> +MODULE_AUTHOR("Ricardo Martins <rasm@fe.up.pt>");
>> +MODULE_AUTHOR("James Nuss <jamesnuss@nanometrics.ca>");
>> +MODULE_DESCRIPTION("Use raw IRQs as PPS source");
>> +MODULE_LICENSE("GPL");
>> +MODULE_VERSION(PPS_IRQ_VERSION);
>
>
> --
>  Alexander
>

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[Alexander Gordeev]

[-- Attachment #1: Type: text/plain, Size: 11444 bytes --]

В Thu, 28 Apr 2011 16:03:59 -0400
James Nuss <jamesnuss@nanometrics.ca> пишет:

> Hi Alexander,
> 
> Thanks for reviewing the code.
> 
> On Thu, Apr 28, 2011 at 7:22 AM, Alexander Gordeev
> <lasaine@lvk.cs.msu.su> wrote:
> > Hi James,
> >
> > В Wed, 27 Apr 2011 14:14:14 -0400
> > James Nuss <jamesnuss@nanometrics.ca> пишет:
> >
> >> This client driver allows you to use raw IRQs as a source for PPS
> >> signals.
> >>
> >> This driver is based on the work by Ricardo Martins <rasm@fe.up.pt> who
> >> submitted an initial implementation [1] of a PPS IRQ client driver to
> >> the linuxpps mailing-list on Dec 3 2010.
> >>
> >> [1] http://ml.enneenne.com/pipermail/linuxpps/2010-December/004155.html
> >>
> >> Signed-off-by: James Nuss <jamesnuss@nanometrics.ca>
> >> Reviewed-by: Ben Gardiner <bengardiner@nanometrics.ca>
> >> CC: Ricardo Martins <rasm@fe.up.pt>
> >> ---
> >> NOTE: drivers/pps/clients/Kconfig does not have the obligatory statements
> >> regarding building the drivers as modules, even though they are all tristates.
> >> This was purposefully omitted from the new config item for consistency.
> >> This could be addressed in a separate patch.
> >>
> >>  drivers/pps/clients/Kconfig   |    8 ++
> >>  drivers/pps/clients/Makefile  |    1 +
> >>  drivers/pps/clients/pps-irq.c |  169 +++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 178 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/pps/clients/pps-irq.c
> >>
> >> diff --git a/drivers/pps/clients/Kconfig b/drivers/pps/clients/Kconfig
> >> index 8520a7f..94b2cc6 100644
> >> --- a/drivers/pps/clients/Kconfig
> >> +++ b/drivers/pps/clients/Kconfig
> >> @@ -29,4 +29,12 @@ config PPS_CLIENT_PARPORT
> >>         If you say yes here you get support for a PPS source connected
> >>         with the interrupt pin of your parallel port.
> >>
> >> +config PPS_CLIENT_IRQ
> >> +     tristate "PPS client based on raw IRQs"
> >> +     depends on PPS
> >> +     help
> >> +       If you say yes here you get support for a PPS source based
> >> +       on raw IRQs. To be useful, you must also register a platform
> >> +       device with an IRQ resource, usually in your board setup.
> >> +
> >>  endif
> >> diff --git a/drivers/pps/clients/Makefile b/drivers/pps/clients/Makefile
> >> index 42517da..609a332 100644
> >> --- a/drivers/pps/clients/Makefile
> >> +++ b/drivers/pps/clients/Makefile
> >> @@ -5,6 +5,7 @@
> >>  obj-$(CONFIG_PPS_CLIENT_KTIMER)      += pps-ktimer.o
> >>  obj-$(CONFIG_PPS_CLIENT_LDISC)       += pps-ldisc.o
> >>  obj-$(CONFIG_PPS_CLIENT_PARPORT) += pps_parport.o
> >> +obj-$(CONFIG_PPS_CLIENT_IRQ) += pps-irq.o
> >>
> >>  ifeq ($(CONFIG_PPS_DEBUG),y)
> >>  EXTRA_CFLAGS += -DDEBUG
> >> diff --git a/drivers/pps/clients/pps-irq.c b/drivers/pps/clients/pps-irq.c
> >> new file mode 100644
> >> index 0000000..33c1bf4
> >> --- /dev/null
> >> +++ b/drivers/pps/clients/pps-irq.c
> >> @@ -0,0 +1,169 @@
> >> +/*
> >> + * pps-irq.c -- PPS IRQ client
> >> + *
> >> + *
> >> + * Copyright (C) 2010 Ricardo Martins <rasm@fe.up.pt>
> >> + * Copyright (C) 2011 James Nuss <jamesnuss@nanometrics.ca>
> >> + *
> >> + *   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.
> >> + *
> >> + *   This program is distributed in the hope that it will be useful,
> >> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + *   GNU General Public License for more details.
> >> + *
> >> + *   You should have received a copy of the GNU General Public License
> >> + *   along with this program; if not, write to the Free Software
> >> + *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> >> + */
> >> +
> >> +#include <linux/init.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/pps_kernel.h>
> >> +#include <linux/gpio.h>
> >> +#include <linux/list.h>
> >> +
> >> +#define PPS_IRQ_NAME            "pps-irq"
> >> +#define PPS_IRQ_VERSION         "1.1.0"
> >> +
> >> +/* Info for each registered platform device */
> >> +struct pps_irq_data {
> >> +     int irq;                        /* IRQ used as PPS source */
> >> +     struct pps_device *pps;         /* PPS source device */
> >> +     struct pps_source_info info;    /* PPS source information */
> >> +};
> >> +
> >> +/*
> >> + * Report the PPS event
> >> + */
> >> +
> >> +static irqreturn_t pps_irq_irq_handler(int irq, void *data)
> >> +{
> >> +     struct pps_irq_data *info;
> >> +     struct pps_event_time ts;
> >> +
> >> +     /* Get the time stamp first */
> >> +     pps_get_ts(&ts);
> >> +
> >> +     info = (struct pps_irq_data *)data;
> >> +     pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL);
> >> +
> >> +     return IRQ_HANDLED;
> >> +}
> >> +
> >> +static int pps_irq_probe(struct platform_device *pdev)
> >> +{
> >> +     struct pps_irq_data *data;
> >> +     struct resource *res;
> >> +     int irq;
> >> +     int ret;
> >> +
> >> +     /* Validate resource. */
> >> +     if (pdev->num_resources != 1) {
> >> +             pr_err(PPS_IRQ_NAME ": this driver handles only one IRQ resource");
> >
> > Shouldn't the format string in pr_*/dev_* call be ended with a '\n' or I
> > missed something?
> > Also I suggest you what I was once suggested: to define pr_fmt() macro
> > with a module name as in drivers/pps/clients/pps_parport.c to omit
> > it's use in every pr_* call. :)
> 
> You are right with the '\n'. My mistake.
> Good suggestion on the pr_fmt() macro - that's much cleaner.

Ok, good.

> >
> >> +             return -EINVAL;
> >> +     }
> >> +
> >> +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> +     if (res == NULL) {
> >> +             pr_err(PPS_IRQ_NAME ": no IRQ resource was given");
> >> +             return -EINVAL;
> >> +     }
> >> +
> >> +     if (!(res->flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
> >> +             pr_err(PPS_IRQ_NAME ": given IRQ resource must be edge triggered");
> >> +             return -EINVAL;
> >> +     }
> >
> > I think it doesn't actually expect that both flags are set because it
> > always treats it as assert in the irq handler. What does your signal
> > look like?
> 
> The conditional logic is that one of either IRQF_TRIGGER_RISING or
> IRQF_TRIGGER_FALLING must be set. It doesn't make much sense to have
> neither set for PPS signals.
> My intention is that the driver is generic enough so you can register
> an IRQ resource with either IRQF_TRIGGER_RISING or
> IRQF_TRIGGER_FALLING and you will get and assert event for that edge.
> Clear events are not generated as you suggest but I believe this is
> OK.
> My signal is a simple low-to-high transition indicating the PPS. But I
> believe you could register a device using this driver referencing the
> other edge if required.

Ok, but is there a way one can register an IRQ resource with both flags
set? If yes, then it would be nice to have a stricter check here to
prevent two situations:
1. none flag is set (it is already in place)
2. both flags are set

The latter will definitely mess things up, right?

> >
> >> +     /* Allocate space for device info */
> >> +     data = kzalloc(sizeof(struct pps_irq_data), GFP_KERNEL);
> >> +     if (data == NULL)
> >> +             return -ENOMEM;
> >> +
> >> +     /* Initialize PPS specific parts of the bookkeeping data structure. */
> >> +     data->info.mode = PPS_CAPTUREASSERT | PPS_OFFSETASSERT |
> >> +                     PPS_ECHOASSERT | PPS_CANWAIT | PPS_TSFMT_TSPEC;
> >> +     data->info.owner = THIS_MODULE;
> >> +     snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d",
> >> +              pdev->name, pdev->id);
> >> +
> >> +     /* Register PPS source. */
> >> +     irq = platform_get_irq(pdev, 0);
> >> +     pr_info(PPS_IRQ_NAME ": registering IRQ %d as PPS source", irq);
> >> +     data->pps = pps_register_source(&data->info, PPS_CAPTUREASSERT
> >> +                                     | PPS_OFFSETASSERT);
> >> +     if (data->pps == NULL) {
> >> +             kfree(data);
> >> +             pr_err(PPS_IRQ_NAME ": failed to register IRQ %d as PPS source",
> >> +                             irq);
> >> +             return -1;
> >> +     }
> >> +
> >> +     /* Request IRQ. */
> >> +     pr_info(PPS_IRQ_NAME ": requesting IRQ %d", irq);
> >> +     ret = request_irq(irq, pps_irq_irq_handler,
> >> +                      res->flags & IRQF_TRIGGER_MASK,
> >> +                      data->info.name, data);
> >> +     if (ret) {
> >> +             pps_unregister_source(data->pps);
> >> +             kfree(data);
> >> +             pr_err(PPS_IRQ_NAME ": failed to acquire IRQ %d\n", irq);
> >> +             return ret;
> >> +     }
> >> +     data->irq = irq;
> >> +
> >> +     platform_set_drvdata(pdev, data);
> >> +     dev_info(data->pps->dev, "Registered IRQ %d as PPS source", data->irq);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int pps_irq_remove(struct platform_device *pdev)
> >> +{
> >> +     struct pps_irq_data *data = platform_get_drvdata(pdev);
> >> +     platform_set_drvdata(pdev, NULL);
> >> +     free_irq(data->irq, data);
> >> +     pps_unregister_source(data->pps);
> >> +     pr_info(PPS_IRQ_NAME ": removed IRQ %d as PPS source", data->irq);
> >> +     kfree(data);
> >> +     return 0;
> >> +}
> >> +
> >> +static struct platform_driver pps_irq_driver = {
> >> +     .probe          = pps_irq_probe,
> >> +     .remove         =  __devexit_p(pps_irq_remove),
> >> +     .driver         = {
> >> +             .name   = PPS_IRQ_NAME,
> >> +             .owner  = THIS_MODULE
> >> +     },
> >> +};
> >> +
> >> +static int __init pps_irq_init(void)
> >> +{
> >> +     int ret = platform_driver_register(&pps_irq_driver);
> >> +     if (ret < 0)
> >> +             pr_err(PPS_IRQ_NAME ": failed to register platform driver");
> >> +     return ret;
> >> +}
> >> +
> >> +static void __exit pps_irq_exit(void)
> >> +{
> >> +     platform_driver_unregister(&pps_irq_driver);
> >> +     pr_debug(PPS_IRQ_NAME ": unregistered platform driver");
> >> +}
> >> +
> >> +module_init(pps_irq_init);
> >> +module_exit(pps_irq_exit);
> >> +
> >> +MODULE_AUTHOR("Ricardo Martins <rasm@fe.up.pt>");
> >> +MODULE_AUTHOR("James Nuss <jamesnuss@nanometrics.ca>");
> >> +MODULE_DESCRIPTION("Use raw IRQs as PPS source");
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_VERSION(PPS_IRQ_VERSION);
> >
> >
> > --
> >  Alexander
> >


-- 
  Alexander

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[Alexander Gordeev]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

В Fri, 29 Apr 2011 00:55:24 +0400
Alexander Gordeev <lasaine@lvk.cs.msu.su> пишет:

> В Thu, 28 Apr 2011 16:03:59 -0400
> James Nuss <jamesnuss@nanometrics.ca> пишет:
> 
> > Hi Alexander,
> > 
> > Thanks for reviewing the code.
> > 
> > On Thu, Apr 28, 2011 at 7:22 AM, Alexander Gordeev
> > <lasaine@lvk.cs.msu.su> wrote:
> > > Hi James,
> > >
> > > В Wed, 27 Apr 2011 14:14:14 -0400
> > > James Nuss <jamesnuss@nanometrics.ca> пишет:
> > >
> > >> +             return -EINVAL;
> > >> +     }
> > >> +
> > >> +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > >> +     if (res == NULL) {
> > >> +             pr_err(PPS_IRQ_NAME ": no IRQ resource was given");
> > >> +             return -EINVAL;
> > >> +     }
> > >> +
> > >> +     if (!(res->flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
> > >> +             pr_err(PPS_IRQ_NAME ": given IRQ resource must be edge triggered");
> > >> +             return -EINVAL;
> > >> +     }
> > >
> > > I think it doesn't actually expect that both flags are set because it
> > > always treats it as assert in the irq handler. What does your signal
> > > look like?
> > 
> > The conditional logic is that one of either IRQF_TRIGGER_RISING or
> > IRQF_TRIGGER_FALLING must be set. It doesn't make much sense to have
> > neither set for PPS signals.
> > My intention is that the driver is generic enough so you can register
> > an IRQ resource with either IRQF_TRIGGER_RISING or
> > IRQF_TRIGGER_FALLING and you will get and assert event for that edge.
> > Clear events are not generated as you suggest but I believe this is
> > OK.
> > My signal is a simple low-to-high transition indicating the PPS. But I
> > believe you could register a device using this driver referencing the
> > other edge if required.
> 
> Ok, but is there a way one can register an IRQ resource with both flags
> set? If yes, then it would be nice to have a stricter check here to
> prevent two situations:
> 1. none flag is set (it is already in place)
> 2. both flags are set
> 
> The latter will definitely mess things up, right?

I mean, one surely can register an IRQ resource with both flags set. And
if the underlying hardware works as it is described (i.e. raises an irq
on both edges) then it will be a problem.

-- 
  Alexander

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[Igor Plyatov]

Hi!

> В Fri, 29 Apr 2011 00:55:24 +0400
> Alexander Gordeev<lasaine@lvk.cs.msu.su>  пишет:
>
>> В Thu, 28 Apr 2011 16:03:59 -0400
>> James Nuss<jamesnuss@nanometrics.ca>  пишет:
>>
>>> Hi Alexander,
>>>
>>> Thanks for reviewing the code.
>>>
>>> On Thu, Apr 28, 2011 at 7:22 AM, Alexander Gordeev
>>> <lasaine@lvk.cs.msu.su>  wrote:
>>>> Hi James,
>>>>
>>>> В Wed, 27 Apr 2011 14:14:14 -0400
>>>> James Nuss<jamesnuss@nanometrics.ca>  пишет:
>>>>
>>>>> +             return -EINVAL;
>>>>> +     }
>>>>> +
>>>>> +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>>> +     if (res == NULL) {
>>>>> +             pr_err(PPS_IRQ_NAME ": no IRQ resource was given");
>>>>> +             return -EINVAL;
>>>>> +     }
>>>>> +
>>>>> +     if (!(res->flags&  (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
>>>>> +             pr_err(PPS_IRQ_NAME ": given IRQ resource must be edge triggered");
>>>>> +             return -EINVAL;
>>>>> +     }
>>>> I think it doesn't actually expect that both flags are set because it
>>>> always treats it as assert in the irq handler. What does your signal
>>>> look like?
>>> The conditional logic is that one of either IRQF_TRIGGER_RISING or
>>> IRQF_TRIGGER_FALLING must be set. It doesn't make much sense to have
>>> neither set for PPS signals.
>>> My intention is that the driver is generic enough so you can register
>>> an IRQ resource with either IRQF_TRIGGER_RISING or
>>> IRQF_TRIGGER_FALLING and you will get and assert event for that edge.
>>> Clear events are not generated as you suggest but I believe this is
>>> OK.
>>> My signal is a simple low-to-high transition indicating the PPS. But I
>>> believe you could register a device using this driver referencing the
>>> other edge if required.
>> Ok, but is there a way one can register an IRQ resource with both flags
>> set? If yes, then it would be nice to have a stricter check here to
>> prevent two situations:
>> 1. none flag is set (it is already in place)
>> 2. both flags are set
>>
>> The latter will definitely mess things up, right?
> I mean, one surely can register an IRQ resource with both flags set. And
> if the underlying hardware works as it is described (i.e. raises an irq
> on both edges) then it will be a problem.

Please don't try to abandon one of ASSERT or CLEAR events!
It is very useful to register both of them, because in this case its 
possible to measure pulse width and decode PPS signals like DCF77.

I write similar driver (pps-client-gpio.c) for Linux-2.6.36 with helper 
drivers (virtual-gps.c and pps-decoders.c) which allows to receive 
GeoSIG Ltd. T1PPS (DCF77 alike) time signal, register both events, then 
decode them and provide a virtual GPS (NMEA time source) device.

These drivers operates very well together with standard NTP server.

>
> _______________________________________________
> LinuxPPS mailing list
> LinuxPPS@ml.enneenne.com
> http://ml.enneenne.com/cgi-bin/mailman/listinfo/linuxpps
> Wiki:http://wiki.enneenne.com/index.php/LinuxPPS_support

Best regards!

-- 
Igor Plyatov

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[Rodolfo Giometti]

On Thu, Apr 28, 2011 at 04:03:59PM -0400, James Nuss wrote:
> >> +
> >> +     if (!(res->flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
> >> +             pr_err(PPS_IRQ_NAME ": given IRQ resource must be edge triggered");
> >> +             return -EINVAL;
> >> +     }
> >
> > I think it doesn't actually expect that both flags are set because it
> > always treats it as assert in the irq handler. What does your signal
> > look like?
> 
> The conditional logic is that one of either IRQF_TRIGGER_RISING or
> IRQF_TRIGGER_FALLING must be set. It doesn't make much sense to have
> neither set for PPS signals.
> My intention is that the driver is generic enough so you can register
> an IRQ resource with either IRQF_TRIGGER_RISING or
> IRQF_TRIGGER_FALLING and you will get and assert event for that edge.
> Clear events are not generated as you suggest but I believe this is
> OK.
> My signal is a simple low-to-high transition indicating the PPS. But I
> believe you could register a device using this driver referencing the
> other edge if required.

The driver is ok for me but if you say that your «intention is that
the driver is generic enough» you should consider adding CLEAR events
since some GPIOs controller can manager IRQs for both edges... ;)

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[Rodolfo Giometti]

On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:

>>> The latter will definitely mess things up, right?
>> I mean, one surely can register an IRQ resource with both flags set. And
>> if the underlying hardware works as it is described (i.e. raises an irq
>> on both edges) then it will be a problem.
>
> Please don't try to abandon one of ASSERT or CLEAR events!
> It is very useful to register both of them, because in this case its  
> possible to measure pulse width and decode PPS signals like DCF77.

At this point I suppose we should add both ASSERT and CLEAR events...

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[James Nuss]

Thanks for you comments Igor, Alexander and Rodolfo.

You all make a good point that it would be useful to also register a
CLEAR event. Before I go ahead and implement any changes I would like
clarify a couple of things.

My original patch which only included the ASSERT event was simple
because all you needed to do was register an IRQ resource with a
particular edge trigger and you would get an ASSERT event on that
edge. It becomes a little more complicated when you want to capture
both ASSERT and CLEAR events because you also have to indicate to the
driver which edge is the ASSERT and which is the CLEAR. This is
because I presume you could have systems that generate a pulse which
transitions from low->high->low OR a pulse which transitions from
high->low->high, and we would want a driver which is generic enough to
handle both cases. Is my understanding correct that an ASSERT event is
simply the first edge of a single pulse, regardless of "polarity"?

I suppose the driver could be intelligent enough to figure out which
is the first edge on the assumption that the pulse width is always
very small - at least less than 0.5 second, more likely less than a
few hundred milliseconds. Obviously it would need at least 2 pulses
before it decides to register either an ASSERT or CLEAR event. Is it
safe enough to say that an ASSERT event will only be generated if the
previous edge was greater than 0.5 second ago, otherwise a CLEAR event
will be generated?

If I was to implement the driver this way then you would have exactly
3 ways to register a device to use this driver:

1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
This will generate an ASSERT event on rising edges (no CLEAR events)

2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
This will generate an ASSERT event on falling edges (no CLEAR events)

3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
IORESOURCE_IRQ_HIGHEDGE set:
This will generate ASSERT and CLEAR events with the driver dynamically
determining which edge is the ASSERT based on the logic above.

I hope this covers all the potential use cases.

cheers,
James


On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti <giometti@enneenne.com> wrote:
> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>
>>>> The latter will definitely mess things up, right?
>>> I mean, one surely can register an IRQ resource with both flags set. And
>>> if the underlying hardware works as it is described (i.e. raises an irq
>>> on both edges) then it will be a problem.
>>
>> Please don't try to abandon one of ASSERT or CLEAR events!
>> It is very useful to register both of them, because in this case its
>> possible to measure pulse width and decode PPS signals like DCF77.
>
> At this point I suppose we should add both ASSERT and CLEAR events...
>
> Ciao,
>
> Rodolfo
>
> --
>
> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
> Linux Device Driver                          giometti@linux.it
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming                     skype:  rodolfo.giometti
> Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[Igor Plyatov]

Hi James!

> Thanks for you comments Igor, Alexander and Rodolfo.
>
> You all make a good point that it would be useful to also register a
> CLEAR event. Before I go ahead and implement any changes I would like
> clarify a couple of things.
>
> My original patch which only included the ASSERT event was simple
> because all you needed to do was register an IRQ resource with a
> particular edge trigger and you would get an ASSERT event on that
> edge. It becomes a little more complicated when you want to capture
> both ASSERT and CLEAR events because you also have to indicate to the
> driver which edge is the ASSERT and which is the CLEAR. This is
> because I presume you could have systems that generate a pulse which
> transitions from low->high->low OR a pulse which transitions from
> high->low->high, and we would want a driver which is generic enough to
> handle both cases. Is my understanding correct that an ASSERT event is
> simply the first edge of a single pulse, regardless of "polarity"?
>
> I suppose the driver could be intelligent enough to figure out which
> is the first edge on the assumption that the pulse width is always
> very small - at least less than 0.5 second, more likely less than a
> few hundred milliseconds. Obviously it would need at least 2 pulses
> before it decides to register either an ASSERT or CLEAR event. Is it
> safe enough to say that an ASSERT event will only be generated if the
> previous edge was greater than 0.5 second ago, otherwise a CLEAR event
> will be generated?

This way can lead to problems when developer tries to debug PPS 
subsystem with some specific signal parameters.
In my opinion it much safer to explicitly declare ".active_low = 0" or 
".active_low = 1" in the platform initialization code.
See example:

/*
  * pps client gpio
  */
static struct pps_client_gpio pps_client_gpios[] = {
     {
         .gpio            = GPI_PPS0_IN,
         .active_low        = 0, /* ASSERT is a _/ */
         .desc            = "pps0 source",
         .decoder_type        = PPS_DECODER_GEOSIG_T1PPS,
     },
     {
         .gpio            = GPI_PPS1_IN,
         .active_low        = 1, /* ASSERT is a \_ */
         .desc            = "pps1 source",
     }
};

static struct pps_client_gpio_platform_data pps_client_gpios_data = {
     .pclient    = pps_client_gpios,
     .num_gpios    = ARRAY_SIZE(pps_client_gpios),
};

static struct platform_device pps_client_gpio_device = {
     .name        = "pps-client-gpio",
     .id        = 0,
     .dev        = {
         .platform_data    = &pps_client_gpios_data,
     }
};


> If I was to implement the driver this way then you would have exactly
> 3 ways to register a device to use this driver:
>
> 1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
> This will generate an ASSERT event on rising edges (no CLEAR events)
>
> 2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
> This will generate an ASSERT event on falling edges (no CLEAR events)
>
> 3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
> IORESOURCE_IRQ_HIGHEDGE set:
> This will generate ASSERT and CLEAR events with the driver dynamically
> determining which edge is the ASSERT based on the logic above.
>
> I hope this covers all the potential use cases.
>
> cheers,
> James
>
>
> On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti<giometti@enneenne.com>  wrote:
>> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>>
>>>>> The latter will definitely mess things up, right?
>>>> I mean, one surely can register an IRQ resource with both flags set. And
>>>> if the underlying hardware works as it is described (i.e. raises an irq
>>>> on both edges) then it will be a problem.
>>> Please don't try to abandon one of ASSERT or CLEAR events!
>>> It is very useful to register both of them, because in this case its
>>> possible to measure pulse width and decode PPS signals like DCF77.
>> At this point I suppose we should add both ASSERT and CLEAR events...
>>
>> Ciao,
>>
>> Rodolfo
>>
>> --
>>
>> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
>> Linux Device Driver                          giometti@linux.it
>> Embedded Systems                     phone:  +39 349 2432127
>> UNIX programming                     skype:  rodolfo.giometti
>> Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>

Best regards!

-- 
Igor Plyatov

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[James Nuss]

Hi Igor,

I agree, platform_data is a good candidate for this type of parameter.
Where is the appropriate place to keep the header file containing the
struct declaration(s)? e.g in your case "struct
pps_client_gpio_platform_data" and "struct pps_client_gpio" Presumably
it needs to be in a readily accessible header file so the board setup
code can use it.

cheers,
James

>
> This way can lead to problems when developer tries to debug PPS subsystem
> with some specific signal parameters.
> In my opinion it much safer to explicitly declare ".active_low = 0" or
> ".active_low = 1" in the platform initialization code.
> See example:
>
> /*
>  * pps client gpio
>  */
> static struct pps_client_gpio pps_client_gpios[] = {
>    {
>        .gpio            = GPI_PPS0_IN,
>        .active_low        = 0, /* ASSERT is a _/ */
>        .desc            = "pps0 source",
>        .decoder_type        = PPS_DECODER_GEOSIG_T1PPS,
>    },
>    {
>        .gpio            = GPI_PPS1_IN,
>        .active_low        = 1, /* ASSERT is a \_ */
>        .desc            = "pps1 source",
>    }
> };
>
> static struct pps_client_gpio_platform_data pps_client_gpios_data = {
>    .pclient    = pps_client_gpios,
>    .num_gpios    = ARRAY_SIZE(pps_client_gpios),
> };
>
> static struct platform_device pps_client_gpio_device = {
>    .name        = "pps-client-gpio",
>    .id        = 0,
>    .dev        = {
>        .platform_data    = &pps_client_gpios_data,
>    }
> };
>
>
>> If I was to implement the driver this way then you would have exactly
>> 3 ways to register a device to use this driver:
>>
>> 1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
>> This will generate an ASSERT event on rising edges (no CLEAR events)
>>
>> 2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
>> This will generate an ASSERT event on falling edges (no CLEAR events)
>>
>> 3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
>> IORESOURCE_IRQ_HIGHEDGE set:
>> This will generate ASSERT and CLEAR events with the driver dynamically
>> determining which edge is the ASSERT based on the logic above.
>>
>> I hope this covers all the potential use cases.
>>
>> cheers,
>> James
>>
>>
>> On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti<giometti@enneenne.com>
>>  wrote:
>>>
>>> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>>>
>>>>>> The latter will definitely mess things up, right?
>>>>>
>>>>> I mean, one surely can register an IRQ resource with both flags set.
>>>>> And
>>>>> if the underlying hardware works as it is described (i.e. raises an irq
>>>>> on both edges) then it will be a problem.
>>>>
>>>> Please don't try to abandon one of ASSERT or CLEAR events!
>>>> It is very useful to register both of them, because in this case its
>>>> possible to measure pulse width and decode PPS signals like DCF77.
>>>
>>> At this point I suppose we should add both ASSERT and CLEAR events...
>>>
>>> Ciao,
>>>
>>> Rodolfo
>>>
>>> --
>>>
>>> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
>>> Linux Device Driver                          giometti@linux.it
>>> Embedded Systems                     phone:  +39 349 2432127
>>> UNIX programming                     skype:  rodolfo.giometti
>>> Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>> in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
>
> Best regards!
>
> --
> Igor Plyatov
>
>

Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

[Igor Plyatov]

Hi James!

> Hi Igor,
>
> I agree, platform_data is a good candidate for this type of parameter.
> Where is the appropriate place to keep the header file containing the
> struct declaration(s)? e.g in your case "struct
> pps_client_gpio_platform_data" and "struct pps_client_gpio" Presumably
> it needs to be in a readily accessible header file so the board setup
> code can use it.

This header file must be "include/linux/pps-client-gpio.h".
Here is its content:

/*
  * pps-client-gpio.h -- PPS client for IRQ capable GPIO
  *
  *
  * Copyright (C) 2011 Igor Plyatov <plyatov@gmail.com>
  *
  *   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.
  *
  *   This program is distributed in the hope that it will be useful,
  *   but WITHOUT ANY WARRANTY; without even the implied warranty of
  *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  *   GNU General Public License for more details.
  *
  *   You should have received a copy of the GNU General Public License
  *   along with this program; if not, write to the Free Software
  *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */

#ifndef _PPS_CLIENT_GPIO_H
#define _PPS_CLIENT_GPIO_H

struct pps_client_gpio {
     /* Configuration parameters */
     unsigned gpio;
     unsigned active_low;
     const char *desc;
     unsigned char decoder_type;
};

struct pps_client_gpio_platform_data {
     struct pps_client_gpio *pclient;
     int num_gpios;
};

#endif

> cheers,
> James
>
>> This way can lead to problems when developer tries to debug PPS subsystem
>> with some specific signal parameters.
>> In my opinion it much safer to explicitly declare ".active_low = 0" or
>> ".active_low = 1" in the platform initialization code.
>> See example:
>>
>> /*
>>   * pps client gpio
>>   */
>> static struct pps_client_gpio pps_client_gpios[] = {
>>     {
>>         .gpio            = GPI_PPS0_IN,
>>         .active_low        = 0, /* ASSERT is a _/ */
>>         .desc            = "pps0 source",
>>         .decoder_type        = PPS_DECODER_GEOSIG_T1PPS,
>>     },
>>     {
>>         .gpio            = GPI_PPS1_IN,
>>         .active_low        = 1, /* ASSERT is a \_ */
>>         .desc            = "pps1 source",
>>     }
>> };
>>
>> static struct pps_client_gpio_platform_data pps_client_gpios_data = {
>>     .pclient    = pps_client_gpios,
>>     .num_gpios    = ARRAY_SIZE(pps_client_gpios),
>> };
>>
>> static struct platform_device pps_client_gpio_device = {
>>     .name        = "pps-client-gpio",
>>     .id        = 0,
>>     .dev        = {
>>         .platform_data    =&pps_client_gpios_data,
>>     }
>> };
>>
>>
>>> If I was to implement the driver this way then you would have exactly
>>> 3 ways to register a device to use this driver:
>>>
>>> 1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
>>> This will generate an ASSERT event on rising edges (no CLEAR events)
>>>
>>> 2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
>>> This will generate an ASSERT event on falling edges (no CLEAR events)
>>>
>>> 3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
>>> IORESOURCE_IRQ_HIGHEDGE set:
>>> This will generate ASSERT and CLEAR events with the driver dynamically
>>> determining which edge is the ASSERT based on the logic above.
>>>
>>> I hope this covers all the potential use cases.
>>>
>>> cheers,
>>> James
>>>
>>>
>>> On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti<giometti@enneenne.com>
>>>   wrote:
>>>> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>>>>
>>>>>>> The latter will definitely mess things up, right?
>>>>>> I mean, one surely can register an IRQ resource with both flags set.
>>>>>> And
>>>>>> if the underlying hardware works as it is described (i.e. raises an irq
>>>>>> on both edges) then it will be a problem.
>>>>> Please don't try to abandon one of ASSERT or CLEAR events!
>>>>> It is very useful to register both of them, because in this case its
>>>>> possible to measure pulse width and decode PPS signals like DCF77.
>>>> At this point I suppose we should add both ASSERT and CLEAR events...
>>>>
>>>> Ciao,
>>>>
>>>> Rodolfo
>>>>
>>>> --
>>>>
>>>> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
>>>> Linux Device Driver                          giometti@linux.it
>>>> Embedded Systems                     phone:  +39 349 2432127
>>>> UNIX programming                     skype:  rodolfo.giometti
>>>> Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>>> in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>>
>> Best regards!
>>
>> --
>> Igor Plyatov
>>
>>

Best regards!
-- 

Igor Plyatov