[PATCH 1/3] ARM: dts: imx6qdl-sabresd: add light sensor support

[PATCH 1/3] ARM: dts: imx6qdl-sabresd: add light sensor support

[Anson Huang]

Add isl29023 light sensor support on i2c3 bus, the light
sensor's power is controlled by a fixed regulator, since
the isl29023 driver and most of other sensors on same
board like mag3110 and mma8451 do NOT support regulator
operation currently, they are all controlled by this
regulator, so this patch also adds the fixed regulator
support and make it always on.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index d7389b5..f96ae54 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -62,6 +62,19 @@
 			gpio = <&gpio3 19 0>;
 			enable-active-high;
 		};
+
+		reg_sensor: regulator@4 {
+			compatible = "regulator-fixed";
+			reg = <4>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_sensor_reg>;
+			regulator-name = "sensor-supply";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio2 31 GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+			regulator-always-on;
+		};
 	};
 
 	gpio-keys {
@@ -420,6 +433,15 @@
 		interrupts = <7 2>;
 		wakeup-gpios = <&gpio6 7 0>;
 	};
+
+	isl29023@44 {
+		compatible = "isil,isl29023";
+		reg = <0x44>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i2c3_isl29023_int>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <9 IRQ_TYPE_EDGE_FALLING>;
+	};
 };
 
 &iomuxc {
@@ -521,6 +543,12 @@
 			>;
 		};
 
+		pinctrl_i2c3_isl29023_int: i2c3isl29023intgrp {
+			fsl,pins = <
+				MX6QDL_PAD_EIM_DA9__GPIO3_IO09		0xb0b1
+			>;
+		};
+
 		pinctrl_ipu1_csi0: ipu1csi0grp {
 			fsl,pins = <
 				MX6QDL_PAD_CSI0_DAT12__IPU1_CSI0_DATA12    0x1b0b0
@@ -569,6 +597,12 @@
 			>;
 		};
 
+		pinctrl_sensor_reg: sensorreggrp {
+			fsl,pins = <
+				MX6QDL_PAD_EIM_EB3__GPIO2_IO31		0x1b0b0
+			>;
+		};
+
 		pinctrl_uart1: uart1grp {
 			fsl,pins = <
 				MX6QDL_PAD_CSI0_DAT10__UART1_TX_DATA	0x1b0b1
-- 
2.7.4

[PATCH 2/3] ARM: dts: imx6qdl-sabresd: add magnetometer sensor support

[Anson Huang]

Add magnetometer sensor mag3110 support on i2c3 bus.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index f96ae54..141550a 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -434,6 +434,15 @@
 		wakeup-gpios = <&gpio6 7 0>;
 	};
 
+	magnetometer@e {
+		compatible = "fsl,mag3110";
+		reg = <0x0e>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i2c3_mag3110_int>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <16 IRQ_TYPE_EDGE_RISING>;
+	};
+
 	isl29023@44 {
 		compatible = "isil,isl29023";
 		reg = <0x44>;
@@ -549,6 +558,12 @@
 			>;
 		};
 
+		pinctrl_i2c3_mag3110_int: i2c3mag3110intgrp {
+			fsl,pins = <
+				MX6QDL_PAD_EIM_D16__GPIO3_IO16		0xb0b1
+			>;
+		};
+
 		pinctrl_ipu1_csi0: ipu1csi0grp {
 			fsl,pins = <
 				MX6QDL_PAD_CSI0_DAT12__IPU1_CSI0_DATA12    0x1b0b0
-- 
2.7.4

Re: [PATCH 2/3] ARM: dts: imx6qdl-sabresd: add magnetometer sensor support

[Fabio Estevam]

On Wed, Dec 5, 2018 at 7:20 AM Anson Huang <anson.huang@nxp.com> wrote:
>
> Add magnetometer sensor mag3110 support on i2c3 bus.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

[PATCH 3/3] ARM: dts: imx6qdl-sabresd: add accelerometer sensor support

[Anson Huang]

Add accelerometer sensor mma8451 support on i2c1 bus.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 141550a..fd9127f 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -253,6 +253,15 @@
 		>;
 	};
 
+	accelerometer@1c {
+		compatible = "fsl,mma8451";
+		reg = <0x1c>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i2c1_mma8451_int>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <18 IRQ_TYPE_LEVEL_LOW>;
+	};
+
 	ov5642: camera@3c {
 		compatible = "ovti,ov5642";
 		pinctrl-names = "default";
@@ -532,6 +541,12 @@
 			>;
 		};
 
