linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC] Add Input IOCTL for accelerometer devices
@ 2009-05-15 13:16 Felipe Balbi
  2009-05-15 18:06 ` Trilok Soni
  0 siblings, 1 reply; 22+ messages in thread
From: Felipe Balbi @ 2009-05-15 13:16 UTC (permalink / raw)
  To: linux-kernel; +Cc: Dmitry Torokhov

Hi all,

the following patch is just an idea to see how the community feels about
it. Considering accelerometer devices, you might have different use
cases for it while running different applications. You could be using it
for screen rotation in one case but when opening a game, you could use
it as a game controller by turning the device side-by-side.

Thinking about that, I decided it would be nice to have some parameters
configured at runtime in order to be able to use the accelerometer on
different use cases.

The following proposal (really draft) tries to add a new IOCTL on input
layer such as userland could use that for passing proper parameters to
accelerometer drivers.

ps: the patch is in really early stage, I'm guessing the best would be
to, as force feedback devices, provide proper registration for an
accelerometer device.

diff --git a/include/linux/input.h b/include/linux/input.h
index adc1332..c3c9e82 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -55,6 +55,26 @@ struct input_absinfo {
        __s32 flat;
 };
 
+#define INPUT_ACCEL_IRQ_NONE   0
+#define INPUT_ACCEL_IRQ_HIGH   BIT(0)
+#define INPUT_ACCEL_IRQ_LOW    BIT(1)
+#define INPUT_ACCEL_IRQ_LATCH  BIT(2)
+
+struct input_accelinfo {
+       __u32 threshold;
+       __u32 duration;
+
+       /* the following 4 fields configure how the irqs will be generated:
+        * - High Event
+        * - Low Event
+        * - Latch irq request
+        */
+       __u8 x_irq;
+       __u8 y_irq;
+       __u8 z_irq;
+       __u8 click_irq;
+};
+
 #define EVIOCGVERSION          _IOR('E', 0x01, int)                    /* get driver version */
 #define EVIOCGID               _IOR('E', 0x02, struct input_id)        /* get device ID */
 #define EVIOCGREP              _IOR('E', 0x03, int[2])                 /* get repeat settings */
@@ -81,6 +101,8 @@ struct input_absinfo {
 
 #define EVIOCGRAB              _IOW('E', 0x90, int)                    /* Grab/Release device */
 
+#define EVIOCACCELINFO         _IOW('E', 0x91, struct input_accelinfo) /* set accelerometer's parameters */
+
 /*
  * Event types
  */
@@ -975,6 +997,8 @@ struct ff_effect {
 #include <linux/timer.h>
 #include <linux/mod_devicetable.h>
 
+struct input_accelinfo;
+
 /**
  * struct input_dev - represents an input device
  * @name: name of the device
@@ -1069,6 +1093,7 @@ struct input_dev {
        void *keycode;
        int (*setkeycode)(struct input_dev *dev, int scancode, int keycode);
        int (*getkeycode)(struct input_dev *dev, int scancode, int *keycode);
+       int (*set_accelinfo)(struct input_dev *dev, struct input_accelinfo *info);
 
        struct ff_device *ff;
 


-- 
balbi

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-15 13:16 [RFC] Add Input IOCTL for accelerometer devices Felipe Balbi
@ 2009-05-15 18:06 ` Trilok Soni
  2009-05-15 19:30   ` Mohamed Ikbel Boulabiar
  0 siblings, 1 reply; 22+ messages in thread
From: Trilok Soni @ 2009-05-15 18:06 UTC (permalink / raw)
  To: felipe.balbi; +Cc: linux-kernel, Dmitry Torokhov, linux-input, Jonathan Cameron

Hi Felipe,

Adding linux-input and Jonathan, so not deleting any lines from this e-mail.

On Fri, May 15, 2009 at 6:46 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> Hi all,
>
> the following patch is just an idea to see how the community feels about
> it. Considering accelerometer devices, you might have different use
> cases for it while running different applications. You could be using it
> for screen rotation in one case but when opening a game, you could use
> it as a game controller by turning the device side-by-side.

There was one proposal from Jonathan called Industrial IO patchset
which tried to address these sensor devices. Please grep in your
linux-kernel archieve. I believe there are accelerometer drivers under
drivers/hwmon.

>
> Thinking about that, I decided it would be nice to have some parameters
> configured at runtime in order to be able to use the accelerometer on
> different use cases.
>
> The following proposal (really draft) tries to add a new IOCTL on input
> layer such as userland could use that for passing proper parameters to
> accelerometer drivers.
>
> ps: the patch is in really early stage, I'm guessing the best would be
> to, as force feedback devices, provide proper registration for an
> accelerometer device.
>
> diff --git a/include/linux/input.h b/include/linux/input.h
> index adc1332..c3c9e82 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -55,6 +55,26 @@ struct input_absinfo {
>        __s32 flat;
>  };
>
> +#define INPUT_ACCEL_IRQ_NONE   0
> +#define INPUT_ACCEL_IRQ_HIGH   BIT(0)
> +#define INPUT_ACCEL_IRQ_LOW    BIT(1)
> +#define INPUT_ACCEL_IRQ_LATCH  BIT(2)
> +
> +struct input_accelinfo {
> +       __u32 threshold;
> +       __u32 duration;
> +
> +       /* the following 4 fields configure how the irqs will be generated:
> +        * - High Event
> +        * - Low Event
> +        * - Latch irq request
> +        */
> +       __u8 x_irq;
> +       __u8 y_irq;
> +       __u8 z_irq;
> +       __u8 click_irq;
> +};
> +
>  #define EVIOCGVERSION          _IOR('E', 0x01, int)                    /* get driver version */
>  #define EVIOCGID               _IOR('E', 0x02, struct input_id)        /* get device ID */
>  #define EVIOCGREP              _IOR('E', 0x03, int[2])                 /* get repeat settings */
> @@ -81,6 +101,8 @@ struct input_absinfo {
>
>  #define EVIOCGRAB              _IOW('E', 0x90, int)                    /* Grab/Release device */
>
> +#define EVIOCACCELINFO         _IOW('E', 0x91, struct input_accelinfo) /* set accelerometer's parameters */
> +
>  /*
>  * Event types
>  */
> @@ -975,6 +997,8 @@ struct ff_effect {
>  #include <linux/timer.h>
>  #include <linux/mod_devicetable.h>
>
> +struct input_accelinfo;
> +
>  /**
>  * struct input_dev - represents an input device
>  * @name: name of the device
> @@ -1069,6 +1093,7 @@ struct input_dev {
>        void *keycode;
>        int (*setkeycode)(struct input_dev *dev, int scancode, int keycode);
>        int (*getkeycode)(struct input_dev *dev, int scancode, int *keycode);
> +       int (*set_accelinfo)(struct input_dev *dev, struct input_accelinfo *info);
>
>        struct ff_device *ff;
>
>
>
> --
> balbi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-15 18:06 ` Trilok Soni
@ 2009-05-15 19:30   ` Mohamed Ikbel Boulabiar
  2009-05-15 20:02     ` Felipe Balbi
  0 siblings, 1 reply; 22+ messages in thread
From: Mohamed Ikbel Boulabiar @ 2009-05-15 19:30 UTC (permalink / raw)
  To: Trilok Soni
  Cc: felipe.balbi, linux-kernel, Dmitry Torokhov, linux-input,
	Jonathan Cameron

Hi all,

I am really interested about that.

But I want to know more about the device, its type, name, ...
The device isn't HID (Human Interface Device) ? If so, we should
rethink adding such thing but modify/use hid-input instead.

Because, I have an accelerometer phidget device and it is HID.
Handling should be the same.



