[PATCH 1/3] iio: imu: adis: add doc-string for `adis` struct

[PATCH 1/3] iio: imu: adis: add doc-string for `adis` struct

[Alexandru Ardelean]

This change adds a doc-string for the `adis` struct. It details the fields
and their roles.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/linux/iio/imu/adis.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 48444f28b003..dbdf421d35dd 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -54,6 +54,20 @@ struct adis_data {
 	bool has_paging;
 };
 
+/**
+ * struct adis - ADIS device instance data
+ * @spi: Reference to SPI device which owns this ADIS IIO device
+ * @trig: IIO trigger object data
+ * @data: ADIS chip variant specific data
+ * @burst: ADIS burst transfer information
+ * @state_lock: Lock used by the device to protect state
+ * @msg: SPI message object
+ * @xfer: SPI transfer objects to be used for a @msg
+ * @current_page: Some ADIS devices have registers, this selects current page
+ * @buffer: Data buffer for information read from the device
+ * @tx: Cacheline aligned TX buffer for SPI transfers
+ * @rx: Cacheline aligned RX buffer for SPI transfers
+ */
 struct adis {
 	struct spi_device	*spi;
 	struct iio_trigger	*trig;
-- 
2.20.1

[PATCH 2/3] iio: imu: adis: update `adis_data` struct doc-string

[Alexandru Ardelean]

The doc-string has been neglected over time.
This change updates it with all the missing info.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/linux/iio/imu/adis.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index dbdf421d35dd..27ebd740f626 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -30,8 +30,13 @@ struct adis_burst;
  * @glob_cmd_reg: Register address of the GLOB_CMD register
  * @msc_ctrl_reg: Register address of the MSC_CTRL register
  * @diag_stat_reg: Register address of the DIAG_STAT register
- * @status_error_msgs: Array of error messgaes
- * @status_error_mask:
+ * @self_test_mask: Bitmask of supported self-test operations
+ * @self_test_no_autoclear: True if device's self-test needs clear of ctrl reg
+ * @startup_delay: Delay in us to be used during ADIS init routines
+ * @status_error_msgs: Array of error message strings
+ * @status_error_mask: Bitmask of errors supported by the device
+ * @enable_irq: Hook for ADIS devices that have a special IRQ enable/disable
+ * @has_paging: True if ADIS device has paged registers
  */
 struct adis_data {
 	unsigned int read_delay;
-- 
2.20.1

[PATCH 3/3] iio: imu: adis: add a note better explaining state_lock

[Alexandru Ardelean]

The `state_lock` mutex was renamed from `txrx_lock` in a previous patch and
is intended to be used by ADIS drivers to protect the state of devices
during consecutive R/W ops.
The initial patch that introduced this change did not do a good [well, any]
job at explaining this. This patch adds a comment to the `state_lock`
better explaining it's use.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/linux/iio/imu/adis.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 27ebd740f626..abd4bd07e960 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -80,6 +80,17 @@ struct adis {
 	const struct adis_data	*data;
 	struct adis_burst	*burst;
 
+	/**
+	 * The state_lock is meant to be used during operations that require
+	 * a sequence of SPI R/W in order to protect the SPI transfer
+	 * information (fields 'xfer', 'msg' & 'current_page') between
+	 * potential concurrent accesses.
+	 * This lock is used by all "adis_{functions}" that have to read/write
+	 * registers. These functions also have unlocked variants
+	 * (see "__adis_{functions}"), which don't hold this lock.
+	 * This allows users of the ADIS library to group SPI R/W into
+	 * the drivers, but they also must manage this lock themselves.
+	 */
 	struct mutex		state_lock;
 	struct spi_message	msg;
 	struct spi_transfer	*xfer;
-- 
2.20.1

Re: [PATCH 1/3] iio: imu: adis: add doc-string for `adis` struct

[Jonathan Cameron]

On Tue, 8 Oct 2019 11:02:37 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change adds a doc-string for the `adis` struct. It details the fields
> and their roles.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  include/linux/iio/imu/adis.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 48444f28b003..dbdf421d35dd 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -54,6 +54,20 @@ struct adis_data {
>  	bool has_paging;
>  };
>  
> +/**
> + * struct adis - ADIS device instance data
> + * @spi: Reference to SPI device which owns this ADIS IIO device
> + * @trig: IIO trigger object data
> + * @data: ADIS chip variant specific data
> + * @burst: ADIS burst transfer information
> + * @state_lock: Lock used by the device to protect state
> + * @msg: SPI message object
> + * @xfer: SPI transfer objects to be used for a @msg
> + * @current_page: Some ADIS devices have registers, this selects current page
> + * @buffer: Data buffer for information read from the device
> + * @tx: Cacheline aligned TX buffer for SPI transfers
> + * @rx: Cacheline aligned RX buffer for SPI transfers
That one is ever so slightly untrue.  It's in the same cacheline as the
tx and so safe (device transfers can only cause trouble for themselves).

I'll tweak to just...
	RX buffer for SPI transfers. May share cacheline with tx.

Hope you don't mind as don't want to waste your time on a v2 for just that.

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it.

Thanks,

Jonathan


> + */
>  struct adis {
>  	struct spi_device	*spi;
>  	struct iio_trigger	*trig;

Re: [PATCH 3/3] iio: imu: adis: add a note better explaining state_lock

[Jonathan Cameron]

On Tue, 8 Oct 2019 11:02:39 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The `state_lock` mutex was renamed from `txrx_lock` in a previous patch and
> is intended to be used by ADIS drivers to protect the state of devices
> during consecutive R/W ops.
> The initial patch that introduced this change did not do a good [well, any]
> job at explaining this. This patch adds a comment to the `state_lock`
> better explaining it's use.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Tiny tweak. See below.

Applied to the togreg branch of iio.git and pushed out as testing.

One thing to note though.  I don't normally run kernel-doc on random
headers, and my eyes missed it, but there are errors in the docs
for read and write functions that could do with fixing.

[jic23@archlinux iio]$ ./scripts/kernel-doc  include/linux/iio/imu/adis.h > temp.html
include/linux/iio/imu/adis.h:138: warning: Function parameter or member 'val' not described in '__adis_write_reg_8'
include/linux/iio/imu/adis.h:138: warning: Excess function parameter 'value' description in '__adis_write_reg_8'
include/linux/iio/imu/adis.h:150: warning: Function parameter or member 'val' not described in '__adis_write_reg_16'
include/linux/iio/imu/adis.h:150: warning: Excess function parameter 'value' description in '__adis_write_reg_16'
include/linux/iio/imu/adis.h:162: warning: Function parameter or member 'val' not described in '__adis_write_reg_32'
include/linux/iio/imu/adis.h:162: warning: Excess function parameter 'value' description in '__adis_write_reg_32'
include/linux/iio/imu/adis.h:217: warning: Function parameter or member 'val' not described in 'adis_write_reg'
include/linux/iio/imu/adis.h:217: warning: Excess function parameter 'value' description in 'adis_write_reg'
include/linux/iio/imu/adis.h:254: warning: Function parameter or member 'val' not described in 'adis_write_reg_8'
include/linux/iio/imu/adis.h:254: warning: Excess function parameter 'value' description in 'adis_write_reg_8'
include/linux/iio/imu/adis.h:266: warning: Function parameter or member 'val' not described in 'adis_write_reg_16'
include/linux/iio/imu/adis.h:266: warning: Excess function parameter 'value' description in 'adis_write_reg_16'
include/linux/iio/imu/adis.h:278: warning: Function parameter or member 'val' not described in 'adis_write_reg_32'
include/linux/iio/imu/adis.h:278: warning: Excess function parameter 'value' description in 'adis_write_reg_32'

If you could role a patch for those, that would be great.  Thanks!

Thanks,

Jonathan

> ---
>  include/linux/iio/imu/adis.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 27ebd740f626..abd4bd07e960 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -80,6 +80,17 @@ struct adis {
>  	const struct adis_data	*data;
>  	struct adis_burst	*burst;
>  
> +	/**
This isn't kernel doc so
 /* 

I'll fix.

> +	 * The state_lock is meant to be used during operations that require
> +	 * a sequence of SPI R/W in order to protect the SPI transfer
> +	 * information (fields 'xfer', 'msg' & 'current_page') between
> +	 * potential concurrent accesses.
> +	 * This lock is used by all "adis_{functions}" that have to read/write
> +	 * registers. These functions also have unlocked variants
> +	 * (see "__adis_{functions}"), which don't hold this lock.
> +	 * This allows users of the ADIS library to group SPI R/W into
> +	 * the drivers, but they also must manage this lock themselves.
> +	 */
>  	struct mutex		state_lock;
>  	struct spi_message	msg;
>  	struct spi_transfer	*xfer;

Re: [PATCH 1/3] iio: imu: adis: add doc-string for 'adis' struct

[Ardelean, Alexandru]

On Fri, 2020-02-21 at 16:14 +0000, Jonathan Cameron wrote:
> On Fri, 21 Feb 2020 15:59:35 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Fri, 2020-02-21 at 13:11 +0000, Jonathan Cameron wrote:
> > > On Fri, 21 Feb 2020 13:49:41 +0200
> > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > >   
> > > > This change adds a doc-string for the 'adis' struct. It details the
> > > > fields
> > > > and their roles.
> > > > 
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > ---
> > > >  include/linux/iio/imu/adis.h | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> > > > index ac7cfd073804..0787a3aabd05 100644
> > > > --- a/include/linux/iio/imu/adis.h
> > > > +++ b/include/linux/iio/imu/adis.h
> > > > @@ -73,6 +73,20 @@ struct adis_data {
> > > >  	bool has_paging;
> > > >  };
> > > >  
> > > > +/**
> > > > + * struct adis - ADIS device instance data
> > > > + * @spi: Reference to SPI device which owns this ADIS IIO device
> > > > + * @trig: IIO trigger object data
> > > > + * @data: ADIS chip variant specific data
> > > > + * @burst: ADIS burst transfer information
> > > > + * @state_lock: Lock used by the device to protect state
> > > > + * @msg: SPI message object
> > > > + * @xfer: SPI transfer objects to be used for a @msg
> > > > + * @current_page: Some ADIS devices have registers, this selects
> > > > current
> > > > page
> > > > + * @buffer: Data buffer for information read from the device
> > > > + * @tx: Cacheline aligned TX buffer for SPI transfers
> > > > + * @rx: Cacheline aligned RX buffer for SPI transfers  
> > > 
> > > This last one isn't true..   
> > 
> > Oh right.
> > I noticed this at some point, then forgot about it.
> > [ The "joys" of multi-tasking; sorry about it :) ]
> > I guess I should also add a patch in this series making it cacheline-
> > aligned.
> > I don't see a reason why not-to [unless I am misunderstanging something]
> No.  The code is fine.  The purpose of the cacheline stuff is to ensure
> no other data ends up in the same cacheline and might be changed in parallel
> with the spi dma taking place.   You only need to force it for the first
> element when you have 2 buffers like this.
> 
> So you need to update the comment in this patch to say something less
> specific.
> Perhaps
> 
> @tx: DMA safe TX buffer for SPI transfers
> @rx: DMA safe RX buffer for SPI transfers.
> 

Sounds good to me.

Thanks
Alex

> Thanks,
> 
> Jonathan
> 
> > 
> > >   
> > > > + */
> > > >  struct adis {
> > > >  	struct spi_device	*spi;
> > > >  	struct iio_trigger	*trig;  

Re: [PATCH 1/3] iio: imu: adis: add doc-string for 'adis' struct

[Jonathan Cameron]

On Fri, 21 Feb 2020 15:59:35 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Fri, 2020-02-21 at 13:11 +0000, Jonathan Cameron wrote:
> > On Fri, 21 Feb 2020 13:49:41 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >   
> > > This change adds a doc-string for the 'adis' struct. It details the fields
> > > and their roles.
> > > 
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > >  include/linux/iio/imu/adis.h | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> > > index ac7cfd073804..0787a3aabd05 100644
> > > --- a/include/linux/iio/imu/adis.h
> > > +++ b/include/linux/iio/imu/adis.h
> > > @@ -73,6 +73,20 @@ struct adis_data {
> > >  	bool has_paging;
> > >  };
> > >  
> > > +/**
> > > + * struct adis - ADIS device instance data
> > > + * @spi: Reference to SPI device which owns this ADIS IIO device
> > > + * @trig: IIO trigger object data
> > > + * @data: ADIS chip variant specific data
> > > + * @burst: ADIS burst transfer information
> > > + * @state_lock: Lock used by the device to protect state
> > > + * @msg: SPI message object
> > > + * @xfer: SPI transfer objects to be used for a @msg
> > > + * @current_page: Some ADIS devices have registers, this selects current
> > > page
> > > + * @buffer: Data buffer for information read from the device
> > > + * @tx: Cacheline aligned TX buffer for SPI transfers
> > > + * @rx: Cacheline aligned RX buffer for SPI transfers  
> > 
> > This last one isn't true..   
> 
> Oh right.
> I noticed this at some point, then forgot about it.
> [ The "joys" of multi-tasking; sorry about it :) ]
> I guess I should also add a patch in this series making it cacheline-aligned.
> I don't see a reason why not-to [unless I am misunderstanging something]
No.  The code is fine.  The purpose of the cacheline stuff is to ensure
no other data ends up in the same cacheline and might be changed in parallel
with the spi dma taking place.   You only need to force it for the first
element when you have 2 buffers like this.

So you need to update the comment in this patch to say something less specific.
Perhaps

@tx: DMA safe TX buffer for SPI transfers
@rx: DMA safe RX buffer for SPI transfers.

Thanks,

Jonathan

> 
> 
> >   
> > > + */
> > >  struct adis {
> > >  	struct spi_device	*spi;
> > >  	struct iio_trigger	*trig;  

Re: [PATCH 1/3] iio: imu: adis: add doc-string for 'adis' struct

[Ardelean, Alexandru]

On Fri, 2020-02-21 at 13:11 +0000, Jonathan Cameron wrote:
> On Fri, 21 Feb 2020 13:49:41 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This change adds a doc-string for the 'adis' struct. It details the fields
> > and their roles.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  include/linux/iio/imu/adis.h | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> > index ac7cfd073804..0787a3aabd05 100644
> > --- a/include/linux/iio/imu/adis.h
> > +++ b/include/linux/iio/imu/adis.h
> > @@ -73,6 +73,20 @@ struct adis_data {
> >  	bool has_paging;
> >  };
> >  
> > +/**
> > + * struct adis - ADIS device instance data
> > + * @spi: Reference to SPI device which owns this ADIS IIO device
> > + * @trig: IIO trigger object data
> > + * @data: ADIS chip variant specific data
> > + * @burst: ADIS burst transfer information
> > + * @state_lock: Lock used by the device to protect state
> > + * @msg: SPI message object
> > + * @xfer: SPI transfer objects to be used for a @msg
> > + * @current_page: Some ADIS devices have registers, this selects current
> > page
> > + * @buffer: Data buffer for information read from the device
> > + * @tx: Cacheline aligned TX buffer for SPI transfers
> > + * @rx: Cacheline aligned RX buffer for SPI transfers
> 
> This last one isn't true.. 

Oh right.
I noticed this at some point, then forgot about it.
[ The "joys" of multi-tasking; sorry about it :) ]
I guess I should also add a patch in this series making it cacheline-aligned.
I don't see a reason why not-to [unless I am misunderstanging something]


> 
> > + */
> >  struct adis {
> >  	struct spi_device	*spi;
> >  	struct iio_trigger	*trig;

Re: [PATCH 1/3] iio: imu: adis: add doc-string for 'adis' struct

[Jonathan Cameron]

On Fri, 21 Feb 2020 13:49:41 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change adds a doc-string for the 'adis' struct. It details the fields
> and their roles.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  include/linux/iio/imu/adis.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index ac7cfd073804..0787a3aabd05 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -73,6 +73,20 @@ struct adis_data {
>  	bool has_paging;
>  };
>  
> +/**
> + * struct adis - ADIS device instance data
> + * @spi: Reference to SPI device which owns this ADIS IIO device
> + * @trig: IIO trigger object data
> + * @data: ADIS chip variant specific data
> + * @burst: ADIS burst transfer information
> + * @state_lock: Lock used by the device to protect state
> + * @msg: SPI message object
> + * @xfer: SPI transfer objects to be used for a @msg
> + * @current_page: Some ADIS devices have registers, this selects current page
> + * @buffer: Data buffer for information read from the device
> + * @tx: Cacheline aligned TX buffer for SPI transfers
> + * @rx: Cacheline aligned RX buffer for SPI transfers

This last one isn't true.. 

> + */
>  struct adis {
>  	struct spi_device	*spi;
>  	struct iio_trigger	*trig;

[PATCH 1/3] iio: imu: adis: add doc-string for 'adis' struct

[Alexandru Ardelean]

This change adds a doc-string for the 'adis' struct. It details the fields
and their roles.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/linux/iio/imu/adis.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index ac7cfd073804..0787a3aabd05 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -73,6 +73,20 @@ struct adis_data {
 	bool has_paging;
 };
 
+/**
+ * struct adis - ADIS device instance data
+ * @spi: Reference to SPI device which owns this ADIS IIO device
+ * @trig: IIO trigger object data
+ * @data: ADIS chip variant specific data
+ * @burst: ADIS burst transfer information
+ * @state_lock: Lock used by the device to protect state
+ * @msg: SPI message object
+ * @xfer: SPI transfer objects to be used for a @msg
+ * @current_page: Some ADIS devices have registers, this selects current page
+ * @buffer: Data buffer for information read from the device
+ * @tx: Cacheline aligned TX buffer for SPI transfers
+ * @rx: Cacheline aligned RX buffer for SPI transfers
+ */
 struct adis {
 	struct spi_device	*spi;
 	struct iio_trigger	*trig;
-- 
2.20.1