Re: [PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Beniamin Bia <biabeniamin@outlook.com>
Cc: jic23@kernel.org, devel@driverdev.osuosl.org, lars@metafoo.de,
	Michael.Hennerich@analog.com, linux-iio@vger.kernel.org,
	gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
	pmeerw@pmeerw.net, knaack.h@gmx.de,
	Beniamin Bia <beniamin.bia@analog.com>
Subject: Re: [PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types
Date: Wed, 6 Feb 2019 17:59:19 +0300	[thread overview]
Message-ID: <20190206145919.GA1978@kadam> (raw)
In-Reply-To: <20190206120542.16617-1-biabeniamin@outlook.com>

On Wed, Feb 06, 2019 at 02:05:41PM +0200, Beniamin Bia wrote:
> Frequency attribute is added with a standard type from iio framework
> instead of custom attribute. This is a small step towards removing any
> unnecessary custom attribute.
> 
> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>

This doesn't match your email.  You should probably add a From: to
give your @analog.com address authorship credit.

> ---
>  drivers/staging/iio/frequency/ad9834.c | 97 +++++++++++++++++++++-----
>  1 file changed, 80 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
> index f036f75d1f22..370e8263899e 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -29,8 +29,7 @@
>  /* Registers */
>  
>  #define AD9834_REG_CMD		0
> -#define AD9834_REG_FREQ0	BIT(14)
> -#define AD9834_REG_FREQ1	BIT(15)
> +#define AD9834_REG_FREQ(chann)	(BIT(14) << (chann))

Only one 'n' in "chan", please.  But we actually pass the address not
the channel here so that was unexpected to me.  I think the channel is
always the same as the address?  So maybe it doesn't matter.

>  #define AD9834_REG_PHASE0	(BIT(15) | BIT(14))
>  #define AD9834_REG_PHASE1	(BIT(15) | BIT(14) | BIT(13))
>  
> @@ -81,6 +80,9 @@ struct ad9834_state {
>  	struct spi_message		freq_msg;
>  	struct mutex                    lock;   /* protect sensor state */
>  
> +	unsigned long			frequency0;
> +	unsigned long			frequency1;

I don't understand why we have two frequencies when we only ever use
one.

> +
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -100,6 +102,25 @@ enum ad9834_supported_device_ids {
>  	ID_AD9838,
>  };
>  
> +#define AD9833_CHANNEL(_chan) {						\

No need for the underscore.  (I think?)

> +		.type = IIO_ALTVOLTAGE,					\
> +		.indexed = 1,						\
> +		.output = 1,						\
> +		.address = (_chan),					\
> +		.channel = (_chan),					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY)	\
> +}

regards,
dan carpenter