Re: [PATCH v3 1/1] input: add driver for Bosch Sensortec's BMA150 accelerometer
From: Eric Andersson <hidden>
Date: 2011-07-15 11:00:34
Also in:
lkml
Thanks for your input Jonathan!
quoted
quoted
quoted
+ values represent the range given as +/- G. + Possible values are: 2, 4, 8. + + Reading: returns the current acceleration range. + + Writing: sets a new acceleration range.Same comment as last time - these values are not discoverable so it should set the nearest bigger range.And same answer as last time - The values can be retreived by doing a: cat range. This whole idea comes from a review comment that Jonathan Cameron gave on your bma023 driver [1].Needs documenting. Right now there is no mention of that here.
True. I'll add a comment on how to get the valid values in v4.
quoted
If we could agree on how these values should be represented I will be glad to fix it! Dmitry, any preferences?quoted
[1] http://www.spinics.net/lists/linux-input/msg14271.htmlquoted
quoted
+ for (i = 0, ret = 0; i < ARRAY_SIZE(bw_val); i++) + ret += sprintf(buf + ret, + (bw_val[i].reg == bw) ? "[%d] " : "%d ", + bw_val[i].value);sysfs nodes should really be single valuesOnce again it's a previous comment from Jonathan [1]. Please specify how you want this and I'll fix it.Yes in general to the single value, but this is a pretty common syntax for choosing one of a set. Got to indicate it somehow and it's either this or a list of available ones in a separate attribute. There was an LWN article on this a while back that lays out the options very clearly http://lwn.net/Articles/378884/
An extra attribute does sound a bit over-kill to me. Would it be acceptable to keep it the way it is?
quoted
quoted
quoted
+static const struct i2c_device_id bma150_id[] = { + { "bma150", 0 }, + { "smb380", 0 },bma023 ?As mentioned earlier bma023 is not an official product name from Bosch Sensortec. Hence, there will be no datasheet available or references from Bosch Sensortec. IMHO it would be confusing to add it.Just ignore the marketing and add it anyway :)
You are probably right. I'll add it.
quoted
quoted
It also doesn't expose the thresholds or support interrupts which the one I posted did.We were told by Bosch S that the interrupt is triggered every 333 us (data ready) which would pretty much flood the irq handler.Somewhat quick, but maybe still worth having... Just use a one shot threaded interrupt and it will at least run safely even if it isn't keeping up with all of them. Alan, do you have a device that has frequency control on chip? (very unusual not to see this on an accelerometer).
Ok, I will add irq handling for v4. It will be threshold triggered, meaning I will also add the threshold parameters from bma023. If no .irq is given it will use polldev. Does that work for you Alan? Regarding the sysfs paramters - how about removing them and use platform data only? Best regards, Eric http://www.unixphere.com