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

[robin]

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