| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 03 Jan 2017 17:24:01 +0000
From: Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To: Benjamin Gaignard <benjamin.gaignard@...aro.org>,
Jonathan Cameron <jic23@...nel.org>
CC: Lee Jones <lee.jones@...aro.org>, robh+dt@...nel.org,
Mark Rutland <mark.rutland@....com>,
Alexandre Torgue <alexandre.torgue@...com>,
devicetree@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Thierry Reding <thierry.reding@...il.com>,
Linux PWM List <linux-pwm@...r.kernel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
linux-iio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
Fabrice Gasnier <fabrice.gasnier@...com>,
Gerald Baeza <gerald.baeza@...com>,
Arnaud Pouliquen <arnaud.pouliquen@...com>,
Linus Walleij <linus.walleij@...aro.org>,
Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
Benjamin Gaignard <benjamin.gaignard@...com>
Subject: Re: [PATCH v6 6/8] IIO: add STM32 timer trigger driver
On 3 January 2017 12:59:20 GMT+00:00, Benjamin Gaignard <benjamin.gaignard@...aro.org> wrote:
>2017-01-03 10:23 GMT+01:00 Benjamin Gaignard
><benjamin.gaignard@...aro.org>:
>> 2017-01-02 19:22 GMT+01:00 Jonathan Cameron <jic23@...nel.org>:
>>> On 02/01/17 08:46, Benjamin Gaignard wrote:
>>>> 2016-12-30 22:12 GMT+01:00 Jonathan Cameron <jic23@...nel.org>:
>>>>> On 09/12/16 14:15, Benjamin Gaignard wrote:
>>>>>> Timers IPs can be used to generate triggers for other IPs like
>>>>>> DAC, ADC or other timers.
>>>>>> Each trigger may result of timer internals signals like counter
>enable,
>>>>>> reset or edge, this configuration could be done through
>"master_mode"
>>>>>> device attribute.
>>>>>>
>>>>>> A timer device could be triggered by other timers, we use the
>trigger
>>>>>> name and is_stm32_iio_timer_trigger() function to distinguish
>them
>>>>>> and configure IP input switch.
>>>>>>
>>>>>> Timer may also decide on which event (edge, level) they could
>>>>>> be activated by a trigger, this configuration is done by writing
>in
>>>>>> "slave_mode" device attribute.
>>>>>>
>>>>>> Since triggers could also be used by DAC or ADC their names are
>defined
>>>>>> in include/ nux/iio/timer/stm32-timer-trigger.h so those IPs will
>be able
>>>>>> to configure themselves in valid_trigger function
>>>>>>
>>>>>> Trigger have a "sampling_frequency" attribute which allow to
>configure
>>>>>> timer sampling frequency without using PWM interface
>>>>>>
>>>>>> version 5:
>>>>>> - simplify tables of triggers
>>>>>> - only create an IIO device when needed
>>>>>>
>>>>>> version 4:
>>>>>> - get triggers configuration from "reg" in DT
>>>>>> - add tables of triggers
>>>>>> - sampling frequency is enable/disable when writing in trigger
>>>>>> sampling_frequency attribute
>>>>>> - no more use of interruptions
>>>>>>
>>>>>> version 3:
>>>>>> - change compatible to "st,stm32-timer-trigger"
>>>>>> - fix attributes access right
>>>>>> - use string instead of int for master_mode and slave_mode
>>>>>> - document device attributes in sysfs-bus-iio-timer-stm32
>>>>>>
>>>>>> version 2:
>>>>>> - keep only one compatible
>>>>>> - use st,input-triggers-names and st,output-triggers-names
>>>>>> to know which triggers are accepted and/or create by the device
>>>>> Firstly, sorry it has taken me so long to get back to this.
>>>>>
>>>>> I'm still not keen on this use of iio_device elements just to act
>as
>>>>> glue between triggers. I think we need to work out a more light
>weight
>>>>> way to do this. As you are only using them for validation and to
>provide
>>>>> somewhere to hang the control attibutes off, there is nothing
>stopping us
>>>>> moving that over to the iio_trigger instead which would avoid the
>messy
>>>>> duality going on here.
>>>>
>>>> I have add an iio_device because each hardware can generate
>multiple
>>>> triggers (up to 5: trgo, ch 1...4) and slave_mode attribute will
>impact all the
>>>> triggers of a device. For me it was making sense to centralize that
>in an
>>>> iio_device rather than having an attribute "shared" (from hardware
>>>> point of view)
>>>> on multiple triggers.
>>>> Since master_mode attribute is only used by trgo and not impact
>ch1...4
>>>> triggers I will move it to trigger instead of the iio_device.
>>>>
>>>> I also wanted to be able to connect triggers on a iio_device as I
>>>> could do for an
>>>> ADC with a command like 'echo "tim1_trgo" >
>iio_deviceX/trigger/current_trigger'
>>> This is interesting, but with a bit of refactoring I would think it
>would
>>> be possible to share some of that code thus allowing non IIO devices
>to
>>> bind to triggers. Ultimately I want to be able to bind a trigger to
>>> a trigger - I appreciate here the topology is more limited than that
>>> so some complexity comes in.
>>>
>>> My gut feeling is that representing that topology explicitly is hard
>>> to do in a remotely general way, but lets try it and see.
>>> We run into this sort of interdependency issue between different
>bits of
>>> the hardware all the time. Setting a value somewhere effects the
>configuration
>>> elsewhere - often the best plan is to just let that happen and leave
>it up to
>>> userspace to check for changes if it cares.
>>
>> okay
>>
>>>> If I change that to parent_trigger attribute it change this
>behavior
>>>> and I will have to
>>>> duplicated what is done in iio_trigger_write_current() to find and
>>>> validate triggers.
>>> I get the reasoning, but we still end up with something represented
>>> by an IIO device that isn't providing any channels at all. It's
>simply
>>> using some of the infrastructure. To my mind it is 'something else'
>>> and should be represented as such. I have no problem at all with
>>> you registering additional elements in /sysfs/bus/iio/ to represent
>>> these shared elements - we already have drivers that do that to
>>> provide some centralized infrastructure (e.g. the sysfs-trigger)
>>
>> My hardware block are timers maybe I can add a channel type
>"IIO_TIMER"
>> and declare a channel with info_mask_separate =
>BIT(IIO_CHAN_INFO_SAMP_FREQ)
>> so I will be able to write/read sampling frequency on IIO device.
>>
>
>If it isn't possible to implement IIO_TIMER I will simply drop device
>part of
>my driver until we find a solution because I would like to upstream at
>least what is
>need to ADC/DAC.
Agreed. Let's move forward with the easy stuff.
I might get a chance to look at trigger trees later
this week depending on how some tedious time in a pharma clean room and on planes goes.
I think we can mock most of this up with software triggers and the dummy driver.
Perhaps a sysfs trigger driving a timer trigger will get us moving in the right direction API wise.
Might let us figure out something generic without needing hardware.
A few other bits of hardware can do similar to yours but I don't think I have any in my pile of boards.
J
>
>>> I'm worried about the scope spread we get for an IIO device
>otherwise.
>>> They serve a well defined purpose at the moment, and that isn't what
>>> is happening here.
>>>
>>> So my gut feeling is we are better deliberately not representing the
>>> inter dependence and claiming all triggers we are creating are
>>> independent. That way we can have a nice generic infrastructure
>>> that will work in all cases (be it pushing the sanity checking to
>>> userspace).
>>>
>>> So each trigger has direct access to what controls it. Changing
>anything
>>> can effect other triggers in weird ways.
>>>
>>> I'm finding it hard to see anything else generalizing sufficiently
>>> as we'll always get cases where we can't represent the topology
>without
>>> diving into the complexity of something like the media controller
>>> framework.
>>>
>>> Jonathan
>>>>
>>>>> I might still be missing something though!
>>>>>
>>>>> You would only I think need 3 attributes
>>>>>
>>>>> parrent_trigger
>>>>> and something like your master_mode and slave_mode attributes.
>>>>>
>>>>> The parrent_trigger would need some validation etc, but if we keep
>it
>>>>> within this driver initially that won't be hard to do. Checking
>the device
>>>>> parent matches will do most of it.
>>>>>
>>>>> Jonathan
>>>>>>
>>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...com>
>>>>>> ---
>>>>>> .../ABI/testing/sysfs-bus-iio-timer-stm32 | 55 +++
>>>>>> drivers/iio/Kconfig | 2 +-
>>>>>> drivers/iio/Makefile | 1 +
>>>>>> drivers/iio/timer/Kconfig | 13 +
>>>>>> drivers/iio/timer/Makefile | 1 +
>>>>>> drivers/iio/timer/stm32-timer-trigger.c | 466
>+++++++++++++++++++++
>>>>>> drivers/iio/trigger/Kconfig | 1 -
>>>>>> include/linux/iio/timer/stm32-timer-trigger.h | 62 +++
>>>>>> 8 files changed, 599 insertions(+), 2 deletions(-)
>>>>>> create mode 100644
>Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
>>>>>> create mode 100644 drivers/iio/timer/Kconfig
>>>>>> create mode 100644 drivers/iio/timer/Makefile
>>>>>> create mode 100644 drivers/iio/timer/stm32-timer-trigger.c
>>>>>> create mode 100644 include/linux/iio/timer/stm32-timer-trigger.h
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
>b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
>>>>>> new file mode 100644
>>>>>> index 0000000..26583dd
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
>>>>>> @@ -0,0 +1,55 @@
>>>>>> +What:
>/sys/bus/iio/devices/iio:deviceX/master_mode_available
>>>>>> +KernelVersion: 4.10
>>>>>> +Contact: benjamin.gaignard@...com
>>>>>> +Description:
>>>>>> + Reading returns the list possible master modes
>which are:
>>>>>> + - "reset" : The UG bit from the TIMx_EGR
>register is used as trigger output (TRGO).
>>>>>> + - "enable" : The Counter Enable signal CNT_EN is
>used as trigger output.
>>>>>> + - "update" : The update event is selected as
>trigger output.
>>>>>> + For instance a master timer can
>then be used as a prescaler for a slave timer.
>>>>>> + - "compare_pulse" : The trigger output send a
>positive pulse when the CC1IF flag is to be set.
>>>>>> + - "OC1REF" : OC1REF signal is used as trigger
>output.
>>>>>> + - "OC2REF" : OC2REF signal is used as trigger
>output.
>>>>>> + - "OC3REF" : OC3REF signal is used as trigger
>output.
>>>>>> + - "OC4REF" : OC4REF signal is used as trigger
>output.
>>>>>> +
>>>>>> +What:
>/sys/bus/iio/devices/iio:deviceX/master_mode
>>>>>> +KernelVersion: 4.10
>>>>>> +Contact: benjamin.gaignard@...com
>>>>>> +Description:
>>>>>> + Reading returns the current master modes.
>>>>>> + Writing set the master mode
>>>>>> +
>>>>>> +What:
>/sys/bus/iio/devices/iio:deviceX/slave_mode_available
>>>>>> +KernelVersion: 4.10
>>>>>> +Contact: benjamin.gaignard@...com
>>>>>> +Description:
>>>>>> + Reading returns the list possible slave modes which
>are:
>>>>>> + - "disabled" : The prescaler is clocked directly
>by the internal clock.
>>>>>> + - "encoder_1" : Counter counts up/down on TI2FP1
>edge depending on TI1FP2 level.
>>>>>> + - "encoder_2" : Counter counts up/down on TI1FP2
>edge depending on TI2FP1 level.
>>>>>> + - "encoder_3" : Counter counts up/down on both
>TI1FP1 and TI2FP2 edges depending
>>>>>> + on the level of the other input.
>>>>>> + - "reset" : Rising edge of the selected trigger
>input reinitializes the counter
>>>>>> + and generates an update of the
>registers.
>>>>>> + - "gated" : The counter clock is enabled when
>the trigger input is high.
>>>>>> + The counter stops (but is not
>reset) as soon as the trigger becomes low.
>>>>>> + Both start and stop of the counter
>are controlled.
>>>>>> + - "trigger" : The counter starts at a rising edge
>of the trigger TRGI (but it is not
>>>>>> + reset). Only the start of the
>counter is controlled.
>>>>>> + - "external_clock": Rising edges of the selected
>trigger (TRGI) clock the counter.
>>>>>> +
>>>>>> +What: /sys/bus/iio/devices/iio:deviceX/slave_mode
>>>>>> +KernelVersion: 4.10
>>>>>> +Contact: benjamin.gaignard@...com
>>>>>> +Description:
>>>>>> + Reading returns the current slave mode.
>>>>>> + Writing set the slave mode
>>>>>> +
>>>>>> +What:
>/sys/bus/iio/devices/triggerX/sampling_frequency
>>>>>> +KernelVersion: 4.10
>>>>>> +Contact: benjamin.gaignard@...com
>>>>>> +Description:
>>>>>> + Reading returns the current sampling frequency.
>>>>>> + Writing an value different of 0 set and start
>sampling.
>>>>>> + Writing 0 stop sampling.
>>>>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>>>>> index 6743b18..2de2a80 100644
>>>>>> --- a/drivers/iio/Kconfig
>>>>>> +++ b/drivers/iio/Kconfig
>>>>>> @@ -90,5 +90,5 @@ source "drivers/iio/potentiometer/Kconfig"
>>>>>> source "drivers/iio/pressure/Kconfig"
>>>>>> source "drivers/iio/proximity/Kconfig"
>>>>>> source "drivers/iio/temperature/Kconfig"
>>>>>> -
>>>>>> +source "drivers/iio/timer/Kconfig"
>>>>>> endif # IIO
>>>>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>>>>> index 87e4c43..b797c08 100644
>>>>>> --- a/drivers/iio/Makefile
>>>>>> +++ b/drivers/iio/Makefile
>>>>>> @@ -32,4 +32,5 @@ obj-y += potentiometer/
>>>>>> obj-y += pressure/
>>>>>> obj-y += proximity/
>>>>>> obj-y += temperature/
>>>>>> +obj-y += timer/
>>>>>> obj-y += trigger/
>>>>>> diff --git a/drivers/iio/timer/Kconfig
>b/drivers/iio/timer/Kconfig
>>>>>> new file mode 100644
>>>>>> index 0000000..e3c21f2
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/iio/timer/Kconfig
>>>>>> @@ -0,0 +1,13 @@
>>>>>> +#
>>>>>> +# Timers drivers
>>>>>> +
>>>>>> +menu "Timers"
>>>>>> +
>>>>>> +config IIO_STM32_TIMER_TRIGGER
>>>>>> + tristate "STM32 Timer Trigger"
>>>>>> + depends on (ARCH_STM32 && OF && MFD_STM32_TIMERS) ||
>COMPILE_TEST
>>>>>> + select IIO_TRIGGERED_EVENT
>>>>>> + help
>>>>>> + Select this option to enable STM32 Timer Trigger
>>>>>> +
>>>>>> +endmenu
>>>>>> diff --git a/drivers/iio/timer/Makefile
>b/drivers/iio/timer/Makefile
>>>>>> new file mode 100644
>>>>>> index 0000000..4ad95ec9
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/iio/timer/Makefile
>>>>>> @@ -0,0 +1 @@
>>>>>> +obj-$(CONFIG_IIO_STM32_TIMER_TRIGGER) += stm32-timer-trigger.o
>>>>>> diff --git a/drivers/iio/timer/stm32-timer-trigger.c
>b/drivers/iio/timer/stm32-timer-trigger.c
>>>>>> new file mode 100644
>>>>>> index 0000000..8d16e8f
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/iio/timer/stm32-timer-trigger.c
>>>>>> @@ -0,0 +1,466 @@
>>>>>> +/*
>>>>>> + * Copyright (C) STMicroelectronics 2016
>>>>>> + *
>>>>>> + * Author: Benjamin Gaignard <benjamin.gaignard@...com>
>>>>>> + *
>>>>>> + * License terms: GNU General Public License (GPL), version 2
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/iio/iio.h>
>>>>>> +#include <linux/iio/sysfs.h>
>>>>>> +#include <linux/iio/timer/stm32-timer-trigger.h>
>>>>>> +#include <linux/iio/trigger.h>
>>>>>> +#include <linux/iio/triggered_event.h>
>>>>>> +#include <linux/interrupt.h>
>>>>>> +#include <linux/mfd/stm32-timers.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +
>>>>>> +#define MAX_TRIGGERS 6
>>>>>> +#define MAX_VALIDS 5
>>>>>> +
>>>>>> +/* List the triggers created by each timer */
>>>>>> +static const void *triggers_table[][MAX_TRIGGERS] = {
>>>>>> + { TIM1_TRGO, TIM1_CH1, TIM1_CH2, TIM1_CH3, TIM1_CH4,},
>>>>>> + { TIM2_TRGO, TIM2_CH1, TIM2_CH2, TIM2_CH3, TIM2_CH4,},
>>>>>> + { TIM3_TRGO, TIM3_CH1, TIM3_CH2, TIM3_CH3, TIM3_CH4,},
>>>>>> + { TIM4_TRGO, TIM4_CH1, TIM4_CH2, TIM4_CH3, TIM4_CH4,},
>>>>>> + { TIM5_TRGO, TIM5_CH1, TIM5_CH2, TIM5_CH3, TIM5_CH4,},
>>>>>> + { TIM6_TRGO,},
>>>>>> + { TIM7_TRGO,},
>>>>>> + { TIM8_TRGO, TIM8_CH1, TIM8_CH2, TIM8_CH3, TIM8_CH4,},
>>>>>> + { TIM9_TRGO, TIM9_CH1, TIM9_CH2,},
>>>>>> + { TIM12_TRGO, TIM12_CH1, TIM12_CH2,},
>>>>>> +};
>>>>>> +
>>>>>> +/* List the triggers accepted by each timer */
>>>>>> +static const void *valids_table[][MAX_VALIDS] = {
>>>>>> + { TIM5_TRGO, TIM2_TRGO, TIM4_TRGO, TIM3_TRGO,},
>>>>>> + { TIM1_TRGO, TIM8_TRGO, TIM3_TRGO, TIM4_TRGO,},
>>>>>> + { TIM1_TRGO, TIM8_TRGO, TIM5_TRGO, TIM4_TRGO,},
>>>>>> + { TIM1_TRGO, TIM2_TRGO, TIM3_TRGO, TIM8_TRGO,},
>>>>>> + { TIM2_TRGO, TIM3_TRGO, TIM4_TRGO, TIM8_TRGO,},
>>>>>> + { }, /* timer 6 */
>>>>>> + { }, /* timer 7 */
>>>>>> + { TIM1_TRGO, TIM2_TRGO, TIM4_TRGO, TIM5_TRGO,},
>>>>>> + { TIM2_TRGO, TIM3_TRGO,},
>>>>>> + { TIM4_TRGO, TIM5_TRGO,},
>>>>>> +};
>>>>>> +
>>>>>> +struct stm32_timer_trigger {
>>>>>> + struct device *dev;
>>>>>> + struct regmap *regmap;
>>>>>> + struct clk *clk;
>>>>>> + u32 max_arr;
>>>>>> + const void *triggers;
>>>>>> + const void *valids;
>>>>>> +};
>>>>>> +
>>>>>> +static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>>>>> + unsigned int frequency)
>>>>>> +{
>>>>>> + unsigned long long prd, div;
>>>>>> + int prescaler = 0;
>>>>>> + u32 ccer, cr1;
>>>>>> +
>>>>>> + /* Period and prescaler values depends of clock rate */
>>>>>> + div = (unsigned long long)clk_get_rate(priv->clk);
>>>>>> +
>>>>>> + do_div(div, frequency);
>>>>>> +
>>>>>> + prd = div;
>>>>>> +
>>>>>> + /*
>>>>>> + * Increase prescaler value until we get a result that fit
>>>>>> + * with auto reload register maximum value.
>>>>>> + */
>>>>>> + while (div > priv->max_arr) {
>>>>>> + prescaler++;
>>>>>> + div = prd;
>>>>>> + do_div(div, (prescaler + 1));
>>>>>> + }
>>>>>> + prd = div;
>>>>>> +
>>>>>> + if (prescaler > MAX_TIM_PSC) {
>>>>>> + dev_err(priv->dev, "prescaler exceeds the maximum
>value\n");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + /* Check if nobody else use the timer */
>>>>>> + regmap_read(priv->regmap, TIM_CCER, &ccer);
>>>>>> + if (ccer & TIM_CCER_CCXE)
>>>>>> + return -EBUSY;
>>>>>> +
>>>>>> + regmap_read(priv->regmap, TIM_CR1, &cr1);
>>>>>> + if (!(cr1 & TIM_CR1_CEN))
>>>>>> + clk_enable(priv->clk);
>>>>>> +
>>>>>> + regmap_write(priv->regmap, TIM_PSC, prescaler);
>>>>>> + regmap_write(priv->regmap, TIM_ARR, prd - 1);
>>>>>> + regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE,
>TIM_CR1_ARPE);
>>>>>> +
>>>>>> + /* Force master mode to update mode */
>>>>>> + regmap_update_bits(priv->regmap, TIM_CR2, TIM_CR2_MMS,
>0x20);
>>>>>> +
>>>>>> + /* Make sure that registers are updated */
>>>>>> + regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG,
>TIM_EGR_UG);
>>>>>> +
>>>>>> + /* Enable controller */
>>>>>> + regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>TIM_CR1_CEN);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>>>>>> +{
>>>>>> + u32 ccer, cr1;
>>>>>> +
>>>>>> + regmap_read(priv->regmap, TIM_CCER, &ccer);
>>>>>> + if (ccer & TIM_CCER_CCXE)
>>>>>> + return;
>>>>>> +
>>>>>> + regmap_read(priv->regmap, TIM_CR1, &cr1);
>>>>>> + if (cr1 & TIM_CR1_CEN)
>>>>>> + clk_disable(priv->clk);
>>>>>> +
>>>>>> + /* Stop timer */
>>>>>> + regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>>>>>> + regmap_write(priv->regmap, TIM_PSC, 0);
>>>>>> + regmap_write(priv->regmap, TIM_ARR, 0);
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t stm32_tt_store_frequency(struct device *dev,
>>>>>> + struct device_attribute
>*attr,
>>>>>> + const char *buf, size_t
>len)
>>>>>> +{
>>>>>> + struct iio_trigger *trig = to_iio_trigger(dev);
>>>>>> + struct stm32_timer_trigger *priv =
>iio_trigger_get_drvdata(trig);
>>>>>> + unsigned int freq;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = kstrtouint(buf, 10, &freq);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + if (freq == 0) {
>>>>>> + stm32_timer_stop(priv);
>>>>>> + } else {
>>>>>> + ret = stm32_timer_start(priv, freq);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + return len;
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t stm32_tt_read_frequency(struct device *dev,
>>>>>> + struct device_attribute
>*attr, char *buf)
>>>>>> +{
>>>>>> + struct iio_trigger *trig = to_iio_trigger(dev);
>>>>>> + struct stm32_timer_trigger *priv =
>iio_trigger_get_drvdata(trig);
>>>>>> + u32 psc, arr, cr1;
>>>>>> + unsigned long long freq = 0;
>>>>>> +
>>>>>> + regmap_read(priv->regmap, TIM_CR1, &cr1);
>>>>>> + regmap_read(priv->regmap, TIM_PSC, &psc);
>>>>>> + regmap_read(priv->regmap, TIM_ARR, &arr);
>>>>>> +
>>>>>> + if (psc && arr && (cr1 & TIM_CR1_CEN)) {
>>>>>> + freq = (unsigned long long)clk_get_rate(priv->clk);
>>>>>> + do_div(freq, psc);
>>>>>> + do_div(freq, arr);
>>>>>> + }
>>>>>> +
>>>>>> + return sprintf(buf, "%d\n", (unsigned int)freq);
>>>>>> +}
>>>>>> +
>>>>>> +static IIO_DEV_ATTR_SAMP_FREQ(0660,
>>>>>> + stm32_tt_read_frequency,
>>>>>> + stm32_tt_store_frequency);
>>>>>> +
>>>>>> +static struct attribute *stm32_trigger_attrs[] = {
>>>>>> + &iio_dev_attr_sampling_frequency.dev_attr.attr,
>>>>>> + NULL,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct attribute_group stm32_trigger_attr_group = {
>>>>>> + .attrs = stm32_trigger_attrs,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct attribute_group *stm32_trigger_attr_groups[]
>= {
>>>>>> + &stm32_trigger_attr_group,
>>>>>> + NULL,
>>>>>> +};
>>>>>> +
>>>>>> +static char *master_mode_table[] = {
>>>>>> + "reset",
>>>>>> + "enable",
>>>>>> + "update",
>>>>>> + "compare_pulse",
>>>>>> + "OC1REF",
>>>>>> + "OC2REF",
>>>>>> + "OC3REF",
>>>>>> + "OC4REF"
>>>>>> +};
>>>>>> +
>>>>>> +static ssize_t stm32_tt_show_master_mode(struct device *dev,
>>>>>> + struct device_attribute
>*attr,
>>>>>> + char *buf)
>>>>>> +{
>>>>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>>>> + struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>>>>> + u32 cr2;
>>>>>> +
>>>>>> + regmap_read(priv->regmap, TIM_CR2, &cr2);
>>>>>> + cr2 = (cr2 & TIM_CR2_MMS) >> TIM_CR2_MMS_SHIFT;
>>>>>> +
>>>>>> + return snprintf(buf, PAGE_SIZE, "%s\n",
>master_mode_table[cr2]);
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t stm32_tt_store_master_mode(struct device *dev,
>>>>>> + struct device_attribute
>*attr,
>>>>>> + const char *buf, size_t
>len)
>>>>>> +{
>>>>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>>>> + struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < ARRAY_SIZE(master_mode_table); i++) {
>>>>>> + if (!strncmp(master_mode_table[i], buf,
>>>>>> + strlen(master_mode_table[i]))) {
>>>>>> + regmap_update_bits(priv->regmap, TIM_CR2,
>>>>>> + TIM_CR2_MMS, i <<
>TIM_CR2_MMS_SHIFT);
>>>>>> + return len;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + return -EINVAL;
>>>>>> +}
>>>>>> +
>>>>>> +static IIO_CONST_ATTR(master_mode_available,
>>>>>> + "reset enable update compare_pulse OC1REF OC2REF OC3REF
>OC4REF");
>>>>>> +
>>>>>> +static IIO_DEVICE_ATTR(master_mode, 0660,
>>>>>> + stm32_tt_show_master_mode,
>>>>>> + stm32_tt_store_master_mode,
>>>>>> + 0);
>>>>>> +
>>>>>> +static char *slave_mode_table[] = {
>>>>>> + "disabled",
>>>>>> + "encoder_1",
>>>>>> + "encoder_2",
>>>>>> + "encoder_3",
>>>>>> + "reset",
>>>>>> + "gated",
>>>>>> + "trigger",
>>>>>> + "external_clock",
>>>>>> +};
>>>>>> +
>>>>>> +static ssize_t stm32_tt_show_slave_mode(struct device *dev,
>>>>>> + struct device_attribute
>*attr,
>>>>>> + char *buf)
>>>>>> +{
>>>>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>>>> + struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>>>>> + u32 smcr;
>>>>>> +
>>>>>> + regmap_read(priv->regmap, TIM_SMCR, &smcr);
>>>>>> + smcr &= TIM_SMCR_SMS;
>>>>>> +
>>>>>> + return snprintf(buf, PAGE_SIZE, "%s\n",
>slave_mode_table[smcr]);
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t stm32_tt_store_slave_mode(struct device *dev,
>>>>>> + struct device_attribute
>*attr,
>>>>>> + const char *buf, size_t
>len)
>>>>>> +{
>>>>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>>>> + struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < ARRAY_SIZE(slave_mode_table); i++) {
>>>>>> + if (!strncmp(slave_mode_table[i], buf,
>>>>>> + strlen(slave_mode_table[i]))) {
>>>>>> + regmap_update_bits(priv->regmap,
>>>>>> + TIM_SMCR, TIM_SMCR_SMS,
>i);
>>>>>> + return len;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + return -EINVAL;
>>>>>> +}
>>>>>> +
>>>>>> +static IIO_CONST_ATTR(slave_mode_available,
>>>>>> +"disabled encoder_1 encoder_2 encoder_3 reset gated trigger
>external_clock");
>>>>>> +
>>>>>> +static IIO_DEVICE_ATTR(slave_mode, 0660,
>>>>>> + stm32_tt_show_slave_mode,
>>>>>> + stm32_tt_store_slave_mode,
>>>>>> + 0);
>>>>>> +
>>>>>> +static struct attribute *stm32_timer_attrs[] = {
>>>>>> + &iio_dev_attr_master_mode.dev_attr.attr,
>>>>>> + &iio_const_attr_master_mode_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_slave_mode.dev_attr.attr,
>>>>>> + &iio_const_attr_slave_mode_available.dev_attr.attr,
>>>>>> + NULL,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct attribute_group stm32_timer_attr_group = {
>>>>>> + .attrs = stm32_timer_attrs,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct iio_trigger_ops timer_trigger_ops = {
>>>>>> + .owner = THIS_MODULE,
>>>>>> +};
>>>>>> +
>>>>>> +static int stm32_setup_iio_triggers(struct stm32_timer_trigger
>*priv)
>>>>>> +{
>>>>>> + int ret;
>>>>>> + const char * const *cur = priv->triggers;
>>>>>> +
>>>>>> + while (cur && *cur) {
>>>>>> + struct iio_trigger *trig;
>>>>>> +
>>>>>> + trig = devm_iio_trigger_alloc(priv->dev, "%s",
>*cur);
>>>>>> + if (!trig)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + trig->dev.parent = priv->dev->parent;
>>>>>> + trig->ops = &timer_trigger_ops;
>>>>>> + trig->dev.groups = stm32_trigger_attr_groups;
>>>>>> + iio_trigger_set_drvdata(trig, priv);
>>>>>> +
>>>>>> + ret = devm_iio_trigger_register(priv->dev, trig);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + cur++;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * is_stm32_timer_trigger
>>>>>> + * @trig: trigger to be checked
>>>>>> + *
>>>>>> + * return true if the trigger is a valid stm32 iio timer trigger
>>>>>> + * either return false
>>>>>> + */
>>>>>> +bool is_stm32_timer_trigger(struct iio_trigger *trig)
>>>>>> +{
>>>>>> + return (trig->ops == &timer_trigger_ops);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(is_stm32_timer_trigger);
>>>>>> +
>>>>>> +static int stm32_validate_trigger(struct iio_dev *indio_dev,
>>>>>> + struct iio_trigger *trig)
>>>>>> +{
>>>>>> + struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>>>>> + const char * const *cur = priv->valids;
>>>>>> + unsigned int i = 0;
>>>>>> +
>>>>>> + if (!is_stm32_timer_trigger(trig))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + while (cur && *cur) {
>>>>>> + if (!strncmp(trig->name, *cur, strlen(trig->name)))
>{
>>>>>> + regmap_update_bits(priv->regmap,
>>>>>> + TIM_SMCR, TIM_SMCR_TS,
>>>>>> + i << TIM_SMCR_TS_SHIFT);
>>>>>> + return 0;
>>>>>> + }
>>>>>> + cur++;
>>>>>> + i++;
>>>>>> + }
>>>>>> +
>>>>>> + return -EINVAL;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct iio_info stm32_trigger_info = {
>>>>>> + .driver_module = THIS_MODULE,
>>>>>> + .validate_trigger = stm32_validate_trigger,
>>>>>> + .attrs = &stm32_timer_attr_group,
>>>>>> +};
>>>>>> +
>>>>>> +static struct stm32_timer_trigger *stm32_setup_iio_device(struct
>device *dev)
>>>>>> +{
>>>>>> + struct iio_dev *indio_dev;
>>>>>> + int ret;
>>>>>> +
>>>>>> + indio_dev = devm_iio_device_alloc(dev,
>>>>>> + sizeof(struct
>stm32_timer_trigger));
>>>>>> + if (!indio_dev)
>>>>>> + return NULL;
>>>>>> +
>>>>>> + indio_dev->name = dev_name(dev);
>>>>>> + indio_dev->dev.parent = dev;
>>>>>> + indio_dev->info = &stm32_trigger_info;
>>>>>> + indio_dev->modes = INDIO_EVENT_TRIGGERED;
>>>>>> + indio_dev->num_channels = 0;
>>>>>> + indio_dev->dev.of_node = dev->of_node;
>>>>>> +
>>>>>> + ret = devm_iio_device_register(dev, indio_dev);
>>>>>> + if (ret)
>>>>>> + return NULL;
>>>>>> +
>>>>>> + return iio_priv(indio_dev);
>>>>>> +}
>>>>>> +
>>>>>> +static int stm32_timer_trigger_probe(struct platform_device
>*pdev)
>>>>>> +{
>>>>>> + struct device *dev = &pdev->dev;
>>>>>> + struct stm32_timer_trigger *priv;
>>>>>> + struct stm32_timers *ddata =
>dev_get_drvdata(pdev->dev.parent);
>>>>>> + unsigned int index;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (of_property_read_u32(dev->of_node, "reg", &index))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + if (index >= ARRAY_SIZE(triggers_table))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + /* Create an IIO device only if we have triggers to be
>validated */
>>>>>> + if (*valids_table[index])
>>>>>> + priv = stm32_setup_iio_device(dev);
>>>>>
>>>>> I still don't like this. Really feels like we shouldn't be
>creating an
>>>>> iio device with all the bagage that carries just to allow us to do
>the
>>>>> trigger trees. We ought to have a much more light weight solution
>for this
>>>>> functionality - we aren't typically even using the interrupt tree
>stuff
>>>>> that the triggers for devices are all really about.
>>>>>
>>>>> A simpler approach of allowing each trigger the option of a parent
>seems like
>>>>> it would be cleaner. Could be done entirely within this driver in
>the first
>>>>> instance. Basically it would just look like your master and slave
>attributes
>>>>> but have those under triggerX not iio:deviceX.
>>>>>
>>>>> We can work out how to make it more generic later - including
>perhaps the
>>>>> option to trigger from triggers outside this driver, using some
>parallel
>>>>> infrastructure to the device triggering.
>>>>>
>>>>>
>>>>>> + else
>>>>>> + priv = devm_kzalloc(dev, sizeof(*priv),
>GFP_KERNEL);
>>>>>> +
>>>>>> + if (!priv)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + priv->dev = dev;
>>>>>> + priv->regmap = ddata->regmap;
>>>>>> + priv->clk = ddata->clk;
>>>>>> + priv->max_arr = ddata->max_arr;
>>>>>> + priv->triggers = triggers_table[index];
>>>>>> + priv->valids = valids_table[index];
>>>>>> +
>>>>>> + ret = stm32_setup_iio_triggers(priv);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + platform_set_drvdata(pdev, priv);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct of_device_id stm32_trig_of_match[] = {
>>>>>> + { .compatible = "st,stm32-timer-trigger", },
>>>>>> + { /* end node */ },
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
>>>>>> +
>>>>>> +static struct platform_driver stm32_timer_trigger_driver = {
>>>>>> + .probe = stm32_timer_trigger_probe,
>>>>>> + .driver = {
>>>>>> + .name = "stm32-timer-trigger",
>>>>>> + .of_match_table = stm32_trig_of_match,
>>>>>> + },
>>>>>> +};
>>>>>> +module_platform_driver(stm32_timer_trigger_driver);
>>>>>> +
>>>>>> +MODULE_ALIAS("platform: stm32-timer-trigger");
>>>>>> +MODULE_DESCRIPTION("STMicroelectronics STM32 Timer Trigger
>driver");
>>>>>> +MODULE_LICENSE("GPL v2");
>>>>>> diff --git a/drivers/iio/trigger/Kconfig
>b/drivers/iio/trigger/Kconfig
>>>>>> index 809b2e7..f2af4fe 100644
>>>>>> --- a/drivers/iio/trigger/Kconfig
>>>>>> +++ b/drivers/iio/trigger/Kconfig
>>>>>> @@ -46,5 +46,4 @@ config IIO_SYSFS_TRIGGER
>>>>>>
>>>>>> To compile this driver as a module, choose M here: the
>>>>>> module will be called iio-trig-sysfs.
>>>>>> -
>>>>> Clean this up.
>>>>
>>>> ok
>>>>
>>>>>> endmenu
>>>>>> diff --git a/include/linux/iio/timer/stm32-timer-trigger.h
>b/include/linux/iio/timer/stm32-timer-trigger.h
>>>>>> new file mode 100644
>>>>>> index 0000000..55535ae
>>>>>> --- /dev/null
>>>>>> +++ b/include/linux/iio/timer/stm32-timer-trigger.h
>>>>>> @@ -0,0 +1,62 @@
>>>>>> +/*
>>>>>> + * Copyright (C) STMicroelectronics 2016
>>>>>> + *
>>>>>> + * Author: Benjamin Gaignard <benjamin.gaignard@...com>
>>>>>> + *
>>>>>> + * License terms: GNU General Public License (GPL), version 2
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef _STM32_TIMER_TRIGGER_H_
>>>>>> +#define _STM32_TIMER_TRIGGER_H_
>>>>>> +
>>>>>> +#define TIM1_TRGO "tim1_trgo"
>>>>>> +#define TIM1_CH1 "tim1_ch1"
>>>>>> +#define TIM1_CH2 "tim1_ch2"
>>>>>> +#define TIM1_CH3 "tim1_ch3"
>>>>>> +#define TIM1_CH4 "tim1_ch4"
>>>>>> +
>>>>>> +#define TIM2_TRGO "tim2_trgo"
>>>>>> +#define TIM2_CH1 "tim2_ch1"
>>>>>> +#define TIM2_CH2 "tim2_ch2"
>>>>>> +#define TIM2_CH3 "tim2_ch3"
>>>>>> +#define TIM2_CH4 "tim2_ch4"
>>>>>> +
>>>>>> +#define TIM3_TRGO "tim3_trgo"
>>>>>> +#define TIM3_CH1 "tim3_ch1"
>>>>>> +#define TIM3_CH2 "tim3_ch2"
>>>>>> +#define TIM3_CH3 "tim3_ch3"
>>>>>> +#define TIM3_CH4 "tim3_ch4"
>>>>>> +
>>>>>> +#define TIM4_TRGO "tim4_trgo"
>>>>>> +#define TIM4_CH1 "tim4_ch1"
>>>>>> +#define TIM4_CH2 "tim4_ch2"
>>>>>> +#define TIM4_CH3 "tim4_ch3"
>>>>>> +#define TIM4_CH4 "tim4_ch4"
>>>>>> +
>>>>>> +#define TIM5_TRGO "tim5_trgo"
>>>>>> +#define TIM5_CH1 "tim5_ch1"
>>>>>> +#define TIM5_CH2 "tim5_ch2"
>>>>>> +#define TIM5_CH3 "tim5_ch3"
>>>>>> +#define TIM5_CH4 "tim5_ch4"
>>>>>> +
>>>>>> +#define TIM6_TRGO "tim6_trgo"
>>>>>> +
>>>>>> +#define TIM7_TRGO "tim7_trgo"
>>>>>> +
>>>>>> +#define TIM8_TRGO "tim8_trgo"
>>>>>> +#define TIM8_CH1 "tim8_ch1"
>>>>>> +#define TIM8_CH2 "tim8_ch2"
>>>>>> +#define TIM8_CH3 "tim8_ch3"
>>>>>> +#define TIM8_CH4 "tim8_ch4"
>>>>>> +
>>>>>> +#define TIM9_TRGO "tim9_trgo"
>>>>>> +#define TIM9_CH1 "tim9_ch1"
>>>>>> +#define TIM9_CH2 "tim9_ch2"
>>>>>> +
>>>>>> +#define TIM12_TRGO "tim12_trgo"
>>>>>> +#define TIM12_CH1 "tim12_ch1"
>>>>>> +#define TIM12_CH2 "tim12_ch2"
>>>>>> +
>>>>>> +bool is_stm32_timer_trigger(struct iio_trigger *trig);
>>>>>> +
>>>>>> +#endif
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>> --
>> Benjamin Gaignard
>>
>> Graphic Study Group
>>
>> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro: Facebook | Twitter | Blog
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Powered by blists - more mailing lists