Re: [PATCH v2] pinctrl: amd: print debounce filter info in debugfs

[Coiby Xu]

On Mon, Oct 26, 2020 at 05:22:45PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 26, 2020 at 5:16 PM Coiby Xu <coiby.xu@xxxxxxxxx> wrote:
> > Print the status of debounce filter as follows,
> > $ cat /sys/kernel/debug/gpio
> > pin129          interrupt is disabled| interrupt is masked| disable wakeup in S0i3 state| disable wakeup in S3 state|
> >  disable wakeup in S4/S5 state| input is high|   pull-up is disabled| Pull-down is disabled|   output is disabled| debouncing filter disabled|   0x50000
> > pin130          interrupt is disabled| interrupt is masked| disable wakeup in S0i3 state| disable wakeup in S3 state|
> >  disable wakeup in S4/S5 state| input is high|   pull-up is disabled| Pull-down is disabled|   output is disabled| debouncing filter (high) enabled| debouncing timeout is 124800 (us)| 0x503c8
> >                                                                                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Thanks for an update!
> In general looks good, one nit below (sorry, missed it in v1 round)
Thank you for the feedbacks!
> ...
>
> > +       char debounce_value[40];
>
> (1)
>
> ...
>
> > +                               if (tmr_large) {
> > +                                       if (tmr_out_unit)
> > +                                               unit = 62500;
> > +                                       else
>
> > +                                               unit = 15600;
>
> Side note: Hmm... Shouldn't be 15625? As 1/4.

Thank you for discovering the inconsistency! I wrote these code based on
amd_gpio_set_debounce. I'll send an email to the original author to
confirm it.

static int amd_gpio_set_debounce(struct gpio_chip *gc, unsigned offset,
		unsigned debounce)
{
    ...
	if (debounce) {
		pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF;
		pin_reg &= ~DB_TMR_OUT_MASK;
		/*
		Debounce	Debounce	Timer	Max
		TmrLarge	TmrOutUnit	Unit	Debounce
							Time
		0	0	61 usec (2 RtcClk)	976 usec
		0	1	244 usec (8 RtcClk)	3.9 msec
		1	0	15.6 msec (512 RtcClk)	250 msec
		1	1	62.5 msec (2048 RtcClk)	1 sec
		*/

		if (debounce < 61) {
			pin_reg |= 1;
			pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
			pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
		} else if (debounce < 976) {
			time = debounce / 61;
			pin_reg |= time & DB_TMR_OUT_MASK;
			pin_reg &= ~BIT(DB_TMR_OUT_UNIT_OFF);
			pin_reg &= ~BIT(DB_TMR_LARGE_OFF);
        ...
> > +                               } else {
> > +                                       if (tmr_out_unit)
> > +                                               unit = 244;
> > +                                       else
> > +                                               unit = 61;
>
> ...
>
>
> > +                               snprintf(debounce_value, 40,
> > +                                        "debouncing timeout is %u (us)|", time * unit);
>
> (2)
>
> ...
>
> > +                               snprintf(debounce_value, 40, " ");
>
> (3)
>
> Because of definition (1) can you in (2) and (3) use sizeof() ?
I've considered defining a constant. Obviously sizeof is a better
idea:)

> --
> With Best Regards,
> Andy Shevchenko

--
Best regards,
Coiby