Messages in this thread |  | | | From | "Hennerich, Michael" <> | | Date | Tue, 14 Dec 2010 10:16:31 +0000 | | Subject | RE: [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 > > > >
|  |