Re: [PATCH v2 6/8] iio: trigger: Add STM32 LPTimer trigger driver

[Jonathan Cameron]

On Fri, 30 Jun 2017 18:26:56 +0200
Fabrice Gasnier <fabrice.gasnier@xxxxxx> wrote:

> On 06/30/2017 03:57 PM, Jonathan Cameron wrote:
> > On Mon, 26 Jun 2017 18:41:36 +0200
> > Fabrice Gasnier <fabrice.gasnier@xxxxxx> wrote:
> >   
> >> On 06/24/2017 10:13 PM, Jonathan Cameron wrote:  
> >>> On Wed, 21 Jun 2017 16:30:13 +0200
> >>> Fabrice Gasnier <fabrice.gasnier@xxxxxx> wrote:
> >>>     
> >>>> Add support for LPTIMx_OUT triggers that can be found on some STM32
> >>>> devices. These triggers can be used then by ADC or DAC.
> >>>> Typical usage is to configure LPTimer as PWM output (via pwm-stm32-lp)
> >>>> and have synchronised analog conversions with these triggers.
> >>>>
> >>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx>    
> >>> Given this can't be used as a trigger for other devices (no exposed
> >>> interrupt?) I'd expect to see a validate_device callback provided for
> >>> the trigger ops.  That would prevent other devices trying to use it.    
> >>
> >> Hi Jonathan,
> >>
> >> This is something I had in mind also earlier. Only thing is...
> >> Basically, this is limiting: when trigger poll happens on device side
> >> (e.g. ADC), another device could use same trigger. But I admit this
> >> looks like corner case.
> >>
> >> I'll add it in next version, with additional patch for ADC part to
> >> validate it's a valid device (No DAC yet).
> >> I think I'll use INDIO_HARDWARE_TRIGGERED mode:
> >> - in adc driver: indio_dev->modes |= INDIO_HARDWARE_TRIGGERED;
> >> - in lptimer: if (indio_dev->modes & INDIO_HARDWARE_TRIGGERED)...  
> > That may not be enough in of itself.  Could be other hardware
> > triggered elements on the platform (quite likely with these
> > stm parts!)
> >   
> 
> Hi Jonathan,
> 
> As far as I can see, devices supporting it need to check trigger anyway
> (e.g. validate_trigger() cb). That is the case currently on stm32-adc
> driver. Do you think this "handshake" is okay ?
> 
> If not, I can probably add an export symbol instead, in stm32-adc, like:
> bool is_stm32_adc_iio_dev(struct iio_dev *indio_dev);
> Then use it in validate_device callback.
Anything is fine as long as it guarantees that we can't bind a device to
a trigger it can't use, or the other way around.  You have check from
both sides to prevent other devices using the trigger, or other triggers
being used by the device.  As far as I can see these are tightly coupled.
If there weren't several of them to chose from I'd argue we should drop
the fact the trigger is exposed at all (like we do in quite a few 
devices with a fifo).

