| Message ID | 20181210094358.3241-4-markz@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | NVIDIA Shield TV device tree patch set | expand |
On Mon, Dec 10, 2018 at 05:43:56PM +0800, Mark Zhang wrote: > Add gpio-keys nodes for the power button. > > Signed-off-by: Mark Zhang <markz@nvidia.com> > --- > arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi > index ac52286ab9ab..1053f18cc4ae 100644 > --- a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi > +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi > @@ -1,5 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0 > > +#include <dt-bindings/input/input.h> > #include "tegra210.dtsi" > > / { > @@ -52,6 +53,20 @@ > }; > }; > > + gpio-keys { > + compatible = "gpio-keys"; > + gpio-keys,name = "gpio-keys"; This property is not only undocumented, it's also unsupported. Let's just drop it. Bonus points for sending in a patch that removes the only other occurrence of this from: arch/arm64/boot/dts/nvidia/tegra210-smaug.dts Also, the Shield TV has an LED that's on when the device is powered on, does that happen automatically in hardware, or would that be done as a GPIO (or LED) as well? Thierry
On 12/10/2018 6:29 PM, Thierry Reding wrote: > On Mon, Dec 10, 2018 at 05:43:56PM +0800, Mark Zhang wrote: >> Add gpio-keys nodes for the power button. >> >> Signed-off-by: Mark Zhang <markz@nvidia.com> >> --- >> arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi >> index ac52286ab9ab..1053f18cc4ae 100644 >> --- a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi >> +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi >> @@ -1,5 +1,6 @@ >> // SPDX-License-Identifier: GPL-2.0 >> >> +#include <dt-bindings/input/input.h> >> #include "tegra210.dtsi" >> >> / { >> @@ -52,6 +53,20 @@ >> }; >> }; >> >> + gpio-keys { >> + compatible = "gpio-keys"; >> + gpio-keys,name = "gpio-keys"; > > This property is not only undocumented, it's also unsupported. Let's > just drop it. Bonus points for sending in a patch that removes the only > other occurrence of this from: I saw this property is documented here: Documentation/devicetree/bindings/input/gpio-keys.txt Also in drivers/input/keyboard/gpio_keys.c, there is driver which is compatible with this property though I didn't read it in detail. Anyway could you explain more why this property is not supported? > > arch/arm64/boot/dts/nvidia/tegra210-smaug.dts > > Also, the Shield TV has an LED that's on when the device is powered on, > does that happen automatically in hardware, or would that be done as a > GPIO (or LED) as well? Yeah that "green" LED is actually a PWM LED and the brightness is adjustable. So what this patch tries to introduce is the power button of the Shield which is a GPIO key. Mark > > Thierry >
On Tue, Dec 11, 2018 at 10:18:24AM +0800, Mark Zhang wrote: > On 12/10/2018 6:29 PM, Thierry Reding wrote: > > On Mon, Dec 10, 2018 at 05:43:56PM +0800, Mark Zhang wrote: > >> Add gpio-keys nodes for the power button. > >> > >> Signed-off-by: Mark Zhang <markz@nvidia.com> > >> --- > >> arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi > >> index ac52286ab9ab..1053f18cc4ae 100644 > >> --- a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi > >> +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi > >> @@ -1,5 +1,6 @@ > >> // SPDX-License-Identifier: GPL-2.0 > >> > >> +#include <dt-bindings/input/input.h> > >> #include "tegra210.dtsi" > >> > >> / { > >> @@ -52,6 +53,20 @@ > >> }; > >> }; > >> > >> + gpio-keys { > >> + compatible = "gpio-keys"; > >> + gpio-keys,name = "gpio-keys"; > > > > This property is not only undocumented, it's also unsupported. Let's > > just drop it. Bonus points for sending in a patch that removes the only > > other occurrence of this from: > > I saw this property is documented here: > Documentation/devicetree/bindings/input/gpio-keys.txt You must be looking at a different version than I. Where is the version of that file that you're looking at? > Also in drivers/input/keyboard/gpio_keys.c, there is driver which is > compatible with this property though I didn't read it in detail. There's no code in that driver to parse this property. There is a property which seems to do what you want, but it is called "label". It's also not necessary since the gpio-keys driver will use the device name if no label was defined and the device name in this case should match what you're passing in as name ("gpio-keys"), so you're not gaining anything by adding that property. > Anyway could you explain more why this property is not supported? > > > > > arch/arm64/boot/dts/nvidia/tegra210-smaug.dts > > > > Also, the Shield TV has an LED that's on when the device is powered on, > > does that happen automatically in hardware, or would that be done as a > > GPIO (or LED) as well? > > Yeah that "green" LED is actually a PWM LED and the brightness is > adjustable. So what this patch tries to introduce is the power button of > the Shield which is a GPIO key. Okay, good. Sounds like it should be easy to add the LED later on. Thierry
On 12/11/2018 6:26 PM, Thierry Reding wrote: > On Tue, Dec 11, 2018 at 10:18:24AM +0800, Mark Zhang wrote: >> On 12/10/2018 6:29 PM, Thierry Reding wrote: >>> On Mon, Dec 10, 2018 at 05:43:56PM +0800, Mark Zhang wrote: >>>> Add gpio-keys nodes for the power button. >>>> >>>> Signed-off-by: Mark Zhang <markz@nvidia.com> >>>> --- >>>> arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi >>>> index ac52286ab9ab..1053f18cc4ae 100644 >>>> --- a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi >>>> +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi >>>> @@ -1,5 +1,6 @@ >>>> // SPDX-License-Identifier: GPL-2.0 >>>> >>>> +#include <dt-bindings/input/input.h> >>>> #include "tegra210.dtsi" >>>> >>>> / { >>>> @@ -52,6 +53,20 @@ >>>> }; >>>> }; >>>> >>>> + gpio-keys { >>>> + compatible = "gpio-keys"; >>>> + gpio-keys,name = "gpio-keys"; >>> >>> This property is not only undocumented, it's also unsupported. Let's >>> just drop it. Bonus points for sending in a patch that removes the only >>> other occurrence of this from: >> >> I saw this property is documented here: >> Documentation/devicetree/bindings/input/gpio-keys.txt > > You must be looking at a different version than I. Where is the version > of that file that you're looking at? > >> Also in drivers/input/keyboard/gpio_keys.c, there is driver which is >> compatible with this property though I didn't read it in detail. > > There's no code in that driver to parse this property. There is a > property which seems to do what you want, but it is called "label". OH OH I see, you're talking about "gpio-keys,name" this property right? I was thinking that you're suggesting to remove the whole "gpio-keys" section. Yeah gpio-keys,name is not supported, and I will send a patch to remove it in tegra210-smaug.dts as well. Mark > > It's also not necessary since the gpio-keys driver will use the device > name if no label was defined and the device name in this case should > match what you're passing in as name ("gpio-keys"), so you're not > gaining anything by adding that property. > >> Anyway could you explain more why this property is not supported? >> >>> >>> arch/arm64/boot/dts/nvidia/tegra210-smaug.dts >>> >>> Also, the Shield TV has an LED that's on when the device is powered on, >>> does that happen automatically in hardware, or would that be done as a >>> GPIO (or LED) as well? >> >> Yeah that "green" LED is actually a PWM LED and the brightness is >> adjustable. So what this patch tries to introduce is the power button of >> the Shield which is a GPIO key. > > Okay, good. Sounds like it should be easy to add the LED later on. > > Thierry >
diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi index ac52286ab9ab..1053f18cc4ae 100644 --- a/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 +#include <dt-bindings/input/input.h> #include "tegra210.dtsi" / { @@ -52,6 +53,20 @@ }; }; + gpio-keys { + compatible = "gpio-keys"; + gpio-keys,name = "gpio-keys"; + status = "okay"; + + power { + label = "Power"; + gpios = <&gpio TEGRA_GPIO(X, 7) GPIO_ACTIVE_LOW>; + linux,code = <KEY_POWER>; + wakeup-source; + debounce-interval = <30>; + }; + }; + cpus { cpu@0 { enable-method = "psci";
Add gpio-keys nodes for the power button. Signed-off-by: Mark Zhang <markz@nvidia.com> --- arch/arm64/boot/dts/nvidia/tegra210-p2894.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+)