On Fri, May 15, 2009 at 7:06 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
> Hi Felipe,
>
> Adding linux-input and Jonathan, so not deleting any lines from this e-mail.
>
> On Fri, May 15, 2009 at 6:46 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
>> Hi all,
>>
>> the following patch is just an idea to see how the community feels about
>> it. Considering accelerometer devices, you might have different use
>> cases for it while running different applications. You could be using it
>> for screen rotation in one case but when opening a game, you could use
>> it as a game controller by turning the device side-by-side.
>
> There was one proposal from Jonathan called Industrial IO patchset
> which tried to address these sensor devices. Please grep in your
> linux-kernel archieve. I believe there are accelerometer drivers under
> drivers/hwmon.
>
>>
>> Thinking about that, I decided it would be nice to have some parameters
>> configured at runtime in order to be able to use the accelerometer on
>> different use cases.
>>
>> The following proposal (really draft) tries to add a new IOCTL on input
>> layer such as userland could use that for passing proper parameters to
>> accelerometer drivers.
>>
>> ps: the patch is in really early stage, I'm guessing the best would be
>> to, as force feedback devices, provide proper registration for an
>> accelerometer device.
>>
>> diff --git a/include/linux/input.h b/include/linux/input.h
>> index adc1332..c3c9e82 100644
>> --- a/include/linux/input.h
>> +++ b/include/linux/input.h
>> @@ -55,6 +55,26 @@ struct input_absinfo {
>>        __s32 flat;
>>  };
>>
>> +#define INPUT_ACCEL_IRQ_NONE   0
>> +#define INPUT_ACCEL_IRQ_HIGH   BIT(0)
>> +#define INPUT_ACCEL_IRQ_LOW    BIT(1)
>> +#define INPUT_ACCEL_IRQ_LATCH  BIT(2)
>> +
>> +struct input_accelinfo {
>> +       __u32 threshold;
>> +       __u32 duration;
>> +
>> +       /* the following 4 fields configure how the irqs will be generated:
>> +        * - High Event
>> +        * - Low Event
>> +        * - Latch irq request
>> +        */
>> +       __u8 x_irq;
>> +       __u8 y_irq;
>> +       __u8 z_irq;
>> +       __u8 click_irq;
>> +};
>> +
>>  #define EVIOCGVERSION          _IOR('E', 0x01, int)                    /* get driver version */
>>  #define EVIOCGID               _IOR('E', 0x02, struct input_id)        /* get device ID */
>>  #define EVIOCGREP              _IOR('E', 0x03, int[2])                 /* get repeat settings */
>> @@ -81,6 +101,8 @@ struct input_absinfo {
>>
>>  #define EVIOCGRAB              _IOW('E', 0x90, int)                    /* Grab/Release device */
>>
>> +#define EVIOCACCELINFO         _IOW('E', 0x91, struct input_accelinfo) /* set accelerometer's parameters */
>> +
>>  /*
>>  * Event types
>>  */
>> @@ -975,6 +997,8 @@ struct ff_effect {
>>  #include <linux/timer.h>
>>  #include <linux/mod_devicetable.h>
>>
>> +struct input_accelinfo;
>> +
>>  /**
>>  * struct input_dev - represents an input device
>>  * @name: name of the device
>> @@ -1069,6 +1093,7 @@ struct input_dev {
>>        void *keycode;
>>        int (*setkeycode)(struct input_dev *dev, int scancode, int keycode);
>>        int (*getkeycode)(struct input_dev *dev, int scancode, int *keycode);
>> +       int (*set_accelinfo)(struct input_dev *dev, struct input_accelinfo *info);
>>
>>        struct ff_device *ff;
>>
>>
>>
>> --
>> balbi
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
>
>
> --
> ---Trilok Soni
> http://triloksoni.wordpress.com
> http://www.linkedin.com/in/triloksoni
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-15 19:30   ` Mohamed Ikbel Boulabiar
@ 2009-05-15 20:02     ` Felipe Balbi
  2009-05-16  4:19       ` Mohamed Ikbel Boulabiar
  2009-05-18  7:45       ` Kim Kyuwon
  0 siblings, 2 replies; 22+ messages in thread
From: Felipe Balbi @ 2009-05-15 20:02 UTC (permalink / raw)
  To: ext Mohamed Ikbel Boulabiar
  Cc: Trilok Soni, Balbi Felipe (Nokia-D/Helsinki),
	linux-kernel, Dmitry Torokhov, linux-input, Jonathan Cameron

Hi all,


On Fri, May 15, 2009 at 09:30:41PM +0200, ext Mohamed Ikbel Boulabiar wrote:
> I am really interested about that.
> 
> But I want to know more about the device, its type, name, ...
> The device isn't HID (Human Interface Device) ? If so, we should
> rethink adding such thing but modify/use hid-input instead.
> 
> Because, I have an accelerometer phidget device and it is HID.
> Handling should be the same.

Yeah, let's try to define the best way to expose accelerometers with
linux kernel and avoid a sysfs hell. Better sooner than later.

> On Fri, May 15, 2009 at 7:06 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
> > Hi Felipe,
> >
> > Adding linux-input and Jonathan, so not deleting any lines from this e-mail.
> >
> > On Fri, May 15, 2009 at 6:46 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> >> Hi all,
> >>
> >> the following patch is just an idea to see how the community feels about
> >> it. Considering accelerometer devices, you might have different use
> >> cases for it while running different applications. You could be using it
> >> for screen rotation in one case but when opening a game, you could use
> >> it as a game controller by turning the device side-by-side.
> >
> > There was one proposal from Jonathan called Industrial IO patchset
> > which tried to address these sensor devices. Please grep in your
> > linux-kernel archieve. I believe there are accelerometer drivers under
> > drivers/hwmon.

The problem is that it doesn't really seem to me that all accelerometers
will be doing hw monitoring. The ones used in laptops, for sure, trying
to prevent the hd from drying during a fall. But imagine the
accelerometers used in, say, wii-mote, or cellphones, or such stuff ?

Say we wanna use the accelerometer for both screen rotation and gaming,
that device isn't doing hw monitoring and still we _do_ want to set
different thresholds and irq requests/types for different use cases,
right ?

I'll grep around the Industrial IO patchset if that's supposed to a
'sensor fw' sorta thing. Hope it does report the devices as input
devices. I'm pretty sure we're gonna fall into similar issues when
dealing with magnetometers, for instance, what's the best way to expose
those device with current linux frameworks ? It's not really user input
data...

Well, let's see where this discussion takes us. Willing to hear from
Jonatha, Dmitry and all of you guys

-- 
balbi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-15 20:02     ` Felipe Balbi
@ 2009-05-16  4:19       ` Mohamed Ikbel Boulabiar
  2009-05-16 12:50         ` Felipe Balbi
  2009-05-18  7:45       ` Kim Kyuwon
  1 sibling, 1 reply; 22+ messages in thread
From: Mohamed Ikbel Boulabiar @ 2009-05-16  4:19 UTC (permalink / raw)
  To: felipe.balbi
  Cc: Trilok Soni, linux-kernel, Dmitry Torokhov, linux-input,
	Jonathan Cameron

On Fri, May 15, 2009 at 9:02 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:

> Yeah, let's try to define the best way to expose accelerometers with
> linux kernel and avoid a sysfs hell. Better sooner than later.

isn't the best way to add a thing like the one for battery for any input device
/proc/acpi/battery/CMB0/state
or even in /sys/input/devices/XXXX

and let any userspace application do what she wants with information ?

> The problem is that it doesn't really seem to me that all accelerometers
> will be doing hw monitoring. The ones used in laptops, for sure, trying
> to prevent the hd from drying during a fall. But imagine the
> accelerometers used in, say, wii-mote, or cellphones, or such stuff ?

Many of them are HID devices (99%), so can be treated the same way
according the standard.

> Say we wanna use the accelerometer for both screen rotation and gaming,
> that device isn't doing hw monitoring and still we _do_ want to set
> different thresholds and irq requests/types for different use cases,
> right ?

Another reason for preparing the interface I just cited.

> I'll grep around the Industrial IO patchset if that's supposed to a
> 'sensor fw' sorta thing. Hope it does report the devices as input
> devices. I'm pretty sure we're gonna fall into similar issues when
> dealing with magnetometers, for instance, what's the best way to expose
> those device with current linux frameworks ? It's not really user input
> data...
>
> Well, let's see where this discussion takes us. Willing to hear from
> Jonatha, Dmitry and all of you guys

Yes guys, I also wait for your comments :)


ikbel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-16  4:19       ` Mohamed Ikbel Boulabiar
@ 2009-05-16 12:50         ` Felipe Balbi
  0 siblings, 0 replies; 22+ messages in thread
From: Felipe Balbi @ 2009-05-16 12:50 UTC (permalink / raw)
  To: ext Mohamed Ikbel Boulabiar
  Cc: Balbi Felipe (Nokia-D/Helsinki),
	Trilok Soni, linux-kernel, Dmitry Torokhov, linux-input,
	Jonathan Cameron

Hi,

On Sat, May 16, 2009 at 06:19:33AM +0200, ext Mohamed Ikbel Boulabiar wrote:
> On Fri, May 15, 2009 at 9:02 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> 
> > Yeah, let's try to define the best way to expose accelerometers with
> > linux kernel and avoid a sysfs hell. Better sooner than later.
> 
> isn't the best way to add a thing like the one for battery for any input device
> /proc/acpi/battery/CMB0/state
> or even in /sys/input/devices/XXXX
> 
> and let any userspace application do what she wants with information ?

This case is the other way around. We are trying to set proper
thresholding and other parameters depending on the user application
(gaming, screen rotation, etc).

> > Say we wanna use the accelerometer for both screen rotation and gaming,
> > that device isn't doing hw monitoring and still we _do_ want to set
> > different thresholds and irq requests/types for different use cases,
> > right ?
> 
> Another reason for preparing the interface I just cited.

Yes, and the interface would be an input ioctl so that userland would
give us the parameters and the driver just sets them to proper
registers.

-- 
balbi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-15 20:02     ` Felipe Balbi
  2009-05-16  4:19       ` Mohamed Ikbel Boulabiar
@ 2009-05-18  7:45       ` Kim Kyuwon
  2009-05-18  9:12         ` Felipe Balbi
  1 sibling, 1 reply; 22+ messages in thread
From: Kim Kyuwon @ 2009-05-18  7:45 UTC (permalink / raw)
  To: felipe.balbi
  Cc: ext Mohamed Ikbel Boulabiar, Trilok Soni, linux-kernel,
	Dmitry Torokhov, linux-input, Jonathan Cameron, Kim Kyuwon,
	Kyungmin Park, Jean Delvare

Hi All,

It's very nice of Felipe to make an issue of accelerometers in Linux
kernel again.
Before further discussions, we'd better see previous threads about
accelerometer.

http://lkml.org/lkml/2008/5/20/135
http://lkml.org/lkml/2008/12/1/156
--

I have considered how I can add my accelerometer driver into the Linux
kernel nicely for a few months.

As Trilok said, there are many accelerometer drivers under
drivers/hwmon. So I first tried to add my driver as hwmon, but Jean
Delvare didn't agree this idea. Please refer to the following URL:
http://lists.lm-sensors.org/pipermail/lm-sensors/2009-April/025686.html

And from the next URL, Dmitry don't think it is great idea to add
accelerometer as Input system.
http://lkml.org/lkml/2008/5/27/283

On Sat, May 16, 2009 at 5:02 AM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> Hi all,
>
>
> On Fri, May 15, 2009 at 09:30:41PM +0200, ext Mohamed Ikbel Boulabiar wrote:
>> I am really interested about that.
>>
>> But I want to know more about the device, its type, name, ...
>> The device isn't HID (Human Interface Device) ? If so, we should
>> rethink adding such thing but modify/use hid-input instead.
>>
>> Because, I have an accelerometer phidget device and it is HID.
>> Handling should be the same.
>
> Yeah, let's try to define the best way to expose accelerometers with
> linux kernel and avoid a sysfs hell. Better sooner than later.

Felipe,
Can I ask why did you say "avoid a sysfs hell"?. I have thought Kernel
developers prefer sysfs to IOCTL lately.

>> On Fri, May 15, 2009 at 7:06 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
>> > Hi Felipe,
>> >
>> > Adding linux-input and Jonathan, so not deleting any lines from this e-mail.
>> >
>> > On Fri, May 15, 2009 at 6:46 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
>> >> Hi all,
>> >>
>> >> the following patch is just an idea to see how the community feels about
>> >> it. Considering accelerometer devices, you might have different use
>> >> cases for it while running different applications. You could be using it
>> >> for screen rotation in one case but when opening a game, you could use
>> >> it as a game controller by turning the device side-by-side.
>> >
>> > There was one proposal from Jonathan called Industrial IO patchset
>> > which tried to address these sensor devices. Please grep in your
>> > linux-kernel archieve. I believe there are accelerometer drivers under
>> > drivers/hwmon.
>
> The problem is that it doesn't really seem to me that all accelerometers
> will be doing hw monitoring. The ones used in laptops, for sure, trying
> to prevent the hd from drying during a fall. But imagine the
> accelerometers used in, say, wii-mote, or cellphones, or such stuff ?
>
> Say we wanna use the accelerometer for both screen rotation and gaming,
> that device isn't doing hw monitoring and still we _do_ want to set
> different thresholds and irq requests/types for different use cases,
> right ?

Yes, I agree that accelerometer needs new interface. However setting
parameters of accelerometer is very different from devices and device
specific. Until now, I met two accelerometer, SMB380 from
bosch-sensortec and KXSD9 from Kionix. As far as I know, these two
accelerometers are quite different from each other and existing
accelerometer drivers located /driver/hwmon/ in current Linux kernel.
Thus I think sysfs interface (including hwmon-sysfs) is the best
solution for setting various parameters of accelerometer..

On the other hand, accelerometers are mostly used as Input device in
these days. Most APIs(input_allocate_device,
input_allocate_polled_device, ...) and macros(ABS_X,  ABS_Y, ...)of
Input subsystem are useful to accelerometer too. If we create another
APIs and Macros for accelerometers, I think It's another duplicate
work and result.

It seems like Dmitry concerns input_dev becomes too big with hundreds
of sensors.(right?) However, Market trend makes us consider
accelerometer as an input device now. I'm sure there is a good way to
add accelerometer input system without enlarging input_dev much.

In conclusion,
We need the inheritance concept in the object-oriented programming.
Accelerometer device sometimes can be hwmon device, sometimes input
device. So let accelerometer drivers use both APIs of hwmon and input
subsystems(hwmon_device_register, input_register_device,
input_register_polled_device). Acutally this is what many
accelerometer drivers in current Linux kernel are doing, so we don't
have to do much.

Let's
1) Introduce a new maintainer of accelerometer (Felipe?).
2) Move accelerometer drivers in current Linux kernel to /driver/accelerometer.
3) If we find the common functions of accelerometer or have idea about
new API or Macro, let's make at driver/accelerometer/acccelerometer.c,
input/linux/acccelerometer.h file or modify input.h little.
4) Add every new accelerometer into /driver/accelerometer.


Kyuwon (규원)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-18  7:45       ` Kim Kyuwon
@ 2009-05-18  9:12         ` Felipe Balbi
  2009-05-18 10:11           ` Jonathan Cameron
  2009-05-19  2:41           ` Kim Kyuwon
  0 siblings, 2 replies; 22+ messages in thread
From: Felipe Balbi @ 2009-05-18  9:12 UTC (permalink / raw)
  To: ext Kim Kyuwon
  Cc: Balbi Felipe (Nokia-D/Helsinki),
	ext Mohamed Ikbel Boulabiar, Trilok Soni, linux-kernel,
	Dmitry Torokhov, linux-input, Jonathan Cameron, Kim Kyuwon,
	Kyungmin Park, Jean Delvare

On Mon, May 18, 2009 at 09:45:55AM +0200, ext Kim Kyuwon wrote:
> Hi All,
> 
> It's very nice of Felipe to make an issue of accelerometers in Linux
> kernel again.
> Before further discussions, we'd better see previous threads about
> accelerometer.
> 
> http://lkml.org/lkml/2008/5/20/135
> http://lkml.org/lkml/2008/12/1/156
> --
> 
> I have considered how I can add my accelerometer driver into the Linux
> kernel nicely for a few months.
> 
> As Trilok said, there are many accelerometer drivers under
> drivers/hwmon. So I first tried to add my driver as hwmon, but Jean
> Delvare didn't agree this idea. Please refer to the following URL:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2009-April/025686.html
> 
> And from the next URL, Dmitry don't think it is great idea to add
> accelerometer as Input system.
> http://lkml.org/lkml/2008/5/27/283

Good links, I'll read them all :-)

Thanks

> On Sat, May 16, 2009 at 5:02 AM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> > Hi all,
> >
> >
> > On Fri, May 15, 2009 at 09:30:41PM +0200, ext Mohamed Ikbel Boulabiar wrote:
> >> I am really interested about that.
> >>
> >> But I want to know more about the device, its type, name, ...
> >> The device isn't HID (Human Interface Device) ? If so, we should
> >> rethink adding such thing but modify/use hid-input instead.
> >>
> >> Because, I have an accelerometer phidget device and it is HID.
> >> Handling should be the same.
> >
> > Yeah, let's try to define the best way to expose accelerometers with
> > linux kernel and avoid a sysfs hell. Better sooner than later.
> 
> Felipe,
> Can I ask why did you say "avoid a sysfs hell"?. I have thought Kernel
> developers prefer sysfs to IOCTL lately.

For sure sysfs is prefered, but I meant that without a proper
abstraction or definition of how to export the device, each device
driver write will export sysfs nodes as they want and that's really bad
since we create the 'userland interface'. If it's messed up from the
beginning, it's gonna be like that for ages.

> >> On Fri, May 15, 2009 at 7:06 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
> >> > Hi Felipe,
> >> >
> >> > Adding linux-input and Jonathan, so not deleting any lines from this e-mail.
> >> >
> >> > On Fri, May 15, 2009 at 6:46 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> >> >> Hi all,
> >> >>
> >> >> the following patch is just an idea to see how the community feels about
> >> >> it. Considering accelerometer devices, you might have different use
> >> >> cases for it while running different applications. You could be using it
> >> >> for screen rotation in one case but when opening a game, you could use
> >> >> it as a game controller by turning the device side-by-side.
> >> >
> >> > There was one proposal from Jonathan called Industrial IO patchset
> >> > which tried to address these sensor devices. Please grep in your
> >> > linux-kernel archieve. I believe there are accelerometer drivers under
> >> > drivers/hwmon.
> >
> > The problem is that it doesn't really seem to me that all accelerometers
> > will be doing hw monitoring. The ones used in laptops, for sure, trying
> > to prevent the hd from drying during a fall. But imagine the
> > accelerometers used in, say, wii-mote, or cellphones, or such stuff ?
> >
> > Say we wanna use the accelerometer for both screen rotation and gaming,
> > that device isn't doing hw monitoring and still we _do_ want to set
> > different thresholds and irq requests/types for different use cases,
> > right ?
> 
> Yes, I agree that accelerometer needs new interface. However setting
> parameters of accelerometer is very different from devices and device
> specific. Until now, I met two accelerometer, SMB380 from
> bosch-sensortec and KXSD9 from Kionix. As far as I know, these two
> accelerometers are quite different from each other and existing
> accelerometer drivers located /driver/hwmon/ in current Linux kernel.
> Thus I think sysfs interface (including hwmon-sysfs) is the best
> solution for setting various parameters of accelerometer..

what if you wanna use the accelerometer as joystick for gaming ? Imagine
a portable device...

> On the other hand, accelerometers are mostly used as Input device in
> these days. Most APIs(input_allocate_device,
> input_allocate_polled_device, ...) and macros(ABS_X,  ABS_Y, ...)of
> Input subsystem are useful to accelerometer too. If we create another
> APIs and Macros for accelerometers, I think It's another duplicate
> work and result.

for sure

> It seems like Dmitry concerns input_dev becomes too big with hundreds
> of sensors.(right?) However, Market trend makes us consider
> accelerometer as an input device now. I'm sure there is a good way to
> add accelerometer input system without enlarging input_dev much.
> 
> In conclusion,
> We need the inheritance concept in the object-oriented programming.
> Accelerometer device sometimes can be hwmon device, sometimes input
> device. So let accelerometer drivers use both APIs of hwmon and input
> subsystems(hwmon_device_register, input_register_device,
> input_register_polled_device). Acutally this is what many
> accelerometer drivers in current Linux kernel are doing, so we don't
> have to do much.
> 
> Let's
> 1) Introduce a new maintainer of accelerometer (Felipe?).
> 2) Move accelerometer drivers in current Linux kernel to /driver/accelerometer.
> 3) If we find the common functions of accelerometer or have idea about
> new API or Macro, let's make at driver/accelerometer/acccelerometer.c,
> input/linux/acccelerometer.h file or modify input.h little.
> 4) Add every new accelerometer into /driver/accelerometer.

How about extending these to several kinds of sensors ?? Why not having
a sensor framework that abstracts the creating of the input_dev for
accelerometer ? But then comes another question: what to do with
magnetometers, gyroscopes, etc etc ??

-- 
balbi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-18  9:12         ` Felipe Balbi
@ 2009-05-18 10:11           ` Jonathan Cameron
  2009-05-18 10:31             ` Felipe Balbi
  2009-05-19  6:32             ` Kim Kyuwon
  2009-05-19  2:41           ` Kim Kyuwon
  1 sibling, 2 replies; 22+ messages in thread
From: Jonathan Cameron @ 2009-05-18 10:11 UTC (permalink / raw)
  To: felipe.balbi
  Cc: ext Kim Kyuwon, ext Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kim Kyuwon,
	Kyungmin Park, Jean Delvare, Zhang, Xing Z, Gross, Mark,
	jketreno, Trisal, Kalhan

Hi All,

Firstly sorry for lack of response, my email filters hid this in a
folder I don't read often!

I've copied in a few other people I've discussed similar things with
in the past. (sorry if guys if you were getting this via the mailing
list anyway!)

To give you my background, I'm generally interested in sensors
attached to mote's (ultra small linux machines) which tend not to
have user interfaces as such and hence supporting these things for
input is for a matter of interest rather than requirements. 

Whether it makes sense to write unified drivers for my types of uses
and input is still an open question.

Felipe Balbi wrote:
> On Mon, May 18, 2009 at 09:45:55AM +0200, ext Kim Kyuwon wrote:
>> Hi All,
>>
>> It's very nice of Felipe to make an issue of accelerometers in Linux
>> kernel again.
>> Before further discussions, we'd better see previous threads about
>> accelerometer.
>>
>> http://lkml.org/lkml/2008/5/20/135
>> http://lkml.org/lkml/2008/12/1/156
>> --
>>
>> I have considered how I can add my accelerometer driver into the Linux
>> kernel nicely for a few months.
>>
>> As Trilok said, there are many accelerometer drivers under
>> drivers/hwmon. So I first tried to add my driver as hwmon, but Jean
>> Delvare didn't agree this idea. Please refer to the following URL:
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2009-April/025686.html
>>
>> And from the next URL, Dmitry don't think it is great idea to add
>> accelerometer as Input system.
>> http://lkml.org/lkml/2008/5/27/283
> 
> Good links, I'll read them all :-)
Enjoy ;)

If you are interested in iio, there's a git tree at
http://www.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git

and a draft of a white paper at:

www-sigproc.eng.cam.ac.uk/~jic23/iio.pdf

I'm just waiting to have a spare day to clean that up before posting more widely!
(all comments welcomed!)
> Thanks
> 
>> On Sat, May 16, 2009 at 5:02 AM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
>>> Hi all,
>>>
>>>
>>> On Fri, May 15, 2009 at 09:30:41PM +0200, ext Mohamed Ikbel Boulabiar wrote:
>>>> I am really interested about that.
>>>>
>>>> But I want to know more about the device, its type, name, ...
>>>> The device isn't HID (Human Interface Device) ? If so, we should
>>>> rethink adding such thing but modify/use hid-input instead.
>>>>
>>>> Because, I have an accelerometer phidget device and it is HID.
>>>> Handling should be the same.
>>> Yeah, let's try to define the best way to expose accelerometers with
>>> linux kernel and avoid a sysfs hell. Better sooner than later.
>> Felipe,
>> Can I ask why did you say "avoid a sysfs hell"?. I have thought Kernel
>> developers prefer sysfs to IOCTL lately.
> 
> For sure sysfs is prefered, but I meant that without a proper
> abstraction or definition of how to export the device, each device
> driver write will export sysfs nodes as they want and that's really bad
> since we create the 'userland interface'. If it's messed up from the
> beginning, it's gonna be like that for ages.
This was very much one of the thinks IIO was designed to address.
One thing to keep in mind is that the framework
was not intended to replace input / hwmon if they are the appropriate places
for a given driver. In fact one of the conclusions of the discussions linked
above was that, in cases where an accelerometer (or other sensor) serves
different purposes in different devices it may make sense to actually provide
more than one kernel driver. (obviously sharing code where appropriate.

There have been a few off list discussions about how to provide a 'query' type
function so as to allow very different sensors to be used transparently by user
space.  HID was suggested, but to my mind isn't necessarily the way to go. I'm
very much of the view that the kernel should expose enough parameters for userspace
to be able to figure out the 'calibration' type stuff.  This would be in a similar
fashion to that used by hwmon.  I'm not sure I've gotten in right in IIO.

If things are chip specific then it's simply a case of discussing naming / parameter
conventions on the relevant list and keeping definitions in driver. When they start
turning up on lots of chips then they get moved into a 'device type header'
> 
>>>> On Fri, May 15, 2009 at 7:06 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
>>>>> Hi Felipe,
>>>>>
>>>>> Adding linux-input and Jonathan, so not deleting any lines from this e-mail.
Thanks.
>>>>> On Fri, May 15, 2009 at 6:46 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> the following patch is just an idea to see how the community feels about
>>>>>> it. Considering accelerometer devices, you might have different use
>>>>>> cases for it while running different applications. You could be using it
>>>>>> for screen rotation in one case but when opening a game, you could use
>>>>>> it as a game controller by turning the device side-by-side.
>>>>> There was one proposal from Jonathan called Industrial IO patchset
>>>>> which tried to address these sensor devices. Please grep in your
>>>>> linux-kernel archieve. I believe there are accelerometer drivers under
>>>>> drivers/hwmon.
>>> The problem is that it doesn't really seem to me that all accelerometers
>>> will be doing hw monitoring. The ones used in laptops, for sure, trying
>>> to prevent the hd from drying during a fall. But imagine the
>>> accelerometers used in, say, wii-mote, or cellphones, or such stuff ?
>>>
>>> Say we wanna use the accelerometer for both screen rotation and gaming,
>>> that device isn't doing hw monitoring and still we _do_ want to set
>>> different thresholds and irq requests/types for different use cases,
>>> right ?
>> Yes, I agree that accelerometer needs new interface. However setting
>> parameters of accelerometer is very different from devices and device
>> specific. Until now, I met two accelerometer, SMB380 from
>> bosch-sensortec and KXSD9 from Kionix.
I also have a KXSD9 and there is limited support for it in IIO. It's currently
acting as my example of a minimalist driver (hence I haven't done event handling
or buffering for it yet) - if you want to know how this would work, take a look
at the lis3l02dq accelerometer driver.
> As far as I know, these two
>> accelerometers are quite different from each other and existing
>> accelerometer drivers located /driver/hwmon/ in current Linux kernel.
>> Thus I think sysfs interface (including hwmon-sysfs) is the best
>> solution for setting various parameters of accelerometer..
> 
> what if you wanna use the accelerometer as joystick for gaming ? Imagine
> a portable device...
One option that we agreed to postpone for now in IIO was to use in kernel
interfaces to allow a single underlying layer to ship stuff up to input
if appropriate. It's not something I've looked at a lot, but should be
possible.  As stated above a decision on whether to do this was left 
for a later day in the original lkml discussions.

> 
>> On the other hand, accelerometers are mostly used as Input device in
>> these days.
I wouldn't necessarily say that, though I'll admit that's a big use case.

 Most APIs(input_allocate_device,
>> input_allocate_polled_device, ...) and macros(ABS_X,  ABS_Y, ...)of
>> Input subsystem are useful to accelerometer too. If we create another
>> APIs and Macros for accelerometers, I think It's another duplicate
>> work and result.
beware of the fact that x,y,z aren't exactly cleanly defined for a given
sensor!
> 
> for sure
Agreed.  If you know a given accelerometer is only going to be used for
input then that's where the driver belongs.  However, these chips are
generally capable of a lot more and it tends to be annoying specific.
Take for example things like calibration offsets, and for the really fun
cases on chip event driven ring buffers?  These really don't fit into
your classic input cases.
> 
>> It seems like Dmitry concerns input_dev becomes too big with hundreds
>> of sensors.(right?) However, Market trend makes us consider
>> accelerometer as an input device now. I'm sure there is a good way to
>> add accelerometer input system without enlarging input_dev much.
Optimist ;)
>> In conclusion,
>> We need the inheritance concept in the object-oriented programming.
>> Accelerometer device sometimes can be hwmon device, sometimes input
>> device.
I'd also argue the problem here is that you are going to end up with a
large class of similar devices.  If you start directly adding accelerometers
to Input then the same arguement applies to magnetometers, bend sensors,
gyroscopes etc.  Also beware that many accelerometers are going to be wired
up to adc's (rather than providing digital outputs) so you'll need some
framework to specify this connectivity. (open area in IIO to and the moment).

>So let accelerometer drivers use both APIs of hwmon and input
>> subsystems(hwmon_device_register, input_register_device,
>> input_register_polled_device). Acutally this is what many
>> accelerometer drivers in current Linux kernel are doing, so we don't
>> have to do much.
>>
>> Let's
>> 1) Introduce a new maintainer of accelerometer (Felipe?).
>> 2) Move accelerometer drivers in current Linux kernel to /driver/accelerometer.
>> 3) If we find the common functions of accelerometer or have idea about
>> new API or Macro, let's make at driver/accelerometer/acccelerometer.c,
>> input/linux/acccelerometer.h file or modify input.h little.
>> 4) Add every new accelerometer into /driver/accelerometer..
 > How about extending these to several kinds of sensors ??
Yup, lots out there - and lots of them don't fit into your nice clean categories.
 Why not having
> a sensor framework that abstracts the creating of the input_dev for
> accelerometer ?
In my view too limited.  (which is where IIO came from in the first place!)
Just because their primary use in your applications is input doesn't mean
that's going to be the case in future.
 But then comes another question: what to do with
> magnetometers, gyroscopes, etc etc ?
The really big and weird and wonderful option is to include general purpose
ADC's.  The really fun bit is that a lot of these devices are made up of
a number of different sensing elements.  So if you have gyro's / accelerometer
/ magnetomers you can have a crack at full orientation / 3d position, but
that's a case of a lot of heavyweight algorithms running in userspace rather
than the kernel.
(oops should have read to end of email before writing earlier bit) 

Other than the focus being a bit more on input that the data capture direction of
the original discussions that led to IIO we are rehashing much the same arguments.

For info, the reason IIO hasn't gone anywhere is that no one who is interested has
had time for reviewing the code. (Last post to lkml got one reply and the poster
in question didn't reply to my response).  If anyone is interested in assisting
with that project then please get in touch!

Jonathan


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-18 10:11           ` Jonathan Cameron
@ 2009-05-18 10:31             ` Felipe Balbi
  2009-05-18 11:37               ` Jonathan Cameron
  2009-05-19  6:32             ` Kim Kyuwon
  1 sibling, 1 reply; 22+ messages in thread
From: Felipe Balbi @ 2009-05-18 10:31 UTC (permalink / raw)
  To: ext Jonathan Cameron
  Cc: Balbi Felipe (Nokia-D/Helsinki),
	ext Kim Kyuwon, ext Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kim Kyuwon,
	Kyungmin Park, Jean Delvare, Zhang, Xing Z, Gross, Mark,
	jketreno, Trisal, Kalhan

Hi,

On Mon, May 18, 2009 at 12:11:23PM +0200, ext Jonathan Cameron wrote:
> Hi All,
> 
> Firstly sorry for lack of response, my email filters hid this in a
> folder I don't read often!
> 
> I've copied in a few other people I've discussed similar things with
> in the past. (sorry if guys if you were getting this via the mailing
> list anyway!)
> 
> To give you my background, I'm generally interested in sensors
> attached to mote's (ultra small linux machines) which tend not to
> have user interfaces as such and hence supporting these things for
> input is for a matter of interest rather than requirements. 

You're right here...

> Whether it makes sense to write unified drivers for my types of uses
> and input is still an open question.

I guess iio could provide 'links' to other subsystems where applicable.

> If you are interested in iio, there's a git tree at
> http://www.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git

cloning :-p

> and a draft of a white paper at:
> 
> www-sigproc.eng.cam.ac.uk/~jic23/iio.pdf

Printed

> This was very much one of the thinks IIO was designed to address.
> One thing to keep in mind is that the framework
> was not intended to replace input / hwmon if they are the appropriate places
> for a given driver. In fact one of the conclusions of the discussions linked
> above was that, in cases where an accelerometer (or other sensor) serves
> different purposes in different devices it may make sense to actually provide
> more than one kernel driver. (obviously sharing code where appropriate.

why not one kernel driver exporting the device through different
subsystems ?? Otherwise we'd need to fake it's a multifunction device,
right ?!?

> There have been a few off list discussions about how to provide a 'query' type
> function so as to allow very different sensors to be used transparently by user
> space.  HID was suggested, but to my mind isn't necessarily the way to go. I'm
> very much of the view that the kernel should expose enough parameters for userspace
> to be able to figure out the 'calibration' type stuff.  This would be in a similar
> fashion to that used by hwmon.  I'm not sure I've gotten in right in IIO.
> 
> If things are chip specific then it's simply a case of discussing naming / parameter
> conventions on the relevant list and keeping definitions in driver. When they start
> turning up on lots of chips then they get moved into a 'device type header'

right

> Agreed.  If you know a given accelerometer is only going to be used for
> input then that's where the driver belongs.  However, these chips are
> generally capable of a lot more and it tends to be annoying specific.
> Take for example things like calibration offsets, and for the really fun
> cases on chip event driven ring buffers?  These really don't fit into
> your classic input cases.

but that'll never be the case. It can be used as input only in my device
but what about yours ? That's why we need ways to make the driver as
generic as possible so it can be used for several applications.

> I'd also argue the problem here is that you are going to end up with a
> large class of similar devices.  If you start directly adding accelerometers
> to Input then the same arguement applies to magnetometers, bend sensors,
> gyroscopes etc.  Also beware that many accelerometers are going to be wired
> up to adc's (rather than providing digital outputs) so you'll need some
> framework to specify this connectivity. (open area in IIO to and the moment).

sure, I agree with you here.

> >So let accelerometer drivers use both APIs of hwmon and input
> >> subsystems(hwmon_device_register, input_register_device,
> >> input_register_polled_device). Acutally this is what many
> >> accelerometer drivers in current Linux kernel are doing, so we don't
> >> have to do much.
> >>
> >> Let's
> >> 1) Introduce a new maintainer of accelerometer (Felipe?).
> >> 2) Move accelerometer drivers in current Linux kernel to /driver/accelerometer.
> >> 3) If we find the common functions of accelerometer or have idea about
> >> new API or Macro, let's make at driver/accelerometer/acccelerometer.c,
> >> input/linux/acccelerometer.h file or modify input.h little.
> >> 4) Add every new accelerometer into /driver/accelerometer..
>  > How about extending these to several kinds of sensors ??
> Yup, lots out there - and lots of them don't fit into your nice clean categories.
>  Why not having
> > a sensor framework that abstracts the creating of the input_dev for
> > accelerometer ?
> In my view too limited.  (which is where IIO came from in the first place!)
> Just because their primary use in your applications is input doesn't mean
> that's going to be the case in future.

well, I'm thinking mostly about accelerometers, mostly. But same comment
applies to magnetometers, those can't be exported through input, that
one doesn't make sense at all, so how to export it ? and when that's
defined, make iio export helper functions for registering such devices..

-- 
balbi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-18 10:31             ` Felipe Balbi
@ 2009-05-18 11:37               ` Jonathan Cameron
  0 siblings, 0 replies; 22+ messages in thread
From: Jonathan Cameron @ 2009-05-18 11:37 UTC (permalink / raw)
  To: felipe.balbi
  Cc: ext Kim Kyuwon, ext Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kim Kyuwon,
	Kyungmin Park, Jean Delvare, Zhang, Xing Z, Gross, Mark,
	jketreno, Trisal, Kalhan

Felipe Balbi wrote:
> Hi,
> 
> On Mon, May 18, 2009 at 12:11:23PM +0200, ext Jonathan Cameron wrote:
>> Hi All,
>>
>> Firstly sorry for lack of response, my email filters hid this in a
>> folder I don't read often!
>>
>> I've copied in a few other people I've discussed similar things with
>> in the past. (sorry if guys if you were getting this via the mailing
>> list anyway!)
>>
>> To give you my background, I'm generally interested in sensors
>> attached to mote's (ultra small linux machines) which tend not to
>> have user interfaces as such and hence supporting these things for
>> input is for a matter of interest rather than requirements. 
> 
> You're right here...
> 
>> Whether it makes sense to write unified drivers for my types of uses
>> and input is still an open question.
> 
> I guess iio could provide 'links' to other subsystems where applicable.
> 
>> If you are interested in iio, there's a git tree at
>> http://www.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> 
> cloning :-p
Not entirely sure that's up to date. I'll check, though I'm going
top be out of the office for the next couple of days.

I'll regenerate the tree sometime next week as it's gotten to the point
where there are lots of minor fixes to code that was never reviewed
in the first place. As a temporary stop gap, I've pushed my development
tree to the 'mess' branch.
If you want to see it in used in anger, take a look at the imote2-linux
tree:
http://www.kernel.org/pub/scm/linux/kernel/git/jic23/imote2.git
which is using the current version up to a few comment clean ups.
(board config as relevant is in arch/arm/mach-pxa/imote2.c)
Currently that's my only user, but the userbase of that have been
doing a fine job of finding bugs!

>> and a draft of a white paper at:
>>
>> www-sigproc.eng.cam.ac.uk/~jic23/iio.pdf
> 
> Printed
Enjoy the typo's (really ought to have spell checked that ;)
> 
>> This was very much one of the thinks IIO was designed to address.
>> One thing to keep in mind is that the framework
>> was not intended to replace input / hwmon if they are the appropriate places
>> for a given driver. In fact one of the conclusions of the discussions linked
>> above was that, in cases where an accelerometer (or other sensor) serves
>> different purposes in different devices it may make sense to actually provide
>> more than one kernel driver. (obviously sharing code where appropriate.
> 
> why not one kernel driver exporting the device through different
> subsystems ?? Otherwise we'd need to fake it's a multifunction device,
> right ?!?
I'm actually beginning to wonder if going down that route might not be the right
way to go.  Some elements of the IIO approach already effectively do this. For
example the lis3l02dq driver provides an iio:device which handles stuff like the
general interrupts (threshold detection etc) and an iio:trigger that is driven from
the data ready signal (allows you to trigger other sensors at the 'same' time as
the accelerometer).  The other option is to have drivers with a lot of Kconfig
options.  We don't want to require input / hwmon support be built into the kernel
if they aren't relevant to the given platform.
>> There have been a few off list discussions about how to provide a 'query' type
>> function so as to allow very different sensors to be used transparently by user
>> space.  HID was suggested, but to my mind isn't necessarily the way to go. I'm
>> very much of the view that the kernel should expose enough parameters for userspace
>> to be able to figure out the 'calibration' type stuff.  This would be in a similar
>> fashion to that used by hwmon.  I'm not sure I've gotten in right in IIO.
>>
>> If things are chip specific then it's simply a case of discussing naming / parameter
>> conventions on the relevant list and keeping definitions in driver. When they start
>> turning up on lots of chips then they get moved into a 'device type header'
> 
> right
> 
>> Agreed.  If you know a given accelerometer is only going to be used for
>> input then that's where the driver belongs.  However, these chips are
>> generally capable of a lot more and it tends to be annoying specific.
>> Take for example things like calibration offsets, and for the really fun
>> cases on chip event driven ring buffers?  These really don't fit into
>> your classic input cases.
> 
> but that'll never be the case. It can be used as input only in my device
> but what about yours ? That's why we need ways to make the driver as
> generic as possible so it can be used for several applications.

Yes, I entirely agree.

>> I'd also argue the problem here is that you are going to end up with a
>> large class of similar devices.  If you start directly adding accelerometers
>> to Input then the same arguement applies to magnetometers, bend sensors,
>> gyroscopes etc.  Also beware that many accelerometers are going to be wired
>> up to adc's (rather than providing digital outputs) so you'll need some
>> framework to specify this connectivity. (open area in IIO to and the moment).
> 
> sure, I agree with you here.
It may be that we need something along the lines of how ASoC handles connectivity,
but that's complex to say the least and may be massive overkill.

It would certainly ramp up the learning curve needed to write a driver.

>>> So let accelerometer drivers use both APIs of hwmon and input
>>>> subsystems(hwmon_device_register, input_register_device,
>>>> input_register_polled_device). Acutally this is what many
>>>> accelerometer drivers in current Linux kernel are doing, so we don't
>>>> have to do much.
>>>>
>>>> Let's
>>>> 1) Introduce a new maintainer of accelerometer (Felipe?).
>>>> 2) Move accelerometer drivers in current Linux kernel to /driver/accelerometer.
>>>> 3) If we find the common functions of accelerometer or have idea about
>>>> new API or Macro, let's make at driver/accelerometer/acccelerometer.c,
>>>> input/linux/acccelerometer.h file or modify input.h little.
>>>> 4) Add every new accelerometer into /driver/accelerometer..
>>  > How about extending these to several kinds of sensors ??
>> Yup, lots out there - and lots of them don't fit into your nice clean categories.
>>  Why not having
>>> a sensor framework that abstracts the creating of the input_dev for
>>> accelerometer ?
>> In my view too limited.  (which is where IIO came from in the first place!)
>> Just because their primary use in your applications is input doesn't mean
>> that's going to be the case in future.
> 
> well, I'm thinking mostly about accelerometers, mostly. But same comment
> applies to magnetometers, those can't be exported through input, that
> one doesn't make sense at all, so how to export it ? and when that's
> defined, make iio export helper functions for registering such devices..
Actually I'd argue that a magnetometer is no worse than an accelerometer.
In the low acceleration case (which assumed by most games) and low magnetic
distortion case (probably fair in many apps). The acceleration points down,
whilst the magnetic field points in a different direction. If you really want
orientation info, then one of the standard ways to get an estimate is to
pair 3d accelerometer with 3d magnetometer.  Still that's not terribly relevant
to this discussion!

Anyhow, two options for exporting this.  Either we do it via sysfs (probably
polled) or via a character device under /dev/...

Not entirely sure what makes best sense in an 'input' style case.
IIO currently uses chardev's for event passing where events are not normal
data cases. Chardev's are also used for ring buffer access (take into account
that increasingly sensors are providing some internal buffering to cut down
the load on the host cpu). n.b. Events in input tend to correspond to 'we have
a new reading and it is x' as well as button presses.

In an input situation you tend to want a single chrdev from which to read a
class of data (hence aggregation that occurs in input) whilst caring little
about what sensor produced it whereas for the apps I was targeting, knowing
which sensor is providing the data is vital hence a given device has up to
(typcially) 3 chrdevs.  1 is for hardware events (threshold detectors etc),
1 for ring buffer events (50% full etc) and 1 for ring buffer access.
(note this required dynamic allocation / deallocation of chrdevs)  It
may make sense to add a 4th which corresponds much more closely to that provided
by input and will block until a reading is available.  Obviously this only
works if the device either uses an internal clock, or some other trigger
source is used (e.g. periodic triggers) to prevent userspace from driving
the device to be read more often that is sensible.

Lots of fun and games!

Glad interest in this stuff is picking up.  From the iio perspective, I'm
in the process of expanding the driver set considerably which should help
pin down a few of the open questions about interfaces.

When looking at the code, it is crucial to remember that not all devices
will be expected to make use of all of the facilities it provides. Minimal
driver support is designed to be as slim as possible.

Jonathan



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-18  9:12         ` Felipe Balbi
  2009-05-18 10:11           ` Jonathan Cameron
@ 2009-05-19  2:41           ` Kim Kyuwon
  2009-05-19 10:42             ` Felipe Balbi
  1 sibling, 1 reply; 22+ messages in thread
From: Kim Kyuwon @ 2009-05-19  2:41 UTC (permalink / raw)
  To: felipe.balbi
  Cc: ext Kim Kyuwon, ext Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Jonathan Cameron,
	Kyungmin Park, Jean Delvare

Hi Felipe and Jonathan.

Sorry but it's really hard for me to understand Jonathan's email and iio 
driver. [ I'm not the native speaker of English :) ]
Before asking Jonathan a few questions about his comments and iio, I 
want to reply this mail first.

Felipe Balbi wrote:
> On Mon, May 18, 2009 at 09:45:55AM +0200, ext Kim Kyuwon wrote:
>> Hi All,
>>
>> It's very nice of Felipe to make an issue of accelerometers in Linux
>> kernel again.
>> Before further discussions, we'd better see previous threads about
>> accelerometer.
>>
>> http://lkml.org/lkml/2008/5/20/135
>> http://lkml.org/lkml/2008/12/1/156
>> --
>>
>> I have considered how I can add my accelerometer driver into the Linux
>> kernel nicely for a few months.
>>
>> As Trilok said, there are many accelerometer drivers under
>> drivers/hwmon. So I first tried to add my driver as hwmon, but Jean
>> Delvare didn't agree this idea. Please refer to the following URL:
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2009-April/025686.html
>>
>> And from the next URL, Dmitry don't think it is great idea to add
>> accelerometer as Input system.
>> http://lkml.org/lkml/2008/5/27/283
> 
> Good links, I'll read them all :-)
> 
> Thanks
> 
>> On Sat, May 16, 2009 at 5:02 AM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
>>> Hi all,
>>>
>>>
>>> On Fri, May 15, 2009 at 09:30:41PM +0200, ext Mohamed Ikbel Boulabiar wrote:
>>>> I am really interested about that.
>>>>
>>>> But I want to know more about the device, its type, name, ...
>>>> The device isn't HID (Human Interface Device) ? If so, we should
>>>> rethink adding such thing but modify/use hid-input instead.
>>>>
>>>> Because, I have an accelerometer phidget device and it is HID.
>>>> Handling should be the same.
>>> Yeah, let's try to define the best way to expose accelerometers with
>>> linux kernel and avoid a sysfs hell. Better sooner than later.
>> Felipe,
>> Can I ask why did you say "avoid a sysfs hell"?. I have thought Kernel
>> developers prefer sysfs to IOCTL lately.
> 
> For sure sysfs is prefered, but I meant that without a proper
> abstraction or definition of how to export the device, each device
> driver write will export sysfs nodes as they want and that's really bad
> since we create the 'userland interface'. If it's messed up from the
> beginning, it's gonna be like that for ages.

Agreed. Thank you for your explanation.

>>>> On Fri, May 15, 2009 at 7:06 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
>>>>> Hi Felipe,
>>>>>
>>>>> Adding linux-input and Jonathan, so not deleting any lines from this e-mail.
>>>>>
>>>>> On Fri, May 15, 2009 at 6:46 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> the following patch is just an idea to see how the community feels about
>>>>>> it. Considering accelerometer devices, you might have different use
>>>>>> cases for it while running different applications. You could be using it
>>>>>> for screen rotation in one case but when opening a game, you could use
>>>>>> it as a game controller by turning the device side-by-side.
>>>>> There was one proposal from Jonathan called Industrial IO patchset
>>>>> which tried to address these sensor devices. Please grep in your
>>>>> linux-kernel archieve. I believe there are accelerometer drivers under
>>>>> drivers/hwmon.
>>> The problem is that it doesn't really seem to me that all accelerometers
>>> will be doing hw monitoring. The ones used in laptops, for sure, trying
>>> to prevent the hd from drying during a fall. But imagine the
>>> accelerometers used in, say, wii-mote, or cellphones, or such stuff ?
>>>
>>> Say we wanna use the accelerometer for both screen rotation and gaming,
>>> that device isn't doing hw monitoring and still we _do_ want to set
>>> different thresholds and irq requests/types for different use cases,
>>> right ?
>> Yes, I agree that accelerometer needs new interface. However setting
>> parameters of accelerometer is very different from devices and device
>> specific. Until now, I met two accelerometer, SMB380 from
>> bosch-sensortec and KXSD9 from Kionix. As far as I know, these two
>> accelerometers are quite different from each other and existing
>> accelerometer drivers located /driver/hwmon/ in current Linux kernel.
>> Thus I think sysfs interface (including hwmon-sysfs) is the best
>> solution for setting various parameters of accelerometer..
> 
> what if you wanna use the accelerometer as joystick for gaming ? Imagine
> a portable device...

So I said that accelerometer driver can use input_register_device, 
input_register_polled_device functions.

>> On the other hand, accelerometers are mostly used as Input device in
>> these days. Most APIs(input_allocate_device,
>> input_allocate_polled_device, ...) and macros(ABS_X,  ABS_Y, ...)of
>> Input subsystem are useful to accelerometer too. If we create another
>> APIs and Macros for accelerometers, I think It's another duplicate
>> work and result.
> 
> for sure
> 
>> It seems like Dmitry concerns input_dev becomes too big with hundreds
>> of sensors.(right?) However, Market trend makes us consider
>> accelerometer as an input device now. I'm sure there is a good way to
>> add accelerometer input system without enlarging input_dev much.
>>
>> In conclusion,
>> We need the inheritance concept in the object-oriented programming.
>> Accelerometer device sometimes can be hwmon device, sometimes input
>> device. So let accelerometer drivers use both APIs of hwmon and input
>> subsystems(hwmon_device_register, input_register_device,
>> input_register_polled_device). Acutally this is what many
>> accelerometer drivers in current Linux kernel are doing, so we don't
>> have to do much.
>>
>> Let's
>> 1) Introduce a new maintainer of accelerometer (Felipe?).
>> 2) Move accelerometer drivers in current Linux kernel to /driver/accelerometer.
>> 3) If we find the common functions of accelerometer or have idea about
>> new API or Macro, let's make at driver/accelerometer/acccelerometer.c,
>> input/linux/acccelerometer.h file or modify input.h little.
>> 4) Add every new accelerometer into /driver/accelerometer.
> 
> How about extending these to several kinds of sensors ?? Why not having
> a sensor framework that abstracts the creating of the input_dev for
> accelerometer ? 

Good point. We should consider the extensibility. I agree a sensor 
framework that abstracts input_dev. However we should discuss with Jean 
Delvare about the boundary between lm-sensors and input(?)-sensors

* However we should first confirm and review that Jonathan's iio can be 
the solution for input(?)-sensors *

 > But then comes another question: what to do with
> magnetometers, gyroscopes, etc etc ??

If we make a extensible sensor driver, I think we can add these new etc 
sensors in the future. step by step.

Regards,
Kyuwon

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-18 10:11           ` Jonathan Cameron
  2009-05-18 10:31             ` Felipe Balbi
@ 2009-05-19  6:32             ` Kim Kyuwon
  2009-05-21 11:42               ` Jonathan Cameron
  1 sibling, 1 reply; 22+ messages in thread
From: Kim Kyuwon @ 2009-05-19  6:32 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: felipe.balbi, Kim Kyuwon, ext Mohamed Ikbel Boulabiar,
	Trilok Soni, linux-kernel, Dmitry Torokhov, linux-input,
	Kyungmin Park, Jean Delvare, Zhang, Xing Z, Gross, Mark,
	jketreno, Trisal, Kalhan

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

Hello Jonathan,

Jonathan Cameron wrote:
>>>> Yeah, let's try to define the best way to expose accelerometers with
>>>> linux kernel and avoid a sysfs hell. Better sooner than later.
>>> Felipe,
>>> Can I ask why did you say "avoid a sysfs hell"?. I have thought Kernel
>>> developers prefer sysfs to IOCTL lately.
>> For sure sysfs is prefered, but I meant that without a proper
>> abstraction or definition of how to export the device, each device
>> driver write will export sysfs nodes as they want and that's really bad
>> since we create the 'userland interface'. If it's messed up from the
>> beginning, it's gonna be like that for ages.
> This was very much one of the thinks IIO was designed to address.
> One thing to keep in mind is that the framework
> was not intended to replace input / hwmon if they are the appropriate places
> for a given driver. In fact one of the conclusions of the discussions linked
> above was that, in cases where an accelerometer (or other sensor) serves
> different purposes in different devices it may make sense to actually provide
> more than one kernel driver. (obviously sharing code where appropriate.

I can't understand this:
"in cases where an accelerometer (or other sensor) serves different 
purposes in different devices it may make sense to actually provide more 
than one kernel driver. (obviously sharing code where appropriate."
Was the conclusion "Make a framework(iio) which can do various functions?"

>>> input_allocate_polled_device, ...) and macros(ABS_X,  ABS_Y, ...)of
>>> Input subsystem are useful to accelerometer too. If we create another
>>> APIs and Macros for accelerometers, I think It's another duplicate
>>> work and result.
> beware of the fact that x,y,z aren't exactly cleanly defined for a given
> sensor!
>> for sure
> Agreed.  If you know a given accelerometer is only going to be used for
> input then that's where the driver belongs.  However, these chips are
> generally capable of a lot more and it tends to be annoying specific.
> Take for example things like calibration offsets, and for the really fun
> cases on chip event driven ring buffers?  These really don't fit into
> your classic input cases.

I can't understand why you inserted ring buffers related stuffs in iio. 
  Please let me understand in easy words. And please note that in 
/Documentation/SubmittingPatches

644 4) Don't over-design.
645
646 Don't try to anticipate nebulous future cases which may or may not
647 be useful:  "Make it as simple as you can, and no simpler."

>>> In conclusion,
>>> We need the inheritance concept in the object-oriented programming.
>>> Accelerometer device sometimes can be hwmon device, sometimes input
>>> device.
> I'd also argue the problem here is that you are going to end up with a
> large class of similar devices.  If you start directly adding accelerometers
> to Input then the same arguement applies to magnetometers, bend sensors,
> gyroscopes etc.

I already answer this issue in previous mail that I sent.

 > Also beware that many accelerometers are going to be wired
> up to adc's (rather than providing digital outputs) so you'll need some
> framework to specify this connectivity. (open area in IIO to and the moment).

I can't understand. Why we should make accelerometer(or sensor) 
framework relate to ADC? My two accelerometer are not in this cases. If 
really sensors are related to ADC, it's better make this as library or 
application. Why don't we to be simple. Please let me understand in easy 
words.

Let me ask a few question in conclusion.

1) Does your iio subsystem have all functions which attached kxsd9 
driver has? i.e.
a. works in polling mode
b. works in interrupt mode
c. provide an interface to user space in order setting a few parameters 
and read x, y, z variances.

2) How long will it take to your iio subsystem is merged into mainline?
I have quick review of you iio subsytem. Sorry but I think it' somewhat 
complicated and have a lot of functions which are still not needed. And 
it seems like your coding style doesn't look like Linux coding style 
that I'm familiar with.

I just hope we can add some accelerometer feature into mainline kernel 
as soon as possible ;)

Regards,
Kyuwon

[-- Attachment #2: kxsd9.c --]
[-- Type: text/plain, Size: 14411 bytes --]

/*
 * kxsd9.c - KXSD9 Tri-axis Accelerometer hardware monitoring driver
 *
 * Copyright (c) 2008-2009 Samsung Eletronics
 * Authors:
 *	Kim Kyuwon <q1.kim@samsung.com>
 *	Kyungmin Park <kyungmin.park@samsung.com>
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 *
 * Datasheet: http://www.kionix.com/sensors/accelerometer-products.html
 *
 */

#include <linux/module.h>
#include <linux/init.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
#include <linux/interrupt.h>
#include <linux/platform_device.h>
#include <linux/workqueue.h>
#include <linux/mutex.h>
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/delay.h>
#include <linux/gpio.h>
#include <linux/input-polldev.h>
#include <linux/kxsd9.h>

#define KXSD9_XOUT_H		0x00
#define KXSD9_XOUT_L		0x01
#define KXSD9_YOUT_H		0x02
#define KXSD9_YOUT_L		0x03
#define KXSD9_ZOUT_H		0x04
#define KXSD9_ZOUT_L		0x05
#define KXSD9_AUXOUT_H		0x06
#define KXSD9_AUXOUT_L		0x07
#define KXSD9_RESET_WRITE	0x0A
#define KXSD9_CTRL_REGC		0x0C
#define KXSD9_CTRL_REGB		0x0D
#define KXSD9_CTRL_REGA		0x0E

#define KXSD9_RESET_KEY		0xCA

#define KXSD9_REGC_LP2		(1 << 7)
#define KXSD9_REGC_LP1		(1 << 6)
#define KXSD9_REGC_LP0		(1 << 5)
#define KXSD9_REGC_MOTLEV	(1 << 4)
#define KXSD9_REGC_MOTLAT	(1 << 3)
#define KXSD9_REGC_FS1		(1 << 1)
#define KXSD9_REGC_FS0		(1 << 0)

/*
 * 'CLKhld=1' makes KXSD9 hold SCL low during A/D conversions. This may
 * affect the operation of other I2C devices. Thus, I don't think it is
 * good to set CLKhld to 1.
 */
#define KXSD9_REGB_CLKHLD	(1 << 7)
#define KXSD9_REGB_ENABLE	(1 << 6)
#define KXSD9_REGB_MOTIEN	(1 << 2)

#define KXSD9_REGA_MOTI		(1 << 1)

/*
 * Default Full Scale Range: +/-2 g
 * 12-bit Sensitivity : 819 counts/g
 */
#define KXSD9_RANGE_DEFAULT	(KXSD9_REGC_FS1 | KXSD9_REGC_FS0)

/* Default Motion Wake Up Acceleration Threshold: +/-1 g */
#define KXSD9_THRESHOLD_DEFAULT	(KXSD9_REGC_MOTLEV | KXSD9_RANGE_DEFAULT)

/* Default Operational Bandwidth (Filter Corner Frequency): 50Mhz */
#define KXSD9_BW_DEFAULT	(KXSD9_REGC_LP0 | KXSD9_REGC_LP1 |\
				KXSD9_REGC_LP2)

#define MAX_12BIT		((1 << 12) - 1)
#define POLL_INTERVAL_DEFAULT	100

enum input_type {
	INPUT_NONE,
	INPUT_POLL,	/* Polling */
	INPUT_INT,	/* Interrupt */
};

struct kxsd9_sensor {
	struct i2c_client			*client;
	struct device				*dev;
	struct input_polled_dev			*ipdev;
	struct input_dev			*idev;
	struct kxsd9_sensor_platform_data	*pdata;
	struct work_struct			work;
	struct mutex				lock;

	enum input_type				type;
};

static int kxsd9_write_reg(struct i2c_client *client, u8 reg, u8 val)
{
	int ret = i2c_smbus_write_byte_data(client, reg, val);

	if (ret < 0)
		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
						__func__, reg, val, ret);
	return ret;
}

