Re: [PATCH 3/4 v2] GPIO: add support for RDC321x GPIO controller

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 11 Mar 2010 09:42:13 +0100
Florian Fainelli <florian@openwrt.org> wrote:

> This patch adds a new GPIO driver for the RDC321x SoC GPIO controller.
> 

Minor points:

>
> +static int rdc_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
> +{
> +	struct rdc321x_gpio *gpch =
> +				container_of(chip, struct rdc321x_gpio, chip);
> +	u32 value = 0;
> +	int reg;

erk, ugly trick to make checkpatch shut up.  This is better:

	struct rdc321x_gpio *gpch;
	u32 value = 0;
	int reg;

	gpch = container_of(chip, struct rdc321x_gpio, chip);


> +/*
> + * Cache the initial value of both GPIO data registers
> + */
> +static int __devinit rdc321x_gpio_probe(struct platform_device *pdev)
> +{
> +	int err;
> +	struct resource *r;
> +	struct rdc321x_gpio *rdc321x_gpio_dev;
> +	struct rdc321x_gpio_pdata *pdata;
> +
> +	pdata = pdev->dev.platform_data;
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "no platform data supplied\n");
> +		return -ENODEV;
> +	}
> +
> +	rdc321x_gpio_dev = kzalloc(sizeof(struct rdc321x_gpio), GFP_KERNEL);
> +	if (!rdc321x_gpio_dev) {
> +		dev_err(&pdev->dev, "failed to allocate private data\n");
> +		return -ENOMEM;
> +	}
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpio-reg1");
> +	if (!r) {
> +		dev_err(&pdev->dev, "failed to get gpio-reg1 resource\n");
> +		err = -ENODEV;
> +		goto out_free;
> +	}
> +
> +	rdc321x_gpio_dev->reg1_ctrl_base = r->start;
> +	rdc321x_gpio_dev->reg1_data_base = r->start + 0x4;
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpio-reg2");
> +	if (!r) {
> +		dev_err(&pdev->dev, "failed to get gpio-reg2 resource\n");
> +		err = -ENODEV;
> +		goto out_free;
> +	}
> +
> +	rdc321x_gpio_dev->reg2_ctrl_base = r->start;
> +	rdc321x_gpio_dev->reg2_data_base = r->start + 0x4;
> +
> +	rdc321x_gpio_dev->chip.label = "rdc321x-gpio";
> +	rdc321x_gpio_dev->chip.direction_input = rdc_gpio_direction_input;
> +	rdc321x_gpio_dev->chip.direction_output = rdc_gpio_config;
> +	rdc321x_gpio_dev->chip.get = rdc_gpio_get_value;
> +	rdc321x_gpio_dev->chip.set = rdc_gpio_set_value;
> +	rdc321x_gpio_dev->chip.base = 0;
> +	rdc321x_gpio_dev->chip.ngpio = pdata->max_gpios;
> +
> +	platform_set_drvdata(pdev, rdc321x_gpio_dev);
> +
> +	/* This might not be, what others (BIOS, bootloader, etc.)
> +	   wrote to these registers before, but it's a good guess. Still
> +	   better than just using 0xffffffff. */
> +	err = rdc321x_pci_read(rdc321x_gpio_dev->reg1_data_base,
> +					&rdc321x_gpio_dev->data_reg[0]);
> +	if (err)
> +		goto out_drvdata;
> +
> +	err = rdc321x_pci_read(rdc321x_gpio_dev->reg2_data_base,
> +					&rdc321x_gpio_dev->data_reg[1]);
> +	if (err)
> +		goto out_drvdata;
> +
> +	spin_lock_init(&rdc321x_gpio_dev->lock);

>From a robustness/defensiveness point of view, it would be better to
initialise this lock as soon as possible.  This reduces the possibility
that someone will later insert code here which takes that lock, but
they only test the code on UP, or on setups where it happens-to-work.

> +	printk(KERN_INFO "rdc321x-gpio: registering %d GPIOs\n",
> +					rdc321x_gpio_dev->chip.ngpio);
> +	return gpiochip_add(&rdc321x_gpio_dev->chip);
> +
> +out_drvdata:
> +	platform_set_drvdata(pdev, NULL);
> +out_free:
> +	kfree(rdc321x_gpio_dev);
> +	return err;
> +}
> ...