[PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Masahiro Yamada-2
This GPIO controller device is used on UniPhier SoCs.

Signed-off-by: Masahiro Yamada <[hidden email]>
---

Changes in v3:
  - Use module_platform_driver()

Changes in v2:
  - Fix typos in the comment block

 drivers/gpio/Kconfig         |   6 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-uniphier.c | 262 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/gpio/gpio-uniphier.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 4c9fa58..37cb4f2 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -462,6 +462,12 @@ config GPIO_TZ1090_PDC
  help
   Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs.
 
+config GPIO_UNIPHIER
+ tristate "UniPhier GPIO"
+ depends on ARCH_UNIPHIER && OF_GPIO
+ help
+  Say yes here to support UniPhier GPIOs.
+
 config GPIO_VF610
  def_bool y
  depends on ARCH_MXC && SOC_VF610
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index f82cd67..3538b0c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -102,6 +102,7 @@ obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o
 obj-$(CONFIG_GPIO_TZ1090) += gpio-tz1090.o
 obj-$(CONFIG_GPIO_TZ1090_PDC) += gpio-tz1090-pdc.o
 obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
+obj-$(CONFIG_GPIO_UNIPHIER) += gpio-uniphier.o
 obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o
 obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o
 obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o
diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
new file mode 100644
index 0000000..1afacaa
--- /dev/null
+++ b/drivers/gpio/gpio-uniphier.c
@@ -0,0 +1,262 @@
+/*
+ * Copyright (C) 2015 Masahiro Yamada <[hidden email]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+/*
+ * Unfortunately, the hardware specification adopts weird GPIO pin labeling.
+ * The ports are named as
+ *   PORT00,  PORT01,  PORT02,  ..., PORT07,
+ *   PORT10,  PORT11,  PORT12,  ..., PORT17,
+ *   PORT20,  PORT21,  PORT22,  ..., PORT27,
+ *    ...
+ *   PORT90,  PORT91,  PORT92,  ..., PORT97,
+ *   PORT100, PORT101, PORT102, ..., PORT107,
+ *    ...
+ *
+ * The PORTs with 8 or 9 in the one's place are missing, i.e. the one's place
+ * is octal, while the other places are decimal.  If we handle the port numbers
+ * as seen in the hardware documents, the GPIO offsets must be non-contiguous.
+ * It is possible to have sparse GPIO pins, but not handy for GPIO range
+ * mappings, register accessing, etc.
+ *
+ * To make things simpler (for driver and device tree implementation), this
+ * driver takes contiguously-numbered GPIO offsets.  GPIO consumers should make
+ * sure to convert the PORT number into the one that fits in this driver.
+ * The conversion logic is very easy math, for example,
+ *   PORT15  -->  GPIO offset 13   (8 * 1 + 5)
+ *   PORT123 -->  GPIO offset 99   (8 * 12 + 3)
+ */
+#define UNIPHIER_GPIO_PORTS_PER_BANK 8
+#define UNIPHIER_GPIO_BANK_MASK \
+ ((1UL << (UNIPHIER_GPIO_PORTS_PER_BANK)) - 1)
+
+#define UNIPHIER_GPIO_REG_DATA 0 /* data */
+#define UNIPHIER_GPIO_REG_DIR 4 /* direction (1:in, 0:out) */
+
+struct uniphier_gpio_priv {
+ struct of_mm_gpio_chip mmchip;
+ spinlock_t lock;
+};
+
+static unsigned uniphier_gpio_bank_to_reg(unsigned bank, unsigned reg_type)
+{
+ unsigned reg;
+
+ reg = (bank + 1) * 8 + reg_type;
+
+ /*
+ * Unfortunately, there is a register hole at offset 0x90-0x9f.
+ * Add 0x10 when crossing the hole.
+ */
+ if (reg >= 0x90)
+ reg += 0x10;
+
+ return reg;
+}
+
+static void uniphier_gpio_bank_write(struct gpio_chip *chip,
+     unsigned bank, unsigned reg_type,
+     unsigned mask, unsigned value)
+{
+ struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
+ struct uniphier_gpio_priv *priv;
+ unsigned long flags;
+ unsigned reg;
+ u32 tmp;
+
+ if (!mask)
+ return;
+
+ priv = container_of(mmchip, struct uniphier_gpio_priv, mmchip);
+
+ reg = uniphier_gpio_bank_to_reg(bank, reg_type);
+
+ /*
+ * Note
+ * regmap_update_bits() should not be used here.
+ *
+ * The DATA registers return the current readback of pins, not the
+ * previously written data when they are configured as "input".
+ * The DATA registers must be overwritten even if the data you are
+ * going to write is the same as what readl() has returned.
+ *
+ * regmap_update_bits() does not write back if the data is not changed.
+ */
+ spin_lock_irqsave(&priv->lock, flags);
+ tmp = readl(mmchip->regs + reg);
+ tmp &= ~mask;
+ tmp |= mask & value;
+ writel(tmp, mmchip->regs + reg);
+ spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static void uniphier_gpio_offset_write(struct gpio_chip *chip, unsigned offset,
+       unsigned reg_type, int value)
+{
+ unsigned bank = offset / UNIPHIER_GPIO_PORTS_PER_BANK;
+ unsigned bit = offset % UNIPHIER_GPIO_PORTS_PER_BANK;
+
+ uniphier_gpio_bank_write(chip, bank, reg_type, BIT(bit), value << bit);
+}
+
+static int uniphier_gpio_offset_read(struct gpio_chip *chip, unsigned offset,
+     unsigned reg_type)
+{
+ struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
+ unsigned bank = offset / UNIPHIER_GPIO_PORTS_PER_BANK;
+ unsigned bit = offset % UNIPHIER_GPIO_PORTS_PER_BANK;
+ unsigned reg;
+
+ reg = uniphier_gpio_bank_to_reg(bank, reg_type);
+
+ return readl(mmchip->regs + reg) & BIT(bit) ? 1 : 0;
+}
+
+static int uniphier_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+ return pinctrl_request_gpio(chip->base + offset);
+}
+
+static void uniphier_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+ pinctrl_free_gpio(chip->base + offset);
+}
+
+static int uniphier_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+ return uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset) ?
+ GPIOF_DIR_IN : GPIOF_DIR_OUT;
+}
+
+static int uniphier_gpio_direction_input(struct gpio_chip *chip,
+ unsigned offset)
+{
+ uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_REG_DIR, 1);
+
+ return 0;
+}
+
+static int uniphier_gpio_direction_output(struct gpio_chip *chip,
+  unsigned offset, int value)
+{
+ uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_REG_DATA, value);
+ uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_REG_DIR, 0);
+
+ return 0;
+}
+
+static int uniphier_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+ return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA);
+}
+
+static void uniphier_gpio_set(struct gpio_chip *chip,
+      unsigned offset, int value)
+{
+ uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_REG_DATA, value);
+}
+
+static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
+       unsigned long *mask,
+       unsigned long *bits)
+{
+ unsigned bank, shift, bank_mask, bank_bits;
+ int i;
+
+ for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) {
+ bank = i / UNIPHIER_GPIO_PORTS_PER_BANK;
+ shift = i % BITS_PER_LONG;
+ bank_mask = (mask[BIT_WORD(i)] >> shift) &
+ UNIPHIER_GPIO_BANK_MASK;
+ bank_bits = bits[BIT_WORD(i)] >> shift;
+
+ uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA,
+ bank_mask, bank_bits);
+ }
+}
+
+static int uniphier_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct uniphier_gpio_priv *priv;
+ u32 ngpio;
+ int ret;
+
+ ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio);
+ if (ret) {
+ dev_err(dev, "failed to get ngpio property\n");
+ return ret;
+ }
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ spin_lock_init(&priv->lock);
+
+ priv->mmchip.gc.dev = dev;
+ priv->mmchip.gc.owner = THIS_MODULE;
+ priv->mmchip.gc.request = uniphier_gpio_request;
+ priv->mmchip.gc.free = uniphier_gpio_free;
+ priv->mmchip.gc.get_direction = uniphier_gpio_get_direction;
+ priv->mmchip.gc.direction_input = uniphier_gpio_direction_input;
+ priv->mmchip.gc.direction_output = uniphier_gpio_direction_output;
+ priv->mmchip.gc.get = uniphier_gpio_get;
+ priv->mmchip.gc.set = uniphier_gpio_set;
+ priv->mmchip.gc.set_multiple = uniphier_gpio_set_multiple;
+ priv->mmchip.gc.ngpio = ngpio;
+
+ ret = of_mm_gpiochip_add(dev->of_node, &priv->mmchip);
+ if (ret) {
+ dev_err(dev, "failed to add memory mapped gpiochip\n");
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static int uniphier_gpio_remove(struct platform_device *pdev)
+{
+ struct uniphier_gpio_priv *priv = platform_get_drvdata(pdev);
+
+ of_mm_gpiochip_remove(&priv->mmchip);
+
+ return 0;
+}
+
+static const struct of_device_id uniphier_gpio_match[] = {
+ { .compatible = "socionext,uniphier-gpio" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, uniphier_gpio_match);
+
+static struct platform_driver uniphier_gpio_driver = {
+ .probe = uniphier_gpio_probe,
+ .remove = uniphier_gpio_remove,
+ .driver = {
+ .name = "uniphier-gpio",
+ .of_match_table = uniphier_gpio_match,
+ },
+};
+module_platform_driver(uniphier_gpio_driver);
+
+MODULE_AUTHOR("Masahiro Yamada <[hidden email]>");
+MODULE_DESCRIPTION("UniPhier GPIO driver");
+MODULE_LICENSE("GPL");
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Linus Walleij
On Tue, Jul 14, 2015 at 4:43 AM, Masahiro Yamada
<[hidden email]> wrote:

> This GPIO controller device is used on UniPhier SoCs.
>
> Signed-off-by: Masahiro Yamada <[hidden email]>
> ---
>
> Changes in v3:
>   - Use module_platform_driver()
>
> Changes in v2:
>   - Fix typos in the comment block

OK why no device tree bindings? Are they in a separate patch?

> +/*
> + * Unfortunately, the hardware specification adopts weird GPIO pin labeling.
> + * The ports are named as
> + *   PORT00,  PORT01,  PORT02,  ..., PORT07,
> + *   PORT10,  PORT11,  PORT12,  ..., PORT17,
> + *   PORT20,  PORT21,  PORT22,  ..., PORT27,
> + *    ...
> + *   PORT90,  PORT91,  PORT92,  ..., PORT97,
> + *   PORT100, PORT101, PORT102, ..., PORT107,
> + *    ...
> + *
> + * The PORTs with 8 or 9 in the one's place are missing, i.e. the one's place
> + * is octal, while the other places are decimal.  If we handle the port numbers
> + * as seen in the hardware documents, the GPIO offsets must be non-contiguous.
> + * It is possible to have sparse GPIO pins, but not handy for GPIO range
> + * mappings, register accessing, etc.
> + *
> + * To make things simpler (for driver and device tree implementation), this
> + * driver takes contiguously-numbered GPIO offsets.  GPIO consumers should make
> + * sure to convert the PORT number into the one that fits in this driver.
> + * The conversion logic is very easy math, for example,
> + *   PORT15  -->  GPIO offset 13   (8 * 1 + 5)
> + *   PORT123 -->  GPIO offset 99   (8 * 12 + 3)
> + */
> +#define UNIPHIER_GPIO_PORTS_PER_BANK   8
> +#define UNIPHIER_GPIO_BANK_MASK                \
> +                               ((1UL << (UNIPHIER_GPIO_PORTS_PER_BANK)) - 1)



> +
> +#define UNIPHIER_GPIO_REG_DATA         0       /* data */
> +#define UNIPHIER_GPIO_REG_DIR          4       /* direction (1:in, 0:out) */
> +
> +struct uniphier_gpio_priv {
> +       struct of_mm_gpio_chip mmchip;
> +       spinlock_t lock;
> +};
> +
> +static unsigned uniphier_gpio_bank_to_reg(unsigned bank, unsigned reg_type)
> +{
> +       unsigned reg;
> +
> +       reg = (bank + 1) * 8 + reg_type;
> +
> +       /*
> +        * Unfortunately, there is a register hole at offset 0x90-0x9f.
> +        * Add 0x10 when crossing the hole.
> +        */
> +       if (reg >= 0x90)
> +               reg += 0x10;
> +
> +       return reg;
> +}
> +
> +static void uniphier_gpio_bank_write(struct gpio_chip *chip,
> +                                    unsigned bank, unsigned reg_type,
> +                                    unsigned mask, unsigned value)
> +{
> +       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
> +       struct uniphier_gpio_priv *priv;
> +       unsigned long flags;
> +       unsigned reg;
> +       u32 tmp;
> +
> +       if (!mask)
> +               return;
> +
> +       priv = container_of(mmchip, struct uniphier_gpio_priv, mmchip);
> +
> +       reg = uniphier_gpio_bank_to_reg(bank, reg_type);
> +
> +       /*
> +        * Note
> +        * regmap_update_bits() should not be used here.
> +        *
> +        * The DATA registers return the current readback of pins, not the
> +        * previously written data when they are configured as "input".
> +        * The DATA registers must be overwritten even if the data you are
> +        * going to write is the same as what readl() has returned.
> +        *
> +        * regmap_update_bits() does not write back if the data is not changed.
> +        */

Why is this mentioned when the driver doesn't even use regmap?
Development artifact?

> +static int uniphier_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> +       return uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset) ?
> +                                               GPIOF_DIR_IN : GPIOF_DIR_OUT;

Just use
return !!uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset);

> +static int uniphier_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA);

