Re: [PATCH 3/4] iio: dac: stm32: add support for trigger events

From: Jonathan Cameron
Date: Sat Apr 08 2017 - 13:19:47 EST

Next message: Piotr Luc: "[PATCH v1] x86/mce: enable PPIN for Knights Landing/Mill"
Previous message: Arushi Singhal: "[PATCH] net: netfilter: ipvs: Replace explicit NULL comparison"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 05/04/17 17:44, Fabrice Gasnier wrote:
> On 04/02/2017 02:21 PM, Jonathan Cameron wrote:
>> On 02/04/17 12:45, Jonathan Cameron wrote:
>>> On 31/03/17 12:45, Fabrice Gasnier wrote:
>>>> STM32 DAC supports triggers to synchronize conversions. When trigger
>>>> occurs, data is transferred from DHR (data holding register) to DOR
>>>> (data output register) so output voltage is updated.
>>>> Both hardware and software triggers are supported.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx>
>>> Hmm. This is a somewhat different use of triggered event from normal...
>>>
> Waveform generator in STM32 DAC requires a trigger to increment /
> decrement internal counter in case of triangle generator. Noise
> generator is a bit different, but same trigger usage applies. I agree
> this is unusual.
> Is it acceptable to use event trigger for this use ?
Not sure. It's kind of a like an output trigger but with the buffer
tied to a hardware source (a bit like the hardware consumers we have
in the other direction).

I think it's closer to an output trigger than an event trigger certainly.
Question is whether it should be something else entirely...
>
>>> What you have here is rather closer to the output buffers stuff that Analog
>>> have in their tree which hasn't made it upstream yet.
>>> To that end I'll want Lars to have a look at this... I've completely
>>> lost track of where they are with this.
>>> Perhaps Lars can give us a quick update?
>>>
>>> If that was in place (or what I have in my head was true anyway),
>>> it would look like the reverse of the triggered buffer input devices.
>>> You'd be able to write to a software buffer and it would clock them
>>> out as the trigger fires (here I think it would have to keep updating
>>> the DHR whenever the trigger occurs).
>
> Hmm.. for waveform generator mode, there is no need for data buffer. DAC
> generate samples itself, using trigger. But, i agree it would be nice
> for playing data samples (write DHR registers, or dma), yes.
Definitely in the nice to have category - except wrt to having a
hardware_buffer_provider or similar to provide the data stream.
This is kind of similar to when we have a data pipeline on incoming data
where the data is never actually visible to some IIO device because it's
pushed into some processing engine directly.

It's a bit of an oddity that we need to think about when looking at output
triggering. The previous DDS devices I have seen have all be directly
clocked...
>
>>>
>>> Even if it's not there, we aren't necessarily looking at terribly big job
>>> to implement it in the core and that would make this handling a lot more
>>> 'standard' and consistent.
>>
>> Having tracked down some limited docs (AN3126 - Audio and waveform
>> generation using the DAC in STM32 microcontrollers) the fact this
>> can also be driven by DMA definitely argues in favour of working with
>> Analog on getting the output buffers support upstream.
>>
>> *crosses fingers people have the time!*
>
> Hopefully this can happen.
>
> For the time being, I'll propose a similar patch in V2. I found out this
> patch is missing a clear path to (re-)assign trigger, once set by
> userland. Also, driver never gets informed in case trigger gets changed
> or removed, without re-enabling it:
> e.g. like echo "" > trigger/current_trigger
> I'll propose a small change. Hope you agree with this approach.
>
Cool. Definitely looking for some analog devices input on this.
They have a quite a few similar out of tree drivers beyond those currently
in staging...

Michael / Lars?

I briefly discussed output buffers with Lars earlier in the week and the
main outstanding issues were around userspace ABI. Last time we talked
about this in detail was quite a few years ago IIRC so time for a revisit.
+ hopefully some progress.

