Re: [PATCH RESEND] x86, intel_mid: ADC management

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/10/2012 06:58 PM, Mark Brown wrote:
> On Tue, Apr 10, 2012 at 05:56:32PM +0100, Alan Cox wrote:
>>>> We can't go around blocking entire platforms because of the
>>>> IIO blob. I raised this point with the whole previous
>>>> *generation* of Intel SoC devices about IIO and nothing has
>>>> been done about it.
> 
>>> Including by Intel, of course.
> 
>> Given the responses I got to moving it out of staging were of the
>> "not yet" and "no" variety it hardly got a request to be worked
>> upon.
> 
> The "not yet" bit of it certainly seems like something that could
> be worked on, presumably "not yet" included a "we need to do X
> first" element.
> 
Entirely possibly I gave my usual evasive set of a few things that
were top of the pile to knock over!  I've been promising a 'state of
iio email' for a while now. Sorry about that.

I was anxious to not move anything out of staging that we 'knew' needed
a major refactoring.  There is one more such refactoring going through
revisions at the moment. (Push in kernel interfaces to allow for
interrupt driven users such as a client iio driver that pushes data to
input).  Last attempt to move out of staging unfortunately fell foul of
some issues Greg KH raised in a review (correctly!)  These were with the
interfaces for requesting in kernel access to IIO device channels and we
really needed to get that right so I pulled back from sending a pull
request to Linus.  Since then time has been tight on my side. Some
others have been very helpful in picking up the slack.

Anyhow to repeat my whats left comments.

1)  Review of code.  This is crucial. If people have a little time
ripping holes in the core IIO code is what we need.  Arnd did a good job
of this a while back. Others have done bits of it since.

2) Getting the push code tidied up and pushed out.  I'll post it as an
updated rfc to linux-iio shortly.  All I had left that definitely
wanted doing here was cleaning up the example iio to input bridge
driver.  That can happen later.

Other bits that are less than ideal.

* Event passing to consumers else where in the kernel. Right now an
input driver can readings from a sensor, but there is no way of
requesting threshold interrupts.

* Interaction between consumer drivers (e.g. hwmon or input) where some
are requesting data by polling when they want it and others want a
stream of data driven by interrupts. I can give details of one
approach to this to anyone interested but it's basically a simply one
element buffer and some changes to channel requesting (entirely in the
IIO core
- - external drivers will just see requests that would previously be
refused start working).

