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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20100319135140.7d645a7c.akpm@linux-foundation.org>
Date:	Fri, 19 Mar 2010 13:51:40 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Florian Fainelli <florian@...nwrt.org>
Cc:	linux-kernel@...r.kernel.org, Samuel Ortiz <sameo@...ux.intel.com>,
	Wim Van Sebroeck <wim@...ana.be>, Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH 3/4 v2] GPIO: add support for RDC321x GPIO controller

On Thu, 11 Mar 2010 09:42:13 +0100
Florian Fainelli <florian@...nwrt.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;
> +}
> ...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/