return !!uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA);

> +static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
> +                                      unsigned long *mask,
> +                                      unsigned long *bits)
> +{
> +       unsigned bank, shift, bank_mask, bank_bits;
> +       int i;
> +
> +       for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) {
> +               bank = i / UNIPHIER_GPIO_PORTS_PER_BANK;
> +               shift = i % BITS_PER_LONG;
> +               bank_mask = (mask[BIT_WORD(i)] >> shift) &
> +                                               UNIPHIER_GPIO_BANK_MASK;
> +               bank_bits = bits[BIT_WORD(i)] >> shift;
> +
> +               uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA,
> +                                        bank_mask, bank_bits);
> +       }

This looks like a piece of algorithm that we could make generic like in a
static function in drivers/gpio/gpiolib.h or so, that it may be shared with
other drivers. Do you see some clear way to break out the core of this?
Or is it as generic as I think?

> +static int uniphier_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct uniphier_gpio_priv *priv;
> +       u32 ngpio;
> +       int ret;
> +
> +       ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio);
> +       if (ret) {
> +               dev_err(dev, "failed to get ngpio property\n");
> +               return ret;
> +       }

This needs to be documented, plus I don't see why it's needed.
The driver for this very specific hardware should already know
how many GPIOs there are in this hardware, it should not come
from the device tree.

ngpio is typically for the case where the hardware has 32 bits
for GPIO pins, but only the first 11 are actually used in the hardware.

> +static const struct of_device_id uniphier_gpio_match[] = {
> +       { .compatible = "socionext,uniphier-gpio" },
> +       { /* sentinel */ }
> +};

Needs a binding document.

Apart from this it looks nice!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Masahiro Yamada-2
Hi Linus,


2015-07-15 23:15 GMT+09:00 Linus Walleij <[hidden email]>:

> On Tue, Jul 14, 2015 at 4:43 AM, Masahiro Yamada
> <[hidden email]> wrote:
>
>> This GPIO controller device is used on UniPhier SoCs.
>>
>> Signed-off-by: Masahiro Yamada <[hidden email]>
>> ---
>>
>> Changes in v3:
>>   - Use module_platform_driver()
>>
>> Changes in v2:
>>   - Fix typos in the comment block
>
> OK why no device tree bindings? Are they in a separate patch?


Sorry, I was planning to do it later.

OK.  I will come back with
Documentation/devicetree/bindings/gpio/uniphier-gpio.txt in binding info in it.


>> +/*
>> + * Unfortunately, the hardware specification adopts weird GPIO pin labeling.
>> + * The ports are named as
>> + *   PORT00,  PORT01,  PORT02,  ..., PORT07,
>> + *   PORT10,  PORT11,  PORT12,  ..., PORT17,
>> + *   PORT20,  PORT21,  PORT22,  ..., PORT27,
>> + *    ...
>> + *   PORT90,  PORT91,  PORT92,  ..., PORT97,
>> + *   PORT100, PORT101, PORT102, ..., PORT107,
>> + *    ...
>> + *
>> + * The PORTs with 8 or 9 in the one's place are missing, i.e. the one's place
>> + * is octal, while the other places are decimal.  If we handle the port numbers
>> + * as seen in the hardware documents, the GPIO offsets must be non-contiguous.
>> + * It is possible to have sparse GPIO pins, but not handy for GPIO range
>> + * mappings, register accessing, etc.
>> + *
>> + * To make things simpler (for driver and device tree implementation), this
>> + * driver takes contiguously-numbered GPIO offsets.  GPIO consumers should make
>> + * sure to convert the PORT number into the one that fits in this driver.
>> + * The conversion logic is very easy math, for example,
>> + *   PORT15  -->  GPIO offset 13   (8 * 1 + 5)
>> + *   PORT123 -->  GPIO offset 99   (8 * 12 + 3)
>> + */
>> +#define UNIPHIER_GPIO_PORTS_PER_BANK   8
>> +#define UNIPHIER_GPIO_BANK_MASK                \
>> +                               ((1UL << (UNIPHIER_GPIO_PORTS_PER_BANK)) - 1)
>
>
>
>> +
>> +#define UNIPHIER_GPIO_REG_DATA         0       /* data */
>> +#define UNIPHIER_GPIO_REG_DIR          4       /* direction (1:in, 0:out) */
>> +
>> +struct uniphier_gpio_priv {
>> +       struct of_mm_gpio_chip mmchip;
>> +       spinlock_t lock;
>> +};
>> +
>> +static unsigned uniphier_gpio_bank_to_reg(unsigned bank, unsigned reg_type)
>> +{
>> +       unsigned reg;
>> +
>> +       reg = (bank + 1) * 8 + reg_type;
>> +
>> +       /*
>> +        * Unfortunately, there is a register hole at offset 0x90-0x9f.
>> +        * Add 0x10 when crossing the hole.
>> +        */
>> +       if (reg >= 0x90)
>> +               reg += 0x10;
>> +
>> +       return reg;
>> +}
>> +
>> +static void uniphier_gpio_bank_write(struct gpio_chip *chip,
>> +                                    unsigned bank, unsigned reg_type,
>> +                                    unsigned mask, unsigned value)
>> +{
>> +       struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
>> +       struct uniphier_gpio_priv *priv;
>> +       unsigned long flags;
>> +       unsigned reg;
>> +       u32 tmp;
>> +
>> +       if (!mask)
>> +               return;
>> +
>> +       priv = container_of(mmchip, struct uniphier_gpio_priv, mmchip);
>> +
>> +       reg = uniphier_gpio_bank_to_reg(bank, reg_type);
>> +
>> +       /*
>> +        * Note
>> +        * regmap_update_bits() should not be used here.
>> +        *
>> +        * The DATA registers return the current readback of pins, not the
>> +        * previously written data when they are configured as "input".
>> +        * The DATA registers must be overwritten even if the data you are
>> +        * going to write is the same as what readl() has returned.
>> +        *
>> +        * regmap_update_bits() does not write back if the data is not changed.
>> +        */
>
> Why is this mentioned when the driver doesn't even use regmap?
> Development artifact?


At first, I thought regmap_update_bits() might be useful,
but it tuned out a bad idea.

Anyway, it did not use regmap in this driver, so this comment sounds a
bit weird.
I will delete it in v4.



>> +static int uniphier_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       return uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset) ?
>> +                                               GPIOF_DIR_IN : GPIOF_DIR_OUT;
>
> Just use
> return !!uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset);


OK, will fix.

>> +static int uniphier_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA);
>
> return !!uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA);

Likewise.


>> +static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
>> +                                      unsigned long *mask,
>> +                                      unsigned long *bits)
>> +{
>> +       unsigned bank, shift, bank_mask, bank_bits;
>> +       int i;
>> +
>> +       for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) {
>> +               bank = i / UNIPHIER_GPIO_PORTS_PER_BANK;
>> +               shift = i % BITS_PER_LONG;
>> +               bank_mask = (mask[BIT_WORD(i)] >> shift) &
>> +                                               UNIPHIER_GPIO_BANK_MASK;
>> +               bank_bits = bits[BIT_WORD(i)] >> shift;
>> +
>> +               uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA,
>> +                                        bank_mask, bank_bits);
>> +       }
>
> This looks like a piece of algorithm that we could make generic like in a
> static function in drivers/gpio/gpiolib.h or so, that it may be shared with
> other drivers. Do you see some clear way to break out the core of this?
> Or is it as generic as I think?

I assume this comment has no intention to block my patch.



We already have a generic handling in gpio_chip_set_multiple()
in case .set_multiple() is not supported.

Given that not many drivers support .set_multiple,
I am not sure if we should add another generalized helper func.





>> +static int uniphier_gpio_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct uniphier_gpio_priv *priv;
>> +       u32 ngpio;
>> +       int ret;
>> +
>> +       ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio);
>> +       if (ret) {
>> +               dev_err(dev, "failed to get ngpio property\n");
>> +               return ret;
>> +       }
>
> This needs to be documented, plus I don't see why it's needed.
> The driver for this very specific hardware should already know
> how many GPIOs there are in this hardware, it should not come
> from the device tree.

