Use of_device_get_match_data() for getting matched data
instead of implementing this locally.
Signed-off-by: Laxman Dewangan <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
Reviewed-by: Alexandre Courbot <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
Collected reviewed/ack by from Stephen, Alexandre and Thieery.
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
The data member of the of_device_id is the constant type
and hence all static structure which is used for this
initialisation as static.
Signed-off-by: Laxman Dewangan <[email protected]>
Suggested-by: Thierry Reding <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
---
Changes from V2:
-This is new in series based on discussion on previous patches.
Changes from V3:
- Collected RB of Stephen.
drivers/gpio/gpio-tegra.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 1b0c497..cd69422 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -560,12 +560,12 @@ static int tegra_gpio_probe(struct platform_device *pdev)
return 0;
}
-static struct tegra_gpio_soc_config tegra20_gpio_config = {
+static const struct tegra_gpio_soc_config tegra20_gpio_config = {
.bank_stride = 0x80,
.upper_offset = 0x800,
};
-static struct tegra_gpio_soc_config tegra30_gpio_config = {
+static const struct tegra_gpio_soc_config tegra30_gpio_config = {
.bank_stride = 0x100,
.upper_offset = 0x80,
};
--
2.1.4
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 <[email protected]>
---
Changes from V1:
- Write debounce count before enable.
- Make sure the debounce count do not have any boot residuals.
Changes from V2:
- Only access register for debounce when SoC support debounce.
Changes from V3:
- Add locking mechanism in debounce count register update.
- Move DBC register from prev patch to here.
drivers/gpio/gpio-tegra.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 68 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 6af5eb2..45d80ec 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -46,10 +46,13 @@
#define GPIO_INT_ENB(t, x) (GPIO_REG(t, x) + 0x50)
#define GPIO_INT_LVL(t, x) (GPIO_REG(t, x) + 0x60)
#define GPIO_INT_CLR(t, x) (GPIO_REG(t, x) + 0x70)
+#define GPIO_DBC_CNT(t, x) (GPIO_REG(t, x) + 0xF0)
+
#define GPIO_MSK_CNF(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x00)
#define GPIO_MSK_OE(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x10)
#define GPIO_MSK_OUT(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0X20)
+#define GPIO_MSK_DBC_EN(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x30)
#define GPIO_MSK_INT_STA(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x40)
#define GPIO_MSK_INT_ENB(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x50)
#define GPIO_MSK_INT_LVL(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x60)
@@ -67,6 +70,7 @@ struct tegra_gpio_bank {
int bank;
int irq;
spinlock_t lvl_lock[4];
+ spinlock_t dbc_lock[4]; /* Lock for updating debounce count register */
#ifdef CONFIG_PM_SLEEP
u32 cnf[4];
u32 out[4];
@@ -74,11 +78,14 @@ struct tegra_gpio_bank {
u32 int_enb[4];
u32 int_lvl[4];
u32 wake_enb[4];
+ u32 dbc_enb[4];
#endif
+ u32 dbc_cnt[4];
struct tegra_gpio_info *tgi;
};
struct tegra_gpio_soc_config {
+ bool debounce_supported;
u32 bank_stride;
u32 upper_offset;
};
@@ -182,6 +189,38 @@ 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)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+ unsigned int debounce_ms = DIV_ROUND_UP(debounce, 1000);
+ unsigned long flags;
+ int port = GPIO_PORT(offset);
+ int bank = GPIO_BANK(offset);
+
+ if (!debounce_ms) {
+ tegra_gpio_mask_write(tgi, GPIO_MSK_DBC_EN(tgi, 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.
+ */
+ spin_lock_irqsave(&tgi->bank_info[bank].dbc_lock[port], flags);
+ if (tgi->bank_info[bank].dbc_cnt[port] < debounce_ms) {
+ tegra_gpio_writel(tgi, debounce_ms, GPIO_DBC_CNT(tgi, offset));
+ tgi->bank_info[bank].dbc_cnt[port] = debounce_ms;
+ }
+ spin_unlock_irqrestore(&tgi->bank_info[bank].dbc_lock[port], flags);
+
+ tegra_gpio_mask_write(tgi, GPIO_MSK_DBC_EN(tgi, offset), offset, 1);
+
+ return 0;
+}
+
static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
{
struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
@@ -197,6 +236,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,
};
@@ -360,6 +400,14 @@ static int tegra_gpio_resume(struct device *dev)
unsigned int gpio = (b<<5) | (p<<3);
tegra_gpio_writel(tgi, bank->cnf[p],
GPIO_CNF(tgi, gpio));
+
+ if (tgi->soc->debounce_supported) {
+ tegra_gpio_writel(tgi, bank->dbc_cnt[p],
+ GPIO_DBC_CNT(tgi, gpio));
+ tegra_gpio_writel(tgi, bank->dbc_enb[p],
+ GPIO_MSK_DBC_EN(tgi, gpio));
+ }
+
tegra_gpio_writel(tgi, bank->out[p],
GPIO_OUT(tgi, gpio));
tegra_gpio_writel(tgi, bank->oe[p],
@@ -395,6 +443,13 @@ static int tegra_gpio_suspend(struct device *dev)
GPIO_OUT(tgi, gpio));
bank->oe[p] = tegra_gpio_readl(tgi,
GPIO_OE(tgi, gpio));
+ if (tgi->soc->debounce_supported) {
+ bank->dbc_enb[p] = tegra_gpio_readl(tgi,
+ GPIO_MSK_DBC_EN(tgi, gpio));
+ bank->dbc_enb[p] = (bank->dbc_enb[p] << 8) |
+ bank->dbc_enb[p];
+ }
+
bank->int_enb[p] = tegra_gpio_readl(tgi,
GPIO_INT_ENB(tgi, gpio));
bank->int_lvl[p] = tegra_gpio_readl(tgi,
@@ -547,6 +602,9 @@ static int tegra_gpio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, tgi);
+ if (!config->debounce_supported)
+ tgi->gc->set_debounce = NULL;
+
tgi->bank_info = devm_kzalloc(&pdev->dev, tgi->bank_count *
sizeof(*tgi->bank_info), GFP_KERNEL);
if (!tgi->bank_info)
@@ -607,8 +665,10 @@ static int tegra_gpio_probe(struct platform_device *pdev)
irq_set_chained_handler_and_data(bank->irq,
tegra_gpio_irq_handler, bank);
- for (j = 0; j < 4; j++)
+ for (j = 0; j < 4; j++) {
spin_lock_init(&bank->lvl_lock[j]);
+ spin_lock_init(&bank->dbc_lock[j]);
+ }
}
tegra_gpio_debuginit(tgi);
@@ -626,7 +686,14 @@ static const struct tegra_gpio_soc_config tegra30_gpio_config = {
.upper_offset = 0x80,
};
+static const struct tegra_gpio_soc_config tegra210_gpio_config = {
+ .debounce_supported = true,
+ .bank_stride = 0x100,
+ .upper_offset = 0x80,
+};
+
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
Move the file scoped multiple global variable from Tegra GPIO
driver to the structure and make this as gpiochip data which
can be referred from GPIO chip callbacks.
Signed-off-by: Laxman Dewangan <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
---
This patch is reworked on top of earlier patch
gpio: tegra: Remove the need of keeping device handle for gpio driver
There was review comment that we should get for all variable and hence
this is outcome of the discussion.
Changes from V3:
- Remove DBC/EN registers.
- Remove non-required new lines.
- Collected RB from Stephen.
drivers/gpio/gpio-tegra.c | 296 +++++++++++++++++++++++++++-------------------
1 file changed, 176 insertions(+), 120 deletions(-)
diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index cd69422..6af5eb2 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -35,24 +35,24 @@
#define GPIO_PORT(x) (((x) >> 3) & 0x3)
#define GPIO_BIT(x) ((x) & 0x7)
-#define GPIO_REG(x) (GPIO_BANK(x) * tegra_gpio_bank_stride + \
+#define GPIO_REG(tgi, x) (GPIO_BANK(x) * tgi->soc->bank_stride + \
GPIO_PORT(x) * 4)
-#define GPIO_CNF(x) (GPIO_REG(x) + 0x00)
-#define GPIO_OE(x) (GPIO_REG(x) + 0x10)
-#define GPIO_OUT(x) (GPIO_REG(x) + 0X20)
-#define GPIO_IN(x) (GPIO_REG(x) + 0x30)
-#define GPIO_INT_STA(x) (GPIO_REG(x) + 0x40)
-#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_MSK_CNF(x) (GPIO_REG(x) + tegra_gpio_upper_offset + 0x00)
-#define GPIO_MSK_OE(x) (GPIO_REG(x) + tegra_gpio_upper_offset + 0x10)
-#define GPIO_MSK_OUT(x) (GPIO_REG(x) + tegra_gpio_upper_offset + 0X20)
-#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_CNF(t, x) (GPIO_REG(t, x) + 0x00)
+#define GPIO_OE(t, x) (GPIO_REG(t, x) + 0x10)
+#define GPIO_OUT(t, x) (GPIO_REG(t, x) + 0X20)
+#define GPIO_IN(t, x) (GPIO_REG(t, x) + 0x30)
+#define GPIO_INT_STA(t, x) (GPIO_REG(t, x) + 0x40)
+#define GPIO_INT_ENB(t, x) (GPIO_REG(t, x) + 0x50)
+#define GPIO_INT_LVL(t, x) (GPIO_REG(t, x) + 0x60)
+#define GPIO_INT_CLR(t, x) (GPIO_REG(t, x) + 0x70)
+
+#define GPIO_MSK_CNF(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x00)
+#define GPIO_MSK_OE(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x10)
+#define GPIO_MSK_OUT(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0X20)
+#define GPIO_MSK_INT_STA(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x40)
+#define GPIO_MSK_INT_ENB(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x50)
+#define GPIO_MSK_INT_LVL(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x60)
#define GPIO_INT_LVL_MASK 0x010101
#define GPIO_INT_LVL_EDGE_RISING 0x000101
@@ -61,6 +61,8 @@
#define GPIO_INT_LVL_LEVEL_HIGH 0x000001
#define GPIO_INT_LVL_LEVEL_LOW 0x000000
+struct tegra_gpio_info;
+
struct tegra_gpio_bank {
int bank;
int irq;
@@ -73,6 +75,7 @@ struct tegra_gpio_bank {
u32 int_lvl[4];
u32 wake_enb[4];
#endif
+ struct tegra_gpio_info *tgi;
};
struct tegra_gpio_soc_config {
@@ -80,22 +83,25 @@ 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;
-static u32 tegra_gpio_bank_stride;
-static u32 tegra_gpio_upper_offset;
-static struct tegra_gpio_bank *tegra_gpio_banks;
+struct tegra_gpio_info {
+ struct device *dev;
+ void __iomem *regs;
+ struct irq_domain *irq_domain;
+ struct tegra_gpio_bank *bank_info;
+ const struct tegra_gpio_soc_config *soc;
+ struct gpio_chip *gc;
+ u32 bank_count;
+};
-static inline void tegra_gpio_writel(u32 val, u32 reg)
+static inline void tegra_gpio_writel(struct tegra_gpio_info *tgi,
+ u32 val, u32 reg)
{
- __raw_writel(val, regs + reg);
+ __raw_writel(val, tgi->regs + reg);
}
-static inline u32 tegra_gpio_readl(u32 reg)
+static inline u32 tegra_gpio_readl(struct tegra_gpio_info *tgi, u32 reg)
{
- return __raw_readl(regs + reg);
+ return __raw_readl(tgi->regs + reg);
}
static int tegra_gpio_compose(int bank, int port, int bit)
@@ -103,24 +109,25 @@ static int tegra_gpio_compose(int bank, int port, int bit)
return (bank << 5) | ((port & 0x3) << 3) | (bit & 0x7);
}
-static void tegra_gpio_mask_write(u32 reg, int gpio, int value)
+static void tegra_gpio_mask_write(struct tegra_gpio_info *tgi, u32 reg,
+ int gpio, int value)
{
u32 val;
val = 0x100 << GPIO_BIT(gpio);
if (value)
val |= 1 << GPIO_BIT(gpio);
- tegra_gpio_writel(val, reg);
+ tegra_gpio_writel(tgi, val, reg);
}
-static void tegra_gpio_enable(int gpio)
+static void tegra_gpio_enable(struct tegra_gpio_info *tgi, int gpio)
{
- tegra_gpio_mask_write(GPIO_MSK_CNF(gpio), gpio, 1);
+ tegra_gpio_mask_write(tgi, GPIO_MSK_CNF(tgi, gpio), gpio, 1);
}
-static void tegra_gpio_disable(int gpio)
+static void tegra_gpio_disable(struct tegra_gpio_info *tgi, int gpio)
{
- tegra_gpio_mask_write(GPIO_MSK_CNF(gpio), gpio, 0);
+ tegra_gpio_mask_write(tgi, GPIO_MSK_CNF(tgi, gpio), gpio, 0);
}
static int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
@@ -130,44 +137,56 @@ static int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
static void tegra_gpio_free(struct gpio_chip *chip, unsigned offset)
{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
pinctrl_free_gpio(offset);
- tegra_gpio_disable(offset);
+ tegra_gpio_disable(tgi, offset);
}
static void tegra_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
{
- tegra_gpio_mask_write(GPIO_MSK_OUT(offset), offset, value);
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+ tegra_gpio_mask_write(tgi, GPIO_MSK_OUT(tgi, offset), offset, value);
}
static int tegra_gpio_get(struct gpio_chip *chip, unsigned offset)
{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+ int bval = BIT(GPIO_BIT(offset));
+
/* If gpio is in output mode then read from the out value */
- if ((tegra_gpio_readl(GPIO_OE(offset)) >> GPIO_BIT(offset)) & 1)
- return (tegra_gpio_readl(GPIO_OUT(offset)) >>
- GPIO_BIT(offset)) & 0x1;
+ if (tegra_gpio_readl(tgi, GPIO_OE(tgi, offset)) & bval)
+ return !!(tegra_gpio_readl(tgi, GPIO_OUT(tgi, offset)) & bval);
- return (tegra_gpio_readl(GPIO_IN(offset)) >> GPIO_BIT(offset)) & 0x1;
+ return !!(tegra_gpio_readl(tgi, GPIO_IN(tgi, offset)) & bval);
}
static int tegra_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
{
- tegra_gpio_mask_write(GPIO_MSK_OE(offset), offset, 0);
- tegra_gpio_enable(offset);
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+ tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 0);
+ tegra_gpio_enable(tgi, offset);
return 0;
}
static int tegra_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
int value)
{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
tegra_gpio_set(chip, offset, value);
- tegra_gpio_mask_write(GPIO_MSK_OE(offset), offset, 1);
- tegra_gpio_enable(offset);
+ tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 1);
+ tegra_gpio_enable(tgi, offset);
return 0;
}
static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
{
- return irq_find_mapping(irq_domain, offset);
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+ return irq_find_mapping(tgi->irq_domain, offset);
}
static struct gpio_chip tegra_gpio_chip = {
@@ -184,29 +203,36 @@ static struct gpio_chip tegra_gpio_chip = {
static void tegra_gpio_irq_ack(struct irq_data *d)
{
+ struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+ struct tegra_gpio_info *tgi = bank->tgi;
int gpio = d->hwirq;
- tegra_gpio_writel(1 << GPIO_BIT(gpio), GPIO_INT_CLR(gpio));
+ tegra_gpio_writel(tgi, 1 << GPIO_BIT(gpio), GPIO_INT_CLR(tgi, gpio));
}
static void tegra_gpio_irq_mask(struct irq_data *d)
{
+ struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+ struct tegra_gpio_info *tgi = bank->tgi;
int gpio = d->hwirq;
- tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 0);
+ tegra_gpio_mask_write(tgi, GPIO_MSK_INT_ENB(tgi, gpio), gpio, 0);
}
static void tegra_gpio_irq_unmask(struct irq_data *d)
{
+ struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+ struct tegra_gpio_info *tgi = bank->tgi;
int gpio = d->hwirq;
- tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 1);
+ tegra_gpio_mask_write(tgi, GPIO_MSK_INT_ENB(tgi, gpio), gpio, 1);
}
static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
{
int gpio = d->hwirq;
struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+ struct tegra_gpio_info *tgi = bank->tgi;
int port = GPIO_PORT(gpio);
int lvl_type;
int val;
@@ -238,23 +264,24 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
return -EINVAL;
}
- ret = gpiochip_lock_as_irq(&tegra_gpio_chip, gpio);
+ ret = gpiochip_lock_as_irq(tgi->gc, gpio);
if (ret) {
- dev_err(dev, "unable to lock Tegra GPIO %d as IRQ\n", gpio);
+ dev_err(tgi->dev,
+ "unable to lock Tegra GPIO %d as IRQ\n", gpio);
return ret;
}
spin_lock_irqsave(&bank->lvl_lock[port], flags);
- val = tegra_gpio_readl(GPIO_INT_LVL(gpio));
+ val = tegra_gpio_readl(tgi, GPIO_INT_LVL(tgi, gpio));
val &= ~(GPIO_INT_LVL_MASK << GPIO_BIT(gpio));
val |= lvl_type << GPIO_BIT(gpio);
- tegra_gpio_writel(val, GPIO_INT_LVL(gpio));
+ tegra_gpio_writel(tgi, val, GPIO_INT_LVL(tgi, gpio));
spin_unlock_irqrestore(&bank->lvl_lock[port], flags);
- tegra_gpio_mask_write(GPIO_MSK_OE(gpio), gpio, 0);
- tegra_gpio_enable(gpio);
+ tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, gpio), gpio, 0);
+ tegra_gpio_enable(tgi, gpio);
if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
irq_set_handler_locked(d, handle_level_irq);
@@ -266,9 +293,11 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
static void tegra_gpio_irq_shutdown(struct irq_data *d)
{
+ struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+ struct tegra_gpio_info *tgi = bank->tgi;
int gpio = d->hwirq;
- gpiochip_unlock_as_irq(&tegra_gpio_chip, gpio);
+ gpiochip_unlock_as_irq(tgi->gc, gpio);
}
static void tegra_gpio_irq_handler(struct irq_desc *desc)
@@ -276,19 +305,24 @@ static void tegra_gpio_irq_handler(struct irq_desc *desc)
int port;
int pin;
int unmasked = 0;
+ int gpio;
+ u32 lvl;
+ unsigned long sta;
struct irq_chip *chip = irq_desc_get_chip(desc);
struct tegra_gpio_bank *bank = irq_desc_get_handler_data(desc);
+ struct tegra_gpio_info *tgi = bank->tgi;
chained_irq_enter(chip, desc);
for (port = 0; port < 4; port++) {
- int gpio = tegra_gpio_compose(bank->bank, port, 0);
- unsigned long sta = tegra_gpio_readl(GPIO_INT_STA(gpio)) &
- tegra_gpio_readl(GPIO_INT_ENB(gpio));
- u32 lvl = tegra_gpio_readl(GPIO_INT_LVL(gpio));
+ gpio = tegra_gpio_compose(bank->bank, port, 0);
+ sta = tegra_gpio_readl(tgi, GPIO_INT_STA(tgi, gpio)) &
+ tegra_gpio_readl(tgi, GPIO_INT_ENB(tgi, gpio));
+ lvl = tegra_gpio_readl(tgi, GPIO_INT_LVL(tgi, gpio));
for_each_set_bit(pin, &sta, 8) {
- tegra_gpio_writel(1 << pin, GPIO_INT_CLR(gpio));
+ tegra_gpio_writel(tgi, 1 << pin,
+ GPIO_INT_CLR(tgi, gpio));
/* if gpio is edge triggered, clear condition
* before executing the handler so that we don't
@@ -311,22 +345,29 @@ static void tegra_gpio_irq_handler(struct irq_desc *desc)
#ifdef CONFIG_PM_SLEEP
static int tegra_gpio_resume(struct device *dev)
{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct tegra_gpio_info *tgi = platform_get_drvdata(pdev);
unsigned long flags;
int b;
int p;
local_irq_save(flags);
- for (b = 0; b < tegra_gpio_bank_count; b++) {
- struct tegra_gpio_bank *bank = &tegra_gpio_banks[b];
+ for (b = 0; b < tgi->bank_count; b++) {
+ struct tegra_gpio_bank *bank = &tgi->bank_info[b];
for (p = 0; p < ARRAY_SIZE(bank->oe); p++) {
unsigned int gpio = (b<<5) | (p<<3);
- tegra_gpio_writel(bank->cnf[p], GPIO_CNF(gpio));
- tegra_gpio_writel(bank->out[p], GPIO_OUT(gpio));
- 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(tgi, bank->cnf[p],
+ GPIO_CNF(tgi, gpio));
+ tegra_gpio_writel(tgi, bank->out[p],
+ GPIO_OUT(tgi, gpio));
+ tegra_gpio_writel(tgi, bank->oe[p],
+ GPIO_OE(tgi, gpio));
+ tegra_gpio_writel(tgi, bank->int_lvl[p],
+ GPIO_INT_LVL(tgi, gpio));
+ tegra_gpio_writel(tgi, bank->int_enb[p],
+ GPIO_INT_ENB(tgi, gpio));
}
}
@@ -336,25 +377,32 @@ static int tegra_gpio_resume(struct device *dev)
static int tegra_gpio_suspend(struct device *dev)
{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct tegra_gpio_info *tgi = platform_get_drvdata(pdev);
unsigned long flags;
int b;
int p;
local_irq_save(flags);
- for (b = 0; b < tegra_gpio_bank_count; b++) {
- struct tegra_gpio_bank *bank = &tegra_gpio_banks[b];
+ for (b = 0; b < tgi->bank_count; b++) {
+ struct tegra_gpio_bank *bank = &tgi->bank_info[b];
for (p = 0; p < ARRAY_SIZE(bank->oe); p++) {
unsigned int gpio = (b<<5) | (p<<3);
- bank->cnf[p] = tegra_gpio_readl(GPIO_CNF(gpio));
- bank->out[p] = tegra_gpio_readl(GPIO_OUT(gpio));
- 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->cnf[p] = tegra_gpio_readl(tgi,
+ GPIO_CNF(tgi, gpio));
+ bank->out[p] = tegra_gpio_readl(tgi,
+ GPIO_OUT(tgi, gpio));
+ bank->oe[p] = tegra_gpio_readl(tgi,
+ GPIO_OE(tgi, gpio));
+ bank->int_enb[p] = tegra_gpio_readl(tgi,
+ GPIO_INT_ENB(tgi, gpio));
+ bank->int_lvl[p] = tegra_gpio_readl(tgi,
+ GPIO_INT_LVL(tgi, gpio));
/* Enable gpio irq for wake up source */
- tegra_gpio_writel(bank->wake_enb[p],
- GPIO_INT_ENB(gpio));
+ tegra_gpio_writel(tgi, bank->wake_enb[p],
+ GPIO_INT_ENB(tgi, gpio));
}
}
local_irq_restore(flags);
@@ -387,22 +435,23 @@ static int tegra_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
static int dbg_gpio_show(struct seq_file *s, void *unused)
{
+ struct tegra_gpio_info *tgi = s->private;
int i;
int j;
- for (i = 0; i < tegra_gpio_bank_count; i++) {
+ for (i = 0; i < tgi->bank_count; i++) {
for (j = 0; j < 4; j++) {
int gpio = tegra_gpio_compose(i, j, 0);
seq_printf(s,
"%d:%d %02x %02x %02x %02x %02x %02x %06x\n",
i, j,
- tegra_gpio_readl(GPIO_CNF(gpio)),
- tegra_gpio_readl(GPIO_OE(gpio)),
- tegra_gpio_readl(GPIO_OUT(gpio)),
- tegra_gpio_readl(GPIO_IN(gpio)),
- tegra_gpio_readl(GPIO_INT_STA(gpio)),
- tegra_gpio_readl(GPIO_INT_ENB(gpio)),
- tegra_gpio_readl(GPIO_INT_LVL(gpio)));
+ tegra_gpio_readl(tgi, GPIO_CNF(tgi, gpio)),
+ tegra_gpio_readl(tgi, GPIO_OE(tgi, gpio)),
+ tegra_gpio_readl(tgi, GPIO_OUT(tgi, gpio)),
+ tegra_gpio_readl(tgi, GPIO_IN(tgi, gpio)),
+ tegra_gpio_readl(tgi, GPIO_INT_STA(tgi, gpio)),
+ tegra_gpio_readl(tgi, GPIO_INT_ENB(tgi, gpio)),
+ tegra_gpio_readl(tgi, GPIO_INT_LVL(tgi, gpio)));
}
}
return 0;
@@ -410,7 +459,7 @@ static int dbg_gpio_show(struct seq_file *s, void *unused)
static int dbg_gpio_open(struct inode *inode, struct file *file)
{
- return single_open(file, dbg_gpio_show, &inode->i_private);
+ return single_open(file, dbg_gpio_show, inode->i_private);
}
static const struct file_operations debug_fops = {
@@ -420,15 +469,15 @@ static const struct file_operations debug_fops = {
.release = single_release,
};
-static void tegra_gpio_debuginit(void)
+static void tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
{
(void) debugfs_create_file("tegra_gpio", S_IRUGO,
- NULL, NULL, &debug_fops);
+ NULL, tgi, &debug_fops);
}
#else
-static inline void tegra_gpio_debuginit(void)
+static inline void tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
{
}
@@ -458,6 +507,7 @@ static struct lock_class_key gpio_lock_class;
static int tegra_gpio_probe(struct platform_device *pdev)
{
const struct tegra_gpio_soc_config *config;
+ struct tegra_gpio_info *tgi;
struct resource *res;
struct tegra_gpio_bank *bank;
int ret;
@@ -465,79 +515,85 @@ 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");
return -ENODEV;
}
- tegra_gpio_bank_stride = config->bank_stride;
- tegra_gpio_upper_offset = config->upper_offset;
+ tgi = devm_kzalloc(&pdev->dev, sizeof(*tgi), GFP_KERNEL);
+ if (!tgi)
+ return -ENODEV;
+
+ tgi->soc = config;
+ tgi->dev = &pdev->dev;
for (;;) {
- res = platform_get_resource(pdev, IORESOURCE_IRQ, tegra_gpio_bank_count);
+ res = platform_get_resource(pdev, IORESOURCE_IRQ,
+ tgi->bank_count);
if (!res)
break;
- tegra_gpio_bank_count++;
+ tgi->bank_count++;
}
- if (!tegra_gpio_bank_count) {
+ if (!tgi->bank_count) {
dev_err(&pdev->dev, "Missing IRQ resource\n");
return -ENODEV;
}
- tegra_gpio_chip.ngpio = tegra_gpio_bank_count * 32;
+ tgi->gc = &tegra_gpio_chip;
+ tgi->gc->ngpio = tgi->bank_count * 32;
+ tgi->gc->parent = &pdev->dev;
+ tgi->gc->of_node = pdev->dev.of_node;
- tegra_gpio_banks = devm_kzalloc(&pdev->dev,
- tegra_gpio_bank_count * sizeof(*tegra_gpio_banks),
- GFP_KERNEL);
- if (!tegra_gpio_banks)
+ platform_set_drvdata(pdev, tgi);
+
+ tgi->bank_info = devm_kzalloc(&pdev->dev, tgi->bank_count *
+ sizeof(*tgi->bank_info), GFP_KERNEL);
+ if (!tgi->bank_info)
return -ENODEV;
- irq_domain = irq_domain_add_linear(pdev->dev.of_node,
- tegra_gpio_chip.ngpio,
- &irq_domain_simple_ops, NULL);
- if (!irq_domain)
+ tgi->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
+ tgi->gc->ngpio,
+ &irq_domain_simple_ops, NULL);
+ if (!tgi->irq_domain)
return -ENODEV;
- for (i = 0; i < tegra_gpio_bank_count; i++) {
+ for (i = 0; i < tgi->bank_count; i++) {
res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
if (!res) {
dev_err(&pdev->dev, "Missing IRQ resource\n");
return -ENODEV;
}
- bank = &tegra_gpio_banks[i];
+ bank = &tgi->bank_info[i];
bank->bank = i;
bank->irq = res->start;
+ bank->tgi = tgi;
}
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- regs = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(regs))
- return PTR_ERR(regs);
+ tgi->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(tgi->regs))
+ return PTR_ERR(tgi->regs);
- for (i = 0; i < tegra_gpio_bank_count; i++) {
+ for (i = 0; i < tgi->bank_count; i++) {
for (j = 0; j < 4; j++) {
int gpio = tegra_gpio_compose(i, j, 0);
- tegra_gpio_writel(0x00, GPIO_INT_ENB(gpio));
+ tegra_gpio_writel(tgi, 0x00, GPIO_INT_ENB(tgi, gpio));
}
}
- tegra_gpio_chip.of_node = pdev->dev.of_node;
-
- ret = devm_gpiochip_add_data(&pdev->dev, &tegra_gpio_chip, NULL);
+ ret = devm_gpiochip_add_data(&pdev->dev, tgi->gc, tgi);
if (ret < 0) {
- irq_domain_remove(irq_domain);
+ irq_domain_remove(tgi->irq_domain);
return ret;
}
- for (gpio = 0; gpio < tegra_gpio_chip.ngpio; gpio++) {
- int irq = irq_create_mapping(irq_domain, gpio);
+ for (gpio = 0; gpio < tgi->gc->ngpio; gpio++) {
+ int irq = irq_create_mapping(tgi->irq_domain, gpio);
/* No validity check; all Tegra GPIOs are valid IRQs */
- bank = &tegra_gpio_banks[GPIO_BANK(gpio)];
+ bank = &tgi->bank_info[GPIO_BANK(gpio)];
irq_set_lockdep_class(irq, &gpio_lock_class);
irq_set_chip_data(irq, bank);
@@ -545,8 +601,8 @@ static int tegra_gpio_probe(struct platform_device *pdev)
handle_simple_irq);
}
- for (i = 0; i < tegra_gpio_bank_count; i++) {
- bank = &tegra_gpio_banks[i];
+ for (i = 0; i < tgi->bank_count; i++) {
+ bank = &tgi->bank_info[i];
irq_set_chained_handler_and_data(bank->irq,
tegra_gpio_irq_handler, bank);
@@ -555,7 +611,7 @@ static int tegra_gpio_probe(struct platform_device *pdev)
spin_lock_init(&bank->lvl_lock[j]);
}
- tegra_gpio_debuginit();
+ tegra_gpio_debuginit(tgi);
return 0;
}
--
2.1.4
On 04/22/2016 04:09 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.
Reviewed-by: Stephen Warren <[email protected]>
On Fri, Apr 22, 2016 at 7:09 PM, Laxman Dewangan <[email protected]> wrote:
> Move the file scoped multiple global variable from Tegra GPIO
> driver to the structure and make this as gpiochip data which
> can be referred from GPIO chip callbacks.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
> Reviewed-by: Stephen Warren <[email protected]>
>
> ---
> This patch is reworked on top of earlier patch
> gpio: tegra: Remove the need of keeping device handle for gpio driver
>
> There was review comment that we should get for all variable and hence
> this is outcome of the discussion.
>
> Changes from V3:
> - Remove DBC/EN registers.
> - Remove non-required new lines.
> - Collected RB from Stephen.
>
> drivers/gpio/gpio-tegra.c | 296 +++++++++++++++++++++++++++-------------------
> 1 file changed, 176 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index cd69422..6af5eb2 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -35,24 +35,24 @@
> #define GPIO_PORT(x) (((x) >> 3) & 0x3)
> #define GPIO_BIT(x) ((x) & 0x7)
>
> -#define GPIO_REG(x) (GPIO_BANK(x) * tegra_gpio_bank_stride + \
> +#define GPIO_REG(tgi, x) (GPIO_BANK(x) * tgi->soc->bank_stride + \
> GPIO_PORT(x) * 4)
>
> -#define GPIO_CNF(x) (GPIO_REG(x) + 0x00)
> -#define GPIO_OE(x) (GPIO_REG(x) + 0x10)
> -#define GPIO_OUT(x) (GPIO_REG(x) + 0X20)
> -#define GPIO_IN(x) (GPIO_REG(x) + 0x30)
> -#define GPIO_INT_STA(x) (GPIO_REG(x) + 0x40)
> -#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_MSK_CNF(x) (GPIO_REG(x) + tegra_gpio_upper_offset + 0x00)
> -#define GPIO_MSK_OE(x) (GPIO_REG(x) + tegra_gpio_upper_offset + 0x10)
> -#define GPIO_MSK_OUT(x) (GPIO_REG(x) + tegra_gpio_upper_offset + 0X20)
> -#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_CNF(t, x) (GPIO_REG(t, x) + 0x00)
> +#define GPIO_OE(t, x) (GPIO_REG(t, x) + 0x10)
> +#define GPIO_OUT(t, x) (GPIO_REG(t, x) + 0X20)
> +#define GPIO_IN(t, x) (GPIO_REG(t, x) + 0x30)
> +#define GPIO_INT_STA(t, x) (GPIO_REG(t, x) + 0x40)
> +#define GPIO_INT_ENB(t, x) (GPIO_REG(t, x) + 0x50)
> +#define GPIO_INT_LVL(t, x) (GPIO_REG(t, x) + 0x60)
> +#define GPIO_INT_CLR(t, x) (GPIO_REG(t, x) + 0x70)
> +
> +#define GPIO_MSK_CNF(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x00)
> +#define GPIO_MSK_OE(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x10)
> +#define GPIO_MSK_OUT(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0X20)
> +#define GPIO_MSK_INT_STA(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x40)
> +#define GPIO_MSK_INT_ENB(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x50)
> +#define GPIO_MSK_INT_LVL(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x60)
>
> #define GPIO_INT_LVL_MASK 0x010101
> #define GPIO_INT_LVL_EDGE_RISING 0x000101
> @@ -61,6 +61,8 @@
> #define GPIO_INT_LVL_LEVEL_HIGH 0x000001
> #define GPIO_INT_LVL_LEVEL_LOW 0x000000
>
> +struct tegra_gpio_info;
> +
> struct tegra_gpio_bank {
> int bank;
> int irq;
> @@ -73,6 +75,7 @@ struct tegra_gpio_bank {
> u32 int_lvl[4];
> u32 wake_enb[4];
> #endif
> + struct tegra_gpio_info *tgi;
> };
>
> struct tegra_gpio_soc_config {
> @@ -80,22 +83,25 @@ 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;
> -static u32 tegra_gpio_bank_stride;
> -static u32 tegra_gpio_upper_offset;
> -static struct tegra_gpio_bank *tegra_gpio_banks;
> +struct tegra_gpio_info {
I think tegra_gpio_chip would be a better name for this structure
(especially if you make "struct gpio_chip gc" its first member to
highlight the fact that it inherits from it) and more in line with
what other GPIO drivers do.
You can then rename the former tegra_gpio_chip to something like
tegra_gpio_funcs since it would just be used to set the chip's
functions if you follow my comment on patch 4/4.
> + struct device *dev;
> + void __iomem *regs;
> + struct irq_domain *irq_domain;
> + struct tegra_gpio_bank *bank_info;
> + const struct tegra_gpio_soc_config *soc;
> + struct gpio_chip *gc;
> + u32 bank_count;
> +};
>
> -static inline void tegra_gpio_writel(u32 val, u32 reg)
> +static inline void tegra_gpio_writel(struct tegra_gpio_info *tgi,
> + u32 val, u32 reg)
> {
> - __raw_writel(val, regs + reg);
> + __raw_writel(val, tgi->regs + reg);
> }
>
> -static inline u32 tegra_gpio_readl(u32 reg)
> +static inline u32 tegra_gpio_readl(struct tegra_gpio_info *tgi, u32 reg)
> {
> - return __raw_readl(regs + reg);
> + return __raw_readl(tgi->regs + reg);
> }
>
> static int tegra_gpio_compose(int bank, int port, int bit)
> @@ -103,24 +109,25 @@ static int tegra_gpio_compose(int bank, int port, int bit)
> return (bank << 5) | ((port & 0x3) << 3) | (bit & 0x7);
> }
>
> -static void tegra_gpio_mask_write(u32 reg, int gpio, int value)
> +static void tegra_gpio_mask_write(struct tegra_gpio_info *tgi, u32 reg,
> + int gpio, int value)
> {
> u32 val;
>
> val = 0x100 << GPIO_BIT(gpio);
> if (value)
> val |= 1 << GPIO_BIT(gpio);
> - tegra_gpio_writel(val, reg);
> + tegra_gpio_writel(tgi, val, reg);
> }
>
> -static void tegra_gpio_enable(int gpio)
> +static void tegra_gpio_enable(struct tegra_gpio_info *tgi, int gpio)
> {
> - tegra_gpio_mask_write(GPIO_MSK_CNF(gpio), gpio, 1);
> + tegra_gpio_mask_write(tgi, GPIO_MSK_CNF(tgi, gpio), gpio, 1);
> }
>
> -static void tegra_gpio_disable(int gpio)
> +static void tegra_gpio_disable(struct tegra_gpio_info *tgi, int gpio)
> {
> - tegra_gpio_mask_write(GPIO_MSK_CNF(gpio), gpio, 0);
> + tegra_gpio_mask_write(tgi, GPIO_MSK_CNF(tgi, gpio), gpio, 0);
> }
>
> static int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
> @@ -130,44 +137,56 @@ static int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
>
> static void tegra_gpio_free(struct gpio_chip *chip, unsigned offset)
> {
> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +
> pinctrl_free_gpio(offset);
> - tegra_gpio_disable(offset);
> + tegra_gpio_disable(tgi, offset);
> }
>
> static void tegra_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> {
> - tegra_gpio_mask_write(GPIO_MSK_OUT(offset), offset, value);
> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +
> + tegra_gpio_mask_write(tgi, GPIO_MSK_OUT(tgi, offset), offset, value);
> }
>
> static int tegra_gpio_get(struct gpio_chip *chip, unsigned offset)
> {
> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> + int bval = BIT(GPIO_BIT(offset));
> +
> /* If gpio is in output mode then read from the out value */
> - if ((tegra_gpio_readl(GPIO_OE(offset)) >> GPIO_BIT(offset)) & 1)
> - return (tegra_gpio_readl(GPIO_OUT(offset)) >>
> - GPIO_BIT(offset)) & 0x1;
> + if (tegra_gpio_readl(tgi, GPIO_OE(tgi, offset)) & bval)
> + return !!(tegra_gpio_readl(tgi, GPIO_OUT(tgi, offset)) & bval);
>
> - return (tegra_gpio_readl(GPIO_IN(offset)) >> GPIO_BIT(offset)) & 0x1;
> + return !!(tegra_gpio_readl(tgi, GPIO_IN(tgi, offset)) & bval);
> }
>
> static int tegra_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> {
> - tegra_gpio_mask_write(GPIO_MSK_OE(offset), offset, 0);
> - tegra_gpio_enable(offset);
> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +
> + tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 0);
> + tegra_gpio_enable(tgi, offset);
> return 0;
> }
>
> static int tegra_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
> int value)
> {
> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +
> tegra_gpio_set(chip, offset, value);
> - tegra_gpio_mask_write(GPIO_MSK_OE(offset), offset, 1);
> - tegra_gpio_enable(offset);
> + tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 1);
> + tegra_gpio_enable(tgi, offset);
> return 0;
> }
>
> static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> {
> - return irq_find_mapping(irq_domain, offset);
> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> +
> + return irq_find_mapping(tgi->irq_domain, offset);
> }
>
> static struct gpio_chip tegra_gpio_chip = {
> @@ -184,29 +203,36 @@ static struct gpio_chip tegra_gpio_chip = {
>
> static void tegra_gpio_irq_ack(struct irq_data *d)
> {
> + struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> + struct tegra_gpio_info *tgi = bank->tgi;
> int gpio = d->hwirq;
>
> - tegra_gpio_writel(1 << GPIO_BIT(gpio), GPIO_INT_CLR(gpio));
> + tegra_gpio_writel(tgi, 1 << GPIO_BIT(gpio), GPIO_INT_CLR(tgi, gpio));
> }
>
> static void tegra_gpio_irq_mask(struct irq_data *d)
> {
> + struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> + struct tegra_gpio_info *tgi = bank->tgi;
> int gpio = d->hwirq;
>
> - tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 0);
> + tegra_gpio_mask_write(tgi, GPIO_MSK_INT_ENB(tgi, gpio), gpio, 0);
> }
>
> static void tegra_gpio_irq_unmask(struct irq_data *d)
> {
> + struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> + struct tegra_gpio_info *tgi = bank->tgi;
> int gpio = d->hwirq;
>
> - tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 1);
> + tegra_gpio_mask_write(tgi, GPIO_MSK_INT_ENB(tgi, gpio), gpio, 1);
> }
>
> static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> {
> int gpio = d->hwirq;
> struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> + struct tegra_gpio_info *tgi = bank->tgi;
> int port = GPIO_PORT(gpio);
> int lvl_type;
> int val;
> @@ -238,23 +264,24 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> return -EINVAL;
> }
>
> - ret = gpiochip_lock_as_irq(&tegra_gpio_chip, gpio);
> + ret = gpiochip_lock_as_irq(tgi->gc, gpio);
> if (ret) {
> - dev_err(dev, "unable to lock Tegra GPIO %d as IRQ\n", gpio);
> + dev_err(tgi->dev,
> + "unable to lock Tegra GPIO %d as IRQ\n", gpio);
> return ret;
> }
>
> spin_lock_irqsave(&bank->lvl_lock[port], flags);
>
> - val = tegra_gpio_readl(GPIO_INT_LVL(gpio));
> + val = tegra_gpio_readl(tgi, GPIO_INT_LVL(tgi, gpio));
> val &= ~(GPIO_INT_LVL_MASK << GPIO_BIT(gpio));
> val |= lvl_type << GPIO_BIT(gpio);
> - tegra_gpio_writel(val, GPIO_INT_LVL(gpio));
> + tegra_gpio_writel(tgi, val, GPIO_INT_LVL(tgi, gpio));
>
> spin_unlock_irqrestore(&bank->lvl_lock[port], flags);
>
> - tegra_gpio_mask_write(GPIO_MSK_OE(gpio), gpio, 0);
> - tegra_gpio_enable(gpio);
> + tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, gpio), gpio, 0);
> + tegra_gpio_enable(tgi, gpio);
>
> if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> irq_set_handler_locked(d, handle_level_irq);
> @@ -266,9 +293,11 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>
> static void tegra_gpio_irq_shutdown(struct irq_data *d)
> {
> + struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> + struct tegra_gpio_info *tgi = bank->tgi;
> int gpio = d->hwirq;
>
> - gpiochip_unlock_as_irq(&tegra_gpio_chip, gpio);
> + gpiochip_unlock_as_irq(tgi->gc, gpio);
> }
>
> static void tegra_gpio_irq_handler(struct irq_desc *desc)
> @@ -276,19 +305,24 @@ static void tegra_gpio_irq_handler(struct irq_desc *desc)
> int port;
> int pin;
> int unmasked = 0;
> + int gpio;
> + u32 lvl;
> + unsigned long sta;
Why do you move these declarations here? They are not used outside of
the loop IIUC.
> struct irq_chip *chip = irq_desc_get_chip(desc);
> struct tegra_gpio_bank *bank = irq_desc_get_handler_data(desc);
> + struct tegra_gpio_info *tgi = bank->tgi;
>
> chained_irq_enter(chip, desc);
>
> for (port = 0; port < 4; port++) {
> - int gpio = tegra_gpio_compose(bank->bank, port, 0);
> - unsigned long sta = tegra_gpio_readl(GPIO_INT_STA(gpio)) &
> - tegra_gpio_readl(GPIO_INT_ENB(gpio));
> - u32 lvl = tegra_gpio_readl(GPIO_INT_LVL(gpio));
> + gpio = tegra_gpio_compose(bank->bank, port, 0);
> + sta = tegra_gpio_readl(tgi, GPIO_INT_STA(tgi, gpio)) &
> + tegra_gpio_readl(tgi, GPIO_INT_ENB(tgi, gpio));
> + lvl = tegra_gpio_readl(tgi, GPIO_INT_LVL(tgi, gpio));
>
> for_each_set_bit(pin, &sta, 8) {
> - tegra_gpio_writel(1 << pin, GPIO_INT_CLR(gpio));
> + tegra_gpio_writel(tgi, 1 << pin,
> + GPIO_INT_CLR(tgi, gpio));
>
> /* if gpio is edge triggered, clear condition
> * before executing the handler so that we don't
> @@ -311,22 +345,29 @@ static void tegra_gpio_irq_handler(struct irq_desc *desc)
> #ifdef CONFIG_PM_SLEEP
> static int tegra_gpio_resume(struct device *dev)
> {
> + struct platform_device *pdev = to_platform_device(dev);
> + struct tegra_gpio_info *tgi = platform_get_drvdata(pdev);
> unsigned long flags;
> int b;
> int p;
>
> local_irq_save(flags);
>
> - for (b = 0; b < tegra_gpio_bank_count; b++) {
> - struct tegra_gpio_bank *bank = &tegra_gpio_banks[b];
> + for (b = 0; b < tgi->bank_count; b++) {
> + struct tegra_gpio_bank *bank = &tgi->bank_info[b];
>
> for (p = 0; p < ARRAY_SIZE(bank->oe); p++) {
> unsigned int gpio = (b<<5) | (p<<3);
> - tegra_gpio_writel(bank->cnf[p], GPIO_CNF(gpio));
> - tegra_gpio_writel(bank->out[p], GPIO_OUT(gpio));
> - 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(tgi, bank->cnf[p],
> + GPIO_CNF(tgi, gpio));
> + tegra_gpio_writel(tgi, bank->out[p],
> + GPIO_OUT(tgi, gpio));
> + tegra_gpio_writel(tgi, bank->oe[p],
> + GPIO_OE(tgi, gpio));
> + tegra_gpio_writel(tgi, bank->int_lvl[p],
> + GPIO_INT_LVL(tgi, gpio));
> + tegra_gpio_writel(tgi, bank->int_enb[p],
> + GPIO_INT_ENB(tgi, gpio));
> }
> }
>
> @@ -336,25 +377,32 @@ static int tegra_gpio_resume(struct device *dev)
>
> static int tegra_gpio_suspend(struct device *dev)
> {
> + struct platform_device *pdev = to_platform_device(dev);
> + struct tegra_gpio_info *tgi = platform_get_drvdata(pdev);
> unsigned long flags;
> int b;
> int p;
>
> local_irq_save(flags);
> - for (b = 0; b < tegra_gpio_bank_count; b++) {
> - struct tegra_gpio_bank *bank = &tegra_gpio_banks[b];
> + for (b = 0; b < tgi->bank_count; b++) {
> + struct tegra_gpio_bank *bank = &tgi->bank_info[b];
>
> for (p = 0; p < ARRAY_SIZE(bank->oe); p++) {
> unsigned int gpio = (b<<5) | (p<<3);
> - bank->cnf[p] = tegra_gpio_readl(GPIO_CNF(gpio));
> - bank->out[p] = tegra_gpio_readl(GPIO_OUT(gpio));
> - 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->cnf[p] = tegra_gpio_readl(tgi,
> + GPIO_CNF(tgi, gpio));
> + bank->out[p] = tegra_gpio_readl(tgi,
> + GPIO_OUT(tgi, gpio));
> + bank->oe[p] = tegra_gpio_readl(tgi,
> + GPIO_OE(tgi, gpio));
> + bank->int_enb[p] = tegra_gpio_readl(tgi,
> + GPIO_INT_ENB(tgi, gpio));
> + bank->int_lvl[p] = tegra_gpio_readl(tgi,
> + GPIO_INT_LVL(tgi, gpio));
>
> /* Enable gpio irq for wake up source */
> - tegra_gpio_writel(bank->wake_enb[p],
> - GPIO_INT_ENB(gpio));
> + tegra_gpio_writel(tgi, bank->wake_enb[p],
> + GPIO_INT_ENB(tgi, gpio));
> }
> }
> local_irq_restore(flags);
> @@ -387,22 +435,23 @@ static int tegra_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
>
> static int dbg_gpio_show(struct seq_file *s, void *unused)
> {
> + struct tegra_gpio_info *tgi = s->private;
> int i;
> int j;
>
> - for (i = 0; i < tegra_gpio_bank_count; i++) {
> + for (i = 0; i < tgi->bank_count; i++) {
> for (j = 0; j < 4; j++) {
> int gpio = tegra_gpio_compose(i, j, 0);
> seq_printf(s,
> "%d:%d %02x %02x %02x %02x %02x %02x %06x\n",
> i, j,
> - tegra_gpio_readl(GPIO_CNF(gpio)),
> - tegra_gpio_readl(GPIO_OE(gpio)),
> - tegra_gpio_readl(GPIO_OUT(gpio)),
> - tegra_gpio_readl(GPIO_IN(gpio)),
> - tegra_gpio_readl(GPIO_INT_STA(gpio)),
> - tegra_gpio_readl(GPIO_INT_ENB(gpio)),
> - tegra_gpio_readl(GPIO_INT_LVL(gpio)));
> + tegra_gpio_readl(tgi, GPIO_CNF(tgi, gpio)),
> + tegra_gpio_readl(tgi, GPIO_OE(tgi, gpio)),
> + tegra_gpio_readl(tgi, GPIO_OUT(tgi, gpio)),
> + tegra_gpio_readl(tgi, GPIO_IN(tgi, gpio)),
> + tegra_gpio_readl(tgi, GPIO_INT_STA(tgi, gpio)),
> + tegra_gpio_readl(tgi, GPIO_INT_ENB(tgi, gpio)),
> + tegra_gpio_readl(tgi, GPIO_INT_LVL(tgi, gpio)));
> }
> }
> return 0;
> @@ -410,7 +459,7 @@ static int dbg_gpio_show(struct seq_file *s, void *unused)
>
> static int dbg_gpio_open(struct inode *inode, struct file *file)
> {
> - return single_open(file, dbg_gpio_show, &inode->i_private);
> + return single_open(file, dbg_gpio_show, inode->i_private);
> }
>
> static const struct file_operations debug_fops = {
> @@ -420,15 +469,15 @@ static const struct file_operations debug_fops = {
> .release = single_release,
> };
>
> -static void tegra_gpio_debuginit(void)
> +static void tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
> {
> (void) debugfs_create_file("tegra_gpio", S_IRUGO,
> - NULL, NULL, &debug_fops);
> + NULL, tgi, &debug_fops);
> }
>
> #else
>
> -static inline void tegra_gpio_debuginit(void)
> +static inline void tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
> {
> }
>
> @@ -458,6 +507,7 @@ static struct lock_class_key gpio_lock_class;
> static int tegra_gpio_probe(struct platform_device *pdev)
> {
> const struct tegra_gpio_soc_config *config;
> + struct tegra_gpio_info *tgi;
> struct resource *res;
> struct tegra_gpio_bank *bank;
> int ret;
> @@ -465,79 +515,85 @@ 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");
> return -ENODEV;
> }
>
> - tegra_gpio_bank_stride = config->bank_stride;
> - tegra_gpio_upper_offset = config->upper_offset;
> + tgi = devm_kzalloc(&pdev->dev, sizeof(*tgi), GFP_KERNEL);
> + if (!tgi)
> + return -ENODEV;
> +
> + tgi->soc = config;
> + tgi->dev = &pdev->dev;
>
> for (;;) {
> - res = platform_get_resource(pdev, IORESOURCE_IRQ, tegra_gpio_bank_count);
> + res = platform_get_resource(pdev, IORESOURCE_IRQ,
> + tgi->bank_count);
> if (!res)
> break;
> - tegra_gpio_bank_count++;
> + tgi->bank_count++;
> }
> - if (!tegra_gpio_bank_count) {
> + if (!tgi->bank_count) {
> dev_err(&pdev->dev, "Missing IRQ resource\n");
> return -ENODEV;
> }
>
> - tegra_gpio_chip.ngpio = tegra_gpio_bank_count * 32;
> + tgi->gc = &tegra_gpio_chip;
> + tgi->gc->ngpio = tgi->bank_count * 32;
> + tgi->gc->parent = &pdev->dev;
> + tgi->gc->of_node = pdev->dev.of_node;
As said in 4/4, I really think gc should not be a pointer so several
instances can coexist. Most other GPIO drivers follow this model.
With these small details fixed, I believe this looks good!
Reviewed-by: Alexandre Courbot <[email protected]>
Sorry, just realized I commented on v3...
On Fri, Apr 22, 2016 at 7:09 PM, Laxman Dewangan <[email protected]> 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.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
>
> ---
> Changes from V1:
> - Write debounce count before enable.
> - Make sure the debounce count do not have any boot residuals.
>
> Changes from V2:
> - Only access register for debounce when SoC support debounce.
>
> Changes from V3:
> - Add locking mechanism in debounce count register update.
> - Move DBC register from prev patch to here.
>
> drivers/gpio/gpio-tegra.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index 6af5eb2..45d80ec 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -46,10 +46,13 @@
> #define GPIO_INT_ENB(t, x) (GPIO_REG(t, x) + 0x50)
> #define GPIO_INT_LVL(t, x) (GPIO_REG(t, x) + 0x60)
> #define GPIO_INT_CLR(t, x) (GPIO_REG(t, x) + 0x70)
> +#define GPIO_DBC_CNT(t, x) (GPIO_REG(t, x) + 0xF0)
> +
>
> #define GPIO_MSK_CNF(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x00)
> #define GPIO_MSK_OE(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x10)
> #define GPIO_MSK_OUT(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0X20)
> +#define GPIO_MSK_DBC_EN(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x30)
> #define GPIO_MSK_INT_STA(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x40)
> #define GPIO_MSK_INT_ENB(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x50)
> #define GPIO_MSK_INT_LVL(t, x) (GPIO_REG(t, x) + t->soc->upper_offset + 0x60)
> @@ -67,6 +70,7 @@ struct tegra_gpio_bank {
> int bank;
> int irq;
> spinlock_t lvl_lock[4];
> + spinlock_t dbc_lock[4]; /* Lock for updating debounce count register */
I'm nit'ing here, but maybe one spinlock shared by all ports would be
enough? (the same would apply to lvl_lock, so feel free to do this as
a separate patch) I don't think we expect *that* many concurrent
accesses, do we?
> #ifdef CONFIG_PM_SLEEP
> u32 cnf[4];
> u32 out[4];
> @@ -74,11 +78,14 @@ struct tegra_gpio_bank {
> u32 int_enb[4];
> u32 int_lvl[4];
> u32 wake_enb[4];
> + u32 dbc_enb[4];
> #endif
> + u32 dbc_cnt[4];
> struct tegra_gpio_info *tgi;
> };
>
> struct tegra_gpio_soc_config {
> + bool debounce_supported;
> u32 bank_stride;
> u32 upper_offset;
> };
> @@ -182,6 +189,38 @@ 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)
> +{
> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> + unsigned int debounce_ms = DIV_ROUND_UP(debounce, 1000);
> + unsigned long flags;
> + int port = GPIO_PORT(offset);
> + int bank = GPIO_BANK(offset);
Maybe declare "bank" as follows:
struct tegra_gpio_bank *bank = &tgi->bank_info[GPIO_BANK(offset)];
This will allow you to simplify the code that follows:
> +
> + if (!debounce_ms) {
> + tegra_gpio_mask_write(tgi, GPIO_MSK_DBC_EN(tgi, 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.
> + */
> + spin_lock_irqsave(&tgi->bank_info[bank].dbc_lock[port], flags);
> + if (tgi->bank_info[bank].dbc_cnt[port] < debounce_ms) {
> + tegra_gpio_writel(tgi, debounce_ms, GPIO_DBC_CNT(tgi, offset));
> + tgi->bank_info[bank].dbc_cnt[port] = debounce_ms;
> + }
> + spin_unlock_irqrestore(&tgi->bank_info[bank].dbc_lock[port], flags);
Becomes:
spin_lock_irqsave(bank->dbc_lock[port], flags);
if (bank->dbc_cnt[port] < debounce_ms) {
tegra_gpio_writel(tgi, debounce_ms, GPIO_DBC_CNT(tgi, offset));
bank->dbc_cnt[port] = debounce_ms;
}
spin_unlock_irqrestore(&bank->dbc_lock[port], flags);
Which is nicer to the eyes.
Extra points if you initialize port and bank after we ensure that
debounce_ms is not zero and that their value will actually be used.
> +
> + tegra_gpio_mask_write(tgi, GPIO_MSK_DBC_EN(tgi, offset), offset, 1);
> +
> + return 0;
> +}
> +
> static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> {
> struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
> @@ -197,6 +236,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,
> };
> @@ -360,6 +400,14 @@ static int tegra_gpio_resume(struct device *dev)
> unsigned int gpio = (b<<5) | (p<<3);
> tegra_gpio_writel(tgi, bank->cnf[p],
> GPIO_CNF(tgi, gpio));
> +
> + if (tgi->soc->debounce_supported) {
> + tegra_gpio_writel(tgi, bank->dbc_cnt[p],
> + GPIO_DBC_CNT(tgi, gpio));
> + tegra_gpio_writel(tgi, bank->dbc_enb[p],
> + GPIO_MSK_DBC_EN(tgi, gpio));
> + }
> +
> tegra_gpio_writel(tgi, bank->out[p],
> GPIO_OUT(tgi, gpio));
> tegra_gpio_writel(tgi, bank->oe[p],
> @@ -395,6 +443,13 @@ static int tegra_gpio_suspend(struct device *dev)
> GPIO_OUT(tgi, gpio));
> bank->oe[p] = tegra_gpio_readl(tgi,
> GPIO_OE(tgi, gpio));
> + if (tgi->soc->debounce_supported) {
> + bank->dbc_enb[p] = tegra_gpio_readl(tgi,
> + GPIO_MSK_DBC_EN(tgi, gpio));
> + bank->dbc_enb[p] = (bank->dbc_enb[p] << 8) |
> + bank->dbc_enb[p];
> + }
> +
> bank->int_enb[p] = tegra_gpio_readl(tgi,
> GPIO_INT_ENB(tgi, gpio));
> bank->int_lvl[p] = tegra_gpio_readl(tgi,
> @@ -547,6 +602,9 @@ static int tegra_gpio_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, tgi);
>
> + if (!config->debounce_supported)
> + tgi->gc->set_debounce = NULL;
Yep, we really want one gpio_chip instance in the tegra_gpio_info
struct, otherwise this kind of limits the purpose of getting rid of
these global variables...
On Monday 25 April 2016 10:43 AM, Alexandre Courbot wrote:
> On Fri, Apr 22, 2016 at 7:09 PM, Laxman Dewangan <[email protected]> wrote:
>> -static struct tegra_gpio_bank *tegra_gpio_banks;
>> +struct tegra_gpio_info {
> I think tegra_gpio_chip would be a better name for this structure
> (especially if you make "struct gpio_chip gc" its first member to
> highlight the fact that it inherits from it) and more in line with
> what other GPIO drivers do.
The gpio library has struct gpio_chip and it is better to have the
variable name matching with it.
So the struct gpio_chp tegra_gpio_chip is better way here.
For gpio library, the driver specific data is called as treated as drvdata.
I will keep with tegra_gpio_info here.
>
> You can then rename the former tegra_gpio_chip to something like
> tegra_gpio_funcs since it would just be used to set the chip's
> functions if you follow my comment on patch 4/4.
>
>> + struct device *dev;
>> + void __iomem *regs;
>> + struct irq_domain *irq_domain;
>> + struct tegra_gpio_bank *bank_info;
>> + const struct tegra_gpio_soc_config *soc;
>> + struct gpio_chip *gc;
>> + u32 bank_count;
>> +};
>>
>> -static inline void tegra_gpio_writel(u32 val, u32 reg)
>> +static inline void tegra_gpio_writel(struct tegra_gpio_info *tgi,
>> + u32 val, u32 reg)
>> {
>> - __raw_writel(val, regs + reg);
>> + __raw_writel(val, tgi->regs + reg);
>> }
>>
>> -static inline u32 tegra_gpio_readl(u32 reg)
>> +static inline u32 tegra_gpio_readl(struct tegra_gpio_info *tgi, u32 reg)
>> {
>> - return __raw_readl(regs + reg);
>> + return __raw_readl(tgi->regs + reg);
>> }
>>
>> static int tegra_gpio_compose(int bank, int port, int bit)
>> @@ -103,24 +109,25 @@ static int tegra_gpio_compose(int bank, int port, int bit)
>> return (bank << 5) | ((port & 0x3) << 3) | (bit & 0x7);
>> }
>>
>> -static void tegra_gpio_mask_write(u32 reg, int gpio, int value)
>> +static void tegra_gpio_mask_write(struct tegra_gpio_info *tgi, u32 reg,
>> + int gpio, int value)
>> {
>> u32 val;
>>
>> val = 0x100 << GPIO_BIT(gpio);
>> if (value)
>> val |= 1 << GPIO_BIT(gpio);
>> - tegra_gpio_writel(val, reg);
>> + tegra_gpio_writel(tgi, val, reg);
>> }
>>
>> -static void tegra_gpio_enable(int gpio)
>> +static void tegra_gpio_enable(struct tegra_gpio_info *tgi, int gpio)
>> {
>> - tegra_gpio_mask_write(GPIO_MSK_CNF(gpio), gpio, 1);
>> + tegra_gpio_mask_write(tgi, GPIO_MSK_CNF(tgi, gpio), gpio, 1);
>> }
>>
>> -static void tegra_gpio_disable(int gpio)
>> +static void tegra_gpio_disable(struct tegra_gpio_info *tgi, int gpio)
>> {
>> - tegra_gpio_mask_write(GPIO_MSK_CNF(gpio), gpio, 0);
>> + tegra_gpio_mask_write(tgi, GPIO_MSK_CNF(tgi, gpio), gpio, 0);
>> }
>>
>> static int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
>> @@ -130,44 +137,56 @@ static int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
>>
>> static void tegra_gpio_free(struct gpio_chip *chip, unsigned offset)
>> {
>> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
>> +
>> pinctrl_free_gpio(offset);
>> - tegra_gpio_disable(offset);
>> + tegra_gpio_disable(tgi, offset);
>> }
>>
>> static void tegra_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>> {
>> - tegra_gpio_mask_write(GPIO_MSK_OUT(offset), offset, value);
>> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
>> +
>> + tegra_gpio_mask_write(tgi, GPIO_MSK_OUT(tgi, offset), offset, value);
>> }
>>
>> static int tegra_gpio_get(struct gpio_chip *chip, unsigned offset)
>> {
>> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
>> + int bval = BIT(GPIO_BIT(offset));
>> +
>> /* If gpio is in output mode then read from the out value */
>> - if ((tegra_gpio_readl(GPIO_OE(offset)) >> GPIO_BIT(offset)) & 1)
>> - return (tegra_gpio_readl(GPIO_OUT(offset)) >>
>> - GPIO_BIT(offset)) & 0x1;
>> + if (tegra_gpio_readl(tgi, GPIO_OE(tgi, offset)) & bval)
>> + return !!(tegra_gpio_readl(tgi, GPIO_OUT(tgi, offset)) & bval);
>>
>> - return (tegra_gpio_readl(GPIO_IN(offset)) >> GPIO_BIT(offset)) & 0x1;
>> + return !!(tegra_gpio_readl(tgi, GPIO_IN(tgi, offset)) & bval);
>> }
>>
>> static int tegra_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>> {
>> - tegra_gpio_mask_write(GPIO_MSK_OE(offset), offset, 0);
>> - tegra_gpio_enable(offset);
>> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
>> +
>> + tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 0);
>> + tegra_gpio_enable(tgi, offset);
>> return 0;
>> }
>>
>> static int tegra_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
>> int value)
>> {
>> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
>> +
>> tegra_gpio_set(chip, offset, value);
>> - tegra_gpio_mask_write(GPIO_MSK_OE(offset), offset, 1);
>> - tegra_gpio_enable(offset);
>> + tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 1);
>> + tegra_gpio_enable(tgi, offset);
>> return 0;
>> }
>>
>> static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>> {
>> - return irq_find_mapping(irq_domain, offset);
>> + struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
>> +
>> + return irq_find_mapping(tgi->irq_domain, offset);
>> }
>>
>> static struct gpio_chip tegra_gpio_chip = {
>> @@ -184,29 +203,36 @@ static struct gpio_chip tegra_gpio_chip = {
>>
>> static void tegra_gpio_irq_ack(struct irq_data *d)
>> {
>> + struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> + struct tegra_gpio_info *tgi = bank->tgi;
>> int gpio = d->hwirq;
>>
>> - tegra_gpio_writel(1 << GPIO_BIT(gpio), GPIO_INT_CLR(gpio));
>> + tegra_gpio_writel(tgi, 1 << GPIO_BIT(gpio), GPIO_INT_CLR(tgi, gpio));
>> }
>>
>> static void tegra_gpio_irq_mask(struct irq_data *d)
>> {
>> + struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> + struct tegra_gpio_info *tgi = bank->tgi;
>> int gpio = d->hwirq;
>>
>> - tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 0);
>> + tegra_gpio_mask_write(tgi, GPIO_MSK_INT_ENB(tgi, gpio), gpio, 0);
>> }
>>
>> static void tegra_gpio_irq_unmask(struct irq_data *d)
>> {
>> + struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> + struct tegra_gpio_info *tgi = bank->tgi;
>> int gpio = d->hwirq;
>>
>> - tegra_gpio_mask_write(GPIO_MSK_INT_ENB(gpio), gpio, 1);
>> + tegra_gpio_mask_write(tgi, GPIO_MSK_INT_ENB(tgi, gpio), gpio, 1);
>> }
>>
>> static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> {
>> int gpio = d->hwirq;
>> struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> + struct tegra_gpio_info *tgi = bank->tgi;
>> int port = GPIO_PORT(gpio);
>> int lvl_type;
>> int val;
>> @@ -238,23 +264,24 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> return -EINVAL;
>> }
>>
>> - ret = gpiochip_lock_as_irq(&tegra_gpio_chip, gpio);
>> + ret = gpiochip_lock_as_irq(tgi->gc, gpio);
>> if (ret) {
>> - dev_err(dev, "unable to lock Tegra GPIO %d as IRQ\n", gpio);
>> + dev_err(tgi->dev,
>> + "unable to lock Tegra GPIO %d as IRQ\n", gpio);
>> return ret;
>> }
>>
>> spin_lock_irqsave(&bank->lvl_lock[port], flags);
>>
>> - val = tegra_gpio_readl(GPIO_INT_LVL(gpio));
>> + val = tegra_gpio_readl(tgi, GPIO_INT_LVL(tgi, gpio));
>> val &= ~(GPIO_INT_LVL_MASK << GPIO_BIT(gpio));
>> val |= lvl_type << GPIO_BIT(gpio);
>> - tegra_gpio_writel(val, GPIO_INT_LVL(gpio));
>> + tegra_gpio_writel(tgi, val, GPIO_INT_LVL(tgi, gpio));
>>
>> spin_unlock_irqrestore(&bank->lvl_lock[port], flags);
>>
>> - tegra_gpio_mask_write(GPIO_MSK_OE(gpio), gpio, 0);
>> - tegra_gpio_enable(gpio);
>> + tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, gpio), gpio, 0);
>> + tegra_gpio_enable(tgi, gpio);
>>
>> if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
>> irq_set_handler_locked(d, handle_level_irq);
>> @@ -266,9 +293,11 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>
>> static void tegra_gpio_irq_shutdown(struct irq_data *d)
>> {
>> + struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> + struct tegra_gpio_info *tgi = bank->tgi;
>> int gpio = d->hwirq;
>>
>> - gpiochip_unlock_as_irq(&tegra_gpio_chip, gpio);
>> + gpiochip_unlock_as_irq(tgi->gc, gpio);
>> }
>>
>> static void tegra_gpio_irq_handler(struct irq_desc *desc)
>> @@ -276,19 +305,24 @@ static void tegra_gpio_irq_handler(struct irq_desc *desc)
>> int port;
>> int pin;
>> int unmasked = 0;
>> + int gpio;
>> + u32 lvl;
>> + unsigned long sta;
> Why do you move these declarations here? They are not used outside of
> the loop IIUC.
Declaring and defining in single lime making this more than 80 column
when adding tgi.
Breaking them multiple lines does nto look good and hence move the
declaration outside of loop.
>>
>> + tgi->gc->parent = &pdev->dev;
>> + tgi->gc->of_node = pdev->dev.of_node;
> As said in 4/4, I really think gc should not be a pointer so several
> instances can coexist. Most other GPIO drivers follow this model.
>
> With these small details fixed, I believe this looks good!
>
> Reviewed-by: Alexandre Courbot <[email protected]>
OK will make that change and collect your RB.
On Monday 25 April 2016 11:06 AM, Alexandre Courbot wrote:
> Sorry, just realized I commented on v3...
>
> On Fri, Apr 22, 2016 at 7:09 PM, Laxman Dewangan <[email protected]> wrote:
>> + spinlock_t dbc_lock[4]; /* Lock for updating debounce count register */
> I'm nit'ing here, but maybe one spinlock shared by all ports would be
> enough? (the same would apply to lvl_lock, so feel free to do this as
> a separate patch) I don't think we expect *that* many concurrent
> accesses, do we?
Really no, but to make the stuff uniform, it should be fine here. If the
registers are not conflicting then do not make under same lock.
>>
>>
>> spin_lock_irqsave(bank->dbc_lock[port], flags);
>> if (bank->dbc_cnt[port] < debounce_ms) {
>> tegra_gpio_writel(tgi, debounce_ms, GPIO_DBC_CNT(tgi, offset));
>> bank->dbc_cnt[port] = debounce_ms;
>> }
>> spin_unlock_irqrestore(&bank->dbc_lock[port], flags);
>>
>> Which is nicer to the eyes.
>>
OK, this also looks fine. As I am goign to respin this for V5 (for gc as
instance rather than pointer), I will take care of it.
On Fri, Apr 22, 2016 at 03:39:13PM +0530, Laxman Dewangan wrote:
[...]
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
[...]
> static struct gpio_chip tegra_gpio_chip = {
This variable is still file-scoped. Why not get rid of it at the same
time? It's rather pointless to remove all file-scoped variables except a
single one, because now the driver still isn't properly equipped to deal
with multiple instances (however theoretical that may be).
Thierry
On Monday 25 April 2016 03:30 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Fri, Apr 22, 2016 at 03:39:13PM +0530, Laxman Dewangan wrote:
> [...]
>> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> [...]
>> static struct gpio_chip tegra_gpio_chip = {
> This variable is still file-scoped. Why not get rid of it at the same
> time? It's rather pointless to remove all file-scoped variables except a
> single one, because now the driver still isn't properly equipped to deal
> with multiple instances (however theoretical that may be).
>
Yaah, Alex also pointed that to have the instance of gpio_chip in
tegra_gpio_info and then do as
tgi->gc = tegra_gpio_chip.
same for irq_chip also.
I hope this will resolve the concern.
On Mon, Apr 25, 2016 at 03:23:22PM +0530, Laxman Dewangan wrote:
>
> On Monday 25 April 2016 03:30 PM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Fri, Apr 22, 2016 at 03:39:13PM +0530, Laxman Dewangan wrote:
> > [...]
> > > diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> > [...]
> > > static struct gpio_chip tegra_gpio_chip = {
> > This variable is still file-scoped. Why not get rid of it at the same
> > time? It's rather pointless to remove all file-scoped variables except a
> > single one, because now the driver still isn't properly equipped to deal
> > with multiple instances (however theoretical that may be).
> >
>
> Yaah, Alex also pointed that to have the instance of gpio_chip in
> tegra_gpio_info and then do as
> tgi->gc = tegra_gpio_chip.
>
>
> same for irq_chip also.
>
>
> I hope this will resolve the concern.
An alternative would be to do away with tegra_gpio_chip altogether and
instead assign the fields directly within ->probe(). Both patterns seem
to be used in drivers/gpio, with the latter one being slightly more
common.
Thierry