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

[Fabrice Gasnier]

On 04/09/2017 11:04 AM, Jonathan Cameron wrote:
> On 06/04/17 17:11, 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. I'm really not sure on how to handle this...  The problem to my mind
> is knowing when it has triggered so we can know that it makes sense to
> put the next reading in.... Anyhow bear with me...
> 
> I think the same arguement holds for this as equivalent input devices.
> There we have always argued that if you need multichannel synchronized
> capture (which is 'kind' of what we are looking at here) then you have
> to use the buffered interface.
> 
> I think we need buffered output for this to fit nicely in the subsystem.
> It definitely isn't a correct use of the event triggers.
> 
> That means we really need to figure out the ABI for buffered output and
> get that sorted.  To my mind it shouldn't be too tricky and should look
> much like buffered input, just with us writing to a fifo from userspace
> rather than reading from it.
> 
> The DMA side of that can come later, in a similar fashion to how it is
> added for the ADC side of things.  We can also have 'providers'
> (equivalent of 'consumers' on the ADC side), perhaps giving a neat way
> of describing DDS devices (I'm not so sure on this yet).
> 
> So to my mind, if you are not in buffered mode and do a sysfs write it
> should be 'instant'. If in buffered mode, then it will wait on the
> trigger.
> 
> So the complex side of things is what we 'know' about the data flow.
> 1) Case you have here.  We want to do direct write through to the device,
> but have no way of knowing (or do we?) that it has triggered and written
> the data to the output.  So we have no way of knowing we can push the next
> value in from a fifo yet...  In this case I guess the solution might be to
> have a fifo length of 0.  That is data flows straight to hardware.
> 
> 2) Simple stream case - always enough data in the fifo and we get an interrupt
> to signify that the previous trigger happened.
> 
> 3) Case where we are only just keeping up.  So we won't have data in the fifo
> until sometime after the previous trigger.  In this case we need the fifo to
> push straight through if there isn't data ready to go.
> 
> 4) Case where we are not pushing data fast enough.  Just don't update?
> 
> That last case 4 is nasty as the reason we typically want to do triggered
> DAC updates is to ensure we always have valid states in some control loop,
> but we might get a race here where one DAC has a value ready to go on a trigger
> and another one isn't quite ready.  In this case we might want to hold off
> until all are ready... So there might need to be a sanity check that everyone
> on a given trigger is ready to go - an extra callback.
> 
> So a bit fiddly and I'm not sure I like the representation of through flow as
> a fifo of 0 length... (can't think of a neater way though atm)
> 
> Anyhow, time to sort output buffers out once and for all I think if we can
> get a reasonable group of people together who have the time.
> 
> Sorry Fabrice that this has hit your driver!  Perhaps we can figure
> enough out to be able to at least get the basics (i.e. patches 1,2) in as
> asap.

Hi Jonathan,

Thanks for sharing your view on this.
I sent patches 1,2 updated with your comments. I dropped following
patches for the time being, as it obviously require additions...

I agree with your analysis and concerns above.
I hope Lars or others can give some feedback or guidelines on output
buffer? Is there something already, we may start to work on ?

Thanks all in advance.
Best Regards,
Fabrice

> 
> Jonathan
>> ---
>> Changes in v2:
>> - Fix issue with trigger, by using set_trigger callback
>> - trigger can now be assigned, no matters powerdown state
>> ---
>>  drivers/iio/dac/Kconfig          |   3 +
>>  drivers/iio/dac/stm32-dac-core.h |   8 +++
>>  drivers/iio/dac/stm32-dac.c      | 127 ++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 137 insertions(+), 1 deletion(-)
>>
>> 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 daf0993..e51a468 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,9 +34,16 @@
>>  
>>  /* 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 STM32H7_DAC_CR_HFSEL		BIT(15)
>>  #define STM32_DAC_CR_EN2		BIT(16)
>>  
>> +/* 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)
>>   * @regmap: DAC registers shared via regmap
>> diff --git a/drivers/iio/dac/stm32-dac.c b/drivers/iio/dac/stm32-dac.c
>> index c0d993a..a7a078e 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>
>> @@ -32,15 +36,113 @@
>>  #define STM32_DAC_CHANNEL_1		1
>>  #define STM32_DAC_CHANNEL_2		2
>>  #define STM32_DAC_IS_CHAN_1(ch)		((ch) & STM32_DAC_CHANNEL_1)
>> +/* 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 */
> Can we get here otherwise?
> If not I'd prefer to either see an error on the other case
> (perhaps simply return IRQ_NONE) 
>> +	if (dac->swtrig) {
>> +		u32 swtrig;
>> +
>> +		if (STM32_DAC_IS_CHAN_1(channel))
>> +			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_trigger(struct iio_dev *indio_dev,
>> +				 struct iio_trigger *trig)
>> +{
>> +	struct stm32_dac *dac = iio_priv(indio_dev);
>> +	int channel = indio_dev->channels[0].channel;
>> +	u32 shift = STM32_DAC_IS_CHAN_1(channel) ? 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 iio_dev *indio_dev, int channel)
>>  {
>>  	struct stm32_dac *dac = iio_priv(indio_dev);
>> @@ -167,6 +269,7 @@ static int stm32_dac_debugfs_reg_access(struct iio_dev *indio_dev,
>>  static const struct iio_info stm32_dac_iio_info = {
>>  	.read_raw = stm32_dac_read_raw,
>>  	.write_raw = stm32_dac_write_raw,
>> +	.set_trigger = stm32_dac_set_trigger,
>>  	.debugfs_reg_access = stm32_dac_debugfs_reg_access,
>>  	.driver_module = THIS_MODULE,
>>  };
>> @@ -326,7 +429,28 @@ static int stm32_dac_probe(struct platform_device *pdev)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	return devm_iio_device_register(&pdev->dev, 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;
>> +}
>> +
>> +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;
>>  }
>>  
>>  static const struct of_device_id stm32_dac_of_match[] = {
>> @@ -337,6 +461,7 @@ static int stm32_dac_probe(struct platform_device *pdev)
>>  
>>  static struct platform_driver stm32_dac_driver = {
>>  	.probe = stm32_dac_probe,
>> +	.remove = stm32_dac_remove,
>>  	.driver = {
>>  		.name = "stm32-dac",
>>  		.of_match_table = stm32_dac_of_match,
>>
>