[PATCH 1/3] gpio: tegra: Don't open code of_device_get_match_data()

[PATCH 1/3] gpio: tegra: Don't open code of_device_get_match_data()

[Laxman Dewangan]

Use of_device_get_match_data() for getting matched data
instead of implementing this locally.

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpio/gpio-tegra.c | 50 +++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 790bb11..1b0c497 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -75,6 +75,11 @@ struct tegra_gpio_bank {
 #endif
 };
 
+struct tegra_gpio_soc_config {
+	u32 bank_stride;
+	u32 upper_offset;
+};
+
 static struct device *dev;
 static struct irq_domain *irq_domain;
 static void __iomem *regs;
@@ -445,27 +450,6 @@ static const struct dev_pm_ops tegra_gpio_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(tegra_gpio_suspend, tegra_gpio_resume)
 };
 
-struct tegra_gpio_soc_config {
-	u32 bank_stride;
-	u32 upper_offset;
-};
-
-static struct tegra_gpio_soc_config tegra20_gpio_config = {
-	.bank_stride = 0x80,
-	.upper_offset = 0x800,
-};
-
-static struct tegra_gpio_soc_config tegra30_gpio_config = {
-	.bank_stride = 0x100,
-	.upper_offset = 0x80,
-};
-
-static const struct of_device_id tegra_gpio_of_match[] = {
-	{ .compatible = "nvidia,tegra30-gpio", .data = &tegra30_gpio_config },
-	{ .compatible = "nvidia,tegra20-gpio", .data = &tegra20_gpio_config },
-	{ },
-};
-
 /* This lock class tells lockdep that GPIO irqs are in a different
  * category than their parents, so it won't report false recursion.
  */
