linux-kernel - Re: [PATCH v3] add support for Freescale's MMA8653FC 10 bit accelerometer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 19 Mar 2015 16:03:52 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Martin Kepplinger <martink@...teo.de>
Cc:	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	"dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>,
	"alexander.stein@...tec-electronic.com" 
	<alexander.stein@...tec-electronic.com>,
	"hadess@...ess.net" <hadess@...ess.net>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Martin Kepplinger <martin.kepplinger@...obroma-systems.com>,
	Christoph Muellner <christoph.muellner@...obroma-systems.com>
Subject: Re: [PATCH v3] add support for Freescale's MMA8653FC 10 bit
 accelerometer

> diff --git a/Documentation/devicetree/bindings/misc/fsl,mma8653fc.txt b/Documentation/devicetree/bindings/misc/fsl,mma8653fc.txt
> new file mode 100644
> index 0000000..3921acb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/fsl,mma8653fc.txt
> @@ -0,0 +1,96 @@
> +Freescale MMA8653FC 3-axis Accelerometer
> +
> +Required properties:
> +- compatible
> +       "fsl,mma8653fc"
> +- reg
> +       I2C address
> +
> +Optional properties:
> +
> +- interrupt-parent
> +       a phandle for the interrupt controller (see
> +       Documentation/devicetree/bindings/interrupt-controller/interrupts.txt)
> +- interrupts
> +       interrupt line to which the chip is connected
> +- int1
> +       set to use interrupt line 1 instead of 2

If you have two interrupt output lines, you should have two entries in
interrupts.

You can use interrupt-names to determine which line(s) are wired up.

I don't believe that you need this property.

> +- int_active_high
> +       set interrupt line active high

s/_/-/ in property names please. 

What happens if this isn't set? Is it active-low, or edge-triggered?

It feels like we should be able to query when we need to set this from
the IRQ(s).

> +- ir_freefall_motion_x
> +       activate freefall/motion interrupts on x axis
> +- ir_freefall_motion_y
> +       activate freefall/motion interrupts on y axis
> +- ir_freefall_motion_z
> +       activate freefall/motion interrupts on z axis
> +- irq_threshold
> +       0 < value < 8000: threshold for motion interrupts in mg
> +- ir_landscape_portrait
> +       activate landscape/portrait interrupts
> +- ir_data_ready:
> +       activate data-ready interrupts
> +       Interrupt events can be activated in any combination.

These all sounds like they would be better as runtime options. I don't
see why these should necessarily be in the DT.

> +- range
> +       2, 4, or 8: range in g, default: 2

Likewise.

It would be nice to have a better qualified name than "range".

> +- auto_wake_sleep
> +       auto sleep mode (lower frequency)
> +- motion_mode
> +       use motion mode instead of freefall mode (trigger if >threshold).
> +       per default an interrupt occurs if motion values fall below the
> +       value set in "threshold" and therefore can detect free fall on the
> +       vertical axis (depending on the position of the device).
> +       Setting this values inverts the behaviour and an interrupt occurs
> +       above the threshold value, so usually activate horizontal axis in
> +       this case.

These both sound like they would be better as runtime options.

> +
> +- x-offset
> +       0 < value < 500: calibration offset in mg
> +       this value has an offset of 250 itself:
> +       0 is -250mg, 250 is 0 mg, 500 is 250mg
> +- y-offset
> +       see x-offset
> +- z-offset
> +       see x-offset

I'm unsure about these; it really depends on what the calibration is
for.

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