Re: [RFC PATCHv2 4/5] hwmon: lis3: Remove the referencies to theglobal variable in core driver

[Thadeu Lima de Souza Cascardo]

On Thu, Apr 07, 2011 at 02:59:48PM +0300, Ilkka Koskinen wrote:
> 
> Hi,
> 
> Thanks for the comments!
> 
> On Tue, 5 Apr 2011, ext Thadeu Lima de Souza Cascardo wrote:
> >On Tue, Apr 05, 2011 at 05:45:13PM +0300, Ilkka Koskinen wrote:
> >>Signed-off-by: Ilkka Koskinen <ilkka.koskinen@xxxxxxxxx>
> >>---
> >> drivers/misc/lis3lv02d/lis3lv02d.c |  237 ++++++++++++++++++++----------------
> >> drivers/misc/lis3lv02d/lis3lv02d.h |    3 +
> >> 2 files changed, 135 insertions(+), 105 deletions(-)
> 
> >>@@ -980,14 +1003,18 @@ int lis3lv02d_init_device(struct lis3lv02d *lis3)
> >> 				thread_fn,
> >> 				IRQF_TRIGGER_RISING | IRQF_ONESHOT |
> >> 				irq_flags,
> >>-				DRIVER_NAME, &lis3_dev);
> >>+				DRIVER_NAME, lis3);
> >>
> >> 	if (err < 0) {
> >> 		pr_err("Cannot get IRQ\n");
> >> 		goto out;
> >> 	}
> >>
> >>-	if (misc_register(&lis3lv02d_misc_device))
> >>+	lis3->miscdev.minor	= MISC_DYNAMIC_MINOR;
> >>+	lis3->miscdev.name	= "freefall";
> >>+	lis3->miscdev.fops	= &lis3lv02d_misc_fops;
> >>+
> >>+	if (misc_register(&lis3->miscdev))
> >> 		pr_err("misc_register failed\n");
> >
> >You should not use miscdevice in case there will be multiple devices.
> >First, it will fail. There cannot be more than one device with the same
> >name. Second, current dynamic minor devices is restricted to 64 devices.
> >Since this is reserved to one-shot devices, freefall was OK as a misc
> >device until you fixed it to allow multiple freefall devices.  :-)
> 
> Ah, true :)
> 
> >So, I'd recommend switching to a new device class, and have freefall0,
> >freefall1, etc.
> 
> I wonder if introducing a new class makes sense. I mean, I can
> figure out use cases for several accelerometers but for several free
> fall sensors? :/
> 
> Would it be better to add a mechanism to tell to the core module if
> the particular device is used for free fall detection or not?
> 
> Cheers, Ilkka
> 

I would suggest an input handler or just letting userspace handle that
using input events. Then, I checked out the code and realized that this
is done by the hardware, using an interrupt. Should we handle said
interrupt as a particular kind of event (perhaps a new misc event)?

I've written classmate-laptop driver, which has an accelerometer too
(using ACPI). Should we start documenting how accelerometer drivers
should be written? Input event devices are the best class, but I've seen
cases where drivers use sysfs files. In fact, I've just checked and seen
that lis3lv02d does that too, in the file position. But I've seen
drivers out there that use one file per axis. I guess that's the way
hwmon drivers usually do things, creating sysfs files.

Should we start standardizing these drivers? I think input devices are
good enough to report the data, including for 2-axis devices (I have one
SPI "inclinometer" here - how those should be distinguished?). sysfs
files would be nice for setting parameters. classmate-laptop, for
example, has a sensitivity parameter. Besides freefall, lis3 seems to
have a selftest function and a rate setting.

One last email in linux-input and pdx lists has requested a new MISC
event to report a change in direction for some devices. Although I think
freefall and change in direction could be computed using the
accelerometer values in userspace, these devices do it directly. How
should we handle them in a standard way?

I would like to gather some opinions before writing a draft of
recommendations for writing such drivers. Is linux-input the best forum
for this?

Regards,
Cascardo.


> >Anyway, good job on this.
> >
> >Regards,
> >Cascardo.
>

Attachment: signature.asc
Description: Digital signature