Skip to contentSkip to conversation

gpio-langwell updates

PATCH Applied David Cohen 2 participants 10 messages Dec 19, 2012 – Dec 20, 2012 GPIO

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore

David Cohen

author
#0 lore raw

cover gpio-langwell updates

Hi,

Here goes some Intel updates done on gpio-langwell driver.

Kind regards,

David
---

David Cohen (2):
gpio-langwell: cleanup driver
gpio-langwell: update pci device table

Li, Ning (1):
gpio-langwell: implement irq shutdown interface

 drivers/gpio/gpio-langwell.c |   80 ++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 41 deletions(-)
Signature · 1 line
1.7.10.4
author
#1 lore raw

PATCH 1/3 gpio-langwell: cleanup driver

This patch cleans up cosmetic issues, remove useless functions and add to_lnw_priv() macro to replace many usages of container_of().

Change-Id I70a8fadd20a42493271d91633739bdddff19c8d8
Signed-off-by David Cohen <[email protected]>
---
 drivers/gpio/gpio-langwell.c |   64 ++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 43 deletions(-)
drivers/gpio/gpio-langwell.c +21 -43
index 202a992..8220c04 100644
@@ -71,10 +71,12 @@ struct lnw_gpio {
 	struct irq_domain		*domain;
 };
 
+#define to_lnw_priv(chip)	container_of(chip, struct lnw_gpio, chip)
+
 static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
 			enum GPIO_REG reg_type)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	unsigned nreg = chip->ngpio / 32;
 	u8 reg = offset / 32;
 	void __iomem *ptr;

@@ -86,7 +88,7 @@ static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
 static void __iomem *gpio_reg_2bit(struct gpio_chip *chip, unsigned offset,
 				   enum GPIO_REG reg_type)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	unsigned nreg = chip->ngpio / 32;
 	u8 reg = offset / 16;
 	void __iomem *ptr;

@@ -130,7 +132,7 @@ static void lnw_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 
 static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
 	u32 value;
 	unsigned long flags;

@@ -153,7 +155,7 @@ static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 static int lnw_gpio_direction_output(struct gpio_chip *chip,
 			unsigned offset, int value)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
 	unsigned long flags;
 

@@ -176,7 +178,7 @@ static int lnw_gpio_direction_output(struct gpio_chip *chip,
 
 static int lnw_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	return irq_create_mapping(lnw->domain, offset);
 }
 

@@ -215,19 +217,14 @@ static int lnw_irq_type(struct irq_data *d, unsigned type)
 	return 0;
 }
 
-static void lnw_irq_unmask(struct irq_data *d)
-{
-}
-
-static void lnw_irq_mask(struct irq_data *d)
-{
-}
+static void lnw_irq_noop(struct irq_data *d) {}
 
 static struct irq_chip lnw_irqchip = {
 	.name		= "LNW-GPIO",
-	.irq_mask	= lnw_irq_mask,
-	.irq_unmask	= lnw_irq_unmask,
+	.irq_mask	= lnw_irq_noop,
+	.irq_unmask	= lnw_irq_noop,
 	.irq_set_type	= lnw_irq_type,
+	.irq_ack	= lnw_irq_noop,
 };
 
 static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = {   /* pin number */

@@ -299,17 +296,6 @@ static const struct irq_domain_ops lnw_gpio_irq_ops = {
 	.xlate = irq_domain_xlate_twocell,
 };
 
-#ifdef CONFIG_PM
-static int lnw_gpio_runtime_resume(struct device *dev)
-{
-	return 0;
-}
-
-static int lnw_gpio_runtime_suspend(struct device *dev)
-{
-	return 0;
-}
-
 static int lnw_gpio_runtime_idle(struct device *dev)
 {
 	int err = pm_schedule_suspend(dev, 500);

@@ -320,16 +306,8 @@ static int lnw_gpio_runtime_idle(struct device *dev)
 	return -EBUSY;
 }
 
-#else
-#define lnw_gpio_runtime_suspend	NULL
-#define lnw_gpio_runtime_resume		NULL
-#define lnw_gpio_runtime_idle		NULL
-#endif
-
 static const struct dev_pm_ops lnw_gpio_pm_ops = {
-	.runtime_suspend = lnw_gpio_runtime_suspend,
-	.runtime_resume = lnw_gpio_runtime_resume,
-	.runtime_idle = lnw_gpio_runtime_idle,
+	SET_RUNTIME_PM_OPS(NULL, NULL, lnw_gpio_runtime_idle)
 };
 
 static int __devinit lnw_gpio_probe(struct pci_dev *pdev,

@@ -349,7 +327,7 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
 	retval = pci_request_regions(pdev, "langwell_gpio");
 	if (retval) {
 		dev_err(&pdev->dev, "error requesting resources\n");
-		goto err2;
+		goto err;
 	}
 	/* get the gpio_base from bar1 */
 	start = pci_resource_start(pdev, 1);

@@ -358,7 +336,7 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
 	if (!base) {
 		dev_err(&pdev->dev, "error mapping bar1\n");
 		retval = -EFAULT;
-		goto err3;
+		goto err2;
 	}
 	gpio_base = *((u32 *)base + 1);
 	/* release the IO mapping, since we already get the info from bar1 */

@@ -370,21 +348,21 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
 	if (!base) {
 		dev_err(&pdev->dev, "error mapping bar0\n");
 		retval = -EFAULT;
-		goto err3;
+		goto err2;
 	}
 
-	lnw = devm_kzalloc(&pdev->dev, sizeof(struct lnw_gpio), GFP_KERNEL);
+	lnw = devm_kzalloc(&pdev->dev, sizeof(*lnw), GFP_KERNEL);
 	if (!lnw) {
 		dev_err(&pdev->dev, "can't allocate langwell_gpio chip data\n");
 		retval = -ENOMEM;
-		goto err3;
+		goto err2;
 	}
 
 	lnw->domain = irq_domain_add_linear(pdev->dev.of_node, ngpio,
 					    &lnw_gpio_irq_ops, lnw);
 	if (!lnw->domain) {
 		retval = -ENOMEM;
-		goto err3;
+		goto err2;
 	}
 
 	lnw->reg_base = base;

@@ -403,7 +381,7 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
 	retval = gpiochip_add(&lnw->chip);
 	if (retval) {
 		dev_err(&pdev->dev, "langwell gpiochip_add error %d\n", retval);
-		goto err3;
+		goto err2;
 	}
 
 	lnw_irq_init_hw(lnw);

@@ -418,9 +396,9 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
 
 	return 0;
 
-err3:
-	pci_release_regions(pdev);
 err2:
+	pci_release_regions(pdev);
+err:
 	pci_disable_device(pdev);
 	return retval;
 }
Signature · 1 line
1.7.10.4
#2 lore raw

Re: [PATCH 1/3] gpio-langwell: cleanup driver

Quoted 5 lines · David Cohen

This patch cleans up cosmetic issues, remove useless functions and add to_lnw_priv() macro to replace many usages of container_of().

Change-Id I70a8fadd20a42493271d91633739bdddff19c8d8
Signed-off-by David Cohen <[email protected]>

Hi David. Comments below...

Quoted 50 lines
---
 drivers/gpio/gpio-langwell.c |   64 ++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 43 deletions(-)
drivers/gpio/gpio-langwell.c +6 -4
index 202a992..8220c04 100644
@@ -71,10 +71,12 @@ struct lnw_gpio {
 	struct irq_domain		*domain;
 };
 
+#define to_lnw_priv(chip)	container_of(chip, struct lnw_gpio, chip)
+
 static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
 			enum GPIO_REG reg_type)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	unsigned nreg = chip->ngpio / 32;
 	u8 reg = offset / 32;
 	void __iomem *ptr;

@@ -86,7 +88,7 @@ static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
 static void __iomem *gpio_reg_2bit(struct gpio_chip *chip, unsigned offset,
 				   enum GPIO_REG reg_type)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	unsigned nreg = chip->ngpio / 32;
 	u8 reg = offset / 16;
 	void __iomem *ptr;

@@ -130,7 +132,7 @@ static void lnw_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 
 static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
 	u32 value;
 	unsigned long flags;

@@ -153,7 +155,7 @@ static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 static int lnw_gpio_direction_output(struct gpio_chip *chip,
 			unsigned offset, int value)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
 	unsigned long flags;

