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

[Ben Dooks <ben@trinity.fluff.org>]

On Fri, Jun 11, 2010 at 10:37:18PM -0700, Gregory Bean wrote:
>> 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.

Hmm, thought the compiler was clever enough to inline and sort that
out, I'll have a check later into whether this is true or not for
a few versions of gcc.

>>> +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?

Right, confused holding onto the module to holding onto the device too.

Maybe devices should be refcounted too so that holding an open gpio on
via the driver would force the driver core to refuse to remove the device.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.