Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 20 Feb 2019 11:52:41 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Beniamin Bia <beniamin.bia@analog.com>
Cc:     <lars@metafoo.de>, <Michael.Hennerich@analog.com>,
        <knaack.h@gmx.de>, <pmeerw@pmeerw.net>,
        <gregkh@linuxfoundation.org>, <linux-iio@vger.kernel.org>,
        <devel@driverdev.osuosl.org>, <linux-kernel@vger.kernel.org>,
        <biabeniamin@outlook.com>
Subject: Re: [PATCH v2 2/2] staging: iio: frequency: ad9834: Move phase and
 scale to standard iio attribute
Message-ID: <20190220115241.62d88739@archlinux>
In-Reply-To: <20190214164130.30085-2-beniamin.bia@analog.com>
References: <20190214164130.30085-1-beniamin.bia@analog.com>
        <20190214164130.30085-2-beniamin.bia@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 14 Feb 2019 18:41:30 +0200
Beniamin Bia <beniamin.bia@analog.com> wrote:

> The custom phase and scale attributes were moved to standard iio types.
> 
> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
Hi, 

This hits the same fundamental questions of ABI as the FSK
elements in the previous patch, just now for Phase Shift Keying.

Unfortunately this ABI element is a strong part of the reason
these DDS chips have languished in staging a long time.
It is not easy to generalize.  On that note, we also want to
take into account any other chips that will want to share ABI with
this one as we define it.

Jonathan

> ---
> Changes in v2:
> 	-the personal email address was replaced by the work email
> 	-separate define for every phase channel
> 	-enum used for write_phase functions
> 	-phase variables were replaced by an array
> 
>  drivers/staging/iio/frequency/ad9834.c | 53 ++++++++++++++++----------
>  1 file changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
> index 561617046c20..4366b6121154 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -82,6 +82,7 @@ struct ad9834_state {
>  	struct mutex                    lock;   /* protect sensor state */
>  
>  	unsigned long			frequency[2];
> +	unsigned long			phase[2];
>  
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
> @@ -113,6 +114,8 @@ enum ad9834_supported_device_ids {
>  		.output = 1,						\
>  		.channel = (chan),					\
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY)	\
> +						| BIT(IIO_CHAN_INFO_PHASE),\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>  }
>  
>  static const struct iio_chan_spec ad9833_channels[] = {
> @@ -172,13 +175,26 @@ static int ad9834_write_frequency(struct ad9834_state *st,
>  }
>  
>  static int ad9834_write_phase(struct ad9834_state *st,
> -			      unsigned long addr, unsigned long phase)
> +			      enum ad9834_ch_addr addr,
> +			      unsigned long phase)
>  {
> +	int ret;
> +
>  	if (phase > BIT(AD9834_PHASE_BITS))
>  		return -EINVAL;
> -	st->data = cpu_to_be16(addr | phase);
>  
> -	return spi_sync(st->spi, &st->msg);
> +	if (addr == AD9834_CHANNEL_ADDRESS0)
> +		st->data = cpu_to_be16(AD9834_REG_PHASE0 | phase);
> +	else
> +		st->data = cpu_to_be16(AD9834_REG_PHASE1 | phase);
> +
> +	ret = spi_sync(st->spi, &st->msg);
> +	if (ret)
> +		return ret;
> +
> +	st->phase[(int)addr] = phase;
> +
> +	return 0;
>  }
>  
>  static int ad9834_read_raw(struct iio_dev *indio_dev,
> @@ -191,6 +207,13 @@ static int ad9834_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_FREQUENCY:
>  		*val = st->frequency[chan->channel];
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_PHASE:
> +		*val = st->phase[chan->channel];
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		/*1 hz */
> +		*val = 1;
> +		return IIO_VAL_INT;
>  	}
>  
>  	return -EINVAL;
> @@ -207,6 +230,10 @@ static int ad9834_write_raw(struct iio_dev *indio_dev,
>  		return ad9834_write_frequency(st,
>  					      (enum ad9834_ch_addr)chan->channel,
>  					      val);
> +	case IIO_CHAN_INFO_PHASE:
> +		return ad9834_write_phase(st,
> +					  (enum ad9834_ch_addr)chan->channel,
> +					  val);
>  	default:
>  		return  -EINVAL;
>  	}
> @@ -231,10 +258,6 @@ static ssize_t ad9834_write(struct device *dev,
>  
>  	mutex_lock(&st->lock);
>  	switch ((u32)this_attr->address) {
> -	case AD9834_REG_PHASE0:
> -	case AD9834_REG_PHASE1:
> -		ret = ad9834_write_phase(st, this_attr->address, val);
> -		break;
>  	case AD9834_OPBITEN:
>  		if (st->control & AD9834_MODE) {
>  			ret = -EINVAL;  /* AD9843 reserved mode */
> @@ -394,12 +417,8 @@ static IIO_DEVICE_ATTR(out_altvoltage0_out1_wavetype_available, 0444,
>   */
>  
>  static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9834_write, AD9834_FSEL);
> -static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
>  
> -static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9834_write, AD9834_REG_PHASE0);
> -static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9834_write, AD9834_REG_PHASE1);
>  static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL, ad9834_write, AD9834_PSEL);
> -static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
>  
>  static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
>  	ad9834_write, AD9834_PIN_SW);
> @@ -410,10 +429,6 @@ static IIO_DEV_ATTR_OUT_WAVETYPE(0, 0, ad9834_store_wavetype, 0);
>  static IIO_DEV_ATTR_OUT_WAVETYPE(0, 1, ad9834_store_wavetype, 1);
>  
>  static struct attribute *ad9834_attributes[] = {
> -	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
> -	&iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
> @@ -427,10 +442,6 @@ static struct attribute *ad9834_attributes[] = {
>  };
>  
>  static struct attribute *ad9833_attributes[] = {
> -	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
> -	&iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr,
> @@ -554,11 +565,11 @@ static int ad9834_probe(struct spi_device *spi)
>  	if (ret)
>  		goto error_clock_unprepare;
>  
> -	ret = ad9834_write_phase(st, AD9834_REG_PHASE0, 512);
> +	ret = ad9834_write_phase(st, AD9834_CHANNEL_ADDRESS0, 512);
>  	if (ret)
>  		goto error_clock_unprepare;
>  
> -	ret = ad9834_write_phase(st, AD9834_REG_PHASE1, 1024);
> +	ret = ad9834_write_phase(st, AD9834_CHANNEL_ADDRESS1, 1024);
>  	if (ret)
>  		goto error_clock_unprepare;
>