Re: [PATCH] iio: adc: at91-sama5d2_adc: merge buffer & trigger init into a function

[Jonathan Cameron]

On Thu, 24 Sep 2020 13:29:02 +0300
Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:

> This change is mostly cosmetic, but it's also a pre-cursor to the
> the change for 'iio_buffer_set_attrs()', where the helper gets updated to
> better support multiple IIO buffers for 1 IIO device.
> 
> The only functional change is that the error message for the trigger alloc
> failure is bound to the parent device vs the IIO device object.
> 
> Also, the new at91_adc_buffer_and_trigger_init() function was moved after
> the definition of the 'at91_adc_fifo_attributes'.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>

Looks fine to me, but will let this sit on the list for a while
so others have time to comment.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 78 ++++++++++++++----------------
>  1 file changed, 36 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index ad7d9819f83c..b9c3cc6d5913 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1014,21 +1014,6 @@ static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
>  
>  	return trig;
>  }
> -
> -static int at91_adc_trigger_init(struct iio_dev *indio)
> -{
> -	struct at91_adc_state *st = iio_priv(indio);
> -
> -	st->trig = at91_adc_allocate_trigger(indio, st->selected_trig->name);
> -	if (IS_ERR(st->trig)) {
> -		dev_err(&indio->dev,
> -			"could not allocate trigger\n");
> -		return PTR_ERR(st->trig);
> -	}
> -
> -	return 0;
> -}
> -
>  static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
>  					   struct iio_poll_func *pf)
>  {
> @@ -1156,13 +1141,6 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> -static int at91_adc_buffer_init(struct iio_dev *indio)
> -{
> -	return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> -		&iio_pollfunc_store_time,
> -		&at91_adc_trigger_handler, &at91_buffer_setup_ops);
> -}
> -
>  static unsigned at91_adc_startup_time(unsigned startup_time_min,
>  				      unsigned adc_clk_khz)
>  {
> @@ -1683,6 +1661,40 @@ static const struct iio_info at91_adc_info = {
>  	.hwfifo_set_watermark = &at91_adc_set_watermark,
>  };
>  
> +static int at91_adc_buffer_and_trigger_init(struct device *dev,
> +					    struct iio_dev *indio)
> +{
> +	struct at91_adc_state *st = iio_priv(indio);
> +	int ret;
> +
> +	ret = devm_iio_triggered_buffer_setup(&indio->dev, indio,
> +		&iio_pollfunc_store_time,
> +		&at91_adc_trigger_handler, &at91_buffer_setup_ops);
> +	if (ret < 0) {
> +		dev_err(dev, "couldn't initialize the buffer.\n");
> +		return ret;
> +	}
> +
> +	if (!st->selected_trig->hw_trig)
> +		return 0;
> +
> +	iio_buffer_set_attrs(indio->buffer, at91_adc_fifo_attributes);
> +
> +	st->trig = at91_adc_allocate_trigger(indio, st->selected_trig->name);
> +	if (IS_ERR(st->trig)) {
> +		dev_err(dev, "could not allocate trigger\n");
> +		return PTR_ERR(st->trig);
> +	}
> +
> +	/*
> +	 * Initially the iio buffer has a length of 2 and
> +	 * a watermark of 1
> +	 */
> +	st->dma_st.watermark = 1;
> +
> +	return 0;
> +}
> +
>  static int at91_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
> @@ -1818,27 +1830,9 @@ static int at91_adc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
> -	ret = at91_adc_buffer_init(indio_dev);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "couldn't initialize the buffer.\n");
> +	ret = at91_adc_buffer_and_trigger_init(&pdev->dev, indio_dev);
> +	if (ret < 0)
>  		goto per_clk_disable_unprepare;
> -	}
> -
> -	if (st->selected_trig->hw_trig) {
> -		ret = at91_adc_trigger_init(indio_dev);
> -		if (ret < 0) {
> -			dev_err(&pdev->dev, "couldn't setup the triggers.\n");
> -			goto per_clk_disable_unprepare;
> -		}
> -		/*
> -		 * Initially the iio buffer has a length of 2 and
> -		 * a watermark of 1
> -		 */
> -		st->dma_st.watermark = 1;
> -
> -		iio_buffer_set_attrs(indio_dev->buffer,
> -				     at91_adc_fifo_attributes);
> -	}
>  
>  	if (dma_coerce_mask_and_coherent(&indio_dev->dev, DMA_BIT_MASK(32)))
>  		dev_info(&pdev->dev, "cannot set DMA mask to 32-bit\n");