Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Jonathan Lemon <jonathan.lemon@gmail.com>]

On Fri, Jun 24, 2022 at 12:11:43AM +0100, Vadim Fedorenko wrote:
> On 23.06.2022 19:28, Jonathan Lemon wrote:
> > On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
> > > From: Vadim Fedorenko <vadfed@fb.com>
> > > +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
> > > +{
> > > +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> > > +	int ret;
> > > +
> > > +	if (bp->sma[sma].mode != SMA_MODE_IN)
> > > +		return -1;
> > > +
> > > +	switch (ptp_ocp_sma_get(bp, sma)) {
> > > +	case 0:
> > > +		ret = DPLL_TYPE_EXT_10MHZ;
> > > +		break;
> > > +	case 1:
> > > +	case 2:
> > > +		ret = DPLL_TYPE_EXT_1PPS;
> > > +		break;
> > > +	default:
> > > +		ret = DPLL_TYPE_INT_OSCILLATOR;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > 
> > These case statements switch on private bits.  This needs to match
> > on the selector name instead.
> > 
> 
> Not sure that string comparison is a good idea. Maybe it's better to extend
> struct ocp_selector with netlink type id and fill it according to hardware?

Sure, that could be an option.  But, as this is DPLL only, how does it
handle things when a pin is used for non-clock IO?  In the timecard,
for example, we have the frequency counters for input, and the frequency
generators/VCC/GND for output.

Actually our HW has a multi-level input, where the DPLL selects its
source from an internal mux - this isn't reflected here.  The external
pins feed into some complex HW logic, which performs its own priority
calculations before presenting the end result as an available selection
for the DPLL.
-- 
Jonathan