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

[Mark Rutland <mark.rutland@arm.com>]

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