Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: robin <robin@protonic.nl>
To: Robin Gong <yibin.gong@nxp.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	linux-input@vger.kernel.org, Adam Ford <aford173@gmail.com>,
	Fabio Estevam <festevam@gmail.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S,DL and Q
Date: Tue, 27 Aug 2019 09:55:18 +0200
Message-ID: <83c033a20f5f2e3ce15525a1efd072bb@protonic.nl> (raw)
In-Reply-To: <VE1PR04MB6638754916357F551502102589A00@VE1PR04MB6638.eurprd04.prod.outlook.com>

On 2019-08-27 08:17, Robin Gong wrote:
> On Fri, Aug 23, 2019 at 02:30:02PM +0200, Robin van der Gracht wrote:>
>> The older generation i.MX6 processors send a powerdown request 
>> interrupt
>> if the powerkey 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 powerkey with the older SoCs, we 
>> need to
>> send a keypress AND release when we get the powerdown request 
>> interrupt.
> Please clarify here more clearly that because there is NO press
> interrupt triggered
> but only release interrupt on elder i.mx6 processors and that HW issue
> fixed from
> i.mx6sx.

ACK

>> 
>> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
>> ---
>>  arch/arm/boot/dts/imx6qdl.dtsi       |  2 +-
>>  arch/arm/boot/dts/imx6sll.dtsi       |  2 +-
>>  arch/arm/boot/dts/imx6sx.dtsi        |  2 +-
>>  arch/arm/boot/dts/imx6ul.dtsi        |  2 +-
>>  arch/arm/boot/dts/imx7s.dtsi         |  2 +-
> As Shawn talked, please keep the original "fsl,sec-v4.0-pwrkey", just 
> add
> 'imx6qdl-snvs-pwrkey' for elder i.mx6 processor i.mx6q/dl/sl, thus no 
> need
> to touch other newer processor's dts.

ACK

> 
>> 
>>  static void imx_imx_snvs_check_for_events(struct timer_list *t) @@ 
>> -67,13
>> +85,23 @@ 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);
>> +	struct input_dev *input = pdata->input;
>>  	u32 lp_status;
>> 
>> -	pm_wakeup_event(pdata->input->dev.parent, 0);
>> +	pm_wakeup_event(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->hwtype == IMX6QDL_SNVS) {
>> +			input_report_key(input, pdata->keycode, 1);
>> +			input_report_key(input, pdata->keycode, 0);
>> +			input_sync(input);
>> +			pm_relax(input->dev.parent);
> Could you move the above input event report steps into
> imx_imx_snvs_check_for_events()
> as before? That make code better to understand and less operation in 
> ISR.

I placed it here to avoid the unnessesairy debounce delay (since thats 
already
implemented in hardware).

I do agree with your arguments so I'll move emitting the events to
imx_imx_snvs_check_for_events().

Is it ok if I keep the conditional, but instead of emitting the events,
schedule imx_imx_snvs_check_for_events() immidiatly to avoid the 
debounce,
or should I choose clarity over the 30 ms delay?

>> +		} else {
>> +			mod_timer(&pdata->check_timer,
>> +				jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
>> +		}
>> +	}
>> 
>>  	/* clear SPO status */
>>  	regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO); @@
>> -88,11 +116,24 @@ static void imx_snvs_pwrkey_act(void *pdata)
>>  	del_timer_sync(&pd->check_timer);
>>  }
>> 
>> +static const struct of_device_id imx_snvs_pwrkey_ids[] = {
>> +	{
>> +		.compatible = "fsl,imx6sx-sec-v4.0-pwrkey",
>> +		.data = &imx_snvs_devtype[IMX6SX_SNVS],
>> +	}, {
>> +		.compatible = "fsl,imx6qdl-sec-v4.0-pwrkey",
>> +		.data = &imx_snvs_devtype[IMX6QDL_SNVS],
> No ' IMX6QDL_SNVS ' defined in your patch or am I missing?

I added an enum 'imx_snvs_hwtype' that defines both IMX6SX_SNVS and 
IMX6QDL_SNVS.

>> +	},
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
>> --
>> 2.20.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

      reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-23 12:30 [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S, DL " Robin van der Gracht
2019-08-25  7:10 ` [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S,DL " Shawn Guo
2019-08-27  6:17 ` Robin Gong
2019-08-27  7:55   ` robin [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=83c033a20f5f2e3ce15525a1efd072bb@protonic.nl \
    --to=robin@protonic.nl \
    --cc=aford173@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=festevam@gmail.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=yibin.gong@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.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-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


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