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

From: Alexander Gordeev
Date: Thu Apr 28 2011 - 16:55:43 EST

Next message: john stultz: "Re: [PATCH] clocksource: sh_tmu: __clocksource_updatefreq_hz()update"
Previous message: john stultz: "Re: [PATCH] clocksource: sh_cmt: Runtime PM support"
In reply to: James Nuss: "Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs"
Next in thread: Alexander Gordeev: "Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Ð Thu, 28 Apr 2011 16:03:59 -0400
James Nuss <jamesnuss@xxxxxxxxxxxxxx> ÐÐÑÐÑ:

> Hi Alexander,
>
> Thanks for reviewing the code.
>
> On Thu, Apr 28, 2011 at 7:22 AM, Alexander Gordeev
> <lasaine@xxxxxxxxxxxxx> wrote:
> > Hi James,
> >
> > Ð Wed, 27 Apr 2011 14:14:14 -0400
> > James Nuss <jamesnuss@xxxxxxxxxxxxxx> ÐÐÑÐÑ:
> >
> >> 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@xxxxxxxx> 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@xxxxxxxxxxxxxx>
> >> Reviewed-by: Ben Gardiner <bengardiner@xxxxxxxxxxxxxx>
> >> CC: Ricardo Martins <rasm@xxxxxxxx>
> >> ---
> >> 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@xxxxxxxx>
> >> + * Copyright (C) 2011 James Nuss <jamesnuss@xxxxxxxxxxxxxx>
> >> + *
> >> + * Â 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@xxxxxxxx>");
> >> +MODULE_AUTHOR("James Nuss <jamesnuss@xxxxxxxxxxxxxx>");
> >> +MODULE_DESCRIPTION("Use raw IRQs as PPS source");
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_VERSION(PPS_IRQ_VERSION);
> >
> >
> > --
> > ÂAlexander
> >

--
Alexander

Attachment: signature.asc
Description: PGP signature

Next message: john stultz: "Re: [PATCH] clocksource: sh_tmu: __clocksource_updatefreq_hz()update"
Previous message: john stultz: "Re: [PATCH] clocksource: sh_cmt: Runtime PM support"
In reply to: James Nuss: "Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs"
Next in thread: Alexander Gordeev: "Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]