linux-kernel - Re: [PATCH] gpio: Renesas RZ GPIO driver

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]

Date:	Thu, 14 Nov 2013 18:00:49 +0900
From:	Magnus Damm <magnus.damm@...il.com>
To:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	SH-Linux <linux-sh@...r.kernel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	"Simon Horman [Horms]" <horms@...ge.net.au>
Subject: Re: [PATCH] gpio: Renesas RZ GPIO driver

Hi Laurent,

On Thu, Nov 14, 2013 at 8:55 AM, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
> 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@...nsource.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@...nsource.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 ?

I too would like to hear Linus opinion. From my side, the other two
GPIO drivers that I've written follow this rule. If someone for some
unknown reason want to change things around then perhaps the interface
should also be updated to instead return a bool?

Regardless, since other drivers follow this style (see for instance
davinci_gpio_get()) perhaps this kind of interface adjustment simply
could be handled as a separate discussion with potential incremental
changes?

>> >> +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 ;-)

Ok-ok, gotcha, will use unsigned there too.

As for "i" vs "k", my tired old eyes prefer to see "k" over "i", "I",
"1", "|"...

Thanks,

/ magnus
--
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/