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

Paolo Abeni
2024-01-09T07:46:28Z

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