Jonathan
> Thanks,
> Fabrice
>
>>>
>>> Jonathan
>>>
>>>> ---
>>>> drivers/iio/dac/Kconfig | 3 +
>>>> drivers/iio/dac/stm32-dac-core.h | 12 ++++
>>>> drivers/iio/dac/stm32-dac.c | 124 ++++++++++++++++++++++++++++++++++++++-
>>>> 3 files changed, 136 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>>>> index 7198648..786c38b 100644
>>>> --- a/drivers/iio/dac/Kconfig
>>>> +++ b/drivers/iio/dac/Kconfig
>>>> @@ -278,6 +278,9 @@ config STM32_DAC
>>>> tristate "STMicroelectronics STM32 DAC"
>>>> depends on (ARCH_STM32 && OF) || COMPILE_TEST
>>>> depends on REGULATOR
>>>> + select IIO_TRIGGERED_EVENT
>>>> + select IIO_STM32_TIMER_TRIGGER
>>>> + select MFD_STM32_TIMERS
>>>> select STM32_DAC_CORE
>>>> help
>>>> Say yes here to build support for STMicroelectronics STM32 Digital
>>>> diff --git a/drivers/iio/dac/stm32-dac-core.h b/drivers/iio/dac/stm32-dac-core.h
>>>> index d3099f7..3bf211c 100644
>>>> --- a/drivers/iio/dac/stm32-dac-core.h
>>>> +++ b/drivers/iio/dac/stm32-dac-core.h
>>>> @@ -26,6 +26,7 @@
>>>>
>>>> /* STM32 DAC registers */
>>>> #define STM32_DAC_CR 0x00
>>>> +#define STM32_DAC_SWTRIGR 0x04
>>>> #define STM32_DAC_DHR12R1 0x08
>>>> #define STM32_DAC_DHR12R2 0x14
>>>> #define STM32_DAC_DOR1 0x2C
>>>> @@ -33,8 +34,19 @@
>>>>
>>>> /* STM32_DAC_CR bit fields */
>>>> #define STM32_DAC_CR_EN1 BIT(0)
>>>> +#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)
>>>> +#define STM32_DAC_SWTRIGR_SWTRIG2 BIT(1)
>>>>
>>>> /**
>>>> * struct stm32_dac_common - stm32 DAC driver common data (for all instances)
>>>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>>>> index ee9711d..62e43e9 100644
>>>> --- a/drivers/iio/dac/stm32-dac.c
>>>> +++ b/drivers/iio/dac/stm32-dac.c
>>>> @@ -23,6 +23,10 @@
>>>> #include <linux/bitfield.h>
>>>> #include <linux/delay.h>
>>>> #include <linux/iio/iio.h>
>>>> +#include <linux/iio/timer/stm32-timer-trigger.h>
>>>> +#include <linux/iio/trigger.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +#include <linux/iio/triggered_event.h>
>>>> #include <linux/kernel.h>
>>>> #include <linux/module.h>
>>>> #include <linux/platform_device.h>
>>>> @@ -31,15 +35,112 @@
>>>>
>>>> #define STM32_DAC_CHANNEL_1 1
>>>> #define STM32_DAC_CHANNEL_2 2
>>>> +/* channel2 shift */
>>>> +#define STM32_DAC_CHAN2_SHIFT 16
>>>>
>>>> /**
>>>> * struct stm32_dac - private data of DAC driver
>>>> * @common: reference to DAC common data
>>>> + * @swtrig: Using software trigger
>>>> */
>>>> struct stm32_dac {
>>>> struct stm32_dac_common *common;
>>>> + bool swtrig;
>>>> };
>>>>
>>>> +/**
>>>> + * struct stm32_dac_trig_info - DAC trigger info
>>>> + * @name: name of the trigger, corresponding to its source
>>>> + * @tsel: trigger selection, value to be configured in DAC_CR.TSELx
>>>> + */
>>>> +struct stm32_dac_trig_info {
>>>> + const char *name;
>>>> + u32 tsel;
>>>> +};
>>>> +
>>>> +static const struct stm32_dac_trig_info stm32h7_dac_trinfo[] = {
>>>> + { "swtrig", 0 },
>>>> + { TIM1_TRGO, 1 },
>>>> + { TIM2_TRGO, 2 },
>>>> + { TIM4_TRGO, 3 },
>>>> + { TIM5_TRGO, 4 },
>>>> + { TIM6_TRGO, 5 },
>>>> + { TIM7_TRGO, 6 },
>>>> + { TIM8_TRGO, 7 },
>>>> + {},
>>>> +};
>>>> +
>>>> +static irqreturn_t stm32_dac_trigger_handler(int irq, void *p)
>>>> +{
>>>> + struct iio_poll_func *pf = p;
>>>> + struct iio_dev *indio_dev = pf->indio_dev;
>>>> + struct stm32_dac *dac = iio_priv(indio_dev);
>>>> + int channel = indio_dev->channels[0].channel;
>>>> +
>>>> + /* Using software trigger? Then, trigger it now */
>>>> + if (dac->swtrig) {
>>>> + u32 swtrig;
>>>> +
>>>> + if (channel == STM32_DAC_CHANNEL_1)
>>>> + swtrig = STM32_DAC_SWTRIGR_SWTRIG1;
>>>> + else
>>>> + swtrig = STM32_DAC_SWTRIGR_SWTRIG2;
>>>> + regmap_update_bits(dac->common->regmap, STM32_DAC_SWTRIGR,
>>>> + swtrig, swtrig);
>>>> + }
>>>> +
>>>> + iio_trigger_notify_done(indio_dev->trig);
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static unsigned int stm32_dac_get_trig_tsel(struct stm32_dac *dac,
>>>> + struct iio_trigger *trig)
>>>> +{
>>>> + unsigned int i;
>>>> +
>>>> + /* skip 1st trigger that should be swtrig */
>>>> + for (i = 1; stm32h7_dac_trinfo[i].name; i++) {
>>>> + /*
>>>> + * Checking both stm32 timer trigger type and trig name
>>>> + * should be safe against arbitrary trigger names.
>>>> + */
>>>> + if (is_stm32_timer_trigger(trig) &&
>>>> + !strcmp(stm32h7_dac_trinfo[i].name, trig->name)) {
>>>> + return stm32h7_dac_trinfo[i].tsel;
>>>> + }
>>>> + }
>>>> +
>>>> + /* When no trigger has been found, default to software trigger */
>>>> + dac->swtrig = true;
>>>> +
>>>> + return stm32h7_dac_trinfo[0].tsel;
>>>> +}
>>>> +
>>>> +static int stm32_dac_set_trig(struct stm32_dac *dac, struct iio_trigger *trig,
>>>> + int channel)
>>>> +{
>>>> + struct iio_dev *indio_dev = iio_priv_to_dev(dac);
>>>> + u32 shift = channel == STM32_DAC_CHANNEL_1 ? 0 : STM32_DAC_CHAN2_SHIFT;
>>>> + u32 val = 0, tsel;
>>>> + u32 msk = (STM32H7_DAC_CR_TEN1 | STM32H7_DAC_CR_TSEL1) << shift;
>>>> +
>>>> + dac->swtrig = false;
>>>> + if (trig) {
>>>> + /* select & enable trigger (tsel / ten) */
>>>> + tsel = stm32_dac_get_trig_tsel(dac, trig);
>>>> + val = tsel << STM32H7_DAC_CR_TSEL1_SHIFT;
>>>> + val = (val | STM32H7_DAC_CR_TEN1) << shift;
>>>> + }
>>>> +
>>>> + if (trig)
>>>> + dev_dbg(&indio_dev->dev, "enable trigger: %s\n", trig->name);
>>>> + else
>>>> + dev_dbg(&indio_dev->dev, "disable trigger\n");
>>>> +
>>>> + return regmap_update_bits(dac->common->regmap, STM32_DAC_CR, msk, val);
>>>> +}
>>>> +
>>>> static int stm32_dac_is_enabled(struct stm32_dac *dac, int channel)
>>>> {
>>>> u32 en, val;
>>>> @@ -63,9 +164,16 @@ static int stm32_dac_enable(struct iio_dev *indio_dev, int channel)
>>>> STM32_DAC_CR_EN1 : STM32_DAC_CR_EN2;
>>>> int ret;
>>>>
>>>> + ret = stm32_dac_set_trig(dac, indio_dev->trig, channel);
>>>> + if (ret < 0) {
>>>> + dev_err(&indio_dev->dev, "Trigger setup failed\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, en);
>>>> if (ret < 0) {
>>>> dev_err(&indio_dev->dev, "Enable failed\n");
>>>> + stm32_dac_set_trig(dac, NULL, channel);
>>>> return ret;
>>>> }
>>>>
>>>> @@ -88,10 +196,12 @@ static int stm32_dac_disable(struct iio_dev *indio_dev, int channel)
>>>> int ret;
>>>>
>>>> ret = regmap_update_bits(dac->common->regmap, STM32_DAC_CR, en, 0);
>>>> - if (ret)
>>>> + if (ret) {
>>>> dev_err(&indio_dev->dev, "Disable failed\n");
>>>> + return ret;
>>>> + }
>>>>
>>>> - return ret;
>>>> + return stm32_dac_set_trig(dac, NULL, channel);
>>>> }
>>>>
>>>> static int stm32_dac_get_value(struct stm32_dac *dac, int channel, int *val)
>>>> @@ -258,10 +368,17 @@ static int stm32_dac_probe(struct platform_device *pdev)
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> - ret = iio_device_register(indio_dev);
>>>> + ret = iio_triggered_event_setup(indio_dev, NULL,
>>>> + stm32_dac_trigger_handler);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> + ret = iio_device_register(indio_dev);
>>>> + if (ret) {
>>>> + iio_triggered_event_cleanup(indio_dev);
>>>> + return ret;
>>>> + }
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -269,6 +386,7 @@ static int stm32_dac_remove(struct platform_device *pdev)
>>>> {
>>>> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>>
>>>> + iio_triggered_event_cleanup(indio_dev);
>>>> iio_device_unregister(indio_dev);
>>>>
>>>> return 0;
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

Next message: Piotr Luc: "[PATCH v1] x86/mce: enable PPIN for Knights Landing/Mill"
Previous message: Arushi Singhal: "[PATCH] net: netfilter: ipvs: Replace explicit NULL comparison"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]