[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 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

[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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 0787a3aabd05..11c915b61ae2 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -43,10 +43,14 @@ struct adis_timeout {
  * @diag_stat_reg: Register address of the DIAG_STAT register
  * @prod_id_reg: Register address of the PROD_ID register
  * @prod_id: Product ID code that should be expected when reading @prod_id_reg
+ * @self_test_mask: Bitmask of supported self-test operations
  * @self_test_reg: Register address to request self test command
+ * @self_test_no_autoclear: True if device's self-test needs clear of ctrl reg
  * @status_error_msgs: Array of error messgaes
- * @status_error_mask:
+ * @status_error_mask: Bitmask of errors supported by the device
  * @timeouts: Chip specific delays
+ * @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 11c915b61ae2..a3110c097fa1 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -98,6 +98,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 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;

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

[Jonathan Cameron]

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

> 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>
Looks good to me.  Will pick up in v2.
> ---
>  include/linux/iio/imu/adis.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 0787a3aabd05..11c915b61ae2 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -43,10 +43,14 @@ struct adis_timeout {
>   * @diag_stat_reg: Register address of the DIAG_STAT register
>   * @prod_id_reg: Register address of the PROD_ID register
>   * @prod_id: Product ID code that should be expected when reading @prod_id_reg
> + * @self_test_mask: Bitmask of supported self-test operations
>   * @self_test_reg: Register address to request self test command
> + * @self_test_no_autoclear: True if device's self-test needs clear of ctrl reg
>   * @status_error_msgs: Array of error messgaes
> - * @status_error_mask:
> + * @status_error_mask: Bitmask of errors supported by the device
>   * @timeouts: Chip specific delays
> + * @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;

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

[Jonathan Cameron]

On Fri, 21 Feb 2020 13:49:43 +0200
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.

Nice work.

Will pick up in v2.

Thanks,

Jonathan

> 
> 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 11c915b61ae2..a3110c097fa1 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -98,6 +98,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;

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 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 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;