Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1504861 > unrolled thread

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector Lars-Peter Clausen <lars@metafoo.de> - 2016-10-20 15:00 +0200
    Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector Lars-Peter Clausen <lars@metafoo.de> - 2016-10-20 17:30 +0200
      Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector Jonathan Cameron <jic23@kernel.org> - 2016-10-22 16:30 +0200
    Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2016-10-20 19:40 +0200
      Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector jic23@kernel.org - 2016-10-21 09:20 +0200
        Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector Jonathan Cameron <jic23@kernel.org> - 2016-10-22 16:30 +0200
    Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2016-10-20 19:40 +0200
    Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector Jonathan Cameron <jic23@kernel.org> - 2016-10-22 16:40 +0200

#1504861 — Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

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.

> 
> 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.

[toc] | [next] | [standalone]

#1505032

On 10/20/2016 04:53 PM, Peter Rosin wrote:
[...]
> Good idea! Then the "envelope-detector,inverted" bool can go, and be
> implied by the compatible string. If some way to rebind the irq trigger
> is later discovered that can be added as a channel attr without
> deprecating any dt bindings stuff. While at it, the other properties
> ("envelope-detector,dac-max" and "envelope-detector,comp-interval-ms")
> could also be implied from the compatible string. Would that be better?
> I think so.
> 
> But, the compatible string is one thing and the driver name is another.
> "axentia,tse850-envelope-detector" doesn't seem like the best of driver
> names...

The driver name is not that important we can still change that later if we
have to, the DT compatible string on the other hand is fixed.

> 
> Are there any existing examples of drivers for (generic) things built
> with discrete components like this that could perhaps provide guidance?

Not that I'm aware of.

[toc] | [prev] | [next] | [standalone]

#1506535

On 20/10/16 16:29, Lars-Peter Clausen wrote:
> On 10/20/2016 04:53 PM, Peter Rosin wrote:
> [...]
>> Good idea! Then the "envelope-detector,inverted" bool can go, and be
>> implied by the compatible string. If some way to rebind the irq trigger
>> is later discovered that can be added as a channel attr without
>> deprecating any dt bindings stuff. While at it, the other properties
>> ("envelope-detector,dac-max" and "envelope-detector,comp-interval-ms")
>> could also be implied from the compatible string. Would that be better?
>> I think so.
>>
>> But, the compatible string is one thing and the driver name is another.
>> "axentia,tse850-envelope-detector" doesn't seem like the best of driver
>> names...
> 
> The driver name is not that important we can still change that later if we
> have to, the DT compatible string on the other hand is fixed.
> 
>>
>> Are there any existing examples of drivers for (generic) things built
>> with discrete components like this that could perhaps provide guidance?
> 
> Not that I'm aware of.
> 
Me neither.  Always interesting to break new ground ;)

J

[toc] | [prev] | [next] | [standalone]

#1505131

On 20 October 2016 18:30:19 BST, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
>
>
>On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars@metafoo.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.
http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...
>
>>
>>> 
>>> 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.

[toc] | [prev] | [next] | [standalone]

#1505519

On 20.10.2016 19:17, Peter Rosin wrote:
> On 2016-10-20 19:37, Jonathan Cameron wrote:
>> On 20 October 2016 18:30:19 BST, Jonathan Cameron 
>> <jic23@jic23.retrosnub.co.uk> wrote:
>>> On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars@metafoo.de> 
>>> wrote:
>>>> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>>>> 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.
>> http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...
> 
> Interesting, one issue with that is that it is all in real world
> units, while I'd rather have the raw value.
Um.. It's been a while, but the principle was (IIRC) that every
_available would match the units fo the associated info mask element.
Thus if you have a _raw element it would be in adc counts (most likely).

_input would be in relevant real world units, scale etc in the whatever
units the value itself is in.

> So, I would need to
> convert back to the raw value using the scale, which sounds boring
> but doable. However, I wonder if calibration may also be involved
> with that conversion back to raw for some channels? That sounds a
> bit more driver specific and potentially troublesome...
I've not had a chance to look at your code (only picked up on this as
there was a fair length thread developing), but I wouldn't have thought 
we'd
need to deal with calibrations.   Might need them to move to real world
units from raw but that's always the case anyway (unfortunately).

Jonathan
> 
> Cheers,
> Peter
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1506536

