Date:	Mon, 13 Dec 2010 21:21:34 +0000
From:	Jonathan Cameron <jic23@....ac.uk>
To:	michael.hennerich@...log.com
CC:	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	drivers@...log.com, device-drivers-devel@...ckfin.uclinux.org
Subject: Re: [PATCH 2/3] IIO: dds.h convenience macros

On 12/13/10 16:27, michael.hennerich@...log.com wrote:
> From: Michael Hennerich <michael.hennerich@...log.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!

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).

Jonathan
> Signed-off-by: Michael Hennerich <michael.hennerich@...log.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?
> +
> +/**
> + * /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
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists