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

[Jonathan Cameron <jic23@cam.ac.uk>]

On 07/15/11 09:23, Eric Andersson wrote:
>>> +		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
> 
>>> +	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/
> 
>>> +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 :)
>  
>> 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.
> 
>> 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
>