Two small UIO patches

Two small UIO patches

[Daniel Mack]

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

[PATCH 1/2] UIO: add device clock support

[Daniel Mack]

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 <daniel@caiaq.de>
Cc: Hans J. Koch <hjk@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 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

[PATCH 2/2] UIO: remove 'default n' from Kconfig

[Daniel Mack]

'default n' is the default, there is no need for these lines.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Hans J. Koch <hjk@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 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

Re: [PATCH 2/2] UIO: remove 'default n' from Kconfig

[Greg KH]

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

Re: [PATCH 2/2] UIO: remove 'default n' from Kconfig

[Daniel Mack]

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

Re: [PATCH 2/2] UIO: remove 'default n' from Kconfig

[Greg KH]

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

Re: [PATCH 1/2] UIO: add device clock support

[Greg KH]

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

Re: [PATCH 1/2] UIO: add device clock support

[Daniel Mack]

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

Re: [PATCH 1/2] UIO: add device clock support

[Greg KH]

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

Re: [PATCH 1/2] UIO: add device clock support

[Paul Mundt]

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..

Re: [PATCH 1/2] UIO: add device clock support

[Magnus Damm]

On Thu, Jun 25, 2009 at 3:41 AM, Paul Mundt<lethal@linux-sh.org> 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

Re: [PATCH 1/2] UIO: add device clock support

[Paul Mundt]

On Fri, Jun 26, 2009 at 12:17:03AM +0900, Magnus Damm wrote:
> On Thu, Jun 25, 2009 at 3:41 AM, Paul Mundt<lethal@linux-sh.org> 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.

Re: [PATCH 1/2] UIO: add device clock support

[Magnus Damm]

On Fri, Jun 26, 2009 at 2:47 AM, Paul Mundt<lethal@linux-sh.org> 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<lethal@linux-sh.org> 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