Message-ID: <20100122204718.GA15759@kroah.com>
Date:	Fri, 22 Jan 2010 12:47:18 -0800
From:	Greg KH <greg@...ah.com>
To:	Jonathan Cameron <kernel@...23.retrosnub.co.uk>
Cc:	Jonathan Cameron <jic23@....ac.uk>,
	LKML <linux-kernel@...r.kernel.org>,
	Manuel Stahl <manuel.stahl@....fraunhofer.de>,
	"Hennerich, Michael" <Michael.Hennerich@...log.com>,
	"Frysinger, Michael" <Michael.Frysinger@...log.com>,
	"Getz, Robin" <Robin.Getz@...log.com>,
	Jean Delvare <khali@...ux-fr.org>,
	"Trisal, Kalhan" <kalhan.trisal@...el.com>,
	"Zhang, Xing Z" <xing.z.zhang@...el.com>,
	Ira Snyder <iws@...o.caltech.edu>
Subject: Re: [RFC] Staging:IIO: New ABI

On Wed, Jan 20, 2010 at 04:53:21PM +0000, Jonathan Cameron wrote:
> Hi Greg,
> 
> > On Wed, Jan 20, 2010 at 03:13:40PM +0000, Jonathan Cameron wrote:
> >> What:		/sys/class/iio/device[n]
> >> Description:
> >> 		Hardware chip or device accessed by on communication port.
> >> 		Corresponds to a grouping of sensor channels.
> >>
> >> What:		/sys/class/iio/trigger[n]
> >> Description:
> >> 		An event driven driver of data capture to an in kernel buffer.
> >> 		May be provided a device driver that also has an IIO device
> >> 		based on hardware generated events (e.g. data ready) or
> >> 		provided by other hardware (e.g. periodic timer or gpio)
> >> 		Contains trigger type specific elements. These do not
> >> 		generalize well.
> >>
> >>
> >> What:        /sys/class/iio/ring_buffer[m]
> >> Description:
> >> 	        Link to /sys/class/iio/device[n]/ring_buffer[m]. Ring buffer
> >> 		numbering may not match that of device as some devices do not
> >> 		have ring buffers.
> > 
> > Why is this link needed?  Why can't you just look in the device
> > directory for a ring buffer?  And wouldn't the ring buffer be 1..n for
> > every device?  They shouldn't be "unique" for all iio devices, that
> > would be wierd.
> I'm a little unclear what you mean here (so my apologies if I'm misinterpreting!)
> A given IIO device will indeed have a either none or typically 1 ring buffers.
> They are not currently shared across devices. Aggregation if desired is done
> in userspace.

Ok, that's fine, but the name of those buffers do not have to be unique
to all ring buffers in the system.

Hm, oh yeah, they do, ok, nevermind, stupid me :)

> >> What:		/sys/class/iio/device[n]/name
> >> Description:
> >> 		Description of the physical chip / device. Typically a part
> >> 		number.
> >>
> >> What:		/sys/class/iio/device[n]/volt_[name][m]_raw
> >> Description:
> >> 		Raw (unscaled no bias removal etc) voltage measurement from
> >> 		channel m. name is used in special cases where this does
> >> 		not correspond to externally available input (e.g. supply
> >> 		voltage monitoring).
> > 
> > So what would 'name' be?
> This is for a few device corner cases such as volt_supply_raw for the the
> adis16350 below.  It can't really be used as a general purpose ADC
> so the idea was to separate it out.  The vast majority of voltage channels
> would simply be numbered.  There is an argument that, it might make sense
> to support this single parameter via hwmon, but then there are high end
> gyros where we would be grabbing the voltage, temperature and actual reading
> on each trigger so as to be able to compensate for the huge difference
> these can make to the resulting data.

But again, what would 'name' be?

> >> What:		/sys/class/iio/device[n]/volt_[name][m]_scale
> >> Description:
> >> 		If known for a device, scale to be applied to volt[m]_raw post
> >> 		addition of volt[m]_offset in order to obtain the measured voltage
> >> 		in volts.  If shared across all voltage	channels the m is not present.
> > 
> > For all of these voltage measurements, why use something different from
> > what the kernel already supports for the existing hardware monitoring
> > devices?  There is already a well-defined api for these things.
> Agreed.  We did consider using in0 etc as per hwmon but decided that we were
> breaking sufficiently from the approach there (where the readings are always
> provided in microvolts iirc) that we could change the name to something that
> was a little more specific.  I'm not overly tied to volt but the semantics
> are different enough here  (with offsets and gain passed to userspace) that
> we may actually create confusion by mirroring that api.

I really think you should merge with that api somehow.

I understand that you don't want to do conversions in the kernel, so
perhaps there could be some way to show this like you have described,
that the hwmon interface could also use.

That way userspace tools will work for all types of devices, and you
don't have a custom interface just for these device.  Unification is a
good thing :)

