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

[Gregory Bean]

> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/