I was planning to use the UIO layer for a driver I was working on and
liked the aproach a lot.
Eventually I didn't use it because there were registers which needed to
know the physical address of a shared memory area which is not easily
doable with that framework.
However, my way I stumbled over two small things which I would still
like to share. One is adds the support for device clocks, the other one
is a nit-picking cleanup.
Daniel
[PATCH 1/2] UIO: add device clock support
[PATCH 2/2] UIO: remove 'default n' from Kconfig
'default n' is the default, there is no need for these lines.
Signed-off-by: Daniel Mack <[email protected]>
Cc: Hans J. Koch <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/uio/Kconfig | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 7f86534..45200fd 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -1,7 +1,6 @@
menuconfig UIO
tristate "Userspace I/O drivers"
depends on !S390
- default n
help
Enable this to allow the userspace driver core code to be
built. This code allows userspace programs easy access to
@@ -16,7 +15,6 @@ if UIO
config UIO_CIF
tristate "generic Hilscher CIF Card driver"
depends on PCI
- default n
help
Driver for Hilscher CIF DeviceNet and Profibus cards. This
driver requires a userspace component that handles all of the
@@ -48,7 +46,6 @@ config UIO_PDRV_GENIRQ
config UIO_SMX
tristate "SMX cryptengine UIO interface"
- default n
help
Userspace IO interface to the Cryptography engine found on the
Nias Digital SMX boards. These will be available from Q4 2008
@@ -61,7 +58,6 @@ config UIO_SMX
config UIO_AEC
tristate "AEC video timestamp device"
depends on PCI
- default n
help
UIO driver for the Adrienne Electronics Corporation PCI time
@@ -78,7 +74,6 @@ config UIO_AEC
config UIO_SERCOS3
tristate "Automata Sercos III PCI card driver"
- default n
help
Userspace I/O interface for the Sercos III PCI card from
Automata GmbH. The userspace part of this driver will be
--
1.6.3.1
Add a pointer to a 'struct clk' to uio_info. Drivers can set
this pointer if a clock is needed, and the UIO core will care
to enable and disable it upon device open and release.
Signed-off-by: Daniel Mack <[email protected]>
Cc: Hans J. Koch <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/uio/uio.c | 15 ++++++++++++++-
include/linux/uio_driver.h | 2 ++
2 files changed, 16 insertions(+), 1 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 03efb06..6ba95cf 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -479,6 +479,12 @@ static int uio_open(struct inode *inode, struct file *filep)
listener->event_count = atomic_read(&idev->event);
filep->private_data = listener;
+ if (idev->info->clk) {
+ ret = clk_enable(idev->info->clk);
+ if (ret)
+ goto err_clkenable;
+ }
+
if (idev->info->open) {
ret = idev->info->open(idev->info, inode);
if (ret)
@@ -486,6 +492,7 @@ static int uio_open(struct inode *inode, struct file *filep)
}
return 0;
+err_clkenable:
err_infoopen:
kfree(listener);
@@ -510,8 +517,14 @@ static int uio_release(struct inode *inode, struct file *filep)
struct uio_listener *listener = filep->private_data;
struct uio_device *idev = listener->dev;
- if (idev->info->release)
+ if (idev->info->release) {
ret = idev->info->release(idev->info, inode);
+ if (ret)
+ dev_err(idev->dev, "release() failed (%d)\n", ret);
+ }
+
+ if (idev->info->clk)
+ clk_disable(idev->info->clk);
module_put(idev->owner);
kfree(listener);
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 5dcc9ff..6bc0e7e 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -17,6 +17,7 @@
#include <linux/module.h>
#include <linux/fs.h>
#include <linux/interrupt.h>
+#include <linux/clk.h>
struct uio_map;
@@ -87,6 +88,7 @@ struct uio_info {
long irq;
unsigned long irq_flags;
void *priv;
+ struct clk *clk;
irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
int (*open)(struct uio_info *info, struct inode *inode);
--
1.6.3.1
On Wed, Jun 24, 2009 at 05:30:24PM +0200, Daniel Mack wrote:
> Add a pointer to a 'struct clk' to uio_info. Drivers can set
> this pointer if a clock is needed, and the UIO core will care
> to enable and disable it upon device open and release.
Do you have a UIO driver that needs this?
If so, please submit it at the same time, otherwise adding
infrastructure for no driver that needs it, is pretty pointless.
thanks,
greg k-h
On Wed, Jun 24, 2009 at 05:30:25PM +0200, Daniel Mack wrote:
> 'default n' is the default, there is no need for these lines.
Why, because 'n' is the generic default?
If so, it really can't hurt, right? :)
thanks,
greg k-h
On Wed, Jun 24, 2009 at 09:34:14AM -0700, Greg KH wrote:
> On Wed, Jun 24, 2009 at 05:30:24PM +0200, Daniel Mack wrote:
> > Add a pointer to a 'struct clk' to uio_info. Drivers can set
> > this pointer if a clock is needed, and the UIO core will care
> > to enable and disable it upon device open and release.
>
> Do you have a UIO driver that needs this?
No.
> If so, please submit it at the same time, otherwise adding
> infrastructure for no driver that needs it, is pretty pointless.
As I wrote in my introduction email, I wanted to use this framework but
eventually I didn't. But then I had these patches flying around and I
thought sharing them is better than throwing them away. They work well,
it's just that this particular feature has no uses at the moment.
Anyway, feel free to discard them if you don't like them. Just wanted to
let you know :)
Daniel
On Wed, Jun 24, 2009 at 09:34:56AM -0700, Greg KH wrote:
> On Wed, Jun 24, 2009 at 05:30:25PM +0200, Daniel Mack wrote:
> > 'default n' is the default, there is no need for these lines.
>
> Why, because 'n' is the generic default?
> If so, it really can't hurt, right? :)
Well, it's a cleanup. In it's purest manner. :)
Daniel
On Wed, Jun 24, 2009 at 08:29:36PM +0200, Daniel Mack wrote:
> On Wed, Jun 24, 2009 at 09:34:14AM -0700, Greg KH wrote:
> > On Wed, Jun 24, 2009 at 05:30:24PM +0200, Daniel Mack wrote:
> > > Add a pointer to a 'struct clk' to uio_info. Drivers can set
> > > this pointer if a clock is needed, and the UIO core will care
> > > to enable and disable it upon device open and release.
> >
> > Do you have a UIO driver that needs this?
>
> No.
>
> > If so, please submit it at the same time, otherwise adding
> > infrastructure for no driver that needs it, is pretty pointless.
>
> As I wrote in my introduction email, I wanted to use this framework but
> eventually I didn't. But then I had these patches flying around and I
> thought sharing them is better than throwing them away. They work well,
> it's just that this particular feature has no uses at the moment.
Thanks for the change, it looks nice, but until we have a driver that
needs it, I say we hold off on this patch.
thanks,
greg k-h
On Wed, Jun 24, 2009 at 08:32:12PM +0200, Daniel Mack wrote:
> On Wed, Jun 24, 2009 at 09:34:56AM -0700, Greg KH wrote:
> > On Wed, Jun 24, 2009 at 05:30:25PM +0200, Daniel Mack wrote:
> > > 'default n' is the default, there is no need for these lines.
> >
> > Why, because 'n' is the generic default?
> > If so, it really can't hurt, right? :)
>
> Well, it's a cleanup. In it's purest manner. :)
Heh, fair enough, I'll queue it up for .32.
thanks,
greg k-h
On Wed, Jun 24, 2009 at 09:34:14AM -0700, Greg KH wrote:
> On Wed, Jun 24, 2009 at 05:30:24PM +0200, Daniel Mack wrote:
> > Add a pointer to a 'struct clk' to uio_info. Drivers can set
> > this pointer if a clock is needed, and the UIO core will care
> > to enable and disable it upon device open and release.
>
> Do you have a UIO driver that needs this?
>
> If so, please submit it at the same time, otherwise adding
> infrastructure for no driver that needs it, is pretty pointless.
>
We can use this for the uio_pdrv_genirq case on sh, but open/release is a
bit coarse grained. Presently we default-enable clocks for devices that
are handled through uio, so doing it at open/release is at least better
than that. The other thing to consider is if we really want to add a
HAVE_CLK depdendency outright, or just ifdef around it..
On Thu, Jun 25, 2009 at 3:41 AM, Paul Mundt<[email protected]> wrote:
> On Wed, Jun 24, 2009 at 09:34:14AM -0700, Greg KH wrote:
>> On Wed, Jun 24, 2009 at 05:30:24PM +0200, Daniel Mack wrote:
>> > Add a pointer to a 'struct clk' to uio_info. Drivers can set
>> > this pointer if a clock is needed, and the UIO core will care
>> > to enable and disable it upon device open and release.
>>
>> Do you have a UIO driver that needs this?
>>
>> If so, please submit it at the same time, otherwise adding
>> infrastructure for no driver that needs it, is pretty pointless.
>>
> We can use this for the uio_pdrv_genirq case on sh, but open/release is a
> bit coarse grained. Presently we default-enable clocks for devices that
> are handled through uio, so doing it at open/release is at least better
> than that. The other thing to consider is if we really want to add a
> HAVE_CLK depdendency outright, or just ifdef around it..
Yeah, we export quite a few UIO devices on SuperH, and
stopping/starting clocks is on my TODO list.
Like Paul says, open/release only is a bit coarse grained. I'd like to
be able to keep the device open in user space, having a bunch of
memory windows mmap()ed but still being able to power down the
hardware block somehow.
The patch seems to be about enabling and disabling clocks, the actual
clock frequency is not really exposed to user space what I can tell. I
was more thinking along the lines of having an array of clocks per uio
device - pretty much like the memory windows - and just expose the
clocks and their frequencies to user space and letting user space
enable and disable using sysfs.
I wonder if it would be possible to enable the clock in open() and
stop the clock automatically from the ISR and use writenotify and/or
some kind of trapping mechanism to restart the clock when page faults
are received. If this works or not will probably vary from device to
device so it's probably not a good idea either.
Also, there may be some overlap with the Runtime PM code since this
seems more to be about stopping clocks than exposing frequencies to
user space.
Cheers,
/ magnus
On Fri, Jun 26, 2009 at 12:17:03AM +0900, Magnus Damm wrote:
> On Thu, Jun 25, 2009 at 3:41 AM, Paul Mundt<[email protected]> wrote:
> > On Wed, Jun 24, 2009 at 09:34:14AM -0700, Greg KH wrote:
> >> On Wed, Jun 24, 2009 at 05:30:24PM +0200, Daniel Mack wrote:
> >> > Add a pointer to a 'struct clk' to uio_info. Drivers can set
> >> > this pointer if a clock is needed, and the UIO core will care
> >> > to enable and disable it upon device open and release.
> >>
> >> Do you have a UIO driver that needs this?
> >>
> >> If so, please submit it at the same time, otherwise adding
> >> infrastructure for no driver that needs it, is pretty pointless.
> >>
> > We can use this for the uio_pdrv_genirq case on sh, but open/release is a
> > bit coarse grained. Presently we default-enable clocks for devices that
> > are handled through uio, so doing it at open/release is at least better
> > than that. The other thing to consider is if we really want to add a
> > HAVE_CLK depdendency outright, or just ifdef around it..
>
> Yeah, we export quite a few UIO devices on SuperH, and
> stopping/starting clocks is on my TODO list.
>
> Like Paul says, open/release only is a bit coarse grained. I'd like to
> be able to keep the device open in user space, having a bunch of
> memory windows mmap()ed but still being able to power down the
> hardware block somehow.
>
> The patch seems to be about enabling and disabling clocks, the actual
> clock frequency is not really exposed to user space what I can tell. I
> was more thinking along the lines of having an array of clocks per uio
> device - pretty much like the memory windows - and just expose the
> clocks and their frequencies to user space and letting user space
> enable and disable using sysfs.
>
This is longer term work that still needs to be discussed and agreed
upon, which is totally out of scope for this patch. Doing it at
open/close time is already better than what we have today, so I am
inclined to roll this patch in to a topic branch and convert the existing
sh users over to it. Any improvement with regards to making it more fine
grained, or deciding what to expose to userspace, is something to look at
much later, particularly as the runtime pm code is going to be a
prerequisite for a lot of it (ie, 2.6.33).
Greg, assuming you have no objections, if you can ack the patch I can
carry it in my tree and do the sh conversion on top of it. It will
require HAVE_CLK ifdefs for now, but I'll take care of that.
On Fri, Jun 26, 2009 at 2:47 AM, Paul Mundt<[email protected]> wrote:
> On Fri, Jun 26, 2009 at 12:17:03AM +0900, Magnus Damm wrote:
>> On Thu, Jun 25, 2009 at 3:41 AM, Paul Mundt<[email protected]> wrote:
>> > On Wed, Jun 24, 2009 at 09:34:14AM -0700, Greg KH wrote:
>> >> On Wed, Jun 24, 2009 at 05:30:24PM +0200, Daniel Mack wrote:
>> >> > Add a pointer to a 'struct clk' to uio_info. Drivers can set
>> >> > this pointer if a clock is needed, and the UIO core will care
>> >> > to enable and disable it upon device open and release.
>> >>
>> >> Do you have a UIO driver that needs this?
>> >>
>> >> If so, please submit it at the same time, otherwise adding
>> >> infrastructure for no driver that needs it, is pretty pointless.
>> >>
>> > We can use this for the uio_pdrv_genirq case on sh, but open/release is a
>> > bit coarse grained. Presently we default-enable clocks for devices that
>> > are handled through uio, so doing it at open/release is at least better
>> > than that. The other thing to consider is if we really want to add a
>> > HAVE_CLK depdendency outright, or just ifdef around it..
>>
>> Yeah, we export quite a few UIO devices on SuperH, and
>> stopping/starting clocks is on my TODO list.
>>
>> Like Paul says, open/release only is a bit coarse grained. I'd like to
>> be able to keep the device open in user space, having a bunch of
>> memory windows mmap()ed but still being able to power down the
>> hardware block somehow.
>>
>> The patch seems to be about enabling and disabling clocks, the actual
>> clock frequency is not really exposed to user space what I can tell. I
>> was more thinking along the lines of having an array of clocks per uio
>> device - pretty much like the memory windows - and just expose the
>> clocks and their frequencies to user space and letting user space
>> enable and disable using sysfs.
>>
> This is longer term work that still needs to be discussed and agreed
> upon, which is totally out of scope for this patch. Doing it at
> open/close time is already better than what we have today, so I am
> inclined to roll this patch in to a topic branch and convert the existing
> sh users over to it. Any improvement with regards to making it more fine
> grained, or deciding what to expose to userspace, is something to look at
> much later, particularly as the runtime pm code is going to be a
> prerequisite for a lot of it (ie, 2.6.33).
Performing clk_enable()/clk_disable() at open()/close() time sounds
good, but we probably also want to extend the uio_pdrv_genirq driver
to do clk_get()/clk_put() at probe()/remove() time well.
/ magnus