From: Greg Kroah-Hartman
> Sent: 27 April 2022 06:55
>
> On Wed, Apr 27, 2022 at 07:42:23AM +0200, Philipp Hortmann wrote:
> > Replace macro VNSvInPortD with ioread32 and as it was
> > the only user, it can now be removed.
> > The name of macro and the arguments use CamelCase which
> > is not accepted by checkpatch.pl
> >
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Signed-off-by: Philipp Hortmann <philipp.g.hortmann@xxxxxxxxx>
> > ---
> > V1 -> V2: This patch was 5/7 and is now 4/6
> > V2 -> V3: Inserted patch that was before in a different way in
> > "Rename macros VNSvInPortB,W,D with CamelCase ..."
> > This patch was part of 4/6 and is now 3/7
> > V3 -> V4: Removed casting of the output variable
> > V4 -> V5: Joint patch "Replace two VNSvInPortD with ioread64_lo_hi"
> > with this patch. Changed ioread64 to two ioread32 as
> > ioread64 does not work with 32 Bit computers.
> > Shorted and simplified patch description.
> > V5 -> V6: Added missing version in subject
> > ---
> > drivers/staging/vt6655/card.c | 6 ++++--
> > drivers/staging/vt6655/device_main.c | 6 +++---
> > drivers/staging/vt6655/mac.h | 18 +++++++++---------
> > drivers/staging/vt6655/rf.c | 2 +-
> > drivers/staging/vt6655/upc.h | 3 ---
> > 5 files changed, 17 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/staging/vt6655/card.c b/drivers/staging/vt6655/card.c
> > index 022310af5485..0dd13e475d6b 100644
> > --- a/drivers/staging/vt6655/card.c
> > +++ b/drivers/staging/vt6655/card.c
> > @@ -744,6 +744,7 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
> > void __iomem *iobase = priv->port_offset;
> > unsigned short ww;
> > unsigned char data;
> > + u32 low, high;
> >
> > MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
> > for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {
> > @@ -753,8 +754,9 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
> > }
> > if (ww == W_MAX_TIMEOUT)
> > return false;
> > - VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF);
> > - VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1);
> > + low = ioread32(iobase + MAC_REG_TSFCNTR);
> > + high = ioread32(iobase + MAC_REG_TSFCNTR + 4);
> > + *pqwCurrTSF = low + ((u64)high << 32);
>
> Are you _sure_ this is doing the same thing?
>
> Adding 1 to a u64 pointer increments it by a full u64. So I guess the
> cast to u32 * moves it only by a u32? Hopefully? That's messy.
The new code is mostly better.
But is different on BE systems - who knows what is actually needed.
Depends what is being copied.
Actually I suspect that 'iobase' should be an __iomem structure
pointer, pqwCurrTSF a point of the same type and MAC_REG_xxxx
structure members.
Then the code should be using readl() not ioread32().
I very much doubt that 'iobase' is in PCI IO space.
David
>
> Why not keep the current mess and do:
> pqwCurrTSF = ioread32(iobase + MAC_REG_TSFCNTR);
> ((u32 *)pqwCurTSF + 1) = ioread32(iobase + MAC_REG_TSFCNTR + 4);
>
> Or does that not compile? Ick, how about:
> u32 *temp = (u32 *)pqwCurTSF;
>
> temp = ioread32(iobase + MAC_REG_TSFCNTR);
> temp++;
> temp = ioread32(iobase + MAC_REG_TSFCNTR + 4);
> As that duplicates the current code a bit better.
>
> I don't know, it's like polishing dirt, in the end, it's still dirt...
>
> How about looking at the caller of this to see what it expects to do
> with this information? Unwind the mess from there?
>
> thanks,
>
> greg k-h
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
