Re: [PATCH v3 1/7] staging: vt6655: Replace VNSvInPortB with ioread8

[Greg Kroah-Hartman]

On Mon, Apr 11, 2022 at 10:49:20PM +0200, Philipp Hortmann wrote:
> Replace macro VNSvInPortB with ioread8.
> The name of macro and the arguments use CamelCase which
> is not accepted by checkpatch.pl
> 
> Since there are more than one checkpatch issue per line,
> more steps are rquired to fix.
> 
> Signed-off-by: Philipp Hortmann <philipp.g.hortmann@xxxxxxxxx>
> ---
> V1 -> V2: This patch was 4/7 and is now 3/6
> V2 -> V3: Changed from rename macro arguments to replace
>           macro VNSvInPortB with ioread8
>           This patch was 3/6 and is now 1/7
> ---
>  drivers/staging/vt6655/baseband.c    |  6 +++---
>  drivers/staging/vt6655/card.c        |  2 +-
>  drivers/staging/vt6655/device_main.c |  4 ++--
>  drivers/staging/vt6655/mac.h         | 26 ++++++++++----------------
>  drivers/staging/vt6655/srom.c        |  6 +++---
>  drivers/staging/vt6655/upc.h         |  3 ---
>  6 files changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/baseband.c b/drivers/staging/vt6655/baseband.c
> index dfdb0ebf43b5..decb55e96f02 100644
> --- a/drivers/staging/vt6655/baseband.c
> +++ b/drivers/staging/vt6655/baseband.c
> @@ -1916,13 +1916,13 @@ bool bb_read_embedded(struct vnt_private *priv, unsigned char by_bb_addr,
>  	MACvRegBitsOn(iobase, MAC_REG_BBREGCTL, BBREGCTL_REGR);
>  	/* W_MAX_TIMEOUT is the timeout period */
>  	for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {
> -		VNSvInPortB(iobase + MAC_REG_BBREGCTL, &by_value);
> +		by_value = ioread8(iobase + MAC_REG_BBREGCTL);

This type of change is great, but...

> @@ -597,18 +597,12 @@ do {								\
>  #define MACvReadEtherAddress(iobase, pbyEtherAddr)		\
>  do {								\
>  	VNSvOutPortB(iobase + MAC_REG_PAGE1SEL, 1);		\
> -	VNSvInPortB(iobase + MAC_REG_PAR0,			\
> -		    (unsigned char *)pbyEtherAddr);		\
> -	VNSvInPortB(iobase + MAC_REG_PAR0 + 1,		\
> -		    pbyEtherAddr + 1);				\
> -	VNSvInPortB(iobase + MAC_REG_PAR0 + 2,		\
> -		    pbyEtherAddr + 2);				\
> -	VNSvInPortB(iobase + MAC_REG_PAR0 + 3,		\
> -		    pbyEtherAddr + 3);				\
> -	VNSvInPortB(iobase + MAC_REG_PAR0 + 4,		\
> -		    pbyEtherAddr + 4);				\
> -	VNSvInPortB(iobase + MAC_REG_PAR0 + 5,		\
> -		    pbyEtherAddr + 5);				\
> +	(*((unsigned char *)pbyEtherAddr) = ioread8(iobase + MAC_REG_PAR0));		\

Why cast one and not all?  No casts should be needed.  If so, the caller
is wrong.

> +	(*(pbyEtherAddr + 1) = ioread8(iobase + MAC_REG_PAR0 + 1));	\

That's odd.

Ok, I guess it's an array, right?  How about:
	pbyEtherAddr[0] = ioread8(iobase + MAC_REG_PAR0);	\
	pbyEtherAddr[1] = ioread8(iobase + MAC_REG_PAR0 + 1);	\
and so on?

Much simpler and easier to understand.

And then there's this odd stuff:

>  #define MACvGPIOIn(iobase, pbyValue)					\
> -	VNSvInPortB(iobase + MAC_REG_GPIOCTL1, pbyValue)
> +	*(pbyValue) = ioread8(iobase + MAC_REG_GPIOCTL1)

pbyValue?  unwind this first, you should be able to just have a normal
ioread() call, right?

Or, make it a function.

thanks,

greg k-h