>>> That's not where the rest of the embedded community has been
>>> coming from on this stuff and from a deployment point of view
>>> staging isn't really that big a blocker to users
> 
>> Non-staging code cannot depend upon staging code. That is the
>> rule GregKH laid down. The Intel drivers involved are established
>> non staging drivers and the gpadc layer is basically cleaning up
>> the fact they all do this themselves in private right now without
>> any central co-ordination or abstraction.
> 
> So, that's a bit different and not at all obvious from your e-mail
> - the diffstat shows only the code you're adding, not the code
> you've factored out of the existing mainline drivers.  The diffstat
> you posted was:
> 
> | arch/x86/include/asm/intel_mid_gpadc.h   |   13 + |
> arch/x86/platform/mrst/Makefile          |    1 |
> arch/x86/platform/mrst/intel_mid_gpadc.c |  645
> ++++++++++++++++++++++++++++++ | arch/x86/platform/mrst/mrst.c
> |    6 | 4 files changed, 665 insertions(+), 0 deletions(-)
> 
> which is a pure addition of code and I'm not seeing anything in
> the changelog about this either.
> 
>>> Frankly at this point I don't understand why we can't just lift
>>> IIO out of staging as-is, perhaps with the userspace ABI
>>> nobbled or moved into debugfs for the time being if that's
>>> still a concern.  Alternatively there is the option of you
>>> proposing some other generic framework.
> 
>> I've been saying this for over a year. It's still a huge blob of
>> code
> 
> I've CCed in Greg and Jonathan here - guys, what is happening with 
> getting IIO out of staging?  It's been there since 2009 and from
> an outside point of view it's really difficult to see progress
> here, the time taken to merge the subsystem seems really long.
Agreed :(

> 
> To me it really feels like the subsystem is pretty mature at this
> point and that if anything staging is blocking things rather than
> helping them, the subsystem feels like it's getting normal
> development rather than work to integrate it into the rest of the
> kernel and being in staging is discouraging users who don't
> absolutely need to use it.
> 
> For example, when we get extcon (or whatever it ends up being
> called) merged we'll either have to start writing a raft of auxadc
> specific drivers for it or we'll have to come up with *some* kind
> of subsystem to use to abstract out the standard DC measurement
> pattern.
> 
> If the code was moved out of staging today what would go wrong?
Churn in interfaces is probably about it.  Maybe a good use of any time
would be for people to take their non IIO drivers that they think might
fit (or data sheets!) and see whether there are things that they would
like to be different.

An initial move would just take (possibly with some file name tidying up!)

industrialio-core.c -> core.c
industrialio-trigger.c -> trigger.c
industrialio-event.c -> event.c
inkern.c -> inkern.c
kfifo-buf.c  -> buffer_kfifo.c

iio.h
buffer.h
driver.h
machine.h
consumer.h
trigger.h
trigger_consumer.h
types.h
iio_core.h
kfifo_buf.h
sysfs.h (stripped of a few legacy bits at the end that can go into a
sysfs-staging.h until the users are all cleaned up).

Some name stripping etc needed and a wholesale switch of headers.

Doing it this way would be a lot less painful than the build up approach
we suggested last time, but more controversial as this is large complex
code drop (to anyone who hasn't followed it in staging).

> 
>> that isn't needed for pure low level stuff, but that's fixable
>> and something which can eventually be worked upon.
> 
> We do have to be careful about this sort of "this is a little bit
> of low level code" thing - it (along with "our hardware is unique")
> comes up rather a lot and it's often missing a good chunk of the
> picture.
> 
>> You are trying to make the tail wag the dog. Staging is basically
>> out of
> 
> You keep saying "you" here.  To repeat once more, this is pretty
> much what the general embedded community has been pushing towards,
> this is not particularly my personal opinion.
> 
>> kernel. The x86 stuff is *in kernel*, this driver is the basis
>> of cleaning it up. We can sort out making it talk to IIO later.
>> Right now all you are doing is blocking a pile of much needed
>> code cleanups, and without them you *won't* have an API for the
>> gpadc on Intel, it'll be hardcoded in each driver still. That
>> will mean you have *zero* change of getting IIO support at all.
> 
> Like I said above you'd not posted the patches which factor the
> code out of the existing users, if that's what you're trying to do
> that's a bit different to what your patch looked like.
> 
>> Whether gpadc then adopts an IIO interface is a question to ask
>> later, after (if) IIO is ever merged. If the platform ever starts
>> using gpadc for non internal hardware stuff then I'm pretty sure
>> it'll end up adopting whatever generic GPADC layer eventually
>> emerges simply because we'll want to share drivers.
> 
> Again, this is the sort of thing that SoC vendors routinely say
> but don't end up doing quite so routinely as one would hope.  This
> is understandable as this sort of thing is more of a problem for
> people working in off-SoC devices and system integration than the
> SoC vendors themselves.

The 'fun' bit for most IIO developers is that we weren't originally
interested in SoCs at all.  The support we've added for in kernel
users of IIO has all come out of Mark and other's comments on how
it would be useful to them.  It's been interesting and there have
been considerable gains for our other usecases, but it has take time
to do.

Anyhow, if people have spare cycles, then as ever review time is what
we need, even better if people are willing to dive in and propose
fixes to stuff they don't like.

Now that was probably mostly irrelevant to the majority of the
the thread :)

Jonathan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPhIw4AAoJEFSFNJnE9BaIEmAP/2l/Ks11td0SGyU1uDz394V3
PuTPtzAxtGpHPApxafHnN7ci270N2YloSGefP3qZ5bcMwbj8XBOw4VQ8M2rWuSWa
xghMmc1IxfYa4BsazsvEEtlU1QZq6DTjrh51azXv6sgiJcFOttPCnIAxpJnnEQXG
EVb7guUnQhzzgNCAA11up2DHohvB4CGig/jx5d7JO2puhSw483+qEzY71bW0T6FA
NNXZF8zTsppcMg/YKYheCyJ2ihDUQXrhMUDc14Pz3UHiSx9q2sGrygOOnPNBoP5r
HTRe3sKCpEQrPjavX3f6l0BHGd4EW4UHwQ+ZELY2SYiXqcltleuw3EjGMQA9j/6z
p2opkGXMI2Zw7oI4C4XZ6ivpFQxUjpeAnNGhGHNDVQWUuXDsNmc/z6yYwW8CN1sn
BgAueu9hdoxQF7AoQyQ9cl6PC93aUvAkzf5TbNAfP+8Rsboo6xqxKAJGq2ddFkad
KA5Zpxe5VbalmNlMUeNmad6A1KSJe1XfPv+L1heAL+dE+w18pFUmaN8HYghDSOYV
IUqxGJ2VjEA+irYndYv+qjWfojpv8xrUc2WSJaYjclAWEm+CmTHTblzro5oDZE+C
tZjYGz1LobGSIO13hQqW3d1QrIDIt3Vx7Z6iROBSPbll44PaDPXV3DVZh8YiLn97
eLxK5n3x0jNJwVt8ACbO
=AbrL
-----END PGP SIGNATURE-----

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:

> 1)  Review of code.  This is crucial. If people have a little time
> ripping holes in the core IIO code is what we need.  Arnd did a good job
> of this a while back. Others have done bits of it since.

