Re: [PATCH/RFC] Hookable IO operations

[Arnd Bergmann <arnd@arndb.de>]

On Friday 03 November 2006 23:32, Benjamin Herrenschmidt wrote:

> > If you leave out the 'extern', it fits into a normal line ;-)
> 
> Did I change them from the initial definition ? Anyway, I dislike
> prototypes in headers without "extern".

I prefer writing down the extern as well, but the common style
seems to have moved away from it now.

> 
> > > +
> > > +extern void (*__memset_io)(volatile void __iomem *addr, int c,
> > > +			   unsigned long n);
> > > +extern void (*__memcpy_fromio)(void *dest, const volatile void __iomem *src,
> > > +			       unsigned long n);
> > > +extern void (*__memcpy_toio)(volatile void __iomem *dest, const void *src,
> > > +			     unsigned long n);
> > 
> > Why are these all separate symbols? Wouldn't it be clearer to group them
> > in ppc_md, or a similar structure of function pointers?
> 
> Because the IO ones are separate already and I want to keep things
> consistent with them.

Actually I meant all of the I/O pointers, not just these three. When
you were describing your idea to me, I was thinking of something like

struct ppc_io {
	void (*writeb)(u8 val);
	void (*writew)(u16 val);
	void (*writel)(u32 val);
	void (*writeq)(u64 val);
	...
	void (*memset_io)(volatile void __iomem *addr, int c,
				 unsigned long n);
	void (*__memcpy_fromio)(void *dest,
			const volatile void __iomem *src, unsigned long n);
	void (*__memcpy_toio)(volatile void __iomem *dest,
			const void *src, unsigned long n);
} *ppc_io;

Did you never consider this, or did you make your mind up in the
process?

Is your current code more efficient?

> > > -static u8 iSeries_Read_Byte(const volatile void __iomem *IoAddress)
> > > +static u8 iseries_readb(const volatile void __iomem *IoAddress)
> > 
> > Since you're converting iSeries_Read_Byte from SilLycAps, shouldn't
> > you change IoAddress to something more sensible at the same time?
> 
> No, I'm only changing the prototype & name to better match my hooks,
> further cleanups of the content of these functions is something I'll do
> in a separate patch.

ok, makes sense.

	Arnd <><