[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20090108162826.GA26413@suse.de>
Date: Thu, 8 Jan 2009 08:28:26 -0800
From: Greg KH <gregkh@...e.de>
To: Davide Rizzo <elpa.rizzo@...il.com>
Cc: linux-kernel@...r.kernel.org, hjk@...utronix.de,
ben-linux@...ff.org
Subject: Re: [PATCH 1/2] Driver for user access to internal timer
On Thu, Jan 08, 2009 at 11:05:30AM +0100, Davide Rizzo wrote:
> Allows user programs to manage internal timers, through virtual files
> frequency and duty.
If you are adding userspace apis, please also add the proper
documentation to Documentation/ABI/ as well.
> If pwm_one_shot is implemented, it allows also waiting for one-shot period.
How does userspace know if it is implemented or not?
> +++ linux-2.6.28.elpa/drivers/uio/Makefile 2009-01-06 18:20:17.000000000 +0100
> @@ -1,4 +1,5@@
> obj-$(CONFIG_UIO) += uio.o
> +obj-$(CONFIG_UIO_TIMER) += timer.o
timer.ko is a pretty generic name for a module, how about changing it to
something a bit more unique and descriptive?
> --- linux-2.6.28/drivers/uio/timer.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.28.elpa/drivers/uio/timer.c 2009-01-07 19:02:25.000000000 +0100
> @@ -0,0 +1,302 @@
> +/*
> + drivers/misc/timer.c
> +
> + Written Jan 2009 by Davide Rizzo <elpa-rizzo@...il.com>
> +
> + This driver allows user to access internal timers
> + You can use any timer to generate a fixed frequency signal, or to wait for a
> + defined short time (if hardware can do it).
> + To generate a PWM signal, you have to write the frequency value in Hz into the
> + virtual file "freq". After that you can program the duty cycle in the virtual
> + file "duty", in 1/1000 units (500 = 50%).
> + To wait for a time, you have to write the number of microseconds in virtual
> + file "wait". Reading from same file will complete when timer will expire.
Where are these files located at?
And can't you do this already with the existing kernel/user interfaces
for time? What is different here from them? The PWM signal? Or
something else?
> + 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.
Do you really mean "any later version"?
> +#ifdef CONFIG_UIO_TIMER_ONESHOT
Please don't put #ifdefs in c files if at all possible.
> +#define NS_IN_HZ (1000000000UL)
Don't we already have this define in some .h file somewhere?
> +#ifdef CONFIG_UIO_TIMER_ONESHOT
> + init_waitqueue_head(&info->wq);
> + info->flag = 1;
> + err = device_create_file(&pdev->dev, &dev_attr_wait);
<snip>
You can do all the creation and deletion of files using a group of
attributes, which would make your code much simpler.
> +MODULE_SUPPORTED_DEVICE("timers");
That sounds pretty generic :(
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists