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

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

[Sagi Maimon]

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@gmail.com>
---
 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
index d88ab2f86b1b..1d0a06f82281 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -35,6 +35,7 @@
 
 #define PCI_VENDOR_ID_ADVA			0xad5a
 #define PCI_DEVICE_ID_ADVA_TIMECARD		0x0400
+#define PCI_DEVICE_ID_ADVA_TIMECARD_X1	0x0410
 
 static struct class timecard_class = {
 	.name		= "timecard",
@@ -420,12 +421,16 @@ static int ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r);
 
 static int ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r);
 
+static int ptp_ocp_adva_board_x1_init(struct ptp_ocp *bp, struct ocp_resource *r);
+
 static const struct ocp_attr_group fb_timecard_groups[];
 
 static const struct ocp_attr_group art_timecard_groups[];
 
 static const struct ocp_attr_group adva_timecard_groups[];
 
+static const struct ocp_attr_group adva_timecard_x1_groups[];
+
 struct ptp_ocp_eeprom_map {
 	u16	off;
 	u16	len;
@@ -1030,11 +1035,212 @@ static struct ocp_resource ocp_adva_resource[] = {
 	{ }
 };
 
+static struct ocp_resource ocp_adva_x1_resource[] = {
+	{
+		OCP_MEM_RESOURCE(reg),
+		.offset = 0x01000000, .size = 0x10000,
+	},
+	{
+		OCP_EXT_RESOURCE(ts0),
+		.offset = 0x01010000, .size = 0x10000, .irq_vec = 1,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 0,
+			.irq_fcn = ptp_ocp_ts_irq,
+			.enable = ptp_ocp_ts_enable,
+		},
+	},
+	{
+		OCP_EXT_RESOURCE(ts1),
+		.offset = 0x01020000, .size = 0x10000, .irq_vec = 2,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 1,
+			.irq_fcn = ptp_ocp_ts_irq,
+			.enable = ptp_ocp_ts_enable,
+		},
+	},
+	{
+		OCP_EXT_RESOURCE(ts2),
+		.offset = 0x01060000, .size = 0x10000, .irq_vec = 6,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 2,
+			.irq_fcn = ptp_ocp_ts_irq,
+			.enable = ptp_ocp_ts_enable,
+		},
+	},
+	{
+		OCP_EXT_RESOURCE(ts3),
+		.offset = 0x01110000, .size = 0x10000, .irq_vec = 15,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 3,
+			.irq_fcn = ptp_ocp_ts_irq,
+			.enable = ptp_ocp_ts_enable,
+		},
+	},
+	{
+		OCP_EXT_RESOURCE(ts4),
+		.offset = 0x01120000, .size = 0x10000, .irq_vec = 16,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 4,
+			.irq_fcn = ptp_ocp_ts_irq,
+			.enable = ptp_ocp_ts_enable,
+		},
+	},
+	/* Timestamp for PHC and/or PPS generator */
+	{
+		OCP_EXT_RESOURCE(pps),
+		.offset = 0x010C0000, .size = 0x10000, .irq_vec = 0,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 5,
+			.irq_fcn = ptp_ocp_ts_irq,
+			.enable = ptp_ocp_ts_enable,
+		},
+	},
+	{
+		OCP_EXT_RESOURCE(signal_out[0]),
+		.offset = 0x010D0000, .size = 0x10000, .irq_vec = 11,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 1,
+			.irq_fcn = ptp_ocp_signal_irq,
+			.enable = ptp_ocp_signal_enable,
+		},
+	},
+	{
+		OCP_EXT_RESOURCE(signal_out[1]),
+		.offset = 0x010E0000, .size = 0x10000, .irq_vec = 12,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 2,
+			.irq_fcn = ptp_ocp_signal_irq,
+			.enable = ptp_ocp_signal_enable,
+		},
+	},
+	{
+		OCP_EXT_RESOURCE(signal_out[2]),
+		.offset = 0x010F0000, .size = 0x10000, .irq_vec = 13,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 3,
+			.irq_fcn = ptp_ocp_signal_irq,
+			.enable = ptp_ocp_signal_enable,
+		},
+	},
+	{
+		OCP_EXT_RESOURCE(signal_out[3]),
+		.offset = 0x01100000, .size = 0x10000, .irq_vec = 14,
+		.extra = &(struct ptp_ocp_ext_info) {
+			.index = 4,
+			.irq_fcn = ptp_ocp_signal_irq,
+			.enable = ptp_ocp_signal_enable,
+		},
+	},
+	{
+		OCP_MEM_RESOURCE(pps_to_ext),
+		.offset = 0x01030000, .size = 0x10000,
+	},
+	{
+		OCP_MEM_RESOURCE(pps_to_clk),
+		.offset = 0x01040000, .size = 0x10000,
+	},
+	{
+		OCP_MEM_RESOURCE(tod),
+		.offset = 0x01050000, .size = 0x10000,
+	},
+	{
+		OCP_MEM_RESOURCE(image),
+		.offset = 0x00020000, .size = 0x1000,
+	},
+	{
+		OCP_MEM_RESOURCE(pps_select),
+		.offset = 0x00130000, .size = 0x1000,
+	},
+	{
+		OCP_MEM_RESOURCE(sma_map1),
+		.offset = 0x00140000, .size = 0x1000,
+	},
+	{
+		OCP_MEM_RESOURCE(sma_map2),
+		.offset = 0x00220000, .size = 0x1000,
+	},
+	{
+		OCP_SERIAL_RESOURCE(port[PORT_GNSS]),
+		.offset = 0x00160000 + 0x1000, .irq_vec = 3,
+		.extra = &(struct ptp_ocp_serial_port) {
+			.baud = 9600,
+		},
+	},
+	{
+		OCP_SERIAL_RESOURCE(port[PORT_MAC]),
+		.offset = 0x00180000 + 0x1000, .irq_vec = 5,
+		.extra = &(struct ptp_ocp_serial_port) {
+			.baud = 115200,
+		},
+	},
+	{
+		OCP_MEM_RESOURCE(freq_in[0]),
+		.offset = 0x01200000, .size = 0x10000,
+	},
+	{
+		OCP_MEM_RESOURCE(freq_in[1]),
+		.offset = 0x01210000, .size = 0x10000,
+	},
+	{
+		OCP_MEM_RESOURCE(freq_in[2]),
+		.offset = 0x01220000, .size = 0x10000,
+	},
+	{
+		OCP_MEM_RESOURCE(freq_in[3]),
+		.offset = 0x01230000, .size = 0x10000,
+	},
+	{
+		OCP_SPI_RESOURCE(spi_flash),
+		.offset = 0x00310000, .size = 0x10000, .irq_vec = 9,
+		.extra = &(struct ptp_ocp_flash_info) {
+			.name = "xilinx_spi", .pci_offset = 0,
+			.data_size = sizeof(struct xspi_platform_data),
+			.data = &(struct xspi_platform_data) {
+				.num_chipselect = 1,
+				.bits_per_word = 8,
+				.num_devices = 1,
+				.force_irq = true,
+				.devices = &(struct spi_board_info) {
+					.modalias = "spi-nor",
+				},
+			},
+		},
+	},
+	{
+		OCP_I2C_RESOURCE(i2c_ctrl),
+		.offset = 0x00150000, .size = 0x10000, .irq_vec = 7,
+		.extra = &(struct ptp_ocp_i2c_info) {
+			.name = "xiic-i2c",
+			.fixed_rate = 50000000,
+			.data_size = sizeof(struct xiic_i2c_platform_data),
+			.data = &(struct xiic_i2c_platform_data) {
+				.num_devices = 2,
+				.devices = (struct i2c_board_info[]) {
+					{ I2C_BOARD_INFO("24c02", 0x50) },
+					{ I2C_BOARD_INFO("24mac402", 0x58),
+					  .platform_data = "mac" },
+				},
+			},
+		},
+	},
+	{
+		.setup = ptp_ocp_adva_board_x1_init,
+		.extra = &(struct ptp_ocp_servo_conf) {
+			.servo_offset_p = 0xc000,
+			.servo_offset_i = 0x1000,
+			.servo_drift_p = 0,
+			.servo_drift_i = 0,
+		},
+	},
+	{ }
+};
+
 static const struct pci_device_id ptp_ocp_pcidev_id[] = {
 	{ PCI_DEVICE_DATA(META, TIMECARD, &ocp_fb_resource) },
 	{ PCI_DEVICE_DATA(CELESTICA, TIMECARD, &ocp_fb_resource) },
 	{ PCI_DEVICE_DATA(OROLIA, ARTCARD, &ocp_art_resource) },
 	{ PCI_DEVICE_DATA(ADVA, TIMECARD, &ocp_adva_resource) },
+	{ PCI_DEVICE_DATA(ADVA, TIMECARD_X1, &ocp_adva_x1_resource) },
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, ptp_ocp_pcidev_id);
@@ -1137,6 +1343,34 @@ static const struct ocp_selector ptp_ocp_adva_sma_out[] = {
 	{ }
 };
 
