| 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
| ||
|
Message-ID: <9b44ab23-b4a6-ef9a-48a9-8283e917223d@kernel.org>
Date: Fri, 14 Apr 2017 16:28:57 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Fabrice Gasnier <fabrice.gasnier@...com>, linux@...linux.org.uk,
robh+dt@...nel.org, linux-arm-kernel@...ts.infradead.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: linux-iio@...r.kernel.org, mark.rutland@....com,
mcoquelin.stm32@...il.com, alexandre.torgue@...com,
lars@...afoo.de, knaack.h@....de, pmeerw@...erw.net,
benjamin.gaignard@...aro.org, benjamin.gaignard@...com
Subject: Re: [PATCH v2 5/5] iio: dac: stm32: add support for waveform
generator
On 10/04/17 17:43, Fabrice Gasnier wrote:
> On 04/09/2017 11:34 AM, Jonathan Cameron wrote:
>> On 06/04/17 17:11, Fabrice Gasnier wrote:
>>> STM32 DAC has built-in noise or triangle waveform generator.
>>> - "wavetype" extended attribute selects noise or triangle.
>>> - "amplitude" extended attribute selects amplitude for waveform generator
>>>
>>> A DC offset can be added to waveform generator output. This can be done
>>> using out_voltage[1/2]_offset
>>>
>>> Waveform generator requires a trigger to be configured, to increment /
>>> decrement internal counter in case of triangle generator. Noise
>>> generator is a bit different, but also requires a trigger to generate
>>> samples.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
>>
>> Various bits inline. Mostly I think the blockers on this will be
>> making sure the ABI defined is generic enough to handle the more crazy
>> DDS chips out there... (basically the ones doing frequency modulation).
>>
>> Jonathan
>>> ---
>>> Changes in v2:
>>> - use _offset parameter to add DC offset to waveform generator
>> Conceptually this offset is just the normal DAC output value (particularly
> yes
>> in the flat case) I guess we can paper over this by having the _raw
>> and this always have th same value, but it's a little inelegant.
>> Still people are going to expect _raw to control it when in DAC mode but
>> that makes limited sense in DDS mode.
>>
>> Mind you nothing stops us defining all DDS channels as the sum of whatever
>> the DDS is doing and whatever is _raw is set to. Perhaps we tidy this up
>> purely through documentation. Think of the DDS as a modulation on top
>> of the DAC...
>>
>>> - Rework ABI to better fit existing DDS ABI: use out_voltageY_wavetype,
>>> out_voltage_wavetype_available, out_voltageY_amplitude,
>>> out_voltage_amplitude_available
>> Hmm. I'm thinking those amplitude values aren't nice and don't fit well
>> with the more general ABI.
>>
>> I suggested (but didn't really expand upon) having standard defined types
>> for each waveform then using scale to control the amplitude.
>
> Do you mean _scale attribute ?
Yes
>>
>> Is that something that might work here?
>
> I probably miss the point here...
>>
>> So say we have our triangle standard form having an amplitude of 1V Peak to
>> Peak. Then we can use scale to make it whatever we actually have in this
>> case? The docs for wave type will need to describe those standard forms
>> though.
> ... scale is fixed here, in line with _raw attribute. In 'amplitude'
> description for STM32 DAC here (e.g. from 1...4095), same scale is used.
> Can you elaborate ?
Good point - it is already effectively been applied to the _raw
value - I'd forgotten that.
Seems like abuse of the ABI to use calibscale, so we might need something
new here - wavescale maybe or ddsscale? Not sure.
>
>>
>> Hmm. Whether this is worth doing is unclear however as we'll still have
>> to describe the 'frequency' in terms of the clock ticks (here the triggers)
>
> Describing frequency may be an issue, not sure it makes senses in this
> case: 'clock ticks', e.g. triggers here may be timers, but also an EXTI
> (external...), or even software trig perhaps.
To describe the waveform in a remotely standard way we'll have to address
this to some degree. What's the slope?
>
>> So maybe amplitude is worth having. Again, looking for input from ADI lot
>> on this... There are some really fiddly cases to describe were we are doing
>> symbol encoding so have multiple waveforms that we are switching between based
>> on some external signal. Any ABI needs to encompass that sort of usage.
>> Parts like the AD9833 for example...
>>
>>> - Better explain trigger usage in case of waveform generator.
>>> ---
>>> Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 | 16 +++
>>> drivers/iio/dac/stm32-dac-core.h | 4 +
>>> drivers/iio/dac/stm32-dac.c | 158 +++++++++++++++++++++-
>>> 3 files changed, 177 insertions(+), 1 deletion(-)
>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>>> new file mode 100644
>>> index 0000000..8f1fa009
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-stm32
>> Fair enough to initially introduced these for this part only, but I'd very
>> much like to see us agree on these sufficiently to get them into the main
>> docs asap so we have something to work with for getting the DDS chips out
>> of staging...
>>> @@ -0,0 +1,16 @@
>>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_wavetype
>>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltage_wavetype_available
>>> +KernelVersion: 4.12
>>> +Contact: fabrice.gasnier@...com
>>> +Description:
>>> + List and/or select waveform generation provided by STM32 DAC:
>>> + - "flat": waveform generator disabled (default)
>>> + - "noise": select noise waveform
>>> + - "triangle": select triangle waveform
>>> +
>>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_amplitude
>>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltage_amplitude_available
>>> +KernelVersion: 4.12
>>> +Contact: fabrice.gasnier@...com
>>> +Description:
>>> + List and/or select amplitude used for waveform generator
>>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>>> index e51a468..0f02975 100644
>>> --- a/drivers/iio/dac/stm32-dac-core.h
>>> +++ b/drivers/iio/dac/stm32-dac-core.h
>>> @@ -37,8 +37,12 @@
>>> #define STM32H7_DAC_CR_TEN1 BIT(1)
>>> #define STM32H7_DAC_CR_TSEL1_SHIFT 2
>>> #define STM32H7_DAC_CR_TSEL1 GENMASK(5, 2)
>>> +#define STM32_DAC_CR_WAVE1 GENMASK(7, 6)
>>> +#define STM32_DAC_CR_MAMP1 GENMASK(11, 8)
>>> #define STM32H7_DAC_CR_HFSEL BIT(15)
>>> #define STM32_DAC_CR_EN2 BIT(16)
>>> +#define STM32_DAC_CR_WAVE2 GENMASK(23, 22)
>>> +#define STM32_DAC_CR_MAMP2 GENMASK(27, 24)
>>>
>>> /* STM32_DAC_SWTRIGR bit fields */
>>> #define STM32_DAC_SWTRIGR_SWTRIG1 BIT(0)
>>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>>> index a7a078e..2ed75db 100644
>>> --- a/drivers/iio/dac/stm32-dac.c
>>> +++ b/drivers/iio/dac/stm32-dac.c
>>> @@ -42,10 +42,12 @@
>>> /**
>>> * struct stm32_dac - private data of DAC driver
>>> * @common: reference to DAC common data
>>> + * @wavetype: waveform generator
>>> * @swtrig: Using software trigger
>>> */
>>> struct stm32_dac {
>>> struct stm32_dac_common *common;
>>> + u32 wavetype;
>>> bool swtrig;
>>> };
>>>
>>> @@ -222,6 +224,29 @@ static int stm32_dac_set_value(struct stm32_dac *dac, int channel, int val)
>>> return ret;
>>> }
>>>
>>> +static int stm32_dac_get_offset(struct stm32_dac *dac, int channel, int *val)
>>> +{
>>> + int ret;
>>> +
>>> + /* Offset is only relevant in waveform generation mode. */
>>> + if (!dac->wavetype) {
>>> + *val = 0;
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + /*
>>> + * In waveform generation mode, DC offset in DHR is added to waveform
>>> + * generator output, then stored to DOR (data output register).
>>> + * Read offset from DHR.
>>> + */
>> Just thinking what fun we could have if we do the fifo based output to push
>> this that I was suggesting for the previous patch ;) triangles on top
>> of fun general waveforms..
>>
>>> + if (STM32_DAC_IS_CHAN_1(channel))
>>> + ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R1, val);
>>> + else
>>> + ret = regmap_read(dac->common->regmap, STM32_DAC_DHR12R2, val);
>>> +
>>> + return ret ? ret : IIO_VAL_INT;
>>> +}
>>> +
>>> static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>>> struct iio_chan_spec const *chan,
>>> int *val, int *val2, long mask)
>>> @@ -231,6 +256,8 @@ static int stm32_dac_read_raw(struct iio_dev *indio_dev,
>>> switch (mask) {
>>> case IIO_CHAN_INFO_RAW:
>>> return stm32_dac_get_value(dac, chan->channel, val);
>>> + case IIO_CHAN_INFO_OFFSET:
>>> + return stm32_dac_get_offset(dac, chan->channel, val);
>>> case IIO_CHAN_INFO_SCALE:
>>> *val = dac->common->vref_mv;
>>> *val2 = chan->scan_type.realbits;
>>> @@ -247,8 +274,16 @@ static int stm32_dac_write_raw(struct iio_dev *indio_dev,
>>> struct stm32_dac *dac = iio_priv(indio_dev);
>>>
>>> switch (mask) {
>>> + case IIO_CHAN_INFO_OFFSET:
>>> + /* Offset only makes sense in waveform generation mode */
>>> + if (dac->wavetype)
>>> + return stm32_dac_set_value(dac, chan->channel, val);
>>> + return -EBUSY;
>> Yeah, I think I sent you down a blind alley here. If people agree, lets
>> just define DDS signals as always being the sum of _raw + the dds element.
>> Then it's easy.
> Ok, I can revert back to use _raw if this is fine ;-)
>
>>> case IIO_CHAN_INFO_RAW:
>>> - return stm32_dac_set_value(dac, chan->channel, val);
>>> + if (!dac->wavetype)
>>> + return stm32_dac_set_value(dac, chan->channel, val);
>>> + /* raw value is read only in waveform generation mode */
>>> + return -EBUSY;
>>> default:
>>> return -EINVAL;
>>> }
>>> @@ -334,6 +369,122 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>> .set = stm32_dac_set_powerdown_mode,
>>> };
>>>
>>> +/* waveform generator wave selection */
>>> +static const char * const stm32_dac_wavetype_desc[] = {
>>> + "flat",
>>> + "noise",
>>> + "triangle",
>>> +};
>>> +
>>> +static int stm32_dac_set_wavetype(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + unsigned int wavetype)
>>> +{
>>> + struct stm32_dac *dac = iio_priv(indio_dev);
>>> + u32 mask, val;
>>> + int ret;
>>> +
>>> + /*
>>> + * Waveform generator requires a trigger to be configured, to increment
>>> + * or decrement internal counter in case of triangle generator. Noise
>>> + * generator is a bit different, but also requires a trigger to
>>> + * generate samples.
>>> + */
>>> + if (wavetype && !indio_dev->trig)
>>> + dev_dbg(&indio_dev->dev, "Wavegen requires a trigger\n");
>>> +
>>> + if (STM32_DAC_IS_CHAN_1(chan->channel)) {
>>> + val = FIELD_PREP(STM32_DAC_CR_WAVE1, wavetype);
>>> + mask = STM32_DAC_CR_WAVE1;
>>> + } else {
>>> + val = FIELD_PREP(STM32_DAC_CR_WAVE2, wavetype);
>>> + mask = STM32_DAC_CR_WAVE2;
>>> + }
>>> +
>>> + ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
>>> + if (ret)
>>> + return ret;
>>> + dac->wavetype = wavetype;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int stm32_dac_get_wavetype(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan)
>>> +{
>>> + struct stm32_dac *dac = iio_priv(indio_dev);
>>> + u32 val;
>>> + int ret;
>>> +
>>> + ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (STM32_DAC_IS_CHAN_1(chan->channel))
>>> + return FIELD_GET(STM32_DAC_CR_WAVE1, val);
>>> + else
>>> + return FIELD_GET(STM32_DAC_CR_WAVE2, val);
>>> +}
>>> +
>>> +static const struct iio_enum stm32_dac_wavetype_enum = {
>>> + .items = stm32_dac_wavetype_desc,
>>> + .num_items = ARRAY_SIZE(stm32_dac_wavetype_desc),
>>> + .get = stm32_dac_get_wavetype,
>>> + .set = stm32_dac_set_wavetype,
>>> +};
>>> +
>>> +/*
>>> + * waveform generator mamp selection: mask/amplitude
>>> + * - noise: LFSR mask (linear feedback shift register, umasks bit 0, [1:0]...)
>> This needs breaking out into two attributes - for noise it isn't amplitude in
>> any conventional sense... Keep the result device specific for now. I'm not
>> even sure what type of pseudo random source that actually is...
> Do you suggest to create specific attribute for this ? This will end-up
> to write same register/bitfield as 'amplitude' for triangle generator.
I think it should definitely be a separate attribute. Otherwise the userspace
ABI will be really confusing as this definitely isn't amplitude in any normal
sense!
>
> Thanks & Regards,
> Fabrice
>
>> If anyone wants to figure it out it would be great to document it in a general
>> form!
>>
>>> + * - triangle: amplitude (equal to 1, 3, 5, 7... 4095)
>>> + */
>>> +static const char * const stm32_dac_amplitude_desc[] = {
>>> + "1", "3", "7", "15", "31", "63", "127", "255", "511", "1023", "2047",
>>> + "4095",
>>> +};
>>> +
>>> +static int stm32_dac_set_amplitude(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + unsigned int amplitude)
>>> +{
>>> + struct stm32_dac *dac = iio_priv(indio_dev);
>>> + u32 mask, val;
>>> +
>>> + if (STM32_DAC_IS_CHAN_1(chan->channel)) {
>>> + val = FIELD_PREP(STM32_DAC_CR_MAMP1, amplitude);
>>> + mask = STM32_DAC_CR_MAMP1;
>>> + } else {
>>> + val = FIELD_PREP(STM32_DAC_CR_MAMP2, amplitude);
>>> + mask = STM32_DAC_CR_MAMP2;
>>> + }
>>> +
>>> + return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, mask, val);
>>> +}
>>> +
>>> +static int stm32_dac_get_amplitude(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan)
>>> +{
>>> + struct stm32_dac *dac = iio_priv(indio_dev);
>>> + u32 val;
>>> + int ret;
>>> +
>>> + ret = regmap_read(dac->common->regmap, STM32_DAC_CR, &val);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (STM32_DAC_IS_CHAN_1(chan->channel))
>>> + return FIELD_GET(STM32_DAC_CR_MAMP1, val);
>>> + else
>>> + return FIELD_GET(STM32_DAC_CR_MAMP2, val);
>>> +}
>>> +
>>> +static const struct iio_enum stm32_dac_amplitude_enum = {
>>> + .items = stm32_dac_amplitude_desc,
>>> + .num_items = ARRAY_SIZE(stm32_dac_amplitude_desc),
>>> + .get = stm32_dac_get_amplitude,
>>> + .set = stm32_dac_set_amplitude,
>>> +};
>>> +
>>> static const struct iio_chan_spec_ext_info stm32_dac_ext_info[] = {
>>> {
>>> .name = "powerdown",
>>> @@ -343,6 +494,10 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>> },
>>> IIO_ENUM("powerdown_mode", IIO_SEPARATE, &stm32_dac_powerdown_mode_en),
>>> IIO_ENUM_AVAILABLE("powerdown_mode", &stm32_dac_powerdown_mode_en),
>>> + IIO_ENUM("wavetype", IIO_SEPARATE, &stm32_dac_wavetype_enum),
>>> + IIO_ENUM_AVAILABLE("wavetype", &stm32_dac_wavetype_enum),
>>> + IIO_ENUM("amplitude", IIO_SEPARATE, &stm32_dac_amplitude_enum),
>>> + IIO_ENUM_AVAILABLE("amplitude", &stm32_dac_amplitude_enum),
>>> {},
>>> };
>>>
>>> @@ -352,6 +507,7 @@ static ssize_t stm32_dac_write_powerdown(struct iio_dev *indio_dev,
>>> .output = 1, \
>>> .channel = chan, \
>>> .info_mask_separate = \
>>> + BIT(IIO_CHAN_INFO_OFFSET) | \
>>> BIT(IIO_CHAN_INFO_RAW) | \
>>> BIT(IIO_CHAN_INFO_SCALE), \
>>> /* scan_index is always 0 as num_channels is 1 */ \
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Powered by blists - more mailing lists