> 2) Getting the push code tidied up and pushed out.  I'll post it as an
> updated rfc to linux-iio shortly.  All I had left that definitely
> wanted doing here was cleaning up the example iio to input bridge
> driver.  That can happen later.

For these two can we refactor in place?  That's pretty much what seems
to have been happening anyway...

> * Event passing to consumers else where in the kernel. Right now an
> input driver can readings from a sensor, but there is no way of
> requesting threshold interrupts.

> * Interaction between consumer drivers (e.g. hwmon or input) where some
> are requesting data by polling when they want it and others want a

These sound like something that can be added incrementally?

> > If the code was moved out of staging today what would go wrong?

> Churn in interfaces is probably about it.  Maybe a good use of any time

I guess the big question is then if we can live with that.

> would be for people to take their non IIO drivers that they think might
> fit (or data sheets!) and see whether there are things that they would
> like to be different.

In tree there's a few auxadc and comparator drivers in drivers/mfd, plus
things like arch/arm/plat-samsung/adc.c in the arch direcories.  These
are all broadly similar to the at91 code that's been sent to IIO
already.  There's also the code Alan posted at the top of this thread.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

>On Tue, Apr 10,=
 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
>
>> 1)  Review of code.=
  This is crucial. If people have a little time
>> ripping holes in the cor=
e IIO code is what we need.  Arnd did a good
>job
>> of this a while back. =
Others have done bits of it since.
>
>> 2) Getting the push code tidied up =
and pushed out.  I'll post it as
>an
>> updated rfc to linux-iio shortly.  =
All I had left that definitely
>> wanted doing here was cleaning up the exa=
mple iio to input bridge
>> driver.  That can happen later.
>
>For these tw=
o can we refactor in place?  That's pretty much what seems
>to have been ha=
ppening anyway...
>
I guess it comes down to whether Linus will pull.  2 sh=
ould be there within a week or so anyway depending mostly on analog testing=
 I haven't broken any of their drivers.
>> * Event passing to consumers els=
e where in the kernel. Right now an
>> input driver can readings from a sen=
sor, but there is no way of
>> requesting threshold interrupts.
>
>> * Inte=
raction between consumer drivers (e.g. hwmon or input) where
>some
>> are r=
equesting data by polling when they want it and others want a
>
>These soun=
d like something that can be added incrementally?
>
>> > If the code was mo=
ved out of staging today what would go wrong?
>
>> Churn in interfaces is p=
robably about it.  Maybe a good use of any
>time
>
>I guess the big questio=
n is then if we can live with that.

Agreed.  
>> would be for people to ta=
ke their non IIO drivers that they think
>might
>> fit (or data sheets!) an=
d see whether there are things that they
>would
>> like to be different.
>
=
>In tree there's a few auxadc and comparator drivers in drivers/mfd,
>plus
=
>things like arch/arm/plat-samsung/adc.c in the arch direcories.  These
>ar=
e all broadly similar to the at91 code that's been sent to IIO
>already.  T=
here's also the code Alan posted at the top of this thread.
Quite a lot of =
things in miscellaneous as well to possibly pull in over time.



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

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

This is a fairly stupid place to reply but I don't have any way of 
replying to the original
patch whilst at work.  Looking quickly at it in a web browser, looks 
like we fundamentally
have a couple of interfaces exported. An alloc and free pair and two 
types of sample?
The second, during gsmpulse sample is some sort of triggered capture?

