Message-ID: <8B9238AE-D53C-4A22-84CF-EC42FDA2DFB2@jic23.retrosnub.co.uk>
Date:   Thu, 20 Oct 2016 18:30:19 +0100
From:   Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To:     Lars-Peter Clausen <lars@...afoo.de>,
        Peter Rosin <peda@...ntia.se>, linux-kernel@...r.kernel.org
CC:     Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector



On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars@...afoo.de> wrote:
>On 10/20/2016 11:25 AM, Peter Rosin wrote:
>> Hi!
>> 
>> These two drivers share the fact that they wrap another iio channel,
>> and I use the first in combination with the second, which is why I'm
>> submitting them as a pair.
>> 
>> The first driver is a simple wrapper converting an iio dpot into an
>> iio dac. It only changes the unit and scale. It also does not add any
>> fancy iio buffer support that I don't need. I suppose that can be
>> added. By someone else :-)
>> 
>> Please look over the scale conversion, notably for the fractional
>log2
>> case that I don't need myself, so is untested. Maybe I should just
>> remove it?
>> 
>> Also, is there some agreed-upon way to dig out the maximum value from
>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from the
>> dt bindings, which would have been nice...
>
>Yes, this is something we could really use. In a sense it exists for
>the
>devices with buffer-capable channels where there is the real_bits field
>which tells us the data width of the channel. But a dedicated mechanism
>for
>querying the maximum (and minimum) valid code seems like a useful
>feature.
>Not only for in-kernel clients, but also for userspace.
This was something that was addressed by the rather ancient patch series i posted that added 
an available call back which provided info on range and values for all info mask elements.
Series got buried by there being a lot of precursors but quite a few of those have merged since.

Hmm Google won't let me find it on my phone. Was a while back now. Will try to get on pc with
 decent email archive later and dig out a reference.

>
>> 
>> I'm also wondering if I'm somehow abusing the regulator? I only added
>> it to get rid of a "dpot-dac,max-voltage" thing from the dt bindings.
>> It feels right though, but maybe I should do more with it than check
>> its voltage? What?
>
>Enable the regulator when it is in use?
>
>> 
>> The second driver (the envelope detector) is more involved. It also
>> explains why I need the dpot-dac driver. I wanted the envelope
>> detector to be generic and work with any dac, but I had a dpot...
>> 
>> The envelope detector was previously discussed late last year [1],
>> and this is what I came up with instead of that mess.
>> 
>> There are a couple of things to be said about the envelope detector,
>> one question is where it should live? I placed it in the adc
>directory,
>> but maybe it deserves an iio directory of its own? I'm also a bit
>> worried that the name is a wee bit too generic. But what is a good
>> name? I don't want it to be too long like dac-comp-envelope-detector
>> and something like dac-comp-env-det is just unreadable. Naming is
>> difficult... And suggestions?
>
>Yeah, it is a bit tricky. It is a envelope detector built from discrete
>components, but of course there are many more ways to build one. If you
>have
>a codename for your platform you could use this for the DT compatible
>string, like 'vendor,foobar-envelope-detector'.
>
>> 
>> Another thing is that I'm not 100% satisfied with the fact that you
>> have to decide at instantiation if you are going to invert the search
>> or not (search from below). But in order for that to be selectable
>> at runtime with a channel attribute of some sort, I need to be able
>> to rebind the interrupt to the other edge and I want to do that
>> without releasing the irq and grabbing it again (someone might
>> otherwise steal the irq, making the driver lose the irq all
>together).
>> I don't see any API to change the irq trigger condition. Is there
>> such a thing?
>> 
>> Anyway, despite all the above questions and remarks, this works for
>> me. Please consider applying.
>
>In general this series looks really good, good and clear implementation
>as
>well as documentation. A few minor bits here and there, but that is
>normal.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Powered by blists - more mailing lists