Re: [PATCH v3 1/1] input: add driver for Bosch Sensortec's BMA150 accelerometer

On 07/15/11 09:23, Eric Andersson wrote:

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.

If we could agree on how these values should be represented I will be glad
to fix it! Dmitry, any preferences?

[1] http://www.spinics.net/lists/linux-input/msg14271.html

quoted

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 values

Once 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/

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 :)

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).

However, if this is stopping the patch we can add it. Would it be acceptable to have a driver
that supports both a polling and irq driven setup, e.g. by checking if the .irq is non zero?

Yes: common situation. See the recent kionix driver for an example.
You definitely want the theshold stuff from Alan's driver though.

quoted

So while its better it still seems incomplete - its certainly nowhere
near where it can replace the one I posted months ago and doesn't seem
to
be making any headway.

Dmitry shall I repost the intel one - at this point I think it would
be a
better starting point as it supports more features, interrupts and the
like although it's not perfect either.

My hope is that we can sort out the differences before I post v4. Dmitry, if you have time
could you please give an objective review of the patch?

Best regards,
 Eric

http://www.unixphere.com
--
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