On Tue, Jan 13, 2015 at 4:40 PM, Linus Walleij [off-list ref] wrote:

On Wed, Dec 17, 2014 at 6:10 AM, Y Vo [off-list ref] wrote:

quoted

Add APM X-Gene standby GPIO controller driver.

Write something about what platform an arch this is and so
on, this is too terse commit message for a new driver.

quoted

Signed-off-by: Y Vo <redacted>

(...)

quoted

+config GPIO_XGENE_SB
+       tristate "APM X-Gene GPIO standby controller support"
+       depends on ARCH_XGENE && OF_GPIO

select GPIO_GENERIC

Why ?

see below on why.

quoted

+       help
+         This driver supports the GPIO block within the APM X-Gene
+         Standby Domain. Say yes here to enable the GPIO functionality.
+
 config GPIO_XILINX
        bool "Xilinx GPIO support"
        depends on PPC_OF || MICROBLAZE || ARCH_ZYNQ

diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
new file mode 100644
index 0000000..1248448

(...)

quoted

+++ b/drivers/gpio/gpio-xgene-sb.c
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>

#include <linux/gpio/driver.h>

instead.

quoted

+#include <linux/of_irq.h>
+#include <linux/acpi.h>

OK.

OK, why?

quoted

+#include <linux/efi.h>

I will remove.

Why?

quoted

+#include <linux/string.h>
+#include <linux/of_address.h>

I will remove.

#include <linux/basic_mmio_gpio.h>
when you start using GENERIC_GPIO

quoted

+
+#define XGENE_MAX_GPIO_DS              22
+#define XGENE_MAX_GPIO_DS_IRQ          6
+
+#define GPIO_MASK(x)                   (1U << ((x) % 32))
+#define GPIO_DIR_IN                    0
+#define GPIO_DIR_OUT                   1
+
+#define MPA_GPIO_INT_LVL               0x0290
+#define MPA_GPIO_OE_ADDR               0x029c
+#define MPA_GPIO_OUT_ADDR              0x02a0
+#define MPA_GPIO_IN_ADDR               0x02a4
+#define MPA_GPIO_SEL_LO                        0x0294
+#define MPA_GPIO_SEL_HIGH              0x029c
+
+struct xgene_gpio_sb {
+       struct of_mm_gpio_chip mm;
+       u32 *irq;
+       u32 nirq;
+       spinlock_t lock; /* mutual exclusion */
+};

Add kerneldoc to this struct.

quoted

+static void xgene_gpio_set_bit(void __iomem *reg, u32 gpio, int val)

(...)

quoted

+static int xgene_gpio_sb_get(struct gpio_chip *gc, u32 gpio)

This looks very much like a memory-mapped standard GPIO controller,
use drivers/gpio/gpio-generic.c with some IRQ add-on, compare
to any other driver selecting GPIO_GENERIC for guidance:
drivers/gpio/gpio-74xx-mmio.c, gpio-dwapb.c etc.

You mean I must rewrite to use GPIO_GENERIC follow gpio-74xx-mmio.c,
gpio-dwapb.c ?

quoted

+static int apm_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
+{
+       struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+       struct xgene_gpio_sb *chip = to_xgene_gpio_sb(mm_gc);
+
+       if (chip->irq[gpio])
+               return chip->irq[gpio];
+
+       return -ENXIO;
+}
+
+static int xgene_gpio_sb_probe(struct platform_device *pdev)
+{
+       struct of_mm_gpio_chip *mm;
+       struct xgene_gpio_sb *apm_gc;
+       u32 ret, i;
+       u32 default_pins[] = {0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D};

Um, pins... call these "lines" instead to avoid confusion with the pin
control subsystem.

I will change.

quoted

+       struct resource *res;
+
+       apm_gc = devm_kzalloc(&pdev->dev, sizeof(*apm_gc), GFP_KERNEL);
+       if (!apm_gc)
+               return -ENOMEM;
+
+       mm = &apm_gc->mm;
+       mm->gc.direction_input = xgene_gpio_sb_dir_in;
+       mm->gc.direction_output = xgene_gpio_sb_dir_out;
+       mm->gc.get = xgene_gpio_sb_get;
+       mm->gc.set = xgene_gpio_sb_set;
+       mm->gc.to_irq = apm_gpio_sb_to_irq;
+       mm->gc.base = -1;
+       mm->gc.label = dev_name(&pdev->dev);

gc has a device tree .np node and a .dev struct device *
pointer, assign them here. (Also after you switch to GENERIC_GPIO.)

Yours,
Linus Walleij