I want to use this driver on various SoCs, but
the number of GPIO pins varies by SoC.

ngpio == 248 for some SoCs,
and ngpio == 136 for some, etc.


I will document it.


> ngpio is typically for the case where the hardware has 32 bits
> for GPIO pins, but only the first 11 are actually used in the hardware.
>
>> +static const struct of_device_id uniphier_gpio_match[] = {
>> +       { .compatible = "socionext,uniphier-gpio" },
>> +       { /* sentinel */ }
>> +};
>
> Needs a binding document.
>
> Apart from this it looks nice!
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> [hidden email]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Linus Walleij
On Thu, Jul 16, 2015 at 5:44 AM, Masahiro Yamada
<[hidden email]> wrote:
> 2015-07-15 23:15 GMT+09:00 Linus Walleij <[hidden email]>:

>>> +       for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) {
>>> +               bank = i / UNIPHIER_GPIO_PORTS_PER_BANK;
>>> +               shift = i % BITS_PER_LONG;
>>> +               bank_mask = (mask[BIT_WORD(i)] >> shift) &
>>> +                                               UNIPHIER_GPIO_BANK_MASK;
>>> +               bank_bits = bits[BIT_WORD(i)] >> shift;
>>> +
>>> +               uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA,
>>> +                                        bank_mask, bank_bits);
>>> +       }
>>
>> This looks like a piece of algorithm that we could make generic like in a
>> static function in drivers/gpio/gpiolib.h or so, that it may be shared with
>> other drivers. Do you see some clear way to break out the core of this?
>> Or is it as generic as I think?
>
> I assume this comment has no intention to block my patch.

