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

[Jonathan Cameron <jic23@kernel.org>]

> > +static int adf41513_pm_suspend(struct device *dev)
> > +{
> > +	return adf41513_suspend(dev_get_drvdata(dev));
> > +}  
> 
> 	Is it safe to call adf41513_suspend() without acquiring st->lock?
> 	This function modifies the shared software register cache and executes SPI
> 	writes. It seems this could race concurrently with sysfs reads/writes or
> 	IIO core accesses.
> 
> Is this a real concern?

Sadly I think this is correct. Nothing stops suspend racing with an ongoing sysfs access.

> 
> ...
> 
> > +static int adf41513_probe(struct spi_device *spi)
> > +{  
> 
> ...
> 
> > +	st->chip_enable = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(st->chip_enable))
> > +		return dev_err_probe(dev, PTR_ERR(st->chip_enable),
> > +				     "fail to request chip enable GPIO\n");  
> 
> ...
> 
> > +	ret = adf41513_setup(st);  
> 
> 	Is a delay needed between asserting the chip_enable GPIO and sending the first
> 	SPI command in adf41513_setup()?
> 	Hardware PLLs typically require a wake-up or stabilization time for internal
> 	regulators. Without a usleep_range(), could the initial SPI commands be
> 	dropped by the hardware?
> 
> I havent see the need for this, mostly because a lot seems to be happening before the
> first SPI write and after the chip enable goes high, so I will ignore this.

If we do have a documented time it would be better to sleep just to be sure.

> 
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to setup device\n");
> > +
> > +	ret = devm_add_action_or_reset(dev, adf41513_power_down, st);  
> 
> 	If adf41513_setup() returns an error, the probe function aborts before
> 	devm_add_action_or_reset() registers the adf41513_power_down action.
> 	Since chip_enable is acquired with GPIOD_OUT_HIGH, devres will disable the
> 	regulators but leave the GPIO high. Could this cause the host processor to
> 	backpower the unpowered synthesizer IC through its ESD diodes?
> 
> Another good point, will separate the reset actions for sw powerdown and chip enable
> gpio.
>