Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev() helper

[Jonathan Cameron]

On Mon, 25 May 2020 13:53:41 +0300
Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:

> We may want to get rid of the iio_priv_to_dev() helper. The reason is that
> we will hide some of the members of the iio_dev structure (to prevent
> drivers from accessing them directly), and that will also mean hiding the
> implementation of the iio_priv_to_dev() helper inside the IIO core.
> 
> Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
> may not be fast anymore, so a general idea is to try to get rid of the
> iio_priv_to_dev() altogether.
> The iio_priv() helper won't be affected by the rework, as the iio_dev
> struct will keep a reference to the private information.
> 
> For this driver, not using iio_priv_to_dev(), means reworking some paths to
> pass the iio device and using iio_priv() to access the private information,
> and also keeping a reference to the iio device for some quirky paths.
> 
> One [quirky] path is the at91_adc_workq_handler() which requires the IIO
> device & the state struct to push to buffers.
> Since this requires the back-ref to the IIO device, the
> at91_adc_touch_pos() also uses it. This simplifies the patch a bit. The
> information required in this function is mostly for debugging purposes.
> Replacing it with a reference to the IIO device would have been a slightly
> bigger change, which may not be worth it (for just the debugging purpose
> and given that we need the back-ref to the IIO device anyway).

That workq is indeed quirky.  This looks fine to me in general. I'll
want an appropriate ack from the at91 side of things if possible so
let's leave this on the list for a while longer.

Poke me if no action in a few weeks.

Thanks,

Jonathan

> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 9abbbdcc7420..7bce18444430 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -402,6 +402,7 @@ struct at91_adc_state {
>  	wait_queue_head_t		wq_data_available;
>  	struct at91_adc_dma		dma_st;
>  	struct at91_adc_touch		touch_st;
> +	struct iio_dev			*indio_dev;
>  	u16				buffer[AT91_BUFFER_MAX_HWORDS];
>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
> @@ -642,13 +643,13 @@ static u16 at91_adc_touch_pos(struct at91_adc_state *st, int reg)
>  	/* first half of register is the x or y, second half is the scale */
>  	val = at91_adc_readl(st, reg);
>  	if (!val)
> -		dev_dbg(&iio_priv_to_dev(st)->dev, "pos is 0\n");
> +		dev_dbg(&st->indio_dev->dev, "pos is 0\n");
>  
>  	pos = val & AT91_SAMA5D2_XYZ_MASK;
>  	result = (pos << AT91_SAMA5D2_MAX_POS_BITS) - pos;
>  	scale = (val >> 16) & AT91_SAMA5D2_XYZ_MASK;
>  	if (scale == 0) {
> -		dev_err(&iio_priv_to_dev(st)->dev, "scale is 0\n");
> +		dev_err(&st->indio_dev->dev, "scale is 0\n");
>  		return 0;
>  	}
>  	result /= scale;
> @@ -1204,9 +1205,9 @@ static unsigned at91_adc_startup_time(unsigned startup_time_min,
>  	return i;
>  }
>  
> -static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
> +static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev, unsigned freq)
>  {
> -	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
>  	unsigned f_per, prescal, startup, mr;
>  
>  	f_per = clk_get_rate(st->per_clk);
> @@ -1275,9 +1276,9 @@ static void at91_adc_pen_detect_interrupt(struct at91_adc_state *st)
>  	st->touch_st.touching = true;
>  }
>  
> -static void at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
> +static void at91_adc_no_pen_detect_interrupt(struct iio_dev *indio_dev)
>  {
> -	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
>  
>  	at91_adc_writel(st, AT91_SAMA5D2_TRGR,
>  			AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER);
> @@ -1297,7 +1298,7 @@ static void at91_adc_workq_handler(struct work_struct *workq)
>  					struct at91_adc_touch, workq);
>  	struct at91_adc_state *st = container_of(touch_st,
>  					struct at91_adc_state, touch_st);
> -	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> +	struct iio_dev *indio_dev = st->indio_dev;
>  
>  	iio_push_to_buffers(indio_dev, st->buffer);
>  }
> @@ -1318,7 +1319,7 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  		at91_adc_pen_detect_interrupt(st);
>  	} else if ((status & AT91_SAMA5D2_IER_NOPEN)) {
>  		/* nopen detected IRQ */
> -		at91_adc_no_pen_detect_interrupt(st);
> +		at91_adc_no_pen_detect_interrupt(indio);
>  	} else if ((status & AT91_SAMA5D2_ISR_PENS) &&
>  		   ((status & rdy_mask) == rdy_mask)) {
>  		/* periodic trigger IRQ - during pen sense */
> @@ -1486,7 +1487,7 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  		    val > st->soc_info.max_sample_rate)
>  			return -EINVAL;
>  
> -		at91_adc_setup_samp_freq(st, val);
> +		at91_adc_setup_samp_freq(indio_dev, val);
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -1624,8 +1625,10 @@ static int at91_adc_update_scan_mode(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> -static void at91_adc_hw_init(struct at91_adc_state *st)
> +static void at91_adc_hw_init(struct iio_dev *indio_dev)
>  {
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
>  	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_SWRST);
>  	at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
>  	/*
> @@ -1635,7 +1638,7 @@ static void at91_adc_hw_init(struct at91_adc_state *st)
>  	at91_adc_writel(st, AT91_SAMA5D2_MR,
>  			AT91_SAMA5D2_MR_TRANSFER(2) | AT91_SAMA5D2_MR_ANACH);
>  
> -	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> +	at91_adc_setup_samp_freq(indio_dev, st->soc_info.min_sample_rate);
>  
>  	/* configure extended mode register */
>  	at91_adc_config_emr(st);
> @@ -1718,6 +1721,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
>  
>  	st = iio_priv(indio_dev);
> +	st->indio_dev = indio_dev;
>  
>  	bitmap_set(&st->touch_st.channels_bitmask,
>  		   AT91_SAMA5D2_TOUCH_X_CHAN_IDX, 1);
> @@ -1829,7 +1833,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  		goto vref_disable;
>  	}
>  
> -	at91_adc_hw_init(st);
> +	at91_adc_hw_init(indio_dev);
>  
>  	ret = clk_prepare_enable(st->per_clk);
>  	if (ret)
> @@ -1945,7 +1949,7 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
>  	if (ret)
>  		goto vref_disable_resume;
>  
> -	at91_adc_hw_init(st);
> +	at91_adc_hw_init(indio_dev);
>  
>  	/* reconfiguring trigger hardware state */
>  	if (!iio_buffer_enabled(indio_dev))