Nah. Just thinking the code looks neat.

>>> +       ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio);
>>> +       if (ret) {
>>> +               dev_err(dev, "failed to get ngpio property\n");
>>> +               return ret;
>>> +       }
>>
>> This needs to be documented, plus I don't see why it's needed.
>> The driver for this very specific hardware should already know
>> how many GPIOs there are in this hardware, it should not come
>> from the device tree.
>
> I want to use this driver on various SoCs, but
> the number of GPIO pins varies by SoC.
>
> ngpio == 248 for some SoCs,
> and ngpio == 136 for some, etc.

That is the wrong way to handle different SoC. That should be handled
by different compatible strings, and then you select the number of GPIOs
for the version corresponding to that compatibe string.

>>> +static const struct of_device_id uniphier_gpio_match[] = {
>>> +       { .compatible = "socionext,uniphier-gpio" },
>>> +       { /* sentinel */ }
>>> +};

i.e. you should use the .data field of of_device_id to carry variant-specific
information.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Masahiro Yamada-2
Hi Linus,



2015-07-16 16:07 GMT+09:00 Linus Walleij <[hidden email]>:

> On Thu, Jul 16, 2015 at 5:44 AM, Masahiro Yamada
> <[hidden email]> wrote:
>> 2015-07-15 23:15 GMT+09:00 Linus Walleij <[hidden email]>:
>
>>>> +       for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) {
>>>> +               bank = i / UNIPHIER_GPIO_PORTS_PER_BANK;
>>>> +               shift = i % BITS_PER_LONG;
>>>> +               bank_mask = (mask[BIT_WORD(i)] >> shift) &
>>>> +                                               UNIPHIER_GPIO_BANK_MASK;
>>>> +               bank_bits = bits[BIT_WORD(i)] >> shift;
>>>> +
>>>> +               uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA,
>>>> +                                        bank_mask, bank_bits);
>>>> +       }
>>>
>>> This looks like a piece of algorithm that we could make generic like in a
>>> static function in drivers/gpio/gpiolib.h or so, that it may be shared with
>>> other drivers. Do you see some clear way to break out the core of this?
>>> Or is it as generic as I think?
>>
>> I assume this comment has no intention to block my patch.
>
> Nah. Just thinking the code looks neat.
>
>>>> +       ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio);
>>>> +       if (ret) {
>>>> +               dev_err(dev, "failed to get ngpio property\n");
>>>> +               return ret;
>>>> +       }
>>>
>>> This needs to be documented, plus I don't see why it's needed.
>>> The driver for this very specific hardware should already know
>>> how many GPIOs there are in this hardware, it should not come
>>> from the device tree.
>>
>> I want to use this driver on various SoCs, but
>> the number of GPIO pins varies by SoC.
>>
>> ngpio == 248 for some SoCs,
>> and ngpio == 136 for some, etc.
>
> That is the wrong way to handle different SoC. That should be handled
> by different compatible strings, and then you select the number of GPIOs
> for the version corresponding to that compatibe string.
>
>>>> +static const struct of_device_id uniphier_gpio_match[] = {
>>>> +       { .compatible = "socionext,uniphier-gpio" },
>>>> +       { /* sentinel */ }
>>>> +};
>
> i.e. you should use the .data field of of_device_id to carry variant-specific
> information.


Currently, I want to use this driver on 7 SoCs

PH1-sLD3:    ngpio == 136
PH1-LD4 :    ngpio == 136
PH1-Pro4:    ngpio == 248
PH1-sLD8:    ngpio == 136
PH1-Pro5:    ngpio == 248
ProXstream2: ngpio == 232
PH1-LD6b:    ngpio == 232