static int kxsd9_read_reg(struct i2c_client *client, u8 reg)
{
	int ret = i2c_smbus_read_byte_data(client, reg);

	if (ret < 0)
		dev_err(&client->dev, "%s, reg 0x%x, err %d\n",
						__func__, reg, ret);
	return ret;
}

static void kxsd9_read_xyz(struct i2c_client *client,
						s16 *x, s16 *y, s16 *z)
{
	*x = kxsd9_read_reg(client, KXSD9_XOUT_H) << 4 |
				kxsd9_read_reg(client, KXSD9_XOUT_L) >> 4;

	*y = kxsd9_read_reg(client, KXSD9_YOUT_H) << 4 |
				kxsd9_read_reg(client, KXSD9_YOUT_L) >> 4;

	*z = kxsd9_read_reg(client, KXSD9_ZOUT_H) << 4 |
				kxsd9_read_reg(client, KXSD9_ZOUT_L) >> 4;
}

static void kxsd9_set_bits(struct i2c_client *client, u8 reg, u8 bits, int on)
{
	struct kxsd9_sensor *sensor = i2c_get_clientdata(client);
	int val;

	mutex_lock(&sensor->lock);
	if (on)
		val = kxsd9_read_reg(client, reg) | bits;
	else
		val = kxsd9_read_reg(client, reg) & ~bits;
	kxsd9_write_reg(client, reg, (u8)val);
	mutex_unlock(&sensor->lock);
}

static int kxsd9_read_bit(struct i2c_client *client, u8 reg, u8 bit)
{
	struct kxsd9_sensor *sensor = i2c_get_clientdata(client);
	int val;

	mutex_lock(&sensor->lock);
	val = kxsd9_read_reg(client, reg) & bit;
	mutex_unlock(&sensor->lock);

	return !!val;
}

static ssize_t kxsd9_show_xyz(struct device *dev,
				struct device_attribute *attr, char *buf)
{
	struct kxsd9_sensor *sensor = dev_get_drvdata(dev);
	s16 x, y, z;

	x = y = z = 0;
	kxsd9_read_xyz(sensor->client, &x, &y, &z);

	return sprintf(buf, "%d, %d, %d\n", x, y, z);
}
static SENSOR_DEVICE_ATTR(xyz, S_IRUGO, kxsd9_show_xyz, NULL, 0);

#define KXSD9_SET_REGISTER(_name, _reg)					\
static ssize_t kxsd9_store_##_name(struct device *dev,			\
	struct device_attribute *attr, const char *buf, size_t count)	\
{									\
	struct kxsd9_sensor *sensor = dev_get_drvdata(dev);		\
	unsigned long val;						\
	int ret = strict_strtoul(buf, 16, &val);			\
	if (ret)							\
		return ret;						\
	mutex_lock(&sensor->lock);					\
	kxsd9_write_reg(sensor->client, KXSD9_CTRL_##_reg, (u8) val);	\
	mutex_unlock(&sensor->lock);					\
	return count;							\
}									\
static SENSOR_DEVICE_ATTR(_name, S_IRUGO | S_IWUSR,			\
					NULL, kxsd9_store_##_name, 0);

KXSD9_SET_REGISTER(regc, REGC);
KXSD9_SET_REGISTER(regb, REGB);

#define KXSD9_CONTROL_BIT(_reg, _bit)					\
static ssize_t kxsd9_show_##_bit(struct device *dev,			\
			struct device_attribute *attr, char *buf)	\
{									\
	struct kxsd9_sensor *sensor = dev_get_drvdata(dev);		\
	int ret = kxsd9_read_bit(sensor->client, KXSD9_CTRL_##_reg,	\
						KXSD9_##_reg##_##_bit);	\
	return sprintf(buf, "%d\n", ret);				\
}									\
static ssize_t kxsd9_store_##_bit(struct device *dev,			\
	struct device_attribute *attr, const char *buf, size_t count)	\
{									\
	struct kxsd9_sensor *sensor = dev_get_drvdata(dev);		\
	unsigned long val;						\
	int ret = strict_strtoul(buf, 10, &val);			\
	if (ret)							\
		return ret;						\
	kxsd9_set_bits(sensor->client, KXSD9_CTRL_##_reg,		\
					KXSD9_##_reg##_##_bit, val);	\
	return count;							\
}									\
static SENSOR_DEVICE_ATTR(_bit, S_IRUGO | S_IWUSR,			\
			kxsd9_show_##_bit, kxsd9_store_##_bit, 0);

KXSD9_CONTROL_BIT(REGB, MOTIEN);
KXSD9_CONTROL_BIT(REGB, ENABLE);

static struct attribute *kxsd9_attributes[] = {
	&sensor_dev_attr_xyz.dev_attr.attr,
	&sensor_dev_attr_regc.dev_attr.attr,
	&sensor_dev_attr_regb.dev_attr.attr,
	&sensor_dev_attr_MOTIEN.dev_attr.attr,
	&sensor_dev_attr_ENABLE.dev_attr.attr,
	NULL
};

static const struct attribute_group kxsd9_group = {
	.attrs	= kxsd9_attributes,
};

static void kxsd9_report_abs(struct kxsd9_sensor *sensor,
						struct input_dev *idev)
{
	s16 x, y, z;

	x = y = z = 0;

	mutex_lock(&sensor->lock);

	kxsd9_read_xyz(sensor->client, &x, &y, &z);

	input_report_abs(idev, ABS_X, x);
	input_report_abs(idev, ABS_Y, y);
	input_report_abs(idev, ABS_Z, z);

	input_sync(idev);

	mutex_unlock(&sensor->lock);
}

static void kxsd9_work(struct work_struct *work)
{
	struct kxsd9_sensor *sensor =
			container_of(work, struct kxsd9_sensor, work);
	s16 x, y, z;
	int ret;

	kxsd9_read_xyz(sensor->client, &x, &y, &z);

	/* The MOT pin can be cleared by reading CTRL_REGA. */
	ret = kxsd9_read_bit(sensor->client, KXSD9_CTRL_REGA, KXSD9_REGA_MOTI);
	if (!ret)
		dev_err(&sensor->client->dev, "MOTI bit is not set\n");

	/*
	 * To re-enter low power motion wakeup mode, reset the MOTIen bit to 1
	 */
	kxsd9_set_bits(sensor->client, KXSD9_CTRL_REGB, KXSD9_REGB_MOTIEN, 1);

	kxsd9_report_abs(sensor, sensor->idev);

	enable_irq(sensor->client->irq);
}

static irqreturn_t kxsd9_irq(int irq, void *dev_id)
{
	struct kxsd9_sensor *sensor = (struct kxsd9_sensor *) dev_id;

	if (!work_pending(&sensor->work)) {
		disable_irq_nosync(irq);
		schedule_work(&sensor->work);
	} else
		dev_err(&sensor->client->dev, "work_pending\n");

	return IRQ_HANDLED;
}

static void kxsd9_initialize(struct kxsd9_sensor *sensor)
{
	/*
	 * The reset time of the KXSD9 accelerometer takes about 11 msec.
	 * So write proper register values at first instead of resetting kxsd9.
	 */

	kxsd9_write_reg(sensor->client, KXSD9_CTRL_REGC,
		KXSD9_BW_DEFAULT | KXSD9_THRESHOLD_DEFAULT
		| KXSD9_REGC_MOTLAT);
	kxsd9_write_reg(sensor->client, KXSD9_CTRL_REGB,
		KXSD9_REGB_ENABLE | KXSD9_REGB_MOTIEN);
}

static void kxsd9_ipdev_poll(struct input_polled_dev *dev)
{
	struct kxsd9_sensor *sensor = dev->private;
	struct input_dev *idev = dev->input;

	kxsd9_report_abs(sensor, idev);
}

static void kxsd9_initialize_input_device(struct input_dev *idev,
						struct i2c_client *client)
{
	idev->name = "KXSD9 Accel";
	idev->id.bustype = BUS_I2C;
	idev->dev.parent = &client->dev;

	/* Let's use idev->open, idev->close later */

	input_set_abs_params(idev, ABS_X, 0, MAX_12BIT, 0, 0);
	input_set_abs_params(idev, ABS_Y, 0, MAX_12BIT, 0, 0);
	input_set_abs_params(idev, ABS_Z, 0, MAX_12BIT, 0, 0);

	set_bit(EV_ABS, idev->evbit);
}

static int kxsd9_register_polled_input_device(struct kxsd9_sensor *sensor)
{
	struct kxsd9_sensor_platform_data *pdata = sensor->pdata;
	struct i2c_client *client = sensor->client;
	struct input_polled_dev *ipdev;
	int ret;

	ipdev = sensor->ipdev = input_allocate_polled_device();
	if (!ipdev) {
		dev_err(&client->dev, "fail: allocate poll input device\n");
		ret = -ENOMEM;
		goto failed_quit_polled_input;
	}

	ipdev->private = sensor;
	ipdev->poll = kxsd9_ipdev_poll;
	ipdev->poll_interval = pdata->poll_interval;

	kxsd9_initialize_input_device(ipdev->input, client);

	ret = input_register_polled_device(ipdev);
	if (ret) {
		dev_err(&client->dev, "fail: register to poll input device\n");
		goto failed_free_polled_input;
	}

	sensor->type = INPUT_POLL;

	return 0;

failed_free_polled_input:
	input_free_polled_device(sensor->ipdev);
failed_quit_polled_input:

	return ret;
}

static void kxsd9_unregister_polled_input_devce(struct kxsd9_sensor *sensor)
{
	input_unregister_polled_device(sensor->ipdev);
	input_free_polled_device(sensor->ipdev);

	sensor->type = INPUT_NONE;
}

static int kxsd9_register_interrupt_input_device(struct kxsd9_sensor *sensor)
{
	struct i2c_client *client = sensor->client;
	int ret;

	sensor->idev = input_allocate_device();
	if (!sensor->idev) {
		dev_err(&client->dev, "fail: allocate input device\n");
		ret = -ENOMEM;
		goto failed_quit_interrupt_input;
	}

	kxsd9_initialize_input_device(sensor->idev, client);

	ret = input_register_device(sensor->idev);
	if (ret) {
		dev_err(&client->dev, "fail: register to input device\n");
		goto failed_free_interrupt_input;
	}

	ret = request_irq(client->irq, kxsd9_irq, IRQF_TRIGGER_HIGH,
						"KXSD9 Accel", sensor);
	if (ret) {
		dev_err(&client->dev, "can't get IRQ %d, ret %d\n",
						client->irq, ret);
		goto failed_unregister_input;
	}

	sensor->type = INPUT_INT;

	return 0;

failed_unregister_input:
	input_unregister_device(sensor->idev);
failed_free_interrupt_input:
	input_free_device(sensor->idev);
failed_quit_interrupt_input:

	return ret;
}

static void kxsd9_unregister_interrupt_input_devce(struct kxsd9_sensor *sensor)
{
	struct i2c_client *client = sensor->client;

	free_irq(client->irq, sensor);
	input_unregister_device(sensor->idev);
	input_free_device(sensor->idev);

	sensor->type = INPUT_NONE;
 }

static int kxsd9_unregister_input_device(struct kxsd9_sensor *sensor)
{
	struct i2c_client *client = sensor->client;

	if (sensor->type == INPUT_INT)
		kxsd9_unregister_interrupt_input_devce(sensor);
	else if (sensor->type == INPUT_POLL)
		kxsd9_unregister_polled_input_devce(sensor);
	else
		dev_err(&client->dev, "No input device to unregister\n");

	return 0;
}

static int __devinit kxsd9_probe(struct i2c_client *client,
					const struct i2c_device_id *id)
{
	struct kxsd9_sensor *sensor;
	int ret;

	sensor = kzalloc(sizeof(struct kxsd9_sensor), GFP_KERNEL);
	if (!sensor) {
		dev_err(&client->dev, "failed to allocate driver data\n");
		return -ENOMEM;
	}

	sensor->client = client;
	sensor->pdata = client->dev.platform_data;
	i2c_set_clientdata(client, sensor);

	/* Detect KXSD9 */
	ret = kxsd9_read_reg(client, KXSD9_CTRL_REGC);
	if (ret < 0) {
		dev_err(&client->dev, "failed to detect device\n");
		goto failed_free;
	} else
		dev_info(&client->dev, "successed to detect device\n");

	/* Initialize KXSD9 */
	kxsd9_initialize(sensor);

	INIT_WORK(&sensor->work, kxsd9_work);
	mutex_init(&sensor->lock);

	ret = sysfs_create_group(&client->dev.kobj, &kxsd9_group);
	if (ret) {
		dev_err(&client->dev, "failed to create kxsd9 attribute group");
		goto failed_free;
	}

	sensor->dev = hwmon_device_register(&client->dev);
	if (IS_ERR(sensor->dev)) {
		dev_err(&client->dev, "failed to register to hwmon device");
		ret = PTR_ERR(sensor->dev);
		goto failed_remove_sysfs;
	}

	if (client->irq > 0)
		ret = kxsd9_register_interrupt_input_device(sensor);
	else
		ret = kxsd9_register_polled_input_device(sensor);
	if (ret)
		goto failed_unregister_hwmon;

	return 0;

failed_unregister_hwmon:
	hwmon_device_unregister(&client->dev);
failed_remove_sysfs:
	sysfs_remove_group(&client->dev.kobj, &kxsd9_group);
failed_free:
	i2c_set_clientdata(client, NULL);
	kfree(sensor);

	return ret;
}

static int __devexit kxsd9_remove(struct i2c_client *client)
{
	struct kxsd9_sensor *sensor = i2c_get_clientdata(client);

	kxsd9_set_bits(client, KXSD9_CTRL_REGB, KXSD9_REGB_ENABLE, 0);
	kxsd9_unregister_input_device(sensor);
	hwmon_device_unregister(&client->dev);
	sysfs_remove_group(&client->dev.kobj, &kxsd9_group);
	i2c_set_clientdata(client, NULL);
	kfree(sensor);

	return 0;
}

static int kxsd9_suspend(struct i2c_client *client, pm_message_t mesg)
{
	disable_irq(client->irq);
	kxsd9_set_bits(client, KXSD9_CTRL_REGB, KXSD9_REGB_ENABLE, 0);

	return 0;
}

static int kxsd9_resume(struct i2c_client *client)
{
	kxsd9_set_bits(client, KXSD9_CTRL_REGB, KXSD9_REGB_ENABLE, 1);
	enable_irq(client->irq);

	return 0;
}

static const struct i2c_device_id kxsd9_id[] = {
	{ "KXSD9", 0 },
	{ }
};
MODULE_DEVICE_TABLE(i2c, kxsd9_id);

static struct i2c_driver kxsd9_i2c_driver = {
	.class		= I2C_CLASS_HWMON,
	.driver	= {
		.name	= "KXSD9",
	},
	.probe		= kxsd9_probe,
	.remove		= __devexit_p(kxsd9_remove),
	.suspend	= kxsd9_suspend,
	.resume		= kxsd9_resume,
	.id_table	= kxsd9_id,
};

static int __init kxsd9_init(void)
{
	return i2c_add_driver(&kxsd9_i2c_driver);
}
module_init(kxsd9_init);

static void __exit kxsd9_exit(void)
{
	i2c_del_driver(&kxsd9_i2c_driver);
}
module_exit(kxsd9_exit);

MODULE_AUTHOR("Kim Kyuwon <q1.kim@samsung.com>");
MODULE_DESCRIPTION("KXSD9 hardware monitoring driver");
MODULE_LICENSE("GPL v2");

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-19  2:41           ` Kim Kyuwon
@ 2009-05-19 10:42             ` Felipe Balbi
  2009-05-19 12:34               ` Mohamed Ikbel Boulabiar
  0 siblings, 1 reply; 22+ messages in thread
From: Felipe Balbi @ 2009-05-19 10:42 UTC (permalink / raw)
  To: ext Kim Kyuwon
  Cc: Balbi Felipe (Nokia-D/Helsinki),
	ext Kim Kyuwon, ext Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Jonathan Cameron,
	Kyungmin Park, Jean Delvare

Hi,

On Tue, May 19, 2009 at 04:41:24AM +0200, ext Kim Kyuwon wrote:
> > what if you wanna use the accelerometer as joystick for gaming ? Imagine
> > a portable device...
> 
> So I said that accelerometer driver can use input_register_device, 
> input_register_polled_device functions.

exactly...

> Good point. We should consider the extensibility. I agree a sensor 
> framework that abstracts input_dev. However we should discuss with Jean 
> Delvare about the boundary between lm-sensors and input(?)-sensors
> 
> * However we should first confirm and review that Jonathan's iio can be 
> the solution for input(?)-sensors *

sure, makes sense to me

>  > But then comes another question: what to do with
> > magnetometers, gyroscopes, etc etc ??
> 
> If we make a extensible sensor driver, I think we can add these new etc 
> sensors in the future. step by step.

agreed here. But we have to think that this might get extended, so we
should start thinking of an abstraction layer that could evolve for
magnetometers, gyroscopes and other sensors

-- 
balbi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-19 10:42             ` Felipe Balbi
@ 2009-05-19 12:34               ` Mohamed Ikbel Boulabiar
  2009-05-21 10:21                 ` Jonathan Cameron
  0 siblings, 1 reply; 22+ messages in thread
From: Mohamed Ikbel Boulabiar @ 2009-05-19 12:34 UTC (permalink / raw)
  To: felipe.balbi
  Cc: ext Kim Kyuwon, ext Kim Kyuwon, Trilok Soni, linux-kernel,
	Dmitry Torokhov, linux-input, Jonathan Cameron, Kyungmin Park,
	Jean Delvare

Hi,

>>  > But then comes another question: what to do with
>> > magnetometers, gyroscopes, etc etc ??
>>
>> If we make a extensible sensor driver, I think we can add these new etc
>> sensors in the future. step by step.
>
> agreed here. But we have to think that this might get extended, so we
> should start thinking of an abstraction layer that could evolve for
> magnetometers, gyroscopes and other sensors

Agree with you, there should be an abstraction layer.

According to this, we should collect similar functionalities in one
module and make sub-module describing exceptions devices or ones that
needs special handling.
But in the end for the user space apps, they should "see" an
accelerometer in a standard way. All exceptions and pre-handling
should be done in the abstraction layer as described.
We can't bring as many full drivers as devices found, but one driver
serving for a group and that can load submodules when necessary.
The way HID is coded in Linux is an example for that, dealing with
many types of devices.
It isn't perfect, but its architecture can be used for other devices.

To stay with accelerometers, they are sensors, and sensors sense
physical properties.
Every device use its own methods but in the end they sense the same
thing and user wait for a similar feedback.

I post a link describing the HID standard, you can see in page 37 to
39 how they define units, and exponents for sensors.
http://www.usb.org/developers/devclass_docs/HID1_11.pdf

Also, we can't imagine all use cases and scenarios, we should bring
useful information to userspace apps and they do what they want with
these informations.
KDE developers may use sensors to automatically rotate the screen
90°/180° if the laptop is rotated. Others will use it to play games,
Some others will use light sensors to lighten the keyboard or to
change the brightness of the screen,...


What do you think ?

_
ikbel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-19 12:34               ` Mohamed Ikbel Boulabiar
@ 2009-05-21 10:21                 ` Jonathan Cameron
  0 siblings, 0 replies; 22+ messages in thread
From: Jonathan Cameron @ 2009-05-21 10:21 UTC (permalink / raw)
  To: Mohamed Ikbel Boulabiar
  Cc: felipe.balbi, ext Kim Kyuwon, ext Kim Kyuwon, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kyungmin Park,
	Jean Delvare


> Hi,
> 
>>>  > But then comes another question: what to do with
>>>> magnetometers, gyroscopes, etc etc ??
>>> If we make a extensible sensor driver, I think we can add these new etc
>>> sensors in the future. step by step.
>> agreed here. But we have to think that this might get extended, so we
>> should start thinking of an abstraction layer that could evolve for
>> magnetometers, gyroscopes and other sensors
> 
> Agree with you, there should be an abstraction layer.
Indeed. There is however a question of where some of this should take
place. (Pretty much userspace vs kernel space).

Getting some of the more useful information from these sensors
(particularly where multiple types are available together) will
require quite sophisticated algorithms that are not available in kernel.

An example of devices where this is true would be the new wii controller
add on (although in this case it's all handled on the remote I think).
Here multiple sensors are used in conjunction to give results that simply
can't be established with a single sensor type.  Even in cases such
as analogs adis16350 and similar IMUs there is a lot of processing to be
done off the chip it self.

Yes we can do the dumb iphone style (hideously smoothed) accelerometer
type control, but that only touches the tip of what is possible.
 
> According to this, we should collect similar functionalities in one
> module and make sub-module describing exceptions devices or ones that
> needs special handling.
Some things can be wrapped into a core subsystem driver, but a frightening
amount of this stuff is chip specific.
> But in the end for the user space apps, they should "see" an
> accelerometer in a standard way. All exceptions and pre-handling
> should be done in the abstraction layer as described.
Such an abstraction layer should exist, but shouldn't be the only way to
access these devices.  A lot of them provide weird and wonderful elements
(based on internal processing) that would lead to an obscene amount of bloat
on any attempt at a general abstraction.
> We can't bring as many full drivers as devices found, but one driver
> serving for a group and that can load submodules when necessary.
Doesn't work.  In hid you can do this because the hardware itself conforms
to a tight spec.  This simply isn't the case with the types of sensor we
are talking about here.  They structure that would be needed would be much
closer to that taken by hwmon, where every chip (or family of chips) has
to have it's own driver.   Sure it then registers certain elements with
the core, but fundamentally they aren't similar enough to share all that
much code.
> The way HID is coded in Linux is an example for that, dealing with
> many types of devices.
> It isn't perfect, but its architecture can be used for other devices.
 
> To stay with accelerometers, they are sensors, and sensors sense
> physical properties.
> Every device use its own methods but in the end they sense the same
> thing and user wait for a similar feedback.
Partly true.  There is certainly a core functionality that is shared
across some of these chips.  However take a look at the drivers in IIO
or some data sheets and you'll find a frightening variety of other stuff
in each driver.  For basic input stuff it may be fine to only support the
common elements, but that's not an option for many other applications.
You tend to have picked a chip with particular features for a reason!
> 
> I post a link describing the HID standard, you can see in page 37 to
> 39 how they define units, and exponents for sensors.
> http://www.usb.org/developers/devclass_docs/HID1_11.pdf
> 
> Also, we can't imagine all use cases and scenarios, we should bring
> useful information to userspace apps and they do what they want with
> these informations.
> KDE developers may use sensors to automatically rotate the screen
> 90°/180° if the laptop is rotated. Others will use it to play games,
> Some others will use light sensors to lighten the keyboard or to
> change the brightness of the screen,...
> 
In a previous (off list) discussion, faking HID was brought up, so I'll
summarize where we got to.

Firstly note we would be faking HID.  In it's conventional use, it would be
the sensors themselves that would provide information about their facilities
and add all the stuff that it's described in the document above.  Sadly for
these devices very few manufacturers have gone down that path.  The driver has
to know what a given chip supports, there is no query method.  It would be possible
to make this work, but it's a seriously nasty case of blugeoning data into a
format that although flexible enough, was very much designed for other things.
Feel free to try and persuade the manufacturers to conform to HID!

To keep this maintenance nightmare minimal, we'd probably have to keep pretty
much completely separate from the HID system and just rely on matching interfaces.
(worth bringing in some HID experts but that's the feeling I got from reading
the code last time round?)

This in turn means we going to generate a nasty problem with testing any userspace
developments against both conventional hid and our 'fake' hid implementation.

If we were to go this way it would have to only be one of several interfaces.
The overhead will be large, and frankly for the high performance sampling end of
things that I'm interested in (and others who are in the CC's above) this isn't
an option.  Do you want to make HID deal with hardware (or software) ring buffering
for example?  It's way out of scope.

My personal preference for this problem area would be to look at how to add some
minimal core functionality to IIO to make it provide input interfaces as well.
In a similar fashion to lm-sensors it will be possible to establish functionality
of a given device by looking at what it exports in sysfs and hence applications
(or more likely a nice userspace library) can use this to identify what sort of
data is available and do any aggregation and processing.  In a similar fashion
to lm-sensors, this library would be kept in sync with the kernel tree.

HID might be an option, but I'm yet to be convinced it would be practical.

Jonathan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-19  6:32             ` Kim Kyuwon
@ 2009-05-21 11:42               ` Jonathan Cameron
  2009-05-22  8:21                 ` Kim Kyuwon
  0 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2009-05-21 11:42 UTC (permalink / raw)
  To: Kim Kyuwon
  Cc: felipe.balbi, Kim Kyuwon, ext Mohamed Ikbel Boulabiar,
	Trilok Soni, linux-kernel, Dmitry Torokhov, linux-input,
	Kyungmin Park, Jean Delvare, Zhang, Xing Z, Gross, Mark,
	jketreno, Trisal, Kalhan

Hi Kyuwon, 


Kim Kyuwon wrote:
> Hello Jonathan,
> 
> Jonathan Cameron wrote:
>>>>> Yeah, let's try to define the best way to expose accelerometers with
>>>>> linux kernel and avoid a sysfs hell. Better sooner than later.
>>>> Felipe,
>>>> Can I ask why did you say "avoid a sysfs hell"?. I have thought Kernel
>>>> developers prefer sysfs to IOCTL lately.
>>> For sure sysfs is prefered, but I meant that without a proper
>>> abstraction or definition of how to export the device, each device
>>> driver write will export sysfs nodes as they want and that's really bad
>>> since we create the 'userland interface'. If it's messed up from the
>>> beginning, it's gonna be like that for ages.
>> This was very much one of the thinks IIO was designed to address.
>> One thing to keep in mind is that the framework
>> was not intended to replace input / hwmon if they are the appropriate
>> places
>> for a given driver. In fact one of the conclusions of the discussions
>> linked
>> above was that, in cases where an accelerometer (or other sensor) serves
>> different purposes in different devices it may make sense to actually
>> provide
>> more than one kernel driver. (obviously sharing code where appropriate.
> 
> I can't understand this:
> "in cases where an accelerometer (or other sensor) serves different
> purposes in different devices it may make sense to actually provide more
> than one kernel driver. (obviously sharing code where appropriate."
> Was the conclusion "Make a framework(iio) which can do various functions?"
No, the conclusion was to wait and see if this is practical. It was kept in
mind during the several iterations of design of IIO, but the focus of
development was always high performance capture and handling all the facilities
of these highly variable sensors as consistently as possible.

I guess now we are in a position to see if it is indeed practical to add input
support. 

> 
>>>> input_allocate_polled_device, ...) and macros(ABS_X,  ABS_Y, ...)of
>>>> Input subsystem are useful to accelerometer too. If we create another
>>>> APIs and Macros for accelerometers, I think It's another duplicate
>>>> work and result.
>> beware of the fact that x,y,z aren't exactly cleanly defined for a given
>> sensor!
>>> for sure
>> Agreed.  If you know a given accelerometer is only going to be used for
>> input then that's where the driver belongs.  However, these chips are
>> generally capable of a lot more and it tends to be annoying specific.
>> Take for example things like calibration offsets, and for the really fun
>> cases on chip event driven ring buffers?  These really don't fit into
>> your classic input cases.
> 
> I can't understand why you inserted ring buffers related stuffs in iio.
>  Please let me understand in easy words. And please note that in
> /Documentation/SubmittingPatches
Because the hardware I have uses hardware ring buffers e.g. VTI SCA3000 series,
analog adxl345.  More and more sensors are moving this way as without a true
real time os, it's the only way of ensuring high data rate data is available
for sophisticated almost real time processing.  You can't afford to have gaps
in your data.

 Also, for my primary application (fast, consistent capture
from a number of different sensors) this is the simplest design that provides
the performance needed.  We spent a lot of time trying to do this in other
ways.
> 
> 644 4) Don't over-design.
> 645
> 646 Don't try to anticipate nebulous future cases which may or may not
> 647 be useful:  "Make it as simple as you can, and no simpler."

The are useful. I'm using them in real applications every day.  It might not
be your use case but it is a number of other peoples (some of whom are cc'd
above).

The absolutely crucial thing about the design is that if you don't want ring
buffers, then don't use them (by which I mean don't compile them in - it's a
modular design so if you don't want them you don't have to have them.)
In the first instance I probably wouldn't be pushing that element into the kernel.
As you are illustrating this is controversial and needs a thorough discussion.

At that point the whole system reduces to  much simpler core which handles
the sysfs interface registration and management of chrdevs associated with events.
At this level it's fairly similar to input, but without the event aggregation and
with things like event escalation and dynamic chrdev allocation.

>>>> In conclusion,
>>>> We need the inheritance concept in the object-oriented programming.
>>>> Accelerometer device sometimes can be hwmon device, sometimes input
>>>> device.
>> I'd also argue the problem here is that you are going to end up with a
>> large class of similar devices.  If you start directly adding
>> accelerometers
>> to Input then the same arguement applies to magnetometers, bend sensors,
>> gyroscopes etc.
> 
> I already answer this issue in previous mail that I sent.
> 
>> Also beware that many accelerometers are going to be wired
>> up to adc's (rather than providing digital outputs) so you'll need some
>> framework to specify this connectivity. (open area in IIO to and the
>> moment).
> 
> I can't understand. Why we should make accelerometer(or sensor)
> framework relate to ADC?
There are a couple of reasons:
1) Accelerometers are effectively analog sensors wired up to an ADC. As such
they share a lot of common features.

2) Many real accelerometer rigs out there actually use a pair of chips,
so as far as Linux is concerned you are talking to an adc not the accelerometer
on the other side. Handling this case is still an open issue and it was to this
that I was referring above.
  
> My two accelerometer are not in this cases. 
Indeed not.  But many others are and here we are talking about a general
framework.
> If really sensors are related to ADC, it's better make this as library or
> application.
That is an open question.  There are cases where it would have to be done
in kernel (where both the accelerometer and the adc have separate control
interfaces). 

 Why don't we to be simple. Please let me understand in easy
> words.
> 
> Let me ask a few question in conclusion.
Firstly thanks for sending this driver, its always enlightening to see
how people have handled such a device.  Out of interest, have you
posted this to the input list?  I'd be interested to see what their
comments are.

> 1) Does your iio subsystem have all functions which attached kxsd9
> driver has? i.e.
> a. works in polling mode
Yes,
> b. works in interrupt mode
No, but only because I was utilizing it as an example of a minimalist driver.
See the lis3l02dq driver in the git tree for how it would be extended.

Firstly, beware, the interrupt case for this chip is very different to the more
common cases where the interrupt in question indicates the availability of a new
reading.

As a more general comment, I'd also argue that the reading you
take on interrupt is irrelevant. The event was the threshold pass, not the
value of the reading. It should be up to userspace to decide if it cares what
the reading is and if it does, it has a polling mode with which to read it.
As I read the data sheet, the device isn't storing the accelerations at
interrupt, so the value you are reading is not relevant to the event at all, but
typically will be another reading.

I like the code you have for bringing it up from low power mode to grab a current
value and then go back to sleep. I haven't seen this functionality in many chips.

There are a couple of ways that it could be handled.
1) Register the MOTLat event to be passed up to userspace and let userspace take
   a reading via the normal poll modes. (pretty much what would happen in input)

2) Configure this as and IIO trigger which would allow it to be used to 'trigger'
   the reading of a number of other sensors as well as the acceleration.

It should be possible to support both of these but as per your earlier comment
about not adding features until they are needed, I'd advocate only supporting the
first option until the someone needs the second.

> c. provide an interface to user space in order setting a few parameters
> and read x, y, z variances.
Yes, via sysfs.
 
> 2) How long will it take to your iio subsystem is merged into mainline?
Not sure - I still don't have enough reviewers. As far as I'm concerned, bar
changes relating to reviews it's been ready for merging for a couple of
months.  Since the last posting that got almost no responses the only changes
have been minor changes and comment clean ups.

My intention is to shortly seek advice on how to go about doing this, but
my current feeling is that it will have to be done in stages.  The idea
is that each stage will involve a relatively small code base and hence
be easier for reviewers to handle.  The white paper was written to assist
with this process by providing an overview of how everything links together
without reviewers having to read the full code.

1) The core functionality.   This gives us something very close to input with
the principal changes as described above.

2) A set of example drivers

At this point we will have a common framework against which to merge new drivers.

3) The advanced features (triggering and ring buffering)

4) The drivers for devices requiring 3.
 
> I have quick review of you iio subsytem. Sorry but I think it' somewhat
> complicated and have a lot of functions which are still not needed.
By you - I repeat that I have been using this system in anger for about 6
months now.  I only know of one bit of functionality in there that I'm not
currently using (the first of the trigger lists) and that is simply because
I haven't completed the driver that uses it as yet and dropping it would be
a trivial change.
> And
> it seems like your coding style doesn't look like Linux coding style
> that I'm familiar with.
As far as I know the code absolutely conforms to every coding style
convention of the kernel.  The version in the 'mess' branch has a few comment
clean ups as a result of checking the kernel doc comments.  If you have
specific examples please send them on and I'll be happy to clean them up.
> 
> I just hope we can add some accelerometer feature into mainline kernel
> as soon as possible ;)
I agree.  The stop gap solution (which a number drivers have taken) is to put
them in drivers/misc on the basis that when we do have an agreed system we can
then move them over at a later date.  I have already offered to do the
conversions for a number of such drivers when iio gets merged.

Jonathan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-21 11:42               ` Jonathan Cameron
@ 2009-05-22  8:21                 ` Kim Kyuwon
  2009-05-22 13:35                   ` Jonathan Cameron
  0 siblings, 1 reply; 22+ messages in thread
From: Kim Kyuwon @ 2009-05-22  8:21 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: felipe.balbi, Kim Kyuwon, Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kyungmin Park,
	Jean Delvare, Zhang, Xing Z, Gross, Mark, jketreno, Trisal,
	Kalhan

Jonathan Cameron wrote:
> No, the conclusion was to wait and see if this is practical. It was kept in
> mind during the several iterations of design of IIO, but the focus of
> development was always high performance capture and handling all the facilities
> of these highly variable sensors as consistently as possible.
> 
> I guess now we are in a position to see if it is indeed practical to add input
> support. 
>

Thank you for your clarification.

>> I can't understand why you inserted ring buffers related stuffs in iio.
>>  Please let me understand in easy words. And please note that in
>> /Documentation/SubmittingPatches
> Because the hardware I have uses hardware ring buffers e.g. VTI SCA3000 series,
> analog adxl345.  More and more sensors are moving this way as without a true
> real time os, it's the only way of ensuring high data rate data is available
> for sophisticated almost real time processing. 

How can you be so sure that more and more sensors use h/w ring buffer? 
Is there any data or experts who supports your point?

And although assuming you are right here, STILL only your SCA3000 series 
have h/w ring buffer. Thus, I think there is no reason the framework 
should have ring buffer feature.

 > You can't afford to have gaps
> in your data.
> 
>  Also, for my primary application (fast, consistent capture
> from a number of different sensors) this is the simplest design that provides
> the performance needed.  We spent a lot of time trying to do this in other
> ways.

Your words('simplest' and 'spending a lot of time') are subjective.

>> 644 4) Don't over-design.
>> 645
>> 646 Don't try to anticipate nebulous future cases which may or may not
>> 647 be useful:  "Make it as simple as you can, and no simpler."
> 
> The are useful. I'm using them in real applications every day.  It might not
> be your use case but it is a number of other peoples (some of whom are cc'd
> above).

Our objective is merging code upstream. We have lots of kernel drivers 
and kernel features which we are using everyday However, I know these 
are can't be merged kernel.

> The absolutely crucial thing about the design is that if you don't want ring
> buffers, then don't use them (by which I mean don't compile them in - it's a
> modular design so if you don't want them you don't have to have them.)

I'm just thinking the right functions should be located in the right 
position. If only SCA3000 series use ring buffer, it should be in the 
SCA3000 series driver.

> In the first instance I probably wouldn't be pushing that element into the kernel.
> As you are illustrating this is controversial and needs a thorough discussion.
> 
> At that point the whole system reduces to  much simpler core which handles
> the sysfs interface registration and management of chrdevs associated with events.
> At this level it's fairly similar to input, but without the event aggregation and
> with things like event escalation and dynamic chrdev allocation.

We already have a nice 'input subsystem'. why not just using 'input 
subsystem'

>> I can't understand. Why we should make accelerometer(or sensor)
>> framework relate to ADC?
> There are a couple of reasons:
> 1) Accelerometers are effectively analog sensors wired up to an ADC. As such
> they share a lot of common features.
> 2) Many real accelerometer rigs out there actually use a pair of chips,
> so as far as Linux is concerned you are talking to an adc not the accelerometer
> on the other side. Handling this case is still an open issue and it was to this
> that I was referring above.