J
> 
> Please let me know.
> 
> Best regards,
> Fabrice
> 
> > J  
> >>  
> >>>
> >>> Otherwise, looks good.    
> >>
> >> Many thanks for your review.
> >> Best Regards,
> >> Fabrice
> >>  
> >>>
> >>> Jonathan    
> >>>> ---
> >>>> Changes in v2:
> >>>> - s/Low Power/Low-Power
> >>>> - update few comments
> >>>> ---
> >>>>  drivers/iio/trigger/Kconfig                   |  11 +++
> >>>>  drivers/iio/trigger/Makefile                  |   1 +
> >>>>  drivers/iio/trigger/stm32-lptimer-trigger.c   | 110 ++++++++++++++++++++++++++
> >>>>  include/linux/iio/timer/stm32-lptim-trigger.h |  24 ++++++
> >>>>  4 files changed, 146 insertions(+)
> >>>>  create mode 100644 drivers/iio/trigger/stm32-lptimer-trigger.c
> >>>>  create mode 100644 include/linux/iio/timer/stm32-lptim-trigger.h
> >>>>
> >>>> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
> >>>> index e4d4e63..a633d2c 100644
> >>>> --- a/drivers/iio/trigger/Kconfig
> >>>> +++ b/drivers/iio/trigger/Kconfig
> >>>> @@ -24,6 +24,17 @@ config IIO_INTERRUPT_TRIGGER
> >>>>  	  To compile this driver as a module, choose M here: the
> >>>>  	  module will be called iio-trig-interrupt.
> >>>>  
> >>>> +config IIO_STM32_LPTIMER_TRIGGER
> >>>> +	tristate "STM32 Low-Power Timer Trigger"
> >>>> +	depends on MFD_STM32_LPTIMER || COMPILE_TEST
> >>>> +	help
> >>>> +	  Select this option to enable STM32 Low-Power Timer Trigger.
> >>>> +	  This can be used as trigger source for STM32 internal ADC
> >>>> +	  and/or DAC.
> >>>> +
> >>>> +	  To compile this driver as a module, choose M here: the
> >>>> +	  module will be called stm32-lptimer-trigger.
> >>>> +
> >>>>  config IIO_STM32_TIMER_TRIGGER
> >>>>  	tristate "STM32 Timer Trigger"
> >>>>  	depends on (ARCH_STM32 && OF && MFD_STM32_TIMERS) || COMPILE_TEST
> >>>> diff --git a/drivers/iio/trigger/Makefile b/drivers/iio/trigger/Makefile
> >>>> index 5c4ecd3..0a72a2a 100644
> >>>> --- a/drivers/iio/trigger/Makefile
> >>>> +++ b/drivers/iio/trigger/Makefile
> >>>> @@ -6,6 +6,7 @@
> >>>>  
> >>>>  obj-$(CONFIG_IIO_HRTIMER_TRIGGER) += iio-trig-hrtimer.o
> >>>>  obj-$(CONFIG_IIO_INTERRUPT_TRIGGER) += iio-trig-interrupt.o
> >>>> +obj-$(CONFIG_IIO_STM32_LPTIMER_TRIGGER) += stm32-lptimer-trigger.o
> >>>>  obj-$(CONFIG_IIO_STM32_TIMER_TRIGGER) += stm32-timer-trigger.o
> >>>>  obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
> >>>>  obj-$(CONFIG_IIO_TIGHTLOOP_TRIGGER) += iio-trig-loop.o
> >>>> diff --git a/drivers/iio/trigger/stm32-lptimer-trigger.c b/drivers/iio/trigger/stm32-lptimer-trigger.c
> >>>> new file mode 100644
> >>>> index 0000000..bcb9aa2
> >>>> --- /dev/null
> >>>> +++ b/drivers/iio/trigger/stm32-lptimer-trigger.c
> >>>> @@ -0,0 +1,110 @@
> >>>> +/*
> >>>> + * STM32 Low-Power Timer Trigger driver
> >>>> + *
> >>>> + * Copyright (C) STMicroelectronics 2017
> >>>> + *
> >>>> + * Author: Fabrice Gasnier <fabrice.gasnier@xxxxxx>.
> >>>> + *
> >>>> + * License terms:  GNU General Public License (GPL), version 2
> >>>> + *
> >>>> + * Inspired by Benjamin Gaignard's stm32-timer-trigger driver
> >>>> + */
> >>>> +
> >>>> +#include <linux/iio/iio.h>
> >>>> +#include <linux/iio/timer/stm32-lptim-trigger.h>
> >>>> +#include <linux/iio/trigger.h>
> >>>> +#include <linux/mfd/stm32-lptimer.h>
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/platform_device.h>
> >>>> +
> >>>> +/* List Low-Power Timer triggers */
> >>>> +static const char * const stm32_lptim_triggers[] = {
> >>>> +	LPTIM1_OUT,
> >>>> +	LPTIM2_OUT,
> >>>> +	LPTIM3_OUT,
> >>>> +};
> >>>> +
> >>>> +struct stm32_lptim_trigger {
> >>>> +	struct device *dev;
> >>>> +	const char *trg;
> >>>> +};
> >>>> +
> >>>> +static const struct iio_trigger_ops stm32_lptim_trigger_ops = {
> >>>> +	.owner = THIS_MODULE,
> >>>> +};
> >>>> +
> >>>> +/**
> >>>> + * is_stm32_lptim_trigger
> >>>> + * @trig: trigger to be checked
> >>>> + *
> >>>> + * return true if the trigger is a valid STM32 IIO Low-Power Timer Trigger
> >>>> + * either return false
> >>>> + */
> >>>> +bool is_stm32_lptim_trigger(struct iio_trigger *trig)
> >>>> +{
> >>>> +	return (trig->ops == &stm32_lptim_trigger_ops);
> >>>> +}
> >>>> +EXPORT_SYMBOL(is_stm32_lptim_trigger);
> >>>> +
> >>>> +static int stm32_lptim_setup_trig(struct stm32_lptim_trigger *priv)
> >>>> +{
> >>>> +	struct iio_trigger *trig;
> >>>> +
> >>>> +	trig = devm_iio_trigger_alloc(priv->dev, "%s", priv->trg);
> >>>> +	if  (!trig)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	trig->dev.parent = priv->dev->parent;
> >>>> +	trig->ops = &stm32_lptim_trigger_ops;
> >>>> +	iio_trigger_set_drvdata(trig, priv);
> >>>> +
> >>>> +	return devm_iio_trigger_register(priv->dev, trig);
> >>>> +}
> >>>> +
> >>>> +static int stm32_lptim_trigger_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +	struct stm32_lptim_trigger *priv;
> >>>> +	u32 index;
> >>>> +	int ret;
> >>>> +
> >>>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> >>>> +	if (!priv)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	if (of_property_read_u32(pdev->dev.of_node, "reg", &index))
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	if (index >= ARRAY_SIZE(stm32_lptim_triggers))
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	priv->dev = &pdev->dev;
> >>>> +	priv->trg = stm32_lptim_triggers[index];
> >>>> +
> >>>> +	ret = stm32_lptim_setup_trig(priv);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	platform_set_drvdata(pdev, priv);
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static const struct of_device_id stm32_lptim_trig_of_match[] = {
> >>>> +	{ .compatible = "st,stm32-lptimer-trigger", },
> >>>> +	{},
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(of, stm32_lptim_trig_of_match);
> >>>> +
> >>>> +static struct platform_driver stm32_lptim_trigger_driver = {
> >>>> +	.probe = stm32_lptim_trigger_probe,
> >>>> +	.driver = {
> >>>> +		.name = "stm32-lptimer-trigger",
> >>>> +		.of_match_table = stm32_lptim_trig_of_match,
> >>>> +	},
> >>>> +};
> >>>> +module_platform_driver(stm32_lptim_trigger_driver);
> >>>> +
> >>>> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@xxxxxx>");
> >>>> +MODULE_ALIAS("platform:stm32-lptimer-trigger");
> >>>> +MODULE_DESCRIPTION("STMicroelectronics STM32 LPTIM trigger driver");
> >>>> +MODULE_LICENSE("GPL v2");
> >>>> diff --git a/include/linux/iio/timer/stm32-lptim-trigger.h b/include/linux/iio/timer/stm32-lptim-trigger.h
> >>>> new file mode 100644
> >>>> index 0000000..cb795b1
> >>>> --- /dev/null
> >>>> +++ b/include/linux/iio/timer/stm32-lptim-trigger.h
> >>>> @@ -0,0 +1,24 @@
> >>>> +/*
> >>>> + * Copyright (C) STMicroelectronics 2017
> >>>> + *
> >>>> + * Author: Fabrice Gasnier <fabrice.gasnier@xxxxxx>
> >>>> + *
> >>>> + * License terms:  GNU General Public License (GPL), version 2
> >>>> + */
> >>>> +
> >>>> +#ifndef _STM32_LPTIM_TRIGGER_H_
> >>>> +#define _STM32_LPTIM_TRIGGER_H_
> >>>> +
> >>>> +#define LPTIM1_OUT	"lptim1_out"
> >>>> +#define LPTIM2_OUT	"lptim2_out"
> >>>> +#define LPTIM3_OUT	"lptim3_out"
> >>>> +
> >>>> +#if IS_ENABLED(CONFIG_IIO_STM32_LPTIMER_TRIGGER)
> >>>> +bool is_stm32_lptim_trigger(struct iio_trigger *trig);
> >>>> +#else
> >>>> +static inline bool is_stm32_lptim_trigger(struct iio_trigger *trig)
> >>>> +{
> >>>> +	return false;
> >>>> +}
> >>>> +#endif
> >>>> +#endif    
> >>>     
> > 
> > --
> > 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
> >