[2/6] dt-bindings: Input: edt-ft5x06 - add disable wakeup-source documentation
diff mbox series

Message ID 20190917155808.27818-3-m.felsch@pengutronix.de
State Changes Requested
Headers show
Series
  • EDT-FT5x06 improvements
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Marco Felsch Sept. 17, 2019, 3:58 p.m. UTC
The default driver behaviour is to enable the wakeup-source everytime.
There are hardware designs which have a dedicated gpio to act as wakeup
device. So it must be allowed to disable the wakeup-source capability.

This patch adds the binding documentation to disable the wakeup-source
capability.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dmitry Torokhov Sept. 17, 2019, 5:07 p.m. UTC | #1
On Tue, Sep 17, 2019 at 05:58:04PM +0200, Marco Felsch wrote:
> The default driver behaviour is to enable the wakeup-source everytime.
> There are hardware designs which have a dedicated gpio to act as wakeup
> device. So it must be allowed to disable the wakeup-source capability.
> 
> This patch adds the binding documentation to disable the wakeup-source
> capability.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> index 870b8c5cce9b..4d6524fe3cf4 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -35,6 +35,10 @@ Optional properties:
>   - pinctrl-0:   a phandle pointing to the pin settings for the
>                  control gpios
>  
> + - edt,disable-wakeup-source: If left the device will act as wakeup-source
> +			      (for legacy compatibility). Add the property
> +			      so the device won't act as wakeup-source.

I think this is too ugly and I consider it being a bug in the driver
that it enables wakeup unconditionally.

Let's just update DTS in tree for devices that actually want it (I am
curious how many that do not declare "wakeup-source" have it working and
actually want it) and key the dirver behavior off the standard property.
Until we start seeing this controller in devices that actually have DTS
in hardware device tree I think it is better to use standard property.

Thanks.
Marco Felsch Sept. 17, 2019, 5:18 p.m. UTC | #2
Hi Dmitry,

On 19-09-17 10:07, Dmitry Torokhov wrote:
> On Tue, Sep 17, 2019 at 05:58:04PM +0200, Marco Felsch wrote:
> > The default driver behaviour is to enable the wakeup-source everytime.
> > There are hardware designs which have a dedicated gpio to act as wakeup
> > device. So it must be allowed to disable the wakeup-source capability.
> > 
> > This patch adds the binding documentation to disable the wakeup-source
> > capability.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > index 870b8c5cce9b..4d6524fe3cf4 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > @@ -35,6 +35,10 @@ Optional properties:
> >   - pinctrl-0:   a phandle pointing to the pin settings for the
> >                  control gpios
> >  
> > + - edt,disable-wakeup-source: If left the device will act as wakeup-source
> > +			      (for legacy compatibility). Add the property
> > +			      so the device won't act as wakeup-source.
> 
> I think this is too ugly and I consider it being a bug in the driver
> that it enables wakeup unconditionally.

That's right.

> Let's just update DTS in tree for devices that actually want it (I am
> curious how many that do not declare "wakeup-source" have it working and
> actually want it) and key the dirver behavior off the standard property.

There are a few DTS using this driver and the current driver behaviour.
We need to keep the backward compatibility since the DTB is part of the
firmware and firmware isn't always included during a system-update. I
know its ugly but IMHO that's the right way to go to keep the backward
compatibility. Let us see what the DT-folk says.

> Until we start seeing this controller in devices that actually have DTS
> in hardware device tree I think it is better to use standard property.

Sorry, I didn't get you here..

Regards,
  Marco


