lkml.org 
[lkml]   [2010]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [PATCH 2/3] IIO: dds.h convenience macros
> Jonathan Cameron wrote on 2010-12-13:
> > On 12/13/10 16:27, michael.hennerich@analog.com wrote:
> > From: Michael Hennerich <michael.hennerich@analog.com>
> >
> > Changes since RFC/v1:
> > IIO: Apply list review feedback
> >
> > Apply list review feedback:
> > Rename attributes to fit IIO convention used in other drivers.
> > Provide ddsX_out_enable as opposed to ddsX_out_disable.
> > Fix typos.
> > Hi Michael,
>
> I'm afraid all the comments on docs carry over to this file. If you
> aren't particularly attached to having docs in here I'd be tempted to
> drop them purely so we don't end up with two differing sets in the
> future. The docs file is more or less obligatory whereas nice docs
> in here is just a bonus for driver devs. I really don't mind, but
> we'll have to keep a close eye to ensure they don't get significantly
> out of sync!

I'll drop the descriptions within the comments, but keep the file names.

> The other comments are almost all about restricting parameters to be
> write only. I think this is a bit restrictive and some drivers may
> allow much of this stuff to be read back (even if only from cache).

The parts I looked at so far were write only parts, but right you are.
I'll add the read methods to the macros.

> Jonathan
> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > ---
> > drivers/staging/iio/dds/dds.h | 152
> +++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 152 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..a8dd7be
> > --- /dev/null
> > +++ b/drivers/staging/iio/dds/dds.h
> > @@ -0,0 +1,152 @@
> > +/*
> > + * 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/device[n]/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)
> Can envision some parts allowing this to be read back. So I'd be
> inclined
> to make mode a parameter of the macro and allow a read function. Also
> is _device
> a little misleading? Perhaps use _channel?

Right, channel fits better.

> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/ddsX_freqY_scale
> > + * 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/device[n]/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);
> Again I can envision parts for which this may be read back so this is
> probably
> to restrictive.
> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/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)
> Another one that I think should allow reading (if the driver wants to
> provide
> it anyway!)
> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/ddsX_phaseY_scale
> > + * 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/device[n]/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);
> Another that I'd imagine some drivers may allow to be read back.
> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/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);
> ditto.
> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/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);
> ditto.
> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/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);
> ditto
> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/ddsX_out_enable
> > + * Description:
> > + * Disables any signal generation on all outputs.
> > + */
> > +
> > +#define IIO_DEV_ATTR_OUT_ENABLE(_device, _store, _addr)
> \
> > + IIO_DEVICE_ATTR(dds##_device##_out_enable, \
> > + S_IWUSR, NULL, _store, _addr);
> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/ddsX_outY_enable
> > + * Description:
> > + * Disables any signal generation on output Y.
> Comment is lagging changes in the macro.
> > + */
> > +
> > +#define IIO_DEV_ATTR_OUTY_ENABLE(_device, _output, _store, _addr)
> \
> > + IIO_DEVICE_ATTR(dds##_device##_out##_output##_enable, \
> > + S_IWUSR, NULL, _store, _addr);
> Why write only?
> > +
> > +/**
> > + * /sys/bus/iio/devices/device[n]/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/device[n]/ddsX_outY_wavetype_available
> > + * Description:
> > + * Lists all available output waveform options.
> > + */
> > +#define IIO_CONST_ATTR_OUT_WAVETYPES_AVAILABLE(_device, _output,
> _modes)\
> > + IIO_CONST_ATTR(dds##_device##_out##_output##_wavetype_available,\
> > + _modes);
> > --
> > 1.6.0.2
> >
> >



\
 
 \ /
  Last update: 2010-12-14 11:19    [W:0.067 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site