Re: [PATCH v2 3/6] iio: adc: Add support for STM32 ADC

[Fabrice Gasnier]

On 11/14/2016 01:11 PM, Lars-Peter Clausen wrote:
> On 11/10/2016 05:18 PM, Fabrice Gasnier wrote:
> [...]
> > + static int stm32_adc_single_conv(struct iio_dev *indio_dev,
> > +				 const struct iio_chan_spec *chan,
> > +				 int *res)
> > +{
> > +	struct stm32_adc *adc = iio_priv(indio_dev);
> > +	long timeout;
> > +	u32 val;
> > +	u16 result;
> > +	int ret;
> > +
> > +	reinit_completion(&adc->completion);
> > +
> > +	adc->buffer = &result;
> > +
> > +	/* Program chan number in regular sequence */
> > +	val = stm32_adc_readl(adc, STM32F4_ADCX_SQR3);
> > +	val &= ~STM32F4_SQ1_MASK;
> > +	val |= chan->channel << STM32F4_SQ1_SHIFT;
> > +	stm32_adc_writel(adc, STM32F4_ADCX_SQR3, val);
> > +
> > +	/* Set regular sequence len (0 for 1 conversion) */
> > +	stm32_adc_clr_bits(adc, STM32F4_ADCX_SQR1, STM32F4_L_MASK);
> > +
> > +	/* Trigger detection disabled (conversion can be launched in SW) */
> > +	stm32_adc_clr_bits(adc, STM32F4_ADCX_CR2, STM32F4_EXTEN_MASK);
> > +
> > +	stm32_adc_conv_irq_enable(adc);
> > +
> > +	stm32_adc_start_conv(adc);
> > +
> > +	timeout = wait_for_completion_interruptible_timeout(
> > +					&adc->completion, STM32_ADC_TIMEOUT);
> > +	if (timeout == 0) {
> > +		dev_warn(&indio_dev->dev, "Conversion timed out!\n");
> This should be dev_dbg() at most. This out of band reporting is not
> particular useful for applications as it is impossible to match the error to
> the action that triggered it. And you also report the error through the
> error code, so the applications knows what is going on.
>
> > +		ret = -ETIMEDOUT;
> > +	} else if (timeout < 0) {
> > +		dev_warn(&indio_dev->dev, "Interrupted conversion!\n");
> > +		ret = -EINTR;
> This should just propagate the error returned by wait_for_completion...().
> This will make sure that the right behavior occurs based on the SA_RESTART
> policy.
Hi Lars,

Thanks for reviewing.
I'll update this in next revision.

Regards,
Fabrice

> > +	} else {
> > +		*res = result;
> > +		ret = IIO_VAL_INT;
> > +	}
> > +
> > +	stm32_adc_stop_conv(adc);
> > +
> > +	stm32_adc_conv_irq_disable(adc);
> > +
> > +	return ret;