Date:	Fri, 15 Jul 2011 13:00:30 +0200
From:	"Eric Andersson" <eric.andersson@...xphere.com>
To:	"Jonathan Cameron" <jic23@....ac.uk>
CC:	"Alan Cox" <alan@...rguk.ukuu.org.uk>, dmitry.torokhov@...il.com,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	zhengguang.guo@...ch-sensortec.com, stefan.nilsson@...xphere.com,
	"Albert Zhang" <xu.zhang@...ch-sensortec.com>
Subject: Re: [PATCH v3 1/1] input: add driver for Bosch Sensortec's BMA150
 accelerometer

Thanks for your input Jonathan!

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

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

An extra attribute does sound a bit over-kill to me. Would it be acceptable to
keep it the way it is?

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

> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists