Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Linus Walleij]

2011/10/9 Zoltan Devai <zoss@devai.org>:

> Signed-off-by: Zoltan Devai <zoss@devai.org>
> ---
> =A0arch/arm/mach-spmp8000/adc.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0| =A0465 +++++++++++++++++++++
> =A0arch/arm/mach-spmp8000/include/mach/spmp8000adc.h | =A0 29 ++

I think we stopped stuffing misc drivers under arch/arm/*

And stuffing them under drivers/misc/* won't be popular either.

IIO has some ADCs in drivers/staging/iio/adc
these seem all to be intended to be used from userspace.

There are a few in-kernel ADCs all over the kernel tree
for various specific purposes.

I don't know where we should go with this kind of stuff,
really :-/

Yours,
Linus Walleij

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Jonathan Cameron]

On 10/10/11 02:29, Linus Walleij wrote:
> 2011/10/9 Zoltan Devai <zoss@devai.org>:
> 
>> Signed-off-by: Zoltan Devai <zoss@devai.org>
>> ---
>>  arch/arm/mach-spmp8000/adc.c                      |  465 +++++++++++++++++++++
>>  arch/arm/mach-spmp8000/include/mach/spmp8000adc.h |   29 ++
> 
> I think we stopped stuffing misc drivers under arch/arm/*
> 
> And stuffing them under drivers/misc/* won't be popular either.
> 
> IIO has some ADCs in drivers/staging/iio/adc
> these seem all to be intended to be used from userspace.
Yeah, we've had various discussions about adding support for
in kernel users. Its always fallen on the fact that no one
has had the time to write the code to allow it all to be
linked up.  Right now it's a case of drivers providing their
own interfaces if they want to allow this.
Mark Brown has raised this issue before so I've cc'd him.

For reference of those not following the original thread (based
on a quick scan of the driver code in question).

Driver has two callback sets:

1) Touch panel reads (I guess going straight to an input driver)
(1 of these only). This is annoyingly tied up with the more general
adc registers.
2) General purpose adc callbacks. 

Both are driven off an interrupt, but data ready
appears to manually driven (so software triggered sampling then data
ready signal when sampling is done for a single channel).

So what are these callbacks used for?  Note they all run as
interrupt top halves...

Options at current time are:

mfd supporting input device and either hwmon or iio device.

A driver that hosts both a hwmon / iio and an input device.

Be brave (+ have a lot of time) and propose patches adding the relevant
tying together code to allow iio drivers to be queried in kernel.
It shouldn't be that bad actually if you solve the how to work
out which device / channel you want in a consistent way.

It really depends on what those general adc callbacks get used for.

> 
> There are a few in-kernel ADCs all over the kernel tree
> for various specific purposes.
> 
> I don't know where we should go with this kind of stuff,
> really :-/
> 
> Yours,
> Linus Walleij
> --
> 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
> 

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Jonathan Cameron]

On 10/10/11 10:42, Jonathan Cameron wrote:
> On 10/10/11 02:29, Linus Walleij wrote:
>> 2011/10/9 Zoltan Devai <zoss@devai.org>:
>>
>>> Signed-off-by: Zoltan Devai <zoss@devai.org>
>>> ---
>>>  arch/arm/mach-spmp8000/adc.c                      |  465 +++++++++++++++++++++
>>>  arch/arm/mach-spmp8000/include/mach/spmp8000adc.h |   29 ++
>>
>> I think we stopped stuffing misc drivers under arch/arm/*
>>
>> And stuffing them under drivers/misc/* won't be popular either.
>>
>> IIO has some ADCs in drivers/staging/iio/adc
>> these seem all to be intended to be used from userspace.
> Yeah, we've had various discussions about adding support for
> in kernel users. Its always fallen on the fact that no one
> has had the time to write the code to allow it all to be
> linked up.  Right now it's a case of drivers providing their
> own interfaces if they want to allow this.
> Mark Brown has raised this issue before so I've cc'd him.
> 
> For reference of those not following the original thread (based
> on a quick scan of the driver code in question).
> 
> Driver has two callback sets:
> 
> 1) Touch panel reads (I guess going straight to an input driver)
> (1 of these only). This is annoyingly tied up with the more general
> adc registers.
> 2) General purpose adc callbacks. 
> 
> Both are driven off an interrupt, but data ready
> appears to manually driven (so software triggered sampling then data
> ready signal when sampling is done for a single channel).
> 
> So what are these callbacks used for?  Note they all run as
> interrupt top halves...
> 
> Options at current time are:
> 
> mfd supporting input device and either hwmon or iio device.
> 
> A driver that hosts both a hwmon / iio and an input device.
> 
> Be brave (+ have a lot of time) and propose patches adding the relevant
> tying together code to allow iio drivers to be queried in kernel.
> It shouldn't be that bad actually if you solve the how to work
> out which device / channel you want in a consistent way.
> 
> It really depends on what those general adc callbacks get used for.
Ah, just taken a look at your dts file.

Looks like some are used for 'analog-keys'.  So input? If so this
should probably be an entirely input driver for now.

> 
>>
>> There are a few in-kernel ADCs all over the kernel tree
>> for various specific purposes.
>>
>> I don't know where we should go with this kind of stuff,
>> really :-/
>>
>> Yours,
>> Linus Walleij
>> --
>> 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
>>
> 
> --
> 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
> 

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Mark Brown]

On Mon, Oct 10, 2011 at 10:42:49AM +0100, Jonathan Cameron wrote:

> It really depends on what those general adc callbacks get used for.

If it's a general purpose ADC it's up to the board designer - you
sometimes see things like battery or supply monitoring but really it
could be any low volume analogue input.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Zoltan Devai]

2011/10/10 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Mon, Oct 10, 2011 at 10:42:49AM +0100, Jonathan Cameron wrote:
>
>> It really depends on what those general adc callbacks get used for.
>
> If it's a general purpose ADC it's up to the board designer - you
> sometimes see things like battery or supply monitoring but really it
> could be any low volume analogue input.

It's really up to the board designer. In this case, all real-world
scenarios use the adc for analog-keys AND battery monitoring.

Jonathan, your analysis of the driver is completely correct:
Currently, only the not-yet-submitted analog-keys input driver uses
this, but I'd like to add battery monitoring shortly, so this has at
least two users.

Where should the adc driver go in this case ?

Zoltan

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Mark Brown]

On Mon, Oct 10, 2011 at 01:42:30PM +0200, Zoltan Devai wrote:

> Where should the adc driver go in this case ?

At the minute it seems to me that arch/arm is as good a place as any
really - currently this code is getting dumped wherever the main device
is.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Arnd Bergmann]

On Monday 10 October 2011, Mark Brown wrote:
> On Mon, Oct 10, 2011 at 01:42:30PM +0200, Zoltan Devai wrote:
> 
> > Where should the adc driver go in this case ?
> 
> At the minute it seems to me that arch/arm is as good a place as any
> really - currently this code is getting dumped wherever the main device
> is.

My feeling is that there is a general class of drivers involving
the gpio, pinmux, pwm, led, adc and dac: these are basically all
things you do with a single pin. Right now we have subsystems for
some of them, adding some others and don't have anything for adc
basides iio (which only partially fits here).

Since there is no externally visible interface for this kind of
adc driver, we can always fix it later, which helps a lot. How
about putting it into drivers/adc or drivers/misc/adc for now
and waiting for others to join it? We can then later make it a
proper subsystem along the lines of gpio and pwm and move
interfaces for input, hwmon and iio into the subsystem.

	Arnd

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Mark Brown]

On Tue, Oct 11, 2011 at 04:17:51PM +0200, Arnd Bergmann wrote:
> On Monday 10 October 2011, Mark Brown wrote:

> > At the minute it seems to me that arch/arm is as good a place as any
> > really - currently this code is getting dumped wherever the main device
> > is.

> My feeling is that there is a general class of drivers involving
> the gpio, pinmux, pwm, led, adc and dac: these are basically all
> things you do with a single pin. Right now we have subsystems for
> some of them, adding some others and don't have anything for adc
> basides iio (which only partially fits here).

We've been round this loop repeatedly before, trying to push all this
stuff into pins really doesn't map terribly well.  For example, SoCs
will frequently have PWM controllers which are isolated IP blocks within
the device and may each have multiple mappings out of the device onto
pins.  In the case of DACs, ADCs and PWM you definitely don't have a one
to one mapping with a single pin - they can be differential inputs and
outputs, or entirely internal to a device.  Trying to push everything
into a single namespace just makes things confusing.

What we *do* have a pattern of is clk API type lookups based on a
device/name pair.

> Since there is no externally visible interface for this kind of
> adc driver, we can always fix it later, which helps a lot. How
> about putting it into drivers/adc or drivers/misc/adc for now
> and waiting for others to join it? We can then later make it a
> proper subsystem along the lines of gpio and pwm and move
> interfaces for input, hwmon and iio into the subsystem.

I'm not sure that IIO isn't the kernel subsystem we're looking for here
- as I keep saying when this comes up it's just representing bare DACs
and ADCs pretty directly which looks like a generic subsystem to me.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Arnd Bergmann]

On Tuesday 11 October 2011, Mark Brown wrote:
> On Tue, Oct 11, 2011 at 04:17:51PM +0200, Arnd Bergmann wrote:
> > On Monday 10 October 2011, Mark Brown wrote:
> 
> > > At the minute it seems to me that arch/arm is as good a place as any
> > > really - currently this code is getting dumped wherever the main device
> > > is.
> 
> > My feeling is that there is a general class of drivers involving
> > the gpio, pinmux, pwm, led, adc and dac: these are basically all
> > things you do with a single pin. Right now we have subsystems for
> > some of them, adding some others and don't have anything for adc
> > basides iio (which only partially fits here).
> 
> We've been round this loop repeatedly before, trying to push all this
> stuff into pins really doesn't map terribly well.  For example, SoCs
> will frequently have PWM controllers which are isolated IP blocks within
> the device and may each have multiple mappings out of the device onto
> pins.  In the case of DACs, ADCs and PWM you definitely don't have a one
> to one mapping with a single pin - they can be differential inputs and
> outputs, or entirely internal to a device.  Trying to push everything
> into a single namespace just makes things confusing.

I didn't mean to suggest that they should be the same subsystem, but
it would be nice to use similar in-kernel interfaces so that a driver
author doesn't have to learn about six different interfaces for
a new soc. If we add an adc subsystem, I would definitely recommend
looking at how the others work.

> > Since there is no externally visible interface for this kind of
> > adc driver, we can always fix it later, which helps a lot. How
> > about putting it into drivers/adc or drivers/misc/adc for now
> > and waiting for others to join it? We can then later make it a
> > proper subsystem along the lines of gpio and pwm and move
> > interfaces for input, hwmon and iio into the subsystem.
> 
> I'm not sure that IIO isn't the kernel subsystem we're looking for here
> - as I keep saying when this comes up it's just representing bare DACs
> and ADCs pretty directly which looks like a generic subsystem to me.

Possible, yes. Until now, IIO is a subsystem for user-level access
not for kernel access though, so it's not the right place yet.
If we decide to let IIO handle all ADC input, do you think it would
also be the right place to do PWM output, rather than having a
separate subsystem for that?

	Arnd

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Jonathan Cameron]

On 10/11/11 16:24, Arnd Bergmann wrote:
> On Tuesday 11 October 2011, Mark Brown wrote:
>> On Tue, Oct 11, 2011 at 04:17:51PM +0200, Arnd Bergmann wrote:
>>> On Monday 10 October 2011, Mark Brown wrote:
>>
>>>> At the minute it seems to me that arch/arm is as good a place as any
>>>> really - currently this code is getting dumped wherever the main device
>>>> is.
>>
>>> My feeling is that there is a general class of drivers involving
>>> the gpio, pinmux, pwm, led, adc and dac: these are basically all
>>> things you do with a single pin. Right now we have subsystems for
>>> some of them, adding some others and don't have anything for adc
>>> basides iio (which only partially fits here).
>>
>> We've been round this loop repeatedly before, trying to push all this
>> stuff into pins really doesn't map terribly well.  For example, SoCs
>> will frequently have PWM controllers which are isolated IP blocks within
>> the device and may each have multiple mappings out of the device onto
>> pins.  In the case of DACs, ADCs and PWM you definitely don't have a one
>> to one mapping with a single pin - they can be differential inputs and
>> outputs, or entirely internal to a device.  Trying to push everything
>> into a single namespace just makes things confusing.
> 
> I didn't mean to suggest that they should be the same subsystem, but
> it would be nice to use similar in-kernel interfaces so that a driver
> author doesn't have to learn about six different interfaces for
> a new soc. If we add an adc subsystem, I would definitely recommend
> looking at how the others work.
Yup, I'm reading the clk api stuff right now.
> 
>>> Since there is no externally visible interface for this kind of
>>> adc driver, we can always fix it later, which helps a lot. How
>>> about putting it into drivers/adc or drivers/misc/adc for now
>>> and waiting for others to join it? We can then later make it a
>>> proper subsystem along the lines of gpio and pwm and move
>>> interfaces for input, hwmon and iio into the subsystem.
>>
>> I'm not sure that IIO isn't the kernel subsystem we're looking for here
>> - as I keep saying when this comes up it's just representing bare DACs
>> and ADCs pretty directly which looks like a generic subsystem to me.
> 
> Possible, yes. Until now, IIO is a subsystem for user-level access
> not for kernel access though, so it's not the right place yet.
Technically proof of concept code for kernel access went out this morning
(sorry, didn't cc you as you hadn't entered this thread yet).   It's
all of 10 lines of code right now.  But yeah, its userspace focussed.

https://lkml.org/lkml/2011/10/11/141

Covers basic hwmon type usage (polled single channel read).  I'm not nearly
as sure how to handle complex channel scans.  There's a lot of infrastructure
involved in those.

> If we decide to let IIO handle all ADC input, do you think it would
> also be the right place to do PWM output, rather than having a
> separate subsystem for that?
That's one I've been avoiding talking about.  The fuzzy region is between
DDS (direct digital synthesis - waveform generator) chips which we have
been taking in IIO so far.   We don't actually handle these that well
at the moment.  Our core channel registration code doesn't handle the
weird routing these can do - single underlying generator does several
synchronized waveforms.

Pwm is small compact and nice.  Perhaps a similar shim to that I proposed
to hwmon alongside that patch earlier.  So simple pwm only device sits
in pwm, but we have a general purpose driver sitting on IIO that can
provide pwm from an appropriate channel of a more complex device.
It would need hooks that aren't there at the moment and more of the
DDS control to become part of the IIO core.

We may ultimately want to hive off more of IIO into a layer sitting
at same level as other users. It's tricky to know where to make
such divisions though.

Jonathan

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Mark Brown]

On Tue, Oct 11, 2011 at 05:24:47PM +0200, Arnd Bergmann wrote:
> On Tuesday 11 October 2011, Mark Brown wrote:

> > I'm not sure that IIO isn't the kernel subsystem we're looking for here
> > - as I keep saying when this comes up it's just representing bare DACs
> > and ADCs pretty directly which looks like a generic subsystem to me.

> Possible, yes. Until now, IIO is a subsystem for user-level access
> not for kernel access though, so it's not the right place yet.
> If we decide to let IIO handle all ADC input, do you think it would
> also be the right place to do PWM output, rather than having a
> separate subsystem for that?

Off the top of my head I'd expect PWM to be a separate thing which could
optionally synthesize stuff onto an IIO device (bitbanging the PWM
essentially).  The specialized PWM stuff tends to be "output this steady
state for an indefinite period" type interface rather than "here's a
batch of samples, output them".

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Jonathan Cameron]

On 10/12/11 15:42, Mark Brown wrote:
> On Tue, Oct 11, 2011 at 05:24:47PM +0200, Arnd Bergmann wrote:
>> On Tuesday 11 October 2011, Mark Brown wrote:
> 
>>> I'm not sure that IIO isn't the kernel subsystem we're looking for here
>>> - as I keep saying when this comes up it's just representing bare DACs
>>> and ADCs pretty directly which looks like a generic subsystem to me.
> 
>> Possible, yes. Until now, IIO is a subsystem for user-level access
>> not for kernel access though, so it's not the right place yet.
>> If we decide to let IIO handle all ADC input, do you think it would
>> also be the right place to do PWM output, rather than having a
>> separate subsystem for that?
> 
> Off the top of my head I'd expect PWM to be a separate thing which could
> optionally synthesize stuff onto an IIO device (bitbanging the PWM
> essentially).  The specialized PWM stuff tends to be "output this steady
> state for an indefinite period" type interface rather than "here's a
> batch of samples, output them".
Same can be true of dds chips, with the addition that they often have
gpio type switching between a set of predefined frequencies (FM basically).
+ weird waveform choices sometimes.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Russell King - ARM Linux]

On Mon, Oct 10, 2011 at 12:44:08PM +0100, Mark Brown wrote:
> On Mon, Oct 10, 2011 at 01:42:30PM +0200, Zoltan Devai wrote:
> 
> > Where should the adc driver go in this case ?
> 
> At the minute it seems to me that arch/arm is as good a place as any
> really - currently this code is getting dumped wherever the main device
> is.

No it isn't - we want drivers out of arch/arm (it's already been a topic
of flame for Linus, so it's something that we should try _really_ hard
to avoid.)

As this is clearly a device driver (it has a device driver struct, it
relies upon a struct device, etc) then it needs to live outside of
arch/arm/ - and I think Arnd's suggestion of drivers/adc is probably
the right place for it to move to (even though its probably the first
to create the directory.)  More stuff can come along in the future,
and then its all together to start creating a common API in there.

The other thing I'd say is that things like the callback definitions
and driver data should not be in some arch/arm/ include file - if
it's private to the ADC, it should go in drivers/adc too.  If not,
then somewhere in include/linux (maybe include/linux/adc) is a better
place.

What we *do* need to avoid is having the spmp8000 ADC callback function
data structure, the foo ADC callback function data structure, the bar
ADC callback function data structure and so forth.  We need to think
*NOW* about defining a common ADC callback structure so that we don't
spawn a new problem which'll take effort to solve.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Linus Walleij]

On Thu, Oct 13, 2011 at 11:47 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 10, 2011 at 12:44:08PM +0100, Mark Brown wrote:
>> On Mon, Oct 10, 2011 at 01:42:30PM +0200, Zoltan Devai wrote:
>>
>> > Where should the adc driver go in this case ?
>>
>> At the minute it seems to me that arch/arm is as good a place as any
>> really - currently this code is getting dumped wherever the main device
>> is.
>
> No it isn't - we want drivers out of arch/arm (it's already been a topic
> of flame for Linus, so it's something that we should try _really_ hard
> to avoid.)

Amen to that.

> As this is clearly a device driver (it has a device driver struct, it
> relies upon a struct device, etc) then it needs to live outside of
> arch/arm/ - and I think Arnd's suggestion of drivers/adc is probably
> the right place for it to move to (even though its probably the first
> to create the directory.) =A0More stuff can come along in the future,
> and then its all together to start creating a common API in there.

That sort of suggest that the drivers/staging/iio/adc is in
trouble now. It will have an in-kernel competitor for similar
stuff.

Having drivers/adc and drivers/iio/adc compete about
taking drivers on preference as to whether they were intended
for userspace or kernelspace does not seem like a good
thing.

>>From a quick review of the IIO ADC subsystem I cannot really
find a big problem with it other than it brings the entire IIO
subsystem along, including the userspace interfaces I guess.

The circumstances seem to suggest that drivers/iio/adc should
be moved down to drivers/adc without the rest of the iio
stuff, with a in-kernel API only and then have iio use that
(this is probably true for some more drivers in IIO like
dac, accel, addac, gyro...).

So the concept of IIO stuff would change from the
userspace-first centric view to the kernelspace-first
centric, with optional userspace interfaces in the form
of IIO.

I'm not sure about what Jonathan thinks about that
though.

Yours,
Linus Walleij

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Mark Brown]

On Thu, Oct 13, 2011 at 10:47:17AM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 10, 2011 at 12:44:08PM +0100, Mark Brown wrote:

> > At the minute it seems to me that arch/arm is as good a place as any
> > really - currently this code is getting dumped wherever the main device
> > is.

> No it isn't - we want drivers out of arch/arm (it's already been a topic
> of flame for Linus, so it's something that we should try _really_ hard
> to avoid.)

I said it was as good a place as any, I didn't say it was a good place.
Currently we don't have a good place for this code but just dumping the
code somewhere else isn't really helping anything either unless there's
some meaningful subsytem (or at least effort towards a subsystem) to
look after it, it's bookkeeping.

> As this is clearly a device driver (it has a device driver struct, it
> relies upon a struct device, etc) then it needs to live outside of
> arch/arm/ - and I think Arnd's suggestion of drivers/adc is probably
> the right place for it to move to (even though its probably the first
> to create the directory.)  More stuff can come along in the future,
> and then its all together to start creating a common API in there.

Creating drivers/adc just seems like a sucky idea.  We've already got at
least one ongoing effort at creating a general purpose ADC/DAC interface
in the form of the IIO subsystem (which is currently mostly targeted at
higher volume devices and userspace only but the abstractions needed
aren't terribly different depending on the data volumes) and does have
the rather substantial advantage that there's people actually trying to
make a subsystem.  On the other hand it's in staging for now so it'd
create pain trying to merge the in-kernel clients that go along with
these devices which doesn't seem constructive.

There's also the fact that ADCs and DACs are pretty much the same thing
to software with the data line driven from the opposite end of the link
sO if we've got a subsystem for ADCs only it seems clear we're doing
something wrong.

> What we *do* need to avoid is having the spmp8000 ADC callback function
> data structure, the foo ADC callback function data structure, the bar
> ADC callback function data structure and so forth.  We need to think
> *NOW* about defining a common ADC callback structure so that we don't
> spawn a new problem which'll take effort to solve.

The cat's already well out of the bag on that one, we've got a bunch of
these things in tree already - the Cragganmore system I've got sitting
here on my desk has three auxiliary ADCs I'm aware of, all of which are
supported in mainline, and they're far from the only ones.

I think we should just carry on as we are while the IIO work proceeds,
either we'll decide that that's the way forward and we can then kick all
the ADCs into there or we'll get fed up, decide that's never going to
work then go and can do something else.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Jonathan Cameron]

On 10/13/11 12:09, Linus Walleij wrote:
> On Thu, Oct 13, 2011 at 11:47 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Mon, Oct 10, 2011 at 12:44:08PM +0100, Mark Brown wrote:
>>> On Mon, Oct 10, 2011 at 01:42:30PM +0200, Zoltan Devai wrote:
>>>
>>>> Where should the adc driver go in this case ?
>>>
>>> At the minute it seems to me that arch/arm is as good a place as any
>>> really - currently this code is getting dumped wherever the main device
>>> is.
>>
>> No it isn't - we want drivers out of arch/arm (it's already been a topic
>> of flame for Linus, so it's something that we should try _really_ hard
>> to avoid.)
> 
> Amen to that.
> 
>> As this is clearly a device driver (it has a device driver struct, it
>> relies upon a struct device, etc) then it needs to live outside of
>> arch/arm/ - and I think Arnd's suggestion of drivers/adc is probably
>> the right place for it to move to (even though its probably the first
>> to create the directory.)  More stuff can come along in the future,
>> and then its all together to start creating a common API in there.
> 
> That sort of suggest that the drivers/staging/iio/adc is in
> trouble now. It will have an in-kernel competitor for similar
> stuff.
> 
> Having drivers/adc and drivers/iio/adc compete about
> taking drivers on preference as to whether they were intended
> for userspace or kernelspace does not seem like a good
> thing.
The problem with a straight adc directory is the fact that many of
these devices are combined input and output.  The also tend to
have numerous things that to a casual don't look like ADC's
(light sensors etc).  We only did the grouping in IIO for the
sake of not having a huge flat directory.  Many of the drivers
overlap between the different categories.
> 
>>>From a quick review of the IIO ADC subsystem I cannot really
> find a big problem with it other than it brings the entire IIO
> subsystem along, including the userspace interfaces I guess.
> 
> The circumstances seem to suggest that drivers/iio/adc should
> be moved down to drivers/adc without the rest of the iio
> stuff, with a in-kernel API only and then have iio use that
> (this is probably true for some more drivers in IIO like
> dac, accel, addac, gyro...).
> 
> So the concept of IIO stuff would change from the
> userspace-first centric view to the kernelspace-first
> centric, with optional userspace interfaces in the form
> of IIO.
> 
> I'm not sure about what Jonathan thinks about that
> though.
That wouldn't be too hard to do. It's just a case of breaking
iio_register_device function in two.  Right now (with the in kernel
RFC) on top.  The only thing needed for in kernel access is that the
configured struct iio_dev gets added to the list of available devices.

I think we need to maintain the form of struct iio_chan_spec 
(or whatever does the same job) with the intent of it directly mapping
to userspace interfaces (keeps things clean and makes sure there is enough
information to actually use the channel for anything).

The key thing is the unified specification of channel capabilities
and the ability to read them.  We have one way of doing that which
has developed against a 'lot' of drivers.

The nasty corner is optimized reading of channel scans.  They tend to
be irrelevant for memory mapped device but are vital for many devices
on low bandwidth buses.  Right now we leave that entirely to the
individual drivers (they push this data out rather than it being
pulled from the core like everything else).  It may be possible to
flip this around with out too much pain, but I'm not sure.
The other nasty bits are:
* hardware buffering
* triggered capture control - we really care about this for IIO
and I don't think anyone else does.  Could maybe get away with using
device tree or similar to specify defaults for this and not provide
any means of changing it for devices other than those with full IIO
userspace in place.

As you can imagine I'm a little wary of sending IIO through another
refactoring right now (proposal to move the core out of staging is
scheduled for my next free afternoon!).  So in principal in favour
of this approach but in practice not sure I'll be in a position
to do it (need to eat and my funding source is about to run out -
awaiting decision on more).

So my personal preference would be to add the relevant access functions
etc to IIO as is.  Two models:

1) Pull from drivers - (give me reading now).
Done - we need to nick the clkdev mapping approach to join everything
up and play a few tricks to get a unique consistent name.

2) Push from triggered capture  - (here is a reading).
Easiest option from our point of view is to provide this as a 'buffer'
implementation.  Normally we'd notify userspace of new data, here we
notify in kernel users (or just push it into their callbacks).

Post that we start to make IIO userspace interfaces optional.

It gives a progression towards what you are suggesting and doesn't
hold us up in staging for another long cycle.

There are exceptions. I'd argue any element that is really built for
say hwmon or input ought to register directly with those subsytems.
That includes the touchscreen element of the adc that started this
conversation.  No point in adding overhead and generality to a non
general element.

Sorry for quick reply, mad day.

Jonathan

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Russell King - ARM Linux]

On Thu, Oct 13, 2011 at 12:35:06PM +0100, Mark Brown wrote:
> On Thu, Oct 13, 2011 at 10:47:17AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 10, 2011 at 12:44:08PM +0100, Mark Brown wrote:
> 
> > > At the minute it seems to me that arch/arm is as good a place as any
> > > really - currently this code is getting dumped wherever the main device
> > > is.
> 
> > No it isn't - we want drivers out of arch/arm (it's already been a topic
> > of flame for Linus, so it's something that we should try _really_ hard
> > to avoid.)
> 
> I said it was as good a place as any, I didn't say it was a good place.

I'm saying it is a *BAD* place.  I'm saying that we've been flamed over
this several times before.  We need to change our behaviour RIGHT NOW,
not continue on ignoring the problem, and demonstrating to Linus that
we don't take his concerns seriously.

If we can't find a place for it that's outside of arch/arm, then it
doesn't go in.

> I think we should just carry on as we are while the IIO work proceeds,
> either we'll decide that that's the way forward and we can then kick all
> the ADCs into there or we'll get fed up, decide that's never going to
> work then go and can do something else.

You mean like other stuff which is already in the kernel gets fixed?
It doesn't get fixed.  We persist with per-driver private data structures
which multiply all over the place.

Look at what happened with MTD and the flash_platform_data structure.
That got ignored and instead people just continued merrily creating their
own private crap time and time again.

We can not continue like this.  We can not continue to be soo permissive.

I'm now saying a big NO to this - I don't care that the "cat is already
out of the bag" - that's a defeatest attitude.  I'm saying no *now* so
that there _is_ some kind of movement towards fixing this problem.  I
don't care whether that means it shares an existing ADC drivers callback
data structures or what - I just don't want to see yet another driver
private data structure being created for NO REASON what so ever.

Or maybe you'd like to be on the receiving end of another Linus flame?

Some people would like to avoid such things - maybe you feed off them,
that's your decision.  I personally am on the side of the folk who'd
like to avoid being on the receiving end of the next Linus flame.

So.  No new drivers in arch/arm.  And I'm going to be saying no to any
new per-driver data structures in mach/*.h for stuff which should be
generic which haven't been justified for why they need to be different
from someone elses.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Arnd Bergmann]

On Thursday 13 October 2011, Russell King - ARM Linux wrote:
> On Thu, Oct 13, 2011 at 12:35:06PM +0100, Mark Brown wrote:
> > On Thu, Oct 13, 2011 at 10:47:17AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Oct 10, 2011 at 12:44:08PM +0100, Mark Brown wrote:
> > 
> > > > At the minute it seems to me that arch/arm is as good a place as any
> > > > really - currently this code is getting dumped wherever the main device
> > > > is.
> > 
> > > No it isn't - we want drivers out of arch/arm (it's already been a topic
> > > of flame for Linus, so it's something that we should try really hard
> > > to avoid.)
> > 
> > I said it was as good a place as any, I didn't say it was a good place.
> 
> I'm saying it is a BAD place.  I'm saying that we've been flamed over
> this several times before.  We need to change our behaviour RIGHT NOW,
> not continue on ignoring the problem, and demonstrating to Linus that
> we don't take his concerns seriously.
> 
> If we can't find a place for it that's outside of arch/arm, then it
> doesn't go in.

How about deferring the decision for now and putting it into
drivers/staging/adc, with a single TODO item listing "Work out proper
API"? After all, the reason this driver doesn't fit anywhere is that
there is no established subsystem for it, at least not outside of
staging.

AFAICT, the driver is not essential for spmp8000 systems, but you
would want to have it there in practice. It makes little sense to
put the driver into drivers/adc and then later change the interface
fundamentally again if we decide to go with drivers/iio/adc providing
the common abstraction lation.

Of course, if the spmp8000 adc driver gets put into staging, all its
dependencies (input, battery) also have to go there because no
regular driver must link against interfaces provided by a staging
driver.

	Arnd

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Mark Brown]

On Thu, Oct 13, 2011 at 04:19:39PM +0200, Arnd Bergmann wrote:

> Of course, if the spmp8000 adc driver gets put into staging, all its
> dependencies (input, battery) also have to go there because no
> regular driver must link against interfaces provided by a staging
> driver.

Yes, that's the big problem with this - it blocks the actual drivers.
Putting it in drivers/misc would be another option that sidesteps that.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Mark Brown]

On Thu, Oct 13, 2011 at 01:17:45PM +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 13, 2011 at 12:35:06PM +0100, Mark Brown wrote:

> > I said it was as good a place as any, I didn't say it was a good place.

> I'm saying it is a *BAD* place.  I'm saying that we've been flamed over
> this several times before.  We need to change our behaviour RIGHT NOW,
> not continue on ignoring the problem, and demonstrating to Linus that
> we don't take his concerns seriously.

I think we're agreeing here...

> > I think we should just carry on as we are while the IIO work proceeds,
> > either we'll decide that that's the way forward and we can then kick all
> > the ADCs into there or we'll get fed up, decide that's never going to
> > work then go and can do something else.

> You mean like other stuff which is already in the kernel gets fixed?
> It doesn't get fixed.  We persist with per-driver private data structures
> which multiply all over the place.

> Look at what happened with MTD and the flash_platform_data structure.
> That got ignored and instead people just continued merrily creating their
> own private crap time and time again.

The situation here is a bit different; there's people working on
handling these devices already but we can't point people at it as
something they can use yet.  As soon as we've got something to point
people at we should absolutely be going and actively pushing them
towards it but right now we don't have that option.

> We can not continue like this.  We can not continue to be soo permissive.

> I'm now saying a big NO to this - I don't care that the "cat is already
> out of the bag" - that's a defeatest attitude.  I'm saying no *now* so
> that there _is_ some kind of movement towards fixing this problem.  I

Half the problem that's being pointed out is that there is already
movement towards a solution for these devices, it's just not quite at
the point where it can be used for this class of devices yet.  The point
with the existing drivers being there is that this isn't a new driver
setting a precedent, if we had chosen to merge the driver we'd simply be
taking a decision to avoid creating churn that disrupts the existing
work.

> don't care whether that means it shares an existing ADC drivers callback
> data structures or what - I just don't want to see yet another driver
> private data structure being created for NO REASON what so ever.

Well, what do we do then?  I guess so long as it's outside arch/arm you
don't mind too much.

> Or maybe you'd like to be on the receiving end of another Linus flame?

> Some people would like to avoid such things - maybe you feed off them,
> that's your decision.  I personally am on the side of the folk who'd
> like to avoid being on the receiving end of the next Linus flame.

On the other hand we also don't want to overreact and we don't want to
irritate people by just putting stuff into a different directory without
actually improving it.  The goal isn't to troll Linus, the goal is to be
pragmatic about what we're actually achieving.  It's similar to the
decision that was taken to allow platforms to proceed without the
generic clk API or device tree bindings for the clk API.

> So.  No new drivers in arch/arm.  And I'm going to be saying no to any
> new per-driver data structures in mach/*.h for stuff which should be
> generic which haven't been justified for why they need to be different
> from someone elses.

The driver specific data structures should probably just be moving to
include/linux/platform_data which is the approved location for that sort
of thing.

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Arnd Bergmann]

On Thursday 13 October 2011, Mark Brown wrote:
> > So.  No new drivers in arch/arm.  And I'm going to be saying no to any
> > new per-driver data structures in mach/*.h for stuff which should be
> > generic which haven't been justified for why they need to be different
> > from someone elses.
> 
> The driver specific data structures should probably just be moving to
> include/linux/platform_data which is the approved location for that sort
> of thing.

Note that we also need someone to watch over that and say no to silly
platform_data definitions being put in there, e.g. using wildly different
ways of expressing the same things in almost-identical drivers, or
using function pointers where a simple data structure is enough.

	Arnd

Re: [PATCH 4/9] ARM: SPMP8000: Add ADC driver

[Mark Brown]

On Thu, Oct 13, 2011 at 04:56:56PM +0200, Arnd Bergmann wrote:
> On Thursday 13 October 2011, Mark Brown wrote:

> > The driver specific data structures should probably just be moving to
> > include/linux/platform_data which is the approved location for that sort
> > of thing.

> Note that we also need someone to watch over that and say no to silly
> platform_data definitions being put in there, e.g. using wildly different
> ways of expressing the same things in almost-identical drivers, or
> using function pointers where a simple data structure is enough.

Yeah, I'm not sure I entirely understand the plan with it though even
without any structure it doesn't make much difference (in that
everything is being reviewed by the subsystem maintainers just as it was
before we dumped the files in a single directory).  I'm tempted to say
the device tree folks should be looking at it as an awful lot of what
they're doing is exactly the same thing.