linux-kernel - Re: [PATCH 1/2] gpio: msm7200a: Add gpiolib support for MSM chips.

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]

Message-ID: <4C131D0E.60109@codeaurora.org>
Date:	Fri, 11 Jun 2010 22:37:18 -0700
From:	Gregory Bean <gbean@...eaurora.org>
To:	Ben Dooks <ben-linux@...ff.org>
CC:	akpm@...ux-foundation.org, linux-arm-msm@...r.kernel.org,
	linux-kernel@...r.kernel.org, Joe Perches <joe@...ches.com>,
	"David S. Miller" <davem@...emloft.net>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Randy Dunlap <randy.dunlap@...cle.com>,
	Michael Hennerich <michael.hennerich@...log.com>,
	Mike Frysinger <vapier@...too.org>,
	David Brown <davidb@...eaurora.org>,
	Daniel Walker <dwalker@...eaurora.org>,
	Bryan Huntsman <bryanh@...eaurora.org>
Subject: Re: [PATCH 1/2] gpio: msm7200a: Add gpiolib support for MSM chips.

> Why not put this under arch/arm?

Is there an appropriate place for loadable device drivers under 
arch/arm?  I don't know of one.

>> +static inline void set_gpio_bit(unsigned n, void __iomem *reg)
>> +{
>> +	writel(readl(reg) | bit(n), reg);
>> +}
>> +
>> +/*
>> + * This function assumes that msm_gpio_dev::lock is held.
>> + */
>> +static inline void clr_gpio_bit(unsigned n, void __iomem *reg)
>> +{
>> +	writel(readl(reg)&  ~bit(n), reg);
>> +}
>> +
>> +/*
>> + * This function assumes that msm_gpio_dev::lock is held.
>> + */
>> +static inline void
>> +msm_gpio_write(struct msm_gpio_dev *dev, unsigned n, unsigned on)
>> +{
>> +	if (on)
>> +		set_gpio_bit(n, dev->regs.out);
>> +	else
>> +		clr_gpio_bit(n, dev->regs.out);
>> +}
>
> wouldn't it be easier to inline a set_to function and just role the
> set and clr bit functions into it, since they pretty much do the
> same thing. even better, on arm the code won't require a branch.

I'm not sure I understand you.  Can you clarify?  set_ and clr_gpio_bit 
are used in more places than just here, so they can't just be rolled 
into msm_gpio_write and disappear.

>> +static int msm_gpio_remove(struct platform_device *dev)
>> +{
>> +	struct msm_gpio_dev *msm_gpio = platform_get_drvdata(dev);
>> +	int ret = gpiochip_remove(&msm_gpio->gpio_chip);
>> +
>> +	if (ret == 0)
>> +		kfree(msm_gpio);
>
> hmm, not sure if you really need to check the result here before
> kfrree() the memory.

I feel that this is important.  If any clients are still holding gpio 
lines, gpiochip_remove will fail.  In those circumstances, is it not 
important that the device not be freed (which would leave clients with 
stale references) and that the remove call return a proper failure code?
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
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/