So, should I describe the OF match table like this?

static const struct of_device_id uniphier_gpio_match[] = {
       { .compatible = "socionext,ph1-sld3-gpio"       .data = (void *)136 },
       { .compatible = "socionext,ph1-ld4-gpio"        .data = (void *)136 },
       { .compatible = "socionext,ph1-pro4-gpio"       .data = (void *)248 },
       { .compatible = "socionext,ph1-sld8-gpio"       .data = (void *)136 },
       { .compatible = "socionext,ph1-pro5-gpio"       .data = (void *)248 },
       { .compatible = "socionext,proxstream2-gpio",   .data = (void *)232 },
       { .compatible = "socionext,ph1-ld6b-gpio",      .data = (void *)232 },
       { /* sentinel */ }
};


One disadvantage for this way is that
I need to touch the driver file every time I add a new SoC support.

If I could support "ngpio" property, I would only have to add a device
tree for a new SoC.




--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Linus Walleij
On Thu, Jul 16, 2015 at 9:35 AM, Masahiro Yamada
<[hidden email]> wrote:
> 2015-07-16 16:07 GMT+09:00 Linus Walleij <[hidden email]>:

>>> ngpio == 248 for some SoCs,
>>> and ngpio == 136 for some, etc.
>>
>> That is the wrong way to handle different SoC. That should be handled
>> by different compatible strings, and then you select the number of GPIOs
>> for the version corresponding to that compatibe string.
>>
>>>>> +static const struct of_device_id uniphier_gpio_match[] = {
>>>>> +       { .compatible = "socionext,uniphier-gpio" },
>>>>> +       { /* sentinel */ }
>>>>> +};
>>
>> i.e. you should use the .data field of of_device_id to carry variant-specific
>> information.
>
>
> Currently, I want to use this driver on 7 SoCs
>
> PH1-sLD3:    ngpio == 136
> PH1-LD4 :    ngpio == 136
> PH1-Pro4:    ngpio == 248
> PH1-sLD8:    ngpio == 136
> PH1-Pro5:    ngpio == 248
> ProXstream2: ngpio == 232
> PH1-LD6b:    ngpio == 232
>
> So, should I describe the OF match table like this?
>
> static const struct of_device_id uniphier_gpio_match[] = {
>        { .compatible = "socionext,ph1-sld3-gpio"       .data = (void *)136 },
>        { .compatible = "socionext,ph1-ld4-gpio"        .data = (void *)136 },
>        { .compatible = "socionext,ph1-pro4-gpio"       .data = (void *)248 },
>        { .compatible = "socionext,ph1-sld8-gpio"       .data = (void *)136 },
>        { .compatible = "socionext,ph1-pro5-gpio"       .data = (void *)248 },
>        { .compatible = "socionext,proxstream2-gpio",   .data = (void *)232 },
>        { .compatible = "socionext,ph1-ld6b-gpio",      .data = (void *)232 },
>        { /* sentinel */ }
> };

Yes.

> One disadvantage for this way is that
> I need to touch the driver file every time I add a new SoC support.

That is appropriate since it is a new hardware. This is the same as
the fact that we touch the kernel to add new USB IDs and PCI IDs
every time a new hardware comes out for x86, we should know
what hardware we are toying with.

Since you seem to have a pin controller in parallel anyways I see
it as natural to do this at the same time as you do that anyways.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Masahiro Yamada-2
Hi Linus,

2015-07-16 16:42 GMT+09:00 Linus Walleij <[hidden email]>:

