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

[Simon Horman <horms@kernel.org>]

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