If there is still an open issue, it is difficult to merge this IIO 
subsystem to upstream kernel.

>> My two accelerometer are not in this cases. 
> Indeed not.  But many others are and here we are talking about a general
> framework.
>> If really sensors are related to ADC, it's better make this as library or
>> application.
> That is an open question.  There are cases where it would have to be done
> in kernel (where both the accelerometer and the adc have separate control
> interfaces). 

Then you can make another device driver module or API that can help 
intercommunicate between two devices.

>  Why don't we to be simple. Please let me understand in easy
>> words.
>>
>> Let me ask a few question in conclusion.
> Firstly thanks for sending this driver, its always enlightening to see
> how people have handled such a device.  Out of interest, have you
> posted this to the input list?  I'd be interested to see what their
> comments are.

I can't add this driver to input list, because this kxsd9 driver is for 
accelerometer.

>> 1) Does your iio subsystem have all functions which attached kxsd9
>> driver has? i.e.
>> a. works in polling mode
> Yes,
>> b. works in interrupt mode
> No, but only because I was utilizing it as an example of a minimalist driver.
> See the lis3l02dq driver in the git tree for how it would be extended.
> 
> Firstly, beware, the interrupt case for this chip is very different to the more
> common cases where the interrupt in question indicates the availability of a new
> reading.
> 
> As a more general comment, I'd also argue that the reading you
> take on interrupt is irrelevant. The event was the threshold pass, not the
> value of the reading. It should be up to userspace to decide if it cares what
> the reading is and if it does, it has a polling mode with which to read it.
> As I read the data sheet, the device isn't storing the accelerations at
> interrupt, so the value you are reading is not relevant to the event at all, but
> typically will be another reading.
> 
> I like the code you have for bringing it up from low power mode to grab a current
> value and then go back to sleep. I haven't seen this functionality in many chips.
> 
> There are a couple of ways that it could be handled.
> 1) Register the MOTLat event to be passed up to userspace and let userspace take
>    a reading via the normal poll modes. (pretty much what would happen in input)
> 
> 2) Configure this as and IIO trigger which would allow it to be used to 'trigger'
>    the reading of a number of other sensors as well as the acceleration.
> 
> It should be possible to support both of these but as per your earlier comment
> about not adding features until they are needed, I'd advocate only supporting the
> first option until the someone needs the second.
> 
>> c. provide an interface to user space in order setting a few parameters
>> and read x, y, z variances.
> Yes, via sysfs.
>  
>> 2) How long will it take to your iio subsystem is merged into mainline?
> Not sure - I still don't have enough reviewers. As far as I'm concerned, bar
> changes relating to reviews it's been ready for merging for a couple of
> months.  Since the last posting that got almost no responses the only changes
> have been minor changes and comment clean ups.
> My intention is to shortly seek advice on how to go about doing this, but
> my current feeling is that it will have to be done in stages.  The idea
> is that each stage will involve a relatively small code base and hence
> be easier for reviewers to handle.  The white paper was written to assist
> with this process by providing an overview of how everything links together
> without reviewers having to read the full code.
> 
> 1) The core functionality.   This gives us something very close to input with
> the principal changes as described above.
> 
> 2) A set of example drivers
> 
> At this point we will have a common framework against which to merge new drivers.
> 
> 3) The advanced features (triggering and ring buffering)
> 
> 4) The drivers for devices requiring 3.
>  
>> I have quick review of you iio subsytem. Sorry but I think it' somewhat
>> complicated and have a lot of functions which are still not needed.
> By you - I repeat that I have been using this system in anger for about 6
> months now.  I only know of one bit of functionality in there that I'm not
> currently using (the first of the trigger lists) and that is simply because
> I haven't completed the driver that uses it as yet and dropping it would be
> a trivial change.
>> And
>> it seems like your coding style doesn't look like Linux coding style
>> that I'm familiar with.
> As far as I know the code absolutely conforms to every coding style
> convention of the kernel.  The version in the 'mess' branch has a few comment
> clean ups as a result of checking the kernel doc comments.  If you have
> specific examples please send them on and I'll be happy to clean them up.

For instance, please
drivers/industrialio/ -exec ./scripts/checkpatch.pl --file {} \;

>> I just hope we can add some accelerometer feature into mainline kernel
>> as soon as possible ;)
> I agree.  The stop gap solution (which a number drivers have taken) is to put
> them in drivers/misc on the basis that when we do have an agreed system we can
> then move them over at a later date.  I have already offered to do the
> conversions for a number of such drivers when iio gets merged.
> 
> Jonathan
>

Finally,
Are you OK if someone trys making another framework for input(?)-sensor 
or accelerometer.

Regards,
Kyuwon

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-22  8:21                 ` Kim Kyuwon
@ 2009-05-22 13:35                   ` Jonathan Cameron
  2009-05-25  8:15                     ` Kim Kyuwon
  0 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2009-05-22 13:35 UTC (permalink / raw)
  To: Kim Kyuwon
  Cc: felipe.balbi, Kim Kyuwon, Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kyungmin Park,
	Jean Delvare, Zhang, Xing Z, Gross, Mark, jketreno, Trisal,
	Kalhan

Hi Kyuwon,
> Jonathan Cameron wrote:
>> No, the conclusion was to wait and see if this is practical. It was
>> kept in
>> mind during the several iterations of design of IIO, but the focus of
>> development was always high performance capture and handling all the
>> facilities
>> of these highly variable sensors as consistently as possible.
>>
>> I guess now we are in a position to see if it is indeed practical to
>> add input
>> support.
> 
> Thank you for your clarification.
> 
>>> I can't understand why you inserted ring buffers related stuffs in iio.
>>>  Please let me understand in easy words. And please note that in
>>> /Documentation/SubmittingPatches
>> Because the hardware I have uses hardware ring buffers e.g. VTI
>> SCA3000 series,
>> analog adxl345.  More and more sensors are moving this way as without
>> a true
>> real time os, it's the only way of ensuring high data rate data is
>> available
>> for sophisticated almost real time processing. 
> 
> How can you be so sure that more and more sensors use h/w ring buffer?
> Is there any data or experts who supports your point?
I'm basing this on observation and believe me I use a lot of different
sensors and work with people who use a lot more. I'm not going to waste
my timing getting them to post a series of 'opinions' on this list when
a bit of Googling and looking at release dates of sensors will give you
the same information.

Until recently the only ones I had encountered were VTI's.  Now Analog have
started producing a range of sensors with various forms of triggered buffer.
Take a look at their 'new' page.  Admittedly some of the newer ones don't
fall easily into my current model so some rethinking of elements of it will
be needed.  The reason none of the below is currently supported is that I
haven't been able to get them as yet. I'm particularly interested in the
16240 for some athlete monitoring applications.

We have:

* ADIS16220  (I hadn't even seen this one before) Digital Vibration sensor.
	This has a 1024 sample buffer with various possible triggering modes.

* ADIS16240 Impact sensor and recorder 3x8192 sample buffer using triggered events.

* ADXL346 Accelerometer. 32 levels of output data FIFO minimizes host processor
	load.

* ADXL335 Analog sensor so unsurprisingly doesn't feature a digital buffer.

If we hit the more button and select products in the last year we also gain 
the ADXL345 wich yet again provides a fifo to reduce load on the processor.
Before that one, I don't think any of Analog's products featured such buffering.

So we now have only 2 different manufacturers making sensors with ring buffers
on them. One of which has put them onto 5 out of their last 6 products (though
I'll admit the 345 and 346 are extremely closely related).  Looks like
they think there is a market.

> And although assuming you are right here, STILL only your SCA3000 series
> have h/w ring buffer. Thus, I think there is no reason the framework
> should have ring buffer feature
> 
>> You can't afford to have gaps
>> in your data.
>>
>>  Also, for my primary application (fast, consistent capture
>> from a number of different sensors) this is the simplest design that
>> provides
>> the performance needed.  We spent a lot of time trying to do this in
>> other
>> ways.
> 
> Your words('simplest' and 'spending a lot of time') are subjective.
Yes, ultimately I wrote a system that fulfilled my requirements, not one
that was designed to meet yours. Let me reemphasize that the framework
is very much designed to be modular.  If you don't want ring buffers then
you don't have to use them.  Their presence has NO effect whatsoever on
drivers that aren't using them.  In fact, even when the driver does support
them, I've mostly made it a Kconfig option on a per driver basis.

For my typical application the more esoteric triggering methods are also
vital.  If I want to synchronize a linux based mote's capture of an
accelerometer with a high end motion capture system and not miss a single
sample, within an adaptable framework then I need pretty much every element
currently in the IIO system.  Likewise, if I want regular 2Khz sampling of
an ADC connected via 400Khz i2c then I need that to be supported.  Yes these
aren't your applications, which I'm assuming from your post are control of
a pretty user interface, but they are an import class of applications for
other users within both the academic world and industry.


> 
>>> 644 4) Don't over-design.
>>> 645
>>> 646 Don't try to anticipate nebulous future cases which may or may not
>>> 647 be useful:  "Make it as simple as you can, and no simpler."
>>
>> The are useful. I'm using them in real applications every day.  It
>> might not
>> be your use case but it is a number of other peoples (some of whom are
>> cc'd
>> above).
> 
> Our objective is merging code upstream. We have lots of kernel drivers
> and kernel features which we are using everyday However, I know these
> are can't be merged kernel.
This is not the place to get into that sort of argument so I'll keep my
reply to this short.

Why not?  If you have something that is useful, then why not submit it?
This is particularly true in cases like this were the useful element
doesn't change anything already in kernel and hence if people don't want
it then it won't effect them.
> 
>> The absolutely crucial thing about the design is that if you don't
>> want ring
>> buffers, then don't use them (by which I mean don't compile them in -
>> it's a
>> modular design so if you don't want them you don't have to have them.)
> 
> I'm just thinking the right functions should be located in the right
> position. If only SCA3000 series use ring buffer, it should be in the
> SCA3000 series driver.
It's not only the SCA3000 series.
Note whilst they may not matter for your applications the support
for software ring buffering in kernel is still absolutely vital to me. Also
see the devices cited above.
> 
>> In the first instance I probably wouldn't be pushing that element into
>> the kernel.
>> As you are illustrating this is controversial and needs a thorough
>> discussion.
>>
>> At that point the whole system reduces to  much simpler core which
>> handles
>> the sysfs interface registration and management of chrdevs associated
>> with events.
>> At this level it's fairly similar to input, but without the event
>> aggregation and
>> with things like event escalation and dynamic chrdev allocation.
> 
> We already have a nice 'input subsystem'. why not just using 'input
> subsystem'
See the original discussion and remember that we are dealing with application
cases beyond using these devices for human input. To quote Dmitry:

>I don't think that input subsystem would be the best choice. While we
>do support the event-driven mechanism for delivering information to
>userspace input is mostly oriented for human interface devices, not
>general data acquisition. If anything input_dev is too fat. Another
>thing is that input layer anonymizes input devices, makes them
>capability-oriented. I.e. we dont really care what model of joystick
>we have, we want something that reports ABS_X, ABS_Y, BTN_FIRE and
>userpsace can use it whether it a simple analog joystick or something
>fancy and USB connected. For your purposes you probably do want to know
>what the device is and what exactly being sampled/measured.

>I'd go with a lean and mean new subsytem.
(http://lkml.org/lkml/2008/5/21/186)

Now, you might argue that the resulting subsystem is not lean and mean,
but to my mind it comes down to a case of what code actually runs on a
given event.  Large chunks of the core subsystem are concerned with
simplifying shared functionality between different devices,
but in most use cases the run path from events happening to them hitting
userspace is as lean and mean as possible whilst maintaining flexibility
where needed.

If you don't care about he anonymous nature of devices under input then
fine, submit your drivers to their system.  Then if the sensor in question
becomes useful to someone who does need some of the additional functionality
that iio provides we will end up with a second driver.  It's exactly this
case that lead to the conclusion in the original discussion that there may
be circumstances where a given device has several separate drivers within
the kernel. Conclusion was that if the requirements were different enough
and they could not be easily merged, this may be the best way to go.
> 
>>> I can't understand. Why we should make accelerometer(or sensor)
>>> framework relate to ADC?
>> There are a couple of reasons:
>> 1) Accelerometers are effectively analog sensors wired up to an ADC.
>> As such
>> they share a lot of common features.
>> 2) Many real accelerometer rigs out there actually use a pair of chips,
>> so as far as Linux is concerned you are talking to an adc not the
>> accelerometer
>> on the other side. Handling this case is still an open issue and it
>> was to this
>> that I was referring above.
> 
> If there is still an open issue, it is difficult to merge this IIO
> subsystem to upstream kernel.
I disagree. It's irrelevant to merging the subsystem. You don't have to 
have every use case and every possible setup handled by a subsystem from
inception.  For now, if you really need your adc connected accelerometer
to export 'accel_x accel_y' etc then you can trivially write a driver
for the whole system.  This would obviously lead to a certain amount of
code duplication and hence I'm suggesting that the question of whether
this should be a userspace element (similar to lm-sensors config files)
or the linkages should be specified in kernel (similar to ASoC).
> 
>>> My two accelerometer are not in this cases. 
>> Indeed not.  But many others are and here we are talking about a general
>> framework.
>>> If really sensors are related to ADC, it's better make this as
>>> library or
>>> application.
>> That is an open question.  There are cases where it would have to be done
>> in kernel (where both the accelerometer and the adc have separate control
>> interfaces). 
> 
> Then you can make another device driver module or API that can help
> intercommunicate between two devices.
Yes.  That may be the way to go down the line.
> 
>>  Why don't we to be simple. Please let me understand in easy
>>> words.
>>>
>>> Let me ask a few question in conclusion.
>> Firstly thanks for sending this driver, its always enlightening to see
>> how people have handled such a device.  Out of interest, have you
>> posted this to the input list?  I'd be interested to see what their
>> comments are.
> 
> I can't add this driver to input list, because this kxsd9 driver is for
> accelerometer.
Why does this prevent you posting it there?  It is using the input framework
so if it was going anywhere in the kernel it would require a review and ACK
from the relevant people on that list in much the same way as it should also
be posted to the spi / i2c list as appropriate for review. 
> 
>>> 1) Does your iio subsystem have all functions which attached kxsd9
>>> driver has? i.e.
>>> a. works in polling mode
>> Yes,
>>> b. works in interrupt mode
>> No, but only because I was utilizing it as an example of a minimalist
>> driver.
>> See the lis3l02dq driver in the git tree for how it would be extended.
>>
>> Firstly, beware, the interrupt case for this chip is very different to
>> the more
>> common cases where the interrupt in question indicates the
>> availability of a new
>> reading.
>>
>> As a more general comment, I'd also argue that the reading you
>> take on interrupt is irrelevant. The event was the threshold pass, not
>> the
>> value of the reading. It should be up to userspace to decide if it
>> cares what
>> the reading is and if it does, it has a polling mode with which to
>> read it.
>> As I read the data sheet, the device isn't storing the accelerations at
>> interrupt, so the value you are reading is not relevant to the event
>> at all, but
>> typically will be another reading.
>>
>> I like the code you have for bringing it up from low power mode to
>> grab a current
>> value and then go back to sleep. I haven't seen this functionality in
>> many chips.
>>
>> There are a couple of ways that it could be handled.
>> 1) Register the MOTLat event to be passed up to userspace and let
>> userspace take
>>    a reading via the normal poll modes. (pretty much what would happen
>> in input)
>>
>> 2) Configure this as and IIO trigger which would allow it to be used
>> to 'trigger'
>>    the reading of a number of other sensors as well as the acceleration.
>>
>> It should be possible to support both of these but as per your earlier
>> comment
>> about not adding features until they are needed, I'd advocate only
>> supporting the
>> first option until the someone needs the second.
>>
>>> c. provide an interface to user space in order setting a few parameters
>>> and read x, y, z variances.
>> Yes, via sysfs.
>>  
>>> 2) How long will it take to your iio subsystem is merged into mainline?
>> Not sure - I still don't have enough reviewers. As far as I'm
>> concerned, bar
>> changes relating to reviews it's been ready for merging for a couple of
>> months.  Since the last posting that got almost no responses the only
>> changes
>> have been minor changes and comment clean ups.
>> My intention is to shortly seek advice on how to go about doing this, but
>> my current feeling is that it will have to be done in stages.  The idea
>> is that each stage will involve a relatively small code base and hence
>> be easier for reviewers to handle.  The white paper was written to assist
>> with this process by providing an overview of how everything links
>> together
>> without reviewers having to read the full code.
>>
>> 1) The core functionality.   This gives us something very close to
>> input with
>> the principal changes as described above.
>>
>> 2) A set of example drivers
>>
>> At this point we will have a common framework against which to merge
>> new drivers.
>>
>> 3) The advanced features (triggering and ring buffering)
>>
>> 4) The drivers for devices requiring 3.
>>  
>>> I have quick review of you iio subsytem. Sorry but I think it' somewhat
>>> complicated and have a lot of functions which are still not needed.
>> By you - I repeat that I have been using this system in anger for about 6
>> months now.  I only know of one bit of functionality in there that I'm
>> not
>> currently using (the first of the trigger lists) and that is simply
>> because
>> I haven't completed the driver that uses it as yet and dropping it
>> would be
>> a trivial change.
>>> And
>>> it seems like your coding style doesn't look like Linux coding style
>>> that I'm familiar with.
>> As far as I know the code absolutely conforms to every coding style
>> convention of the kernel.  The version in the 'mess' branch has a few
>> comment
>> clean ups as a result of checking the kernel doc comments.  If you have
>> specific examples please send them on and I'll be happy to clean them up.
> 
> For instance, please
> drivers/industrialio/ -exec ./scripts/checkpatch.pl --file {} \;
Ok, I'm seeing a couple of minor formatting errors which I typically only
clean up properly when I am posting a set to lkml.  Those sets are always
verified with checkpatch. What you are looking at is my development tree
not one that I've carefully checked before posting.  Some of the errors
are entirely deliberate.  I frequently use c99 comments in order to
indicate that I need to clarify them before formally posting patches for
reviews (precisely because checkpatch ensures I don't forget to do it!)

If that's what you mean by 'your coding style doesn't look like Linux coding
style' then you are massively exaggerating.
If these sort of things really cause you trouble then look at the formal
submissions to lkml. If I hadn't wanted to make the latest and most stable
version of our internal code available as it was relevant to this discussion
then I would have waited until I had time to clean up trivial formating
errors.

> 
>>> I just hope we can add some accelerometer feature into mainline kernel
>>> as soon as possible ;)
>> I agree.  The stop gap solution (which a number drivers have taken) is
>> to put
>> them in drivers/misc on the basis that when we do have an agreed
>> system we can
>> then move them over at a later date.  I have already offered to do the
>> conversions for a number of such drivers when iio gets merged.
>>
>> Jonathan
>>
> 
> Finally,
> Are you OK if someone trys making another framework for input(?)-sensor
> or accelerometer.
Of course I have no objections.

If people want a unified framework useful for all the common applications
of these devices then it will need to contain equivalents of what IIO does, if
as previously discussed it makes more sense to have separate drivers then
go ahead.  I'd advocate posting your current driver to the input list and
inquiring what their opinions are on the matter.

My personal opinion on this is:

1) The way accelerometers are currently used for input is very simple, much
more complex systems using information derived from multiple sensors will
become common in the future (similar to the new wii remotes) or the stuff
done by companies like Xsens.  This sort of stuff will require some elements
similar to IIO (principally synchronized triggered capture from multiple
sensors).

2) For the level of device support you are talking about, there is no direct
reason it shouldn't go into the main input system. There is no need for a new
subsystem. Over to Dmitry to point out what I'm missing.  For example, the lis3lv02
driver (in hwmon) does exactly this.  That driver was accepted
by the input guys. Perhaps a drivers/input/accelerometer directory makes sense
to group the drivers.

Jonathan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-22 13:35                   ` Jonathan Cameron
@ 2009-05-25  8:15                     ` Kim Kyuwon
  2009-05-25  8:52                       ` Mohamed Ikbel Boulabiar
  2009-05-25 11:07                       ` Jonathan Cameron
  0 siblings, 2 replies; 22+ messages in thread
From: Kim Kyuwon @ 2009-05-25  8:15 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: felipe.balbi, Kim Kyuwon, Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kyungmin Park,
	Jean Delvare, Zhang, Xing Z, Gross, Mark, jketreno, Trisal,
	Kalhan

Jonathan Cameron wrote:
> Hi Kyuwon,
>> Jonathan Cameron wrote:
>>> No, the conclusion was to wait and see if this is practical. It was
>>> kept in
>>> mind during the several iterations of design of IIO, but the focus of
>>> development was always high performance capture and handling all the
>>> facilities
>>> of these highly variable sensors as consistently as possible.
>>>
>>> I guess now we are in a position to see if it is indeed practical to
>>> add input
>>> support.
>> Thank you for your clarification.
>>
>>>> I can't understand why you inserted ring buffers related stuffs in iio.
>>>>  Please let me understand in easy words. And please note that in
>>>> /Documentation/SubmittingPatches
>>> Because the hardware I have uses hardware ring buffers e.g. VTI
>>> SCA3000 series,
>>> analog adxl345.  More and more sensors are moving this way as without
>>> a true
>>> real time os, it's the only way of ensuring high data rate data is
>>> available
>>> for sophisticated almost real time processing. 
>> How can you be so sure that more and more sensors use h/w ring buffer?
>> Is there any data or experts who supports your point?
> I'm basing this on observation and believe me I use a lot of different
> sensors and work with people who use a lot more. I'm not going to waste
> my timing getting them to post a series of 'opinions' on this list when
> a bit of Googling and looking at release dates of sensors will give you
> the same information.
> 
> Until recently the only ones I had encountered were VTI's.  Now Analog have
> started producing a range of sensors with various forms of triggered buffer.
> Take a look at their 'new' page.  Admittedly some of the newer ones don't
> fall easily into my current model so some rethinking of elements of it will
> be needed.  The reason none of the below is currently supported is that I
> haven't been able to get them as yet. I'm particularly interested in the
> 16240 for some athlete monitoring applications.
> 
> We have:
> 
> * ADIS16220  (I hadn't even seen this one before) Digital Vibration sensor.
> 	This has a 1024 sample buffer with various possible triggering modes.
> 
> * ADIS16240 Impact sensor and recorder 3x8192 sample buffer using triggered events.
> 
> * ADXL346 Accelerometer. 32 levels of output data FIFO minimizes host processor
> 	load.
> 
> * ADXL335 Analog sensor so unsurprisingly doesn't feature a digital buffer.
> 
> If we hit the more button and select products in the last year we also gain 
> the ADXL345 wich yet again provides a fifo to reduce load on the processor.
> Before that one, I don't think any of Analog's products featured such buffering.
> 
> So we now have only 2 different manufacturers making sensors with ring buffers
> on them. One of which has put them onto 5 out of their last 6 products (though
> I'll admit the 345 and 346 are extremely closely related).  Looks like
> they think there is a market.
> 
>> And although assuming you are right here, STILL only your SCA3000 series
>> have h/w ring buffer. Thus, I think there is no reason the framework
>> should have ring buffer feature
>>
>>> You can't afford to have gaps
>>> in your data.
>>>
>>>  Also, for my primary application (fast, consistent capture
>>> from a number of different sensors) this is the simplest design that
>>> provides
>>> the performance needed.  We spent a lot of time trying to do this in
>>> other
>>> ways.
>> Your words('simplest' and 'spending a lot of time') are subjective.
> Yes, ultimately I wrote a system that fulfilled my requirements, not one
> that was designed to meet yours. Let me reemphasize that the framework
> is very much designed to be modular.  If you don't want ring buffers then
> you don't have to use them.  Their presence has NO effect whatsoever on
> drivers that aren't using them.  In fact, even when the driver does support
> them, I've mostly made it a Kconfig option on a per driver basis.
> 
> For my typical application the more esoteric triggering methods are also
> vital.  If I want to synchronize a linux based mote's capture of an
> accelerometer with a high end motion capture system and not miss a single
> sample, within an adaptable framework then I need pretty much every element
> currently in the IIO system.  Likewise, if I want regular 2Khz sampling of
> an ADC connected via 400Khz i2c then I need that to be supported.  Yes these
> aren't your applications, which I'm assuming from your post are control of
> a pretty user interface, but they are an import class of applications for
> other users within both the academic world and industry.
> 
> 
>>>> 644 4) Don't over-design.
>>>> 645
>>>> 646 Don't try to anticipate nebulous future cases which may or may not
>>>> 647 be useful:  "Make it as simple as you can, and no simpler."
>>> The are useful. I'm using them in real applications every day.  It
>>> might not
>>> be your use case but it is a number of other peoples (some of whom are
>>> cc'd
>>> above).
>> Our objective is merging code upstream. We have lots of kernel drivers
>> and kernel features which we are using everyday However, I know these
>> are can't be merged kernel.
> This is not the place to get into that sort of argument so I'll keep my
> reply to this short.
> 
> Why not?  If you have something that is useful, then why not submit it?

I didn't say we have something useful.

> This is particularly true in cases like this were the useful element
> doesn't change anything already in kernel and hence if people don't want
> it then it won't effect them.

If we submit ugly and messy drivers and kernel features, none would be 
happy.

>>> The absolutely crucial thing about the design is that if you don't
>>> want ring
>>> buffers, then don't use them (by which I mean don't compile them in -
>>> it's a
>>> modular design so if you don't want them you don't have to have them.)
>> I'm just thinking the right functions should be located in the right
>> position. If only SCA3000 series use ring buffer, it should be in the
>> SCA3000 series driver.
> It's not only the SCA3000 series.
> Note whilst they may not matter for your applications the support
> for software ring buffering in kernel is still absolutely vital to me. Also
> see the devices cited above.
>>> In the first instance I probably wouldn't be pushing that element into
>>> the kernel.
>>> As you are illustrating this is controversial and needs a thorough
>>> discussion.
>>>
>>> At that point the whole system reduces to  much simpler core which
>>> handles
>>> the sysfs interface registration and management of chrdevs associated
>>> with events.
>>> At this level it's fairly similar to input, but without the event
>>> aggregation and
>>> with things like event escalation and dynamic chrdev allocation.
>> We already have a nice 'input subsystem'. why not just using 'input
>> subsystem'
> See the original discussion and remember that we are dealing with application
> cases beyond using these devices for human input. To quote Dmitry:
> 
>> I don't think that input subsystem would be the best choice. While we
>> do support the event-driven mechanism for delivering information to
>> userspace input is mostly oriented for human interface devices, not
>> general data acquisition. If anything input_dev is too fat. Another
>> thing is that input layer anonymizes input devices, makes them
>> capability-oriented. I.e. we dont really care what model of joystick
>> we have, we want something that reports ABS_X, ABS_Y, BTN_FIRE and
>> userpsace can use it whether it a simple analog joystick or something
>> fancy and USB connected. For your purposes you probably do want to know
>> what the device is and what exactly being sampled/measured.
> 
>> I'd go with a lean and mean new subsytem.
> (http://lkml.org/lkml/2008/5/21/186)
> 
> Now, you might argue that the resulting subsystem is not lean and mean,
> but to my mind it comes down to a case of what code actually runs on a
> given event.  Large chunks of the core subsystem are concerned with
> simplifying shared functionality between different devices,
> but in most use cases the run path from events happening to them hitting
> userspace is as lean and mean as possible whilst maintaining flexibility
> where needed.
> 
> If you don't care about he anonymous nature of devices under input then
> fine, submit your drivers to their system.  Then if the sensor in question
> becomes useful to someone who does need some of the additional functionality
> that iio provides we will end up with a second driver.  It's exactly this
> case that lead to the conclusion in the original discussion that there may
> be circumstances where a given device has several separate drivers within
> the kernel. Conclusion was that if the requirements were different enough
> and they could not be easily merged, this may be the best way to go.
>>>> I can't understand. Why we should make accelerometer(or sensor)
>>>> framework relate to ADC?
>>> There are a couple of reasons:
>>> 1) Accelerometers are effectively analog sensors wired up to an ADC.
>>> As such
>>> they share a lot of common features.
>>> 2) Many real accelerometer rigs out there actually use a pair of chips,
>>> so as far as Linux is concerned you are talking to an adc not the
>>> accelerometer
>>> on the other side. Handling this case is still an open issue and it
>>> was to this
>>> that I was referring above.
>> If there is still an open issue, it is difficult to merge this IIO
>> subsystem to upstream kernel.
> I disagree. It's irrelevant to merging the subsystem. You don't have to 
> have every use case and every possible setup handled by a subsystem from
> inception.  For now, if you really need your adc connected accelerometer
> to export 'accel_x accel_y' etc then you can trivially write a driver
> for the whole system.  This would obviously lead to a certain amount of
> code duplication and hence I'm suggesting that the question of whether
> this should be a userspace element (similar to lm-sensors config files)
> or the linkages should be specified in kernel (similar to ASoC).
>>>> My two accelerometer are not in this cases. 
>>> Indeed not.  But many others are and here we are talking about a general
>>> framework.
>>>> If really sensors are related to ADC, it's better make this as
>>>> library or
>>>> application.
>>> That is an open question.  There are cases where it would have to be done
>>> in kernel (where both the accelerometer and the adc have separate control
>>> interfaces). 
>> Then you can make another device driver module or API that can help
>> intercommunicate between two devices.
> Yes.  That may be the way to go down the line.
>>>  Why don't we to be simple. Please let me understand in easy
>>>> words.
>>>>
>>>> Let me ask a few question in conclusion.
>>> Firstly thanks for sending this driver, its always enlightening to see
>>> how people have handled such a device.  Out of interest, have you
>>> posted this to the input list?  I'd be interested to see what their
>>> comments are.
>> I can't add this driver to input list, because this kxsd9 driver is for
>> accelerometer.
> Why does this prevent you posting it there?  It is using the input framework
> so if it was going anywhere in the kernel it would require a review and ACK
> from the relevant people on that list in much the same way as it should also
> be posted to the spi / i2c list as appropriate for review. 

OK,

To. Dmitry,
Can I merge my kxsd9 *accelerometer* driver to input system?

>>>> 1) Does your iio subsystem have all functions which attached kxsd9
>>>> driver has? i.e.
>>>> a. works in polling mode
>>> Yes,
>>>> b. works in interrupt mode
>>> No, but only because I was utilizing it as an example of a minimalist
>>> driver.
>>> See the lis3l02dq driver in the git tree for how it would be extended.
>>>
>>> Firstly, beware, the interrupt case for this chip is very different to
>>> the more
>>> common cases where the interrupt in question indicates the
>>> availability of a new
>>> reading.
>>>
>>> As a more general comment, I'd also argue that the reading you
>>> take on interrupt is irrelevant. The event was the threshold pass, not
>>> the
>>> value of the reading. It should be up to userspace to decide if it
>>> cares what
>>> the reading is and if it does, it has a polling mode with which to
>>> read it.
>>> As I read the data sheet, the device isn't storing the accelerations at
>>> interrupt, so the value you are reading is not relevant to the event
>>> at all, but
>>> typically will be another reading.
>>>
>>> I like the code you have for bringing it up from low power mode to
>>> grab a current
>>> value and then go back to sleep. I haven't seen this functionality in
>>> many chips.
>>>
>>> There are a couple of ways that it could be handled.
>>> 1) Register the MOTLat event to be passed up to userspace and let
>>> userspace take
>>>    a reading via the normal poll modes. (pretty much what would happen
>>> in input)
>>>
>>> 2) Configure this as and IIO trigger which would allow it to be used
>>> to 'trigger'
>>>    the reading of a number of other sensors as well as the acceleration.
>>>
>>> It should be possible to support both of these but as per your earlier
>>> comment
>>> about not adding features until they are needed, I'd advocate only
>>> supporting the
>>> first option until the someone needs the second.
>>>
>>>> c. provide an interface to user space in order setting a few parameters
>>>> and read x, y, z variances.
>>> Yes, via sysfs.
>>>  
>>>> 2) How long will it take to your iio subsystem is merged into mainline?
>>> Not sure - I still don't have enough reviewers. As far as I'm
>>> concerned, bar
>>> changes relating to reviews it's been ready for merging for a couple of
>>> months.  Since the last posting that got almost no responses the only
>>> changes
>>> have been minor changes and comment clean ups.
>>> My intention is to shortly seek advice on how to go about doing this, but
>>> my current feeling is that it will have to be done in stages.  The idea
>>> is that each stage will involve a relatively small code base and hence
>>> be easier for reviewers to handle.  The white paper was written to assist
>>> with this process by providing an overview of how everything links
>>> together
>>> without reviewers having to read the full code.
>>>
>>> 1) The core functionality.   This gives us something very close to
>>> input with
>>> the principal changes as described above.
>>>
>>> 2) A set of example drivers
>>>
>>> At this point we will have a common framework against which to merge
>>> new drivers.
>>>
>>> 3) The advanced features (triggering and ring buffering)
>>>
>>> 4) The drivers for devices requiring 3.
>>>  
>>>> I have quick review of you iio subsytem. Sorry but I think it' somewhat
>>>> complicated and have a lot of functions which are still not needed.
>>> By you - I repeat that I have been using this system in anger for about 6
>>> months now.  I only know of one bit of functionality in there that I'm
>>> not
>>> currently using (the first of the trigger lists) and that is simply
>>> because
>>> I haven't completed the driver that uses it as yet and dropping it
>>> would be
>>> a trivial change.
>>>> And
>>>> it seems like your coding style doesn't look like Linux coding style
>>>> that I'm familiar with.
>>> As far as I know the code absolutely conforms to every coding style
>>> convention of the kernel.  The version in the 'mess' branch has a few
>>> comment
>>> clean ups as a result of checking the kernel doc comments.  If you have
>>> specific examples please send them on and I'll be happy to clean them up.
>> For instance, please
>> drivers/industrialio/ -exec ./scripts/checkpatch.pl --file {} \;
> Ok, I'm seeing a couple of minor formatting errors which I typically only
> clean up properly when I am posting a set to lkml.  Those sets are always
> verified with checkpatch. What you are looking at is my development tree
> not one that I've carefully checked before posting.  Some of the errors
> are entirely deliberate.  I frequently use c99 comments in order to
> indicate that I need to clarify them before formally posting patches for
> reviews (precisely because checkpatch ensures I don't forget to do it!)

Your development tree is opened to other developers and other developers 
would evaluate you though the code in your development tree. You can use 
enough c99 comments and other checkpatch errors just in your local repo.

> If that's what you mean by 'your coding style doesn't look like Linux coding
> style' then you are massively exaggerating.

I said "For instance".

> If these sort of things really cause you trouble then look at the formal
> submissions to lkml. If I hadn't wanted to make the latest and most stable
> version of our internal code available as it was relevant to this discussion
> then I would have waited until I had time to clean up trivial formating
> errors.
> 
>>>> I just hope we can add some accelerometer feature into mainline kernel
>>>> as soon as possible ;)
>>> I agree.  The stop gap solution (which a number drivers have taken) is
>>> to put
>>> them in drivers/misc on the basis that when we do have an agreed
>>> system we can
>>> then move them over at a later date.  I have already offered to do the
>>> conversions for a number of such drivers when iio gets merged.
>>>
>>> Jonathan
>>>
>> Finally,
>> Are you OK if someone trys making another framework for input(?)-sensor
>> or accelerometer.
> Of course I have no objections.

Thanks,

> If people want a unified framework useful for all the common applications
> of these devices then it will need to contain equivalents of what IIO does, if
> as previously discussed it makes more sense to have separate drivers then
> go ahead.  I'd advocate posting your current driver to the input list and
> inquiring what their opinions are on the matter.
> 
> My personal opinion on this is:
> 
> 1) The way accelerometers are currently used for input is very simple, much
> more complex systems using information derived from multiple sensors will
> become common in the future (similar to the new wii remotes) or the stuff
> done by companies like Xsens.  This sort of stuff will require some elements
> similar to IIO (principally synchronized triggered capture from multiple
> sensors).
> 
> 2) For the level of device support you are talking about, there is no direct
> reason it shouldn't go into the main input system. There is no need for a new
> subsystem. Over to Dmitry to point out what I'm missing.  For example, the lis3lv02
> driver (in hwmon) does exactly this.  That driver was accepted
> by the input guys. Perhaps a drivers/input/accelerometer directory makes sense
> to group the drivers.

I really respect your enthusiasm.
Thank you
Regards,

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-25  8:15                     ` Kim Kyuwon
@ 2009-05-25  8:52                       ` Mohamed Ikbel Boulabiar
  2009-05-25 11:07                       ` Jonathan Cameron
  1 sibling, 0 replies; 22+ messages in thread
