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

[Simon Horman]

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?

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.

  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?

...