Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932703AbdDEQqJ (ORCPT ); Wed, 5 Apr 2017 12:46:09 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:43570 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754288AbdDEQpU (ORCPT ); Wed, 5 Apr 2017 12:45:20 -0400 Subject: Re: [PATCH 3/4] iio: dac: stm32: add support for trigger events To: Jonathan Cameron , , , , , References: <1490960707-22422-1-git-send-email-fabrice.gasnier@st.com> <1490960707-22422-4-git-send-email-fabrice.gasnier@st.com> <7dba5b17-5f82-e68d-3c06-afe4a4e478e7@kernel.org> CC: , , , , , , , , , , From: Fabrice Gasnier Message-ID: Date: Wed, 5 Apr 2017 18:44:21 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <7dba5b17-5f82-e68d-3c06-afe4a4e478e7@kernel.org> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.75.127.45] X-ClientProxiedBy: SFHDAG2NODE2.st.com (10.75.127.5) To SFHDAG5NODE3.st.com (10.75.127.15) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-05_13:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10528 Lines: 307 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 >> 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 ? >> 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. >> >> 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. 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 >>> #include >>> #include >>> +#include >>> +#include >>> +#include >>> +#include >>> #include >>> #include >>> #include >>> @@ -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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >