[RFC] IIO: ABI for Direct digital synthesis (DDS) devices

[RFC] IIO: ABI for Direct digital synthesis (DDS) devices

[michael.hennerich]

[RFC] IIO: ABI for Direct digital synthesis (DDS) devices

Direct digital synthesis (DDS) is a technique for using digital data
processing blocks as a means to generate a frequency- and phase-tunable
output signal referenced to a fixed-frequency precision clock source.

[RFC 1/3] IIO: Direct digital synthesis abi documentation
	Proposed ABI documentation

[RFC 2/3] IIO: dds.h convenience macros
	Attributes considered as common between various DDS devices

[RFC 3/3] IIO: DDS: AD9833 / AD9834 driver
	Example driver using the proposed ABI

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

[RFC 1/3] IIO: Direct digital synthesis abi documentation

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Proposed ABI documentation

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 .../staging/iio/Documentation/sysfs-bus-iio-dds    |  103 ++++++++++++++++++++
 1 files changed, 103 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds

diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-dds b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
new file mode 100644
index 0000000..2c99889
--- /dev/null
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
@@ -0,0 +1,103 @@
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_freqY
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Stores frequency into tuning word register Y.
+		There can be more than one ddsX_freqY file, which allows for
+		pin controlled FSK Frequency Shift Keying
+		(ddsX_pincontrol_freq_en is active) or the user can control
+		the desired active tuning word by writing Y to the
+		ddsX_freqsymbol file.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_freq_scaleY
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Scale to be applied to ddsX_freqY in order to obtain the
+		desired value in Hz. If shared across all frequency registers
+		Y is not present.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_freqsymbol
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Specifies the active output frequency tuning word. The value
+		corresponds to the Y in ddsX_freqY. To exit this mode the user
+		can write ddsX_pincontrol_freq_en or ddsX_out_disable file.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_phaseY
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Stores phase into phase register Y.
+		There can be more than one ddsX_phaseY file, which allows for
+		pin controlled PSK Phase Shift Keying
+		(ddsX_pincontrol_phase_en is active) or the user can
+		control the desired phase Y which is added to the phase
+		accumulator output by writing Y to the en_phase file.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_phase_scaleY
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Scale to be applied to ddsX_phaseY in order to obtain the
+		desired value in rad. If shared across all phase registers
+		Y is not present.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_phasesymbol
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Specifies the active phase Y which is added to the phase
+		accumulator output. The value corresponds to the Y in
+		ddsX_phaseY. To exit this mode the user can write
+		ddsX_pincontrol_phase_en or disable file.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_pincontrol_en
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Both, the active frequency and phase is controlled by the
+		respective phase and frequency control inputs.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_pincontrol_freq_en
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The active frequency is controlled by the respective
+		frequency control/select inputs.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_pincontrol_phase_en
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The active phase is controlled by the respective
+		phase control/select inputs.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_out_disable
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Disables any signal generation on all outputs.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_outY_disable
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Disables any signal generation on output Y.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_outY_wavetype
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Specifies the output waveform.
+		(sine, triangle, ramp, square, ...)
+		For a list of available output waveform options read
+		available_output_modes.
+
+What:		/sys/bus/iio/devices/device[n]/ddsX_outY_available_wavetypes
+KernelVersion:	2.6.37
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Lists all available output waveform options.
--
1.6.0.2

Re: [RFC 1/3] IIO: Direct digital synthesis abi documentation

[Jonathan Cameron]

On 12/02/10 12:21, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Proposed ABI documentation
> 
Hi Michael,

Couple of comments inline.

I've raised a few questions that I don't have a clear answer two.

The other comments are nitpicks about exact ordering of the elements
of the attribute names.

> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>  .../staging/iio/Documentation/sysfs-bus-iio-dds    |  103 ++++++++++++++++++++
>  1 files changed, 103 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds
> 
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-dds b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
> new file mode 100644
> index 0000000..2c99889
> --- /dev/null
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
> @@ -0,0 +1,103 @@
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_freqY
Here we deviate a little from what we did with input channels.
In that case there was the existing interface (from hwmon) to match
so we already had an _input designation to tell us that the number
was in the relevant base units (here it would be Hz).  Hence we added
a _raw label to say it wasn't and tell userspace to apply scale and offset.
This is stretching a point somewhat, but looking at the hwmon docs, they
have pwmX_freq as a value in Hz. That's obviously going to make consistency
rather tricky to achieve!

Do you think we should leave all _freq without modifier as being in Hz and
have ddsX_freqY_raw. Or should we rely on userspace verifying if there are
appropriate scale / offset parameters to be applied and hence working out
for itself whether the value in ddsX_freqY is in Hz or not?

