On Wed, May 21, 2014 at 8:54 AM, Feng Kan [off-list ref] wrote:

quoted

+static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+       struct xgene_gpio_bank *bank =
+                       container_of(gc, struct xgene_gpio_bank, chip);
+       unsigned long flags;
+       u32 data;
+
+       spin_lock_irqsave(&bank->lock, flags);
+
+       data = ioread32(bank->set_dr);
+       data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);

quoted

Couldn't you use set_bit() / clear_bit() instead here?

Set_bit/clear_bit seems a bit heavy, I can use the non atomic methods
of _set_bit/clear_bit
if you agree? In which case I will rewrite all the other method in
likewise fashion.

Sounds good.

quoted

quoted

+       bank = &gpio->banks[offs];
+       bank->gpio = gpio;
+       spin_lock_init(&bank->lock);
+
+       if (of_property_read_u32(bank_np, "ngpios", &ngpio))
+               ngpio = 16;

Here I think you want to distinguish between "property not defined"
(-EINVAL) and "property has no value" (-ENODATA) and other possible
errors. In the later cases you will want to signal the error and
return from here.

I kinda messed up here a bit, I am trying to support ACPI booting but other
properties are also affected. I will rewrite this part so minimum of_ access
methods are required.

quoted

quoted

+
+       if ((ngpio > 16) || (ngpio < 1)) {
+               dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
+                        bank_np->full_name);
+               return -EINVAL;
+       }
+
+       bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
+       bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
+       bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
+       bank->set_dr = gpio->regs + GPIO_SET_DR
+                               + (bank_idx * GPIO_SET_DR_OFFSET);
+
+       bank->chip.direction_input = xgene_gpio_dir_in;
+       bank->chip.direction_output = xgene_gpio_dir_out;
+       bank->chip.get = xgene_gpio_get;
+       bank->chip.set = xgene_gpio_set;
+
+       if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
+               iowrite32(odcfg, bank->od);
+
+       bank->chip.ngpio = ngpio;
+       bank->chip.of_node = bank_np;
+       bank->chip.base = bank_idx * ngpio;
+
+       err = gpiochip_add(&bank->chip);
+       if (err)
+               dev_err(gpio->dev, "failed to register gpiochip for %s\n",
+                       bank_np->full_name);
+
+       return err;
+}
+
+static int xgene_gpio_probe(struct platform_device *pdev)
+{
+       struct device_node *np = pdev->dev.of_node;
+       struct resource *res;
+       struct xgene_gpio *gpio;
+       int err;
+       unsigned int offs = 0;
+
+       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+       if (!gpio)
+               return -ENOMEM;
+       gpio->dev = &pdev->dev;
+
+       gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
+       if (!gpio->nr_banks) {
+               err = -EINVAL;
+               goto err;
+       }

Might be worth checking whether nr_banks >= XGENE_GPIO_MAX_PORTS and
return an error here.

quoted

+
+       gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
+                                  sizeof(*gpio->banks), GFP_KERNEL);
+       if (!gpio->banks) {
+               err = -ENOMEM;
+               goto err;
+       }

I'm fine with printing the error status the way you do, but could you
also do the same for the first kzalloc too?

Or maybe you could put your banks member at the end of the xgene_gpio
struct as a flexible array member and perform only one kzalloc of size
(sizeof(*gpio) + (sizeof(*gpio->banks) * gpio->nr_banks))?

quoted

+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       gpio->regs = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(gpio->regs)) {
+               err = PTR_ERR(gpio->regs);
+               goto err;
+       }
+
+       for_each_child_of_node(pdev->dev.of_node, np) {
+               err = xgene_gpio_add_bank(gpio, np, offs++);
+               if (err)
+                       goto err;
+       }
+
+       platform_set_drvdata(pdev, gpio);
+
+       dev_info(&pdev->dev, "X-Gene GPIO driver registered\n");
+       return 0;
+err:
+       dev_err(&pdev->dev, "X-Gene GPIO driver registration failed\n");
+       return err;
+}
+
+static const struct of_device_id xgene_gpio_of_match[] = {
+       { .compatible = "apm,xgene-gpio", },
+       {},
+};
+MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
+
+static struct platform_driver xgene_gpio_driver = {
+       .driver = {
+               .name = "xgene-gpio",
+               .owner = THIS_MODULE,
+               .of_match_table = xgene_gpio_of_match,
+       },
+       .probe = xgene_gpio_probe,
+};
+
+static int __init xgene_gpio_init(void)
+{
+       return platform_driver_register(&xgene_gpio_driver);
+}
+postcore_initcall(xgene_gpio_init);
+
+MODULE_AUTHOR("AppliedMicro");
+MODULE_DESCRIPTION("APM X-Gene GPIO driver");
+MODULE_LICENSE("GPL");
--
1.9.1

Globally the driver looks well-written, but I don't understand the
need to have one gpio_chip per bank. Could you elaborate on the
reasons for this design?

I see each bank as separate gpio chip. It is a simple way to
abstract the banks since they can operate independently. It also
provided me a way to fix the sysfs gpio base number, regardless if
a particular bank node is pulled out. This is also done in similar way
in some other gpio drivers such as the dwapb gpio driver.

I'd like to see what Linus thinks about this, but if there is
precedence, and the banks can work independently, then I agree it may
make sense to have it that way. In that case you should probably have
each bank appearing as its own, independent node in the device tree,
and be probed directly by this driver instead of going through a
higher-level device which only practical use is to provide the number
of banks in your GPIO chip. I.e. if your bank are independent GPIO
devices, let the driver also reflect that fact.