Message-ID: <6fbbfef2-8c67-da8e-9328-d79abb1bfc5c@kernel.org>
Date:   Sat, 8 Apr 2017 18:19:35 +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,
        linus.walleij@...aro.org, amelie.delaunay@...com,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>
Subject: Re: [PATCH 3/4] iio: dac: stm32: add support for trigger events

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

Powered by blists - more mailing lists