Re: [PATCH mfd 1/1] mfd/ptp: clockmatrix: support 32-bit address space

[Jakub Kicinski]

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?