@@ -473,8 +457,7 @@ static struct lock_class_key gpio_lock_class;
 
 static int tegra_gpio_probe(struct platform_device *pdev)
 {
-	const struct of_device_id *match;
-	struct tegra_gpio_soc_config *config;
+	const struct tegra_gpio_soc_config *config;
 	struct resource *res;
 	struct tegra_gpio_bank *bank;
 	int ret;
@@ -484,12 +467,11 @@ static int tegra_gpio_probe(struct platform_device *pdev)
 
 	dev = &pdev->dev;
 
-	match = of_match_device(tegra_gpio_of_match, &pdev->dev);
-	if (!match) {
+	config = of_device_get_match_data(&pdev->dev);
+	if (!config) {
 		dev_err(&pdev->dev, "Error: No device match found\n");
 		return -ENODEV;
 	}
-	config = (struct tegra_gpio_soc_config *)match->data;
 
 	tegra_gpio_bank_stride = config->bank_stride;
 	tegra_gpio_upper_offset = config->upper_offset;
@@ -578,6 +560,22 @@ static int tegra_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static struct tegra_gpio_soc_config tegra20_gpio_config = {
+	.bank_stride = 0x80,
+	.upper_offset = 0x800,
+};
+
+static struct tegra_gpio_soc_config tegra30_gpio_config = {
+	.bank_stride = 0x100,
+	.upper_offset = 0x80,
+};
+
+static const struct of_device_id tegra_gpio_of_match[] = {
+	{ .compatible = "nvidia,tegra30-gpio", .data = &tegra30_gpio_config },
+	{ .compatible = "nvidia,tegra20-gpio", .data = &tegra20_gpio_config },
+	{ },
+};
+
 static struct platform_driver tegra_gpio_driver = {
 	.driver		= {
 		.name	= "tegra-gpio",
-- 
2.1.4

[PATCH 2/3] gpio: tegra: Remove the need of keeping device handle for gpio driver

[Laxman Dewangan]

Remove the file static device handle variable as this is just
required for prints. The required handle can be stored in
tegra_gpio_chip and hence it become redundancy.

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

diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 1b0c497..de022a9 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -80,7 +80,6 @@ struct tegra_gpio_soc_config {
 	u32 upper_offset;
 };
 
-static struct device *dev;
 static struct irq_domain *irq_domain;
 static void __iomem *regs;
 static u32 tegra_gpio_bank_count;
@@ -240,7 +239,8 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 	ret = gpiochip_lock_as_irq(&tegra_gpio_chip, gpio);
 	if (ret) {
-		dev_err(dev, "unable to lock Tegra GPIO %d as IRQ\n", gpio);
+		dev_err(tegra_gpio_chip.parent,
+			"unable to lock Tegra GPIO %d as IRQ\n", gpio);
 		return ret;
 	}
 
@@ -465,8 +465,6 @@ static int tegra_gpio_probe(struct platform_device *pdev)
 	int i;
 	int j;
 
-	dev = &pdev->dev;
-
 	config = of_device_get_match_data(&pdev->dev);
 	if (!config) {
 		dev_err(&pdev->dev, "Error: No device match found\n");
@@ -488,6 +486,8 @@ static int tegra_gpio_probe(struct platform_device *pdev)
 	}
 
 	tegra_gpio_chip.ngpio = tegra_gpio_bank_count * 32;
+	tegra_gpio_chip.parent = &pdev->dev;
+
 
 	tegra_gpio_banks = devm_kzalloc(&pdev->dev,
 			tegra_gpio_bank_count * sizeof(*tegra_gpio_banks),
-- 
2.1.4

Re: [PATCH 2/3] gpio: tegra: Remove the need of keeping device handle for gpio driver

[Stephen Warren]

On 04/18/2016 02:46 AM, Laxman Dewangan wrote:
> Remove the file static device handle variable as this is just
> required for prints. The required handle can be stored in
> tegra_gpio_chip and hence it become redundancy.

This seems fine as far as it goes, but if it's worth doing this, please 
move all the globals into the GPIO chip rather than just one of the 7 
globals.

Re: [PATCH 2/3] gpio: tegra: Remove the need of keeping device handle for gpio driver

[Laxman Dewangan]

On Monday 18 April 2016 09:59 PM, Stephen Warren wrote:
> On 04/18/2016 02:46 AM, Laxman Dewangan wrote:
>> Remove the file static device handle variable as this is just
>> required for prints. The required handle can be stored in
>> tegra_gpio_chip and hence it become redundancy.
>
> This seems fine as far as it goes, but if it's worth doing this, 
> please move all the globals into the GPIO chip rather than just one of 
> the 7 globals.

the device pointer is part of the gpiochip and so it is better to use 
gpiochip parent member instead of locally duplicating.

However, moving to other global variables needs some major changes and I 
think it should be treated as independent of this patch.
This patch just utilizes the gpiochip.parent here.

Re: [PATCH 2/3] gpio: tegra: Remove the need of keeping device handle for gpio driver

[Stephen Warren]

On 04/18/2016 11:00 AM, Laxman Dewangan wrote:
>
> On Monday 18 April 2016 09:59 PM, Stephen Warren wrote:
>> On 04/18/2016 02:46 AM, Laxman Dewangan wrote:
>>> Remove the file static device handle variable as this is just
>>> required for prints. The required handle can be stored in
>>> tegra_gpio_chip and hence it become redundancy.
>>
>> This seems fine as far as it goes, but if it's worth doing this,
>> please move all the globals into the GPIO chip rather than just one of
>> the 7 globals.
>
> the device pointer is part of the gpiochip and so it is better to use
> gpiochip parent member instead of locally duplicating.
>
> However, moving to other global variables needs some major changes and I
> think it should be treated as independent of this patch.
> This patch just utilizes the gpiochip.parent here.

Looking at the patch this just trades using one global (dev) for another 
(tegra_gpio_chip), so when the other globals are removed, you'll need to 
go back and change tegra_gpio_irq_set_type() again to remove use of the 
global tegra_gpio_chip.

Still, this /does/ remove one global so I guess it's OK. I don't feel 
terribly strongly, especially if you're going to send more patches soon 
to remove the other globals. I'll leave the call to Linus.

Re: [PATCH 2/3] gpio: tegra: Remove the need of keeping device handle for gpio driver

[Alexandre Courbot]

On Mon, Apr 18, 2016 at 5:46 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> Remove the file static device handle variable as this is just
> required for prints. The required handle can be stored in
> tegra_gpio_chip and hence it become redundancy.

Small but still worthy change. "dev" in the file's global namespace is
scary and prone to conflict with local variables declarations.

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Now I hope you will take care of "regs" and the other static variables
as Stephen rightfully suggested. :)

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

[Laxman Dewangan]

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(+)

diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index de022a9..9f7d75b 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -46,6 +46,7 @@
 #define GPIO_INT_ENB(x)		(GPIO_REG(x) + 0x50)
 #define GPIO_INT_LVL(x)		(GPIO_REG(x) + 0x60)
 #define GPIO_INT_CLR(x)		(GPIO_REG(x) + 0x70)
+#define GPIO_DBC_CNT(x)		(GPIO_REG(x) + 0xF0)
 
 #define GPIO_MSK_CNF(x)		(GPIO_REG(x) + tegra_gpio_upper_offset + 0x00)
 #define GPIO_MSK_OE(x)		(GPIO_REG(x) + tegra_gpio_upper_offset + 0x10)
@@ -53,6 +54,7 @@
 #define GPIO_MSK_INT_STA(x)	(GPIO_REG(x) + tegra_gpio_upper_offset + 0x40)
 #define GPIO_MSK_INT_ENB(x)	(GPIO_REG(x) + tegra_gpio_upper_offset + 0x50)
 #define GPIO_MSK_INT_LVL(x)	(GPIO_REG(x) + tegra_gpio_upper_offset + 0x60)
+#define GPIO_MSK_DBC_EN(x)	(GPIO_REG(x) + tegra_gpio_upper_offset + 0x30)
 
 #define GPIO_INT_LVL_MASK		0x010101
 #define GPIO_INT_LVL_EDGE_RISING	0x000101
@@ -72,12 +74,15 @@ struct tegra_gpio_bank {
 	u32 int_enb[4];
 	u32 int_lvl[4];
 	u32 wake_enb[4];
+	u32 dbc_enb[4];
+	u32 dbc_cnt[4];
 #endif
 };
 
 struct tegra_gpio_soc_config {
 	u32 bank_stride;
 	u32 upper_offset;
+	bool debounce_supported;
 };
 
 static struct irq_domain *irq_domain;
@@ -164,6 +169,31 @@ static int tegra_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
 	return 0;
 }
 
+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));
+	max_dbc = max(max_dbc, debounce_ms);
+
+	tegra_gpio_mask_write(GPIO_MSK_DBC_EN(offset), offset, 1);
+	tegra_gpio_writel(max_dbc, GPIO_DBC_CNT(offset));
+
+	return 0;
+}
+
 static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	return irq_find_mapping(irq_domain, offset);
@@ -177,6 +207,7 @@ static struct gpio_chip tegra_gpio_chip = {
 	.get			= tegra_gpio_get,
 	.direction_output	= tegra_gpio_direction_output,
 	.set			= tegra_gpio_set,
+	.set_debounce		= tegra_gpio_set_debounce,
 	.to_irq			= tegra_gpio_to_irq,
 	.base			= 0,
 };
@@ -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));
 		}
 	}
 
@@ -351,6 +385,11 @@ static int tegra_gpio_suspend(struct device *dev)
 			bank->oe[p] = tegra_gpio_readl(GPIO_OE(gpio));
 			bank->int_enb[p] = tegra_gpio_readl(GPIO_INT_ENB(gpio));
 			bank->int_lvl[p] = tegra_gpio_readl(GPIO_INT_LVL(gpio));
+			bank->dbc_enb[p] = tegra_gpio_readl(
+							GPIO_MSK_DBC_EN(gpio));
+			bank->dbc_enb[p] = (bank->dbc_enb[p] << 8) ||
+						bank->dbc_enb[p];
+			bank->dbc_cnt[p] = tegra_gpio_readl(GPIO_DBC_CNT(gpio));
 
 			/* Enable gpio irq for wake up source */
 			tegra_gpio_writel(bank->wake_enb[p],
@@ -473,6 +512,8 @@ static int tegra_gpio_probe(struct platform_device *pdev)
 
 	tegra_gpio_bank_stride = config->bank_stride;
 	tegra_gpio_upper_offset = config->upper_offset;
+	if (!config->debounce_supported)
+		tegra_gpio_chip.set_debounce = NULL;
 
 	for (;;) {
 		res = platform_get_resource(pdev, IORESOURCE_IRQ, tegra_gpio_bank_count);
@@ -570,7 +611,14 @@ static struct tegra_gpio_soc_config tegra30_gpio_config = {
 	.upper_offset = 0x80,
 };
 
+static struct tegra_gpio_soc_config tegra210_gpio_config = {
+	.bank_stride = 0x100,
+	.upper_offset = 0x80,
+	.debounce_supported = true,
+};
+
 static const struct of_device_id tegra_gpio_of_match[] = {
+	{ .compatible = "nvidia,tegra210-gpio", .data = &tegra210_gpio_config },
 	{ .compatible = "nvidia,tegra30-gpio", .data = &tegra30_gpio_config },
 	{ .compatible = "nvidia,tegra20-gpio", .data = &tegra20_gpio_config },
 	{ },
-- 
2.1.4

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

[Stephen Warren]

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.

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

[Laxman Dewangan]

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.

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

[Stephen Warren]

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:-)

Re: [PATCH 1/3] gpio: tegra: Don't open code of_device_get_match_data()

[Stephen Warren]

On 04/18/2016 02:46 AM, Laxman Dewangan wrote:
> Use of_device_get_match_data() for getting matched data
> instead of implementing this locally.

This patch,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

Re: [PATCH 1/3] gpio: tegra: Don't open code of_device_get_match_data()

[Alexandre Courbot]

On Mon, Apr 18, 2016 at 5:46 PM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Use of_device_get_match_data() for getting matched data
> instead of implementing this locally.

Maybe this is moving things around more than needed, but the end
result is arguably nicer.

Reviewed-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>