Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <1f0cd6c635d1f570ff36c4b5a94e8d5a9f596aee.camel@redhat.com>
Subject: Re: [PATCH net-next v6 1/6] ptp: clockmatrix: support 32-bit
 address space
From:   Paolo Abeni <pabeni@redhat.com>
To:     Min Li <lnimi@hotmail.com>, richardcochran@gmail.com,
        lee@kernel.org
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        Min Li <min.li.xe@renesas.com>
Date:   Tue, 05 Dec 2023 10:06:46 +0100
In-Reply-To: <PH7PR03MB70644CE21E835B48799F3EB3A082A@PH7PR03MB7064.namprd03.prod.outlook.com>
References: <PH7PR03MB70644CE21E835B48799F3EB3A082A@PH7PR03MB7064.namprd03.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

Hi,

On Thu, 2023-11-30 at 13:46 -0500, Min Li wrote:
> diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.=
c
> index f6f9d4adce04..f8556627befa 100644
> --- a/drivers/ptp/ptp_clockmatrix.c
> +++ b/drivers/ptp/ptp_clockmatrix.c
> @@ -41,8 +41,8 @@ module_param(firmware, charp, 0);
>  static int _idtcm_adjfine(struct idtcm_channel *channel, long scaled_ppm=
);
> =20
>  static inline int idtcm_read(struct idtcm *idtcm,
> -			     u16 module,
> -			     u16 regaddr,
> +			     u32 module,
> +			     u32 regaddr,
>  			     u8 *buf,
>  			     u16 count)
>  {

[...]
> @@ -553,11 +554,11 @@ static int _sync_pll_output(struct idtcm *idtcm,
>  	val =3D SYNCTRL1_MASTER_SYNC_RST;
> =20
>  	/* Place master sync in reset */
> -	err =3D idtcm_write(idtcm, 0, sync_ctrl1, &val, sizeof(val));
> +	err =3D idtcm_write(idtcm, sync_ctrl1, 0, &val, sizeof(val));
>  	if (err)
>  		return err;

I'm sorry, but it looks like my previous feedback was not clear enough:
now that the 'regaddr' argument in idtcm_write() has 'u32' type, you
don't need anymore to swap the 'module' and 'regaddr' arguments in the
call site.

Specifically, you can drop the above chunk, many later similar ones=20
and additionally even chunks calling to idtcm_write(), for the same
reason.

That would make the patch smaller, and IMHO more clear: it would be=20
strange that something that was known as a register address suddenly
become a 'module'.

Thanks!

Paolo