Re: [PATCH v1] ptp: ocp: Add support for Xilinx-based Adva TimeCard variant

[Vadim Fedorenko]

On 26/02/2026 12:20, Simon Horman wrote:
> On Wed, Feb 25, 2026 at 12:29:38PM +0200, Sagi Maimon wrote:
> > Add support for the Adva TimeCard model built on a Xilinx-based design.
> > This patch enables detection and integration of the new hardware within
> > the existing OCP timecard framework.
> >
> > The Xilinx variant relies on the shared driver infrastructure, requiring
> > only small, targeted additions to accommodate its specific
> > characteristics.
> >
> > Signed-off-by: Sagi Maimon <maimon.sagi@xxxxxxxxx>
> > ---
> >   drivers/ptp/ptp_ocp.c | 318 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 318 insertions(+)
> >
> > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>
> ...
>
> > @@ -2926,6 +3168,45 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
> >   	return ptp_ocp_init_clock(bp, r->extra);
> >   }
> >   
> > +/* ADVA specific board X initializers; last "resource" registered. */
> > +static int
> > +ptp_ocp_adva_board_x1_init(struct ptp_ocp *bp, struct ocp_resource *r)
> > +{
> > +	int err;
> > +	u32 version;
> > +
> > +	bp->flash_start = 0x1000000;
> > +	bp->eeprom_map = fb_eeprom_map;
> > +	bp->sma_op = &ocp_adva_x1_sma_op;
> > +	bp->signals_nr = 4;
> > +	bp->freq_in_nr = 4;
> > +
> > +	version = ioread32(&bp->image->version);
> > +	/* if lower 16 bits are empty, this is the fw loader. */
> > +	if ((version & 0xffff) == 0) {
> > +		version = version >> 16;
> > +		bp->fw_loader = true;
> > +	}
> > +	bp->fw_tag = 3;
> > +	bp->fw_version = version & 0xffff;
> > +	bp->fw_cap = OCP_CAP_BASIC | OCP_CAP_SIGNAL | OCP_CAP_FREQ;
> > +
> > +	ptp_ocp_tod_init(bp);
> > +	ptp_ocp_nmea_out_init(bp);
> > +	ptp_ocp_signal_init(bp);
> > +
> > +	err = ptp_ocp_attr_group_add(bp, adva_timecard_x1_groups);
> > +	if (err)
> > +		return err;
> > +
> > +	err = ptp_ocp_set_pins(bp);
> > +	if (err)
> > +		return err;
> > +	ptp_ocp_sma_init(bp);
> > +
> > +	return ptp_ocp_init_clock(bp, r->extra);
> > +}
>
> Hi Sagi,
>
> I see similar patterns elsewhere in this driver. So I assume that I'm
> overlooking something obvious. But it strikes me that
> ptp_ocp_attr_group_add() and ptp_ocp_set_pins() allocate memory. Shouldn't
> that be cleaned up if an error subsequently occurs in this function?
>
> And also, shouldn't ptp_ocp_attr_group_add() relase the memory it
> allocates if an error occurs in that function?

Well, ptp_ocp_attr_group_add() error is not critical for device init, so
I believe it's ok to keep it and free in ptp_ocp_detach(), there is no
leak. But I agree it goes against common code style of kernel. I'll do
cleanup patch once I have some cycles.

Situation with ptp_ocp_set_pins() is a bit different, but still we end
up with calling ptp_ocp_detach() in error path, which cleans up all
allocated memory. Changing *init() code to unroll everything is actually
a code duplication of ptp_ocp_detach() - I don't think we have to spend
time it.

> Lastly, the AI generated code review suggests that there may be
> some scope for sharing code:
>
>    This isn't a bug, but ptp_ocp_adva_board_x1_init() is nearly identical to
>    ptp_ocp_adva_board_init(), differing only in flash_start (0x1000000 vs
>    0xA00000), sma_op assignment, signals_nr (4 vs 2), freq_in_nr (4 vs 2),
>    and the attr group passed to ptp_ocp_attr_group_add(). The version-reading
>    logic and the entire initialization call sequence are duplicated.

That's what I also asked in my response.

>    Looking at the existing driver convention, each board variant (fb, art,
>    adva) uses this same pattern with separate init functions. Would it be
>    worth parameterizing these differences in a future cleanup?
>
> ...