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.gasn...@st.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.gasn...@st.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.gasn...@st.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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >