| Message ID | 20190806165749.29468-1-GNUtoo@cyberdimension.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: add touchkey nodes for midas | expand |
Hi, On Tue 06 Aug 19, 18:57, Denis 'GNUtoo' Carikli wrote: > From: Simon Shields <simon@lineageos.org> > > this patch adds the fixed VTOUCH_3.3V regulator and configures > the touchkey node + i2c-gpio node. Thanks for the patch, see some comments below. > Signed-off-by: Simon Shields <simon@lineageos.org> > GNUtoo@cyberdimension.org: Fixed keycodes. > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> > --- > arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi | 4 +++ > arch/arm/boot/dts/exynos4412-midas.dtsi | 29 +++++++++++++++++++++ > arch/arm/boot/dts/exynos4412-n710x.dts | 4 +++ > 3 files changed, 37 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi > index ce87d2ff27aa..e71f103ab940 100644 > --- a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi > +++ b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi > @@ -166,5 +166,9 @@ > &s5c73m3 { > standby-gpios = <&gpm0 1 GPIO_ACTIVE_LOW>; /* ISP_STANDBY */ > vdda-supply = <&ldo17_reg>; > +}; > + > +&touchkey_reg { > + gpio = <&gpm0 0 GPIO_ACTIVE_HIGH>; > status = "okay"; It looks like status = "okay" was initially found on the s5c73m3 node. With this change, it's no longer the case so the camera node will remain disabled. So you probably need to duplicate status = "okay", so that it's both on touchkey_reg and s5c73m3. > }; > diff --git a/arch/arm/boot/dts/exynos4412-midas.dtsi b/arch/arm/boot/dts/exynos4412-midas.dtsi > index 83be3a797411..797e8de40580 100644 > --- a/arch/arm/boot/dts/exynos4412-midas.dtsi > +++ b/arch/arm/boot/dts/exynos4412-midas.dtsi > @@ -13,6 +13,7 @@ > #include "exynos4412.dtsi" > #include "exynos4412-ppmu-common.dtsi" > #include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/input/input.h> > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/clock/maxim,max77686.h> > #include <dt-bindings/pinctrl/samsung.h> > @@ -92,6 +93,15 @@ > enable-active-high; > }; > > + touchkey_reg: voltage-regulator-4 { > + compatible = "regulator-fixed"; > + regulator-name = "VTOUCH_3.3V"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + enable-active-high; > + status = "disabled"; > + }; > + > gpio-keys { > compatible = "gpio-keys"; > pinctrl-names = "default"; > @@ -197,6 +207,25 @@ > }; > }; > > + i2c_touchkey: i2c-gpio-4 { Any reason why this node is not marked as disabled here although the regulator it depends on is? > + compatible = "i2c-gpio"; > + sda-gpios = <&gpl0 2 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; > + scl-gpios = <&gpl0 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; > + i2c-gpio,delay-us = <2>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + touchkey@20 { > + compatible = "cypress,midas-touchkey"; > + reg = <0x20>; > + vdd-supply = <&touchkey_reg>; > + vcc-supply = <&ldo5_reg>; > + interrupt-parent = <&gpj0>; > + interrupts = <3 IRQ_TYPE_EDGE_FALLING>; > + linux,keycodes = <KEY_BACK KEY_MENU>; > + }; > + }; > + > i2c-mhl { > compatible = "i2c-gpio"; > gpios = <&gpf0 4 GPIO_ACTIVE_HIGH>, <&gpf0 6 GPIO_ACTIVE_HIGH>; > diff --git a/arch/arm/boot/dts/exynos4412-n710x.dts b/arch/arm/boot/dts/exynos4412-n710x.dts > index fe2bfd76cc4e..6acb19d2bae2 100644 > --- a/arch/arm/boot/dts/exynos4412-n710x.dts > +++ b/arch/arm/boot/dts/exynos4412-n710x.dts > @@ -71,5 +71,9 @@ > &s5c73m3 { > standby-gpios = <&gpm0 6 GPIO_ACTIVE_LOW>; /* ISP_STANDBY */ > vdda-supply = <&cam_vdda_reg>; > +}; > + > +&touchkey_reg { > + gpio = <&gpm0 5 GPIO_ACTIVE_HIGH>; > status = "okay"; And ditto about duplicating status. Cheers, Paul > }; > -- > 2.22.0 >
On Tue, 6 Aug 2019 at 19:04, Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> wrote: > > From: Simon Shields <simon@lineageos.org> > > this patch adds the fixed VTOUCH_3.3V regulator and configures > the touchkey node + i2c-gpio node. Use the simple imperative form and please describe the user-visible impact of this changes (or why you are doing it). Probably you want to enable keys on touchscreen but it's not explicitly stated... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v5.3-rc3&id=e21a712a9685488f5ce80495b37b9fdbe96c230d#n102 > > Signed-off-by: Simon Shields <simon@lineageos.org> > GNUtoo@cyberdimension.org: Fixed keycodes. > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> > --- > arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi | 4 +++ > arch/arm/boot/dts/exynos4412-midas.dtsi | 29 +++++++++++++++++++++ > arch/arm/boot/dts/exynos4412-n710x.dts | 4 +++ > 3 files changed, 37 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi > index ce87d2ff27aa..e71f103ab940 100644 > --- a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi > +++ b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi > @@ -166,5 +166,9 @@ > &s5c73m3 { > standby-gpios = <&gpm0 1 GPIO_ACTIVE_LOW>; /* ISP_STANDBY */ > vdda-supply = <&ldo17_reg>; > +}; > + > +&touchkey_reg { > + gpio = <&gpm0 0 GPIO_ACTIVE_HIGH>; You break existing code... > status = "okay"; > }; > diff --git a/arch/arm/boot/dts/exynos4412-midas.dtsi b/arch/arm/boot/dts/exynos4412-midas.dtsi > index 83be3a797411..797e8de40580 100644 > --- a/arch/arm/boot/dts/exynos4412-midas.dtsi > +++ b/arch/arm/boot/dts/exynos4412-midas.dtsi > @@ -13,6 +13,7 @@ > #include "exynos4412.dtsi" > #include "exynos4412-ppmu-common.dtsi" > #include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/input/input.h> > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/clock/maxim,max77686.h> > #include <dt-bindings/pinctrl/samsung.h> > @@ -92,6 +93,15 @@ > enable-active-high; > }; > > + touchkey_reg: voltage-regulator-4 { There is already voltage-regulator-4 node. > + compatible = "regulator-fixed"; > + regulator-name = "VTOUCH_3.3V"; Let's keep the name as in schematics - "LED_VDD_3.3V"... which brings us to the question is it really needed for touch keys? or for display panel? > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + enable-active-high; > + status = "disabled"; > + }; > + > gpio-keys { > compatible = "gpio-keys"; > pinctrl-names = "default"; > @@ -197,6 +207,25 @@ > }; > }; > > + i2c_touchkey: i2c-gpio-4 { No need for label. Best regards, Krzysztof
On Wed, 7 Aug 2019 10:13:04 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote: Sorry for the delay. I had to find and buy N710x to get faster turnarounds for testing as before I had to borrow an N7100 from a friend to do the testing there as well. I've addressed most of the comments and I'll send a v2 soon. > > diff --git a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi > > b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi index > > ce87d2ff27aa..e71f103ab940 100644 --- > > a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi +++ > > b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi @@ -166,5 +166,9 @@ > > &s5c73m3 { > > standby-gpios = <&gpm0 1 GPIO_ACTIVE_LOW>; /* ISP_STANDBY > > */ vdda-supply = <&ldo17_reg>; > > +}; > > + > > +&touchkey_reg { > > + gpio = <&gpm0 0 GPIO_ACTIVE_HIGH>; > > You break existing code... I didn't understand this comment. > Let's keep the name as in schematics - "LED_VDD_3.3V"... which brings > us to the question is it really needed for touch keys? or for display > panel? On I9300, I9305 and n7105 without the touchkey_reg node, the keys still work but the keys backlight doesn't. Denis.
On Mon, 11 Nov 2019 at 19:59, Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> wrote: > > > diff --git a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi > > > b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi index > > > ce87d2ff27aa..e71f103ab940 100644 --- > > > a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi +++ > > > b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi @@ -166,5 +166,9 @@ > > > &s5c73m3 { > > > standby-gpios = <&gpm0 1 GPIO_ACTIVE_LOW>; /* ISP_STANDBY > > > */ vdda-supply = <&ldo17_reg>; > > > +}; > > > + > > > +&touchkey_reg { > > > + gpio = <&gpm0 0 GPIO_ACTIVE_HIGH>; > > > > You break existing code... > I didn't understand this comment. With this change you break the s5c73m3 by making it disabled. > > > Let's keep the name as in schematics - "LED_VDD_3.3V"... which brings > > us to the question is it really needed for touch keys? or for display > > panel? > On I9300, I9305 and n7105 without the touchkey_reg node, the keys still > work but the keys backlight doesn't. So it's for backlight - let it be the name from schematics "LED_VDD_3.3V". Best regards, Krzysztof
On Tue, 12 Nov 2019 02:25:41 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:
> With this change you break the s5c73m3 by making it disabled.
Thanks, I see it now, I didn't look at enough patch context.
I'll send a v2.
Denis.
diff --git a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi index ce87d2ff27aa..e71f103ab940 100644 --- a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi +++ b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi @@ -166,5 +166,9 @@ &s5c73m3 { standby-gpios = <&gpm0 1 GPIO_ACTIVE_LOW>; /* ISP_STANDBY */ vdda-supply = <&ldo17_reg>; +}; + +&touchkey_reg { + gpio = <&gpm0 0 GPIO_ACTIVE_HIGH>; status = "okay"; }; diff --git a/arch/arm/boot/dts/exynos4412-midas.dtsi b/arch/arm/boot/dts/exynos4412-midas.dtsi index 83be3a797411..797e8de40580 100644 --- a/arch/arm/boot/dts/exynos4412-midas.dtsi +++ b/arch/arm/boot/dts/exynos4412-midas.dtsi @@ -13,6 +13,7 @@ #include "exynos4412.dtsi" #include "exynos4412-ppmu-common.dtsi" #include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/input/input.h> #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/clock/maxim,max77686.h> #include <dt-bindings/pinctrl/samsung.h> @@ -92,6 +93,15 @@ enable-active-high; }; + touchkey_reg: voltage-regulator-4 { + compatible = "regulator-fixed"; + regulator-name = "VTOUCH_3.3V"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + enable-active-high; + status = "disabled"; + }; + gpio-keys { compatible = "gpio-keys"; pinctrl-names = "default"; @@ -197,6 +207,25 @@ }; }; + i2c_touchkey: i2c-gpio-4 { + compatible = "i2c-gpio"; + sda-gpios = <&gpl0 2 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; + scl-gpios = <&gpl0 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; + i2c-gpio,delay-us = <2>; + #address-cells = <1>; + #size-cells = <0>; + + touchkey@20 { + compatible = "cypress,midas-touchkey"; + reg = <0x20>; + vdd-supply = <&touchkey_reg>; + vcc-supply = <&ldo5_reg>; + interrupt-parent = <&gpj0>; + interrupts = <3 IRQ_TYPE_EDGE_FALLING>; + linux,keycodes = <KEY_BACK KEY_MENU>; + }; + }; + i2c-mhl { compatible = "i2c-gpio"; gpios = <&gpf0 4 GPIO_ACTIVE_HIGH>, <&gpf0 6 GPIO_ACTIVE_HIGH>; diff --git a/arch/arm/boot/dts/exynos4412-n710x.dts b/arch/arm/boot/dts/exynos4412-n710x.dts index fe2bfd76cc4e..6acb19d2bae2 100644 --- a/arch/arm/boot/dts/exynos4412-n710x.dts +++ b/arch/arm/boot/dts/exynos4412-n710x.dts @@ -71,5 +71,9 @@ &s5c73m3 { standby-gpios = <&gpm0 6 GPIO_ACTIVE_LOW>; /* ISP_STANDBY */ vdda-supply = <&cam_vdda_reg>; +}; + +&touchkey_reg { + gpio = <&gpm0 5 GPIO_ACTIVE_HIGH>; status = "okay"; };