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

[Gregory Bean <gbean@codeaurora.org>]

> 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.