Mapping to what is now in IIO (and hasn't been for that long!).
We handle alloc and free via a mapping table that registered with the 
core by the
consumer driver (often coming from platform data).

Raw channel reads are available via polling or you can register a 
callback function
(that's the bit that is going through review at the moment) if you have 
a buffered
situation.  So the missing bit is that we don't have a 'request n 
samples' call.
We also don't currently have the functions to switch the triggering 
mechanism from
consumers (though they should be easy to add).

Anyhow, those are the differences and looks like IIO needs a few small 
tweaks to
meet the interface requirements if you did decide to go that way.  The 
only one
I'm not immediately sure how to handle is the 'read n samples' bit.  A 
self unregistering
version of the callback buffer might do it reasonably cleanly....
What you have may correspond to an arbitary scan.  The question is do 
you ever do
uneven reading of different channels?  e.g. AABAAB.

Jonathan
>
> Mark Brown<broonie@opensource.wolfsonmicro.com>  wrote:
>
>> On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
>>
>>> 1)  Review of code.  This is crucial. If people have a little time
>>> ripping holes in the core IIO code is what we need.  Arnd did a good
>> job
>>> of this a while back. Others have done bits of it since.
>>> 2) Getting the push code tidied up and pushed out.  I'll post it as
>> an
>>> updated rfc to linux-iio shortly.  All I had left that definitely
>>> wanted doing here was cleaning up the example iio to input bridge
>>> driver.  That can happen later.
>> For these two can we refactor in place?  That's pretty much what seems
>> to have been happening anyway...
>>
> I guess it comes down to whether Linus will pull.  2 should be there within a week or so anyway depending mostly on analog testing I haven't broken any of their drivers.
>>> * Event passing to consumers else where in the kernel. Right now an
>>> input driver can readings from a sensor, but there is no way of
>>> requesting threshold interrupts.
>>> * Interaction between consumer drivers (e.g. hwmon or input) where
>> some
>>> are requesting data by polling when they want it and others want a
>> These sound like something that can be added incrementally?
>>
>>>> If the code was moved out of staging today what would go wrong?
>>> Churn in interfaces is probably about it.  Maybe a good use of any
>> time
>>
>> I guess the big question is then if we can live with that.
> Agreed.
>>> would be for people to take their non IIO drivers that they think
>> might
>>> fit (or data sheets!) and see whether there are things that they
>> would
>>> like to be different.
>> In tree there's a few auxadc and comparator drivers in drivers/mfd,
>> plus
>> things like arch/arm/plat-samsung/adc.c in the arch direcories.  These
>> are all broadly similar to the at91 code that's been sent to IIO
>> already.  There's also the code Alan posted at the top of this thread.
> Quite a lot of things in miscellaneous as well to possibly pull in over time.
>
>
>

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Greg Kroah-Hartman]

On Wed, Apr 11, 2012 at 07:19:01AM +0100, Jonathan Cameron wrote:
> 
> 
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> 
> >On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
> >
> >> 1)  Review of code.  This is crucial. If people have a little time
> >> ripping holes in the core IIO code is what we need.  Arnd did a good
> >job
> >> of this a while back. Others have done bits of it since.
> >
> >> 2) Getting the push code tidied up and pushed out.  I'll post it as
> >an
> >> updated rfc to linux-iio shortly.  All I had left that definitely
> >> wanted doing here was cleaning up the example iio to input bridge
> >> driver.  That can happen later.
> >
> >For these two can we refactor in place?  That's pretty much what seems
> >to have been happening anyway...
> >
> I guess it comes down to whether Linus will pull.  2 should be there
> within a week or so anyway depending mostly on analog testing I
> haven't broken any of their drivers.

Hm, shouldn't I be the one that moves this out of staging?  :)

Anyway, all I care about to get this code out of staging is that you
feel your userspace api is "sane" and not going to change.  Your
in-kernel stuff can radically change every kernel release with no
objection from me at all.

And from what I can tell, your userspace stuff looks pretty stable now,
right?  So, I don't mind moving this all out of staging for 3.5 as-is.

If so, I'll be glad to make the change to my repo so it starts to show
up in linux-next in the "correct" place whenever you want me to.

thanks,

greg k-h

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

