> 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
