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, >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html