On 9/29/09, Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxxx> wrote:
> On Tue, Sep 29, 2009 at 10:20:44AM +0100, Alan Jenkins wrote:
> Hello, Allan.
>
> Thanks for the feedback. I am considering an investigation whether we
> have lots of other ACPI input devices that could share some code like
> {add,remove}_acpi_notify_device. Regarding the driver naming, I will
> send a second version with it and other modifications considering your
> feedback and that of other people too.
>
> I will also ask for some explicit feedback and add linux-input and
> Dmitry to the loop.
>
I'll leave the input questions for the list. I don't have any
suggestions about your choice of keycode for the house key, or how to
calibrate accelerometers.
>> > +struct device_attribute cmpc_accel_sense_attr = {
>> > + .attr = { .name = "sense", .mode = 0220 },
>> > + .store = cmpc_accel_sense_store
>> > +};
> This changes the accelerometer device sensitivity. I will take a look at
> the ACPI tables to get a range. If very much sensitive, the device will
> generate too much ACPI notifications, even when the classmate PC is no a
> table and there seems to be no movement. If not very much sensitive, you
> must throw it spinning into the air to get anything. :-)
>
> I will pick a default value, although I think we could let it to
> userspace, but a sensible default value will not hurt here. As far as I
> know, there is no ACPI method to do get_sense, but we can mirror the
> last value written and provide a .show.
>
> What do you recommend as a documentation? Something at
> Documentation/ABI/testing/, perhaps? I don't know if there are any other
> devices with something similar, but I would not be surprised if there
> are some of them.
Documentation/ABI seems reasonable; "sysfs-platform-eeepc-laptop" was
accepted there.
You could try (either in place of documentation, or in addition to)
making the interface self-explanatory. Call the attribute
"sensitivity", and add a "max_sensitivity" attribute. It would also
make the interface more generic.
>> > +static void cmpc_tablet_idev_init(struct input_dev *inputdev)
>> > +{
>> > + set_bit(EV_SW, inputdev->evbit);
>> > + set_bit(SW_TABLET_MODE, inputdev->swbit);
>>
>> Don't you need to initialize the switch value here?
>>
>
> No, input_allocate_device does kzalloc. Otherwise, this would apply to
> the other bitmaps as well.
The other bitmaps aren't for switches though. Here's what prompted
this comment - a snippet from the old (2.6.29) version of
Documentation/rfkill.txt:
"2. Input device switches (sources of EV_SW events) DO store their current state
(so you *must* initialize it by issuing a gratuitous input layer event on
driver start-up and also when resuming from sleep), and that state CAN be
queried from userspace through IOCTLs."
>> Also, have you tested this with changing the switch value while
>> suspended? I _think_ you need to update the switch state on resume.
>> This is purely from looking at other acpi drivers and their evolution;
>> I don't have any practical experience with input drivers.
>>
>
> Interesting point. I will do the testing.
Thanks
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
