On Tue, Jan 13, 2015 at 4:40 PM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Wed, Dec 17, 2014 at 6:10 AM, Y Vo <yvo@xxxxxxx> wrote:
>
>> 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.
>
>> Signed-off-by: Y Vo <yvo@xxxxxxx>
> (...)
>
>> +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.
>
>> + 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
>
> (...)
>> +++ 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.
>
>> +#include <linux/of_irq.h>
>> +#include <linux/acpi.h>
OK.
> OK, why?
>
>> +#include <linux/efi.h>
I will remove.
> Why?
>
>> +#include <linux/string.h>
>> +#include <linux/of_address.h>
I will remove.
> #include <linux/basic_mmio_gpio.h>
> when you start using GENERIC_GPIO
>
>> +
>> +#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.
>
>> +static void xgene_gpio_set_bit(void __iomem *reg, u32 gpio, int val)
> (...)
>> +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 ?
>> +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.
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
