Re: [PATCH v12 07/11] iio: frequency: adf41513: driver implementation

[Jonathan Cameron <jic23@kernel.org>]

> >   
> > > +static int adf41513_set_frequency(struct adf41513_state *st, u64 freq_uhz, u16 sync_mask)
> > > +{
> > > +	struct adf41513_pll_settings result;
> > > +	int ret;
> > > +
> > > +	ret = adf41513_calc_pll_settings(st, &result, freq_uhz);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	/* apply computed results to pll settings */
> > > +	st->settings = result;  
> > 
> > [ ... ]
> >   
> > > +	if (st->settings.mode == ADF41513_MODE_INTEGER_N) {
> > > +		st->regs[ADF41513_REG6] |= ADF41513_REG6_INT_MODE_MSK;
> > > +		st->regs[ADF41513_REG6] &= ~ADF41513_REG6_BLEED_ENABLE_MSK;
> > > +	} else {
> > > +		st->regs[ADF41513_REG6] &= ~ADF41513_REG6_INT_MODE_MSK;
> > > +		st->regs[ADF41513_REG6] |= ADF41513_REG6_BLEED_ENABLE_MSK;
> > > +	}
> > > +
> > > +	return adf41513_sync_config(st, sync_mask | ADF41513_SYNC_REG0);
> > > +}  
> > 
> > If the adf41513_sync_config() SPI write fails, st->settings and st->regs
> > have already been updated with the new configuration.
> > 
> > Will a subsequent read of the frequency channel via adf41513_pll_get_rate()
> > return this failed frequency instead of the actual physical hardware frequency?  
> 
> Not seeing much problem with that. Reversing the state on every spi write failure
> would be too much effort for an error that is already reported to the user.
> Most likely another spi write attempt might fail as well, so would it be a platform
> problem?
> 

Agreed.  It is fair that sashikio raises it as a concern to look at but
as you say unwinding state like this on an spi error is fiddly. I think it's
only necessary if the device ends up wedged as a result.  As long as we can just
try rewriting the original thing that triggered this flow, not a problem.

What to do in cases like when we get errors during buffered accesses is more
of a black art. I'm not inclined to spend much effort and greater code complexity
to harden against those - the way out can sometimes be unbind/rebind the driver.


> > [ ... ]
> >   
> > > +static int adf41513_write_raw(struct iio_dev *indio_dev,
> > > +			      struct iio_chan_spec const *chan,
> > > +			      int val, int val2, long info)
> > > +{  
> > 
> > [ ... ]
> >   
> > > +	case IIO_CHAN_INFO_PHASE:
> > > +		if (tmp64 >= ADF41513_MAX_PHASE_MICRORAD)
> > > +			return -EINVAL;
> > > +
> > > +		phase_val = DIV_U64_ROUND_CLOSEST(tmp64 << 12,
> > > +						  ADF41513_MAX_PHASE_MICRORAD);
> > > +		phase_val = min(phase_val, ADF41513_MAX_PHASE_VAL);
> > > +		st->regs[ADF41513_REG2] |= ADF41513_REG2_PHASE_ADJ_MSK;
> > > +		FIELD_MODIFY(ADF41513_REG2_PHASE_VAL_MSK,
> > > +			     &st->regs[ADF41513_REG2], phase_val);
> > > +		return adf41513_sync_config(st, ADF41513_SYNC_REG0);  
> > 
> > The ADF41513_REG2_PHASE_ADJ_MSK bit is set in the software shadow register,
> > but does not appear to be cleared after the shift completes.
> > 
> > Because the hardware evaluates the PHASE_ADJ bit in REG2 on every REG0 write,
> > could this cause unintended phase shifts during future frequency updates?  
> 
> We don't need to clear that, if phase adjust is set then the phase shift is going
> to be applied. There is no phase accumulation or side effects. I think the
> datasheet could improve on that, but later on it says:
> 
> 	The phase word in Register 2 controls the RF output phase. As this
> 	word is changed from 0 to 2^12, the RF output phase changes over a
> 	360° range in steps of phase value × 360°/2^12.

Bit of a running theme that AI bots sometime point out unclear datasheet
text ;)  I know folk were using them to scan specifications looking for this
sort of stuff where it's possible humans would also get them wrong.

> 
> > [ ... ]
> >   
> > > +static int adf41513_pm_resume(struct device *dev)
> > > +{
> > > +	struct adf41513_state *st = dev_get_drvdata(dev);
> > > +
> > > +	guard(mutex)(&st->lock);
> > > +	if (st->powerdown)
> > > +		return 0; /* nothing to do */
> > > +
> > > +	return adf41513_resume(st);
> > > +}  
> > 
> > If the system cuts power to the regulators during sleep, the chip wakes up
> > in its default Power-On Reset (POR) state.
> > 
> > If st->powerdown is true, this function returns early and st->regs_hw is
> > not invalidated.
> > 
> > Could this leave the software state out of sync with the physical hardware,
> > causing future partial writes to leave the chip unconfigured?  
> 
> That is not an issue because partial writes would not power the device.
> Also the resume() function makes sure to write all registers so a power-up
> sequence is completed.

Makes sense.  Thanks for addressing all these nice and clearly!  Helps me
a lot.  Whilst sashikio is great, dealing with everything it reports is
making reviews take a lot longer.

Jonathan

> 
> > -- 
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260510-adf41513-iio-driver-v12-0-34af2ed2779f@analog.com?part=7  
>