I'm think I marginally favour leaving it as you have it here but others may
have different opinions.
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Stores frequency into tuning word register Y.
> +		There can be more than one ddsX_freqY file, which allows for
> +		pin controlled FSK Frequency Shift Keying
> +		(ddsX_pincontrol_freq_en is active) or the user can control
> +		the desired active tuning word by writing Y to the
> +		ddsX_freqsymbol file.
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_freq_scaleY
Would the association be clearer if we went with ddsX_freqY_scale?  I think
that is more consistent with what we have elsewhere in IIO (though this particular
double index case hasn't happened before)
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Scale to be applied to ddsX_freqY in order to obtain the
> +		desired value in Hz. If shared across all frequency registers
> +		Y is not present.
Please add that it is also possible X is not present if shared across
all channels.  In weird cases X might not be present whilst Y is...
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_freqsymbol
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Specifies the active output frequency tuning word. The value
> +		corresponds to the Y in ddsX_freqY. To exit this mode the user
> +		can write ddsX_pincontrol_freq_en or ddsX_out_disable file.
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_phaseY
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Stores phase into phase register Y.
> +		There can be more than one ddsX_phaseY file, which allows for
> +		pin controlled PSK Phase Shift Keying
> +		(ddsX_pincontrol_phase_en is active) or the user can
> +		control the desired phase Y which is added to the phase
> +		accumulator output by writing Y to the en_phase file.
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_phase_scaleY
Same reordering suggestion as per the frequency one above.
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Scale to be applied to ddsX_phaseY in order to obtain the
> +		desired value in rad. If shared across all phase registers
> +		Y is not present.
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_phasesymbol
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Specifies the active phase Y which is added to the phase
> +		accumulator output. The value corresponds to the Y in
> +		ddsX_phaseY. To exit this mode the user can write
> +		ddsX_pincontrol_phase_en or disable file.
> +

It might make sense to group the next three by having multiple What lines
and then an explanation covering all three.  
> +What:		/sys/bus/iio/devices/device[n]/ddsX_pincontrol_en
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Both, the active frequency and phase is controlled by the
> +		respective phase and frequency control inputs.
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_pincontrol_freq_en
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The active frequency is controlled by the respective
> +		frequency control/select inputs.
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_pincontrol_phase_en
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The active phase is controlled by the respective
> +		phase control/select inputs.
> +
Could group the next two sections of documentation into one and describe
both versions together.  See how it's done in the latest main IIO docs.
I think it tends to make it apparent when there are multiple very similar
attributes that may affect the same signals.
> +What:		/sys/bus/iio/devices/device[n]/ddsX_out_disable
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Disables any signal generation on all outputs.
With the X in there you need to say for dds X.
On everything else so far we have tended to go with enable attributes rather than
this way around.  Why do it as disable here?
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_outY_disable
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Disables any signal generation on output Y.
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_outY_wavetype
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Specifies the output waveform.
> +		(sine, triangle, ramp, square, ...)
> +		For a list of available output waveform options read
> +		available_output_modes.
> +
> +What:		/sys/bus/iio/devices/device[n]/ddsX_outY_available_wavetypes
Convention so far in IIO (because it's easy to handle in code) would make this
ddsX_outY_wavetype_available. 
> +KernelVersion:	2.6.37
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Lists all available output waveform options.
> --
> 1.6.0.2
Thanks

Jonathan

RE: [RFC 1/3] IIO: Direct digital synthesis abi documentation

[Hennerich, Michael]

> Jonathan Cameron wrote on 2010-12-03:
> On 12/02/10 12:21, michael.hennerich@analog.com wrote:
> > From: Michael Hennerich <michael.hennerich@analog.com>
> >
> > Proposed ABI documentation
> >
> Hi Michael,
>
> Couple of comments inline.
>
> I've raised a few questions that I don't have a clear answer two.
>
> The other comments are nitpicks about exact ordering of the elements
> of the attribute names.
>
> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > ---
> >  .../staging/iio/Documentation/sysfs-bus-iio-dds    |  103
> ++++++++++++++++++++
> >  1 files changed, 103 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-
> dds
> >
> > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
> b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
> > new file mode 100644
> > index 0000000..2c99889
> > --- /dev/null
> > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
> > @@ -0,0 +1,103 @@
> > +
> > +What:              /sys/bus/iio/devices/device[n]/ddsX_freqY
> Here we deviate a little from what we did with input channels.
> In that case there was the existing interface (from hwmon) to match
> so we already had an _input designation to tell us that the number
> was in the relevant base units (here it would be Hz).  Hence we added
> a _raw label to say it wasn't and tell userspace to apply scale and
> offset.
> This is stretching a point somewhat, but looking at the hwmon docs,
> they
> have pwmX_freq as a value in Hz. That's obviously going to make
> consistency
> rather tricky to achieve!
>
> Do you think we should leave all _freq without modifier as being in Hz
> and
> have ddsX_freqY_raw. Or should we rely on userspace verifying if there
> are
> appropriate scale / offset parameters to be applied and hence working
> out
> for itself whether the value in ddsX_freqY is in Hz or not?
>
> I'm think I marginally favour leaving it as you have it here but others
> may
> have different opinions.

Offset is not likely to be used here - but
these devices actually provide sub Hertz resolution. It's very likely to occur,
that we want to have scale being 1000 and the user writes frequency in mHz.
I might even consider using mHz for the sample driver as well.

> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Stores frequency into tuning word register Y.
> > +           There can be more than one ddsX_freqY file, which allows
> for
> > +           pin controlled FSK Frequency Shift Keying
> > +           (ddsX_pincontrol_freq_en is active) or the user can control
> > +           the desired active tuning word by writing Y to the
> > +           ddsX_freqsymbol file.
> > +
> > +What:              /sys/bus/iio/devices/device[n]/ddsX_freq_scaleY
> Would the association be clearer if we went with ddsX_freqY_scale?  I
> think
> that is more consistent with what we have elsewhere in IIO (though this
> particular
> double index case hasn't happened before)

Well - I thought use ddsX_freqY_scale whenever scale is different upon Y.
And leave without index if it is common between ddsX_freqY.
The Y after scale is just a typo.

> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Scale to be applied to ddsX_freqY in order to obtain the
> > +           desired value in Hz. If shared across all frequency
> registers
> > +           Y is not present.
> Please add that it is also possible X is not present if shared across
> all channels.  In weird cases X might not be present whilst Y is...

ok

> > +
> > +What:              /sys/bus/iio/devices/device[n]/ddsX_freqsymbol
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Specifies the active output frequency tuning word. The
> value
> > +           corresponds to the Y in ddsX_freqY. To exit this mode the
> user
> > +           can write ddsX_pincontrol_freq_en or ddsX_out_disable file.
> > +
> > +What:              /sys/bus/iio/devices/device[n]/ddsX_phaseY
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Stores phase into phase register Y.
> > +           There can be more than one ddsX_phaseY file, which allows
> for
> > +           pin controlled PSK Phase Shift Keying
> > +           (ddsX_pincontrol_phase_en is active) or the user can
> > +           control the desired phase Y which is added to the phase
> > +           accumulator output by writing Y to the en_phase file.
> > +
> > +What:              /sys/bus/iio/devices/device[n]/ddsX_phase_scaleY
> Same reordering suggestion as per the frequency one above.
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Scale to be applied to ddsX_phaseY in order to obtain the
> > +           desired value in rad. If shared across all phase registers
> > +           Y is not present.
> > +
> > +What:              /sys/bus/iio/devices/device[n]/ddsX_phasesymbol
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Specifies the active phase Y which is added to the phase
> > +           accumulator output. The value corresponds to the Y in
> > +           ddsX_phaseY. To exit this mode the user can write
> > +           ddsX_pincontrol_phase_en or disable file.
> > +
>
> It might make sense to group the next three by having multiple What
> lines
> and then an explanation covering all three.

Makes sense, thanks.

> > +What:              /sys/bus/iio/devices/device[n]/ddsX_pincontrol_en
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Both, the active frequency and phase is controlled by the
> > +           respective phase and frequency control inputs.
> > +
> > +What:
>       /sys/bus/iio/devices/device[n]/ddsX_pincontrol_freq_en
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           The active frequency is controlled by the respective
> > +           frequency control/select inputs.
> > +
> > +What:
>       /sys/bus/iio/devices/device[n]/ddsX_pincontrol_phase_en
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           The active phase is controlled by the respective
> > +           phase control/select inputs.
> > +
> Could group the next two sections of documentation into one and
> describe
> both versions together.  See how it's done in the latest main IIO docs.
> I think it tends to make it apparent when there are multiple very
> similar
> attributes that may affect the same signals.

ok

> > +What:              /sys/bus/iio/devices/device[n]/ddsX_out_disable
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Disables any signal generation on all outputs.
> With the X in there you need to say for dds X.
> On everything else so far we have tended to go with enable attributes
> rather than
> this way around.  Why do it as disable here?

We can change the logic. The sample driver enables the output once the
ddsX_outY_wavetype file is written.

> > +
> > +What:              /sys/bus/iio/devices/device[n]/ddsX_outY_disable
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Disables any signal generation on output Y.
> > +
> > +What:              /sys/bus/iio/devices/device[n]/ddsX_outY_wavetype
> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Specifies the output waveform.
> > +           (sine, triangle, ramp, square, ...)
> > +           For a list of available output waveform options read
> > +           available_output_modes.
> > +
> > +What:
>       /sys/bus/iio/devices/device[n]/ddsX_outY_available_wavetypes
> Convention so far in IIO (because it's easy to handle in code) would
> make this
> ddsX_outY_wavetype_available.

ok

> > +KernelVersion:     2.6.37
> > +Contact:   linux-iio@vger.kernel.org
> > +Description:
> > +           Lists all available output waveform options.
> > --
> > 1.6.0.2

Thanks for your review.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Re: [RFC 1/3] IIO: Direct digital synthesis abi documentation

[Jonathan Cameron]

On 12/07/10 05:18, Hennerich, Michael wrote:
>> Jonathan Cameron wrote on 2010-12-03:
>> On 12/02/10 12:21, michael.hennerich@analog.com wrote:
>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>
>>> Proposed ABI documentation
>>>
>> Hi Michael,
>>
>> Couple of comments inline.
>>
>> I've raised a few questions that I don't have a clear answer two.
>>
>> The other comments are nitpicks about exact ordering of the elements
>> of the attribute names.
>>
>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>> ---
>>>  .../staging/iio/Documentation/sysfs-bus-iio-dds    |  103
>> ++++++++++++++++++++
>>>  1 files changed, 103 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-
>> dds
>>>
>>> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
>> b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
>>> new file mode 100644
>>> index 0000000..2c99889
>>> --- /dev/null
>>> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds
>>> @@ -0,0 +1,103 @@
>>> +
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_freqY
>> Here we deviate a little from what we did with input channels.
>> In that case there was the existing interface (from hwmon) to match
>> so we already had an _input designation to tell us that the number
>> was in the relevant base units (here it would be Hz).  Hence we added
>> a _raw label to say it wasn't and tell userspace to apply scale and
>> offset.
>> This is stretching a point somewhat, but looking at the hwmon docs,
>> they
>> have pwmX_freq as a value in Hz. That's obviously going to make
>> consistency
>> rather tricky to achieve!
>>
>> Do you think we should leave all _freq without modifier as being in Hz
>> and
>> have ddsX_freqY_raw. Or should we rely on userspace verifying if there
>> are
>> appropriate scale / offset parameters to be applied and hence working
>> out
>> for itself whether the value in ddsX_freqY is in Hz or not?
>>
>> I'm think I marginally favour leaving it as you have it here but others
>> may
>> have different opinions.
> 
> Offset is not likely to be used here - but
> these devices actually provide sub Hertz resolution. It's very likely to occur,
> that we want to have scale being 1000 and the user writes frequency in mHz.
> I might even consider using mHz for the sample driver as well.
I guess it's a question of whether doing the fixed point arithmetic in kernel
is cheap enough to use Hz but allow for a decimal point?  That would remove the
need for the _scale parameter which would simplify the user interface slightly.

Lets go with what you originally suggested. It works and with clear documentation
the difference between it and some of our other 'frequency' elements shouldn't
confuse anyone.
> 
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Stores frequency into tuning word register Y.
>>> +           There can be more than one ddsX_freqY file, which allows
>> for
>>> +           pin controlled FSK Frequency Shift Keying
>>> +           (ddsX_pincontrol_freq_en is active) or the user can control
>>> +           the desired active tuning word by writing Y to the
>>> +           ddsX_freqsymbol file.
>>> +
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_freq_scaleY
>> Would the association be clearer if we went with ddsX_freqY_scale?  I
>> think
>> that is more consistent with what we have elsewhere in IIO (though this
>> particular
>> double index case hasn't happened before)
> 
> Well - I thought use ddsX_freqY_scale whenever scale is different upon Y.
> And leave without index if it is common between ddsX_freqY.
> The Y after scale is just a typo.
Cool, without the Y it is just fine.  We can document the Y case if/when it
turns up in a driver.
> 
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Scale to be applied to ddsX_freqY in order to obtain the
>>> +           desired value in Hz. If shared across all frequency
>> registers
>>> +           Y is not present.
>> Please add that it is also possible X is not present if shared across
>> all channels.  In weird cases X might not be present whilst Y is...
> 
> ok
> 
>>> +
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_freqsymbol
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Specifies the active output frequency tuning word. The
>> value
>>> +           corresponds to the Y in ddsX_freqY. To exit this mode the
>> user
>>> +           can write ddsX_pincontrol_freq_en or ddsX_out_disable file.
>>> +
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_phaseY
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Stores phase into phase register Y.
>>> +           There can be more than one ddsX_phaseY file, which allows
>> for
>>> +           pin controlled PSK Phase Shift Keying
>>> +           (ddsX_pincontrol_phase_en is active) or the user can
>>> +           control the desired phase Y which is added to the phase
>>> +           accumulator output by writing Y to the en_phase file.
>>> +
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_phase_scaleY
>> Same reordering suggestion as per the frequency one above.
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Scale to be applied to ddsX_phaseY in order to obtain the
>>> +           desired value in rad. If shared across all phase registers
>>> +           Y is not present.
>>> +
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_phasesymbol
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Specifies the active phase Y which is added to the phase
>>> +           accumulator output. The value corresponds to the Y in
>>> +           ddsX_phaseY. To exit this mode the user can write
>>> +           ddsX_pincontrol_phase_en or disable file.
>>> +
>>
>> It might make sense to group the next three by having multiple What
>> lines
>> and then an explanation covering all three.
> 
> Makes sense, thanks.
> 
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_pincontrol_en
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Both, the active frequency and phase is controlled by the
>>> +           respective phase and frequency control inputs.
>>> +
>>> +What:
>>       /sys/bus/iio/devices/device[n]/ddsX_pincontrol_freq_en
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           The active frequency is controlled by the respective
>>> +           frequency control/select inputs.
>>> +
>>> +What:
>>       /sys/bus/iio/devices/device[n]/ddsX_pincontrol_phase_en
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           The active phase is controlled by the respective
>>> +           phase control/select inputs.
>>> +
>> Could group the next two sections of documentation into one and
>> describe
>> both versions together.  See how it's done in the latest main IIO docs.
>> I think it tends to make it apparent when there are multiple very
>> similar
>> attributes that may affect the same signals.
> 
> ok
> 
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_out_disable
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Disables any signal generation on all outputs.
>> With the X in there you need to say for dds X.
>> On everything else so far we have tended to go with enable attributes
>> rather than
>> this way around.  Why do it as disable here?
> 
> We can change the logic. The sample driver enables the output once the
> ddsX_outY_wavetype file is written.
Is that a good idea? What if a device is dependant on having a very particular
frequency fed to it and the user not knowing that wavetype turns things on might
set that before the frequency? The semantics don't make it clear that one must set the
wavetype last.  I would think separating the configuration from enable/disable
might be more intuitive. 
> 
>>> +
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_outY_disable
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Disables any signal generation on output Y.
>>> +
>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_outY_wavetype
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Specifies the output waveform.
>>> +           (sine, triangle, ramp, square, ...)
>>> +           For a list of available output waveform options read
>>> +           available_output_modes.
>>> +
>>> +What:
>>       /sys/bus/iio/devices/device[n]/ddsX_outY_available_wavetypes
>> Convention so far in IIO (because it's easy to handle in code) would
>> make this
>> ddsX_outY_wavetype_available.
> 
> ok
> 
>>> +KernelVersion:     2.6.37
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Lists all available output waveform options.
>>> --
>>> 1.6.0.2
> 
> Thanks for your review.
You are welcome,

Jonathan
 

RE: [RFC 1/3] IIO: Direct digital synthesis abi documentation

[Hennerich, Michael]

Jonathan Cameron wrote on 2010-12-07:
> On 12/07/10 05:18, Hennerich, Michael wrote:
>>> Jonathan Cameron wrote on 2010-12-03:
>>> On 12/02/10 12:21, michael.hennerich@analog.com wrote:
>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>
>>>> Proposed ABI documentation
>>>>

>>>>What:              /sys/bus/iio/devices/device[n]/ddsX_freqY
>>> Here we deviate a little from what we did with input channels. In that
>>> case there was the existing interface (from hwmon) to match so we
>>> already had an _input designation to tell us that the number was in
>>> the relevant base units (here it would be Hz).  Hence we added a _raw
>>> label to say it wasn't and tell userspace to apply scale and offset.
>>> This is stretching a point somewhat, but looking at the hwmon docs,
>>> they have pwmX_freq as a value in Hz. That's obviously going to make
>>> consistency rather tricky to achieve!
>>>
>>> Do you think we should leave all _freq without modifier as being in Hz
>>> and have ddsX_freqY_raw. Or should we rely on userspace verifying if
>>> there are appropriate scale / offset parameters to be applied and
>>> hence working out for itself whether the value in ddsX_freqY is in Hz
>>> or not?
>>>
>>> I'm think I marginally favour leaving it as you have it here but
>>> others may have different opinions.
>>
>> Offset is not likely to be used here - but these devices actually
>> provide sub Hertz resolution. It's very likely to occur, that we
>> want to have scale being 1000 and the user writes frequency in mHz.
>> I might even consider using mHz for the sample driver as well.
> I guess it's a question of whether doing the fixed point arithmetic in
> kernel is cheap enough to use Hz but allow for a decimal point?  That
> would remove the need for the _scale parameter which would simplify
> the user interface slightly.
>
> Lets go with what you originally suggested. It works and with clear
> documentation the difference between it and some of our other
> 'frequency' elements shouldn't confuse anyone.

I prefer the _scale parameter.
Not aware of any other sysfs files, taking decimal points.

>>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_out_disable
>>>> +KernelVersion:     2.6.37
>>>> +Contact:   linux-iio@vger.kernel.org
>>>> +Description:
>>>> +           Disables any signal generation on all outputs.
>>> With the X in there you need to say for dds X. On everything else so
>>> far we have tended to go with enable attributes rather than this way
>>> around.  Why do it as disable here?
>>
>> We can change the logic. The sample driver enables the output once the
>> ddsX_outY_wavetype file is written.
> Is that a good idea? What if a device is dependant on having a very
> particular frequency fed to it and the user not knowing that wavetype
> turns things on might set that before the frequency? The semantics
> don't make it clear that one must set the wavetype last.  I would
> think separating the configuration from enable/disable might be more
> intuitive.

I agree.

Thanks.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Re: [RFC 1/3] IIO: Direct digital synthesis abi documentation

[Jonathan Cameron]

On 12/09/10 05:48, Hennerich, Michael wrote:
> Jonathan Cameron wrote on 2010-12-07:
>> On 12/07/10 05:18, Hennerich, Michael wrote:
>>>> Jonathan Cameron wrote on 2010-12-03:
>>>> On 12/02/10 12:21, michael.hennerich@analog.com wrote:
>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>
>>>>> Proposed ABI documentation
>>>>>
> 
>>>>> What:              /sys/bus/iio/devices/device[n]/ddsX_freqY
>>>> Here we deviate a little from what we did with input channels. In that
>>>> case there was the existing interface (from hwmon) to match so we
>>>> already had an _input designation to tell us that the number was in
>>>> the relevant base units (here it would be Hz).  Hence we added a _raw
>>>> label to say it wasn't and tell userspace to apply scale and offset.
>>>> This is stretching a point somewhat, but looking at the hwmon docs,
>>>> they have pwmX_freq as a value in Hz. That's obviously going to make
>>>> consistency rather tricky to achieve!
>>>>
>>>> Do you think we should leave all _freq without modifier as being in Hz
>>>> and have ddsX_freqY_raw. Or should we rely on userspace verifying if
>>>> there are appropriate scale / offset parameters to be applied and
>>>> hence working out for itself whether the value in ddsX_freqY is in Hz
>>>> or not?
>>>>
>>>> I'm think I marginally favour leaving it as you have it here but
>>>> others may have different opinions.
>>>
>>> Offset is not likely to be used here - but these devices actually
>>> provide sub Hertz resolution. It's very likely to occur, that we
>>> want to have scale being 1000 and the user writes frequency in mHz.
>>> I might even consider using mHz for the sample driver as well.
>> I guess it's a question of whether doing the fixed point arithmetic in
>> kernel is cheap enough to use Hz but allow for a decimal point?  That
>> would remove the need for the _scale parameter which would simplify
>> the user interface slightly.
>>
>> Lets go with what you originally suggested. It works and with clear
>> documentation the difference between it and some of our other
>> 'frequency' elements shouldn't confuse anyone.
> 
> I prefer the _scale parameter.
Fine with me.
> Not aware of any other sysfs files, taking decimal points.
Hmm.. There is at least on in IIO that takes decimal inputs (hmc5834),
though I think only in the category where there is an _available attribute
to list the possibilities. Output wise quite a few use decimals
(your ad799x for starters!)  IIRC there are a few more cases in drivers
Alan Cox has submitted recently (not sure what merge status is on those).



>>>>> +What:              /sys/bus/iio/devices/device[n]/ddsX_out_disable
>>>>> +KernelVersion:     2.6.37
>>>>> +Contact:   linux-iio@vger.kernel.org
>>>>> +Description:
>>>>> +           Disables any signal generation on all outputs.
>>>> With the X in there you need to say for dds X. On everything else so
>>>> far we have tended to go with enable attributes rather than this way
>>>> around.  Why do it as disable here?
>>>
>>> We can change the logic. The sample driver enables the output once the
>>> ddsX_outY_wavetype file is written.
>> Is that a good idea? What if a device is dependant on having a very
>> particular frequency fed to it and the user not knowing that wavetype
>> turns things on might set that before the frequency? The semantics
>> don't make it clear that one must set the wavetype last.  I would
>> think separating the configuration from enable/disable might be more
>> intuitive.
> 
> I agree.

Looks like we've pinned this down so that we are both happy. No one else
has picked up on it, so I guess no one else is currently interested in
DDS support.

I'll be happy to do a final review of an updated patch set.

Thanks,

Jonathan

[RFC 2/3] IIO: dds.h convenience macros

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Attributes considered as common between various DDS devices

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/staging/iio/dds/dds.h |  153 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 153 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/dds/dds.h

diff --git a/drivers/staging/iio/dds/dds.h b/drivers/staging/iio/dds/dds.h
new file mode 100644
index 0000000..e171b33
--- /dev/null
+++ b/drivers/staging/iio/dds/dds.h
@@ -0,0 +1,153 @@
+/*
+ * dds.h - sysfs attributes associated with DDS devices
+ *
+ * Copyright (c) 2010 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_freqY
+ * Description:
+ * Stores frequency into tuning word register Y.
+ * There can be more than one ddsX_freqY file, which allows for pin controlled
+ * FSK Frequency Shift Keying (ddsX_pincontrol_freq_en is active) or the user
+ * can control the desired active tuning word by writing Y to the
+ * ddsX_freqsymbol file.
+ */
+
+#define IIO_DEV_ATTR_FREQ(_device, _num, _store, _addr)			\
+	IIO_DEVICE_ATTR(dds##_device##_freq##_num, S_IWUSR, NULL, _store, _addr)
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_freq_scaleY
+ * Description:
+ * Scale to be applied to ddsX_freqY in order to obtain the
+ * desired value in Hz. If shared across all frequency registers
+ * Y is not present.
+ */
+
+#define IIO_CONST_ATTR_FREQ_SCALE(_device, _string)			\
+	IIO_CONST_ATTR(dds##_device##_freq_scale, _string)
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_freqsymbol
+ * Description:
+ * Specifies the active output frequency tuning word. The value corresponds
+ * to the Y in ddsX_freqY. To exit this mode the user can write
+ * ddsX_pincontrol_freq_en or ddsX_out_disable file.
+ */
+
+#define IIO_DEV_ATTR_FREQSYMBOL(_device, _store, _addr)			\
+	IIO_DEVICE_ATTR(dds##_device##_freqsymbol,			\
+			S_IWUSR, NULL, _store, _addr);
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_phaseY
+ * Description:
+ * Stores phase into phase register Y.
+ * There can be more than one ddsX_phaseY file, which allows for pin controlled
+ * PSK Phase Shift Keying (ddsX_pincontrol_phase_en is active) or the user can
+ * control the desired phase Y which is added to the phase accumulator output
+ * by writing Y to the en_phase file.
+ */
+
+#define IIO_DEV_ATTR_PHASE(_device, _num, _store, _addr)		\
+	IIO_DEVICE_ATTR(dds##_device##_phase##_num,			\
+			S_IWUSR, NULL, _store, _addr)
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_phase_scaleY
+ * Description:
+ * Scale to be applied to ddsX_phaseY in order to obtain the
+ * desired value in rad. If shared across all phase registers
+ * Y is not present.
+ */
+
+#define IIO_CONST_ATTR_PHASE_SCALE(_device, _string)			\
+	IIO_CONST_ATTR(dds##_device##_phase_scale, _string)
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_phasesymbol
+ * Description:
+ * Specifies the active phase Y which is added to the phase accumulator output.
+ * The value corresponds to the Y in ddsX_phaseY. To exit this mode the user
+ * can write ddsX_pincontrol_phase_en or disable file.
+ */
+
+#define IIO_DEV_ATTR_PHASESYMBOL(_device, _store, _addr)		\
+	IIO_DEVICE_ATTR(dds##_device##_phasesymbol,			\
+			S_IWUSR, NULL, _store, _addr);
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_pincontrol_en
+ * Description:
+ * Both, the active frequency and phase is controlled by the respective
+ * phase and frequency control inputs.
+ */
+
+#define IIO_DEV_ATTR_PINCONTROL_EN(_device, _store, _addr)		\
+	IIO_DEVICE_ATTR(dds##_device##_pincontrol_en,			\
+			S_IWUSR, NULL, _store, _addr);
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_pincontrol_freq_en
+ * Description:
+ * The active frequency is controlled by the respective
+ * frequency control/select inputs.
+ */
+
+#define IIO_DEV_ATTR_PINCONTROL_FREQ_EN(_device, _store, _addr)		\
+	IIO_DEVICE_ATTR(dds##_device##_pincontrol_freq_en,		\
+			S_IWUSR, NULL, _store, _addr);
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_pincontrol_phase_en
+ * Description:
+ * The active phase is controlled by the respective
+ * phase control/select inputs.
+ */
+
+#define IIO_DEV_ATTR_PINCONTROL_PHASE_EN(_device, _store, _addr)	\
+	IIO_DEVICE_ATTR(dds##_device##_pincontrol_phase_en,		\
+			S_IWUSR, NULL, _store, _addr);
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_out_disable
+ * Description:
+ * Disables any signal generation on all outputs.
+ */
+
+#define IIO_DEV_ATTR_OUT_DISABLE(_device, _store, _addr)		\
+	IIO_DEVICE_ATTR(dds##_device##_out_disable,			\
+			S_IWUSR, NULL, _store, _addr);
+
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_outY_disable
+ * Description:
+ * Disables any signal generation on output Y.
+ */
+
+#define IIO_DEV_ATTR_OUTY_DISABLE(_device, _output, _store, _addr)	\
+	IIO_DEVICE_ATTR(dds##_device##_out##_output##_disable,		\
+	S_IWUSR, NULL, _store, _addr);
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_outY_wavetype
+ * Description:
+ * Specifies the output waveform. (sine, triangle, ramp, square, ...)
+ * For a list of available output waveform options read available_output_modes.
+ */
+#define IIO_DEV_ATTR_OUT_WAVETYPE(_device, _output, _store, _addr)	\
+	IIO_DEVICE_ATTR(dds##_device##_out##_output##_wavetype,		\
+			S_IWUSR, NULL, _store, _addr);
+
+/**
+ * /sys/bus/iio/devices/deviceX/ddsX_outY_available_wavetypes
+ * Description:
+ * Lists all available output waveform options.
+ */
+#define IIO_CONST_ATTR_OUT_AVAILABLE_WAVETYPES(_device, _output, _modes)\
+	IIO_CONST_ATTR(dds##_device##_out##_output##_available_wavetypes,\
+			_modes);
--
1.6.0.2

[RFC 3/3] IIO: DDS: AD9833 / AD9834 driver

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Example driver using the proposed ABI

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/staging/iio/dds/Kconfig  |    7 +
 drivers/staging/iio/dds/Makefile |    1 +
 drivers/staging/iio/dds/ad9834.c |  483 ++++++++++++++++++++++++++++++++++++++
 drivers/staging/iio/dds/ad9834.h |  112 +++++++++
 4 files changed, 603 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/dds/ad9834.c
 create mode 100644 drivers/staging/iio/dds/ad9834.h

diff --git a/drivers/staging/iio/dds/Kconfig b/drivers/staging/iio/dds/Kconfig
index 7969be2..4c9cce3 100644
--- a/drivers/staging/iio/dds/Kconfig
+++ b/drivers/staging/iio/dds/Kconfig
@@ -17,6 +17,13 @@ config AD9832
 	  Say yes here to build support for Analog Devices DDS chip
 	  ad9832 and ad9835, provides direct access via sysfs.

+config AD9834
+	tristate "Analog Devices ad9833/4/ driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Analog Devices DDS chip
+	  AD9833 and AD9834, provides direct access via sysfs.
+
 config AD9850
 	tristate "Analog Devices ad9850/1 driver"
 	depends on SPI
diff --git a/drivers/staging/iio/dds/Makefile b/drivers/staging/iio/dds/Makefile
index 6f274ac..1477461 100644
--- a/drivers/staging/iio/dds/Makefile
+++ b/drivers/staging/iio/dds/Makefile
@@ -4,6 +4,7 @@

 obj-$(CONFIG_AD5930) += ad5930.o
 obj-$(CONFIG_AD9832) += ad9832.o
+obj-$(CONFIG_AD9834) += ad9834.o
 obj-$(CONFIG_AD9850) += ad9850.o
 obj-$(CONFIG_AD9852) += ad9852.o
 obj-$(CONFIG_AD9910) += ad9910.o
diff --git a/drivers/staging/iio/dds/ad9834.c b/drivers/staging/iio/dds/ad9834.c
new file mode 100644
index 0000000..eb1fabf
--- /dev/null
+++ b/drivers/staging/iio/dds/ad9834.c
@@ -0,0 +1,483 @@
+/*
+ * AD9834 SPI DAC driver
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/list.h>
+#include <linux/spi/spi.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <asm/div64.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
+#include "dds.h"
+
+#include "ad9834.h"
+
+static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long fout)
+{
+	unsigned long long freqreg = (u64) fout * (u64) (1 << AD9834_FREQ_BITS);
+	do_div(freqreg, mclk);
+	return freqreg;
+}
+
+static int ad9834_write_frequency(struct ad9834_state *st,
+				  unsigned long addr, unsigned long fout)
+{
+	unsigned long regval;
+
+	if (fout > (st->mclk / 2))
+		return -EINVAL;
+
+	regval = ad9834_calc_freqreg(st->mclk, fout);
+
+	st->freq_data[0] = cpu_to_be16(addr | (regval &
+				       RES_MASK(AD9834_FREQ_BITS / 2)));
+	st->freq_data[1] = cpu_to_be16(addr | ((regval >>
+				       (AD9834_FREQ_BITS / 2)) &
+				       RES_MASK(AD9834_FREQ_BITS / 2)));
+
+	return spi_sync(st->spi, &st->freq_msg);;
+}
+
+static int ad9834_write_phase(struct ad9834_state *st,
+				  unsigned long addr, unsigned long phase)
+{
+	if (phase > (1 << AD9834_PHASE_BITS))
+		return -EINVAL;
+	st->data = cpu_to_be16(addr | phase);
+
+	return spi_sync(st->spi, &st->msg);
+}
+
+static ssize_t ad9834_write(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf,
+		size_t len)
+{
+	struct iio_dev *dev_info = dev_get_drvdata(dev);
+	struct ad9834_state *st = dev_info->dev_data;
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	int ret;
+	long val;
+
+	ret = strict_strtol(buf, 10, &val);
+	if (ret)
+		goto error_ret;
+
+	mutex_lock(&dev_info->mlock);
+	switch (this_attr->address) {
+	case AD9834_REG_FREQ0:
+	case AD9834_REG_FREQ1:
+		ret = ad9834_write_frequency(st, this_attr->address, val);
+		break;
+	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 */
+			break;
+		}
+
+		if (val)
+			st->control &= ~AD9834_OPBITEN;
+		else
+			st->control |= AD9834_OPBITEN;
+		st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
+		ret = spi_sync(st->spi, &st->msg);
+		break;
+
+	case AD9834_PIN_SW:
+		if (val)
+			st->control |= AD9834_PIN_SW;
+		else
+			st->control &= ~AD9834_PIN_SW;
+		st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
+		ret = spi_sync(st->spi, &st->msg);
+		break;
+	case AD9834_FSEL:
+	case AD9834_PSEL:
+		if (val == 0)
+			st->control &= ~(this_attr->address | AD9834_PIN_SW);
+		else if (val == 1) {
+			st->control |= this_attr->address;
+			st->control &= ~AD9834_PIN_SW;
+		} else {
+			ret = -EINVAL;
+			break;
+		}
+		st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
+		ret = spi_sync(st->spi, &st->msg);
+		break;
+	case AD9834_RESET:
+		if (val)
+			st->control |= AD9834_RESET;
+		else
+			st->control &= ~AD9834_RESET;
+		st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
+		ret = spi_sync(st->spi, &st->msg);
+		break;
+	default:
+		ret = -ENODEV;
+	}
+	mutex_unlock(&dev_info->mlock);
+
+error_ret:
+	return ret ? ret : len;
+}
+
+static ssize_t ad9834_store_wavetype(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf,
+				 size_t len)
+{
+	struct iio_dev *dev_info = dev_get_drvdata(dev);
+	struct ad9834_state *st = dev_info->dev_data;
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	int ret = 0;
+	bool is_ad9833 = st->devid == ID_AD9833;
+
+	mutex_lock(&dev_info->mlock);
+
+	switch (this_attr->address) {
+	case 0:
+		if (sysfs_streq(buf, "sine")) {
+			st->control &= ~AD9834_MODE;
+			if (is_ad9833)
+				st->control &= ~AD9834_OPBITEN;
+		} else if (sysfs_streq(buf, "triangle")) {
+			if (is_ad9833) {
+				st->control &= ~AD9834_OPBITEN;
+				st->control |= AD9834_MODE;
+			} else if (st->control & AD9834_OPBITEN) {
+				ret = -EINVAL;	/* AD9843 reserved mode */
+			} else {
+				st->control |= AD9834_MODE;
+			}
+		} else if (is_ad9833 && sysfs_streq(buf, "square")) {
+			st->control &= ~AD9834_MODE;
+			st->control |= AD9834_OPBITEN;
+		} else {
+			ret = -EINVAL;
+		}
+
+		break;
+	case 1:
+		if (sysfs_streq(buf, "square") &&
+			!(st->control & AD9834_MODE)) {
+			st->control &= ~AD9834_MODE;
+			st->control |= AD9834_OPBITEN;
+		} else {
+			ret = -EINVAL;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	if (!ret) {
+		st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
+		ret = spi_sync(st->spi, &st->msg);
+	}
+		mutex_unlock(&dev_info->mlock);
+
+	return ret ? ret : len;
+}
+
+static ssize_t ad9834_show_name(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct iio_dev *dev_info = dev_get_drvdata(dev);
+	struct ad9834_state *st = iio_dev_get_devdata(dev_info);
+
+	return sprintf(buf, "%s\n", spi_get_device_id(st->spi)->name);
+}
+static IIO_DEVICE_ATTR(name, S_IRUGO, ad9834_show_name, NULL, 0);
+
+static ssize_t ad9834_show_out0_available_wavetypes(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct iio_dev *dev_info = dev_get_drvdata(dev);
+	struct ad9834_state *st = iio_dev_get_devdata(dev_info);
+	char *str;
+
+	if (st->devid == ID_AD9833)
+		str = "sine triangle square";
+	else if (st->control & AD9834_OPBITEN)
+		str = "sine";
+	else
+		str = "sine triangle";
+
+	return sprintf(buf, "%s\n", str);
+}
+
+
+static IIO_DEVICE_ATTR(dds0_out0_available_wavetypes, S_IRUGO,
+		       ad9834_show_out0_available_wavetypes, NULL, 0);
+
+static ssize_t ad9834_show_out1_available_wavetypes(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct iio_dev *dev_info = dev_get_drvdata(dev);
+	struct ad9834_state *st = iio_dev_get_devdata(dev_info);
+	char *str;
+
+	if (st->control & AD9834_MODE)
+		str = "";
+	else
+		str = "square";
+
+	return sprintf(buf, "%s\n", str);
+}
+
+static IIO_DEVICE_ATTR(dds0_out1_available_wavetypes, S_IRUGO,
+		       ad9834_show_out1_available_wavetypes, NULL, 0);
+
+/**
+ * see dds.h for further information
+ */
+
+
+static IIO_DEV_ATTR_FREQ(0, 0, ad9834_write, AD9834_REG_FREQ0);
+static IIO_DEV_ATTR_FREQ(0, 1, ad9834_write, AD9834_REG_FREQ1);
+static IIO_DEV_ATTR_FREQSYMBOL(0, ad9834_write, AD9834_FSEL);
+static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
+
+static IIO_DEV_ATTR_PHASE(0, 0, ad9834_write, AD9834_REG_PHASE0);
+static IIO_DEV_ATTR_PHASE(0, 1, ad9834_write, AD9834_REG_PHASE1);
+static IIO_DEV_ATTR_PHASESYMBOL(0, ad9834_write, AD9834_PSEL);
+static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
+
+static IIO_DEV_ATTR_PINCONTROL_EN(0, ad9834_write, AD9834_PIN_SW);
+static IIO_DEV_ATTR_OUT_DISABLE(0, ad9834_write, AD9834_RESET);
+static IIO_DEV_ATTR_OUTY_DISABLE(0, 1, ad9834_write, AD9834_OPBITEN);
+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_dev_attr_dds0_freq0.dev_attr.attr,
+	&iio_dev_attr_dds0_freq1.dev_attr.attr,
+	&iio_const_attr_dds0_freq_scale.dev_attr.attr,
+	&iio_dev_attr_dds0_phase0.dev_attr.attr,
+	&iio_dev_attr_dds0_phase1.dev_attr.attr,
+	&iio_const_attr_dds0_phase_scale.dev_attr.attr,
+	&iio_dev_attr_dds0_pincontrol_en.dev_attr.attr,
+	&iio_dev_attr_dds0_freqsymbol.dev_attr.attr,
+	&iio_dev_attr_dds0_phasesymbol.dev_attr.attr,
+	&iio_dev_attr_dds0_out_disable.dev_attr.attr,
+	&iio_dev_attr_dds0_out1_disable.dev_attr.attr,
+	&iio_dev_attr_dds0_out0_wavetype.dev_attr.attr,
+	&iio_dev_attr_dds0_out1_wavetype.dev_attr.attr,
+	&iio_dev_attr_dds0_out0_available_wavetypes.dev_attr.attr,
+	&iio_dev_attr_dds0_out1_available_wavetypes.dev_attr.attr,
+	&iio_dev_attr_name.dev_attr.attr,
+	NULL,
+};
+
+static mode_t ad9834_attr_is_visible(struct kobject *kobj,
+				     struct attribute *attr, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct iio_dev *dev_info = dev_get_drvdata(dev);
+	struct ad9834_state *st = iio_dev_get_devdata(dev_info);
+
+	mode_t mode = attr->mode;
+
+	if (st->devid == ID_AD9834)
+		return mode;
+
+	if ((attr == &iio_dev_attr_dds0_out1_disable.dev_attr.attr) ||
+		(attr == &iio_dev_attr_dds0_out1_wavetype.dev_attr.attr) ||
+		(attr ==
+		&iio_dev_attr_dds0_out1_available_wavetypes.dev_attr.attr))
+		mode = 0;
+
+	return mode;
+}
+
+static const struct attribute_group ad9834_attribute_group = {
+	.attrs = ad9834_attributes,
+	.is_visible = ad9834_attr_is_visible,
+};
+
+static int __devinit ad9834_probe(struct spi_device *spi)
+{
+	struct ad9834_platform_data *pdata = spi->dev.platform_data;
+	struct ad9834_state *st;
+	int ret;
+
+	if (!pdata) {
+		dev_dbg(&spi->dev, "no platform data?\n");
+		return -ENODEV;
+	}
+
+	st = kzalloc(sizeof(*st), GFP_KERNEL);
+	if (st == NULL) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+
+	st->reg = regulator_get(&spi->dev, "vcc");
+	if (!IS_ERR(st->reg)) {
+		ret = regulator_enable(st->reg);
+		if (ret)
+			goto error_put_reg;
+	}
+
+	st->mclk = pdata->mclk;
+
+	spi_set_drvdata(spi, st);
+
+	st->spi = spi;
+	st->devid = spi_get_device_id(spi)->driver_data;
+
+	st->indio_dev = iio_allocate_device();
+	if (st->indio_dev == NULL) {
+		ret = -ENOMEM;
+		goto error_disable_reg;
+	}
+
+	st->indio_dev->dev.parent = &spi->dev;
+	st->indio_dev->attrs = &ad9834_attribute_group;
+	st->indio_dev->dev_data = (void *)(st);
+	st->indio_dev->driver_module = THIS_MODULE;
+	st->indio_dev->modes = INDIO_DIRECT_MODE;
+
+	/* Setup default messages */
+
+	st->xfer.tx_buf = &st->data;
+	st->xfer.len = 2;
+
+	spi_message_init(&st->msg);
+	spi_message_add_tail(&st->xfer, &st->msg);
+
+	st->freq_xfer[0].tx_buf = &st->freq_data[0];
+	st->freq_xfer[0].len = 2;
+	st->freq_xfer[0].cs_change = 1;
+	st->freq_xfer[1].tx_buf = &st->freq_data[1];
+	st->freq_xfer[1].len = 2;
+
+	spi_message_init(&st->freq_msg);
+	spi_message_add_tail(&st->freq_xfer[0], &st->freq_msg);
+	spi_message_add_tail(&st->freq_xfer[1], &st->freq_msg);
+
+	st->control = AD9834_B28 | AD9834_RESET;
+
+	if (!pdata->en_div2)
+		st->control |= AD9834_DIV2;
+
+	if (!pdata->en_signbit_msb_out && (st->devid == ID_AD9834))
+		st->control |= AD9834_SIGN_PIB;
+
+	st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
+	ret = spi_sync(st->spi, &st->msg);
+	if (ret) {
+		dev_err(&spi->dev, "device init failed\n");
+		goto error_free_device;
+	}
+
+	ret = ad9834_write_frequency(st, AD9834_REG_FREQ0, pdata->freq0);
+	if (ret)
+		goto error_free_device;
+
+	ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, pdata->freq1);
+	if (ret)
+		goto error_free_device;
+
+	ret = ad9834_write_phase(st, AD9834_REG_PHASE0, pdata->phase0);
+	if (ret)
+		goto error_free_device;
+
+	ret = ad9834_write_phase(st, AD9834_REG_PHASE1, pdata->phase1);
+	if (ret)
+		goto error_free_device;
+
+	st->control &= ~AD9834_RESET;
+	st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
+	ret = spi_sync(st->spi, &st->msg);
+	if (ret)
+		goto error_free_device;
+
+	ret = iio_device_register(st->indio_dev);
+	if (ret)
+		goto error_free_device;
+
+	return 0;
+
+error_free_device:
+	iio_free_device(st->indio_dev);
+error_disable_reg:
+	if (!IS_ERR(st->reg))
+		regulator_disable(st->reg);
+error_put_reg:
+	if (!IS_ERR(st->reg))
+		regulator_put(st->reg);
+	kfree(st);
+error_ret:
+	return ret;
+}
+
+static int ad9834_remove(struct spi_device *spi)
+{
+	struct ad9834_state *st = spi_get_drvdata(spi);
+	struct iio_dev *indio_dev = st->indio_dev;
+
+	iio_device_unregister(indio_dev);
+	if (!IS_ERR(st->reg)) {
+		regulator_disable(st->reg);
+		regulator_put(st->reg);
+	}
+	kfree(st);
+	return 0;
+}
+
+static const struct spi_device_id ad9834_id[] = {
+	{"ad9833", ID_AD9833},
+	{"ad9834", ID_AD9834},
+	{}
+};
+
+static struct spi_driver ad9834_driver = {
+	.driver = {
+		.name	= "ad9834",
+		.bus	= &spi_bus_type,
+		.owner	= THIS_MODULE,
+	},
+	.probe		= ad9834_probe,
+	.remove		= __devexit_p(ad9834_remove),
+	.id_table	= ad9834_id,
+};
+
+static int __init ad9834_init(void)
+{
+	return spi_register_driver(&ad9834_driver);
+}
+module_init(ad9834_init);
+
+static void __exit ad9834_exit(void)
+{
+	spi_unregister_driver(&ad9834_driver);
+}
+module_exit(ad9834_exit);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Analog Devices AD9833/AD9834 DDS");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:ad9834");
diff --git a/drivers/staging/iio/dds/ad9834.h b/drivers/staging/iio/dds/ad9834.h
new file mode 100644
index 0000000..0fc3b88
--- /dev/null
+++ b/drivers/staging/iio/dds/ad9834.h
@@ -0,0 +1,112 @@
+/*
+ * AD9834 SPI DDS driver
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+#ifndef IIO_DDS_AD9834_H_
+#define IIO_DDS_AD9834_H_
+
+/* Registers */
+
+#define AD9834_REG_CMD		(0 << 14)
+#define AD9834_REG_FREQ0	(1 << 14)
+#define AD9834_REG_FREQ1	(2 << 14)
+#define AD9834_REG_PHASE0	(6 << 13)
+#define AD9834_REG_PHASE1	(7 << 13)
+
+/* Command Control Bits */
+
+#define AD9834_B28		(1 << 13)
+#define AD9834_HLB		(1 << 12)
+#define AD9834_FSEL		(1 << 11)
+#define AD9834_PSEL		(1 << 10)
+#define AD9834_PIN_SW		(1 << 9)
+#define AD9834_RESET		(1 << 8)
+#define AD9834_SLEEP1		(1 << 7)
+#define AD9834_SLEEP12		(1 << 6)
+#define AD9834_OPBITEN		(1 << 5)
+#define AD9834_SIGN_PIB		(1 << 4)
+#define AD9834_DIV2		(1 << 3)
+#define AD9834_MODE		(1 << 1)
+
+#define AD9834_FREQ_BITS	28
+#define AD9834_PHASE_BITS	12
+
+#define RES_MASK(bits)	((1 << (bits)) - 1)
+
+/**
+ * struct ad9834_state - driver instance specific data
+ * @indio_dev:		the industrial I/O device
+ * @spi:		spi_device
+ * @reg:		supply regulator
+ * @mclk:		external master clock
+ * @control:		cached control word
+ * @xfer:		default spi transfer
+ * @msg:		default spi message
+ * @freq_xfer:		tuning word spi transfer
+ * @freq_msg:		tuning word spi message
+ * @data:		spi transmit buffer
+ * @freq_data:		tuning word spi transmit buffer
+ */
+
+struct ad9834_state {
+	struct iio_dev			*indio_dev;
+	struct spi_device		*spi;
+	struct regulator		*reg;
+	unsigned int			mclk;
+	unsigned short			control;
+	unsigned short			devid;
+	struct spi_transfer		xfer;
+	struct spi_message		msg;
+	struct spi_transfer		freq_xfer[2];
+	struct spi_message		freq_msg;
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	unsigned short			data ____cacheline_aligned;
+	unsigned short			freq_data[2] ;
+};
+
+
+/*
+ * TODO: struct ad7887_platform_data needs to go into include/linux/iio
+ */
+
+/**
+ * struct ad9834_platform_data - platform specific information
+ * @mclk:		master clock in Hz
+ * @freq0:		power up freq0 tuning word in Hz
+ * @freq1:		power up freq1 tuning word in Hz
+ * @phase0:		power up phase0 value [0..4095] correlates with 0..2PI
+ * @phase1:		power up phase1 value [0..4095] correlates with 0..2PI
+ * @en_div2:		digital output/2 is passed to the SIGN BIT OUT pin
+ * @en_signbit_msb_out:	the MSB (or MSB/2) of the DAC data is connected to the
+ *			SIGN BIT OUT pin. en_div2 controls whether it is the MSB
+ *			or MSB/2 that is output. if en_signbit_msb_out=false,
+ *			the on-board comparator is connected to SIGN BIT OUT
+ */
+
+struct ad9834_platform_data {
+	unsigned int		mclk;
+	unsigned int		freq0;
+	unsigned int		freq1;
+	unsigned short		phase0;
+	unsigned short		phase1;
+	bool			en_div2;
+	bool			en_signbit_msb_out;
+};
+
+/**
+ * ad9834_supported_device_ids:
+ */
+
+enum ad9834_supported_device_ids {
+	ID_AD9833,
+	ID_AD9834,
+};
+
+#endif /* IIO_DDS_AD9834_H_ */
--
1.6.0.2

RE: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver

[Datta, Shubhrajyoti]

> -----Original Message-----
> From: linux-iio-owner@vger.kernel.org [mailto:linux-iio-
> owner@vger.kernel.org] On Behalf Of michael.hennerich@analog.com
> Sent: Thursday, December 02, 2010 5:51 PM
> To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: drivers@analog.com; jic23@cam.ac.uk; device-drivers-
> devel@blackfin.uclinux.org; Michael Hennerich
> Subject: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver
>
> From: Michael Hennerich <michael.hennerich@analog.com>
>
> Example driver using the proposed ABI
>
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>  drivers/staging/iio/dds/Kconfig  |    7 +
>  drivers/staging/iio/dds/Makefile |    1 +
>  drivers/staging/iio/dds/ad9834.c |  483
> ++++++++++++++++++++++++++++++++++++++
>  drivers/staging/iio/dds/ad9834.h |  112 +++++++++
>  4 files changed, 603 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/dds/ad9834.c
>  create mode 100644 drivers/staging/iio/dds/ad9834.h
>
> diff --git a/drivers/staging/iio/dds/Kconfig
> b/drivers/staging/iio/dds/Kconfig
> index 7969be2..4c9cce3 100644
> --- a/drivers/staging/iio/dds/Kconfig
> +++ b/drivers/staging/iio/dds/Kconfig
> @@ -17,6 +17,13 @@ config AD9832
>         Say yes here to build support for Analog Devices DDS chip
>         ad9832 and ad9835, provides direct access via sysfs.
>
> +config AD9834
> +     tristate "Analog Devices ad9833/4/ driver"
> +     depends on SPI
> +     help
> +       Say yes here to build support for Analog Devices DDS chip
> +       AD9833 and AD9834, provides direct access via sysfs.
> +
>  config AD9850
>       tristate "Analog Devices ad9850/1 driver"
>       depends on SPI
> diff --git a/drivers/staging/iio/dds/Makefile
> b/drivers/staging/iio/dds/Makefile
> index 6f274ac..1477461 100644
> --- a/drivers/staging/iio/dds/Makefile
> +++ b/drivers/staging/iio/dds/Makefile
> @@ -4,6 +4,7 @@
>
>  obj-$(CONFIG_AD5930) += ad5930.o
>  obj-$(CONFIG_AD9832) += ad9832.o
> +obj-$(CONFIG_AD9834) += ad9834.o
>  obj-$(CONFIG_AD9850) += ad9850.o
>  obj-$(CONFIG_AD9852) += ad9852.o
>  obj-$(CONFIG_AD9910) += ad9910.o
> diff --git a/drivers/staging/iio/dds/ad9834.c
> b/drivers/staging/iio/dds/ad9834.c
> new file mode 100644
> index 0000000..eb1fabf
> --- /dev/null
> +++ b/drivers/staging/iio/dds/ad9834.c
> @@ -0,0 +1,483 @@
> +/*
> + * AD9834 SPI DAC driver
> + *
> + * Copyright 2010 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/list.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <asm/div64.h>
> +
> +#include "../iio.h"
> +#include "../sysfs.h"
> +#include "dds.h"
> +
> +#include "ad9834.h"
> +
> +static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long
> fout)
> +{
> +     unsigned long long freqreg = (u64) fout * (u64) (1 <<
> AD9834_FREQ_BITS);
> +     do_div(freqreg, mclk);
> +     return freqreg;
> +}
> +
> +static int ad9834_write_frequency(struct ad9834_state *st,
> +                               unsigned long addr, unsigned long fout)
> +{
> +     unsigned long regval;
> +
> +     if (fout > (st->mclk / 2))
> +             return -EINVAL;
> +
> +     regval = ad9834_calc_freqreg(st->mclk, fout);
> +
> +     st->freq_data[0] = cpu_to_be16(addr | (regval &
> +                                    RES_MASK(AD9834_FREQ_BITS / 2)));
> +     st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> +                                    (AD9834_FREQ_BITS / 2)) &
> +                                    RES_MASK(AD9834_FREQ_BITS / 2)));
> +
> +     return spi_sync(st->spi, &st->freq_msg);;
> +}
> +
> +static int ad9834_write_phase(struct ad9834_state *st,
> +                               unsigned long addr, unsigned long phase)
> +{
> +     if (phase > (1 << AD9834_PHASE_BITS))
> +             return -EINVAL;
> +     st->data = cpu_to_be16(addr | phase);
> +
> +     return spi_sync(st->spi, &st->msg);
> +}
> +
> +static ssize_t ad9834_write(struct device *dev,
> +             struct device_attribute *attr,
> +             const char *buf,
> +             size_t len)
> +{
> +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> +     struct ad9834_state *st = dev_info->dev_data;
> +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +     int ret;
> +     long val;
> +
> +     ret = strict_strtol(buf, 10, &val);
> +     if (ret)
> +             goto error_ret;
Could we do some bounds check.
> +
> +     mutex_lock(&dev_info->mlock);
> +     switch (this_attr->address) {
> +     case AD9834_REG_FREQ0:
> +     case AD9834_REG_FREQ1:
> +             ret = ad9834_write_frequency(st, this_attr->address, val);
> +             break;
> +     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 */
> +                     break;
> +             }
> +
> +             if (val)
> +                     st->control &= ~AD9834_OPBITEN;
> +             else
> +                     st->control |= AD9834_OPBITEN;
> +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> +             ret = spi_sync(st->spi, &st->msg);
> +             break;
> +
> +     case AD9834_PIN_SW:
> +             if (val)
> +                     st->control |= AD9834_PIN_SW;
> +             else
> +                     st->control &= ~AD9834_PIN_SW;
> +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> +             ret = spi_sync(st->spi, &st->msg);
> +             break;
> +     case AD9834_FSEL:
> +     case AD9834_PSEL:
> +             if (val == 0)
> +                     st->control &= ~(this_attr->address | AD9834_PIN_SW);
> +             else if (val == 1) {
> +                     st->control |= this_attr->address;
> +                     st->control &= ~AD9834_PIN_SW;
> +             } else {
> +                     ret = -EINVAL;
> +                     break;
> +             }
> +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> +             ret = spi_sync(st->spi, &st->msg);
> +             break;
> +     case AD9834_RESET:
> +             if (val)
> +                     st->control |= AD9834_RESET;
> +             else
> +                     st->control &= ~AD9834_RESET;
> +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> +             ret = spi_sync(st->spi, &st->msg);
> +             break;
> +     default:
> +             ret = -ENODEV;
> +     }
> +     mutex_unlock(&dev_info->mlock);
> +
> +error_ret:
> +     return ret ? ret : len;
> +}
> +
> +static ssize_t ad9834_store_wavetype(struct device *dev,
> +                              struct device_attribute *attr,
> +                              const char *buf,
> +                              size_t len)
> +{
> +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> +     struct ad9834_state *st = dev_info->dev_data;
> +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +     int ret = 0;
> +     bool is_ad9833 = st->devid == ID_AD9833;
> +
> +     mutex_lock(&dev_info->mlock);
> +
> +     switch (this_attr->address) {
> +     case 0:
> +             if (sysfs_streq(buf, "sine")) {
> +                     st->control &= ~AD9834_MODE;
> +                     if (is_ad9833)
> +                             st->control &= ~AD9834_OPBITEN;
> +             } else if (sysfs_streq(buf, "triangle")) {
> +                     if (is_ad9833) {
> +                             st->control &= ~AD9834_OPBITEN;
> +                             st->control |= AD9834_MODE;
> +                     } else if (st->control & AD9834_OPBITEN) {
> +                             ret = -EINVAL;  /* AD9843 reserved mode */
> +                     } else {
> +                             st->control |= AD9834_MODE;
> +                     }
> +             } else if (is_ad9833 && sysfs_streq(buf, "square")) {
> +                     st->control &= ~AD9834_MODE;
> +                     st->control |= AD9834_OPBITEN;
> +             } else {
> +                     ret = -EINVAL;
> +             }
> +
> +             break;
> +     case 1:
> +             if (sysfs_streq(buf, "square") &&
> +                     !(st->control & AD9834_MODE)) {
> +                     st->control &= ~AD9834_MODE;
> +                     st->control |= AD9834_OPBITEN;
> +             } else {
> +                     ret = -EINVAL;
> +             }
> +             break;
> +     default:
> +             ret = -EINVAL;
> +             break;
> +     }
> +
> +     if (!ret) {
> +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> +             ret = spi_sync(st->spi, &st->msg);
> +     }
> +             mutex_unlock(&dev_info->mlock);
> +
> +     return ret ? ret : len;
> +}
> +
> +static ssize_t ad9834_show_name(struct device *dev,
> +                              struct device_attribute *attr,
> +                              char *buf)
> +{
> +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> +     struct ad9834_state *st = iio_dev_get_devdata(dev_info);
> +
> +     return sprintf(buf, "%s\n", spi_get_device_id(st->spi)->name);
> +}
> +static IIO_DEVICE_ATTR(name, S_IRUGO, ad9834_show_name, NULL, 0);
> +
> +static ssize_t ad9834_show_out0_available_wavetypes(struct device *dev,
> +                                             struct device_attribute *attr,
> +                                             char *buf)
> +{
> +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> +     struct ad9834_state *st = iio_dev_get_devdata(dev_info);
> +     char *str;
> +
> +     if (st->devid == ID_AD9833)
> +             str = "sine triangle square";
> +     else if (st->control & AD9834_OPBITEN)
> +             str = "sine";
> +     else
> +             str = "sine triangle";
> +
> +     return sprintf(buf, "%s\n", str);
> +}
> +
> +
> +static IIO_DEVICE_ATTR(dds0_out0_available_wavetypes, S_IRUGO,
> +                    ad9834_show_out0_available_wavetypes, NULL, 0);
> +
> +static ssize_t ad9834_show_out1_available_wavetypes(struct device *dev,
> +                                             struct device_attribute *attr,
> +                                             char *buf)
> +{
> +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> +     struct ad9834_state *st = iio_dev_get_devdata(dev_info);
> +     char *str;
> +
> +     if (st->control & AD9834_MODE)
> +             str = "";
> +     else
> +             str = "square";
> +
> +     return sprintf(buf, "%s\n", str);
> +}
> +
> +static IIO_DEVICE_ATTR(dds0_out1_available_wavetypes, S_IRUGO,
> +                    ad9834_show_out1_available_wavetypes, NULL, 0);
> +
> +/**
> + * see dds.h for further information
> + */
> +
> +
> +static IIO_DEV_ATTR_FREQ(0, 0, ad9834_write, AD9834_REG_FREQ0);
> +static IIO_DEV_ATTR_FREQ(0, 1, ad9834_write, AD9834_REG_FREQ1);
> +static IIO_DEV_ATTR_FREQSYMBOL(0, ad9834_write, AD9834_FSEL);
> +static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
> +
> +static IIO_DEV_ATTR_PHASE(0, 0, ad9834_write, AD9834_REG_PHASE0);
> +static IIO_DEV_ATTR_PHASE(0, 1, ad9834_write, AD9834_REG_PHASE1);
> +static IIO_DEV_ATTR_PHASESYMBOL(0, ad9834_write, AD9834_PSEL);
> +static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
> +
> +static IIO_DEV_ATTR_PINCONTROL_EN(0, ad9834_write, AD9834_PIN_SW);
> +static IIO_DEV_ATTR_OUT_DISABLE(0, ad9834_write, AD9834_RESET);
> +static IIO_DEV_ATTR_OUTY_DISABLE(0, 1, ad9834_write, AD9834_OPBITEN);
> +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_dev_attr_dds0_freq0.dev_attr.attr,
> +     &iio_dev_attr_dds0_freq1.dev_attr.attr,
> +     &iio_const_attr_dds0_freq_scale.dev_attr.attr,
> +     &iio_dev_attr_dds0_phase0.dev_attr.attr,
> +     &iio_dev_attr_dds0_phase1.dev_attr.attr,
> +     &iio_const_attr_dds0_phase_scale.dev_attr.attr,
> +     &iio_dev_attr_dds0_pincontrol_en.dev_attr.attr,
> +     &iio_dev_attr_dds0_freqsymbol.dev_attr.attr,
> +     &iio_dev_attr_dds0_phasesymbol.dev_attr.attr,
> +     &iio_dev_attr_dds0_out_disable.dev_attr.attr,
> +     &iio_dev_attr_dds0_out1_disable.dev_attr.attr,
> +     &iio_dev_attr_dds0_out0_wavetype.dev_attr.attr,
> +     &iio_dev_attr_dds0_out1_wavetype.dev_attr.attr,
> +     &iio_dev_attr_dds0_out0_available_wavetypes.dev_attr.attr,
> +     &iio_dev_attr_dds0_out1_available_wavetypes.dev_attr.attr,
> +     &iio_dev_attr_name.dev_attr.attr,
> +     NULL,
> +};
> +
> +static mode_t ad9834_attr_is_visible(struct kobject *kobj,
> +                                  struct attribute *attr, int n)
> +{
> +     struct device *dev = container_of(kobj, struct device, kobj);
> +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> +     struct ad9834_state *st = iio_dev_get_devdata(dev_info);
> +
> +     mode_t mode = attr->mode;
> +
> +     if (st->devid == ID_AD9834)
> +             return mode;
> +
> +     if ((attr == &iio_dev_attr_dds0_out1_disable.dev_attr.attr) ||
> +             (attr == &iio_dev_attr_dds0_out1_wavetype.dev_attr.attr) ||
> +             (attr ==
> +             &iio_dev_attr_dds0_out1_available_wavetypes.dev_attr.attr))
> +             mode = 0;
> +
> +     return mode;
> +}
> +
> +static const struct attribute_group ad9834_attribute_group = {
> +     .attrs = ad9834_attributes,
> +     .is_visible = ad9834_attr_is_visible,
> +};
> +
> +static int __devinit ad9834_probe(struct spi_device *spi)
> +{
> +     struct ad9834_platform_data *pdata = spi->dev.platform_data;
> +     struct ad9834_state *st;
> +     int ret;
> +
> +     if (!pdata) {
> +             dev_dbg(&spi->dev, "no platform data?\n");
> +             return -ENODEV;
> +     }
> +
> +     st = kzalloc(sizeof(*st), GFP_KERNEL);
> +     if (st == NULL) {
> +             ret = -ENOMEM;
> +             goto error_ret;
> +     }
> +
> +     st->reg = regulator_get(&spi->dev, "vcc");
> +     if (!IS_ERR(st->reg)) {
> +             ret = regulator_enable(st->reg);
> +             if (ret)
> +                     goto error_put_reg;
> +     }
> +
> +     st->mclk = pdata->mclk;
> +
> +     spi_set_drvdata(spi, st);
> +
> +     st->spi = spi;
> +     st->devid = spi_get_device_id(spi)->driver_data;
> +
> +     st->indio_dev = iio_allocate_device();
> +     if (st->indio_dev == NULL) {
> +             ret = -ENOMEM;
> +             goto error_disable_reg;
> +     }
> +
> +     st->indio_dev->dev.parent = &spi->dev;
> +     st->indio_dev->attrs = &ad9834_attribute_group;
> +     st->indio_dev->dev_data = (void *)(st);
> +     st->indio_dev->driver_module = THIS_MODULE;
> +     st->indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +     /* Setup default messages */
> +
> +     st->xfer.tx_buf = &st->data;
> +     st->xfer.len = 2;
> +
> +     spi_message_init(&st->msg);
> +     spi_message_add_tail(&st->xfer, &st->msg);
> +
> +     st->freq_xfer[0].tx_buf = &st->freq_data[0];
> +     st->freq_xfer[0].len = 2;
> +     st->freq_xfer[0].cs_change = 1;
> +     st->freq_xfer[1].tx_buf = &st->freq_data[1];
> +     st->freq_xfer[1].len = 2;
> +
> +     spi_message_init(&st->freq_msg);
> +     spi_message_add_tail(&st->freq_xfer[0], &st->freq_msg);
> +     spi_message_add_tail(&st->freq_xfer[1], &st->freq_msg);
> +
> +     st->control = AD9834_B28 | AD9834_RESET;
> +
> +     if (!pdata->en_div2)
> +             st->control |= AD9834_DIV2;
> +
> +     if (!pdata->en_signbit_msb_out && (st->devid == ID_AD9834))
> +             st->control |= AD9834_SIGN_PIB;
> +
> +     st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> +     ret = spi_sync(st->spi, &st->msg);
> +     if (ret) {
> +             dev_err(&spi->dev, "device init failed\n");
> +             goto error_free_device;
> +     }
> +
> +     ret = ad9834_write_frequency(st, AD9834_REG_FREQ0, pdata->freq0);
> +     if (ret)
> +             goto error_free_device;
> +
> +     ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, pdata->freq1);
> +     if (ret)
> +             goto error_free_device;
> +
> +     ret = ad9834_write_phase(st, AD9834_REG_PHASE0, pdata->phase0);
> +     if (ret)
> +             goto error_free_device;
> +
> +     ret = ad9834_write_phase(st, AD9834_REG_PHASE1, pdata->phase1);
> +     if (ret)
> +             goto error_free_device;
> +
> +     st->control &= ~AD9834_RESET;
> +     st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> +     ret = spi_sync(st->spi, &st->msg);
> +     if (ret)
> +             goto error_free_device;
> +
> +     ret = iio_device_register(st->indio_dev);
> +     if (ret)
> +             goto error_free_device;
> +
> +     return 0;
> +
> +error_free_device:
> +     iio_free_device(st->indio_dev);
> +error_disable_reg:
> +     if (!IS_ERR(st->reg))
> +             regulator_disable(st->reg);
> +error_put_reg:
> +     if (!IS_ERR(st->reg))
> +             regulator_put(st->reg);
> +     kfree(st);
> +error_ret:
> +     return ret;
> +}
> +
> +static int ad9834_remove(struct spi_device *spi)
Could we consider __devexit here
> +{
> +     struct ad9834_state *st = spi_get_drvdata(spi);
> +     struct iio_dev *indio_dev = st->indio_dev;
> +
> +     iio_device_unregister(indio_dev);
> +     if (!IS_ERR(st->reg)) {
> +             regulator_disable(st->reg);
> +             regulator_put(st->reg);
> +     }
> +     kfree(st);
> +     return 0;
> +}
> +
> +static const struct spi_device_id ad9834_id[] = {
> +     {"ad9833", ID_AD9833},
> +     {"ad9834", ID_AD9834},
> +     {}
> +};
> +
> +static struct spi_driver ad9834_driver = {
> +     .driver = {
> +             .name   = "ad9834",
> +             .bus    = &spi_bus_type,
> +             .owner  = THIS_MODULE,
> +     },
> +     .probe          = ad9834_probe,
> +     .remove         = __devexit_p(ad9834_remove),
> +     .id_table       = ad9834_id,
> +};
> +
> +static int __init ad9834_init(void)
> +{
> +     return spi_register_driver(&ad9834_driver);
> +}
> +module_init(ad9834_init);
> +
> +static void __exit ad9834_exit(void)
> +{
> +     spi_unregister_driver(&ad9834_driver);
> +}
> +module_exit(ad9834_exit);
> +
> +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> +MODULE_DESCRIPTION("Analog Devices AD9833/AD9834 DDS");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:ad9834");
> diff --git a/drivers/staging/iio/dds/ad9834.h
> b/drivers/staging/iio/dds/ad9834.h
> new file mode 100644
> index 0000000..0fc3b88
> --- /dev/null
> +++ b/drivers/staging/iio/dds/ad9834.h
> @@ -0,0 +1,112 @@
> +/*
> + * AD9834 SPI DDS driver
> + *
> + * Copyright 2010 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +#ifndef IIO_DDS_AD9834_H_
> +#define IIO_DDS_AD9834_H_
> +
> +/* Registers */
> +
> +#define AD9834_REG_CMD               (0 << 14)
> +#define AD9834_REG_FREQ0     (1 << 14)
> +#define AD9834_REG_FREQ1     (2 << 14)
> +#define AD9834_REG_PHASE0    (6 << 13)
> +#define AD9834_REG_PHASE1    (7 << 13)
> +
> +/* Command Control Bits */
> +
> +#define AD9834_B28           (1 << 13)
> +#define AD9834_HLB           (1 << 12)
> +#define AD9834_FSEL          (1 << 11)
> +#define AD9834_PSEL          (1 << 10)
> +#define AD9834_PIN_SW                (1 << 9)
> +#define AD9834_RESET         (1 << 8)
> +#define AD9834_SLEEP1                (1 << 7)
> +#define AD9834_SLEEP12               (1 << 6)
> +#define AD9834_OPBITEN               (1 << 5)
> +#define AD9834_SIGN_PIB              (1 << 4)
> +#define AD9834_DIV2          (1 << 3)
> +#define AD9834_MODE          (1 << 1)
> +
> +#define AD9834_FREQ_BITS     28
> +#define AD9834_PHASE_BITS    12
> +
> +#define RES_MASK(bits)       ((1 << (bits)) - 1)
> +
> +/**
> + * struct ad9834_state - driver instance specific data
> + * @indio_dev:               the industrial I/O device
> + * @spi:             spi_device
> + * @reg:             supply regulator
> + * @mclk:            external master clock
> + * @control:         cached control word
> + * @xfer:            default spi transfer
> + * @msg:             default spi message
> + * @freq_xfer:               tuning word spi transfer
> + * @freq_msg:                tuning word spi message
> + * @data:            spi transmit buffer
> + * @freq_data:               tuning word spi transmit buffer
> + */
> +
> +struct ad9834_state {
> +     struct iio_dev                  *indio_dev;
> +     struct spi_device               *spi;
> +     struct regulator                *reg;
> +     unsigned int                    mclk;
> +     unsigned short                  control;
> +     unsigned short                  devid;
> +     struct spi_transfer             xfer;
> +     struct spi_message              msg;
> +     struct spi_transfer             freq_xfer[2];
> +     struct spi_message              freq_msg;
> +
> +     /*
> +      * DMA (thus cache coherency maintenance) requires the
> +      * transfer buffers to live in their own cache lines.
> +      */
> +     unsigned short                  data ____cacheline_aligned;
> +     unsigned short                  freq_data[2] ;
> +};
> +
> +
> +/*
> + * TODO: struct ad7887_platform_data needs to go into include/linux/iio
> + */
> +
> +/**
> + * struct ad9834_platform_data - platform specific information
> + * @mclk:            master clock in Hz
> + * @freq0:           power up freq0 tuning word in Hz
> + * @freq1:           power up freq1 tuning word in Hz
> + * @phase0:          power up phase0 value [0..4095] correlates with
> 0..2PI
> + * @phase1:          power up phase1 value [0..4095] correlates with
> 0..2PI
> + * @en_div2:         digital output/2 is passed to the SIGN BIT OUT pin
> + * @en_signbit_msb_out:      the MSB (or MSB/2) of the DAC data is
> connected to the
> + *                   SIGN BIT OUT pin. en_div2 controls whether it is the MSB
> + *                   or MSB/2 that is output. if en_signbit_msb_out=false,
> + *                   the on-board comparator is connected to SIGN BIT OUT
> + */
> +
> +struct ad9834_platform_data {
> +     unsigned int            mclk;
> +     unsigned int            freq0;
> +     unsigned int            freq1;
> +     unsigned short          phase0;
> +     unsigned short          phase1;
> +     bool                    en_div2;
> +     bool                    en_signbit_msb_out;
> +};
> +
> +/**
> + * ad9834_supported_device_ids:
> + */
> +
> +enum ad9834_supported_device_ids {
> +     ID_AD9833,
> +     ID_AD9834,
> +};
> +
> +#endif /* IIO_DDS_AD9834_H_ */
> --
> 1.6.0.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver

