Re: [PATCH] gpio: Renesas RZ GPIO driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Magnus,

On Thursday 14 November 2013 08:49:16 Magnus Damm wrote:
> On Wed, Nov 13, 2013 at 9:03 PM, Laurent Pinchart wrote:
> > Hi Magnus,
> > 
> > Thank you for the patch.
> > 
> > Please read below for a couple of comments in addition to Linus' review.
> > 
> > On Thursday 07 November 2013 08:47:37 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> This patch adds a GPIO driver for the RZ series of SoCs from
> >> Renesas. The driver can be used as platform device with dynamic
> >> or static GPIO assignment or via DT using dynamic GPIOs.
> >> 
> >> The hardware allows control of GPIOs in blocks of up to 16 pins,
> >> and once device may span multiple blocks. Interrupts are not
> >> included in this hardware block, if interrupts are needed then
> >> the PFC needs to be configured to a IRQ pin function which is
> >> handled by the GIC hardware.
> >> 
> >> Tested with yet-to-be-posted platform device and DT devices on
> >> r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
> >> 
> >> Signed-off-by: Magnus Damm <damm@opensource.se>
> >> ---
> >> 
> >>  drivers/gpio/Kconfig                  |    6
> >>  drivers/gpio/Makefile                 |    1
> >>  drivers/gpio/gpio-rz.c                |  241 +++++++++++++++++++++++++++
> >>  include/linux/platform_data/gpio-rz.h |   13 +
> >>  4 files changed, 261 insertions(+)

[snip]

> >> --- /dev/null
> >> +++ work/drivers/gpio/gpio-rz.c       2013-11-06 14:20:02.000000000 +0900
> >> @@ -0,0 +1,241 @@

[snip]

> >> +static inline unsigned long rz_gpio_read_ppr(struct rz_gpio_priv *p, int
> >> offs)
> >> +{
> >> +     unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT);
> >> +     int offset = (offs / RZ_GPIOS_PER_PORT) * 4;
> > 
> > offs and offset are unsigned, you can make them unsigned int.
> 
> Ok!
> 
> >> +     return ioread32(p->io[REG_PPR] + offset) & msk;
> > 
> > I believe you should return !!(...) here, or in the caller, to make sure
> > the gpio_get_value() operation returns either 0 or 1. I would do it here
> > and return a u32 instead of unsigned long.
> 
> I disagree with the !! because it is just pure overhead, please see
> the __gpio_get_value() comment, it says returning zero or nonzero. So
> I left this portion as-is.

OK, good point. Linus, what's the best practice rule for GPIO drivers ? Should 
they just return any non-zero value, or is any specific value preferred ?

> >> +}

[snip]

> >> +static int rz_gpio_probe(struct platform_device *pdev)
> >> +{
> >> +     struct gpio_rz_config *pdata = dev_get_platdata(&pdev->dev);
> >> +     struct rz_gpio_priv *p;
> >> +     struct resource *io[3];
> >> +     struct gpio_chip *gpio_chip;
> >> +     struct device_node *np = pdev->dev.of_node;
> >> +     struct of_phandle_args args;
> >> +     int number_of_pins, gpio_base;
> >> +     int k, nr;
> > 
> > unsigned ?
> 
> Ok!
> 
> > By the way, what's wrong with i as a loop index ? :-)
> 
> Nothing, but I left it as-is anyway! =)

Good to know it's not wrong. But it's still an int in v2 ;-)

> >> +     int ret;

-- 
Regards,

Laurent Pinchart