> >> What:		/sys/class/iio/device[n]/accel_[x|y|z][m]_raw
> >> Description:
> >> 		Acceleration in direction x, y or z (may be arbitrarily assigned
> >> 		but should match other such assignments on device)
> >> 		channel m (not present if only one accelerometer channel at
> >> 		this orientation). Has all of the equivalent parameters as per volt[m].
> >> 		Units after application of scale and offset are m/s^2.
> > 
> > Shouldn't this just use the existing input accelerometer interface so as
> > to keep userspace sane?
> Again, it comes down to whether we process the data or not.  IIO is all about
> the ability to handle things fast.  These sysfs interfaces are actually meant
> to mirror the chrdev ring buffer accesses which are the primary access point
> for higher end devices.  A lot of the use cases are either interested in logging
> for later use, or algorithms that will run in the device units (sometime in integer
> arithmetic).  Hence all conversion to sane units is left to userspace. The
> drivers merely provide sufficient information to do this conversion if it
> is desired.

See above.  I suggest working with the hwmon developers to add the "raw"
type interface to their api making these able to be used there as well.

> >> What:		/sys/class/iio/device[n]/event_line[m]
> >> Description:
> >> 		Configuration of which hardware generated events are passed up to
> >> 		userspace. Some of these are a bit complex to generalize so this
> >> 		section is a work in progress.
> >>
> >> What:		/sys/class/iio/device[n]/event_line[m]/dev
> >> Description:
> >> 		major:minor character device numbers.
> > 
> > No, don't bury devices down a level in sysfs, that's not ok.  Actually,
> > I think it would take you a lot of work to get this to properly work on
> > the implementation side :)
> This bit of the documentation is indeed wrong (oops). These are members of the IIO
> class and so this is just a link (much like the event elements in input) created
> by making them in class iio with the device as a a parent. Again, application
> wise we aren't normally interested in aggregation so there is one (or more) of these
> per device.

Ok.

> > 
> > If event_lines need device nodes, then they should be real devices.
> (oops) The are ;)

Heh, that will not work, as Kay described :)

> > Actually, all of this looks like it needs to be a bus, not a class, if
> > you are having this many different types of things hanging off of them.
> > Have you thought about that instead?  It would make your code a lot
> > easier in the end.
> That is definitely an option. The reason we didn't is more to do with following
> the most similar current case (input) than any particular preference.

As Kay said, this should be a bus, and you should have devices attach to
them, be it virtual or not.  Or, you just tie into the hwmon interface
which makes it much easier for you overall.

> >> Again taking accel_x0 as example and serious liberties with ABI spec.
> >>
> >> What:		/sys/.../event_line[m]/accel_x0_thresh[_high|_low]
> >> Description:
> >> 		Event generated when accel_x0 passes a threshold in correction direction
> >> 		(or stays beyond one). If direction isn't specified, either triggers it.
> >> 		Note driver will assume last p events requested are enabled where p is
> >> 		however many it supports.  So if you want to be sure you have
> >> 		set what you think you have, check the contents of these. Drivers
> >> 		may have to buffer any parameters so that they are consistent when a
> >> 		given event type is enabled a future point (and not those for whatever
> >> 		alarm was previously enabled).
> >>
> >> What:		/sys/.../event_line[m]/accel_x0_roc[_high|_low]
> >> Description:
> >> 		Same as above but based on the first differential of the value.
> >>
> >>
> >> What:		/sys/.../event_line[m]/accel_x0[_thresh|_roc][_high|_low]_period
> >> Description:
> >> 		A period of time (microsecs) for which the condition must be broken
> >> 		before an interrupt is triggered. Applies to all alarms if type is not
> >> 		specified.
> >>
> >> What:		/sys/.../event_line[m]/accel_x0[_thresh|_roc][_high|_low]_value
> >> Description:
> >> 		The actual value of the threshold either in raw device units
> >> 		obtained by reverse application of scale and offfset to the
> >> 		acceleration in m/s^2.
> > 
> > Again, look at our existing apis, I think we already cover these types
> > of things, don't create new ones please.
> I am not aware of these. Could you direct me to the current api? Also note that these
> aren't the actual alarms, merely a means of enabling the relevant event on the related
> event character device. 

Hm, I thought we had an accelerator interface somewhere...

> >> What:		/sys/.../event_line[m]/free_fall
> >> Description:
> >> 		Common hardware event in accelerometers. Takes no parameters.
> > 
> > I know we have this one already, please use it.
> 
> Again, does it make sense to match api when the result is very different? The event
> infrastructure in IIO is much slimmer than that in input (this was one of the original
> reasons for not simply adding these things to input in the first place).  This particular
> example is a bit unusual in that the current api (hwmon/lis3lv02d.c for example) handles
> this via a dedicated freefall character device.  This doesn't really generalize to the more
> complex events handled here. To be honest this one doesn't really make sense in IIO's
> intended applications so it might be best to drop it entirely! 

Yeah, a dedicated char device doesn't seem to make much sense either,
but any unification you could find here would be nice.  Even if it means
the existing driver converts over to your new api :)

> > Again, don't bury devices.  Or if you are, use a bus, not a class,
> > that's the wrong classification.
> Cool, I'll look into making the change.  What we really have here
> is a single conceptual device using a pair or character interfaces.  Is a bus
> the right way to handle that?
> 
> How about the following under a bus (this is for a single physical chip).
> 
> device0
> event0 (for physical events)
> ringaccess0 (actual device from which data is read)
> ringevent0 (associated event interface for the ring  - ideally not aggregated with the event0)
> 
> and sometimes:
> trigger0
> 
> Is that best way to go.  Currently ringacces0 and ring event0 are
> grouped under ring_buffer0 but that was simply for the reason they are
> always matched.  

Yes, that would work, and make sense.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists