Re: [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S,DL and Q

[robin <robin@protonic.nl>]

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