Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Mar 2023 12:56:19 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Min Li <lnimi@hotmail.com>
Cc:     richardcochran@gmail.com, lee@kernel.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        Min Li <min.li.xe@renesas.com>
Subject: Re: [PATCH mfd 1/1] mfd/ptp: clockmatrix: support 32-bit address
 space
Message-ID: <20230322125619.731912cf@kernel.org>
In-Reply-To: <MW5PR03MB69323E281F1360A4F6C92838A0819@MW5PR03MB6932.namprd03.prod.outlook.com>
References: <MW5PR03MB69323E281F1360A4F6C92838A0819@MW5PR03MB6932.namprd03.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 21 Mar 2023 15:10:06 -0400 Min Li wrote:
> -		buf[0] = (u8)(page & 0xff);
> -		buf[1] = (u8)((page >> 8) & 0xff);
> +		buf[0] = (u8)(page & 0xFF);
> +		buf[1] = (u8)((page >> 8) & 0xFF);

why did you decide to change from 0xff to 0xFF as part of this big
change? It's unnecessary churn.

> +		buf[2] = (u8)((page >> 16) & 0xFF);
> +		buf[3] = (u8)((page >> 24) & 0xFF);
> +		bytes = 4;
>  		break;

>  static inline int idtcm_read(struct idtcm *idtcm,
> -			     u16 module,
> +			     u32 module,
>  			     u16 regaddr,
>  			     u8 *buf,
>  			     u16 count)
> @@ -50,7 +50,7 @@ static inline int idtcm_read(struct idtcm *idtcm,
>  }
>  
>  static inline int idtcm_write(struct idtcm *idtcm,
> -			      u16 module,
> +			      u32 module,
>  			      u16 regaddr,
>  			      u8 *buf,
>  			      u16 count)

>  	/* PLL5 can have OUT8 as second additional output. */
>  	if (pll == 5 && qn_plus_1 != 0) {
> -		err = idtcm_read(idtcm, 0, HW_Q8_CTRL_SPARE,
> +		err = idtcm_read(idtcm, HW_Q8_CTRL_SPARE, 0,
>  				 &temp, sizeof(temp));
>  		if (err)
>  			return err;
>  
>  		temp &= ~(Q9_TO_Q8_SYNC_TRIG);
>  
> -		err = idtcm_write(idtcm, 0, HW_Q8_CTRL_SPARE,
> +		err = idtcm_write(idtcm, HW_Q8_CTRL_SPARE, 0,
>  				  &temp, sizeof(temp));
>  		if (err)
>  			return err;
>  
>  		temp |= Q9_TO_Q8_SYNC_TRIG;
>  
> -		err = idtcm_write(idtcm, 0, HW_Q8_CTRL_SPARE,
> +		err = idtcm_write(idtcm, HW_Q8_CTRL_SPARE, 0,
>  				  &temp, sizeof(temp));
>  		if (err)
>  			return err;

Why are you flipping all these arguments?
Isn't HW_Q8_CTRL_SPARE regaddr?

Could you to split your patches into multiple steps to make them easier
to reivew?