Re: [PATCH net-next v7 1/5] ptp: clockmatrix: support 32-bit address space

[Paolo Abeni]

On Thu, 2024-01-04 at 11:36 -0500, Min Li wrote:
> @@ -553,11 +554,11 @@ static int _sync_pll_output(struct idtcm *idtcm,
>  	val = SYNCTRL1_MASTER_SYNC_RST;
>  
>  	/* Place master sync in reset */
> -	err = idtcm_write(idtcm, 0, sync_ctrl1, &val, sizeof(val));
> +	err = idtcm_write(idtcm, sync_ctrl1, 0, &val, sizeof(val));
>  	if (err)
>  		return err;

I'm sorry for the late feedback: I lost track the last replies in the
previous revision and later I was on PTO.

Let me extract the relevant slice from such thread:

On  Wed, 13 Dec 2023 21:04:07 +0000 Min Li wrote:
> > My reading is that this patch reverses the usage of module and regaddr.
> > F.e. the following hunk:
> > 
> > @@ -553,11 +554,11 @@ static int _sync_pll_output(struct idtcm *idtcm,
> > 	val = SYNCTRL1_MASTER_SYNC_RST;
> > 
> > 	/* Place master sync in reset */
> > 	err = idtcm_write(idtcm, 0, sync_ctrl1, &val, sizeof(val));
> > 	err = idtcm_write(idtcm, sync_ctrl1, 0, &val, sizeof(val));
> > 	if (err)
> > 		return err;
> > 
> > 	err = idtcm_write(idtcm, 0, sync_ctrl0, &sync_src, sizeof(sync_src));
> > 	err = idtcm_write(idtcm, sync_ctrl0, 0, &sync_src, sizeof(sync_src));
> > 	if (err)
> > 		return err;
> > 
> > If that is really intended I think it needs to be explained, or possibly a
> > separate patch.
>
> Hi Simon
> sync_ctrl0/1 was meant to be a module and it was in a wrong place. 
> And this patch is just correcting it.

Then you need to move this chunk (and all the later on swapping the
'address' and the 'module' argument in a separate patch. Mixing this
fix and  the address space extension is confusing.

Additionally we are currently preparing the net-next PR for 6.8 and I
don't feel very confident to apply such a large refactor this late. I
think it should be better postpone to the next cycle.

Cheers,

Paolo