[3/3] gpio: tegra: Add support for gpio debounce

NVIDIA's Tegra210 support the HW debounce in the GPIO
controller for all its GPIO pins.

Add support for setting debounce timing by implementing the
set_debounce callback of gpiochip.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/gpio/gpio-tegra.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

On 04/18/2016 02:46 AM, Laxman Dewangan wrote:
> NVIDIA's Tegra210 support the HW debounce in the GPIO
> controller for all its GPIO pins.
>
> Add support for setting debounce timing by implementing the
> set_debounce callback of gpiochip.

> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c

> +static int tegra_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
> +				   unsigned int debounce)
> +{
> +	unsigned int max_dbc;
> +	unsigned int debounce_ms = DIV_ROUND_UP(debounce, 1000);
> +
> +	if (!debounce_ms) {
> +		tegra_gpio_mask_write(GPIO_MSK_DBC_EN(offset), offset, 0);
> +		return 0;
> +	}
> +
> +	debounce_ms = min(debounce_ms, 255U);
> +
> +	/* There is only one debounce count register per port and hence
> +	 * set the maximum of current and requested debounce time.
> +	 */
> +	max_dbc = tegra_gpio_readl(GPIO_DBC_CNT(offset));

What if the system boots with random values in that register, or some 
code that runs before the kernel programs large values into the 
register? That would (incorrectly) impose a lower bound on the possible 
values the kernel driver can impose. Perhaps the kernel should clear the 
DBC_CNT registers at probe(), or should store a shadow copy of the 
DBC_CNT register, use that value here rather than re-reading the 
registers, and clear that SW shadow at probe().

> +	max_dbc = max(max_dbc, debounce_ms);

I wonder if there should be more discussion of how to honor conflicting 
requests. Perhaps we should only allow exactly equal values (someone 
might strictly care about latency, and increasing the latency of GPIO X1 
just because GPIO X5 wanted a longer debounce period might not be 
acceptable). Does the GPIO subsystem define explicit semantics for this 
case?

> +	tegra_gpio_mask_write(GPIO_MSK_DBC_EN(offset), offset, 1);
> +	tegra_gpio_writel(max_dbc, GPIO_DBC_CNT(offset));

I think DBC_CNT should be written first; the debounce process uses that 
data to configure itself. The process shouldn't be enabled before it's 
configured.

> @@ -327,6 +358,9 @@ static int tegra_gpio_resume(struct device *dev)
>   			tegra_gpio_writel(bank->oe[p], GPIO_OE(gpio));
>   			tegra_gpio_writel(bank->int_lvl[p], GPIO_INT_LVL(gpio));
>   			tegra_gpio_writel(bank->int_enb[p], GPIO_INT_ENB(gpio));
> +			tegra_gpio_writel(bank->dbc_enb[p],
> +					  GPIO_MSK_DBC_EN(gpio));
> +			tegra_gpio_writel(bank->dbc_cnt[p], GPIO_DBC_CNT(gpio));

dbg_cnt should be restored before dbc_en, for the same reason as above.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Monday 18 April 2016 10:08 PM, Stephen Warren wrote:
> On 04/18/2016 02:46 AM, Laxman Dewangan wrote:
>
>>
>> +
>> +    /* There is only one debounce count register per port and hence
>> +     * set the maximum of current and requested debounce time.
>> +     */
>> +    max_dbc = tegra_gpio_readl(GPIO_DBC_CNT(offset));
>
> What if the system boots with random values in that register, or some 
> code that runs before the kernel programs large values into the 
> register? That would (incorrectly) impose a lower bound on the 
> possible values the kernel driver can impose. Perhaps the kernel 
> should clear the DBC_CNT registers at probe(), or should store a 
> shadow copy of the DBC_CNT register, use that value here rather than 
> re-reading the registers, and clear that SW shadow at probe().
>

Clearing in probe is better option than shadowing it. If we shadow then 
need loop as there is only one register per port which have 8 pins and 
this function get called per pin basis.




>> +    max_dbc = max(max_dbc, debounce_ms);
>
> I wonder if there should be more discussion of how to honor 
> conflicting requests. Perhaps we should only allow exactly equal 
> values (someone might strictly care about latency, and increasing the 
> latency of GPIO X1 just because GPIO X5 wanted a longer debounce 
> period might not be acceptable). Does the GPIO subsystem define 
> explicit semantics for this case?
>


Not seen form  GOIO subsystem as GOIO subsystem assume this 
configuration is per GPIO, not for group of GPIO.

However, everywhere, the debounce parameter should be provided as 
platform specific from DT and on this case, the platform developer knows 
what is best common value.


>> + tegra_gpio_mask_write(GPIO_MSK_DBC_EN(offset), offset, 1);
>> +    tegra_gpio_writel(max_dbc, GPIO_DBC_CNT(offset));
>
> I think DBC_CNT should be written first; the debounce process uses 
> that data to configure itself. The process shouldn't be enabled before 
> it's configured.
>

OK, make sense as safe game.
The debounce is in effect when any change in the pin and this call 
should be happen before any state change in pin.


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 04/18/2016 11:06 AM, Laxman Dewangan wrote:
>
> On Monday 18 April 2016 10:08 PM, Stephen Warren wrote:
>> On 04/18/2016 02:46 AM, Laxman Dewangan wrote:
>>
>>>
>>> +
>>> +    /* There is only one debounce count register per port and hence
>>> +     * set the maximum of current and requested debounce time.
>>> +     */
>>> +    max_dbc = tegra_gpio_readl(GPIO_DBC_CNT(offset));
>>
>> What if the system boots with random values in that register, or some
>> code that runs before the kernel programs large values into the
>> register? That would (incorrectly) impose a lower bound on the
>> possible values the kernel driver can impose. Perhaps the kernel
>> should clear the DBC_CNT registers at probe(), or should store a
>> shadow copy of the DBC_CNT register, use that value here rather than
>> re-reading the registers, and clear that SW shadow at probe().
>>
>
> Clearing in probe is better option than shadowing it.

Sounds fine.

 > If we shadow then
> need loop as there is only one register per port which have 8 pins and
> this function get called per pin basis.

Note that there is a per-bank data structure "struct tegra_gpio_bank" 
that could be used to store the data, so no need for any loops. You 
could just store an integer per port there, and do the same "max" 
algorithm as in the current patch, just on that variable. Still, just 
clearing the register sounds fine too.

>>> +    max_dbc = max(max_dbc, debounce_ms);
>>
>> I wonder if there should be more discussion of how to honor
>> conflicting requests. Perhaps we should only allow exactly equal
>> values (someone might strictly care about latency, and increasing the
>> latency of GPIO X1 just because GPIO X5 wanted a longer debounce
>> period might not be acceptable). Does the GPIO subsystem define
>> explicit semantics for this case?
>
> Not seen form  GOIO subsystem as GOIO subsystem assume this
> configuration is per GPIO, not for group of GPIO.
>
> However, everywhere, the debounce parameter should be provided as
> platform specific from DT and on this case, the platform developer knows
> what is best common value.

Hopefully true, yes:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html