Re: [PATCH v2 2/3] ARM: dts: add cm36651 light/proximity sensor node

[Tomasz Figa <tomasz.figa@gmail.com>]

Hi Beomho,

On 02.05.2014 08:37, Beomho Seo wrote:
> Exynos4412-trats2 board have light/proximity sensor.
> This patch add cm36651 light/ proximity sensor node for exynos4412.
> cm36651 is required properties as below.
> - Use i2c-gpio for cm36651 sensor.
> - Use fixed regulator for the IR LED.
>    It is a part of the cm36651 for proximity detection.
> - cm36651 is i2c device driver so need to use i2c-gpio driver.
>
> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> ---
> Changes in v2:
> - Compatible is changed according to device tree binding document and driver.
> ---
>   arch/arm/boot/dts/exynos4412-trats2.dts |   26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts
> index 1279a8a..d599215 100644
> --- a/arch/arm/boot/dts/exynos4412-trats2.dts
> +++ b/arch/arm/boot/dts/exynos4412-trats2.dts
> @@ -21,6 +21,7 @@
>
>   	aliases {
>   		i2c8 = &i2c_ak8975;
> +		i2c9 = &i2c_cm36651;
>   	};
>
>   	memory {
> @@ -71,6 +72,14 @@
>   			enable-active-high;
>   		};
>
> +		ps_als_reg: voltage-regulator-2 {
> +			compatible = "regulator-fixed";
> +			regulator-name = "LED_A_3.0V";
> +			regulator-min-microvolt = <3000000>;
> +			regulator-max-microvolt = <3000000>;
> +			gpio = <&gpj0 5 0>;
> +			enable-active-high;
> +		};
>   		/* More to come */
>   	};
>
> @@ -500,6 +509,23 @@
>   		};
>   	};
>
> +	i2c_cm36651: i2c-gpio-2 {
> +		compatible = "i2c-gpio";
> +		gpios = <&gpf0 0 0>, <&gpf0 1 0>;
> +		i2c-gpio,delay-us = <2>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Shouldn't you also have pinctrl settings for both pins specified here, 
at least to disable pull-down that is active by default after SoC reset?

> +		status = "okay";
> +
> +		cm36651@18 {
> +			compatible = "capella,cm36651";
> +			reg = <0x18>;
> +			interrupt-parent = <&gpx0>;
> +			interrupts = <2 0>;

Same here. What kind of signal driver does the interrupt pin of CM36651 
chip have? The most common are normal and open-drain drivers, so I 
suspect that the default pull-down on the pin is not correct.

Also I'd say that you should specify some kind of default trigger here, 
instead of simply using 0 as the second cell in interrupt specifier. How 
does the CM36651 chip trigger an interrupt (level or edge, which polarity)?

Best regards,
Tomasz