[LinuxPPS] [PATCH]
Rodolfo Giometti
giometti at enneenne.com
Fri Feb 1 11:45:13 CET 2008
On Fri, Feb 01, 2008 at 02:00:24AM +0000, Cirilo Bernardo wrote:
> I am attaching a patch to add support for PPS interrupts on an ISA
> bus. This may be useful for some embedded boards.
Thanks! :)
> I hope attachments are OK - since I only have this 'gmail' account I
> have no idea if cut/paste will mangle all the formatting.
Please, use git-format-patch to format a patch for submission. Add a
"title", a "comment" (if needed) and the Signed-off-by line.
Now some comments on your patch.
> diff --git a/drivers/pps/Kconfig b/drivers/pps/Kconfig
> index bfe6621..3b6dddc 100644
> --- a/drivers/pps/Kconfig
> +++ b/drivers/pps/Kconfig
> @@ -29,6 +29,14 @@ config PPS_DEBUG
> messages to the system log. Select this if you are having a
> problem with PPS support and want to see more of what is going on.
>
> +config PPS_ISA
Please, use LinuxPPS stype: PPS_CLIENT_ISA
> + bool "PPS support on ISA IRQs"
See below at [1].
> + depends on PPS
> + help
> + Say Y here if you have a PPS source which is tied directly to
> + an interrupt line. For the WinSystems PPM-GPS, choose N and
> + select the PPS_ISAGPS option.
What is PPS_ISAGPS? Please, no board specific code for generic
drivers.
Also you should put this driver into proper directory pps/clients/.
> +
> source drivers/pps/clients/Kconfig
>
> endmenu
> diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
> index d8ec308..e70c44f 100644
> --- a/drivers/pps/Makefile
> +++ b/drivers/pps/Makefile
> @@ -9,3 +9,8 @@ obj-y += clients/
> ifeq ($(CONFIG_PPS_DEBUG),y)
> EXTRA_CFLAGS += -DDEBUG
> endif
> +
> +ifeq ($(CONFIG_PPS_ISA),y)
> +obj-m += pps_isa.o
> +endif
Why??? =:-o
[1] Just use "tristate" instaed of "bool" into Kconfig and use the
line:
obj-$(CONFIG_PPS_CLIENT_ISA) += pps_isa.o
into pps/clients/Makefile.
> +
> diff --git a/drivers/pps/pps_isa.c b/drivers/pps/pps_isa.c
> new file mode 100644
> index 0000000..4a852c5
> --- /dev/null
> +++ b/drivers/pps/pps_isa.c
> @@ -0,0 +1,126 @@
> +/*
> + * pps_isa.c -- Driver for generic ISA Pulse Per Second signal
> + *
> + * Copyright (C) 2007
> + * Cirilo Bernardo <cirilo.bernardo at gmail.com>
> + * CanSyd Australia Pty. Ltd. <http://www.cansyd.com.au>
> + * CO2CRC <http://www.co2crc.com.au>
> + *
> + * 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.
Please, use __only__ GNU General Public License version 2 as the whole
kernel code does.
> + *
> + * 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.
> + *
> + */
> +
> +/*
> + * Caveat: Some devices such as the Trimble LassenIQ GPS receiver
> + * produce an undisciplined PPS signal when there is no satellite
> + * lock. When a satellite lock is obtained, the PPS may 'jump'.
> + * If the satellite lock is lost for a long period, the PPS will
> + * wander and may jump again when a satellite lock is obtained.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
Already included by linux/module.h.
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/pps.h>
> +
> +#define PFX "pps_isa: "
Just use DEVNAME or, better, use dev_(info|err) functions.
> +#define NDEVS 1
> +#define DEVNAME "pps_isa" /* Device name to pass to various routines */
> +
> +static unsigned int irq = 0;
Set it to -1 and allow 0 as valid IRQ number.
> +MODULE_PARM_DESC(irq, "Interrupt number for PPS signal");
> +module_param(irq, uint, S_IRUGO);
> +
> +static dev_t dev_id;
> +static int pps_source;
Mmm... you sholdn't use static variables at all...
> +
> +static struct pps_source_info pps_isa_info = {
> + .name = DEVNAME,
> + .path = "",
> + .mode = PPS_CAPTUREASSERT|PPS_TSFMT_TSPEC,
Use "PPS_CAPTUREASSERT | PPS_TSFMT_TSPEC".
No PPS_OFFSETASSERT? No PPS_CANWAIT?
> + .echo = NULL,
Not needed.
> + .owner = THIS_MODULE,
> + .dev = NULL,
Ditto.
> +};
> +
> +
> +static irqreturn_t pps_interrupt(int irq, void *dev_id)
> +{
> + pps_event(pps_source, PPS_CAPTUREASSERT, NULL);
Use dev_id to get pps_source data.
> + return IRQ_HANDLED;
> +}
> +
> +
> +static int __init test_init(void)
> +{
> + int ret;
> +
> + if (!irq) {
Check for "irq < 0".
> + printk( KERN_ERR PFX "Invalid IRQ\n");
> + return -EINVAL;
> + }
> +
> + /* Reserve a block of minor numbers */
> + ret = alloc_chrdev_region(&dev_id, 0, NDEVS, DEVNAME);
> + if (ret) {
> + printk( KERN_ERR PFX "Cannot allocate a device block\n");
> + return ret;
> + }
Why????? =:-o
You don't need a chardev at all.
> +
> + /* Register with LinuxPPS as a PPS source */
> + ret = pps_register_source(&pps_isa_info, PPS_CAPTUREASSERT);
No PPS_OFFSETASSERT?
> + if (ret < 0) {
> + printk( KERN_ERR PFX "Cannot register PPS source\n");
> + goto free_chrdev;
> + }
> + pps_source = ret;
> +
> + /* Set the PPS interrupt handler */
> + ret = request_irq(irq, (irq_handler_t) pps_interrupt, 0, DEVNAME, &dev_id);
> + if (ret) {
> + printk( KERN_ERR PFX "Cannot obtain PPS IRQ (%d)\n", irq);
> + goto free_pps;
> + }
> +
> + return 0;
> +
> +free_pps:
> + pps_unregister_source(pps_source);
> +free_chrdev:
> + unregister_chrdev_region(dev_id, NDEVS);
> +
> + return ret;
> +}
> +
> +
> +static void __exit test_exit(void)
> +{
> + pps_unregister_source(pps_source);
> +
> + free_irq(irq, &dev_id);
> + unregister_chrdev_region(dev_id, NDEVS);
> + return;
> +}
> +
> +
> +module_init(test_init);
> +module_exit(test_exit);
I think is better using isa_register_driver() & Co. to manage ISA
devices. Please, refere to linux/drivers/i2c/busses/i2c-pca-isa.c
> +
> +MODULE_AUTHOR("Cirilo Bernardo <cirilo.bernardo at gmail.com>");
> +MODULE_DESCRIPTION("PPS support for ISA interrupts");
> +MODULE_LICENSE("GPL");
Also let me suggest you to use checkpatch.pl utility. See my output:
giometti at zaigor:~/Projects/GIT_ARCHIVES/linux$ scripts/checkpatch.pl /tmp/patch_pps_isa
ERROR: do not initialise statics to 0 or NULL
#83: FILE: drivers/pps/pps_isa.c:45:
+static unsigned int irq = 0;
ERROR: no space after that open parenthesis '('
#112: FILE: drivers/pps/pps_isa.c:74:
+ printk( KERN_ERR PFX "Invalid IRQ\n");
ERROR: no space after that open parenthesis '('
#119: FILE: drivers/pps/pps_isa.c:81:
+ printk( KERN_ERR PFX "Cannot allocate a device block\n");
ERROR: no space after that open parenthesis '('
#126: FILE: drivers/pps/pps_isa.c:88:
+ printk( KERN_ERR PFX "Cannot register PPS source\n");
WARNING: line over 80 characters
#132: FILE: drivers/pps/pps_isa.c:94:
+ ret = request_irq(irq, (irq_handler_t) pps_interrupt, 0, DEVNAME, &dev_id);
ERROR: no space after that open parenthesis '('
#134: FILE: drivers/pps/pps_isa.c:96:
+ printk( KERN_ERR PFX "Cannot obtain PPS IRQ (%d)\n", irq);
ERROR: Missing Signed-off-by: line(s)
total: 6 errors, 1 warnings, 148 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
You should fix all these errors. :)
I'm sorry for such paranoia comments but otherwise we have no chances
to get kernel inclusion. :)
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti at enneenne.com
Linux Device Driver giometti at gnudd.com
Embedded Systems giometti at linux.it
UNIX programming phone: +39 349 2432127
More information about the LinuxPPS
mailing list