Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1620561 > unrolled thread

Re: [PATCH v2 5/5] iio: dac: stm32: add support for waveform generator

Started byFabrice Gasnier <fabrice.gasnier@st.com>
First post2017-04-10 20:00 +0200
Last post2017-04-14 17:30 +0200
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 5/5] iio: dac: stm32: add support for waveform  generator Fabrice Gasnier <fabrice.gasnier@st.com> - 2017-04-10 20:00 +0200
    Re: [PATCH v2 5/5] iio: dac: stm32: add support for waveform  generator Jonathan Cameron <jic23@kernel.org> - 2017-04-14 17:30 +0200

#1620561 — Re: [PATCH v2 5/5] iio: dac: stm32: add support for waveform generator

FromFabrice Gasnier <fabrice.gasnier@st.com>
Date2017-04-10 20:00 +0200
SubjectRe: [PATCH v2 5/5] iio: dac: stm32: add support for waveform generator
Message-ID<tuLLB-3YK-55@gated-at.bofh.it>
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@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 ?
> 
> 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 ?

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

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

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

[toc] | [next] | [standalone]

#1623719

FromJonathan Cameron <jic23@kernel.org>
Date2017-04-14 17:30 +0200
Message-ID<twbkC-2J3-7@gated-at.bofh.it>
In reply to#1620561
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@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.gasnier@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.gasnier@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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web