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

From: James Nuss
Date: Thu Apr 28 2011 - 16:04:07 EST

Next message: Grant Likely: "[PATCH 4/6] dt: generalize irq_of_create_mapping()"
Previous message: Grant Likely: "[PATCH 6/6] powerpc: use irq_alloc_desc() to manage irq allocations"
In reply to: Alexander Gordeev: "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 ]

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.

>
>> + Â Â Â Â Â Â 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@xxxxxxxx>");
>> +MODULE_AUTHOR("James Nuss <jamesnuss@xxxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Use raw IRQs as PPS source");
>> +MODULE_LICENSE("GPL");
>> +MODULE_VERSION(PPS_IRQ_VERSION);
>
>
> --
> ÂAlexander
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Grant Likely: "[PATCH 4/6] dt: generalize irq_of_create_mapping()"
Previous message: Grant Likely: "[PATCH 6/6] powerpc: use irq_alloc_desc() to manage irq allocations"
In reply to: Alexander Gordeev: "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 ]