+static const struct ocp_selector ptp_ocp_adva_x1_sma_in[] = {
+	{ .name = "PPS1",	.value = 0x0001,      .frequency = 1 },
+	{ .name = "TS1",	.value = 0x0004,      .frequency = 0 },
+	{ .name = "TS2",	.value = 0x0008,      .frequency = 0 },
+	{ .name = "TS3",    .value = 0x0040,      .frequency = 0 },
+	{ .name = "TS4",    .value = 0x0080,      .frequency = 0 },
+	{ .name = "FREQ1",	.value = 0x0100,      .frequency = 0 },
+	{ .name = "FREQ2",	.value = 0x0200,      .frequency = 0 },
+	{ .name = "FREQ3",  .value = 0x0400,      .frequency = 0 },
+	{ .name = "FREQ4",  .value = 0x0800,      .frequency = 0 },
+	{ .name = "None",	.value = SMA_DISABLE, .frequency = 0 },
+	{ }
+};
+
+static const struct ocp_selector ptp_ocp_adva_x1_sma_out[] = {
+	{ .name = "10Mhz",	.value = 0x0000,  .frequency = 10000000},
+	{ .name = "PHC",	.value = 0x0001,  .frequency = 1 },
+	{ .name = "MAC",	.value = 0x0002,  .frequency = 1 },
+	{ .name = "GNSS1",	.value = 0x0004,  .frequency = 1 },
+	{ .name = "GEN1",	.value = 0x0040 },
+	{ .name = "GEN2",	.value = 0x0080 },
+	{ .name = "GEN3",	.value = 0x0100 },
+	{ .name = "GEN4",	.value = 0x0200 },
+	{ .name = "GND",	.value = 0x2000 },
+	{ .name = "VCC",	.value = 0x4000 },
+	{ }
+};
+
 struct ocp_sma_op {
 	const struct ocp_selector *tbl[2];
 	void (*init)(struct ptp_ocp *bp);
@@ -2639,6 +2873,14 @@ static const struct ocp_sma_op ocp_adva_sma_op = {
 	.set_output	= ptp_ocp_sma_adva_set_output,
 };
 
+static const struct ocp_sma_op ocp_adva_x1_sma_op = {
+	.tbl		= { ptp_ocp_adva_x1_sma_in, ptp_ocp_adva_x1_sma_out },
+	.init		= ptp_ocp_sma_fb_init,
+	.get		= ptp_ocp_sma_fb_get,
+	.set_inputs	= ptp_ocp_sma_adva_set_inputs,
+	.set_output	= ptp_ocp_sma_adva_set_output,
+};
+
 static int
 ptp_ocp_set_pins(struct ptp_ocp *bp)
 {
@@ -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);
+}
+
 static ssize_t
 ptp_ocp_show_output(const struct ocp_selector *tbl, u32 val, char *buf,
 		    int def_val)
