> +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);
>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.
>> + 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.
>
>> +
>> + 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.
>
>> +
>> + 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))?
>
>> +
>> + 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.
>
> Alex.
--
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
