Re: [PATCH v3 1/9] staging: iio: ad2s1200: Add kernel docs to driver state

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 23 Apr 2018 00:02:51 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Add missing kernel docs to the ad2s1200 driver state.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Hi David,

Comment inline.

> ---
> Changes in v3:
>  - Added more explanation to mutex lock.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 357fe3c382b3..17d65cb437fd 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -33,6 +33,15 @@
>  /* clock period in nano second */
>  #define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
>  
> +/**
> + * struct ad2s1200_state - driver instance specific data.
> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
> + *		concurrent spi transactions.
It isn't actually preventing concurrent spi transactions - that
is done by the spi core itself.  What it is preventing is
concurrent 'actions' with the device - these involve
a mixture of gpio changes and spi transactions.

That would take some considerable explaining and frankly the code
does the job just fine once people know to look.

I'd just leave it as "protects both the GPIO pins and the rx buffer."
Or feel free to see if you can come up with a brief description of
exactly what we need to be 'atomic'.

Thanks,

Jonathan

> + * @sdev:	spi device.
> + * @sample:	GPIO pin SAMPLE.
> + * @rdvel:	GPIO pin RDVEL.
> + * @rx:		buffer for spi transfers.
> + */
>  struct ad2s1200_state {
>  	struct mutex lock;
>  	struct spi_device *sdev;