@@ -3982,6 +4263,43 @@ static const struct ocp_attr_group adva_timecard_groups[] = {
 	{ },
 };
 
+static struct attribute *adva_timecard_x1_attrs[] = {
+	&dev_attr_serialnum.attr,
+	&dev_attr_gnss_sync.attr,
+	&dev_attr_clock_source.attr,
+	&dev_attr_available_clock_sources.attr,
+	&dev_attr_sma1.attr,
+	&dev_attr_sma2.attr,
+	&dev_attr_sma3.attr,
+	&dev_attr_sma4.attr,
+	&dev_attr_available_sma_inputs.attr,
+	&dev_attr_available_sma_outputs.attr,
+	&dev_attr_clock_status_drift.attr,
+	&dev_attr_clock_status_offset.attr,
+	&dev_attr_ts_window_adjust.attr,
+	&dev_attr_utc_tai_offset.attr,
+	&dev_attr_tod_correction.attr,
+	NULL,
+};
+
+static const struct attribute_group adva_timecard_x1_group = {
+	.attrs = adva_timecard_x1_attrs,
+};
+
+static const struct ocp_attr_group adva_timecard_x1_groups[] = {
+	{ .cap = OCP_CAP_BASIC,	    .group = &adva_timecard_x1_group },
+	{ .cap = OCP_CAP_BASIC,	    .group = &ptp_ocp_timecard_tty_group },
+	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal0_group },
+	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal1_group },
+	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal2_group },
+	{ .cap = OCP_CAP_SIGNAL,    .group = &fb_timecard_signal3_group },
+	{ .cap = OCP_CAP_FREQ,	    .group = &fb_timecard_freq0_group },
+	{ .cap = OCP_CAP_FREQ,	    .group = &fb_timecard_freq1_group },
+	{ .cap = OCP_CAP_FREQ,	    .group = &fb_timecard_freq2_group },
+	{ .cap = OCP_CAP_FREQ,	    .group = &fb_timecard_freq3_group },
+	{ },
+};
+
 static void
 gpio_input_map(char *buf, struct ptp_ocp *bp, u16 map[][2], u16 bit,
 	       const char *def)
-- 
2.47.0

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

[Vadim Fedorenko]

On 25/02/2026 10:29, 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.

What are exact differences between this card and OCP design which is
also Xilinx-based?

AFAICS, it's only flash base address and fw tag is different, which can
be moved to resource init part.

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@gmail.com>
> ---
>  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?

...

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@gmail.com>
>> ---
>>   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?
> 
> ...

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

[Simon Horman]

On Thu, Feb 26, 2026 at 01:42:31PM +0000, Vadim Fedorenko wrote:
> 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@gmail.com>
> > > ---
> > >   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.

Thanks. I now see that ptp_ocp_detach() is called which handles cleaning
up these resources on error. So I agree there is no leak.

> > 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.

Yes, sorry for missing that.

> >    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?
> > 
> > ...