linux-kernel - Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512 PCIE-to-UART/GPIO GPIOLIB support

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] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 29 Jan 2016 16:15:49 +0800
From:	Peter Hung <hpeter@...il.com>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	linus.walleij@...aro.org, gnurou@...il.com,
	gregkh@...uxfoundation.org, paul.gortmaker@...driver.com,
	lee.jones@...aro.org, jslaby@...e.com, peter_hong@...tek.com.tw
Cc:	heikki.krogerus@...ux.intel.com, peter@...leysoftware.com,
	soeren.grunewald@...y.de, udknight@...il.com,
	adam.lee@...onical.com, arnd@...db.de, manabian@...il.com,
	scottwood@...escale.com, yamada.masahiro@...ionext.com,
	paul.burton@...tec.com, mans@...sr.com, matthias.bgg@...il.com,
	ralf@...ux-mips.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-serial@...r.kernel.org,
	tom_tsai@...tek.com.tw, Peter Hung <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V2 2/4] gpio: gpio-f81504: Add Fintek F81504/508/512
 PCIE-to-UART/GPIO GPIOLIB support

Hi Andy,

Andy Shevchenko 於 2016/1/28 下午 08:03 寫道:
> On Thu, 2016-01-28 at 17:20 +0800, Peter Hung wrote:
>> +	/* set output data */
>> +	tmp = inb(priv->gpio_ioaddr + gc->idx);
>
> ioread8 is a bit better since it automatically works with IO space and
> MMIO. But if you are certain you will always have the address in IO
> space, you can disregard this comment.

I had only tested on x86 environment currently. We'll try to get an ARM
platform with PCIE to test it. We'll remain it until getting new board.

>> +static int f81504_gpio_probe(struct platform_device *pdev)
>> +{
>> +	int status;
>> +	struct f81504_gpio_chip *gc;
>> +	void *data = dev_get_platdata(&pdev->dev);
>> +	u8 gpio_idx = *(u8 *)data;
>> +	char *name;
>> +
>> +	if (gpio_idx >= ARRAY_SIZE(fintek_gpio_mapping)) {
>> +		dev_err(&pdev->dev, "%s: gpio_idx:%d out of
>> range.\n",
>> +				__func__, gpio_idx);
>> +		return -ENODEV;
>> +	}
>> +
>> +	gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL);
>> +	if (!gc)
>> +		return -ENOMEM;
>> +
>>
>
>> +	kfree(data);
>
> What the heck?

Sorry for the big mistake, I'd confused for dev_get_platdata() &
platform_set_drvdata(). I'll rewrite this and check 8250_f81504.c
too.

>> +	status = gpiochip_add(&gc->chip);
>> +	if (status) {
>> +		dev_err(&pdev->dev, "%s: gpiochip_add failed: %d\n",
>> __func__,
>> +				status);
>> +		return -ENOMEM;
>
> You ignored the status.
>
>> +	}
>> +
>> +	return 0;
>
> Perhaps just
> return gpiochip_add(); ?

Just return gpiochip_add() seems good.

Thanks your advices
-- 
With Best Regards,
Peter Hung