Re: [PATCH v2 2/3] iio: adc: at91-sama5d2_adc: add hw trigger and buffer support

[Eugen Hristev]

On 17.05.2017 10:47, Peter Rosin wrote:
> On 2017-05-16 20:03, Jonathan Cameron wrote:
> > <snip> As we are only left with one area of questions.
> > > > > > > > +static const struct at91_adc_trigger at91_adc_trigger_list[] = {
> > > > > > > > +    {
> > > > > > > > +        .name = "external-rising",
> > > > > > > > +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_RISE,
> > > > > > > > +    },
> > > > > > > > +    {
> > > > > > > > +        .name = "external-falling",
> > > > > > > > +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL,
> > > > > > > > +    },
> > > > > > > > +    {
> > > > > > > > +        .name = "external-any",
> > > > > > > > +        .trgmod_value = AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY,
> > > > > > > > +    },
> > > > > > > Hmm. Should this be a userspace configurable option?  Feels rather like
> > > > > > > it is an element of the hardware - reflecting the characteristics of
> > > > > > > some
> > > > > > > hardware device sat on the pin.
> > > > > > The user can choose from sysfs which trigger
> > > > > > is best suited for the use case, since all
> > > > > > three triggers are provided and can be connected to the buffer.
> > > > > > It reflects more the triggering capability of the ADC rather than
> > > > > > any different hardware device sitting on the pin
> > > > >
> > > > > I am also in favour of a userspace configurable option. For sure it's
> > > > > hardware related but on our board we only provide a trigger pin, we
> > > > > don't know which hardware the customer will put on this pin.
> > > > hmm. OK I'm persuaded I think.
> > > >
> > > > Could do this with devicetree overlays or similar.
> > > >
> > > > So follow up question is whether this is the right interface.
> > > > Can all 3 run at once sensibly?  If not are we not looking at a control
> > > > parameter for a single trigger?
> > >
> > > There is a single trigger hardware pin, but can work in one of the three
> > > modes. Do you suggest I should change it to a single trigger, but then
> > > we need some kind of sysfs entry to control it ?
> > >
> > > Or perhaps change the trigger to be exclusive to the device/buffer, in
> > > which case just one trigger can be used anyway with the current_trigger
> > > option in buffer.
> > >
> > > If somehow more than one trigger would be enabled in the same time,
> > > only the last enabled one will work, since I write the corresponding
> > > trigger edge configuration value in the ADC register. So in fact these three triggers are mutually exclusive, as enabling one disables the
> > > other, however, it's not transparent to the user.
> > Ah that definitely suggests to me it should be a sysfs attribute associated
> > with the trigger rather than 3 separate triggers.  The interpretation
> > of those triggers would require userspace to have some knowledge of what
> > is going on, so I don't think we have any problems by requiring it instead
> > to know about a sysfs attribute.
> > > These kind of different edge triggers used to be in device tree in
> > > older at91 driver.
> > > I have given it thought and decided to let just the driver know about
> > > them. Since they are bound to the ADC IP block and not related to any
> > > hardware description of the board or the special connectivity that
> > > the driver might be interested about. I mean the driver knows the
> > > trigger works this way because it's part of the block, and device tree
> > > cannot change this, so the portability of the driver is not affected
> > > and related to SoC design only.
> > Sort of (I think..)  As I understand it we are still talking about an
> > external pin?  A possible usecase for this sort of thing would be
> > combining with an external sequencer with an analog mux.  In that
> > case only one of the options will make any sense. (this is the
> > hardware equivalent of what Peter Rosin's mux subsystem puts
> > under software control)
The ADTRG is a pin directly connected to the SoC. Since we have
multiple analog channels as input, the mux would be useful if the use
case would imply many more inputs, and then some overload on userspace
to differentiate between the muxed values since the conversion will
happen on the same channel...
The ADTRG will simply tell the SoC when to do the conversion, with
respect to the charging delay which is a few clock cycles. So the
ADTRG is a digital input and depending on which signal we connect,
and which type of edge we want to catch, we can configure it
via the three triggers (or change to a single parametrized trigger).
The ADC block inside the SoC can also be connected to the PWM signal,
and that trigger is yet to be implemented in the driver (I will do it
in a future patch). So the ADTRG can be connected to an external PWM,
or some kind of signal that is already resemblant with which kind of
conversion speed or intervals between conversions that we want to have
on the analog channels.
> > We might be in one of those interesting cases where a devicetree
> > binding should exist for the fixed case, but with the flexibility
> > to allow userspace to specify it if and only if it is not specified
> > in the device tree.  So if the devicetree in some sense describes
> > downstream hardware it is fixed as appropriate.  If it doesn't and
> > we are looking at an 'edge of known world' situation then we let
> > userspace have control?  Does that make sense?
I considered that the ADTRG can work in the three possible ways, so
depending on the need, the application in userspace can decide how to
configure it.
Having it in devicetree doesn't make much difference for
the driver. Instead of automagically knowing that ADTRG has three
different edge configurations, it will read the device tree and in the
end, the configuration should reach userspace one way or the other.
So the devicetree is cleaner and not bloated with extra unneeded
information that the driver should already know.
One other option would be to have some kind of default edge type in
device tree and the driver will work with that. However, changing the
edge type would require a restart, and limits the functionality.
Another option is to have the driver use only one type of edge, which
again limits the functionality.
So that's why I thought to let userspace configure the edge type,
and not put it in devicetree since we can't have other types. (unless
someone might have a strange reason to make unavailable some of the
possible edge configurations)
> > I don't suppose we have any idea what this is actually used for
> > on real world boards?
> >
> > I've pulled in Peter, Mark and Rob as CCs to see if they want to
> > comment.
>
> From what I can tell, this touches the mux code [1] for cases where
> you have (some hypothetical) hardware of this style:
>
>     .----.
>     |a5d2|
>     |    |
>     |GPO0|----------.
>     |GPO1|--------. |
>     |    |        | |
>     |    |     .-------.
>     |    |     |dg4052a|
>     |    |     |       |
>     | ADC|-----|X    X0|---- signal X0
>     |TRGR|--.  |     X1|---- signal X1
>     |    |  |  |     X2|---- signal X2
>     |    |  |  |     X3|---- signal X3
>     |    |  |  |       |
>     |    |  '--|Y    Y0|---- trigger Y0
>     |    |     |     Y1|---- trigger Y1
>     '----'     |     Y2|---- trigger Y2
>                |     Y3|---- trigger Y3
>                '-------'
>
> Where signal X0 (when it's selected but the mux) should be sampled as
> trigger Y0 is rising and signal X1 (when it's selected) should be
> sampled for both edges of trigger Y1. Or something like that...
In this case one would need to enable the rising edge trigger,
do the conversion for X0, then disable and enable the both-edge
trigger, and read conversion for Y1. Or, one can use the both-edge
trigger all the time and just ignore the conversion on the falling
edge for X0. (All this considering the rise-fall time is less than
conversion time of course)
> The iio-mux (patch 7/13 [2]) of course needs to be extended to handle
> triggers and buffers before there is any chance the above will work. The
> iio-mux currently knows nothing about buffers/triggers (I'm also a novice
> in that area and don't have any current need for it).
>
> But, for the iio-mux to possibly handle the above, it needs some way to
> switch the trigger (in inkern.c?) when the mux state is changing. And
> it then becomes obvious that there needs to be some mechanism other than
> device-tree to set the trigger style in the at91 adc driver.
For the ADC IP block, the trigger changing is just a matter of a
write to a register. We could have something like a callback for the
trigger ops that could do that in theory. The driver could provide
something like a trigger_style enum to the subsystem and the callback
could pass back one of the possible configurations. Then the trigger
could auto-adjust to the new type of edge needed, without disabling
and reenabling the trigger.
> I expect that similar issues are present in other adc drivers that
> support triggers, so I don't know if any of this needs to affect the path
> taken with the at91 driver at this time?
At this time we want to implement the support for the hardware trigger
pin that we have on the SoC, and the corresponding buffer management,
and in the future to support more of the hardware features for this
block.

Regards,
Eugen
> Cheers,
> peda
>
> [1] https://lkml.org/lkml/2017/5/14/160
> [2] https://lkml.org/lkml/2017/5/14/163