RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using {read,write}s{b,w}

[Fabrizio Castro]

> From: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> Subject: RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> {read,write}s{b,w}
> 
> Hi Andy,
> 
> Thanks for your reply.
> 
> > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> > {read,write}s{b,w}
> >
> > On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro
> > <fabrizio.castro.jz@xxxxxxxxxxx> wrote:
> > > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using
> > > > {read,write}s{b,w}
> > > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro
> > > > <fabrizio.castro.jz@xxxxxxxxxxx> wrote:
> >
> > ...
> >
> > > > According to the hardware documentation[1], the access size for
> > both
> > > > the
> > > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use
> > writel()
> > > > resp. readl().  So please check with the hardware people first.
> > > >
> > > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30.
> > >
> > > You are right, access is 32 bits (and although this patch works
> > fine,
> > > we should avoid accessing those regs any other way). Now I
> remember
> > > why I decided to go for the bespoke loops in the first place,
> > writesl
> > > and readsl provide the right register access, but the wrong
> pointer
> > > arithmetic for this use case.
> > > For this patch I ended up selecting writesw/writesb/readsw/readsb
> to
> > > get the right pointer arithmetic, but the register access is not
> as
> > > per manual.
> > >
> > > I can either fallback to using the bespoke loops (I can still
> > > remove the unnecessary u8* and u16* casting ;-) ), or I can add
> > > new APIs for this sort of access to io.h (e.g. writesbl, writeswl,
> > > readsbl, readswl, in order to get the pointer arithmetic right for
> > > the type of array handled, while keeping memory access as
> expected).
> > >
> > > What are your thoughts on that?
> >
> > I think that you need to use readsl() / writesl() on the custom
> buffer
> > with something like
> >
> > *_sparse() / *_condence() APIs added (perhaps locally to this
> driver)
> > as they may be reused by others in the future.
> > Having all flavours of read*()/write*() does not scale in my
> opinion.
> 
> Maybe a "generic" macro then (one for reading and one for writing)?
> So that we can pass the data type (to get the pointer arithmetic
> right) and the function name to use for the read/write operations
> (to get the register operations right)?
> Maybe that would scale and cater for most needs (including the one
> from this patch)?

Something like the below perhaps:

#ifndef readsx
#define readsx(atype, readc, addr, buffer, count)	\
	({								\
		if (count) {					\
			unsigned int cnt = count;		\
			atype *buf = buffer;			\
									\
			do {						\
				atype x = readc(addr);		\
				*buf++ = x;				\
			} while (--cnt);				\
		}							\
	})
#endif

#ifndef writesx
#define writesx(atype, writec, addr, buffer, count)	\
	({								\
		if (count) {					\
			unsigned int cnt = count;		\
			const atype *buf = buffer;		\
									\
			do {						\
				writec(*buf++, addr);		\
			} while (--cnt);				\
		}							\

	})

#endif

Cheers,
Fab

> 
> Cheers,
> Fab
> 
> >
> > --
> > With Best Regards,
> > Andy Shevchenko