On 21/10/16 23:58, Peter Rosin wrote:
> On 2016-10-21 09:17, jic23@kernel.org wrote:
>> On 20.10.2016 19:17, Peter Rosin wrote:
>>> On 2016-10-20 19:37, Jonathan Cameron wrote:
>>>> On 20 October 2016 18:30:19 BST, Jonathan Cameron 
>>>> <jic23@jic23.retrosnub.co.uk> wrote:
>>>>> On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars@metafoo.de> 
>>>>> wrote:
>>>>>> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>>>>>> 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.
>>>> http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...
>>>
>>> Interesting, one issue with that is that it is all in real world
>>> units, while I'd rather have the raw value.
>> Um.. It's been a while, but the principle was (IIRC) that every
>> _available would match the units fo the associated info mask element.
>> Thus if you have a _raw element it would be in adc counts (most likely).
>>
>> _input would be in relevant real world units, scale etc in the whatever
>> units the value itself is in.
> 
> Ok, so I forward ported that patch and added code so that the relevant
> channels provide what is available. I also added code to turn the
> rest of the parameter style devicetree properties into iio device/channel
> attributes. So, it is now much neater from a bindings point of view.
> 
> Before I post the updated patches, I'm wondering what the status is
> on that ancient patch? It didn't forward port without issues, but there
> were no real difficulties that I noticed. Should I just start off my v2
> series with that patch? I tend to think that that's the best option,
> because I suspect that adding a "max-ohms" devicetree property as a
> stop-gap pending some new infrastructure is pretty unrealistic...
> 
> Basically, my question is if that ancient patch as any chance of living
> at all in a form close to what it is, or if should start looking for
> an alternative right away?
The stoppers (IIRC) were that at the time we had a lot of drivers not
making full use off the info_mask stuff so there were a whole load
of precursor patches.  A lot of those have been done since, so we are
probably much more ready for it.

The controversial bit was the question of how to describe ranges, but
I don't think that got all that much attention back then.

If you are happy to take on looking after that series I'd certainly
be very happy!   3 years kind of implies I'm not going to get to
it particularly soon myself :(

Will be interesting to see what reviews we get of it when you post it
though.   Perhaps we deliberately push it into drivers only slowly
initially so that if we decide it was a horrible mistake (or that we
need to make changes to the ABI) we only end up supporting obsolete
ABI in a few drivers...

So a slowly but surely one perhaps rather than mass adoption.

Jonathan
> 
> Cheers,
> Peter
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1505133

On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars@metafoo.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.

[toc] | [prev] | [next] | [standalone]

#1506539

On 20/10/16 15:53, Peter Rosin wrote:
> On 2016-10-20 14:55, Lars-Peter Clausen wrote:
>> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>> 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.
> 
> For the dpot I have, real_bits (if provided) would not be too great since
> the maximum value is 256 (i.e. 257 possible wiper positions). I doesn't
> feel like I'm the most qualified person to add these new min/max attributes
> though, as I'm not familiar with most parts of the iio code. I'll happily
> jump on board if they are somehow magically available, of course :-)
> 
>>> 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?
> 
> Right, I didn't express myself all that clearly, I do in fact already
> enable the regulator in ->probe and disable it in ->remove. Anything
> else?
Nope.  This is the same thing we do with ADCs that take a reference voltage.
Sometimes we even query the voltage ever time or handle notifiers that
tell use when it changes... (only example I can immediately think of for
that is the sht15 driver in hwmon - my fault a long time ago ;)
> 
>>> 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'.
> 
> Good idea! Then the "envelope-detector,inverted" bool can go, and be
> implied by the compatible string. If some way to rebind the irq trigger
> is later discovered that can be added as a channel attr without
> deprecating any dt bindings stuff. While at it, the other properties
> ("envelope-detector,dac-max" and "envelope-detector,comp-interval-ms")
> could also be implied from the compatible string. Would that be better?
> I think so.
> 
> But, the compatible string is one thing and the driver name is another.
> "axentia,tse850-envelope-detector" doesn't seem like the best of driver
> names...
> 
> Are there any existing examples of drivers for (generic) things built
> with discrete components like this that could perhaps provide guidance?
> 
>>> 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.
> 
> Thanks, appreciated!
> 
> Cheers,
> Peter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web