[Hennerich, Michael]

> Datta, Shubhrajyoti wrote on 2010-12-02:
> > -----Original Message-----
> > From: linux-iio-owner@vger.kernel.org [mailto:linux-iio-
> > owner@vger.kernel.org] On Behalf Of michael.hennerich@analog.com
> > Sent: Thursday, December 02, 2010 5:51 PM
> > To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > Cc: drivers@analog.com; jic23@cam.ac.uk; device-drivers-
> > devel@blackfin.uclinux.org; Michael Hennerich
> > Subject: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver
> >
> > From: Michael Hennerich <michael.hennerich@analog.com>
> >
> > Example driver using the proposed ABI
> >
> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > ---
> >  drivers/staging/iio/dds/Kconfig  |    7 +
> >  drivers/staging/iio/dds/Makefile |    1 +
> >  drivers/staging/iio/dds/ad9834.c |  483
> > ++++++++++++++++++++++++++++++++++++++
> >  drivers/staging/iio/dds/ad9834.h |  112 +++++++++
> >  4 files changed, 603 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/staging/iio/dds/ad9834.c
> >  create mode 100644 drivers/staging/iio/dds/ad9834.h
> >
> > diff --git a/drivers/staging/iio/dds/Kconfig
> > b/drivers/staging/iio/dds/Kconfig
> > index 7969be2..4c9cce3 100644
> > --- a/drivers/staging/iio/dds/Kconfig
> > +++ b/drivers/staging/iio/dds/Kconfig
> > @@ -17,6 +17,13 @@ config AD9832
> >         Say yes here to build support for Analog Devices DDS chip
> >         ad9832 and ad9835, provides direct access via sysfs.
> >
> > +config AD9834
> > +     tristate "Analog Devices ad9833/4/ driver"
> > +     depends on SPI
> > +     help
> > +       Say yes here to build support for Analog Devices DDS chip
> > +       AD9833 and AD9834, provides direct access via sysfs.
> > +
> >  config AD9850
> >       tristate "Analog Devices ad9850/1 driver"
> >       depends on SPI
> > diff --git a/drivers/staging/iio/dds/Makefile
> > b/drivers/staging/iio/dds/Makefile
> > index 6f274ac..1477461 100644
> > --- a/drivers/staging/iio/dds/Makefile
> > +++ b/drivers/staging/iio/dds/Makefile
> > @@ -4,6 +4,7 @@
> >
> >  obj-$(CONFIG_AD5930) += ad5930.o
> >  obj-$(CONFIG_AD9832) += ad9832.o
> > +obj-$(CONFIG_AD9834) += ad9834.o
> >  obj-$(CONFIG_AD9850) += ad9850.o
> >  obj-$(CONFIG_AD9852) += ad9852.o
> >  obj-$(CONFIG_AD9910) += ad9910.o
> > diff --git a/drivers/staging/iio/dds/ad9834.c
> > b/drivers/staging/iio/dds/ad9834.c
> > new file mode 100644
> > index 0000000..eb1fabf
> > --- /dev/null
> > +++ b/drivers/staging/iio/dds/ad9834.c
> > @@ -0,0 +1,483 @@
> > +/*
> > + * AD9834 SPI DAC driver
> > + *
> > + * Copyright 2010 Analog Devices Inc.
> > + *
> > + * Licensed under the GPL-2 or later.
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/list.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/err.h>
> > +#include <asm/div64.h>
> > +
> > +#include "../iio.h"
> > +#include "../sysfs.h"
> > +#include "dds.h"
> > +
> > +#include "ad9834.h"
> > +
> > +static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned
> long
> > fout)
> > +{
> > +     unsigned long long freqreg = (u64) fout * (u64) (1 <<
> > AD9834_FREQ_BITS);
> > +     do_div(freqreg, mclk);
> > +     return freqreg;
> > +}
> > +
> > +static int ad9834_write_frequency(struct ad9834_state *st,
> > +                               unsigned long addr, unsigned long
> fout)
> > +{
> > +     unsigned long regval;
> > +
> > +     if (fout > (st->mclk / 2))
> > +             return -EINVAL;
> > +
> > +     regval = ad9834_calc_freqreg(st->mclk, fout);
> > +
> > +     st->freq_data[0] = cpu_to_be16(addr | (regval &
> > +                                    RES_MASK(AD9834_FREQ_BITS /
> 2)));
> > +     st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> > +                                    (AD9834_FREQ_BITS / 2)) &
> > +                                    RES_MASK(AD9834_FREQ_BITS /
> 2)));
> > +
> > +     return spi_sync(st->spi, &st->freq_msg);;
> > +}
> > +
> > +static int ad9834_write_phase(struct ad9834_state *st,
> > +                               unsigned long addr, unsigned long
> phase)
> > +{
> > +     if (phase > (1 << AD9834_PHASE_BITS))
> > +             return -EINVAL;
> > +     st->data = cpu_to_be16(addr | phase);
> > +
> > +     return spi_sync(st->spi, &st->msg);
> > +}
> > +
> > +static ssize_t ad9834_write(struct device *dev,
> > +             struct device_attribute *attr,
> > +             const char *buf,
> > +             size_t len)
> > +{
> > +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> > +     struct ad9834_state *st = dev_info->dev_data;
> > +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > +     int ret;
> > +     long val;
> > +
> > +     ret = strict_strtol(buf, 10, &val);
> > +     if (ret)
> > +             goto error_ret;
> Could we do some bounds check.
The functions called with val as argument from the switch cases below, do
some bound checking, where necessary.
It's questionable whether these simple enable switch cases, should return with
error if someone writes something other than 0 or 1.
> > +
> > +     mutex_lock(&dev_info->mlock);
> > +     switch (this_attr->address) {
> > +     case AD9834_REG_FREQ0:
> > +     case AD9834_REG_FREQ1:
> > +             ret = ad9834_write_frequency(st, this_attr->address,
> val);
> > +             break;
> > +     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 */
> > +                     break;
> > +             }
> > +
> > +             if (val)
> > +                     st->control &= ~AD9834_OPBITEN;
> > +             else
> > +                     st->control |= AD9834_OPBITEN;
> > +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> > +             ret = spi_sync(st->spi, &st->msg);
> > +             break;
> > +
> > +     case AD9834_PIN_SW:
> > +             if (val)
> > +                     st->control |= AD9834_PIN_SW;
> > +             else
> > +                     st->control &= ~AD9834_PIN_SW;
> > +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> > +             ret = spi_sync(st->spi, &st->msg);
> > +             break;
> > +     case AD9834_FSEL:
> > +     case AD9834_PSEL:
> > +             if (val == 0)
> > +                     st->control &= ~(this_attr->address |
> AD9834_PIN_SW);
> > +             else if (val == 1) {
> > +                     st->control |= this_attr->address;
> > +                     st->control &= ~AD9834_PIN_SW;
> > +             } else {
> > +                     ret = -EINVAL;
> > +                     break;
> > +             }
> > +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> > +             ret = spi_sync(st->spi, &st->msg);
> > +             break;
> > +     case AD9834_RESET:
> > +             if (val)
> > +                     st->control |= AD9834_RESET;
> > +             else
> > +                     st->control &= ~AD9834_RESET;
> > +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> > +             ret = spi_sync(st->spi, &st->msg);
> > +             break;
> > +     default:
> > +             ret = -ENODEV;
> > +     }
> > +     mutex_unlock(&dev_info->mlock);
> > +
> > +error_ret:
> > +     return ret ? ret : len;
> > +}
> > +
> > +static ssize_t ad9834_store_wavetype(struct device *dev,
> > +                              struct device_attribute *attr,
> > +                              const char *buf,
> > +                              size_t len)
> > +{
> > +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> > +     struct ad9834_state *st = dev_info->dev_data;
> > +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > +     int ret = 0;
> > +     bool is_ad9833 = st->devid == ID_AD9833;
> > +
> > +     mutex_lock(&dev_info->mlock);
> > +
> > +     switch (this_attr->address) {
> > +     case 0:
> > +             if (sysfs_streq(buf, "sine")) {
> > +                     st->control &= ~AD9834_MODE;
> > +                     if (is_ad9833)
> > +                             st->control &= ~AD9834_OPBITEN;
> > +             } else if (sysfs_streq(buf, "triangle")) {
> > +                     if (is_ad9833) {
> > +                             st->control &= ~AD9834_OPBITEN;
> > +                             st->control |= AD9834_MODE;
> > +                     } else if (st->control & AD9834_OPBITEN) {
> > +                             ret = -EINVAL;  /* AD9843 reserved mode
> */
> > +                     } else {
> > +                             st->control |= AD9834_MODE;
> > +                     }
> > +             } else if (is_ad9833 && sysfs_streq(buf, "square")) {
> > +                     st->control &= ~AD9834_MODE;
> > +                     st->control |= AD9834_OPBITEN;
> > +             } else {
> > +                     ret = -EINVAL;
> > +             }
> > +
> > +             break;
> > +     case 1:
> > +             if (sysfs_streq(buf, "square") &&
> > +                     !(st->control & AD9834_MODE)) {
> > +                     st->control &= ~AD9834_MODE;
> > +                     st->control |= AD9834_OPBITEN;
> > +             } else {
> > +                     ret = -EINVAL;
> > +             }
> > +             break;
> > +     default:
> > +             ret = -EINVAL;
> > +             break;
> > +     }
> > +
> > +     if (!ret) {
> > +             st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> > +             ret = spi_sync(st->spi, &st->msg);
> > +     }
> > +             mutex_unlock(&dev_info->mlock);
> > +
> > +     return ret ? ret : len;
> > +}
> > +
> > +static ssize_t ad9834_show_name(struct device *dev,
> > +                              struct device_attribute *attr,
> > +                              char *buf)
> > +{
> > +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> > +     struct ad9834_state *st = iio_dev_get_devdata(dev_info);
> > +
> > +     return sprintf(buf, "%s\n", spi_get_device_id(st->spi)->name);
> > +}
> > +static IIO_DEVICE_ATTR(name, S_IRUGO, ad9834_show_name, NULL, 0);
> > +
> > +static ssize_t ad9834_show_out0_available_wavetypes(struct device
> *dev,
> > +                                             struct device_attribute
> *attr,
> > +                                             char *buf)
> > +{
> > +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> > +     struct ad9834_state *st = iio_dev_get_devdata(dev_info);
> > +     char *str;
> > +
> > +     if (st->devid == ID_AD9833)
> > +             str = "sine triangle square";
> > +     else if (st->control & AD9834_OPBITEN)
> > +             str = "sine";
> > +     else
> > +             str = "sine triangle";
> > +
> > +     return sprintf(buf, "%s\n", str);
> > +}
> > +
> > +
> > +static IIO_DEVICE_ATTR(dds0_out0_available_wavetypes, S_IRUGO,
> > +                    ad9834_show_out0_available_wavetypes, NULL, 0);
> > +
> > +static ssize_t ad9834_show_out1_available_wavetypes(struct device
> *dev,
> > +                                             struct device_attribute
> *attr,
> > +                                             char *buf)
> > +{
> > +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> > +     struct ad9834_state *st = iio_dev_get_devdata(dev_info);
> > +     char *str;
> > +
> > +     if (st->control & AD9834_MODE)
> > +             str = "";
> > +     else
> > +             str = "square";
> > +
> > +     return sprintf(buf, "%s\n", str);
> > +}
> > +
> > +static IIO_DEVICE_ATTR(dds0_out1_available_wavetypes, S_IRUGO,
> > +                    ad9834_show_out1_available_wavetypes, NULL, 0);
> > +
> > +/**
> > + * see dds.h for further information
> > + */
> > +
> > +
> > +static IIO_DEV_ATTR_FREQ(0, 0, ad9834_write, AD9834_REG_FREQ0);
> > +static IIO_DEV_ATTR_FREQ(0, 1, ad9834_write, AD9834_REG_FREQ1);
> > +static IIO_DEV_ATTR_FREQSYMBOL(0, ad9834_write, AD9834_FSEL);
> > +static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
> > +
> > +static IIO_DEV_ATTR_PHASE(0, 0, ad9834_write, AD9834_REG_PHASE0);
> > +static IIO_DEV_ATTR_PHASE(0, 1, ad9834_write, AD9834_REG_PHASE1);
> > +static IIO_DEV_ATTR_PHASESYMBOL(0, ad9834_write, AD9834_PSEL);
> > +static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12
> rad*/
> > +
> > +static IIO_DEV_ATTR_PINCONTROL_EN(0, ad9834_write, AD9834_PIN_SW);
> > +static IIO_DEV_ATTR_OUT_DISABLE(0, ad9834_write, AD9834_RESET);
> > +static IIO_DEV_ATTR_OUTY_DISABLE(0, 1, ad9834_write,
> AD9834_OPBITEN);
> > +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_dev_attr_dds0_freq0.dev_attr.attr,
> > +     &iio_dev_attr_dds0_freq1.dev_attr.attr,
> > +     &iio_const_attr_dds0_freq_scale.dev_attr.attr,
> > +     &iio_dev_attr_dds0_phase0.dev_attr.attr,
> > +     &iio_dev_attr_dds0_phase1.dev_attr.attr,
> > +     &iio_const_attr_dds0_phase_scale.dev_attr.attr,
> > +     &iio_dev_attr_dds0_pincontrol_en.dev_attr.attr,
> > +     &iio_dev_attr_dds0_freqsymbol.dev_attr.attr,
> > +     &iio_dev_attr_dds0_phasesymbol.dev_attr.attr,
> > +     &iio_dev_attr_dds0_out_disable.dev_attr.attr,
> > +     &iio_dev_attr_dds0_out1_disable.dev_attr.attr,
> > +     &iio_dev_attr_dds0_out0_wavetype.dev_attr.attr,
> > +     &iio_dev_attr_dds0_out1_wavetype.dev_attr.attr,
> > +     &iio_dev_attr_dds0_out0_available_wavetypes.dev_attr.attr,
> > +     &iio_dev_attr_dds0_out1_available_wavetypes.dev_attr.attr,
> > +     &iio_dev_attr_name.dev_attr.attr,
> > +     NULL,
> > +};
> > +
> > +static mode_t ad9834_attr_is_visible(struct kobject *kobj,
> > +                                  struct attribute *attr, int n)
> > +{
> > +     struct device *dev = container_of(kobj, struct device, kobj);
> > +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> > +     struct ad9834_state *st = iio_dev_get_devdata(dev_info);
> > +
> > +     mode_t mode = attr->mode;
> > +
> > +     if (st->devid == ID_AD9834)
> > +             return mode;
> > +
> > +     if ((attr == &iio_dev_attr_dds0_out1_disable.dev_attr.attr) ||
> > +             (attr ==
> &iio_dev_attr_dds0_out1_wavetype.dev_attr.attr) ||
> > +             (attr ==
> > +
> &iio_dev_attr_dds0_out1_available_wavetypes.dev_attr.attr))
> > +             mode = 0;
> > +
> > +     return mode;
> > +}
> > +
> > +static const struct attribute_group ad9834_attribute_group = {
> > +     .attrs = ad9834_attributes,
> > +     .is_visible = ad9834_attr_is_visible,
> > +};
> > +
> > +static int __devinit ad9834_probe(struct spi_device *spi)
> > +{
> > +     struct ad9834_platform_data *pdata = spi->dev.platform_data;
> > +     struct ad9834_state *st;
> > +     int ret;
> > +
> > +     if (!pdata) {
> > +             dev_dbg(&spi->dev, "no platform data?\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     st = kzalloc(sizeof(*st), GFP_KERNEL);
> > +     if (st == NULL) {
> > +             ret = -ENOMEM;
> > +             goto error_ret;
> > +     }
> > +
> > +     st->reg = regulator_get(&spi->dev, "vcc");
> > +     if (!IS_ERR(st->reg)) {
> > +             ret = regulator_enable(st->reg);
> > +             if (ret)
> > +                     goto error_put_reg;
> > +     }
> > +
> > +     st->mclk = pdata->mclk;
> > +
> > +     spi_set_drvdata(spi, st);
> > +
> > +     st->spi = spi;
> > +     st->devid = spi_get_device_id(spi)->driver_data;
> > +
> > +     st->indio_dev = iio_allocate_device();
> > +     if (st->indio_dev == NULL) {
> > +             ret = -ENOMEM;
> > +             goto error_disable_reg;
> > +     }
> > +
> > +     st->indio_dev->dev.parent = &spi->dev;
> > +     st->indio_dev->attrs = &ad9834_attribute_group;
> > +     st->indio_dev->dev_data = (void *)(st);
> > +     st->indio_dev->driver_module = THIS_MODULE;
> > +     st->indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +     /* Setup default messages */
> > +
> > +     st->xfer.tx_buf = &st->data;
> > +     st->xfer.len = 2;
> > +
> > +     spi_message_init(&st->msg);
> > +     spi_message_add_tail(&st->xfer, &st->msg);
> > +
> > +     st->freq_xfer[0].tx_buf = &st->freq_data[0];
> > +     st->freq_xfer[0].len = 2;
> > +     st->freq_xfer[0].cs_change = 1;
> > +     st->freq_xfer[1].tx_buf = &st->freq_data[1];
> > +     st->freq_xfer[1].len = 2;
> > +
> > +     spi_message_init(&st->freq_msg);
> > +     spi_message_add_tail(&st->freq_xfer[0], &st->freq_msg);
> > +     spi_message_add_tail(&st->freq_xfer[1], &st->freq_msg);
> > +
> > +     st->control = AD9834_B28 | AD9834_RESET;
> > +
> > +     if (!pdata->en_div2)
> > +             st->control |= AD9834_DIV2;
> > +
> > +     if (!pdata->en_signbit_msb_out && (st->devid == ID_AD9834))
> > +             st->control |= AD9834_SIGN_PIB;
> > +
> > +     st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> > +     ret = spi_sync(st->spi, &st->msg);
> > +     if (ret) {
> > +             dev_err(&spi->dev, "device init failed\n");
> > +             goto error_free_device;
> > +     }
> > +
> > +     ret = ad9834_write_frequency(st, AD9834_REG_FREQ0, pdata-
> >freq0);
> > +     if (ret)
> > +             goto error_free_device;
> > +
> > +     ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, pdata-
> >freq1);
> > +     if (ret)
> > +             goto error_free_device;
> > +
> > +     ret = ad9834_write_phase(st, AD9834_REG_PHASE0, pdata->phase0);
> > +     if (ret)
> > +             goto error_free_device;
> > +
> > +     ret = ad9834_write_phase(st, AD9834_REG_PHASE1, pdata->phase1);
> > +     if (ret)
> > +             goto error_free_device;
> > +
> > +     st->control &= ~AD9834_RESET;
> > +     st->data = cpu_to_be16(AD9834_REG_CMD | st->control);
> > +     ret = spi_sync(st->spi, &st->msg);
> > +     if (ret)
> > +             goto error_free_device;
> > +
> > +     ret = iio_device_register(st->indio_dev);
> > +     if (ret)
> > +             goto error_free_device;
> > +
> > +     return 0;
> > +
> > +error_free_device:
> > +     iio_free_device(st->indio_dev);
> > +error_disable_reg:
> > +     if (!IS_ERR(st->reg))
> > +             regulator_disable(st->reg);
> > +error_put_reg:
> > +     if (!IS_ERR(st->reg))
> > +             regulator_put(st->reg);
> > +     kfree(st);
> > +error_ret:
> > +     return ret;
> > +}
> > +
> > +static int ad9834_remove(struct spi_device *spi)
> Could we consider __devexit here
Good catch.
> > +{
> > +     struct ad9834_state *st = spi_get_drvdata(spi);
> > +     struct iio_dev *indio_dev = st->indio_dev;
> > +
> > +     iio_device_unregister(indio_dev);
> > +     if (!IS_ERR(st->reg)) {
> > +             regulator_disable(st->reg);
> > +             regulator_put(st->reg);
> > +     }
> > +     kfree(st);
> > +     return 0;
> > +}
> > +
> > +static const struct spi_device_id ad9834_id[] = {
> > +     {"ad9833", ID_AD9833},
> > +     {"ad9834", ID_AD9834},
> > +     {}
> > +};
> > +
> > +static struct spi_driver ad9834_driver = {
> > +     .driver = {
> > +             .name   = "ad9834",
> > +             .bus    = &spi_bus_type,
> > +             .owner  = THIS_MODULE,
> > +     },
> > +     .probe          = ad9834_probe,
> > +     .remove         = __devexit_p(ad9834_remove),
> > +     .id_table       = ad9834_id,
> > +};
> > +
> > +static int __init ad9834_init(void)
> > +{
> > +     return spi_register_driver(&ad9834_driver);
> > +}
> > +module_init(ad9834_init);
> > +
> > +static void __exit ad9834_exit(void)
> > +{
> > +     spi_unregister_driver(&ad9834_driver);
> > +}
> > +module_exit(ad9834_exit);
> > +
> > +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> > +MODULE_DESCRIPTION("Analog Devices AD9833/AD9834 DDS");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:ad9834");
> > diff --git a/drivers/staging/iio/dds/ad9834.h
> > b/drivers/staging/iio/dds/ad9834.h
> > new file mode 100644
> > index 0000000..0fc3b88
> > --- /dev/null
> > +++ b/drivers/staging/iio/dds/ad9834.h
> > @@ -0,0 +1,112 @@
> > +/*
> > + * AD9834 SPI DDS driver
> > + *
> > + * Copyright 2010 Analog Devices Inc.
> > + *
> > + * Licensed under the GPL-2 or later.
> > + */
> > +#ifndef IIO_DDS_AD9834_H_
> > +#define IIO_DDS_AD9834_H_
> > +
> > +/* Registers */
> > +
> > +#define AD9834_REG_CMD               (0 << 14)
> > +#define AD9834_REG_FREQ0     (1 << 14)
> > +#define AD9834_REG_FREQ1     (2 << 14)
> > +#define AD9834_REG_PHASE0    (6 << 13)
> > +#define AD9834_REG_PHASE1    (7 << 13)
> > +
> > +/* Command Control Bits */
> > +
> > +#define AD9834_B28           (1 << 13)
> > +#define AD9834_HLB           (1 << 12)
> > +#define AD9834_FSEL          (1 << 11)
> > +#define AD9834_PSEL          (1 << 10)
> > +#define AD9834_PIN_SW                (1 << 9)
> > +#define AD9834_RESET         (1 << 8)
> > +#define AD9834_SLEEP1                (1 << 7)
> > +#define AD9834_SLEEP12               (1 << 6)
> > +#define AD9834_OPBITEN               (1 << 5)
> > +#define AD9834_SIGN_PIB              (1 << 4)
> > +#define AD9834_DIV2          (1 << 3)
> > +#define AD9834_MODE          (1 << 1)
> > +
> > +#define AD9834_FREQ_BITS     28
> > +#define AD9834_PHASE_BITS    12
> > +
> > +#define RES_MASK(bits)       ((1 << (bits)) - 1)
> > +
> > +/**
> > + * struct ad9834_state - driver instance specific data
> > + * @indio_dev:               the industrial I/O device
> > + * @spi:             spi_device
> > + * @reg:             supply regulator
> > + * @mclk:            external master clock
> > + * @control:         cached control word
> > + * @xfer:            default spi transfer
> > + * @msg:             default spi message
> > + * @freq_xfer:               tuning word spi transfer
> > + * @freq_msg:                tuning word spi message
> > + * @data:            spi transmit buffer
> > + * @freq_data:               tuning word spi transmit buffer
> > + */
> > +
> > +struct ad9834_state {
> > +     struct iio_dev                  *indio_dev;
> > +     struct spi_device               *spi;
> > +     struct regulator                *reg;
> > +     unsigned int                    mclk;
> > +     unsigned short                  control;
> > +     unsigned short                  devid;
> > +     struct spi_transfer             xfer;
> > +     struct spi_message              msg;
> > +     struct spi_transfer             freq_xfer[2];
> > +     struct spi_message              freq_msg;
> > +
> > +     /*
> > +      * DMA (thus cache coherency maintenance) requires the
> > +      * transfer buffers to live in their own cache lines.
> > +      */
> > +     unsigned short                  data ____cacheline_aligned;
> > +     unsigned short                  freq_data[2] ;
> > +};
> > +
> > +
> > +/*
> > + * TODO: struct ad7887_platform_data needs to go into
> include/linux/iio
> > + */
> > +
> > +/**
> > + * struct ad9834_platform_data - platform specific information
> > + * @mclk:            master clock in Hz
> > + * @freq0:           power up freq0 tuning word in Hz
> > + * @freq1:           power up freq1 tuning word in Hz
> > + * @phase0:          power up phase0 value [0..4095] correlates with
> > 0..2PI
> > + * @phase1:          power up phase1 value [0..4095] correlates with
> > 0..2PI
> > + * @en_div2:         digital output/2 is passed to the SIGN BIT OUT
> pin
> > + * @en_signbit_msb_out:      the MSB (or MSB/2) of the DAC data is
> > connected to the
> > + *                   SIGN BIT OUT pin. en_div2 controls whether it
> is the MSB
> > + *                   or MSB/2 that is output. if
> en_signbit_msb_out=false,
> > + *                   the on-board comparator is connected to SIGN
> BIT OUT
> > + */
> > +
> > +struct ad9834_platform_data {
> > +     unsigned int            mclk;
> > +     unsigned int            freq0;
> > +     unsigned int            freq1;
> > +     unsigned short          phase0;
> > +     unsigned short          phase1;
> > +     bool                    en_div2;
> > +     bool                    en_signbit_msb_out;
> > +};
> > +
> > +/**
> > + * ad9834_supported_device_ids:
> > + */
> > +
> > +enum ad9834_supported_device_ids {
> > +     ID_AD9833,
> > +     ID_AD9834,
> > +};
> > +
> > +#endif /* IIO_DDS_AD9834_H_ */
> > --
> > 1.6.0.2

Datta,

Thanks for your review.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

RE: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver

[Datta, Shubhrajyoti]

Michel,
Just a thought not a comment. However I think error handling could be dealt later.

> -----Original Message-----
> From: Hennerich, Michael [mailto:Michael.Hennerich@analog.com]
> Sent: Friday, December 03, 2010 4:13 PM
> To: Datta, Shubhrajyoti; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: Drivers; jic23@cam.ac.uk; device-drivers-devel@blackfin.uclinux.org
> Subject: RE: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver
> 
> > Datta, Shubhrajyoti wrote on 2010-12-02:
> > > -----Original Message-----
> > > From: linux-iio-owner@vger.kernel.org [mailto:linux-iio-
> > > owner@vger.kernel.org] On Behalf Of michael.hennerich@analog.com
> > > Sent: Thursday, December 02, 2010 5:51 PM
> > > To: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Cc: drivers@analog.com; jic23@cam.ac.uk; device-drivers-
> > > devel@blackfin.uclinux.org; Michael Hennerich
> > > Subject: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver
> > >
> > > From: Michael Hennerich <michael.hennerich@analog.com>
> > >
> > > Example driver using the proposed ABI
> > >
> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > > ---
> > >  drivers/staging/iio/dds/Kconfig  |    7 +
> > >  drivers/staging/iio/dds/Makefile |    1 +
> > >  drivers/staging/iio/dds/ad9834.c |  483
> > > ++++++++++++++++++++++++++++++++++++++
> > >  drivers/staging/iio/dds/ad9834.h |  112 +++++++++
> > >  4 files changed, 603 insertions(+), 0 deletions(-)
> > >  create mode 100644 drivers/staging/iio/dds/ad9834.c
> > >  create mode 100644 drivers/staging/iio/dds/ad9834.h
> > >
> > > diff --git a/drivers/staging/iio/dds/Kconfig
> > > b/drivers/staging/iio/dds/Kconfig
> > > index 7969be2..4c9cce3 100644
> > > --- a/drivers/staging/iio/dds/Kconfig
> > > +++ b/drivers/staging/iio/dds/Kconfig
> > > @@ -17,6 +17,13 @@ config AD9832
> > >         Say yes here to build support for Analog Devices DDS chip
> > >         ad9832 and ad9835, provides direct access via sysfs.
> > >
> > > +config AD9834
> > > +     tristate "Analog Devices ad9833/4/ driver"
> > > +     depends on SPI
> > > +     help
> > > +       Say yes here to build support for Analog Devices DDS chip
> > > +       AD9833 and AD9834, provides direct access via sysfs.
> > > +
> > >  config AD9850
> > >       tristate "Analog Devices ad9850/1 driver"
> > >       depends on SPI
> > > diff --git a/drivers/staging/iio/dds/Makefile
> > > b/drivers/staging/iio/dds/Makefile
> > > index 6f274ac..1477461 100644
> > > --- a/drivers/staging/iio/dds/Makefile
> > > +++ b/drivers/staging/iio/dds/Makefile
> > > @@ -4,6 +4,7 @@
> > >
> > >  obj-$(CONFIG_AD5930) += ad5930.o
> > >  obj-$(CONFIG_AD9832) += ad9832.o
> > > +obj-$(CONFIG_AD9834) += ad9834.o
> > >  obj-$(CONFIG_AD9850) += ad9850.o
> > >  obj-$(CONFIG_AD9852) += ad9852.o
> > >  obj-$(CONFIG_AD9910) += ad9910.o
> > > diff --git a/drivers/staging/iio/dds/ad9834.c
> > > b/drivers/staging/iio/dds/ad9834.c
> > > new file mode 100644
> > > index 0000000..eb1fabf
> > > --- /dev/null
> > > +++ b/drivers/staging/iio/dds/ad9834.c
> > > @@ -0,0 +1,483 @@
> > > +/*
> > > + * AD9834 SPI DAC driver
> > > + *
> > > + * Copyright 2010 Analog Devices Inc.
> > > + *
> > > + * Licensed under the GPL-2 or later.
> > > + */
> > > +
> > > +#include <linux/interrupt.h>
> > > +#include <linux/workqueue.h>
> > > +#include <linux/device.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/sysfs.h>
> > > +#include <linux/list.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/err.h>
> > > +#include <asm/div64.h>
> > > +
> > > +#include "../iio.h"
> > > +#include "../sysfs.h"
> > > +#include "dds.h"
> > > +
> > > +#include "ad9834.h"
> > > +
> > > +static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned
> > long
> > > fout)
> > > +{
> > > +     unsigned long long freqreg = (u64) fout * (u64) (1 <<
> > > AD9834_FREQ_BITS);
> > > +     do_div(freqreg, mclk);
> > > +     return freqreg;
> > > +}
> > > +
> > > +static int ad9834_write_frequency(struct ad9834_state *st,
> > > +                               unsigned long addr, unsigned long
> > fout)
> > > +{
> > > +     unsigned long regval;
> > > +
> > > +     if (fout > (st->mclk / 2))
> > > +             return -EINVAL;
> > > +
> > > +     regval = ad9834_calc_freqreg(st->mclk, fout);
> > > +
> > > +     st->freq_data[0] = cpu_to_be16(addr | (regval &
> > > +                                    RES_MASK(AD9834_FREQ_BITS /
> > 2)));
> > > +     st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> > > +                                    (AD9834_FREQ_BITS / 2)) &
> > > +                                    RES_MASK(AD9834_FREQ_BITS /
> > 2)));
> > > +
> > > +     return spi_sync(st->spi, &st->freq_msg);;
> > > +}
> > > +
> > > +static int ad9834_write_phase(struct ad9834_state *st,
> > > +                               unsigned long addr, unsigned long
> > phase)
> > > +{
> > > +     if (phase > (1 << AD9834_PHASE_BITS))
> > > +             return -EINVAL;
> > > +     st->data = cpu_to_be16(addr | phase);
> > > +
> > > +     return spi_sync(st->spi, &st->msg);
> > > +}
> > > +
> > > +static ssize_t ad9834_write(struct device *dev,
> > > +             struct device_attribute *attr,
> > > +             const char *buf,
> > > +             size_t len)
> > > +{
> > > +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> > > +     struct ad9834_state *st = dev_info->dev_data;
> > > +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > > +     int ret;
> > > +     long val;
> > > +
> > > +     ret = strict_strtol(buf, 10, &val);
> > > +     if (ret)
> > > +             goto error_ret;
> > Could we do some bounds check.
> The functions called with val as argument from the switch cases below, do
> some bound checking, where necessary.
> It's questionable whether these simple enable switch cases, should return
> with
> error if someone writes something other than 0 or 1.

In that case we could we at least consider ret = strict_strtoul(buf, 10, &val);

> > > +
> > > +     mutex_lock(&dev_info->mlock);
> > > +     switch (this_attr->address) {
> > > +     case AD9834_REG_FREQ0:
> > > +     case AD9834_REG_FREQ1:
> > > +             ret = ad9834_write_frequency(st, this_attr->address,
> > val);
> > > +             break;
> > > +     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 */
> > > +                     break;
> > > +             }
> > > +
> > > +             if (val)
> > > +                     st->control &= ~AD9834_OPBITEN;
> > > +             else
> > > +                     st->control |= AD9834_OPBITEN;
Here you check for something other than 0 if the user passes -1 then it may go through.