> On Thu, Jul 16, 2015 at 9:35 AM, Masahiro Yamada
> <[hidden email]> wrote:
>> 2015-07-16 16:07 GMT+09:00 Linus Walleij <[hidden email]>:
>
>>>> ngpio == 248 for some SoCs,
>>>> and ngpio == 136 for some, etc.
>>>
>>> That is the wrong way to handle different SoC. That should be handled
>>> by different compatible strings, and then you select the number of GPIOs
>>> for the version corresponding to that compatibe string.
>>>
>>>>>> +static const struct of_device_id uniphier_gpio_match[] = {
>>>>>> +       { .compatible = "socionext,uniphier-gpio" },
>>>>>> +       { /* sentinel */ }
>>>>>> +};
>>>
>>> i.e. you should use the .data field of of_device_id to carry variant-specific
>>> information.
>>
>>
>> Currently, I want to use this driver on 7 SoCs
>>
>> PH1-sLD3:    ngpio == 136
>> PH1-LD4 :    ngpio == 136
>> PH1-Pro4:    ngpio == 248
>> PH1-sLD8:    ngpio == 136
>> PH1-Pro5:    ngpio == 248
>> ProXstream2: ngpio == 232
>> PH1-LD6b:    ngpio == 232
>>
>> So, should I describe the OF match table like this?
>>
>> static const struct of_device_id uniphier_gpio_match[] = {
>>        { .compatible = "socionext,ph1-sld3-gpio"       .data = (void *)136 },
>>        { .compatible = "socionext,ph1-ld4-gpio"        .data = (void *)136 },
>>        { .compatible = "socionext,ph1-pro4-gpio"       .data = (void *)248 },
>>        { .compatible = "socionext,ph1-sld8-gpio"       .data = (void *)136 },
>>        { .compatible = "socionext,ph1-pro5-gpio"       .data = (void *)248 },
>>        { .compatible = "socionext,proxstream2-gpio",   .data = (void *)232 },
>>        { .compatible = "socionext,ph1-ld6b-gpio",      .data = (void *)232 },
>>        { /* sentinel */ }
>> };
>
> Yes.
>
>> One disadvantage for this way is that
>> I need to touch the driver file every time I add a new SoC support.
>
> That is appropriate since it is a new hardware. This is the same as
> the fact that we touch the kernel to add new USB IDs and PCI IDs
> every time a new hardware comes out for x86, we should know
> what hardware we are toying with.
>
> Since you seem to have a pin controller in parallel anyways I see
> it as natural to do this at the same time as you do that anyways.
>

OK, I will do it in v4.







--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Masahiro Yamada-2
Hi Linus,


2015-07-16 16:48 GMT+09:00 Masahiro Yamada <[hidden email]>:
> Hi Linus,

>
> OK, I will do it in v4.


I has been away from my GPIO driver upstreaming
for a long time for some reason.

I sent v3 ten months ago.
http://patchwork.ozlabs.org/patch/494860/


In the meantime, there have been various updates
in the GPIO frame-work.

So, I'd like to know the recommended driver coding style
based on 4.7-rc1.


Diving into the git-log so far, I came up the following list:

[1] The "dev" member of gpiochip was renamed to "parent"

[2] The most recommended register function is now
    devm_gpiochip_add_data()

[3] Pass the driver private data to the
    3rd argument of devm_gpiochip_add_data().

[4] Do not use container_of() to covert
    from gpiochip to driver private data.
    Instead, gpiochip_get_data() should be used.

    (I used of_mm_gpio_chip in v3,
     but I needed to use container_of() to
     convert from gpiochip to of_mm_gpio_chip.
     I am wondering if you are happy with it or not.)

[5] gpiochip.owner is being deprecated.
    So, platform drivers need not set
     "gpiochip.owner =  THIS_MODULE".


Correct?
Is there anything else missing?

Any advice is appreciated.


--
Best Regards
Masahiro Yamada
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller

Linus Walleij
On Fri, May 20, 2016 at 6:30 AM, Masahiro Yamada
<[hidden email]> wrote:

> So, I'd like to know the recommended driver coding style
> based on 4.7-rc1.

Sure, best is to look at other recently added drivers
I guess.

> Diving into the git-log so far, I came up the following list:
>
> [1] The "dev" member of gpiochip was renamed to "parent"

Yes.

> [2] The most recommended register function is now
>     devm_gpiochip_add_data()

Yes, saves some code.

> [3] Pass the driver private data to the
>     3rd argument of devm_gpiochip_add_data().

Yes.

> [4] Do not use container_of() to covert
>     from gpiochip to driver private data.
>     Instead, gpiochip_get_data() should be used.

Yes.

>     (I used of_mm_gpio_chip in v3,
>      but I needed to use container_of() to
>      convert from gpiochip to of_mm_gpio_chip.
>      I am wondering if you are happy with it or not.)

We will see when we look at the code ;)
I guess it is OK. The alternative is to use gpio-generic instead,
see commit 42178e2a1e42b480ada954750f248b53d3fb5940
"drivers/gpio: Switch gpio-mpc8xxx to use gpio-generic"
for an example of that approach.

> [5] gpiochip.owner is being deprecated.
>     So, platform drivers need not set
>      "gpiochip.owner =  THIS_MODULE".

Yes.

> Correct?

Yes.

> Is there anything else missing?

I advice to add a .get_direction() callback if possible and
also .set_single_ended() if the GPIO chip supports open
drain / open source in hardware.

Yours,
Linus Walleij
Loading...