+		pinctrl_i2c1_mma8451_int: i2c1mma8451intgrp {
+			fsl,pins = <
+				MX6QDL_PAD_SD1_CMD__GPIO1_IO18		0xb0b1
+			>;
+		};
+
 		pinctrl_i2c2: i2c2grp {
 			fsl,pins = <
 				MX6QDL_PAD_KEY_COL3__I2C2_SCL		0x4001b8b1
-- 
2.7.4

Re: [PATCH 3/3] ARM: dts: imx6qdl-sabresd: add accelerometer sensor support

[Fabio Estevam]

On Wed, Dec 5, 2018 at 7:20 AM Anson Huang <anson.huang@nxp.com> wrote:
>
> Add accelerometer sensor mma8451 support on i2c1 bus.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH 1/3] ARM: dts: imx6qdl-sabresd: add light sensor support

[Fabio Estevam]

Hi Anson,

On Wed, Dec 5, 2018 at 7:20 AM Anson Huang <anson.huang@nxp.com> wrote:
>
> Add isl29023 light sensor support on i2c3 bus, the light
> sensor's power is controlled by a fixed regulator, since
> the isl29023 driver and most of other sensors on same
> board like mag3110 and mma8451 do NOT support regulator
> operation currently, they are all controlled by this
> regulator, so this patch also adds the fixed regulator
> support and make it always on.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index d7389b5..f96ae54 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -62,6 +62,19 @@
>                         gpio = <&gpio3 19 0>;
>                         enable-active-high;
>                 };
> +
> +               reg_sensor: regulator@4 {

I know that you followed the existing pattern for regulators in this
file, but it is not recommended to put regulators under "simple-bus".

I would suggest you to make a first patch of the series that removes
all of the existing regulators from "simple-bus", then you add this
series on top.

Then this regulator becomes:

reg_light_sensor: regulator-light-sensor {

> +                       compatible = "regulator-fixed";
> +                       reg = <4>;
> +                       pinctrl-names = "default";
> +                       pinctrl-0 = <&pinctrl_sensor_reg>;

pinctrl_sensor_reg is too generic. You could use pinctrl_light_sensor_reg

> +                       regulator-name = "sensor-supply";
> +                       regulator-min-microvolt = <3300000>;
> +                       regulator-max-microvolt = <3300000>;
> +                       gpio = <&gpio2 31 GPIO_ACTIVE_HIGH>;
> +                       enable-active-high;
> +                       regulator-always-on;
> +               };
>         };
>
>         gpio-keys {
> @@ -420,6 +433,15 @@
>                 interrupts = <7 2>;
>                 wakeup-gpios = <&gpio6 7 0>;
>         };
> +
> +       isl29023@44 {

Node names should be generic, so: light-sensor@44

Re: [PATCH 1/3] ARM: dts: imx6qdl-sabresd: add light sensor support

[Fabio Estevam]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

Hi Anson,

On Wed, Dec 5, 2018 at 10:15 AM Fabio Estevam <festevam@gmail.com> wrote:

> I know that you followed the existing pattern for regulators in this
> file, but it is not recommended to put regulators under "simple-bus".
>
> I would suggest you to make a first patch of the series that removes
> all of the existing regulators from "simple-bus", then you add this
> series on top.

To make things easier, please find attached a patch that implements
this suggestion.

Please make it the first patch of your series.

Thanks

[-- Attachment #2: 0001-ARM-dts-imx6qdl-sabresd-Move-regulators-outside-of-s.patch --]
[-- Type: text/x-patch, Size: 3040 bytes --]

From a05529c77ab107dab6ccf97e9767ced8fd0b05d3 Mon Sep 17 00:00:00 2001
From: Fabio Estevam <festevam@gmail.com>
Date: Wed, 5 Dec 2018 12:36:35 -0200
Subject: [PATCH] ARM: dts: imx6qdl-sabresd: Move regulators outside of
 "simple-bus"

It is not recommended to place regulators inside "simple-bus", so move
them out to make it cleaner the addition of new regulators.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 82 +++++++++++++++-------------------
 1 file changed, 36 insertions(+), 46 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 6e46a19..d758585 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -16,52 +16,42 @@
 		reg = <0x10000000 0x40000000>;
 	};
 
-	regulators {
-		compatible = "simple-bus";
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		reg_usb_otg_vbus: regulator@0 {
-			compatible = "regulator-fixed";
-			reg = <0>;
-			regulator-name = "usb_otg_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			gpio = <&gpio3 22 0>;
-			enable-active-high;
-			vin-supply = <&swbst_reg>;
-		};
-
-		reg_usb_h1_vbus: regulator@1 {
-			compatible = "regulator-fixed";
-			reg = <1>;
-			regulator-name = "usb_h1_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			gpio = <&gpio1 29 0>;
-			enable-active-high;
-			vin-supply = <&swbst_reg>;
-		};
-
-		reg_audio: regulator@2 {
-			compatible = "regulator-fixed";
-			reg = <2>;
-			regulator-name = "wm8962-supply";
-			gpio = <&gpio4 10 0>;
-			enable-active-high;
-		};
-
-		reg_pcie: regulator@3 {
-			compatible = "regulator-fixed";
-			reg = <3>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_pcie_reg>;
-			regulator-name = "MPCIE_3V3";
-			regulator-min-microvolt = <3300000>;
-			regulator-max-microvolt = <3300000>;
-			gpio = <&gpio3 19 0>;
-			enable-active-high;
-		};
+	reg_usb_otg_vbus: regulator-usb-otg-vbus {
+		compatible = "regulator-fixed";
+		regulator-name = "usb_otg_vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&gpio3 22 0>;
+		enable-active-high;
+		vin-supply = <&swbst_reg>;
+	};
+
+	reg_usb_h1_vbus: regulator-usb-h1-vbus {
+		compatible = "regulator-fixed";
+		regulator-name = "usb_h1_vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&gpio1 29 0>;
+		enable-active-high;
+		vin-supply = <&swbst_reg>;
+	};
+
+	reg_audio: regulator-audio {
+		compatible = "regulator-fixed";
+		regulator-name = "wm8962-supply";
+		gpio = <&gpio4 10 0>;
+		enable-active-high;
+	};
+
+	reg_pcie: regulator-pcie {
+		compatible = "regulator-fixed";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_pcie_reg>;
+		regulator-name = "MPCIE_3V3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&gpio3 19 0>;
+		enable-active-high;
 	};
 
 	gpio-keys {
-- 
2.7.4

RE: [PATCH 1/3] ARM: dts: imx6qdl-sabresd: add light sensor support

[Anson Huang]

Hi, Fabio

Best Regards!
Anson Huang

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam@gmail.com]
> Sent: 2018年12月5日 20:15
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> <s.hauer@pengutronix.de>; Sascha Hauer <kernel@pengutronix.de>; Fabio
> Estevam <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>;
> Mark Rutland <mark.rutland@arm.com>; moderated list:ARM/FREESCALE IMX
> / MXC ARM ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> <devicetree@vger.kernel.org>; linux-kernel <linux-kernel@vger.kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 1/3] ARM: dts: imx6qdl-sabresd: add light sensor support
> 
> Hi Anson,
> 
> On Wed, Dec 5, 2018 at 7:20 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> >
> > Add isl29023 light sensor support on i2c3 bus, the light sensor's
> > power is controlled by a fixed regulator, since the isl29023 driver
> > and most of other sensors on same board like mag3110 and mma8451 do
> > NOT support regulator operation currently, they are all controlled by
> > this regulator, so this patch also adds the fixed regulator support
> > and make it always on.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 34
> > ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > index d7389b5..f96ae54 100644
> > --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > @@ -62,6 +62,19 @@
> >                         gpio = <&gpio3 19 0>;
> >                         enable-active-high;
> >                 };
> > +
> > +               reg_sensor: regulator@4 {
> 
> I know that you followed the existing pattern for regulators in this file, but it is
> not recommended to put regulators under "simple-bus".
> 
> I would suggest you to make a first patch of the series that removes all of the
> existing regulators from "simple-bus", then you add this series on top.
> 
> Then this regulator becomes:
> 
> reg_light_sensor: regulator-light-sensor {
> 
> > +                       compatible = "regulator-fixed";
> > +                       reg = <4>;
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&pinctrl_sensor_reg>;
> 
> pinctrl_sensor_reg is too generic. You could use pinctrl_light_sensor_reg

As this regulator controls all the sensors power on board, including light sensor,
magnetometer sensor and accelerometer sensors, so I think it is better to
use "pinctrl_sensors_reg" instead of "pinctrl_light_sensor_reg", so I use
" pinctrl_sensors_reg" in V2 patch.

> 
> > +                       regulator-name = "sensor-supply";
> > +                       regulator-min-microvolt = <3300000>;
> > +                       regulator-max-microvolt = <3300000>;
> > +                       gpio = <&gpio2 31 GPIO_ACTIVE_HIGH>;
> > +                       enable-active-high;
> > +                       regulator-always-on;
> > +               };
> >         };
> >
> >         gpio-keys {
> > @@ -420,6 +433,15 @@
> >                 interrupts = <7 2>;
> >                 wakeup-gpios = <&gpio6 7 0>;
> >         };
> > +
> > +       isl29023@44 {
> 
> Node names should be generic, so: light-sensor@44

Improved it in V2 patch, and also apply the patch you sent me as
the first patch, please help review them, thanks.

Anson.