[PATCH v2 0/8] iio: core: wrap IIO device into an iio_dev_opaque object

[PATCH v2 0/8] iio: core: wrap IIO device into an iio_dev_opaque object

[Alexandru Ardelean]

This change starts to hide some internal fields of the IIO device into
the framework.

Because the iio_priv_to_dev() will be hidden some pre-work is done to
try to remove it from some interrupt handlers.
iio_priv_to_dev() will become a function call and won't be expandable
into place (as-is now as an inline function).

Changelog v1 -> v2:
- add pre-work patches that remove some calls to iio_priv_to_dev() from
  interrupt handlers
- renamed iio_dev_priv -> iio_dev_opaque
- moved the iio_dev_opaque to 'include/linux/iio/iio-opaque.h' this way
  it should be usable for debugging
- the iio_priv() call, is still an inline function that returns an
  'indio_dev->priv' reference; this field is added to 'struct iio_dev';
  the reference is computed in iio_device_alloc() and should be
  cacheline aligned
- the to_iio_dev_opaque() container is in the
  'include/linux/iio/iio-opaque.h' header; it's still implemented with
  some pointer arithmetic; one idea was to do it via an
  'indio_dev->opaque' field; that may still be an optionif there are
  some opinions in that direction

Alexandru Ardelean (8):
  iio: proximity: ping: pass reference to IIO device via call-stack
  iio: at91-sama5d2_adc: pass ref to IIO device via param for int
    function
  iio: at91_adc: pass ref to IIO device via param for int function
  iio: stm32-dfsdm-adc: pass iio device as arg for the interrupt handler
  iio: stm32-adc: pass iio device as arg for the interrupt handler
  iio: core: wrap IIO device into an iio_dev_opaque object
  iio: core: simplify alloc alignment code
  iio: core: move debugfs data on the private iio dev info

 drivers/iio/adc/at91-sama5d2_adc.c |  7 ++-
 drivers/iio/adc/at91_adc.c         |  5 +-
 drivers/iio/adc/stm32-adc.c        | 10 ++--
 drivers/iio/adc/stm32-dfsdm-adc.c  |  6 +--
 drivers/iio/industrialio-core.c    | 75 ++++++++++++++++++++----------
 drivers/iio/proximity/ping.c       |  5 +-
 include/linux/iio/iio-opaque.h     | 27 +++++++++++
 include/linux/iio/iio.h            | 24 +++-------
 8 files changed, 99 insertions(+), 60 deletions(-)
 create mode 100644 include/linux/iio/iio-opaque.h

-- 
2.17.1

[PATCH v2 1/8] iio: proximity: ping: pass reference to IIO device via call-stack

[Alexandru Ardelean]

Since there will be some changes to how iio_priv_to_dev() is implemented,
it could be that the helper becomes a bit slower, as it will be hidden away
in the IIO core.

For this driver, the IIO device can be passed directly as a parameter to
the ping_read() function, thus making it immune to the change of
iio_priv_to_dev().

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/proximity/ping.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/proximity/ping.c b/drivers/iio/proximity/ping.c
index 12b893c5b0ee..ddc43a5a2ef8 100644
--- a/drivers/iio/proximity/ping.c
+++ b/drivers/iio/proximity/ping.c
@@ -89,14 +89,13 @@ static irqreturn_t ping_handle_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int ping_read(struct ping_data *data)
+static int ping_read(struct iio_dev *indio_dev, struct ping_data *data)
 {
 	int ret;
 	ktime_t ktime_dt;
 	s64 dt_ns;
 	u32 time_ns, distance_mm;
 	struct platform_device *pdev = to_platform_device(data->dev);
-	struct iio_dev *indio_dev = iio_priv_to_dev(data);
 
 	/*
 	 * just one read-echo-cycle can take place at a time
@@ -236,7 +235,7 @@ static int ping_read_raw(struct iio_dev *indio_dev,
 
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
-		ret = ping_read(data);
+		ret = ping_read(indio_dev, data);
 		if (ret < 0)
 			return ret;
 		*val = ret;
-- 
2.17.1

Re: [PATCH v2 1/8] iio: proximity: ping: pass reference to IIO device via call-stack

[Jonathan Cameron]

On Thu, 14 May 2020 16:17:03 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Since there will be some changes to how iio_priv_to_dev() is implemented,
> it could be that the helper becomes a bit slower, as it will be hidden away
> in the IIO core.
> 
> For this driver, the IIO device can be passed directly as a parameter to
> the ping_read() function, thus making it immune to the change of
> iio_priv_to_dev().
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Patch makes sense and I should have spotted this one during original
review :( Going backwards and forwards is never a good idea even without
the rework you have.

But... (you knew that was coming :), there is no need to pass
data to the the read function.  It is the structure returned by
iio_priv() and not used anywhere else in the read_raw callback.
So might as well just pass the iio_dev and get the data structure
with in the read function via iio_priv(indio_dev)

Thanks,

J

> ---
>  drivers/iio/proximity/ping.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/proximity/ping.c b/drivers/iio/proximity/ping.c
> index 12b893c5b0ee..ddc43a5a2ef8 100644
> --- a/drivers/iio/proximity/ping.c
> +++ b/drivers/iio/proximity/ping.c
> @@ -89,14 +89,13 @@ static irqreturn_t ping_handle_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static int ping_read(struct ping_data *data)
> +static int ping_read(struct iio_dev *indio_dev, struct ping_data *data)
>  {
>  	int ret;
>  	ktime_t ktime_dt;
>  	s64 dt_ns;
>  	u32 time_ns, distance_mm;
>  	struct platform_device *pdev = to_platform_device(data->dev);
> -	struct iio_dev *indio_dev = iio_priv_to_dev(data);
>  
>  	/*
>  	 * just one read-echo-cycle can take place at a time
> @@ -236,7 +235,7 @@ static int ping_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
> -		ret = ping_read(data);
> +		ret = ping_read(indio_dev, data);
>  		if (ret < 0)
>  			return ret;
>  		*val = ret;

[PATCH v2 2/8] iio: at91-sama5d2_adc: pass ref to IIO device via param for int function

[Alexandru Ardelean]

Since there will be some changes to how iio_priv_to_dev() is implemented,
it could be that the helper becomes a bit slower, as it will be hidden away
in the IIO core.

For this driver, the IIO device can be passed directly as a parameter to
the at91_adc_no_pen_detect_interrupt() function, thus making it immune to
the change of iio_priv_to_dev().
The function gets called in an interrupt context.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 9abbbdcc7420..822b8782acba 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1275,10 +1275,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 at91_adc_state *st)
 {
-	struct iio_dev *indio_dev = iio_priv_to_dev(st);
-
 	at91_adc_writel(st, AT91_SAMA5D2_TRGR,
 			AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER);
 	at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_NOPEN |
@@ -1318,7 +1317,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, st);
 	} else if ((status & AT91_SAMA5D2_ISR_PENS) &&
 		   ((status & rdy_mask) == rdy_mask)) {
 		/* periodic trigger IRQ - during pen sense */
-- 
2.17.1

Re: [PATCH v2 2/8] iio: at91-sama5d2_adc: pass ref to IIO device via param for int function

[Jonathan Cameron]

On Thu, 14 May 2020 16:17:04 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Since there will be some changes to how iio_priv_to_dev() is implemented,
> it could be that the helper becomes a bit slower, as it will be hidden away
> in the IIO core.
> 
> For this driver, the IIO device can be passed directly as a parameter to
> the at91_adc_no_pen_detect_interrupt() function, thus making it immune to
> the change of iio_priv_to_dev().
> The function gets called in an interrupt context.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Looks sensible to me.   We could get the state structure from the 
iio_dev inside that function, but then it would be different from the
pen version, so I think what you have here is probably best option.

Jonathan

> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 9abbbdcc7420..822b8782acba 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1275,10 +1275,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 at91_adc_state *st)
>  {
> -	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> -
>  	at91_adc_writel(st, AT91_SAMA5D2_TRGR,
>  			AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER);
>  	at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_NOPEN |
> @@ -1318,7 +1317,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, st);
>  	} else if ((status & AT91_SAMA5D2_ISR_PENS) &&
>  		   ((status & rdy_mask) == rdy_mask)) {
>  		/* periodic trigger IRQ - during pen sense */

[PATCH v2 3/8] iio: at91_adc: pass ref to IIO device via param for int function

[Alexandru Ardelean]

Since there will be some changes to how iio_priv_to_dev() is implemented,
it could be that the helper becomes a bit slower, as it will be hidden away
in the IIO core.

For this driver, the IIO device can be passed directly as a parameter to
the at91_ts_sample() function, thus making it immune to the change of
iio_priv_to_dev().
The function gets called in an interrupt context.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/at91_adc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 0368b6dc6d60..5999defe47cd 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -287,13 +287,12 @@ static void handle_adc_eoc_trigger(int irq, struct iio_dev *idev)
 	}
 }
 