>On Wed, Apr 11, 2012 at 07:19:01AM +0100, Jonathan Cameron wrote:
>> 
>> 
>> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
>> 
>> >On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
>> >
>> >> 1)  Review of code.  This is crucial. If people have a little time
>> >> ripping holes in the core IIO code is what we need.  Arnd did a
>good
>> >job
>> >> of this a while back. Others have done bits of it since.
>> >
>> >> 2) Getting the push code tidied up and pushed out.  I'll post it
>as
>> >an
>> >> updated rfc to linux-iio shortly.  All I had left that definitely
>> >> wanted doing here was cleaning up the example iio to input bridge
>> >> driver.  That can happen later.
>> >
>> >For these two can we refactor in place?  That's pretty much what
>seems
>> >to have been happening anyway...
>> >
>> I guess it comes down to whether Linus will pull.  2 should be there
>> within a week or so anyway depending mostly on analog testing I
>> haven't broken any of their drivers.
>
>Hm, shouldn't I be the one that moves this out of staging?  :)

Sorry. Still in mind set of last try where we a parrallel version planned..

>Anyway, all I care about to get this code out of staging is that you
>feel your userspace api is "sane" and not going to change.  Your
>in-kernel stuff can radically change every kernel release with no
>objection from me at all.
>
>And from what I can tell, your userspace stuff looks pretty stable now,
>right?  So, I don't mind moving this all out of staging for 3.5 as-is.

There are corners of the userspace abi that aren't but they aren't related to the core so we can break the abi docs in two and just move the good stuff... 


>
>If so, I'll be glad to make the change to my repo so it starts to show
>up in linux-next in the "correct" place whenever you want me to.

Will clear current queue under review then that would be great.

Note we will need to leave a lot of drivers in staging for now so move may require a few sed scripts to link up headers etc...  quite a lot of drivers are way off.

Lets start a fresh thread on the move to make sure everyone agrees on what is ready!  

Thanks as ever for all your hard work!

>thanks,
>
>greg k-h

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

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Greg Kroah-Hartman]

On Wed, Apr 11, 2012 at 05:30:47PM +0100, Jonathan Cameron wrote:
> >Anyway, all I care about to get this code out of staging is that you
> >feel your userspace api is "sane" and not going to change.  Your
> >in-kernel stuff can radically change every kernel release with no
> >objection from me at all.
> >
> >And from what I can tell, your userspace stuff looks pretty stable now,
> >right?  So, I don't mind moving this all out of staging for 3.5 as-is.
> 
> There are corners of the userspace abi that aren't but they aren't
> related to the core so we can break the abi docs in two and just move
> the good stuff...

Ok, that sounds like a good first step.

> >If so, I'll be glad to make the change to my repo so it starts to show
> >up in linux-next in the "correct" place whenever you want me to.
> 
> Will clear current queue under review then that would be great.
> 
> Note we will need to leave a lot of drivers in staging for now so move
> may require a few sed scripts to link up headers etc...  quite a lot
> of drivers are way off.

Why would some drivers stay?  It should only be because of the userspace
api, not because they don't work properly, that's never stopped drivers
from entering the main part of the kernel before :)

> Lets start a fresh thread on the move to make sure everyone agrees on
> what is ready!

Sounds good.

greg k-h

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

>On Wed, Apr 11, 2012 at 05:30:47PM +0100, Jonathan Cameron wrote:
>> >Anyway, all I care about to get this code out of staging is that you
>> >feel your userspace api is "sane" and not going to change.  Your
>> >in-kernel stuff can radically change every kernel release with no
>> >objection from me at all.
>> >
>> >And from what I can tell, your userspace stuff looks pretty stable
>now,
>> >right?  So, I don't mind moving this all out of staging for 3.5
>as-is.
>> 
>> There are corners of the userspace abi that aren't but they aren't
>> related to the core so we can break the abi docs in two and just move
>> the good stuff...
>
>Ok, that sounds like a good first step.
>
>> >If so, I'll be glad to make the change to my repo so it starts to
>show
>> >up in linux-next in the "correct" place whenever you want me to.
>> 
>> Will clear current queue under review then that would be great.
>> 
>> Note we will need to leave a lot of drivers in staging for now so
>move
>> may require a few sed scripts to link up headers etc...  quite a lot
>> of drivers are way off.
>
>Why would some drivers stay?  It should only be because of the
>userspace
>api, not because they don't work properly, that's never stopped drivers
>from entering the main part of the kernel before :)
>
You got it in one.  They have userspace interfaces that are miles from where they should be... 
>> Lets start a fresh thread on the move to make sure everyone agrees on
>> what is ready!
>
>Sounds good.
>
>greg k-h

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