Re: [PATCH] crypt: bfin_crc: access crc registers by ioread32 and iowrite32 functions

[Marek Vasut <marex@denx.de>]

On Thursday, April 10, 2014 at 05:50:37 AM, Sonic Zhang wrote:
> Hi Marek,
> 
> On Thu, Apr 10, 2014 at 1:32 AM, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, March 27, 2014 at 09:52:33 AM, Sonic Zhang wrote:
> >> From: Sonic Zhang <sonic.zhang@analog.com>
> >> 
> >> Move architecture independant crc header file out of the blackfin
> >> folder.
> >> 
> >> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
> > 
> > [...]
> > 
> >> @@ -530,13 +532,14 @@ static irqreturn_t bfin_crypto_crc_handler(int
> >> irq, void *dev_id) {
> >> 
> >>       struct bfin_crypto_crc *crc = dev_id;
> >> 
> >> -     if (crc->regs->status & DCNTEXP) {
> >> -             crc->regs->status = DCNTEXP;
> >> +     if (ioread32(&crc->regs->status) & DCNTEXP) {
> >> +             iowrite32(DCNTEXP, &crc->regs->status);
> >> 
> >>               /* prepare results */
> >> 
> >> -             put_unaligned_le32(crc->regs->result, crc->req->result);
> >> +             put_unaligned_le32(ioread32(&crc->regs->result),
> >> +                     crc->req->result);
> >> 
> >> -             crc->regs->control &= ~BLKEN;
> >> +             iowrite32(ioread32(&crc->regs->control) & ~BLKEN,
> >> &crc->regs-
> >>
> >>control);
> >>
> > You should avoid combining the IO accessors into each other like this,
> > it's rather cryptic. Please introduce some variable like so:
> > 
> > u32 reg;
> > reg = ioread32(...);
> > reg &= ~....
> > iowrite32(reg, ...);
> > 
> > [...]
> 
> OK
> 
> > Why do you not use readl()/writel() ?
> 
> ioread and iowrite are inline function, which do parameter type
> checking, while readl and writel don't as a macro.
> I am fine if you prefer readl/writel in crypto code.

I was just curious what the rationale behind this was.

Best regards,
Marek Vasut