drivers/gpio/gpio-langwell.c static int lnw_gpio_direction_output(struct gpio_chip *chip, · line 183

Quoted 7 lines
drivers/gpio/gpio-langwell.c +1 -1
@@ -176,7 +178,7 @@ static int lnw_gpio_direction_output(struct gpio_chip *chip,
 
 static int lnw_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
 	return irq_create_mapping(lnw->domain, offset);
 }

Nice cleanup above.

drivers/gpio/gpio-langwell.c static int lnw_irq_type(struct irq_data *d, unsigned type) · line 220

Quoted 13 lines
drivers/gpio/gpio-langwell.c +1 -7
@@ -215,19 +217,14 @@ static int lnw_irq_type(struct irq_data *d, unsigned type)
 	return 0;
 }
 
-static void lnw_irq_unmask(struct irq_data *d)
-{
-}
-
-static void lnw_irq_mask(struct irq_data *d)
-{
-}
+static void lnw_irq_noop(struct irq_data *d) {}

Umm, this looks entirely wrong. There needs to be either mask/unmask or enable/disable ops for irq_chips. Yes I see that this patch is just consolidating two empty functions, but they are two empty functions that appear to be completely wrong.

Quoted 36 lines
drivers/gpio/gpio-langwell.c +3 -15
 static struct irq_chip lnw_irqchip = {
 	.name		= "LNW-GPIO",
-	.irq_mask	= lnw_irq_mask,
-	.irq_unmask	= lnw_irq_unmask,
+	.irq_mask	= lnw_irq_noop,
+	.irq_unmask	= lnw_irq_noop,
 	.irq_set_type	= lnw_irq_type,
+	.irq_ack	= lnw_irq_noop,
 };
 
 static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = {   /* pin number */

@@ -299,17 +296,6 @@ static const struct irq_domain_ops lnw_gpio_irq_ops = {
 	.xlate = irq_domain_xlate_twocell,
 };
 
-#ifdef CONFIG_PM
-static int lnw_gpio_runtime_resume(struct device *dev)
-{
-	return 0;
-}
-
-static int lnw_gpio_runtime_suspend(struct device *dev)
-{
-	return 0;
-}
-
 static int lnw_gpio_runtime_idle(struct device *dev)
 {
 	int err = pm_schedule_suspend(dev, 500);

@@ -320,16 +306,8 @@ static int lnw_gpio_runtime_idle(struct device *dev)
 	return -EBUSY;
 }
 
-#else
-#define lnw_gpio_runtime_suspend	NULL

drivers/gpio/gpio-langwell.c static int lnw_gpio_runtime_idle(struct device *dev) · line 311

Quoted 10 lines
drivers/gpio/gpio-langwell.c +1 -7
-#define lnw_gpio_runtime_resume		NULL
-#define lnw_gpio_runtime_idle		NULL
-#endif
-
 static const struct dev_pm_ops lnw_gpio_pm_ops = {
-	.runtime_suspend = lnw_gpio_runtime_suspend,
-	.runtime_resume = lnw_gpio_runtime_resume,
-	.runtime_idle = lnw_gpio_runtime_idle,
+	SET_RUNTIME_PM_OPS(NULL, NULL, lnw_gpio_runtime_idle)
 };

Also good.

drivers/gpio/gpio-langwell.c static int __devinit lnw_gpio_probe(struct pci_dev *pdev, · line 330

Quoted 8 lines
drivers/gpio/gpio-langwell.c +1 -1
 static int __devinit lnw_gpio_probe(struct pci_dev *pdev,

@@ -349,7 +327,7 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
 	retval = pci_request_regions(pdev, "langwell_gpio");
 	if (retval) {
 		dev_err(&pdev->dev, "error requesting resources\n");
-		goto err2;
+		goto err;

Renaming the labels just increases the noise in the diff. I prefer to see labels in the form "err-what-i-just-tried-to-do:" so they don't need to get reshuffled every time the code logic changes.

There is no good reason for this change. Please drop it.

g.

author
#3 lore raw

Re: [PATCH 1/3] gpio-langwell: cleanup driver

Quoted 7 lines · Grant Likely
Quoted 5 lines · David Cohen

This patch cleans up cosmetic issues, remove useless functions and add to_lnw_priv() macro to replace many usages of container_of().

Change-Id I70a8fadd20a42493271d91633739bdddff19c8d8
Signed-off-by David Cohen <[email protected]>

Hi David. Comments below...

Hi Grant,

Thanks for comments. Let me go through them.

Quoted 65 lines
Quoted 51 lines
---
  drivers/gpio/gpio-langwell.c |   64 ++++++++++++++----------------------------
  1 file changed, 21 insertions(+), 43 deletions(-)
drivers/gpio/gpio-langwell.c +6 -4
index 202a992..8220c04 100644
@@ -71,10 +71,12 @@ struct lnw_gpio {
  	struct irq_domain		*domain;
  };
 
+#define to_lnw_priv(chip)	container_of(chip, struct lnw_gpio, chip)
+
  static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
  			enum GPIO_REG reg_type)
  {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
  	unsigned nreg = chip->ngpio / 32;
  	u8 reg = offset / 32;
  	void __iomem *ptr;

@@ -86,7 +88,7 @@ static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
  static void __iomem *gpio_reg_2bit(struct gpio_chip *chip, unsigned offset,
  				   enum GPIO_REG reg_type)
  {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
  	unsigned nreg = chip->ngpio / 32;
  	u8 reg = offset / 16;
  	void __iomem *ptr;

@@ -130,7 +132,7 @@ static void lnw_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 
  static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
  {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
  	void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
  	u32 value;
  	unsigned long flags;

@@ -153,7 +155,7 @@ static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
  static int lnw_gpio_direction_output(struct gpio_chip *chip,
  			unsigned offset, int value)
  {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
  	void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
  	unsigned long flags;

drivers/gpio/gpio-langwell.c static int lnw_gpio_direction_output(struct gpio_chip *chip, · line 183

Quoted 7 lines
drivers/gpio/gpio-langwell.c +1 -1
@@ -176,7 +178,7 @@ static int lnw_gpio_direction_output(struct gpio_chip *chip,
 
  static int lnw_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
  {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
+	struct lnw_gpio *lnw = to_lnw_priv(chip);
  	return irq_create_mapping(lnw->domain, offset);
  }

Nice cleanup above.

Quoted 5 lines
drivers/gpio/gpio-langwell.c +0 -0
@@ -215,19 +217,14 @@ static int lnw_irq_type(struct irq_data *d, unsigned type)
  	return 0;
  }
Quoted 12 lines

drivers/gpio/gpio-langwell.c static int lnw_irq_type(struct irq_data *d, unsigned type) · line 220

Quoted 8 lines
drivers/gpio/gpio-langwell.c +1 -7
-static void lnw_irq_unmask(struct irq_data *d)
-{
-}
-
-static void lnw_irq_mask(struct irq_data *d)
-{
-}
+static void lnw_irq_noop(struct irq_data *d) {}

Umm, this looks entirely wrong. There needs to be either mask/unmask or enable/disable ops for irq_chips. Yes I see that this patch is just consolidating two empty functions, but they are two empty functions that appear to be completely wrong.

I see your point. The solution does not belong to a clean up patch, so I'll just remove it from here.

Quoted 61 lines
Quoted 37 lines
drivers/gpio/gpio-langwell.c +3 -15
  static struct irq_chip lnw_irqchip = {
  	.name		= "LNW-GPIO",
-	.irq_mask	= lnw_irq_mask,
-	.irq_unmask	= lnw_irq_unmask,
+	.irq_mask	= lnw_irq_noop,
+	.irq_unmask	= lnw_irq_noop,
  	.irq_set_type	= lnw_irq_type,
+	.irq_ack	= lnw_irq_noop,
  };
 
  static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = {   /* pin number */

@@ -299,17 +296,6 @@ static const struct irq_domain_ops lnw_gpio_irq_ops = {
  	.xlate = irq_domain_xlate_twocell,
  };
 
-#ifdef CONFIG_PM
-static int lnw_gpio_runtime_resume(struct device *dev)
-{
-	return 0;
-}
-
-static int lnw_gpio_runtime_suspend(struct device *dev)
-{
-	return 0;
-}
-
  static int lnw_gpio_runtime_idle(struct device *dev)
  {
  	int err = pm_schedule_suspend(dev, 500);

@@ -320,16 +306,8 @@ static int lnw_gpio_runtime_idle(struct device *dev)
  	return -EBUSY;
  }
 
-#else
-#define lnw_gpio_runtime_suspend	NULL

drivers/gpio/gpio-langwell.c static int lnw_gpio_runtime_idle(struct device *dev) · line 311

Quoted 10 lines
drivers/gpio/gpio-langwell.c +1 -7
-#define lnw_gpio_runtime_resume		NULL
-#define lnw_gpio_runtime_idle		NULL
-#endif
-
  static const struct dev_pm_ops lnw_gpio_pm_ops = {
-	.runtime_suspend = lnw_gpio_runtime_suspend,
-	.runtime_resume = lnw_gpio_runtime_resume,
-	.runtime_idle = lnw_gpio_runtime_idle,
+	SET_RUNTIME_PM_OPS(NULL, NULL, lnw_gpio_runtime_idle)
  };

Also good.

drivers/gpio/gpio-langwell.c static int __devinit lnw_gpio_probe(struct pci_dev *pdev, · line 330

Quoted 8 lines
drivers/gpio/gpio-langwell.c +1 -1
  static int __devinit lnw_gpio_probe(struct pci_dev *pdev,

@@ -349,7 +327,7 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
  	retval = pci_request_regions(pdev, "langwell_gpio");
  	if (retval) {
  		dev_err(&pdev->dev, "error requesting resources\n");
-		goto err2;
+		goto err;

Renaming the labels just increases the noise in the diff. I prefer to see labels in the form "err-what-i-just-tried-to-do:" so they don't need to get reshuffled every time the code logic changes.

Quoted 1 line

There is no good reason for this change. Please drop it.

Maybe instead of drop it I'd prefer to fix the labels. They are wrong currently.

Br, David

Quoted 4 lines

g.
.

#4 lore raw

Re: [PATCH 1/3] gpio-langwell: cleanup driver

Quoted 21 lines · David Cohen
Quoted 10 lines · Grant Likely

On Tue, 18 Dec 2012 17:52:11 -0800, David Cohen <[email protected]>

Quoted 9 lines

@@ -215,19 +217,14 @@ static int lnw_irq_type(struct irq_data *d,
unsigned type)

        return 0;

}
-static void lnw_irq_unmask(struct irq_data *d)

drivers/gpio/gpio-langwell.c +0 -4
-{
-}
-
-static void lnw_irq_mask(struct irq_data *d)
Quoted 8 lines

drivers/gpio/gpio-langwell.c static int lnw_irq_type(struct irq_data *d, unsigned type) · line 220

Quoted 3 lines
drivers/gpio/gpio-langwell.c +1 -2
-{
-}
+static void lnw_irq_noop(struct irq_data *d) {}

Umm, this looks entirely wrong. There needs to be either mask/unmask or enable/disable ops for irq_chips. Yes I see that this patch is just consolidating two empty functions, but they are two empty functions that appear to be completely wrong.

Quoted 2 lines

I see your point. The solution does not belong to a clean up patch, so I'll just remove it from here.

Right, that is appropriate.

Quoted 13 lines
Quoted 13 lines

drivers/gpio/gpio-langwell.c static int __devinit lnw_gpio_probe(struct pci_dev · line 331

Quoted 8 lines

static int __devinit lnw_gpio_probe(struct pci_dev *pdev,

drivers/gpio/gpio-langwell.c +1 -1
@@ -349,7 +327,7 @@ static int __devinit lnw_gpio_probe(struct pci_dev
 *pdev,
        retval = pci_request_regions(pdev, "langwell_gpio");
        if (retval) {
                dev_err(&pdev->dev, "error requesting resources\n");
-               goto err2;
+               goto err;

Renaming the labels just increases the noise in the diff. I prefer to see labels in the form "err-what-i-just-tried-to-do:" so they don't need to get reshuffled every time the code logic changes.

Quoted 5 lines
Quoted 1 line

There is no good reason for this change. Please drop it.

Maybe instead of drop it I'd prefer to fix the labels. They are wrong currently.

If there is real breakage here, then split it off into a separate bug fix patch. Otherwise I really wouldn't bother.

g.

author
#5 lore raw

PATCH 2/3 gpio-langwell: update pci device table

In reply to David Cohen

This patch adds Cloverview ids to pci device table.

Signed-off-by David Cohen <[email protected]>
---
 drivers/gpio/gpio-langwell.c |    2 ++
 1 file changed, 2 insertions(+)
drivers/gpio/gpio-langwell.c +2 -0
index 8220c04..dc534a9 100644
@@ -231,6 +231,8 @@ static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = {   /* pin number */
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x080f), .driver_data = 64 },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x081f), .driver_data = 96 },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x081a), .driver_data = 96 },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x08eb), .driver_data = 96 },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x08f7), .driver_data = 96 },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, lnw_gpio_ids);
Signature · 1 line
1.7.10.4
Applied
#6 lore raw

Re: [PATCH 2/3] gpio-langwell: update pci device table

Quoted 3 lines · David Cohen

This patch adds Cloverview ids to pci device table.

Signed-off-by David Cohen <[email protected]>

Applied, thanks.

g.

Quoted 20 lines
---
 drivers/gpio/gpio-langwell.c |    2 ++
 1 file changed, 2 insertions(+)
drivers/gpio/gpio-langwell.c +2 -0
index 8220c04..dc534a9 100644
@@ -231,6 +231,8 @@ static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = {   /* pin number */
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x080f), .driver_data = 64 },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x081f), .driver_data = 96 },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x081a), .driver_data = 96 },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x08eb), .driver_data = 96 },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x08f7), .driver_data = 96 },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, lnw_gpio_ids);
Signature · 1 line
1.7.10.4
Signature · 2 lines
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
author
#7 lore raw

PATCH 3/3 gpio-langwell: implement irq shutdown interface

In reply to David Cohen

From: "Li, Ning" <[email protected]>

Signed-off-by David Cohen <[email protected]>
Signed-off-by Li, Ning <[email protected]>
---
 drivers/gpio/gpio-langwell.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
drivers/gpio/gpio-langwell.c +18 -0
index dc534a9..c702d3d 100644
@@ -219,12 +219,30 @@ static int lnw_irq_type(struct irq_data *d, unsigned type)
 
 static void lnw_irq_noop(struct irq_data *d) {}
 
+static void lnw_irq_shutdown(struct irq_data *d)
+{
+	struct lnw_gpio *lnw = irq_data_get_irq_chip_data(d);
+	u32 gpio = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 value;
+	void __iomem *grer = gpio_reg(&lnw->chip, gpio, GRER);
+	void __iomem *gfer = gpio_reg(&lnw->chip, gpio, GFER);
+
+	spin_lock_irqsave(&lnw->lock, flags);
+	value = readl(grer) & ~BIT(gpio % 32);
+	writel(value, grer);
+	value = readl(gfer) & ~BIT(gpio % 32);
+	writel(value, gfer);
+	spin_unlock_irqrestore(&lnw->lock, flags);
+};
+
 static struct irq_chip lnw_irqchip = {
 	.name		= "LNW-GPIO",
 	.irq_mask	= lnw_irq_noop,
 	.irq_unmask	= lnw_irq_noop,
 	.irq_set_type	= lnw_irq_type,
 	.irq_ack	= lnw_irq_noop,
+	.irq_shutdown	= lnw_irq_shutdown,
 };
 
 static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = {   /* pin number */
Signature · 1 line
1.7.10.4
#8 lore raw

Re: [PATCH 3/3] gpio-langwell: implement irq shutdown interface

Quoted 4 lines · David Cohen

From: "Li, Ning" <[email protected]>

Signed-off-by David Cohen <[email protected]>
Signed-off-by Li, Ning <[email protected]>

I could use some help interpreting this patch since it doesn't have any commit text describing why this patch is needed, nor does it have any comments in the code describing what it is doing.

Having good commit text is important. Use it to tell me and future readers why this patch is important. What it does and how it was tested. Otherwise I end up trying to read your mind.

(The patch does actually look fine to me, but it isn't okay to neglect a patch descirption on anything other than the most trivial of patches... and even then it triggers severe eyebrow raising)

g.

Quoted 42 lines
---
 drivers/gpio/gpio-langwell.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
drivers/gpio/gpio-langwell.c +18 -0
index dc534a9..c702d3d 100644
@@ -219,12 +219,30 @@ static int lnw_irq_type(struct irq_data *d, unsigned type)
 
 static void lnw_irq_noop(struct irq_data *d) {}
 
+static void lnw_irq_shutdown(struct irq_data *d)
+{
+	struct lnw_gpio *lnw = irq_data_get_irq_chip_data(d);
+	u32 gpio = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 value;
+	void __iomem *grer = gpio_reg(&lnw->chip, gpio, GRER);
+	void __iomem *gfer = gpio_reg(&lnw->chip, gpio, GFER);
+
+	spin_lock_irqsave(&lnw->lock, flags);
+	value = readl(grer) & ~BIT(gpio % 32);
+	writel(value, grer);
+	value = readl(gfer) & ~BIT(gpio % 32);
+	writel(value, gfer);
+	spin_unlock_irqrestore(&lnw->lock, flags);
+};
+
 static struct irq_chip lnw_irqchip = {
 	.name		= "LNW-GPIO",
 	.irq_mask	= lnw_irq_noop,
 	.irq_unmask	= lnw_irq_noop,
 	.irq_set_type	= lnw_irq_type,
 	.irq_ack	= lnw_irq_noop,
+	.irq_shutdown	= lnw_irq_shutdown,
 };
 
 static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = {   /* pin number */
Signature · 1 line
1.7.10.4
Signature · 2 lines
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
author
#9 lore raw

Re: [PATCH 3/3] gpio-langwell: implement irq shutdown interface

Quoted 9 lines · Grant Likely
Quoted 4 lines · David Cohen

From: "Li, Ning" <[email protected]>

Signed-off-by David Cohen <[email protected]>
Signed-off-by Li, Ning <[email protected]>

I could use some help interpreting this patch since it doesn't have any commit text describing why this patch is needed, nor does it have any comments in the code describing what it is doing.

Quoted 7 lines

Having good commit text is important. Use it to tell me and future readers why this patch is important. What it does and how it was tested. Otherwise I end up trying to read your mind.

(The patch does actually look fine to me, but it isn't okay to neglect a patch descirption on anything other than the most trivial of patches... and even then it triggers severe eyebrow raising)

Thanks for your reply. I'll add the description and resend it.

Br, David

Quoted 45 lines

g.

Quoted 42 lines
---
  drivers/gpio/gpio-langwell.c |   18 ++++++++++++++++++
  1 file changed, 18 insertions(+)
drivers/gpio/gpio-langwell.c +18 -0
index dc534a9..c702d3d 100644
@@ -219,12 +219,30 @@ static int lnw_irq_type(struct irq_data *d, unsigned type)
 
  static void lnw_irq_noop(struct irq_data *d) {}
 
+static void lnw_irq_shutdown(struct irq_data *d)
+{
+	struct lnw_gpio *lnw = irq_data_get_irq_chip_data(d);
+	u32 gpio = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 value;
+	void __iomem *grer = gpio_reg(&lnw->chip, gpio, GRER);
+	void __iomem *gfer = gpio_reg(&lnw->chip, gpio, GFER);
+
+	spin_lock_irqsave(&lnw->lock, flags);
+	value = readl(grer) & ~BIT(gpio % 32);
+	writel(value, grer);
+	value = readl(gfer) & ~BIT(gpio % 32);
+	writel(value, gfer);
+	spin_unlock_irqrestore(&lnw->lock, flags);
+};
+
  static struct irq_chip lnw_irqchip = {
  	.name		= "LNW-GPIO",
  	.irq_mask	= lnw_irq_noop,
  	.irq_unmask	= lnw_irq_noop,
  	.irq_set_type	= lnw_irq_type,
  	.irq_ack	= lnw_irq_noop,
+	.irq_shutdown	= lnw_irq_shutdown,
  };
 
  static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = {   /* pin number */
Signature · 1 line
1.7.10.4