From: Mohamed Ikbel Boulabiar @ 2009-05-25  8:52 UTC (permalink / raw)
  To: Kim Kyuwon
  Cc: Jonathan Cameron, felipe.balbi, Kim Kyuwon, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kyungmin Park,
	Jean Delvare, Zhang, Xing Z, Gross, Mark, jketreno, Trisal,
	Kalhan

Hi,

I haven't read all documents cited here (IIO, ...), but it seems that
there is a big attention made on performance.

Can we think also in the way information can be shown for a device.
To be concrete, I actually work on some multi-touch tablets, and it is
useful to create 2 or more "views" of the device.

A device file that emulate a mouse when only one contact is on the
tablet, and another showing more general information about all
contacts.
The first is used by simple applications, the other by more advanced
ones that can treat complex multi-touch events.

This is only one use case, but like in the databases, maybe simple
user (or predefined config. by other users) can propose other "views"
of a device.

Then maybe we would need a configurator (or text configuration files)
for creating these views.

I am not also very convinced why events matching are "cabled" inside
driver source code.
The matching between information sent by device and system events
(ABS_X, ...) is inside source code and any modification needs
recompilation.
What if someone should switch between 2 different matching ?

---
ikbel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Add Input IOCTL for accelerometer devices
  2009-05-25  8:15                     ` Kim Kyuwon
  2009-05-25  8:52                       ` Mohamed Ikbel Boulabiar
