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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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