Re: [PATCH v4 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[Jonathan Cameron]

On Fri, 17 Oct 2025 11:01:43 +0200
Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:

> On 9/20/25 11:27, Jonathan Cameron wrote:
> 
> [ ... ]
> 
> >> +static void nxp_sar_adc_remove(struct platform_device *pdev)
> >> +{
> >> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> >> +
> >> +	nxp_sar_adc_stop_conversion(info);  
> > 
> > I assume this is safe even if no start has happened and is here so
> > that the driver remove can run whilst buffered capture is still going on?
> > That should be done by the core as part of unwinding the register().
> > So I'd not expect a need for it here. This may be a side effect of the
> > ordering issue mixing devm and not causes.  The same is true of some
> > of these other calls - I haven't checked them all.
> >   
> >> +	nxp_sar_adc_channels_disable(info, NXP_SAR_ADC_CH_MASK);
> >> +	nxp_sar_adc_dma_channels_disable(info, NXP_SAR_ADC_CH_MASK);
> >> +	nxp_sar_adc_dma_cfg(info, false);
> >> +	nxp_sar_adc_disable(info);
> >> +	dmaengine_terminate_sync(info->dma_chan);  
> > 
> > Mixing devm and non devm is a never a good idea. Here one possible issue is that
> > the userspace interfaces are only torn down when devm unwind gets to unwind
> > devm_iio_device_register();  That happens well after this code has ripped down the
> > dma engine that a channel read will try to use.  It might be safe to do that
> > but it certainly makes the driver harder to reason about.
> > 
> > A simple 'rule' is once you first call a non devm function in probe that needs unwinding
> > in remove, you cannot call any more devm functions.   Whilst there are lots of cases
> > that are safe, this approach ensures none of the cases that aren't sneak in and makes
> > review straight forward.
> > 
> > devm_add_action_or_reset() can often be used to keep the chain of devm calls running,
> > sometimes covering everything such that we don't need a remove callback.
> >   
> >> +}  
> 
> Actually I think these calls are not relevant. If we remove the 
> nxp_sar_adc_remove() function, the iio core code will unregister the device.
> 
> All operations are doing on/off in the different callbacks (raw_read, 
> postenable, predisable). When the device is unregistered it is not 
> possible to have an ongoing conversion, a channel enabled or the adc 
> enabled, as well as the DMA. IINW, we can just remove this block of code.

Great.

> 
> [ ... ]
> 
> 
>