@ 2009-05-25 11:07                       ` Jonathan Cameron
  1 sibling, 0 replies; 22+ messages in thread
From: Jonathan Cameron @ 2009-05-25 11:07 UTC (permalink / raw)
  To: Kim Kyuwon
  Cc: felipe.balbi, Kim Kyuwon, Mohamed Ikbel Boulabiar, Trilok Soni,
	linux-kernel, Dmitry Torokhov, linux-input, Kyungmin Park,
	Jean Delvare, Zhang, Xing Z, Gross, Mark, jketreno, Trisal,
	Kalhan

Hi Kyuwon,

I've cropped this conversation down a fair bit in an attempt to make
it more focused.

...
>>> Our objective is merging code upstream. We have lots of kernel drivers
>>> and kernel features which we are using everyday However, I know these
>>> are can't be merged kernel.
>> This is not the place to get into that sort of argument so I'll keep my
>> reply to this short.
>>
>> Why not?  If you have something that is useful, then why not submit it?
> 
> I didn't say we have something useful.
Fair enough, though that does beg the question of why you are using them!
 
>> This is particularly true in cases like this were the useful element
>> doesn't change anything already in kernel and hence if people don't want
>> it then it won't effect them.
> 
> If we submit ugly and messy drivers and kernel features, none would be
> happy.
Well if you don't have time to clean them up, that's one of the major
reasons for the existence of the staging tree.  Obviously it's a hotly
debated topic of whether that tree is a good idea, but I'd rather there
was a dubious driver readily available to other developers than none
at all.
> 

>> Yes.  That may be the way to go down the line.
>>>>  Why don't we to be simple. Please let me understand in easy
>>>>> words.
>>>>>
>>>>> Let me ask a few question in conclusion.
>>>> Firstly thanks for sending this driver, its always enlightening to see
>>>> how people have handled such a device.  Out of interest, have you
>>>> posted this to the input list?  I'd be interested to see what their
>>>> comments are.
>>> I can't add this driver to input list, because this kxsd9 driver is for
>>> accelerometer.
>> Why does this prevent you posting it there?  It is using the input
>> framework
>> so if it was going anywhere in the kernel it would require a review
>> and ACK
>> from the relevant people on that list in much the same way as it
>> should also
>> be posted to the spi / i2c list as appropriate for review. 
> 
> OK,
> 
> To. Dmitry,
> Can I merge my kxsd9 *accelerometer* driver to input system?
> 
... left the next bit in as I think it might be useful for review
purposes on the input list.

>>>>
>>>> As a more general comment, I'd also argue that the reading you
>>>> take on interrupt is irrelevant. The event was the threshold pass, not
>>>> the
>>>> value of the reading. It should be up to userspace to decide if it
>>>> cares what
>>>> the reading is and if it does, it has a polling mode with which to
>>>> read it.
>>>> As I read the data sheet, the device isn't storing the accelerations at
>>>> interrupt, so the value you are reading is not relevant to the event
>>>> at all, but
>>>> typically will be another reading.
>>>>> it seems like your coding style doesn't look like Linux coding style
>>>>> that I'm familiar with.
>>>> As far as I know the code absolutely conforms to every coding style
>>>> convention of the kernel.  The version in the 'mess' branch has a few
>>>> comment
>>>> clean ups as a result of checking the kernel doc comments.  If you have
>>>> specific examples please send them on and I'll be happy to clean
>>>> them up.
>>> For instance, please
>>> drivers/industrialio/ -exec ./scripts/checkpatch.pl --file {} \;
>> Ok, I'm seeing a couple of minor formatting errors which I typically only
>> clean up properly when I am posting a set to lkml.  Those sets are always
>> verified with checkpatch. What you are looking at is my development tree
>> not one that I've carefully checked before posting.  Some of the errors
>> are entirely deliberate.  I frequently use c99 comments in order to
>> indicate that I need to clarify them before formally posting patches for
>> reviews (precisely because checkpatch ensures I don't forget to do it!)
> 
> Your development tree is opened to other developers and other developers
> would evaluate you though the code in your development tree. You can use
> enough c99 comments and other checkpatch errors just in your local repo.

In general I'd agree with that observation, but in common with patches in
numerous other 'development' trees including for example the staging there
are elements of my trees that are not 'clean'.   Also note that the 'mess'
branch was a direct dump of my local repository as there were a couple of
bug fixes in there that I haven't had time to clean up properly yet.
 
>> If that's what you mean by 'your coding style doesn't look like Linux
>> coding
>> style' then you are massively exaggerating.
> 
> I said "For instance".
Fair enough. I'd appreciate any comments when I do a formal posting of
the patch series.
> 
>> If these sort of things really cause you trouble then look at the formal
>> submissions to lkml. If I hadn't wanted to make the latest and most
>> stable
>> version of our internal code available as it was relevant to this
>> discussion
>> then I would have waited until I had time to clean up trivial formating
>> errors.
>>
>>>>> I just hope we can add some accelerometer feature into mainline kernel
>>>>> as soon as possible ;)
>>>> I agree.  The stop gap solution (which a number drivers have taken) is
>>>> to put
>>>> them in drivers/misc on the basis that when we do have an agreed
>>>> system we can
>>>> then move them over at a later date.  I have already offered to do the
>>>> conversions for a number of such drivers when iio gets merged.
>>>>
>>>> Jonathan
>>>>
>>> Finally,
>>> Are you OK if someone trys making another framework for input(?)-sensor
>>> or accelerometer.
>> Of course I have no objections.
> 
> Thanks,
You are welcome. I'd also be happy to assist with reviewing etc of devices
I actually have.  If the input guys are happy with your driver in general,
I'd particularly like you to address my question about the nature of the
data in the interrupt driven event as mentioned above.
> 
>> If people want a unified framework useful for all the common applications
>> of these devices then it will need to contain equivalents of what IIO
>> does, if
>> as previously discussed it makes more sense to have separate drivers then
>> go ahead.  I'd advocate posting your current driver to the input list and
>> inquiring what their opinions are on the matter.
>>
>> My personal opinion on this is:
>>
>> 1) The way accelerometers are currently used for input is very simple,
>> much
>> more complex systems using information derived from multiple sensors will
>> become common in the future (similar to the new wii remotes) or the stuff
>> done by companies like Xsens.  This sort of stuff will require some
>> elements
>> similar to IIO (principally synchronized triggered capture from multiple
>> sensors).
>>
>> 2) For the level of device support you are talking about, there is no
>> direct
>> reason it shouldn't go into the main input system. There is no need
>> for a new
>> subsystem. Over to Dmitry to point out what I'm missing.  For example,
>> the lis3lv02
>> driver (in hwmon) does exactly this.  That driver was accepted
>> by the input guys. Perhaps a drivers/input/accelerometer directory
>> makes sense
>> to group the drivers.
> 
> I really respect your enthusiasm.
Thanks. This conversation has definitely emphasized a few issues we
left 'for later discussion' in the original threads.

Jonathan

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2009-05-25 11:08 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-15 13:16 [RFC] Add Input IOCTL for accelerometer devices Felipe Balbi
2009-05-15 18:06 ` Trilok Soni
2009-05-15 19:30   ` Mohamed Ikbel Boulabiar
2009-05-15 20:02     ` Felipe Balbi
2009-05-16  4:19       ` Mohamed Ikbel Boulabiar
2009-05-16 12:50         ` Felipe Balbi
2009-05-18  7:45       ` Kim Kyuwon
2009-05-18  9:12         ` Felipe Balbi
2009-05-18 10:11           ` Jonathan Cameron
2009-05-18 10:31             ` Felipe Balbi
2009-05-18 11:37               ` Jonathan Cameron
2009-05-19  6:32             ` Kim Kyuwon
2009-05-21 11:42               ` Jonathan Cameron
2009-05-22  8:21                 ` Kim Kyuwon
2009-05-22 13:35                   ` Jonathan Cameron
2009-05-25  8:15                     ` Kim Kyuwon
2009-05-25  8:52                       ` Mohamed Ikbel Boulabiar
2009-05-25 11:07                       ` Jonathan Cameron
2009-05-19  2:41           ` Kim Kyuwon
2009-05-19 10:42             ` Felipe Balbi
2009-05-19 12:34               ` Mohamed Ikbel Boulabiar
2009-05-21 10:21                 ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).