Re: [PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source

From: Rob Herring <robh+dt@kernel.org>
To: Jonathan Cameron <jic23@kernel.org>
Cc: "Alexandru Ardelean" <alexandru.ardelean@analog.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"open list:IIO SUBSYSTEM AND DRIVERS" <linux-iio@vger.kernel.org>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Michael Hennerich" <Michael.Hennerich@analog.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Dragos Bogdan" <dragos.bogdan@analog.com>,
	"Mircea Caprioru" <mircea.caprioru@analog.com>,
	"Mihail Chindris" <Mihail.Chindris@analog.com>
Subject: Re: [PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source
Date: Thu, 18 Feb 2021 08:35:27 -0600	[thread overview]
Message-ID: <CAL_Jsq+_HYQ1KT7L34nuM2RENnX1A5J8LCazCV2aeNFmkeskGw@mail.gmail.com> (raw)
In-Reply-To: <20210218140506.02b28d8a@archlinux>

On Thu, Feb 18, 2021 at 8:05 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 17 Feb 2021 10:34:38 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > From: Mircea Caprioru <mircea.caprioru@analog.com>
> >
> > A PWM signal will be used as a trigger source to have a deterministic
> > sampling frequency since this family of DAC has no hardware interrupt
> > source.
> >
> > This feature is made optional however, as there are some board setups where
> > this isn't used.
> >
>
> So this is taking a very generic setup, but then implementing it
> as a bit of a hack within the driver.
>
> It's effectively a PWM connected up to an instance
> of iio/triggers/iio-trig-interrupt.c
>
> Now, I've not looked at that trigger driver for a while, so you may well
> need to figure out how to add a binding to instantiate it.
> (looks like no one has used it since board file days, or via instantiation
> from another driver).
>
> It's a slightly odd corner case as what it reflects is that we have
> an interrupt available that is intended to drive some sort of data
> capture or output (it's a trigger signal) - but exactly what is done
> is a runtime configurable.  In this particular case that interrupt
> is hooked up to a PWM and we also want to represent that.
>
> The fact it's being driven via a PWM is interesting but we should be
> able to extend that trigger driver to optionally accept a pwm provider
> and if it has one provide frequency control.
>
> Binding might look something like the following..
>
> interrupt-trigger {
>    interrupts = <>;
>    pwms = <&pwm 0 4000 PWM_POLARITY_INVERTED>;
> };
>
> @Rob, what do you think of this odd beast?

So a PWM routed back to a GPIO interrupt? It needs a compatible, but
otherwise I wouldn't object to the binding if that's what the h/w
looks like. But from an OS perspective, I don't think you need it.

> So all in all, this generic facility needs a generic implementation, not
> one buried in a driver.
>
> Another open question here is whether you really can't just use an hrtimer
> to get similar precision?  Way back at the dawn of time in IIO we had
> code to use the RTC periodic ticks as a trigger with the theory that they
> would give very precise and even timing.  In the end it turned out that
> hrtimers worked just as well (and RTCs drivers emulated the periodic
> ticks via hrtimers, dropping their use of the hardware periodic timers).

+100

A hrtimer is likely going to be more precise. IIRC, timers are
serviced first. Either way, you're going to have some amount of
interrupt service latency, so any precision you think you are gaining
by 'doing it in h/w' isn't really there.

Rob