Re: drivers/usb/musb/musb_io.h

[Andrew Morton <akpm@linux-foundation.org>]

(cc linux-arch)

On Fri, 15 Aug 2008 11:11:55 +0300 Felipe Balbi <me@felipebalbi.com> wrote:

> On Fri, Aug 15, 2008 at 01:02:27AM -0700, Andrew Morton wrote:
> > > SH also defines read/write friends.
> > 
> > That's an unilluminating changelog.
> > What's actually going on in here?
> 
> Those functions are used for fifo reading/writing operations. Some
> architectures provide them and some don't. For those who don't, we add
> the definition to the driver.
> 
> > Why is a driver implementing what appear to be core architecture helper
> > functions?
> 
> Just because the core architecture doesn't provide them.
> 
> > What is a proper fix?
> 
> The proper fix is that we always have, at least, stubs for read/write
> friends even if the arch doesn't provide them.

That is absolutely not the proper fix.  Far from it.  This way we end
up with multiple (and of course, different) implementations of these
things in an arbitrary number of drivers.

A proper fix would be to a) define the semantics of these operations,
b) document them then c) implement them on each architecture.

One possible implementation might be


--- a/include/linux/io.h~a
+++ a/include/linux/io.h
@@ -67,4 +67,41 @@ int check_signature(const volatile void 
 			const unsigned char *signature, int length);
 void devm_ioremap_release(struct device *dev, void *res);
 
+#ifndef readsl
+
+/*
+ * description goes here
+ */
+static inline void readsl(const void __iomem *addr, void *buf, int len)
+{
+	insl((unsigned long)addr, buf, len);
+}
+
+static inline void readsw(const void __iomem *addr, void *buf, int len)
+{
+	insw((unsigned long)addr, buf, len);
+}
+
+static inline void readsb(const void __iomem *addr, void *buf, int len)
+{
+	insb((unsigned long)addr, buf, len);
+}
+
+static inline void writesl(const void __iomem *addr, const void *buf, int len)
+{
+	outsl((unsigned long)addr, buf, len);
+}
+
+static inline void writesw(const void __iomem *addr, const void *buf, int len)
+{
+	outsw((unsigned long)addr, buf, len);
+}
+
+static inline void writesb(const void __iomem *addr, const void *buf, int len)
+{
+	outsb((unsigned long)addr, buf, len);
+}
+
+#endif		/* readsl */
+
 #endif /* _LINUX_IO_H */


But just saying "oh look, the arch layer is all screwed up, let's hack
around it in our driver" is plain irresponsible.

> > avr32 and powerpc might also have conflicts.  Now, or in the future.

Did you review these architectures?