[Top] [All Lists]

Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pin

To: Joe Perches <>
Subject: Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
From: David Daney <>
Date: Thu, 20 Jun 2013 11:51:20 -0700
Cc: Linus Walleij <>,, Ralf Baechle <>, Grant Likely <>, Rob Herring <>, "" <>, "" <>, David Daney <>
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed;; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=Icc2CKTdCR5MIbIKJeeqP8QbNS6fOdtCypBvbw6VL5w=; b=vmYDlm1X80VsFJTmm9tFC8TsWmIyo/T/HsWIT+tS0XyVmamihSIO8VxNWgCSgb7NFO QyDjfjEGfYIgAQUc1G4TejVoZYzkqicUzktAb84A9OpoYx8tFR36qRNTWKWY1Lj1lsOS 2OgupdlYDCWWxi58leCtEzz7dlBrCMMyzyd6Y1OuE1hfhXYhgCsbHhqavjmP8DuFPP6G ImUsGHhst/BuvVZ0w+eovnb5gUQTd0041GmYEfgnAzLCGWKh6dBRiaskjDqQW3rDsXK6 ZOl4hHlbR5QjfxxozyLADVwjrDtWs/hH8QN+xhCXQkiDAMV4WCUuDs+9lEu6DbJiFvRi e3yQ==
In-reply-to: <1371753812.2146.37.camel@joe-AO722>
List-archive: <>
List-help: <>
List-id: linux-mips <>
List-owner: <>
List-post: <>
List-software: Ecartis version 1.0.0
List-subscribe: <>
List-unsubscribe: <>
Original-recipient: rfc822;
References: <> <> <> <1371752324.2146.25.camel@joe-AO722> <> <1371753812.2146.37.camel@joe-AO722>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6
On 06/20/2013 11:43 AM, Joe Perches wrote:
On Thu, 2013-06-20 at 11:27 -0700, David Daney wrote:
On 06/20/2013 11:18 AM, Joe Perches wrote:
On Thu, 2013-06-20 at 11:10 -0700, David Daney wrote:
Sorry for not responding earlier, but my e-mail system seems to have
malfunctioned with respect to this message...
On 06/17/2013 01:51 AM, Linus Walleij wrote:
+static int octeon_gpio_get(struct gpio_chip *chip, unsigned offset)
+       struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio, chip);
+       u64 read_bits = cvmx_read_csr(gpio->register_base + RX_DAT);
+       return ((1ull << offset) & read_bits) != 0;

A common idiom we use for this is:

return !!(read_bits & (1ull << offset));

I hate that idiom, but if its use is a condition of accepting the patch,
I will change it.

Or use an even more common idiom and change the
function to return bool and let the compiler do it.

... but it is part of the gpiochip system interface, so it would have to
be done kernel wide.

Not really.  It's a local static function.

... which we generate a pointer to, and then assign that pointer to a variable with a type defined in the gpiochip system interface. So If we do what you suggest, the result is:

  CC      drivers/gpio/gpio-octeon.o
drivers/gpio/gpio-octeon.c: In function 'octeon_gpio_probe':
drivers/gpio/gpio-octeon.c:113:12: warning: assignment from incompatible pointer type [enabled by default]

Really I don't like the idea of GPIO lines having Boolean truth values
associated with them.  Some represent things that are active-high and
others active-low.  Converting the pin voltage being above or below a
given threshold to something other than zero or one would in my opinion
be confusing.

No worries, just offering options.  Your code, your choice.

<Prev in Thread] Current Thread [Next in Thread>