Re: [PATCH V2 1/1] iio: light: Added CM36672 Proximity Sensor Driver.

[Linus Walleij]

On Thu, Jun 9, 2016 at 11:00 AM, Kevin Tsai <capellamicro@xxxxxxxxx> wrote:

> V2:
> Thanks commends from Peter Meerwald-Stadler, Jonathan Cameron, and Linux
> Walleij.

Haha that again, everybody does that.

> Updated for the following:
>  - Remove unused defines.
>  - Rewrite event registration.
>  - Remove direct register values in the device tree.
>  - Rewrite by regmap API.
>  - Remove irq module parameter.
>  - Correct cm36672_remove().

Awesome. Minor comments still:

> +Required properties:
> +- compatible: must be capella,cm36672"
> +- reg: the I2C slave address, usually 0x60
> +- interrupts: interrupt mapping for GPIO IRQ

Don't say GPIO IRQ. Some chips have hardwired external interrupt lines
rather than GPIOs and those should work just as fine. Does it support
level, edge etc IRQs?

> +Optional properties:
> +- interrupt-parent: interrupt controller
> +- interrupts: interrupt mapping for GPIO IRQ

That is mentioned again, I guess it is not optional and should be removed
from here?

> +- cm36672,prx_led_current: LED current, must be one of the following values:
> +    - 0: 50mA
> +    - 1: 75mA
> +    - 2: 100mA
> +    - 3: 120mA
> +    - 4: 140mA
> +    - 5: 160mA
> +    - 6: 180mA
> +    - 7: 200mA

Do not specify enumerators. Specify milliamps in the device tree
and have the code translate it to enumerators. DTS should be
human-readable and easy to understand without reading tables.

Just use vendor prefix and something you seen before like:
capella,driver-strength = <160>;
For 160mA

> +- cm36672,prx_hd: width of proximity sensor output data, must be one of the
> +  following values:
> +    - 0: 12-bit
> +    - 1: 16-bit

Use
capella,bus-width = <12>;
The SD cards have this pattern for bus widths.

Yours,
Linus Walleij