2017-02-19 16:53 GMT+01:00 Jonathan Cameron <jic23@xxxxxxxxxx>:
> Hi All,
>
> Would be really helpful to get some other input on this.
> It's fiddly to put it lightly but if we get it right I think
> the interface will be useful in all sorts of common cases.
>
> On 16/02/17 14:23, Benjamin Gaignard wrote:
>> Add validate_trigger function in iio_trigger_ops and
>> dev_attr_parent_trigger into trigger attribute group to be able
>> to accept triggers as parents.
>>
>> Because the hardware have 8 different ways to use parent levels and
>> edges, this patch introduce "slave_mode" sysfs attribute for stm32
>> triggers. Modes usages are described in sysfs-bus-iio-timer-stm32
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxx>
> Hi Benjamin,
>
> I'm increasingly convinced we need to be careful with the ABI
> on this to end up with something generic. It's useful stuff, but
> this particular hardware fuses various concepts based on them
> being on the same wire taking no noticed of whether the hardware
> upstream constrains what makes sense.
>
> Rarely are those concepts independent of
> what is actually wired to that signal so on a real system
> it is a lot less flexible than the hardware on it's own might
> imply.
>
> This is really giving me a headache on a Sunday afternoon.
> I don't have a good answer (yet). I'd like to completely
> separate the concepts but it is not obvious how to do it.
Maybe it give you a headache because it is going in wrong way...
I just discover that an quadratic encoder driver exist in IIO (104-quad-8).
>From my point of view it does exactly the same than my hardware:
- there are channels to read/write counter value and set/get preset value.
- counting modes are exposed using iio_enum
- counter direction could read from a channel
The main differences are the number and definition of counting modes.
Implementing my driver in this way is even better since it will allow to get/set
the counter and that was missing in parent trigger approach.
To summarize I could:
- use the current code (iio trigger) set a sampling frequency for ADC/DAC
- add quadratic encoder like (iio device) with counting mode, count direction,
quadrature mode and counter value channels.
That would really simplify the problem !
> I appreciate that what I'm asking will make this driver more
> complex, but I think we need to reflect accurately what the signals
> are in order to not leave userspace with access to modes that make
> absolutely no sense for a given hardware setup.
>
> This is a bit rambling, but hopefully following through will
> make sense...
>>
>> version 2:
>> - use dev_attr_parent_trigger
>> - improve slave modes documentation
>> ---
>> .../ABI/testing/sysfs-bus-iio-timer-stm32 | 43 +++++++++
>> drivers/iio/trigger/stm32-timer-trigger.c | 105 +++++++++++++++++++++
>> 2 files changed, 148 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
>> index 6534a60..7d667f9 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
>> @@ -27,3 +27,46 @@ Description:
>> Reading returns the current sampling frequency.
>> Writing an value different of 0 set and start sampling.
>> Writing 0 stop sampling.
>> +
>> +What: /sys/bus/iio/devices/triggerX/slave_mode_available
> I think we need to drive this towards a generic description, whether that's easy or not.
> This needs to be clear and extensible.
>> +KernelVersion: 4.12
>> +Contact: benjamin.gaignard@xxxxxx
>> +Description:
>> + Reading returns the list possible slave modes which are:
>> + - "disabled" : Parent trigger levels or edges have do not impact on trigger.
>> + Trigger is clocked by the internal clock.
>> + This is the default mode.
> Don't bother with this first one. The way to disable a parent trigger is to not have
> one assigned.
>
> Remember a trigger won't have any effect on anything until we have a buffer
> actually using it so it doesn't matter what mode we come up in initially.
> Let the default be anything as that will be easier for a generic interface.
>
>> + - "encoder_1" : Trigger internal counter counts up/down on channel 2 edge depending on channel 1 level.
>> + - "encoder_2" : Trigger internal counter counts up/down on channel 1 edge depending on channel 2 level.
>> + - "encoder_3" : Trigger internal counter counts up/down on channels 1 & 2 edge
>> + depending on channel 1 & 2 level.
> Conceptually these are clocks that are getting divided down. So the
> child trigger has to have some concept of a divisor being applied.
> We can then describe these, but it needs to be a truly generic
> fashion which will be tricky. In a sense, I'd expect these to be
> properties of the parent trigger rather than how it is being used
> by the child.
>
> Hmm. Not sure on this so would like other opinions.
> The concept of triggers having two channels is somewhat of a stretch.
>
> To my mind, whether the inputs are connected to an encoder and what type
> it is should probably be a device tree property. You wouldn't typically
> pretend for some channels that you have an encoder and reset on other
> channels. So I think a trigger at the top level should be either
> an encoder or not and it should know from DT what type it is.
>
> This may be a pain to implement, but I think we need to do so.
>
>> + - "reset" : Rising edge on parent trigger reinitializes the trigger and generates
>> + an update of auto-reload, prescaler and repetition counter registers.
>> + - "gated" : The trigger is enabled when the parent trigger input is high.
>> + The trigger stops (but is not reset) as soon as the parent trigger becomes low.
> These two are our classic cases on a 'counting' trigger but in this case it is fed by another clock.
> It think we need to separate that relationship.
>
> A trigger that is a clock divider (fires every N clock cycles) can have:
>
> 1) A clock source, be it the default one / external or one of the encoder types
> (though a given input should only be able to provide one of them as that
> is pretty much a wiring question rather than policy decision).
>
> 2) A parent trigger which can drive reset, gating (ignore clock) or starting
>
> I have no issue with these both being controllable from userspace, but
> I can't see a generic interface where they are both via a single
> simple attribute.
>
>
>> + - "trigger" : The trigger starts at a rising edge of the parent trigger (but it is not reset).
>> + - "external_clock": Rising edges of the parent trigger clock the trigger.
>
> Note that in IIO ABI it is absolutely acceptable to have any attribute
> change the value of any other (hardware dependencies are way to complex
> to represent explicitly in some cases - like this one).
>
> So I think we might need quite a few attrs to make this work.
>
> parent-trigger
> - as it is, but only handling the ones that are effecting the 'state' of the trigger counter, not it's rate.
>
> parent-clock
> - allow this to also take a trigger but will be used in only the clock modes. If set to null we are on
> the internal clock. Will be cleared if a parent-trigger is set as it can't be both.
> I'm not sure this will generalise well as we might feed of things that aren't trigger.
>
> parent-clock-interpretation
> - encoder, normal (at a push, maybe the encoder types but with meaningful names reflecting their wiring)
> (I'd actually much prefer these to be facets of the parent trigger - it only makes sense if
> there is an encoder there or not - they also need not be configured from userspace)
>
> parent-trigger-interpretation
> - reset, gated, start
>
>
> We could flatten this perhaps by broadening the parent-trigger concept to explicitly allow it to be a clock.
>
> parent-trigger:
> parent-trigger-interpretation:
> - reset_counter, gated, start_counter, as_clock (only gated is really generic, in that
> it could apply to any trigger including those that aren't periodic counters)
>
> parent-trigger-clock-type
> - encoder, normal - though I think these really ought to be part of the parent trigger itself.
> As I've said, it makes no sense to be an encoder if there isn't encoder hardware on the wires.
>
> I think I'm favouring the last option as long as the clock type in those modes is coming from
> DT or similar for the parent trigger. To my mind it has nothing really to do with the child
> trigger.
>
>
> Anyhow, everyone please take a look at this!. It's a fairly big chunk of ABI that we will
> probably want to use more widely.
>
> Certainly applying a sysfs or interrrupt trigger (on a gpio) as a parent to a high resolution
> timer trigger and using in startcounter or resetcounter modes seems to me a very useful
> tool to have more generally. Even gated might work well for osciloscope type triggered
> captures.
>
> If we generalize the hrt slightly to be on for a period then we can do something like.
>
> gpio based interrupt trigger ---resetcounter--> hrttimer (on after a time, off sometime later
> --- gated ---> hrttimer at high frequency
>
> Which is relatively elegant and uses simple elements.
>
>> + Encoder modes are used to automatically handle the counting direction of the internal counter.
>> + Those modes are typically used for high-accuracy rotor position sensing in electrical motors
>> + or for digital potentiometers. From the two outputs of a quadrature encoder sensor the timer
>> + extracts a clock on each and every active edge and adjusts the counting direction depending on
>> + the relative phase-shift between the two incomings signals. The timer counter thus directly
>> + holds the angular position of the motor or the potentionmeter.
> Not without a reset or index mark being built in, it doesn't. Relative angular position. I've not come
> across a pot that works this way (any links?)
>> +
>> + For "reset", "gated" and "trigger" modes the trigger will fire N+1 times when internal counter
>> + will reach the value of auto-reload register. N is defined by the value of repetition counter.
> That makes these child triggers really periodic timers, just with weird clock inputs.
>> + Those modes could allow parent trigger to control when sampling frequency of the current trigger
>> + start or stop.
>> + Since PWM and trigger features are mixed in the same hardware block those 3 modes could be used
>> + to synchronize PWMs start while PWM sysfs API is used to set period and duty cycle.
>> +
>> + In "external clock" mode parent trigger can control the current trigger clock (and so the sampling
>> + frequency) for example to correct jittering.
>> +
>> +What: /sys/bus/iio/devices/triggerX/slave_mode
>> +KernelVersion: 4.12
>> +Contact: benjamin.gaignard@xxxxxx
>> +Description:
>> + Reading returns the current slave mode.
>> + Writing set the slave mode
>> diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
>> index 994b96d..a4f1061 100644
>> --- a/drivers/iio/trigger/stm32-timer-trigger.c
>> +++ b/drivers/iio/trigger/stm32-timer-trigger.c
>> @@ -15,6 +15,7 @@
>> #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] = {
>> @@ -32,12 +33,29 @@
>> { 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,},
>> + { }, /* timer 10 */
>> + { }, /* timer 11 */
>> + { 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,
>> @@ -221,10 +239,66 @@ static IIO_DEVICE_ATTR(master_mode, 0660,
>> 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_trigger_attrs[] = {
>> &iio_dev_attr_sampling_frequency.dev_attr.attr,
>> &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,
>> + &dev_attr_parent_trigger.attr,
>> NULL,
>> };
>>
>> @@ -237,8 +311,38 @@ static IIO_DEVICE_ATTR(master_mode, 0660,
>> NULL,
>> };
>>
>> +static int stm32_validate_trigger(struct iio_trigger *trig,
>> + struct iio_trigger *parent)
>> +{
>> + struct stm32_timer_trigger *priv = iio_trigger_get_drvdata(trig);
>> + const char * const *cur = priv->valids;
>> + unsigned int i = 0;
>> +
>> + if (!parent) {
>> + regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_TS, 0);
>> + return 0;
>> + }
>> +
>> + if (!is_stm32_timer_trigger(parent))
>> + return -EINVAL;
>> +
>> + while (cur && *cur) {
>> + if (!strncmp(parent->name, *cur, strlen(parent->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_trigger_ops timer_trigger_ops = {
>> .owner = THIS_MODULE,
>> + .validate_trigger = stm32_validate_trigger,
>> };
>>
>> static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
>> @@ -312,6 +416,7 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>> 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)
>>
>
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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