> 
> Thanks.
> 
> -- 
> Dmitry
>
Dmitry Torokhov Sept. 17, 2019, 5:26 p.m. UTC | #3
On Tue, Sep 17, 2019 at 07:18:14PM +0200, Marco Felsch wrote:
> Hi Dmitry,
> 
> On 19-09-17 10:07, Dmitry Torokhov wrote:
> > On Tue, Sep 17, 2019 at 05:58:04PM +0200, Marco Felsch wrote:
> > > The default driver behaviour is to enable the wakeup-source everytime.
> > > There are hardware designs which have a dedicated gpio to act as wakeup
> > > device. So it must be allowed to disable the wakeup-source capability.
> > > 
> > > This patch adds the binding documentation to disable the wakeup-source
> > > capability.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > index 870b8c5cce9b..4d6524fe3cf4 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > @@ -35,6 +35,10 @@ Optional properties:
> > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > >                  control gpios
> > >  
> > > + - edt,disable-wakeup-source: If left the device will act as wakeup-source
> > > +			      (for legacy compatibility). Add the property
> > > +			      so the device won't act as wakeup-source.
> > 
> > I think this is too ugly and I consider it being a bug in the driver
> > that it enables wakeup unconditionally.
> 
> That's right.
> 
> > Let's just update DTS in tree for devices that actually want it (I am
> > curious how many that do not declare "wakeup-source" have it working and
> > actually want it) and key the dirver behavior off the standard property.
> 
> There are a few DTS using this driver and the current driver behaviour.
> We need to keep the backward compatibility since the DTB is part of the
> firmware and firmware isn't always included during a system-update. I
> know its ugly but IMHO that's the right way to go to keep the backward
> compatibility. Let us see what the DT-folk says.
> 
> > Until we start seeing this controller in devices that actually have DTS
> > in hardware device tree I think it is better to use standard property.
> 
> Sorry, I didn't get you here..

What I was trying to say is that I have not actually seen DTB that is
part of hardware or separately upgradable frimware (not talking about
ppc or sparc boxes, but ones that might be using this driver). It is
always built into the kernel in my experience, so backward compatibility
is simply a tool that is being used to prevent us from being too wild
with hacking on bindings, but rarely a practical concern.

In cases like this I think it is worthwhile to simply update in-tree
DTS and arrive at a sane binding.

Thanks.
Marco Felsch Sept. 18, 2019, 8:06 a.m. UTC | #4
On 19-09-17 10:26, Dmitry Torokhov wrote:
> On Tue, Sep 17, 2019 at 07:18:14PM +0200, Marco Felsch wrote:
> > Hi Dmitry,
> > 
> > On 19-09-17 10:07, Dmitry Torokhov wrote:
> > > On Tue, Sep 17, 2019 at 05:58:04PM +0200, Marco Felsch wrote:
> > > > The default driver behaviour is to enable the wakeup-source everytime.
> > > > There are hardware designs which have a dedicated gpio to act as wakeup
> > > > device. So it must be allowed to disable the wakeup-source capability.
> > > > 
> > > > This patch adds the binding documentation to disable the wakeup-source
> > > > capability.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > index 870b8c5cce9b..4d6524fe3cf4 100644
> > > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > @@ -35,6 +35,10 @@ Optional properties:
> > > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > > >                  control gpios
> > > >  
> > > > + - edt,disable-wakeup-source: If left the device will act as wakeup-source
> > > > +			      (for legacy compatibility). Add the property
> > > > +			      so the device won't act as wakeup-source.
> > > 
> > > I think this is too ugly and I consider it being a bug in the driver
> > > that it enables wakeup unconditionally.
> > 
> > That's right.
> > 
> > > Let's just update DTS in tree for devices that actually want it (I am
> > > curious how many that do not declare "wakeup-source" have it working and
> > > actually want it) and key the dirver behavior off the standard property.
> > 
> > There are a few DTS using this driver and the current driver behaviour.
> > We need to keep the backward compatibility since the DTB is part of the
> > firmware and firmware isn't always included during a system-update. I
> > know its ugly but IMHO that's the right way to go to keep the backward
> > compatibility. Let us see what the DT-folk says.
> > 
> > > Until we start seeing this controller in devices that actually have DTS
> > > in hardware device tree I think it is better to use standard property.
> > 
> > Sorry, I didn't get you here..
> 
> What I was trying to say is that I have not actually seen DTB that is
> part of hardware or separately upgradable frimware (not talking about
> ppc or sparc boxes, but ones that might be using this driver). It is
> always built into the kernel in my experience, so backward compatibility
> is simply a tool that is being used to prevent us from being too wild
> with hacking on bindings, but rarely a practical concern.

Thanks, now I got you :)

> In cases like this I think it is worthwhile to simply update in-tree
> DTS and arrive at a sane binding.

I'm with you, should we wait for Rob's ack before we go this way?

Regards,
  Marco

> Thanks.
> 
> -- 
> Dmitry
>
andriy.shevchenko@linux.intel.com Sept. 18, 2019, 8:31 a.m. UTC | #5
On Wed, Sep 18, 2019 at 10:06:09AM +0200, Marco Felsch wrote:
> On 19-09-17 10:26, Dmitry Torokhov wrote:
> > On Tue, Sep 17, 2019 at 07:18:14PM +0200, Marco Felsch wrote:
> > > On 19-09-17 10:07, Dmitry Torokhov wrote:

> > What I was trying to say is that I have not actually seen DTB that is
> > part of hardware or separately upgradable frimware (not talking about
> > ppc or sparc boxes, but ones that might be using this driver). It is
> > always built into the kernel in my experience, so backward compatibility
> > is simply a tool that is being used to prevent us from being too wild
> > with hacking on bindings, but rarely a practical concern.
> 
> Thanks, now I got you :)
> 
> > In cases like this I think it is worthwhile to simply update in-tree
> > DTS and arrive at a sane binding.
> 
> I'm with you, should we wait for Rob's ack before we go this way?

I also support this way.
Rob Herring Sept. 30, 2019, 11:11 p.m. UTC | #6
On Tue, Sep 17, 2019 at 10:26:58AM -0700, Dmitry Torokhov wrote:
> On Tue, Sep 17, 2019 at 07:18:14PM +0200, Marco Felsch wrote:
> > Hi Dmitry,
> > 
> > On 19-09-17 10:07, Dmitry Torokhov wrote:
> > > On Tue, Sep 17, 2019 at 05:58:04PM +0200, Marco Felsch wrote:
> > > > The default driver behaviour is to enable the wakeup-source everytime.
> > > > There are hardware designs which have a dedicated gpio to act as wakeup
> > > > device. So it must be allowed to disable the wakeup-source capability.
> > > > 
> > > > This patch adds the binding documentation to disable the wakeup-source
> > > > capability.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > index 870b8c5cce9b..4d6524fe3cf4 100644
> > > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > @@ -35,6 +35,10 @@ Optional properties:
> > > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > > >                  control gpios
> > > >  
> > > > + - edt,disable-wakeup-source: If left the device will act as wakeup-source
> > > > +			      (for legacy compatibility). Add the property
> > > > +			      so the device won't act as wakeup-source.
> > > 
> > > I think this is too ugly and I consider it being a bug in the driver
> > > that it enables wakeup unconditionally.
> > 
> > That's right.
> > 
> > > Let's just update DTS in tree for devices that actually want it (I am
> > > curious how many that do not declare "wakeup-source" have it working and
> > > actually want it) and key the dirver behavior off the standard property.
> > 
> > There are a few DTS using this driver and the current driver behaviour.
> > We need to keep the backward compatibility since the DTB is part of the
> > firmware and firmware isn't always included during a system-update. I
> > know its ugly but IMHO that's the right way to go to keep the backward
> > compatibility. Let us see what the DT-folk says.
> > 
> > > Until we start seeing this controller in devices that actually have DTS
> > > in hardware device tree I think it is better to use standard property.
> > 
> > Sorry, I didn't get you here..
> 
> What I was trying to say is that I have not actually seen DTB that is
> part of hardware or separately upgradable frimware (not talking about
> ppc or sparc boxes, but ones that might be using this driver). It is
> always built into the kernel in my experience, so backward compatibility
> is simply a tool that is being used to prevent us from being too wild
> with hacking on bindings, but rarely a practical concern.

Well, that's self fulfilling...

> In cases like this I think it is worthwhile to simply update in-tree
> DTS and arrive at a sane binding.

Get the maintainers of the affected platforms to agree to the changes.

Rob
Marco Felsch Oct. 2, 2019, 1 p.m. UTC | #7
Hi,

all of you using a edt,* touchscreen and currently the driver marks
the touchscreen as wakeup source. To keep backward compatibility I added
a workaround binding (see below) but Dmitry prefer to use the normal
"wakeup-source" binding and change the affected device-tree's
(discussion below). Can you give me your ack/nack for Dmitry's solution?

Regards,
  Marco

On 19-09-30 18:11, Rob Herring wrote:
> On Tue, Sep 17, 2019 at 10:26:58AM -0700, Dmitry Torokhov wrote:
> > On Tue, Sep 17, 2019 at 07:18:14PM +0200, Marco Felsch wrote:
> > > Hi Dmitry,
> > > 
> > > On 19-09-17 10:07, Dmitry Torokhov wrote:
> > > > On Tue, Sep 17, 2019 at 05:58:04PM +0200, Marco Felsch wrote:
> > > > > The default driver behaviour is to enable the wakeup-source everytime.
> > > > > There are hardware designs which have a dedicated gpio to act as wakeup
> > > > > device. So it must be allowed to disable the wakeup-source capability.
> > > > > 
> > > > > This patch adds the binding documentation to disable the wakeup-source
> > > > > capability.
> > > > > 
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > ---
> > > > >  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > index 870b8c5cce9b..4d6524fe3cf4 100644
> > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > @@ -35,6 +35,10 @@ Optional properties:
> > > > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > > > >                  control gpios
> > > > >  
> > > > > + - edt,disable-wakeup-source: If left the device will act as wakeup-source
> > > > > +			      (for legacy compatibility). Add the property
> > > > > +			      so the device won't act as wakeup-source.
> > > > 
> > > > I think this is too ugly and I consider it being a bug in the driver
> > > > that it enables wakeup unconditionally.
> > > 
> > > That's right.
> > > 
> > > > Let's just update DTS in tree for devices that actually want it (I am
> > > > curious how many that do not declare "wakeup-source" have it working and
> > > > actually want it) and key the dirver behavior off the standard property.
> > > 
> > > There are a few DTS using this driver and the current driver behaviour.
> > > We need to keep the backward compatibility since the DTB is part of the
> > > firmware and firmware isn't always included during a system-update. I
> > > know its ugly but IMHO that's the right way to go to keep the backward
> > > compatibility. Let us see what the DT-folk says.
> > > 
> > > > Until we start seeing this controller in devices that actually have DTS
> > > > in hardware device tree I think it is better to use standard property.
> > > 
> > > Sorry, I didn't get you here..
> > 
> > What I was trying to say is that I have not actually seen DTB that is
> > part of hardware or separately upgradable frimware (not talking about
> > ppc or sparc boxes, but ones that might be using this driver). It is
> > always built into the kernel in my experience, so backward compatibility
> > is simply a tool that is being used to prevent us from being too wild
> > with hacking on bindings, but rarely a practical concern.
> 
> Well, that's self fulfilling...
> 
> > In cases like this I think it is worthwhile to simply update in-tree
> > DTS and arrive at a sane binding.
> 
> Get the maintainers of the affected platforms to agree to the changes.
> 
> Rob
>
Maxime Ripard Oct. 2, 2019, 2:48 p.m. UTC | #8
On Wed, Oct 02, 2019 at 03:00:18PM +0200, Marco Felsch wrote:
> all of you using a edt,* touchscreen and currently the driver marks
> the touchscreen as wakeup source. To keep backward compatibility I added
> a workaround binding (see below) but Dmitry prefer to use the normal
> "wakeup-source" binding and change the affected device-tree's
> (discussion below). Can you give me your ack/nack for Dmitry's solution?

we don't have any upstrem suspend support on the Allwinner SoCs, so
you can safely ignore all the sunxi DTS here.

Maxime
Benoit Parrot Oct. 2, 2019, 3:20 p.m. UTC | #9
Marco Felsch <m.felsch@pengutronix.de> wrote on Wed [2019-Oct-02 15:00:18 +0200]:
> Hi,
> 
> all of you using a edt,* touchscreen and currently the driver marks
> the touchscreen as wakeup source. To keep backward compatibility I added
> a workaround binding (see below) but Dmitry prefer to use the normal
> "wakeup-source" binding and change the affected device-tree's
> (discussion below). Can you give me your ack/nack for Dmitry's solution?

So, if I understand this correctly, currently the driver always assume it
is a wakeup source regardless if the "wakeup-source" DT property being
present or not. And the proposed change now is to fix the driver so that it
will assume a wakeup source role only if the DT property is present?

If that is the case then ACK from my side for the AM43x devices.

Regards,
Benoit

> 
> Regards,
>   Marco
>
Alexandre Belloni Oct. 2, 2019, 3:24 p.m. UTC | #10
On 02/10/2019 15:00:18+0200, Marco Felsch wrote:
> Hi,
> 
> all of you using a edt,* touchscreen and currently the driver marks
> the touchscreen as wakeup source. To keep backward compatibility I added
> a workaround binding (see below) but Dmitry prefer to use the normal
> "wakeup-source" binding and change the affected device-tree's
> (discussion below). Can you give me your ack/nack for Dmitry's solution?
> 

Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> Regards,
>   Marco
> 
> On 19-09-30 18:11, Rob Herring wrote:
> > On Tue, Sep 17, 2019 at 10:26:58AM -0700, Dmitry Torokhov wrote:
> > > On Tue, Sep 17, 2019 at 07:18:14PM +0200, Marco Felsch wrote:
> > > > Hi Dmitry,
> > > > 
> > > > On 19-09-17 10:07, Dmitry Torokhov wrote:
> > > > > On Tue, Sep 17, 2019 at 05:58:04PM +0200, Marco Felsch wrote:
> > > > > > The default driver behaviour is to enable the wakeup-source everytime.
> > > > > > There are hardware designs which have a dedicated gpio to act as wakeup
> > > > > > device. So it must be allowed to disable the wakeup-source capability.
> > > > > > 
> > > > > > This patch adds the binding documentation to disable the wakeup-source
> > > > > > capability.
> > > > > > 
> > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > ---
> > > > > >  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > > index 870b8c5cce9b..4d6524fe3cf4 100644
> > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > > @@ -35,6 +35,10 @@ Optional properties:
> > > > > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > > > > >                  control gpios
> > > > > >  
> > > > > > + - edt,disable-wakeup-source: If left the device will act as wakeup-source
> > > > > > +			      (for legacy compatibility). Add the property
> > > > > > +			      so the device won't act as wakeup-source.
> > > > > 
> > > > > I think this is too ugly and I consider it being a bug in the driver
> > > > > that it enables wakeup unconditionally.
> > > > 
> > > > That's right.
> > > > 
> > > > > Let's just update DTS in tree for devices that actually want it (I am
> > > > > curious how many that do not declare "wakeup-source" have it working and
> > > > > actually want it) and key the dirver behavior off the standard property.
> > > > 
> > > > There are a few DTS using this driver and the current driver behaviour.
> > > > We need to keep the backward compatibility since the DTB is part of the
> > > > firmware and firmware isn't always included during a system-update. I
> > > > know its ugly but IMHO that's the right way to go to keep the backward
> > > > compatibility. Let us see what the DT-folk says.
> > > > 
> > > > > Until we start seeing this controller in devices that actually have DTS
> > > > > in hardware device tree I think it is better to use standard property.
> > > > 
> > > > Sorry, I didn't get you here..
> > > 
> > > What I was trying to say is that I have not actually seen DTB that is
> > > part of hardware or separately upgradable frimware (not talking about
> > > ppc or sparc boxes, but ones that might be using this driver). It is
> > > always built into the kernel in my experience, so backward compatibility
> > > is simply a tool that is being used to prevent us from being too wild
> > > with hacking on bindings, but rarely a practical concern.
> > 
> > Well, that's self fulfilling...
> > 
> > > In cases like this I think it is worthwhile to simply update in-tree
> > > DTS and arrive at a sane binding.
> > 
> > Get the maintainers of the affected platforms to agree to the changes.
> > 
> > Rob
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Dmitry Torokhov Oct. 2, 2019, 5:30 p.m. UTC | #11
On Wed, Oct 02, 2019 at 10:20:03AM -0500, Benoit Parrot wrote:
> Marco Felsch <m.felsch@pengutronix.de> wrote on Wed [2019-Oct-02 15:00:18 +0200]:
> > Hi,
> > 
> > all of you using a edt,* touchscreen and currently the driver marks
> > the touchscreen as wakeup source. To keep backward compatibility I added
> > a workaround binding (see below) but Dmitry prefer to use the normal
> > "wakeup-source" binding and change the affected device-tree's
> > (discussion below). Can you give me your ack/nack for Dmitry's solution?
> 
> So, if I understand this correctly, currently the driver always assume it
> is a wakeup source regardless if the "wakeup-source" DT property being
> present or not. And the proposed change now is to fix the driver so that it
> will assume a wakeup source role only if the DT property is present?

Yes, that is correct.

> 
> If that is the case then ACK from my side for the AM43x devices.

Thanks.
Shawn Guo Oct. 14, 2019, 9:14 a.m. UTC | #12
On Wed, Oct 02, 2019 at 03:00:18PM +0200, Marco Felsch wrote:
> Hi,
> 
> all of you using a edt,* touchscreen and currently the driver marks
> the touchscreen as wakeup source. To keep backward compatibility I added
> a workaround binding (see below) but Dmitry prefer to use the normal
> "wakeup-source" binding and change the affected device-tree's
> (discussion below). Can you give me your ack/nack for Dmitry's solution?

Acked-by: Shawn Guo <shawnguo@kernel.org>

> 
> Regards,
>   Marco
> 
> On 19-09-30 18:11, Rob Herring wrote:
> > On Tue, Sep 17, 2019 at 10:26:58AM -0700, Dmitry Torokhov wrote:
> > > On Tue, Sep 17, 2019 at 07:18:14PM +0200, Marco Felsch wrote:
> > > > Hi Dmitry,
> > > > 
> > > > On 19-09-17 10:07, Dmitry Torokhov wrote:
> > > > > On Tue, Sep 17, 2019 at 05:58:04PM +0200, Marco Felsch wrote:
> > > > > > The default driver behaviour is to enable the wakeup-source everytime.
> > > > > > There are hardware designs which have a dedicated gpio to act as wakeup
> > > > > > device. So it must be allowed to disable the wakeup-source capability.
> > > > > > 
> > > > > > This patch adds the binding documentation to disable the wakeup-source
> > > > > > capability.
> > > > > > 
> > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > ---
> > > > > >  .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt      | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > > index 870b8c5cce9b..4d6524fe3cf4 100644
> > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > > > @@ -35,6 +35,10 @@ Optional properties:
> > > > > >   - pinctrl-0:   a phandle pointing to the pin settings for the
> > > > > >                  control gpios
> > > > > >  
> > > > > > + - edt,disable-wakeup-source: If left the device will act as wakeup-source
> > > > > > +			      (for legacy compatibility). Add the property
> > > > > > +			      so the device won't act as wakeup-source.
> > > > > 
> > > > > I think this is too ugly and I consider it being a bug in the driver
> > > > > that it enables wakeup unconditionally.
> > > > 
> > > > That's right.
> > > > 
> > > > > Let's just update DTS in tree for devices that actually want it (I am
> > > > > curious how many that do not declare "wakeup-source" have it working and
> > > > > actually want it) and key the dirver behavior off the standard property.
> > > > 
> > > > There are a few DTS using this driver and the current driver behaviour.
> > > > We need to keep the backward compatibility since the DTB is part of the
> > > > firmware and firmware isn't always included during a system-update. I
> > > > know its ugly but IMHO that's the right way to go to keep the backward
> > > > compatibility. Let us see what the DT-folk says.
> > > > 
> > > > > Until we start seeing this controller in devices that actually have DTS
> > > > > in hardware device tree I think it is better to use standard property.
> > > > 
> > > > Sorry, I didn't get you here..
> > > 
> > > What I was trying to say is that I have not actually seen DTB that is
> > > part of hardware or separately upgradable frimware (not talking about
> > > ppc or sparc boxes, but ones that might be using this driver). It is
> > > always built into the kernel in my experience, so backward compatibility
> > > is simply a tool that is being used to prevent us from being too wild
> > > with hacking on bindings, but rarely a practical concern.
> > 
> > Well, that's self fulfilling...
> > 
> > > In cases like this I think it is worthwhile to simply update in-tree
> > > DTS and arrive at a sane binding.
> > 
> > Get the maintainers of the affected platforms to agree to the changes.
> > 
> > Rob
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 870b8c5cce9b..4d6524fe3cf4 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -35,6 +35,10 @@  Optional properties:
  - pinctrl-0:   a phandle pointing to the pin settings for the
                 control gpios
 
+ - edt,disable-wakeup-source: If left the device will act as wakeup-source
+			      (for legacy compatibility). Add the property
+			      so the device won't act as wakeup-source.
+
  - threshold:   allows setting the "click"-threshold in the range
                 from 0 to 80.