lkml.org 
[lkml]   [2010]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [RFC 1/3] IIO: Direct digital synthesis abi documentation
> 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


\
 
 \ /
  Last update: 2010-12-07 06:21    [W:0.071 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site