public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Steven King <sfking@fdwdc.com>
To: Jonathan Cameron <jic23@cam.ac.uk>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH][input] add Honeywell hmc5843 3-Axis Magnetometer (digital compass)  driver.
Date: Fri, 11 Dec 2009 14:26:53 -0800	[thread overview]
Message-ID: <200912111426.54186.sfking@fdwdc.com> (raw)
In-Reply-To: <4B223EE4.5010203@cam.ac.uk>

Hi Jonathan,

Thanks for taking at look at it.

On Friday 11 December 2009 04:45:24 Jonathan Cameron wrote:
> Dear Steven,
>
> Mostly looks good to me.  Couple of minor comments inline below.
>
> I'd also like to see some documentation for this chip.  We don't really
> want people to have to read the data sheet in order to find out what the
> various modes and frequency settings are for example. Datasheets have
> a nasty habit of disappearing in the long run.  Probably needs something
> in Documentation directory rather than merely comments in the code.

Yeah, thats on my todo list, but I thought I should post the bare driver to 
get some feedback before I got too far along...

> Only one I'm really fussy about is making sure you use the unsigned
> strict_strtoul where appropriate.  Fix that and you can add
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

Doh! I had originally used strict_strtoul, then for some reason I can't 
remember (probably caffeine deprivation) I changed them.  Thanks for catching 
that.

> I'm not entirely convinced this one should be in input as I can't really
> believe it is commonly used as an input device?  Over to Dmitry and others
> for that though.   We can always move it at a later date. The requirements
> of a chip this simple (interface wise) would make that trivial.

I wasn't sure either; currently, the only use I'm familiar with is to combine 
the magnetometer with an accelerometer to create a tilt compensated digital 
compass.  In practice, the raw data needs some massaging to be usefull, so 
the plan is to have a daemon that collects the inputs from the accelerometer 
and the magnetometer and does the processing and apps would talk to it, so 
I'm not fixated on any particular interface.

Anyway, I'll cobble together something for Documentation/input/ (for now) and 
fix those strict_strtol (and the ints) and respin the patch.

Thanks,
-- 
Steven King -- sfking at fdwdc dot com


  reply	other threads:[~2009-12-11 22:29 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-12-11  6:50 [PATCH][input] add Honeywell hmc5843 3-Axis Magnetometer (digital compass) driver Steven King
2009-12-11 12:45 ` Jonathan Cameron
2009-12-11 22:26   ` Steven King [this message]
2009-12-15 14:23 ` Dan Carpenter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200912111426.54186.sfking@fdwdc.com \
    --to=sfking@fdwdc.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=jic23@cam.ac.uk \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox