Linux Input Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
@ 2019-09-04  6:23 Robin van der Gracht
  2019-09-04  6:52 ` Marco Felsch
  2019-09-12 20:13 ` Dmitry Torokhov
  0 siblings, 2 replies; 11+ messages in thread
From: Robin van der Gracht @ 2019-09-04  6:23 UTC (permalink / raw)
  To: robin
  Cc: linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, Dmitry Torokhov, RobinGong,
	Pengutronix Kernel Team, Marco Felsch, Shawn Guo, Adam Ford,
	linux-arm-kernel @ lists . infradead . org

The first generation i.MX6 processors does not send an interrupt when the
power key is pressed. It sends a power down request interrupt if the key is
released before a hard shutdown (5 second press). This should allow
software to bring down the SoC safely.

For this driver to work as a regular power key with the older SoCs, we need
to send a keypress AND release when we get the power down request irq.

Signed-off-by: Robin van der Gracht <robin@protonic.nl>
---

Changes v2 -> v3:
 - Drop alt compatible string for identifying first revision snvs hardware,
   read minor revision from register instead.
 - Drop imx6qdl.dtsi modification and device-tree binding documentation.
 - Add an additional input_sync() to create 2 seperate input reports for press
   and release.

 drivers/input/keyboard/Kconfig       |  2 +-
 drivers/input/keyboard/snvs_pwrkey.c | 28 ++++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 7c4f19dab34f..937e58da5ce1 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
 	depends on OF
 	help
 	  This is the snvs powerkey driver for the Freescale i.MX application
-	  processors that are newer than i.MX6 SX.
+	  processors.
 
 	  To compile this driver as a module, choose M here; the
 	  module will be called snvs_pwrkey.
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 5342d8d45f81..828580eee0d2 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -19,6 +19,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
 
+#define SNVS_HPVIDR1_REG 0xF8
 #define SNVS_LPSR_REG	0x4C	/* LP Status Register */
 #define SNVS_LPCR_REG	0x38	/* LP Control Register */
 #define SNVS_HPSR_REG	0x14
@@ -37,6 +38,7 @@ struct pwrkey_drv_data {
 	int wakeup;
 	struct timer_list check_timer;
 	struct input_dev *input;
+	u8 minor_rev;
 };
 
 static void imx_imx_snvs_check_for_events(struct timer_list *t)
@@ -45,6 +47,20 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
 	struct input_dev *input = pdata->input;
 	u32 state;
 
+	if (pdata->minor_rev == 0) {
+		/*
+		 * The first generation i.MX6 SoCs only sends an interrupt on
+		 * button release. To mimic power-key usage, we'll prepend a
+		 * press event.
+		 */
+		input_report_key(input, pdata->keycode, 1);
+		input_sync(input);
+		input_report_key(input, pdata->keycode, 0);
+		input_sync(input);
+		pm_relax(input->dev.parent);
+		return;
+	}
+
 	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
 	state = state & SNVS_HPSR_BTN ? 1 : 0;
 
@@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
 {
 	struct platform_device *pdev = dev_id;
 	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+	unsigned long expire = jiffies;
 	u32 lp_status;
 
 	pm_wakeup_event(pdata->input->dev.parent, 0);
 
 	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
-	if (lp_status & SNVS_LPSR_SPO)
-		mod_timer(&pdata->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
+	if (lp_status & SNVS_LPSR_SPO) {
+		if (pdata->minor_rev > 0)
+			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
+		mod_timer(&pdata->check_timer, expire);
+	}
 
 	/* clear SPO status */
 	regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
@@ -94,6 +114,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	struct input_dev *input = NULL;
 	struct device_node *np;
 	int error;
+	u32 vid;
 
 	/* Get SNVS register Page */
 	np = pdev->dev.of_node;
@@ -123,6 +144,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	regmap_read(pdata->snvs, SNVS_HPVIDR1_REG, &vid);
+	pdata->minor_rev = vid & 0xff;
+
 	regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_DEP_EN, SNVS_LPCR_DEP_EN);
 
 	/* clear the unexpected interrupt before driver ready */
-- 
2.20.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-09-04  6:23 [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q Robin van der Gracht
@ 2019-09-04  6:52 ` Marco Felsch
  2019-09-13  7:39   ` robin
  2019-09-12 20:13 ` Dmitry Torokhov
  1 sibling, 1 reply; 11+ messages in thread
From: Marco Felsch @ 2019-09-04  6:52 UTC (permalink / raw)
  To: Robin van der Gracht
  Cc: linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, Dmitry Torokhov, RobinGong,
	Pengutronix Kernel Team, Shawn Guo, Adam Ford,
	linux-arm-kernel @ lists . infradead . org

Hi Robin,

thanks for the patch it looks quite good, just two minor nitpicks.

On 19-09-04 06:23, Robin van der Gracht wrote:
> The first generation i.MX6 processors does not send an interrupt when the
> power key is pressed. It sends a power down request interrupt if the key is
> released before a hard shutdown (5 second press). This should allow
> software to bring down the SoC safely.
> 
> For this driver to work as a regular power key with the older SoCs, we need
> to send a keypress AND release when we get the power down request irq.
> 
> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> ---
> 
> Changes v2 -> v3:
>  - Drop alt compatible string for identifying first revision snvs hardware,
>    read minor revision from register instead.
>  - Drop imx6qdl.dtsi modification and device-tree binding documentation.
>  - Add an additional input_sync() to create 2 seperate input reports for press
>    and release.
> 
>  drivers/input/keyboard/Kconfig       |  2 +-
>  drivers/input/keyboard/snvs_pwrkey.c | 28 ++++++++++++++++++++++++++--
>  2 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 7c4f19dab34f..937e58da5ce1 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
>  	depends on OF
>  	help
>  	  This is the snvs powerkey driver for the Freescale i.MX application
> -	  processors that are newer than i.MX6 SX.
> +	  processors.
>  
>  	  To compile this driver as a module, choose M here; the
>  	  module will be called snvs_pwrkey.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 5342d8d45f81..828580eee0d2 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -19,6 +19,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
>  
> +#define SNVS_HPVIDR1_REG 0xF8
>  #define SNVS_LPSR_REG	0x4C	/* LP Status Register */
>  #define SNVS_LPCR_REG	0x38	/* LP Control Register */
>  #define SNVS_HPSR_REG	0x14
> @@ -37,6 +38,7 @@ struct pwrkey_drv_data {
>  	int wakeup;
>  	struct timer_list check_timer;
>  	struct input_dev *input;
> +	u8 minor_rev;
>  };
>  
>  static void imx_imx_snvs_check_for_events(struct timer_list *t)
> @@ -45,6 +47,20 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
>  	struct input_dev *input = pdata->input;
>  	u32 state;
>  
> +	if (pdata->minor_rev == 0) {

Should we use a define here and ..

> +		/*
> +		 * The first generation i.MX6 SoCs only sends an interrupt on
> +		 * button release. To mimic power-key usage, we'll prepend a
> +		 * press event.
> +		 */
> +		input_report_key(input, pdata->keycode, 1);
> +		input_sync(input);
> +		input_report_key(input, pdata->keycode, 0);
> +		input_sync(input);
> +		pm_relax(input->dev.parent);
> +		return;
> +	}
> +
>  	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
>  	state = state & SNVS_HPSR_BTN ? 1 : 0;
>  
> @@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
>  {
>  	struct platform_device *pdev = dev_id;
>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> +	unsigned long expire = jiffies;
>  	u32 lp_status;
>  
>  	pm_wakeup_event(pdata->input->dev.parent, 0);
>  
>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> -	if (lp_status & SNVS_LPSR_SPO)
> -		mod_timer(&pdata->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> +	if (lp_status & SNVS_LPSR_SPO) {
> +		if (pdata->minor_rev > 0)

here? Just a nitpick, feel free to add/drop it.

> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
> +		mod_timer(&pdata->check_timer, expire);
> +	}
>  
>  	/* clear SPO status */
>  	regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
> @@ -94,6 +114,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	struct input_dev *input = NULL;
>  	struct device_node *np;
>  	int error;
> +	u32 vid;
>  
>  	/* Get SNVS register Page */
>  	np = pdev->dev.of_node;
> @@ -123,6 +144,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	regmap_read(pdata->snvs, SNVS_HPVIDR1_REG, &vid);

Should we check the return val here?

Regards,
  Marco

> +	pdata->minor_rev = vid & 0xff;
> +
>  	regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_DEP_EN, SNVS_LPCR_DEP_EN);
>  
>  	/* clear the unexpected interrupt before driver ready */
> -- 
> 2.20.1
> 
> 

-- 
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 |

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-09-04  6:23 [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q Robin van der Gracht
  2019-09-04  6:52 ` Marco Felsch
@ 2019-09-12 20:13 ` Dmitry Torokhov
  2019-09-13  7:30   ` robin
  1 sibling, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2019-09-12 20:13 UTC (permalink / raw)
  To: Robin van der Gracht
  Cc: linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, RobinGong,
	Pengutronix Kernel Team, Marco Felsch, Shawn Guo, Adam Ford,
	linux-arm-kernel @ lists . infradead . org

Hi Robin,

On Wed, Sep 04, 2019 at 06:23:29AM +0000, Robin van der Gracht wrote:
> The first generation i.MX6 processors does not send an interrupt when the
> power key is pressed. It sends a power down request interrupt if the key is
> released before a hard shutdown (5 second press). This should allow
> software to bring down the SoC safely.
> 
> For this driver to work as a regular power key with the older SoCs, we need
> to send a keypress AND release when we get the power down request irq.
> 
> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> ---
> 
> Changes v2 -> v3:
>  - Drop alt compatible string for identifying first revision snvs hardware,
>    read minor revision from register instead.
>  - Drop imx6qdl.dtsi modification and device-tree binding documentation.
>  - Add an additional input_sync() to create 2 seperate input reports for press
>    and release.
> 
>  drivers/input/keyboard/Kconfig       |  2 +-
>  drivers/input/keyboard/snvs_pwrkey.c | 28 ++++++++++++++++++++++++++--
>  2 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 7c4f19dab34f..937e58da5ce1 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
>  	depends on OF
>  	help
>  	  This is the snvs powerkey driver for the Freescale i.MX application
> -	  processors that are newer than i.MX6 SX.
> +	  processors.
>  
>  	  To compile this driver as a module, choose M here; the
>  	  module will be called snvs_pwrkey.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 5342d8d45f81..828580eee0d2 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -19,6 +19,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
>  
> +#define SNVS_HPVIDR1_REG 0xF8
>  #define SNVS_LPSR_REG	0x4C	/* LP Status Register */
>  #define SNVS_LPCR_REG	0x38	/* LP Control Register */
>  #define SNVS_HPSR_REG	0x14
> @@ -37,6 +38,7 @@ struct pwrkey_drv_data {
>  	int wakeup;
>  	struct timer_list check_timer;
>  	struct input_dev *input;
> +	u8 minor_rev;
>  };
>  
>  static void imx_imx_snvs_check_for_events(struct timer_list *t)
> @@ -45,6 +47,20 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
>  	struct input_dev *input = pdata->input;
>  	u32 state;
>  
> +	if (pdata->minor_rev == 0) {
> +		/*
> +		 * The first generation i.MX6 SoCs only sends an interrupt on
> +		 * button release. To mimic power-key usage, we'll prepend a
> +		 * press event.
> +		 */
> +		input_report_key(input, pdata->keycode, 1);
> +		input_sync(input);
> +		input_report_key(input, pdata->keycode, 0);
> +		input_sync(input);
> +		pm_relax(input->dev.parent);
> +		return;
> +	}
> +
>  	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
>  	state = state & SNVS_HPSR_BTN ? 1 : 0;
>  
> @@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
>  {
>  	struct platform_device *pdev = dev_id;
>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> +	unsigned long expire = jiffies;
>  	u32 lp_status;
>  
>  	pm_wakeup_event(pdata->input->dev.parent, 0);
>  
>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> -	if (lp_status & SNVS_LPSR_SPO)
> -		mod_timer(&pdata->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> +	if (lp_status & SNVS_LPSR_SPO) {
> +		if (pdata->minor_rev > 0)
> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
> +		mod_timer(&pdata->check_timer, expire);

Why do we even need to fire the timer in case of the first generation
hardware? Just send press and release events directly from the ISR.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-09-12 20:13 ` Dmitry Torokhov
@ 2019-09-13  7:30   ` robin
  2019-09-16  7:45     ` Robin Gong
  0 siblings, 1 reply; 11+ messages in thread
From: robin @ 2019-09-13  7:30 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, RobinGong,
	Pengutronix Kernel Team, Marco Felsch, Shawn Guo, Adam Ford,
	linux-arm-kernel @ lists . infradead . org

Hi Dmitry,

On 2019-09-12 22:13, Dmitry Torokhov wrote:
> Hi Robin,
> 
> On Wed, Sep 04, 2019 at 06:23:29AM +0000, Robin van der Gracht wrote:
>> The first generation i.MX6 processors does not send an interrupt when 
>> the
>> power key is pressed. It sends a power down request interrupt if the 
>> key is
>> released before a hard shutdown (5 second press). This should allow
>> software to bring down the SoC safely.
>> 
>> For this driver to work as a regular power key with the older SoCs, we 
>> need
>> to send a keypress AND release when we get the power down request irq.
>> 
>> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
>> ---
>> 
>> Changes v2 -> v3:
>>  - Drop alt compatible string for identifying first revision snvs 
>> hardware,
>>    read minor revision from register instead.
>>  - Drop imx6qdl.dtsi modification and device-tree binding 
>> documentation.
>>  - Add an additional input_sync() to create 2 seperate input reports 
>> for press
>>    and release.
>> 
>>  drivers/input/keyboard/Kconfig       |  2 +-
>>  drivers/input/keyboard/snvs_pwrkey.c | 28 
>> ++++++++++++++++++++++++++--
>>  2 files changed, 27 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/input/keyboard/Kconfig 
>> b/drivers/input/keyboard/Kconfig
>> index 7c4f19dab34f..937e58da5ce1 100644
>> --- a/drivers/input/keyboard/Kconfig
>> +++ b/drivers/input/keyboard/Kconfig
>> @@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
>>  	depends on OF
>>  	help
>>  	  This is the snvs powerkey driver for the Freescale i.MX 
>> application
>> -	  processors that are newer than i.MX6 SX.
>> +	  processors.
>> 
>>  	  To compile this driver as a module, choose M here; the
>>  	  module will be called snvs_pwrkey.
>> diff --git a/drivers/input/keyboard/snvs_pwrkey.c 
>> b/drivers/input/keyboard/snvs_pwrkey.c
>> index 5342d8d45f81..828580eee0d2 100644
>> --- a/drivers/input/keyboard/snvs_pwrkey.c
>> +++ b/drivers/input/keyboard/snvs_pwrkey.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/regmap.h>
>> 
>> +#define SNVS_HPVIDR1_REG 0xF8
>>  #define SNVS_LPSR_REG	0x4C	/* LP Status Register */
>>  #define SNVS_LPCR_REG	0x38	/* LP Control Register */
>>  #define SNVS_HPSR_REG	0x14
>> @@ -37,6 +38,7 @@ struct pwrkey_drv_data {
>>  	int wakeup;
>>  	struct timer_list check_timer;
>>  	struct input_dev *input;
>> +	u8 minor_rev;
>>  };
>> 
>>  static void imx_imx_snvs_check_for_events(struct timer_list *t)
>> @@ -45,6 +47,20 @@ static void imx_imx_snvs_check_for_events(struct 
>> timer_list *t)
>>  	struct input_dev *input = pdata->input;
>>  	u32 state;
>> 
>> +	if (pdata->minor_rev == 0) {
>> +		/*
>> +		 * The first generation i.MX6 SoCs only sends an interrupt on
>> +		 * button release. To mimic power-key usage, we'll prepend a
>> +		 * press event.
>> +		 */
>> +		input_report_key(input, pdata->keycode, 1);
>> +		input_sync(input);
>> +		input_report_key(input, pdata->keycode, 0);
>> +		input_sync(input);
>> +		pm_relax(input->dev.parent);
>> +		return;
>> +	}
>> +
>>  	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
>>  	state = state & SNVS_HPSR_BTN ? 1 : 0;
>> 
>> @@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int 
>> irq, void *dev_id)
>>  {
>>  	struct platform_device *pdev = dev_id;
>>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
>> +	unsigned long expire = jiffies;
>>  	u32 lp_status;
>> 
>>  	pm_wakeup_event(pdata->input->dev.parent, 0);
>> 
>>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
>> -	if (lp_status & SNVS_LPSR_SPO)
>> -		mod_timer(&pdata->check_timer, jiffies + 
>> msecs_to_jiffies(DEBOUNCE_TIME));
>> +	if (lp_status & SNVS_LPSR_SPO) {
>> +		if (pdata->minor_rev > 0)
>> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
>> +		mod_timer(&pdata->check_timer, expire);
> 
> Why do we even need to fire the timer in case of the first generation
> hardware? Just send press and release events directly from the ISR.

Robin Gong proposed to move the code to imx_imx_snvs_check_for_events()
to improve readability and unload the ISR.

But since I, eventually, couldn't use the existing handling in
imx_imx_snvs_check_for_events(), I do see why you're asking.

I'll move the code to the ISR and submit a new patch.

Robin van der Gracht

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-09-04  6:52 ` Marco Felsch
@ 2019-09-13  7:39   ` robin
  0 siblings, 0 replies; 11+ messages in thread
From: robin @ 2019-09-13  7:39 UTC (permalink / raw)
  To: Marco Felsch
  Cc: linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, Dmitry Torokhov, RobinGong,
	Pengutronix Kernel Team, Shawn Guo, Adam Ford,
	linux-arm-kernel @ lists . infradead . org

On 2019-09-04 08:52, Marco Felsch wrote:
> Hi Robin,
> 
> thanks for the patch it looks quite good, just two minor nitpicks.
> 
> On 19-09-04 06:23, Robin van der Gracht wrote:
>> The first generation i.MX6 processors does not send an interrupt when 
>> the
>> power key is pressed. It sends a power down request interrupt if the 
>> key is
>> released before a hard shutdown (5 second press). This should allow
>> software to bring down the SoC safely.
>> 
>> For this driver to work as a regular power key with the older SoCs, we 
>> need
>> to send a keypress AND release when we get the power down request irq.
>> 
>> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
>> ---
>> 
>> Changes v2 -> v3:
>>  - Drop alt compatible string for identifying first revision snvs 
>> hardware,
>>    read minor revision from register instead.
>>  - Drop imx6qdl.dtsi modification and device-tree binding 
>> documentation.
>>  - Add an additional input_sync() to create 2 seperate input reports 
>> for press
>>    and release.
>> 
>>  drivers/input/keyboard/Kconfig       |  2 +-
>>  drivers/input/keyboard/snvs_pwrkey.c | 28 
>> ++++++++++++++++++++++++++--
>>  2 files changed, 27 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/input/keyboard/Kconfig 
>> b/drivers/input/keyboard/Kconfig
>> index 7c4f19dab34f..937e58da5ce1 100644
>> --- a/drivers/input/keyboard/Kconfig
>> +++ b/drivers/input/keyboard/Kconfig
>> @@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
>>  	depends on OF
>>  	help
>>  	  This is the snvs powerkey driver for the Freescale i.MX 
>> application
>> -	  processors that are newer than i.MX6 SX.
>> +	  processors.
>> 
>>  	  To compile this driver as a module, choose M here; the
>>  	  module will be called snvs_pwrkey.
>> diff --git a/drivers/input/keyboard/snvs_pwrkey.c 
>> b/drivers/input/keyboard/snvs_pwrkey.c
>> index 5342d8d45f81..828580eee0d2 100644
>> --- a/drivers/input/keyboard/snvs_pwrkey.c
>> +++ b/drivers/input/keyboard/snvs_pwrkey.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/regmap.h>
>> 
>> +#define SNVS_HPVIDR1_REG 0xF8
>>  #define SNVS_LPSR_REG	0x4C	/* LP Status Register */
>>  #define SNVS_LPCR_REG	0x38	/* LP Control Register */
>>  #define SNVS_HPSR_REG	0x14
>> @@ -37,6 +38,7 @@ struct pwrkey_drv_data {
>>  	int wakeup;
>>  	struct timer_list check_timer;
>>  	struct input_dev *input;
>> +	u8 minor_rev;
>>  };
>> 
>>  static void imx_imx_snvs_check_for_events(struct timer_list *t)
>> @@ -45,6 +47,20 @@ static void imx_imx_snvs_check_for_events(struct 
>> timer_list *t)
>>  	struct input_dev *input = pdata->input;
>>  	u32 state;
>> 
>> +	if (pdata->minor_rev == 0) {
> 
> Should we use a define here and ..
> 
>> +		/*
>> +		 * The first generation i.MX6 SoCs only sends an interrupt on
>> +		 * button release. To mimic power-key usage, we'll prepend a
>> +		 * press event.
>> +		 */
>> +		input_report_key(input, pdata->keycode, 1);
>> +		input_sync(input);
>> +		input_report_key(input, pdata->keycode, 0);
>> +		input_sync(input);
>> +		pm_relax(input->dev.parent);
>> +		return;
>> +	}
>> +
>>  	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
>>  	state = state & SNVS_HPSR_BTN ? 1 : 0;
>> 
>> @@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int 
>> irq, void *dev_id)
>>  {
>>  	struct platform_device *pdev = dev_id;
>>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
>> +	unsigned long expire = jiffies;
>>  	u32 lp_status;
>> 
>>  	pm_wakeup_event(pdata->input->dev.parent, 0);
>> 
>>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
>> -	if (lp_status & SNVS_LPSR_SPO)
>> -		mod_timer(&pdata->check_timer, jiffies + 
>> msecs_to_jiffies(DEBOUNCE_TIME));
>> +	if (lp_status & SNVS_LPSR_SPO) {
>> +		if (pdata->minor_rev > 0)
> 
> here? Just a nitpick, feel free to add/drop it.

Like a Macro?

#define FIRST_HW_REV(pdata)      (pdata->minor_rev == 0)

if (FIRST_HW_REV(pdata) {
         ...
}


or just a define to identify the minor rev used for the first hw 
revision


#define FIRST_HW_MINOR_REV       0

if (pdata->minor_rev == FIRST_HW_MINOR_REV) {
         ...
}

Regards,
Robin van der Gracht

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-09-13  7:30   ` robin
@ 2019-09-16  7:45     ` Robin Gong
  2019-09-16 23:37       ` Dmitry Torokhov
  0 siblings, 1 reply; 11+ messages in thread
From: Robin Gong @ 2019-09-16  7:45 UTC (permalink / raw)
  To: robin, Dmitry Torokhov
  Cc: Shawn Guo, Marco Felsch, linux-kernel @ vger . kernel . org,
	Pengutronix Kernel Team, linux-input @ vger . kernel . org,
	Adam Ford, linux-arm-kernel @ lists . infradead . org

On 2019/9/13 15:31 robin <robin@protonic.nl> wrote:> 
> Hi Dmitry,
> 
> On 2019-09-12 22:13, Dmitry Torokhov wrote:
> > Hi Robin,
> >
> > On Wed, Sep 04, 2019 at 06:23:29AM +0000, Robin van der Gracht wrote:
> >> The first generation i.MX6 processors does not send an interrupt when
> >> the power key is pressed. It sends a power down request interrupt if
> >> the key is released before a hard shutdown (5 second press). This
> >> should allow software to bring down the SoC safely.
> >>
> >> For this driver to work as a regular power key with the older SoCs,
> >> we need to send a keypress AND release when we get the power down
> >> request irq.
> >>
> >> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> >> ---
> >> @@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int
> >> irq, void *dev_id)  {
> >>  	struct platform_device *pdev = dev_id;
> >>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> >> +	unsigned long expire = jiffies;
> >>  	u32 lp_status;
> >>
> >>  	pm_wakeup_event(pdata->input->dev.parent, 0);
> >>
> >>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> >> -	if (lp_status & SNVS_LPSR_SPO)
> >> -		mod_timer(&pdata->check_timer, jiffies +
> >> msecs_to_jiffies(DEBOUNCE_TIME));
> >> +	if (lp_status & SNVS_LPSR_SPO) {
> >> +		if (pdata->minor_rev > 0)
> >> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
> >> +		mod_timer(&pdata->check_timer, expire);
> >
> > Why do we even need to fire the timer in case of the first generation
> > hardware? Just send press and release events directly from the ISR.
That timer looks like a software debounce to prevent unexpected and
meaningless interrupt/event caused by quick press/release.   
> 
> Robin Gong proposed to move the code to imx_imx_snvs_check_for_events()
> to improve readability and unload the ISR.
> 
> But since I, eventually, couldn't use the existing handling in
> imx_imx_snvs_check_for_events(), I do see why you're asking.
> 
> I'll move the code to the ISR and submit a new patch.
> 
> Robin van der Gracht

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-09-16  7:45     ` Robin Gong
@ 2019-09-16 23:37       ` Dmitry Torokhov
  2019-11-20  9:27         ` Marco Felsch
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2019-09-16 23:37 UTC (permalink / raw)
  To: Robin Gong
  Cc: robin, linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, Pengutronix Kernel Team,
	Marco Felsch, Shawn Guo, Adam Ford,
	linux-arm-kernel @ lists . infradead . org

On Mon, Sep 16, 2019 at 07:45:37AM +0000, Robin Gong wrote:
> On 2019/9/13 15:31 robin <robin@protonic.nl> wrote:> 
> > Hi Dmitry,
> > 
> > On 2019-09-12 22:13, Dmitry Torokhov wrote:
> > > Hi Robin,
> > >
> > > On Wed, Sep 04, 2019 at 06:23:29AM +0000, Robin van der Gracht wrote:
> > >> The first generation i.MX6 processors does not send an interrupt when
> > >> the power key is pressed. It sends a power down request interrupt if
> > >> the key is released before a hard shutdown (5 second press). This
> > >> should allow software to bring down the SoC safely.
> > >>
> > >> For this driver to work as a regular power key with the older SoCs,
> > >> we need to send a keypress AND release when we get the power down
> > >> request irq.
> > >>
> > >> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> > >> ---
> > >> @@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int
> > >> irq, void *dev_id)  {
> > >>  	struct platform_device *pdev = dev_id;
> > >>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> > >> +	unsigned long expire = jiffies;
> > >>  	u32 lp_status;
> > >>
> > >>  	pm_wakeup_event(pdata->input->dev.parent, 0);
> > >>
> > >>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> > >> -	if (lp_status & SNVS_LPSR_SPO)
> > >> -		mod_timer(&pdata->check_timer, jiffies +
> > >> msecs_to_jiffies(DEBOUNCE_TIME));
> > >> +	if (lp_status & SNVS_LPSR_SPO) {
> > >> +		if (pdata->minor_rev > 0)
> > >> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
> > >> +		mod_timer(&pdata->check_timer, expire);
> > >
> > > Why do we even need to fire the timer in case of the first generation
> > > hardware? Just send press and release events directly from the ISR.
> That timer looks like a software debounce to prevent unexpected and
> meaningless interrupt/event caused by quick press/release.   

Right, but in case of the first generation hardware we schedule the
timer immediately (expire == 0) and do not check state of the hardware
in the timer handler, but rather simply emit down/up events, so we do
not really get any benefit from the timer (again, I am talking about
first generation hardware only).

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-09-16 23:37       ` Dmitry Torokhov
@ 2019-11-20  9:27         ` Marco Felsch
  2019-11-20 16:32           ` robin
  0 siblings, 1 reply; 11+ messages in thread
From: Marco Felsch @ 2019-11-20  9:27 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Robin Gong, robin, linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, Pengutronix Kernel Team,
	Shawn Guo, Adam Ford, linux-arm-kernel @ lists . infradead . org

Hi Robin,

On 19-09-16 16:37, Dmitry Torokhov wrote:
> On Mon, Sep 16, 2019 at 07:45:37AM +0000, Robin Gong wrote:
> > On 2019/9/13 15:31 robin <robin@protonic.nl> wrote:> 
> > > Hi Dmitry,
> > > 
> > > On 2019-09-12 22:13, Dmitry Torokhov wrote:
> > > > Hi Robin,
> > > >
> > > > On Wed, Sep 04, 2019 at 06:23:29AM +0000, Robin van der Gracht wrote:
> > > >> The first generation i.MX6 processors does not send an interrupt when
> > > >> the power key is pressed. It sends a power down request interrupt if
> > > >> the key is released before a hard shutdown (5 second press). This
> > > >> should allow software to bring down the SoC safely.
> > > >>
> > > >> For this driver to work as a regular power key with the older SoCs,
> > > >> we need to send a keypress AND release when we get the power down
> > > >> request irq.
> > > >>
> > > >> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> > > >> ---
> > > >> @@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int
> > > >> irq, void *dev_id)  {
> > > >>  	struct platform_device *pdev = dev_id;
> > > >>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> > > >> +	unsigned long expire = jiffies;
> > > >>  	u32 lp_status;
> > > >>
> > > >>  	pm_wakeup_event(pdata->input->dev.parent, 0);
> > > >>
> > > >>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> > > >> -	if (lp_status & SNVS_LPSR_SPO)
> > > >> -		mod_timer(&pdata->check_timer, jiffies +
> > > >> msecs_to_jiffies(DEBOUNCE_TIME));
> > > >> +	if (lp_status & SNVS_LPSR_SPO) {
> > > >> +		if (pdata->minor_rev > 0)
> > > >> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
> > > >> +		mod_timer(&pdata->check_timer, expire);
> > > >
> > > > Why do we even need to fire the timer in case of the first generation
> > > > hardware? Just send press and release events directly from the ISR.
> > That timer looks like a software debounce to prevent unexpected and
> > meaningless interrupt/event caused by quick press/release.   
> 
> Right, but in case of the first generation hardware we schedule the
> timer immediately (expire == 0) and do not check state of the hardware
> in the timer handler, but rather simply emit down/up events, so we do
> not really get any benefit from the timer (again, I am talking about
> first generation hardware only).

Did you prepared a v4? Just ask to avoid a duplicated work :)

Regards,
  Marco

> Thanks.
> 
> -- 
> Dmitry
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-11-20  9:27         ` Marco Felsch
@ 2019-11-20 16:32           ` robin
  2019-11-21 13:17             ` Robin Gong
  0 siblings, 1 reply; 11+ messages in thread
From: robin @ 2019-11-20 16:32 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Dmitry Torokhov, Robin Gong, linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, Pengutronix Kernel Team,
	Shawn Guo, Adam Ford, linux-arm-kernel @ lists . infradead . org

On 2019-11-20 10:27, Marco Felsch wrote:
> Hi Robin,
> 
> On 19-09-16 16:37, Dmitry Torokhov wrote:
>> On Mon, Sep 16, 2019 at 07:45:37AM +0000, Robin Gong wrote:
>> > On 2019/9/13 15:31 robin <robin@protonic.nl> wrote:>
>> > > Hi Dmitry,
>> > >
>> > > On 2019-09-12 22:13, Dmitry Torokhov wrote:
>> > > > Hi Robin,
>> > > >
>> > > > On Wed, Sep 04, 2019 at 06:23:29AM +0000, Robin van der Gracht wrote:
>> > > >> The first generation i.MX6 processors does not send an interrupt when
>> > > >> the power key is pressed. It sends a power down request interrupt if
>> > > >> the key is released before a hard shutdown (5 second press). This
>> > > >> should allow software to bring down the SoC safely.
>> > > >>
>> > > >> For this driver to work as a regular power key with the older SoCs,
>> > > >> we need to send a keypress AND release when we get the power down
>> > > >> request irq.
>> > > >>
>> > > >> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
>> > > >> ---
>> > > >> @@ -67,13 +83,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int
>> > > >> irq, void *dev_id)  {
>> > > >>  	struct platform_device *pdev = dev_id;
>> > > >>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
>> > > >> +	unsigned long expire = jiffies;
>> > > >>  	u32 lp_status;
>> > > >>
>> > > >>  	pm_wakeup_event(pdata->input->dev.parent, 0);
>> > > >>
>> > > >>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
>> > > >> -	if (lp_status & SNVS_LPSR_SPO)
>> > > >> -		mod_timer(&pdata->check_timer, jiffies +
>> > > >> msecs_to_jiffies(DEBOUNCE_TIME));
>> > > >> +	if (lp_status & SNVS_LPSR_SPO) {
>> > > >> +		if (pdata->minor_rev > 0)
>> > > >> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
>> > > >> +		mod_timer(&pdata->check_timer, expire);
>> > > >
>> > > > Why do we even need to fire the timer in case of the first generation
>> > > > hardware? Just send press and release events directly from the ISR.
>> > That timer looks like a software debounce to prevent unexpected and
>> > meaningless interrupt/event caused by quick press/release.
>> 
>> Right, but in case of the first generation hardware we schedule the
>> timer immediately (expire == 0) and do not check state of the hardware
>> in the timer handler, but rather simply emit down/up events, so we do
>> not really get any benefit from the timer (again, I am talking about
>> first generation hardware only).
> 
> Did you prepared a v4? Just ask to avoid a duplicated work :)

No I haven't. Not sure what the public wants. Use timer, don't use 
timer..

v3 has had long term testing though ;)

Regards,
Robin van der Gracht

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-11-20 16:32           ` robin
@ 2019-11-21 13:17             ` Robin Gong
  2019-11-22 22:48               ` Dmitry Torokhov
  0 siblings, 1 reply; 11+ messages in thread
From: Robin Gong @ 2019-11-21 13:17 UTC (permalink / raw)
  To: robin, Marco Felsch
  Cc: Dmitry Torokhov, linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, Pengutronix Kernel Team,
	Shawn Guo, Adam Ford, linux-arm-kernel @ lists . infradead . org


On 2019-11-21 0:33, robin <robin@protonic.nl> wrote:
> On 2019-11-20 10:27, Marco Felsch wrote:
> > Hi Robin,
> >
> > On 19-09-16 16:37, Dmitry Torokhov wrote:
> >> On Mon, Sep 16, 2019 at 07:45:37AM +0000, Robin Gong wrote:
> >> > On 2019/9/13 15:31 robin <robin@protonic.nl> wrote:>
> >> > > Hi Dmitry,
> >> > >
> >> > > On 2019-09-12 22:13, Dmitry Torokhov wrote:
> >> > > > Hi Robin,
> >> > > >
> >> > > > On Wed, Sep 04, 2019 at 06:23:29AM +0000, Robin van der Gracht
> wrote:
> >> > > >> The first generation i.MX6 processors does not send an
> >> > > >> interrupt when the power key is pressed. It sends a power down
> >> > > >> request interrupt if the key is released before a hard
> >> > > >> shutdown (5 second press). This should allow software to bring down
> the SoC safely.
> >> > > >>
> >> > > >> For this driver to work as a regular power key with the older
> >> > > >> SoCs, we need to send a keypress AND release when we get the
> >> > > >> power down request irq.
> >> > > >>
> >> > > >> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> >> > > >> ---
> >> > > >> @@ -67,13 +83,17 @@ static irqreturn_t
> >> > > >> imx_snvs_pwrkey_interrupt(int irq, void *dev_id)  {
> >> > > >>  	struct platform_device *pdev = dev_id;
> >> > > >>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> >> > > >> +	unsigned long expire = jiffies;
> >> > > >>  	u32 lp_status;
> >> > > >>
> >> > > >>  	pm_wakeup_event(pdata->input->dev.parent, 0);
> >> > > >>
> >> > > >>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> >> > > >> -	if (lp_status & SNVS_LPSR_SPO)
> >> > > >> -		mod_timer(&pdata->check_timer, jiffies +
> >> > > >> msecs_to_jiffies(DEBOUNCE_TIME));
> >> > > >> +	if (lp_status & SNVS_LPSR_SPO) {
> >> > > >> +		if (pdata->minor_rev > 0)
> >> > > >> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
> >> > > >> +		mod_timer(&pdata->check_timer, expire);
> >> > > >
> >> > > > Why do we even need to fire the timer in case of the first
> >> > > > generation hardware? Just send press and release events directly from
> the ISR.
> >> > That timer looks like a software debounce to prevent unexpected and
> >> > meaningless interrupt/event caused by quick press/release.
> >>
> >> Right, but in case of the first generation hardware we schedule the
> >> timer immediately (expire == 0) and do not check state of the
> >> hardware in the timer handler, but rather simply emit down/up events,
> >> so we do not really get any benefit from the timer (again, I am
> >> talking about first generation hardware only).
> >
> > Did you prepared a v4? Just ask to avoid a duplicated work :)
> 
> No I haven't. Not sure what the public wants. Use timer, don't use timer..
> 
> v3 has had long term testing though ;)
Sorry for that I miss the mail.
Understood a little bit confusion about immediate timer for
the first press, just stand on the view of clean code shape.
But I'm okay if you prefer to remove timer in the first interrupt
generation.
> 
> Regards,
> Robin van der Gracht

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q
  2019-11-21 13:17             ` Robin Gong
@ 2019-11-22 22:48               ` Dmitry Torokhov
  0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2019-11-22 22:48 UTC (permalink / raw)
  To: Robin Gong
  Cc: robin, Marco Felsch, linux-input @ vger . kernel . org,
	linux-kernel @ vger . kernel . org, Pengutronix Kernel Team,
	Shawn Guo, Adam Ford, linux-arm-kernel @ lists . infradead . org

On Thu, Nov 21, 2019 at 01:17:56PM +0000, Robin Gong wrote:
> 
> On 2019-11-21 0:33, robin <robin@protonic.nl> wrote:
> > On 2019-11-20 10:27, Marco Felsch wrote:
> > > Hi Robin,
> > >
> > > On 19-09-16 16:37, Dmitry Torokhov wrote:
> > >> On Mon, Sep 16, 2019 at 07:45:37AM +0000, Robin Gong wrote:
> > >> > On 2019/9/13 15:31 robin <robin@protonic.nl> wrote:>
> > >> > > Hi Dmitry,
> > >> > >
> > >> > > On 2019-09-12 22:13, Dmitry Torokhov wrote:
> > >> > > > Hi Robin,
> > >> > > >
> > >> > > > On Wed, Sep 04, 2019 at 06:23:29AM +0000, Robin van der Gracht
> > wrote:
> > >> > > >> The first generation i.MX6 processors does not send an
> > >> > > >> interrupt when the power key is pressed. It sends a power down
> > >> > > >> request interrupt if the key is released before a hard
> > >> > > >> shutdown (5 second press). This should allow software to bring down
> > the SoC safely.
> > >> > > >>
> > >> > > >> For this driver to work as a regular power key with the older
> > >> > > >> SoCs, we need to send a keypress AND release when we get the
> > >> > > >> power down request irq.
> > >> > > >>
> > >> > > >> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> > >> > > >> ---
> > >> > > >> @@ -67,13 +83,17 @@ static irqreturn_t
> > >> > > >> imx_snvs_pwrkey_interrupt(int irq, void *dev_id)  {
> > >> > > >>  	struct platform_device *pdev = dev_id;
> > >> > > >>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> > >> > > >> +	unsigned long expire = jiffies;
> > >> > > >>  	u32 lp_status;
> > >> > > >>
> > >> > > >>  	pm_wakeup_event(pdata->input->dev.parent, 0);
> > >> > > >>
> > >> > > >>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> > >> > > >> -	if (lp_status & SNVS_LPSR_SPO)
> > >> > > >> -		mod_timer(&pdata->check_timer, jiffies +
> > >> > > >> msecs_to_jiffies(DEBOUNCE_TIME));
> > >> > > >> +	if (lp_status & SNVS_LPSR_SPO) {
> > >> > > >> +		if (pdata->minor_rev > 0)
> > >> > > >> +			expire = jiffies + msecs_to_jiffies(DEBOUNCE_TIME);
> > >> > > >> +		mod_timer(&pdata->check_timer, expire);
> > >> > > >
> > >> > > > Why do we even need to fire the timer in case of the first
> > >> > > > generation hardware? Just send press and release events directly from
> > the ISR.
> > >> > That timer looks like a software debounce to prevent unexpected and
> > >> > meaningless interrupt/event caused by quick press/release.
> > >>
> > >> Right, but in case of the first generation hardware we schedule the
> > >> timer immediately (expire == 0) and do not check state of the
> > >> hardware in the timer handler, but rather simply emit down/up events,
> > >> so we do not really get any benefit from the timer (again, I am
> > >> talking about first generation hardware only).
> > >
> > > Did you prepared a v4? Just ask to avoid a duplicated work :)
> > 
> > No I haven't. Not sure what the public wants. Use timer, don't use timer..
> > 
> > v3 has had long term testing though ;)
> Sorry for that I miss the mail.
> Understood a little bit confusion about immediate timer for
> the first press, just stand on the view of clean code shape.
> But I'm okay if you prefer to remove timer in the first interrupt
> generation.

Yes, please prepare v4 without the timer for the first generation of the
hardware.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04  6:23 [PATCH v3] input: keyboard: snvs_pwrkey: Send key events for i.MX6 S, DL and Q Robin van der Gracht
2019-09-04  6:52 ` Marco Felsch
2019-09-13  7:39   ` robin
2019-09-12 20:13 ` Dmitry Torokhov
2019-09-13  7:30   ` robin
2019-09-16  7:45     ` Robin Gong
2019-09-16 23:37       ` Dmitry Torokhov
2019-11-20  9:27         ` Marco Felsch
2019-11-20 16:32           ` robin
2019-11-21 13:17             ` Robin Gong
2019-11-22 22:48               ` Dmitry Torokhov

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git