linux-kernel - RE: [PATCH 2/3] IIO: dds.h convenience macros

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <544AC56F16B56944AEC3BD4E3D591771324BCA85C6@LIMKCMBX1.ad.analog.com>
Date:	Tue, 14 Dec 2010 10:16:31 +0000
From:	"Hennerich, Michael" <Michael.Hennerich@...log.com>
To:	Jonathan Cameron <jic23@....ac.uk>
CC:	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Drivers <Drivers@...log.com>,
	"device-drivers-devel@...ckfin.uclinux.org" 
	<device-drivers-devel@...ckfin.uclinux.org>
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@...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!

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@...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?

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

--
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/