-static int at91_ts_sample(struct at91_adc_state *st)
+static int at91_ts_sample(struct iio_dev *idev, struct at91_adc_state *st)
 {
 	unsigned int xscale, yscale, reg, z1, z2;
 	unsigned int x, y, pres, xpos, ypos;
 	unsigned int rxp = 1;
 	unsigned int factor = 1000;
-	struct iio_dev *idev = iio_priv_to_dev(st);
 
 	unsigned int xyz_mask_bits = st->res;
 	unsigned int xyz_mask = (1 << xyz_mask_bits) - 1;
@@ -449,7 +448,7 @@ static irqreturn_t at91_adc_9x5_interrupt(int irq, void *private)
 
 		if (status & AT91_ADC_ISR_PENS) {
 			/* validate data by pen contact */
-			at91_ts_sample(st);
+			at91_ts_sample(idev, st);
 		} else {
 			/* triggered by event that is no pen contact, just read
 			 * them to clean the interrupt and discard all.
-- 
2.17.1

Re: [PATCH v2 3/8] iio: at91_adc: pass ref to IIO device via param for int function

[Jonathan Cameron]

On Thu, 14 May 2020 16:17:05 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Since there will be some changes to how iio_priv_to_dev() is implemented,
> it could be that the helper becomes a bit slower, as it will be hidden away
> in the IIO core.
> 
> For this driver, the IIO device can be passed directly as a parameter to
> the at91_ts_sample() function, thus making it immune to the change of
> iio_priv_to_dev().
> The function gets called in an interrupt context.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
I wonder. Should we just pass the struct device?  It's only used for
error printing I think, so we could make that explicit.

I'm not that bothered either way though.

Jonathan

> ---
>  drivers/iio/adc/at91_adc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index 0368b6dc6d60..5999defe47cd 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -287,13 +287,12 @@ static void handle_adc_eoc_trigger(int irq, struct iio_dev *idev)
>  	}
>  }
>  
> -static int at91_ts_sample(struct at91_adc_state *st)
> +static int at91_ts_sample(struct iio_dev *idev, struct at91_adc_state *st)
>  {
>  	unsigned int xscale, yscale, reg, z1, z2;
>  	unsigned int x, y, pres, xpos, ypos;
>  	unsigned int rxp = 1;
>  	unsigned int factor = 1000;
> -	struct iio_dev *idev = iio_priv_to_dev(st);
>  
>  	unsigned int xyz_mask_bits = st->res;
>  	unsigned int xyz_mask = (1 << xyz_mask_bits) - 1;
> @@ -449,7 +448,7 @@ static irqreturn_t at91_adc_9x5_interrupt(int irq, void *private)
>  
>  		if (status & AT91_ADC_ISR_PENS) {
>  			/* validate data by pen contact */
> -			at91_ts_sample(st);
> +			at91_ts_sample(idev, st);
>  		} else {
>  			/* triggered by event that is no pen contact, just read
>  			 * them to clean the interrupt and discard all.

Re: [PATCH v2 3/8] iio: at91_adc: pass ref to IIO device via param for int function

[Ardelean, Alexandru]

On Sat, 2020-05-16 at 18:17 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Thu, 14 May 2020 16:17:05 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > Since there will be some changes to how iio_priv_to_dev() is implemented,
> > it could be that the helper becomes a bit slower, as it will be hidden away
> > in the IIO core.
> > 
> > For this driver, the IIO device can be passed directly as a parameter to
> > the at91_ts_sample() function, thus making it immune to the change of
> > iio_priv_to_dev().
> > The function gets called in an interrupt context.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> I wonder. Should we just pass the struct device?  It's only used for
> error printing I think, so we could make that explicit.

I was also thinking that for this series, [for some drivers] it would make sense
to put a reference to indio_dev on the state-struct; and just return it.
I'll see about it.
I am feeling that sometimes these IIO core cleanups end up being more than I
want to do. But I'll try to see about it. Maybe I can make time or delegate some
of this.

My personal interest with them, is to reduce my complaints during reviews.
People starting to write IIO drivers: well, I can see their frustration [on
their faces] when I complain that they shouldn't use something, and they copied
it from somewhere.


> 
> I'm not that bothered either way though.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/at91_adc.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> > index 0368b6dc6d60..5999defe47cd 100644
> > --- a/drivers/iio/adc/at91_adc.c
> > +++ b/drivers/iio/adc/at91_adc.c
> > @@ -287,13 +287,12 @@ static void handle_adc_eoc_trigger(int irq, struct
> > iio_dev *idev)
> >  	}
> >  }
> >  
> > -static int at91_ts_sample(struct at91_adc_state *st)
> > +static int at91_ts_sample(struct iio_dev *idev, struct at91_adc_state *st)
> >  {
> >  	unsigned int xscale, yscale, reg, z1, z2;
> >  	unsigned int x, y, pres, xpos, ypos;
> >  	unsigned int rxp = 1;
> >  	unsigned int factor = 1000;
> > -	struct iio_dev *idev = iio_priv_to_dev(st);
> >  
> >  	unsigned int xyz_mask_bits = st->res;
> >  	unsigned int xyz_mask = (1 << xyz_mask_bits) - 1;
> > @@ -449,7 +448,7 @@ static irqreturn_t at91_adc_9x5_interrupt(int irq, void
> > *private)
> >  
> >  		if (status & AT91_ADC_ISR_PENS) {
> >  			/* validate data by pen contact */
> > -			at91_ts_sample(st);
> > +			at91_ts_sample(idev, st);
> >  		} else {
> >  			/* triggered by event that is no pen contact, just read
> >  			 * them to clean the interrupt and discard all.

Re: [PATCH v2 3/8] iio: at91_adc: pass ref to IIO device via param for int function

[Jonathan Cameron]

On Mon, 18 May 2020 08:32:11 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sat, 2020-05-16 at 18:17 +0100, Jonathan Cameron wrote:
> > [External]
> > 
> > On Thu, 14 May 2020 16:17:05 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >   
> > > Since there will be some changes to how iio_priv_to_dev() is implemented,
> > > it could be that the helper becomes a bit slower, as it will be hidden away
> > > in the IIO core.
> > > 
> > > For this driver, the IIO device can be passed directly as a parameter to
> > > the at91_ts_sample() function, thus making it immune to the change of
> > > iio_priv_to_dev().
> > > The function gets called in an interrupt context.
> > > 
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > I wonder. Should we just pass the struct device?  It's only used for
> > error printing I think, so we could make that explicit.  
> 
> I was also thinking that for this series, [for some drivers] it would make sense
> to put a reference to indio_dev on the state-struct; and just return it.
> I'll see about it.
> I am feeling that sometimes these IIO core cleanups end up being more than I
> want to do. But I'll try to see about it. Maybe I can make time or delegate some
> of this.

Absolutely understood.  No problem if you don't have time / energy to
do this stuff.  I very much appreciate it when you do, but I know how
unrewarding it can be!

> 
> My personal interest with them, is to reduce my complaints during reviews.
> People starting to write IIO drivers: well, I can see their frustration [on
> their faces] when I complain that they shouldn't use something, and they copied
> it from somewhere.
> 

That's more or less the only reason I write IIO patches currently!
Though I get to mostly avoid seeing the faces of those who fall
into the traps of old code we should have tidied up years ago :(
Not gotten near any of new hardware pile of IIO hardware in a long time.
Plenty of other new hardware, but not IIO stuff!

Jonathan

> 
> > 
> > I'm not that bothered either way though.
> > 
> > Jonathan
> >   
> > > ---
> > >  drivers/iio/adc/at91_adc.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> > > index 0368b6dc6d60..5999defe47cd 100644
> > > --- a/drivers/iio/adc/at91_adc.c
> > > +++ b/drivers/iio/adc/at91_adc.c
> > > @@ -287,13 +287,12 @@ static void handle_adc_eoc_trigger(int irq, struct
> > > iio_dev *idev)
> > >  	}
> > >  }
> > >  
> > > -static int at91_ts_sample(struct at91_adc_state *st)
> > > +static int at91_ts_sample(struct iio_dev *idev, struct at91_adc_state *st)
> > >  {
> > >  	unsigned int xscale, yscale, reg, z1, z2;
> > >  	unsigned int x, y, pres, xpos, ypos;
> > >  	unsigned int rxp = 1;
> > >  	unsigned int factor = 1000;
> > > -	struct iio_dev *idev = iio_priv_to_dev(st);
> > >  
> > >  	unsigned int xyz_mask_bits = st->res;
> > >  	unsigned int xyz_mask = (1 << xyz_mask_bits) - 1;
> > > @@ -449,7 +448,7 @@ static irqreturn_t at91_adc_9x5_interrupt(int irq, void
> > > *private)
> > >  
> > >  		if (status & AT91_ADC_ISR_PENS) {
> > >  			/* validate data by pen contact */
> > > -			at91_ts_sample(st);
> > > +			at91_ts_sample(idev, st);
> > >  		} else {
> > >  			/* triggered by event that is no pen contact, just read
> > >  			 * them to clean the interrupt and discard all.  

[PATCH v2 4/8] iio: stm32-dfsdm-adc: pass iio device as arg for the interrupt handler

[Alexandru Ardelean]

This changes the argument for the interrupt handler to be the IIO device
instead of the state-struct.
Since there will be some changes to how iio_priv_to_dev() is implemented,
it could be that the helper becomes a bit slower, as it will be hidden away
in the IIO core.

The iio_priv() call will still be fast enough, as it will return a void
pointer from the public IIO device structure. So it's better to switch the
order.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/stm32-dfsdm-adc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 76a60d93fe23..28ef02887bd3 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -1313,8 +1313,8 @@ static const struct iio_info stm32_dfsdm_info_adc = {
 
 static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
 {
-	struct stm32_dfsdm_adc *adc = arg;
-	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+	struct iio_dev *indio_dev = arg;
+	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
 	struct regmap *regmap = adc->dfsdm->regmap;
 	unsigned int status, int_en;
 
@@ -1603,7 +1603,7 @@ static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
 		return irq;
 
 	ret = devm_request_irq(dev, irq, stm32_dfsdm_irq,
-			       0, pdev->name, adc);
+			       0, pdev->name, iio);
 	if (ret < 0) {
 		dev_err(dev, "Failed to request IRQ\n");
 		return ret;
-- 
2.17.1

Re: [PATCH v2 4/8] iio: stm32-dfsdm-adc: pass iio device as arg for the interrupt handler

[Jonathan Cameron]

On Thu, 14 May 2020 16:17:06 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This changes the argument for the interrupt handler to be the IIO device
> instead of the state-struct.
> Since there will be some changes to how iio_priv_to_dev() is implemented,
> it could be that the helper becomes a bit slower, as it will be hidden away
> in the IIO core.
> 
> The iio_priv() call will still be fast enough, as it will return a void
> pointer from the public IIO device structure. So it's better to switch the
> order.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Whilst this one looks fine to me. I'd definitely like an Ack from
one of the stm people in case I'm missing something.

> ---
>  drivers/iio/adc/stm32-dfsdm-adc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 76a60d93fe23..28ef02887bd3 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -1313,8 +1313,8 @@ static const struct iio_info stm32_dfsdm_info_adc = {
>  
>  static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
>  {
> -	struct stm32_dfsdm_adc *adc = arg;
> -	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> +	struct iio_dev *indio_dev = arg;
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>  	struct regmap *regmap = adc->dfsdm->regmap;
>  	unsigned int status, int_en;
>  
> @@ -1603,7 +1603,7 @@ static int stm32_dfsdm_adc_probe(struct platform_device *pdev)
>  		return irq;
>  
>  	ret = devm_request_irq(dev, irq, stm32_dfsdm_irq,
> -			       0, pdev->name, adc);
> +			       0, pdev->name, iio);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to request IRQ\n");
>  		return ret;

[PATCH v2 5/8] iio: stm32-adc: pass iio device as arg for the interrupt handler

[Alexandru Ardelean]

This changes the argument for the interrupt handler to be the IIO device
instead of the state-struct.
Since there will be some changes to how iio_priv_to_dev() is implemented,
it could be that the helper becomes a bit slower, as it will be hidden away
in the IIO core.

The iio_priv() call will still be fast enough, as it will return a void
pointer from the public IIO device structure. So it's better to switch the
order.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/stm32-adc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 80c3f963527b..34885387fbdb 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -1227,8 +1227,8 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
 
 static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
 {
-	struct stm32_adc *adc = data;
-	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+	struct iio_dev *indio_dev = data;
+	struct stm32_adc *adc = iio_priv(indio_dev);
 	const struct stm32_adc_regspec *regs = adc->cfg->regs;
 	u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
 
@@ -1240,8 +1240,8 @@ static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
 
 static irqreturn_t stm32_adc_isr(int irq, void *data)
 {
-	struct stm32_adc *adc = data;
-	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+	struct iio_dev *indio_dev = data;
+	struct stm32_adc *adc = iio_priv(indio_dev);
 	const struct stm32_adc_regspec *regs = adc->cfg->regs;
 	u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
 
@@ -1882,7 +1882,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
 
 	ret = devm_request_threaded_irq(&pdev->dev, adc->irq, stm32_adc_isr,
 					stm32_adc_threaded_isr,
-					0, pdev->name, adc);
+					0, pdev->name, indio_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to request IRQ\n");
 		return ret;
-- 
2.17.1

Re: [PATCH v2 5/8] iio: stm32-adc: pass iio device as arg for the interrupt handler

[Jonathan Cameron]

On Thu, 14 May 2020 16:17:07 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This changes the argument for the interrupt handler to be the IIO device
> instead of the state-struct.
> Since there will be some changes to how iio_priv_to_dev() is implemented,
> it could be that the helper becomes a bit slower, as it will be hidden away
> in the IIO core.
> 
> The iio_priv() call will still be fast enough, as it will return a void
> pointer from the public IIO device structure. So it's better to switch the
> order.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
As with previous patch - looks good to me, but I'd like an stm32
Ack / review.

thanks,

Jonathan

> ---
>  drivers/iio/adc/stm32-adc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 80c3f963527b..34885387fbdb 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -1227,8 +1227,8 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
>  
>  static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
>  {
> -	struct stm32_adc *adc = data;
> -	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> +	struct iio_dev *indio_dev = data;
> +	struct stm32_adc *adc = iio_priv(indio_dev);
>  	const struct stm32_adc_regspec *regs = adc->cfg->regs;
>  	u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
>  
> @@ -1240,8 +1240,8 @@ static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
>  
>  static irqreturn_t stm32_adc_isr(int irq, void *data)
>  {
> -	struct stm32_adc *adc = data;
> -	struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> +	struct iio_dev *indio_dev = data;
> +	struct stm32_adc *adc = iio_priv(indio_dev);
>  	const struct stm32_adc_regspec *regs = adc->cfg->regs;
>  	u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
>  
> @@ -1882,7 +1882,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_threaded_irq(&pdev->dev, adc->irq, stm32_adc_isr,
>  					stm32_adc_threaded_isr,
> -					0, pdev->name, adc);
> +					0, pdev->name, indio_dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to request IRQ\n");
>  		return ret;

[PATCH v2 6/8] iio: core: wrap IIO device into an iio_dev_opaque object

[Alexandru Ardelean]

There are plenty of bad designs we want to discourage or not have to review
manually usually about accessing private (marked as [INTERN]) fields of
'struct iio_dev'.

Sometimes users copy drivers that are not always the best examples.

A better idea is to hide those fields into the framework.
For 'struct iio_dev' this is a 'struct iio_dev_opaque' which wraps a public
'struct iio_dev' object.

In the next series, some fields will be moved to this new struct, each with
it's own rework.

This rework will not be complete-able for a while, as many fields need some
drivers to be reworked in order to finalize them (e.g. 'indio_dev->mlock').

But some fields can already be moved, and in time, all of them may get
there (in the 'struct iio_dev_opaque' object).

Since a lot of drivers also call 'iio_priv()', in order to preserve
fast-paths (where this matters), the public iio_dev object will have a
'priv' field that will have the pointer to the private information already
computed. The reference returned by this field should be guaranteed to be
cacheline aligned.

As for the 'iio_priv_to_dev()' helper, this needs to be hidden away. There
aren't many users of this helper, and arguably drivers shouldn't need to
use it in any fast-paths, as they can maintain a reference to the IIO
device.

The opaque parts will be moved into the 'include/linux/iio/iio-opaque.h'
header. Should the hidden information be required for some debugging or
some special needs, it can be made available via this header.
Otherwise, only the IIO core files should include this file.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 27 ++++++++++++++++++++++-----
 include/linux/iio/iio-opaque.h  | 17 +++++++++++++++++
 include/linux/iio/iio.h         | 11 +++++------
 3 files changed, 44 insertions(+), 11 deletions(-)
 create mode 100644 include/linux/iio/iio-opaque.h

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 462d3e810013..a1b29e0f8fd6 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -25,6 +25,7 @@
 #include <linux/debugfs.h>
 #include <linux/mutex.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/iio-opaque.h>
 #include "iio_core.h"
 #include "iio_core_trigger.h"
 #include <linux/iio/sysfs.h>
@@ -164,6 +165,15 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
 };
 
+struct iio_dev *iio_priv_to_dev(void *priv)
+{
+	struct iio_dev_opaque *iio_dev_opaque =
+		(struct iio_dev_opaque *)((char *)priv -
+				  ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN));
+	return &iio_dev_opaque->indio_dev;
+}
+EXPORT_SYMBOL_GPL(iio_priv_to_dev);
+
 /**
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
@@ -1476,6 +1486,8 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
 static void iio_dev_release(struct device *device)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(device);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
 	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
 		iio_device_unregister_trigger_consumer(indio_dev);
 	iio_device_unregister_eventset(indio_dev);
@@ -1484,7 +1496,7 @@ static void iio_dev_release(struct device *device)
 	iio_buffer_put(indio_dev->buffer);
 
 	ida_simple_remove(&iio_ida, indio_dev->id);
-	kfree(indio_dev);
+	kfree(iio_dev_opaque);
 }
 
 struct device_type iio_device_type = {
@@ -1498,10 +1510,11 @@ struct device_type iio_device_type = {
  **/
 struct iio_dev *iio_device_alloc(int sizeof_priv)
 {
+	struct iio_dev_opaque *iio_dev_opaque;
 	struct iio_dev *dev;
 	size_t alloc_size;
 
-	alloc_size = sizeof(struct iio_dev);
+	alloc_size = sizeof(struct iio_dev_opaque);
 	if (sizeof_priv) {
 		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
 		alloc_size += sizeof_priv;
@@ -1509,10 +1522,14 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
 	/* ensure 32-byte alignment of whole construct ? */
 	alloc_size += IIO_ALIGN - 1;
 
-	dev = kzalloc(alloc_size, GFP_KERNEL);
-	if (!dev)
+	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
+	if (!iio_dev_opaque)
 		return NULL;
 
+	dev = &iio_dev_opaque->indio_dev;
+	dev->priv = (char *)iio_dev_opaque +
+		ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
+
 	dev->dev.groups = dev->groups;
 	dev->dev.type = &iio_device_type;
 	dev->dev.bus = &iio_bus_type;
@@ -1526,7 +1543,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
 	if (dev->id < 0) {
 		/* cannot use a dev_err as the name isn't available */
 		pr_err("failed to get device id\n");
-		kfree(dev);
+		kfree(iio_dev_opaque);
 		return NULL;
 	}
 	dev_set_name(&dev->dev, "iio:device%d", dev->id);
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
new file mode 100644
index 000000000000..1375674f14cd
--- /dev/null
+++ b/include/linux/iio/iio-opaque.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _INDUSTRIAL_IO_OPAQUE_H_
+#define _INDUSTRIAL_IO_OPAQUE_H_
+
+/**
+ * struct iio_dev_opaque - industrial I/O device opaque information
+ * @indio_dev:			public industrial I/O device information
+ */
+struct iio_dev_opaque {
+	struct iio_dev			indio_dev;
+};
+
+#define to_iio_dev_opaque(indio_dev)		\
+	container_of(indio_dev, struct iio_dev_opaque, indio_dev)
+
+#endif
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 5f9f439a4f01..e82693db6578 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -522,6 +522,7 @@ struct iio_buffer_setup_ops {
  * @flags:		[INTERN] file ops related flags including busy flag.
  * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
  * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
+ * @priv:		[DRIVER] reference to driver's private information
  */
 struct iio_dev {
 	int				id;
@@ -571,6 +572,7 @@ struct iio_dev {
 	char				read_buf[20];
 	unsigned int			read_buf_len;
 #endif
+	void				*priv;
 };
 
 const struct iio_chan_spec
@@ -678,16 +680,13 @@ static inline void *iio_device_get_drvdata(struct iio_dev *indio_dev)
 #define IIO_ALIGN L1_CACHE_BYTES
 struct iio_dev *iio_device_alloc(int sizeof_priv);
 
+/* The information at this reference is guaranteed to be cacheline aligned */
 static inline void *iio_priv(const struct iio_dev *indio_dev)
 {
-	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
+	return indio_dev->priv;
 }
 
-static inline struct iio_dev *iio_priv_to_dev(void *priv)
-{
-	return (struct iio_dev *)((char *)priv -
-				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
-}
+struct iio_dev *iio_priv_to_dev(void *priv);
 
 void iio_device_free(struct iio_dev *indio_dev);
 struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv);
-- 
2.17.1

Re: [PATCH v2 6/8] iio: core: wrap IIO device into an iio_dev_opaque object

[Jonathan Cameron]

On Thu, 14 May 2020 16:17:08 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> There are plenty of bad designs we want to discourage or not have to review
> manually usually about accessing private (marked as [INTERN]) fields of
> 'struct iio_dev'.
> 
> Sometimes users copy drivers that are not always the best examples.
> 
> A better idea is to hide those fields into the framework.
> For 'struct iio_dev' this is a 'struct iio_dev_opaque' which wraps a public
> 'struct iio_dev' object.
> 
> In the next series, some fields will be moved to this new struct, each with
> it's own rework.
> 
> This rework will not be complete-able for a while, as many fields need some
> drivers to be reworked in order to finalize them (e.g. 'indio_dev->mlock').
> 
> But some fields can already be moved, and in time, all of them may get
> there (in the 'struct iio_dev_opaque' object).

Hopefully!  This is very nice.  One trivial comment inline.

> 
> Since a lot of drivers also call 'iio_priv()', in order to preserve
> fast-paths (where this matters), the public iio_dev object will have a
> 'priv' field that will have the pointer to the private information already
> computed. The reference returned by this field should be guaranteed to be
> cacheline aligned.
> 
> As for the 'iio_priv_to_dev()' helper, this needs to be hidden away. There
> aren't many users of this helper, and arguably drivers shouldn't need to
> use it in any fast-paths, as they can maintain a reference to the IIO
> device.
> 
> The opaque parts will be moved into the 'include/linux/iio/iio-opaque.h'
> header. Should the hidden information be required for some debugging or
> some special needs, it can be made available via this header.
> Otherwise, only the IIO core files should include this file.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 27 ++++++++++++++++++++++-----
>  include/linux/iio/iio-opaque.h  | 17 +++++++++++++++++
>  include/linux/iio/iio.h         | 11 +++++------
>  3 files changed, 44 insertions(+), 11 deletions(-)
>  create mode 100644 include/linux/iio/iio-opaque.h
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 462d3e810013..a1b29e0f8fd6 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -25,6 +25,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/mutex.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/iio-opaque.h>
>  #include "iio_core.h"
>  #include "iio_core_trigger.h"
>  #include <linux/iio/sysfs.h>
> @@ -164,6 +165,15 @@ static const char * const iio_chan_info_postfix[] = {
>  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
>  };
>  
> +struct iio_dev *iio_priv_to_dev(void *priv)
> +{
> +	struct iio_dev_opaque *iio_dev_opaque =
> +		(struct iio_dev_opaque *)((char *)priv -
> +				  ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN));
> +	return &iio_dev_opaque->indio_dev;
> +}
> +EXPORT_SYMBOL_GPL(iio_priv_to_dev);
> +
>  /**
>   * iio_find_channel_from_si() - get channel from its scan index
>   * @indio_dev:		device
> @@ -1476,6 +1486,8 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
>  static void iio_dev_release(struct device *device)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(device);
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
>  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
>  		iio_device_unregister_trigger_consumer(indio_dev);
>  	iio_device_unregister_eventset(indio_dev);
> @@ -1484,7 +1496,7 @@ static void iio_dev_release(struct device *device)
>  	iio_buffer_put(indio_dev->buffer);
>  
>  	ida_simple_remove(&iio_ida, indio_dev->id);
> -	kfree(indio_dev);
> +	kfree(iio_dev_opaque);
>  }
>  
>  struct device_type iio_device_type = {
> @@ -1498,10 +1510,11 @@ struct device_type iio_device_type = {
>   **/
>  struct iio_dev *iio_device_alloc(int sizeof_priv)
>  {
> +	struct iio_dev_opaque *iio_dev_opaque;
>  	struct iio_dev *dev;
>  	size_t alloc_size;
>  
> -	alloc_size = sizeof(struct iio_dev);
> +	alloc_size = sizeof(struct iio_dev_opaque);
>  	if (sizeof_priv) {
>  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
>  		alloc_size += sizeof_priv;
> @@ -1509,10 +1522,14 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
>  	/* ensure 32-byte alignment of whole construct ? */
>  	alloc_size += IIO_ALIGN - 1;
>  
> -	dev = kzalloc(alloc_size, GFP_KERNEL);
> -	if (!dev)
> +	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
> +	if (!iio_dev_opaque)
>  		return NULL;
>  
> +	dev = &iio_dev_opaque->indio_dev;
> +	dev->priv = (char *)iio_dev_opaque +
> +		ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> +
>  	dev->dev.groups = dev->groups;
>  	dev->dev.type = &iio_device_type;
>  	dev->dev.bus = &iio_bus_type;
> @@ -1526,7 +1543,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
>  	if (dev->id < 0) {
>  		/* cannot use a dev_err as the name isn't available */
>  		pr_err("failed to get device id\n");
> -		kfree(dev);
> +		kfree(iio_dev_opaque);
>  		return NULL;
>  	}
>  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> new file mode 100644
> index 000000000000..1375674f14cd
> --- /dev/null
> +++ b/include/linux/iio/iio-opaque.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _INDUSTRIAL_IO_OPAQUE_H_
> +#define _INDUSTRIAL_IO_OPAQUE_H_
> +
> +/**
> + * struct iio_dev_opaque - industrial I/O device opaque information
> + * @indio_dev:			public industrial I/O device information
> + */
> +struct iio_dev_opaque {
> +	struct iio_dev			indio_dev;
> +};
> +
> +#define to_iio_dev_opaque(indio_dev)		\
> +	container_of(indio_dev, struct iio_dev_opaque, indio_dev)
> +
> +#endif
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 5f9f439a4f01..e82693db6578 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -522,6 +522,7 @@ struct iio_buffer_setup_ops {
>   * @flags:		[INTERN] file ops related flags including busy flag.
>   * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
>   * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
> + * @priv:		[DRIVER] reference to driver's private information
Add a note to this to say it should be accessed only through the
iio_priv() route.  Whilst it's not opaque we might in theory change
what it does sometime in the future - just like we are doing here :)

>   */
>  struct iio_dev {
>  	int				id;
> @@ -571,6 +572,7 @@ struct iio_dev {
>  	char				read_buf[20];
>  	unsigned int			read_buf_len;
>  #endif
> +	void				*priv;
>  };
>  
>  const struct iio_chan_spec
> @@ -678,16 +680,13 @@ static inline void *iio_device_get_drvdata(struct iio_dev *indio_dev)
>  #define IIO_ALIGN L1_CACHE_BYTES
>  struct iio_dev *iio_device_alloc(int sizeof_priv);
>  
> +/* The information at this reference is guaranteed to be cacheline aligned */
>  static inline void *iio_priv(const struct iio_dev *indio_dev)
>  {
> -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
> +	return indio_dev->priv;
>  }
>  
> -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> -{
> -	return (struct iio_dev *)((char *)priv -
> -				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
> -}
> +struct iio_dev *iio_priv_to_dev(void *priv);
>  
>  void iio_device_free(struct iio_dev *indio_dev);
>  struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv);

[PATCH v2 7/8] iio: core: simplify alloc alignment code

[Alexandru Ardelean]

There was a recent discussion about this code:
  https://lore.kernel.org/linux-iio/20200322165317.0b1f0674@archlinux/

This looks like a good time to rework this, since any issues about it
should pop-up under testing, because the iio_dev is having a bit of an
overhaul and stuff being moved to iio_dev_priv.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index a1b29e0f8fd6..7671d36efae7 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1514,13 +1514,9 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
 	struct iio_dev *dev;
 	size_t alloc_size;
 
-	alloc_size = sizeof(struct iio_dev_opaque);
-	if (sizeof_priv) {
-		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
-		alloc_size += sizeof_priv;
-	}
-	/* ensure 32-byte alignment of whole construct ? */
-	alloc_size += IIO_ALIGN - 1;
+	alloc_size = ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
+	if (sizeof_priv)
+		alloc_size += ALIGN(sizeof_priv, IIO_ALIGN);
 
 	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
 	if (!iio_dev_opaque)
-- 
2.17.1

RE: [PATCH v2 7/8] iio: core: simplify alloc alignment code

[Sa, Nuno]

Hey Alex,

Just a small question...

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> On Behalf Of Alexandru Ardelean
> Sent: Donnerstag, 14. Mai 2020 15:17
> To: linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-kernel@vger.kernel.org
> Cc: ludovic.desroches@microchip.com; eugen.hristev@microchip.com;
> jic23@kernel.org; nicolas.ferre@microchip.com;
> alexandre.belloni@bootlin.com; alexandre.torgue@st.com;
> mcoquelin.stm32@gmail.com; ak@it-klinger.de; Ardelean, Alexandru
> <alexandru.Ardelean@analog.com>
> Subject: [PATCH v2 7/8] iio: core: simplify alloc alignment code
> 
> There was a recent discussion about this code:
>   https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20200322165317.0b1f0674@archlinux/__;!!A3Ni8CS0y2Y!pgdUSayJCfxMiE
> w8Fpv0LkEZurCSkX0sEcLnXeDSCLmhpu1xont6-vBQj3ZbCw$
> 
> This looks like a good time to rework this, since any issues about it
> should pop-up under testing, because the iio_dev is having a bit of an
> overhaul and stuff being moved to iio_dev_priv.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index a1b29e0f8fd6..7671d36efae7 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1514,13 +1514,9 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
>  	struct iio_dev *dev;
>  	size_t alloc_size;
> 
> -	alloc_size = sizeof(struct iio_dev_opaque);
> -	if (sizeof_priv) {
> -		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> -		alloc_size += sizeof_priv;
> -	}
> -	/* ensure 32-byte alignment of whole construct ? */
> -	alloc_size += IIO_ALIGN - 1;
> +	alloc_size = ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> +	if (sizeof_priv)
> +		alloc_size += ALIGN(sizeof_priv, IIO_ALIGN);

Do we actually need to do the `ALIGN` again? It seems to me that `alloc_size += sizeof_priv`
would be enough or am I missing something obvious?

- Nuno Sá

Re: [PATCH v2 7/8] iio: core: simplify alloc alignment code

[Ardelean, Alexandru]

On Fri, 2020-05-15 at 07:12 +0000, Sa, Nuno wrote:
> Hey Alex,
> 
> Just a small question...
> 
> > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> > On Behalf Of Alexandru Ardelean
> > Sent: Donnerstag, 14. Mai 2020 15:17
> > To: linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > stm32@st-md-mailman.stormreply.com; linux-kernel@vger.kernel.org
> > Cc: ludovic.desroches@microchip.com; eugen.hristev@microchip.com;
> > jic23@kernel.org; nicolas.ferre@microchip.com;
> > alexandre.belloni@bootlin.com; alexandre.torgue@st.com;
> > mcoquelin.stm32@gmail.com; ak@it-klinger.de; Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com>
> > Subject: [PATCH v2 7/8] iio: core: simplify alloc alignment code
> > 
> > There was a recent discussion about this code:
> >   https://urldefense.com/v3/__https://lore.kernel.org/linux-
> > iio/20200322165317.0b1f0674@archlinux/__;!!A3Ni8CS0y2Y!pgdUSayJCfxMiE
> > w8Fpv0LkEZurCSkX0sEcLnXeDSCLmhpu1xont6-vBQj3ZbCw$
> > 
> > This looks like a good time to rework this, since any issues about it
> > should pop-up under testing, because the iio_dev is having a bit of an
> > overhaul and stuff being moved to iio_dev_priv.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/iio/industrialio-core.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index a1b29e0f8fd6..7671d36efae7 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1514,13 +1514,9 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> >  	struct iio_dev *dev;
> >  	size_t alloc_size;
> > 
> > -	alloc_size = sizeof(struct iio_dev_opaque);
> > -	if (sizeof_priv) {
> > -		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > -		alloc_size += sizeof_priv;
> > -	}
> > -	/* ensure 32-byte alignment of whole construct ? */
> > -	alloc_size += IIO_ALIGN - 1;
> > +	alloc_size = ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> > +	if (sizeof_priv)
> > +		alloc_size += ALIGN(sizeof_priv, IIO_ALIGN);
> 
> Do we actually need to do the `ALIGN` again? It seems to me that `alloc_size
> += sizeof_priv`
> would be enough or am I missing something obvious?

Well, it's not always clear what value 'sizeof_priv' has, and whether it is
provided already aligned.
The requirement is usually that this data be cacheline aligned.

So, sizeof(struct iio_dev_opaque) is aligned already a few lines above, but the
private information should also be aligned [given that it's an unknown value
provided by the driver].
I think this is mostly important, if we need to do DMA access to buffers
allocated on the driver's state-struct, which is allocated here, and which is
usually provided as sizeof_priv.

Tbh, the discussions around this alignment/cacheline-alignment are a bit fuzzy
to me. I haven't run into any of these complicated issues.

> 
> - Nuno Sá
> 

RE: [PATCH v2 7/8] iio: core: simplify alloc alignment code

[Sa, Nuno]

> From: Ardelean, Alexandru <alexandru.Ardelean@analog.com>
> Sent: Freitag, 15. Mai 2020 13:48
> To: linux-arm-kernel@lists.infradead.org; linux-stm32@st-md-
> mailman.stormreply.com; Sa, Nuno <Nuno.Sa@analog.com>; linux-
> kernel@vger.kernel.org; linux-iio@vger.kernel.org
> Cc: ludovic.desroches@microchip.com; nicolas.ferre@microchip.com;
> alexandre.torgue@st.com; ak@it-klinger.de; jic23@kernel.org;
> eugen.hristev@microchip.com; mcoquelin.stm32@gmail.com;
> alexandre.belloni@bootlin.com
> Subject: Re: [PATCH v2 7/8] iio: core: simplify alloc alignment code
> 
> On Fri, 2020-05-15 at 07:12 +0000, Sa, Nuno wrote:
> > Hey Alex,
> >
> > Just a small question...
> >
> > > From: linux-iio-owner@vger.kernel.org <linux-iio-
> owner@vger.kernel.org>
> > > On Behalf Of Alexandru Ardelean
> > > Sent: Donnerstag, 14. Mai 2020 15:17
> > > To: linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-
> > > stm32@st-md-mailman.stormreply.com; linux-kernel@vger.kernel.org
> > > Cc: ludovic.desroches@microchip.com; eugen.hristev@microchip.com;
> > > jic23@kernel.org; nicolas.ferre@microchip.com;
> > > alexandre.belloni@bootlin.com; alexandre.torgue@st.com;
> > > mcoquelin.stm32@gmail.com; ak@it-klinger.de; Ardelean, Alexandru
> > > <alexandru.Ardelean@analog.com>
> > > Subject: [PATCH v2 7/8] iio: core: simplify alloc alignment code
> > >
> > > There was a recent discussion about this code:
> > >   https://urldefense.com/v3/__https://lore.kernel.org/linux-
> > >
> iio/20200322165317.0b1f0674@archlinux/__;!!A3Ni8CS0y2Y!pgdUSayJCfxMiE
> > > w8Fpv0LkEZurCSkX0sEcLnXeDSCLmhpu1xont6-vBQj3ZbCw$
> > >
> > > This looks like a good time to rework this, since any issues about it
> > > should pop-up under testing, because the iio_dev is having a bit of an
> > > overhaul and stuff being moved to iio_dev_priv.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > >  drivers/iio/industrialio-core.c | 10 +++-------
> > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > core.c
> > > index a1b29e0f8fd6..7671d36efae7 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -1514,13 +1514,9 @@ struct iio_dev *iio_device_alloc(int
> sizeof_priv)
> > >  	struct iio_dev *dev;
> > >  	size_t alloc_size;
> > >
> > > -	alloc_size = sizeof(struct iio_dev_opaque);
> > > -	if (sizeof_priv) {
> > > -		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > -		alloc_size += sizeof_priv;
> > > -	}
> > > -	/* ensure 32-byte alignment of whole construct ? */
> > > -	alloc_size += IIO_ALIGN - 1;
> > > +	alloc_size = ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> > > +	if (sizeof_priv)
> > > +		alloc_size += ALIGN(sizeof_priv, IIO_ALIGN);
> >
> > Do we actually need to do the `ALIGN` again? It seems to me that
> `alloc_size
> > += sizeof_priv`
> > would be enough or am I missing something obvious?
> 
> Well, it's not always clear what value 'sizeof_priv' has, and whether it is
> provided already aligned.
> The requirement is usually that this data be cacheline aligned.
> 
> So, sizeof(struct iio_dev_opaque) is aligned already a few lines above, but
> the
> private information should also be aligned [given that it's an unknown value
> provided by the driver].
> I think this is mostly important, if we need to do DMA access to buffers
> allocated on the driver's state-struct, which is allocated here, and which is
> usually provided as sizeof_priv.

Yes, AFAIU this is to guarantee that the priv struct will start at an address that is 
DMA safe (cacheline-aligned). Hence, if there is any data in 'priv' that needs to be DMA
safe, we are fine...

Well, I was also misreading the code. Still, I think it should look something like:

````
alloc_size = sizeof(struct iio_dev_opaque)
if (sizeof_priv)
	alloc_size += ALIGN(alloc_size, IIO_ALIGN);
````

If there is no priv, I think we don't need the padding bytes...

- Nuno Sá

Re: [PATCH v2 7/8] iio: core: simplify alloc alignment code

[Jonathan Cameron]

On Fri, 15 May 2020 12:37:28 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Ardelean, Alexandru <alexandru.Ardelean@analog.com>
> > Sent: Freitag, 15. Mai 2020 13:48
> > To: linux-arm-kernel@lists.infradead.org; linux-stm32@st-md-
> > mailman.stormreply.com; Sa, Nuno <Nuno.Sa@analog.com>; linux-
> > kernel@vger.kernel.org; linux-iio@vger.kernel.org
> > Cc: ludovic.desroches@microchip.com; nicolas.ferre@microchip.com;
> > alexandre.torgue@st.com; ak@it-klinger.de; jic23@kernel.org;
> > eugen.hristev@microchip.com; mcoquelin.stm32@gmail.com;
> > alexandre.belloni@bootlin.com
> > Subject: Re: [PATCH v2 7/8] iio: core: simplify alloc alignment code
> > 
> > On Fri, 2020-05-15 at 07:12 +0000, Sa, Nuno wrote:  
> > > Hey Alex,
> > >
> > > Just a small question...
> > >  
> > > > From: linux-iio-owner@vger.kernel.org <linux-iio-  
> > owner@vger.kernel.org>  
> > > > On Behalf Of Alexandru Ardelean
> > > > Sent: Donnerstag, 14. Mai 2020 15:17
> > > > To: linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;  
> > linux-  
> > > > stm32@st-md-mailman.stormreply.com; linux-kernel@vger.kernel.org
> > > > Cc: ludovic.desroches@microchip.com; eugen.hristev@microchip.com;
> > > > jic23@kernel.org; nicolas.ferre@microchip.com;
> > > > alexandre.belloni@bootlin.com; alexandre.torgue@st.com;
> > > > mcoquelin.stm32@gmail.com; ak@it-klinger.de; Ardelean, Alexandru
> > > > <alexandru.Ardelean@analog.com>
> > > > Subject: [PATCH v2 7/8] iio: core: simplify alloc alignment code
> > > >
> > > > There was a recent discussion about this code:
> > > >   https://urldefense.com/v3/__https://lore.kernel.org/linux-
> > > >  
> > iio/20200322165317.0b1f0674@archlinux/__;!!A3Ni8CS0y2Y!pgdUSayJCfxMiE  
> > > > w8Fpv0LkEZurCSkX0sEcLnXeDSCLmhpu1xont6-vBQj3ZbCw$
> > > >
> > > > This looks like a good time to rework this, since any issues about it
> > > > should pop-up under testing, because the iio_dev is having a bit of an
> > > > overhaul and stuff being moved to iio_dev_priv.
> > > >
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > ---
> > > >  drivers/iio/industrialio-core.c | 10 +++-------
> > > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > > core.c
> > > > index a1b29e0f8fd6..7671d36efae7 100644
> > > > --- a/drivers/iio/industrialio-core.c
> > > > +++ b/drivers/iio/industrialio-core.c
> > > > @@ -1514,13 +1514,9 @@ struct iio_dev *iio_device_alloc(int  
> > sizeof_priv)  
> > > >  	struct iio_dev *dev;
> > > >  	size_t alloc_size;
> > > >
> > > > -	alloc_size = sizeof(struct iio_dev_opaque);
> > > > -	if (sizeof_priv) {
> > > > -		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > -		alloc_size += sizeof_priv;
> > > > -	}
> > > > -	/* ensure 32-byte alignment of whole construct ? */
> > > > -	alloc_size += IIO_ALIGN - 1;
> > > > +	alloc_size = ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> > > > +	if (sizeof_priv)
> > > > +		alloc_size += ALIGN(sizeof_priv, IIO_ALIGN);  
> > >
> > > Do we actually need to do the `ALIGN` again? It seems to me that  
> > `alloc_size  
> > > += sizeof_priv`
> > > would be enough or am I missing something obvious?  
> > 
> > Well, it's not always clear what value 'sizeof_priv' has, and whether it is
> > provided already aligned.
> > The requirement is usually that this data be cacheline aligned.
> > 
> > So, sizeof(struct iio_dev_opaque) is aligned already a few lines above, but
> > the
> > private information should also be aligned [given that it's an unknown value
> > provided by the driver].
> > I think this is mostly important, if we need to do DMA access to buffers
> > allocated on the driver's state-struct, which is allocated here, and which is
> > usually provided as sizeof_priv.  
> 
> Yes, AFAIU this is to guarantee that the priv struct will start at an address that is 
> DMA safe (cacheline-aligned). Hence, if there is any data in 'priv' that needs to be DMA
> safe, we are fine...
> 
> Well, I was also misreading the code. Still, I think it should look something like:
> 
> ````
> alloc_size = sizeof(struct iio_dev_opaque)
> if (sizeof_priv)
> 	alloc_size += ALIGN(alloc_size, IIO_ALIGN);
> ````
> 
> If there is no priv, I think we don't need the padding bytes...
Agreed - no need to guarantee alignment of something that doesn't exist :)

> 
> - Nuno Sá
> 

[PATCH v2 8/8] iio: core: move debugfs data on the private iio dev info

[Alexandru Ardelean]

This change moves all iio_dev debugfs fields to the iio_dev_priv object.
It's not the biggest advantage yet (to the whole thing of abstractization)
but it's a start.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 40 ++++++++++++++++++++++-----------
 include/linux/iio/iio-opaque.h  | 10 +++++++++
 include/linux/iio/iio.h         | 13 +----------
 3 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 7671d36efae7..26bef5032810 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -174,6 +174,13 @@ struct iio_dev *iio_priv_to_dev(void *priv)
 }
 EXPORT_SYMBOL_GPL(iio_priv_to_dev);
 
+struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
+{
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	return iio_dev_opaque->debugfs_dentry;
+}
+EXPORT_SYMBOL_GPL(iio_get_debugfs_dentry);
+
 /**
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
@@ -317,35 +324,37 @@ static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
 			      size_t count, loff_t *ppos)
 {
 	struct iio_dev *indio_dev = file->private_data;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	unsigned val = 0;
 	int ret;
 
 	if (*ppos > 0)
 		return simple_read_from_buffer(userbuf, count, ppos,
-					       indio_dev->read_buf,
-					       indio_dev->read_buf_len);
+					       iio_dev_opaque->read_buf,
+					       iio_dev_opaque->read_buf_len);
 
 	ret = indio_dev->info->debugfs_reg_access(indio_dev,
-						  indio_dev->cached_reg_addr,
+						  iio_dev_opaque->cached_reg_addr,
 						  0, &val);
 	if (ret) {
 		dev_err(indio_dev->dev.parent, "%s: read failed\n", __func__);
 		return ret;
 	}
 
-	indio_dev->read_buf_len = snprintf(indio_dev->read_buf,
-					   sizeof(indio_dev->read_buf),
-					   "0x%X\n", val);
+	iio_dev_opaque->read_buf_len = snprintf(iio_dev_opaque->read_buf,
+					      sizeof(iio_dev_opaque->read_buf),
+					      "0x%X\n", val);
 
 	return simple_read_from_buffer(userbuf, count, ppos,
-				       indio_dev->read_buf,
-				       indio_dev->read_buf_len);
+				       iio_dev_opaque->read_buf,
+				       iio_dev_opaque->read_buf_len);
 }
 
 static ssize_t iio_debugfs_write_reg(struct file *file,
 		     const char __user *userbuf, size_t count, loff_t *ppos)
 {
 	struct iio_dev *indio_dev = file->private_data;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	unsigned reg, val;
 	char buf[80];
 	int ret;
@@ -360,10 +369,10 @@ static ssize_t iio_debugfs_write_reg(struct file *file,
 
 	switch (ret) {
 	case 1:
-		indio_dev->cached_reg_addr = reg;
+		iio_dev_opaque->cached_reg_addr = reg;
 		break;
 	case 2:
-		indio_dev->cached_reg_addr = reg;
+		iio_dev_opaque->cached_reg_addr = reg;
 		ret = indio_dev->info->debugfs_reg_access(indio_dev, reg,
 							  val, NULL);
 		if (ret) {
@@ -387,23 +396,28 @@ static const struct file_operations iio_debugfs_reg_fops = {
 
 static void iio_device_unregister_debugfs(struct iio_dev *indio_dev)
 {
-	debugfs_remove_recursive(indio_dev->debugfs_dentry);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	debugfs_remove_recursive(iio_dev_opaque->debugfs_dentry);
 }
 
 static void iio_device_register_debugfs(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque;
+
 	if (indio_dev->info->debugfs_reg_access == NULL)
 		return;
 
 	if (!iio_debugfs_dentry)
 		return;
 
-	indio_dev->debugfs_dentry =
+	iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	iio_dev_opaque->debugfs_dentry =
 		debugfs_create_dir(dev_name(&indio_dev->dev),
 				   iio_debugfs_dentry);
 
 	debugfs_create_file("direct_reg_access", 0644,
-			    indio_dev->debugfs_dentry, indio_dev,
+			    iio_dev_opaque->debugfs_dentry, indio_dev,
 			    &iio_debugfs_reg_fops);
 }
 #else
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 1375674f14cd..b3f234b4c1e9 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -6,9 +6,19 @@
 /**
  * struct iio_dev_opaque - industrial I/O device opaque information
  * @indio_dev:			public industrial I/O device information
+ * @debugfs_dentry:		device specific debugfs dentry
+ * @cached_reg_addr:		cached register address for debugfs reads
+ * @read_buf:			read buffer to be used for the initial reg read
+ * @read_buf_len:		data length in @read_buf
  */
 struct iio_dev_opaque {
 	struct iio_dev			indio_dev;
+#if defined(CONFIG_DEBUG_FS)
+	struct dentry			*debugfs_dentry;
+	unsigned			cached_reg_addr;
+	char				read_buf[20];
+	unsigned int			read_buf_len;
+#endif
 };
 
 #define to_iio_dev_opaque(indio_dev)		\
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index e82693db6578..a52a9f688b35 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -520,8 +520,6 @@ struct iio_buffer_setup_ops {
  * @groups:		[INTERN] attribute groups
  * @groupcounter:	[INTERN] index of next attribute group
  * @flags:		[INTERN] file ops related flags including busy flag.
- * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
- * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
  * @priv:		[DRIVER] reference to driver's private information
  */
 struct iio_dev {
@@ -566,12 +564,6 @@ struct iio_dev {
 	int				groupcounter;
 
 	unsigned long			flags;
-#if defined(CONFIG_DEBUG_FS)
-	struct dentry			*debugfs_dentry;
-	unsigned			cached_reg_addr;
-	char				read_buf[20];
-	unsigned int			read_buf_len;
-#endif
 	void				*priv;
 };
 
@@ -708,10 +700,7 @@ static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
  * @indio_dev:		IIO device structure for device
  **/
 #if defined(CONFIG_DEBUG_FS)
-static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
-{
-	return indio_dev->debugfs_dentry;
-}
+struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev);
 #else
 static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
 {
-- 
2.17.1

Re: [PATCH v2 0/8] iio: core: wrap IIO device into an iio_dev_opaque object

[Ardelean, Alexandru]

On Thu, 2020-05-14 at 16:17 +0300, Alexandru Ardelean wrote:
> This change starts to hide some internal fields of the IIO device into
> the framework.
> 
> Because the iio_priv_to_dev() will be hidden some pre-work is done to
> try to remove it from some interrupt handlers.
> iio_priv_to_dev() will become a function call and won't be expandable
> into place (as-is now as an inline function).
> 

I'll defer this series.
A cleanup of iio_priv_to_dev() doesn't look like a bit detour.


> Changelog v1 -> v2:
> - add pre-work patches that remove some calls to iio_priv_to_dev() from
>   interrupt handlers
> - renamed iio_dev_priv -> iio_dev_opaque
> - moved the iio_dev_opaque to 'include/linux/iio/iio-opaque.h' this way
>   it should be usable for debugging
> - the iio_priv() call, is still an inline function that returns an
>   'indio_dev->priv' reference; this field is added to 'struct iio_dev';
>   the reference is computed in iio_device_alloc() and should be
>   cacheline aligned
> - the to_iio_dev_opaque() container is in the
>   'include/linux/iio/iio-opaque.h' header; it's still implemented with
>   some pointer arithmetic; one idea was to do it via an
>   'indio_dev->opaque' field; that may still be an optionif there are
>   some opinions in that direction
> 
> Alexandru Ardelean (8):
>   iio: proximity: ping: pass reference to IIO device via call-stack
>   iio: at91-sama5d2_adc: pass ref to IIO device via param for int
>     function
>   iio: at91_adc: pass ref to IIO device via param for int function
>   iio: stm32-dfsdm-adc: pass iio device as arg for the interrupt handler
>   iio: stm32-adc: pass iio device as arg for the interrupt handler
>   iio: core: wrap IIO device into an iio_dev_opaque object
>   iio: core: simplify alloc alignment code
>   iio: core: move debugfs data on the private iio dev info
> 
>  drivers/iio/adc/at91-sama5d2_adc.c |  7 ++-
>  drivers/iio/adc/at91_adc.c         |  5 +-
>  drivers/iio/adc/stm32-adc.c        | 10 ++--
>  drivers/iio/adc/stm32-dfsdm-adc.c  |  6 +--
>  drivers/iio/industrialio-core.c    | 75 ++++++++++++++++++++----------
>  drivers/iio/proximity/ping.c       |  5 +-
>  include/linux/iio/iio-opaque.h     | 27 +++++++++++
>  include/linux/iio/iio.h            | 24 +++-------
>  8 files changed, 99 insertions(+), 60 deletions(-)
>  create mode 100644 include/linux/iio/iio-opaque.h
>