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

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

[David Daney]

From: David Daney <david.daney@cavium.com>

The SOCs in the OCTEON family have 16 (or in some cases 20) on-chip
GPIO pins, this driver handles them all.  Configuring the pins as
interrupt sources is handled elsewhere (OCTEON's irq handling code).

Signed-off-by: David Daney <david.daney@cavium.com>
---

This patch depends somewhat on patches in Ralf's MIPS/Linux -next tree
where we have patches that enable the Kconfig CAVIUM_OCTEON_SOC and
ARCH_REQUIRE_GPIOLIB symbols.  Apart from that it is stand-alone and
is probably suitable for merging via the GPIO tree.

Device tree binding defintions already exist for this device in
Documentation/devicetree/bindings/gpio/cavium-octeon-gpio.txt

 drivers/gpio/Kconfig       |   8 +++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-octeon.c | 153 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+)
 create mode 100644 drivers/gpio/gpio-octeon.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 573c449..7b5df9a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -190,6 +190,14 @@ config GPIO_MXS
 	select GPIO_GENERIC
 	select GENERIC_IRQ_CHIP
 
+config GPIO_OCTEON
+	tristate "Cavium OCTEON GPIO"
+	depends on GPIOLIB && CAVIUM_OCTEON_SOC
+	default y
+	help
+	  Say yes here to support the on-chip GPIO lines on the OCTEON
+	  family of SOCs.
+
 config GPIO_PL061
 	bool "PrimeCell PL061 GPIO support"
 	depends on ARM && ARM_AMBA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 0cb2d65..b8487b6 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_GPIO_MSM_V2)	+= gpio-msm-v2.o
 obj-$(CONFIG_GPIO_MVEBU)        += gpio-mvebu.o
 obj-$(CONFIG_GPIO_MXC)		+= gpio-mxc.o
 obj-$(CONFIG_GPIO_MXS)		+= gpio-mxs.o
+obj-$(CONFIG_GPIO_OCTEON)	+= gpio-octeon.o
 obj-$(CONFIG_ARCH_OMAP)		+= gpio-omap.o
 obj-$(CONFIG_GPIO_PCA953X)	+= gpio-pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= gpio-pcf857x.o
diff --git a/drivers/gpio/gpio-octeon.c b/drivers/gpio/gpio-octeon.c
new file mode 100644
index 0000000..f5bd127
--- /dev/null
+++ b/drivers/gpio/gpio-octeon.c
@@ -0,0 +1,153 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2011, 2012 Cavium Inc.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+
+#include <asm/octeon/octeon.h>
+#include <asm/octeon/cvmx-gpio-defs.h>
+
+#define RX_DAT 0x80
+#define TX_SET 0x88
+#define TX_CLEAR 0x90
+/*
+ * The address offset of the GPIO configuration register for a given
+ * line.
+ */
+static unsigned int bit_cfg_reg(unsigned int gpio)
+{
+	if (gpio < 16)
+		return 8 * gpio;
+	else
+		return 8 * (gpio - 16) + 0x100;
+}
+
+struct octeon_gpio {
+	struct gpio_chip chip;
+	u64 register_base;
+};
+
+static int octeon_gpio_dir_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio, chip);
+
+	cvmx_write_csr(gpio->register_base + bit_cfg_reg(offset), 0);
+	return 0;
+}
+
+static void octeon_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio, chip);
+	u64 mask = 1ull << offset;
+	u64 reg = gpio->register_base + (value ? TX_SET : TX_CLEAR);
+	cvmx_write_csr(reg, mask);
+}
+
+static int octeon_gpio_dir_out(struct gpio_chip *chip, unsigned offset,
+			       int value)
+{
+	struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio, chip);
+	union cvmx_gpio_bit_cfgx cfgx;
+
+	octeon_gpio_set(chip, offset, value);
+
+	cfgx.u64 = 0;
+	cfgx.s.tx_oe = 1;
+
+	cvmx_write_csr(gpio->register_base + bit_cfg_reg(offset), cfgx.u64);
+	return 0;
+}
+
+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;
+}
+
+static int octeon_gpio_probe(struct platform_device *pdev)
+{
+	struct octeon_gpio *gpio;
+	struct gpio_chip *chip;
+	struct resource *res_mem;
+	int err = 0;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+	chip = &gpio->chip;
+
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res_mem == NULL) {
+		dev_err(&pdev->dev, "found no memory resource\n");
+		err = -ENXIO;
+		goto out;
+	}
+	if (!devm_request_mem_region(&pdev->dev, res_mem->start,
+					resource_size(res_mem),
+				     res_mem->name)) {
+		dev_err(&pdev->dev, "request_mem_region failed\n");
+		err = -ENXIO;
+		goto out;
+	}
+	gpio->register_base = (u64)devm_ioremap(&pdev->dev, res_mem->start,
+						resource_size(res_mem));
+
+	pdev->dev.platform_data = chip;
+	chip->label = "octeon-gpio";
+	chip->dev = &pdev->dev;
+	chip->owner = THIS_MODULE;
+	chip->base = 0;
+	chip->can_sleep = 0;
+	chip->ngpio = 20;
+	chip->direction_input = octeon_gpio_dir_in;
+	chip->get = octeon_gpio_get;
+	chip->direction_output = octeon_gpio_dir_out;
+	chip->set = octeon_gpio_set;
+	err = gpiochip_add(chip);
+	if (err)
+		goto out;
+
+	dev_info(&pdev->dev, "OCTEON GPIO\n");
+out:
+	return err;
+}
+
+static int octeon_gpio_remove(struct platform_device *pdev)
+{
+	struct gpio_chip *chip = pdev->dev.platform_data;
+	return gpiochip_remove(chip);
+}
+
+static struct of_device_id octeon_gpio_match[] = {
+	{
+		.compatible = "cavium,octeon-3860-gpio",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, octeon_gpio_match);
+
+static struct platform_driver octeon_gpio_driver = {
+	.driver = {
+		.name		= "octeon_gpio",
+		.owner		= THIS_MODULE,
+		.of_match_table = octeon_gpio_match,
+	},
+	.probe		= octeon_gpio_probe,
+	.remove		= octeon_gpio_remove,
+};
+
+module_platform_driver(octeon_gpio_driver);
+
+MODULE_DESCRIPTION("Cavium Inc. OCTEON GPIO Driver");
+MODULE_AUTHOR("David Daney");
+MODULE_LICENSE("GPL");
-- 
1.7.11.7

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

[Linus Walleij]

On Sat, Jun 15, 2013 at 1:18 AM, David Daney <ddaney.cavm@gmail.com> wrote:

> From: David Daney <david.daney@cavium.com>
>
> The SOCs in the OCTEON family have 16 (or in some cases 20) on-chip
> GPIO pins, this driver handles them all.  Configuring the pins as
> interrupt sources is handled elsewhere (OCTEON's irq handling code).
>
> Signed-off-by: David Daney <david.daney@cavium.com>

> This patch depends somewhat on patches in Ralf's MIPS/Linux -next tree
> where we have patches that enable the Kconfig CAVIUM_OCTEON_SOC and
> ARCH_REQUIRE_GPIOLIB symbols.  Apart from that it is stand-alone and
> is probably suitable for merging via the GPIO tree.

Really? You're using this:

+#include <asm/octeon/octeon.h>
+#include <asm/octeon/cvmx-gpio-defs.h>

I cannot find this in my tree.

Further I ask why that second file is not part of *this* patch?
It surely seems GPIO-related, and would probably need to
go into include/linux/platform_data/gpio-octeon.h or something
rather than such platform dirs.

(...)
> +config GPIO_OCTEON
> +       tristate "Cavium OCTEON GPIO"
> +       depends on GPIOLIB && CAVIUM_OCTEON_SOC

depend on OF as well right? Or does the CAVIUM_OCTEON_SOC already
imply that?

(...)
> +++ b/drivers/gpio/gpio-octeon.c


> +#define RX_DAT 0x80
> +#define TX_SET 0x88
> +#define TX_CLEAR 0x90


> +/*
> + * The address offset of the GPIO configuration register for a given
> + * line.
> + */
> +static unsigned int bit_cfg_reg(unsigned int gpio)
+       default y
+       help
+         Say yes here to support the on-chip GPIO lines on the OCTEON
+         family of SOCs.
+

Maybe the passed variable shall be named "offset" here, as it is named
offset on all call sites, and it surely local for this instance?

> +{
> +       if (gpio < 16)
> +               return 8 * gpio;
> +       else
> +               return 8 * (gpio - 16) + 0x100;

Put this 0x100 in the #defines above with the name something like
STRIDE.

> +struct octeon_gpio {
> +       struct gpio_chip chip;
> +       u64 register_base;
> +};

OMG everything is 64 bit. Well has to come to this I guess.

> +static void octeon_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +       struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio, chip);
> +       u64 mask = 1ull << offset;

And now BIT(offset) does not work anymore because it is defined as
#define BIT(nr)                 (1UL << (nr))
OK we will have to live with this FTM I guess.

> +static int octeon_gpio_dir_out(struct gpio_chip *chip, unsigned offset,
> +                              int value)
> +{
> +       struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio, chip);
> +       union cvmx_gpio_bit_cfgx cfgx;
> +
> +       octeon_gpio_set(chip, offset, value);
> +
> +       cfgx.u64 = 0;
> +       cfgx.s.tx_oe = 1;

This makes me want to review that magic header file of yours,
I guess this comes from <asm/octeon/cvmx-gpio-defs.h>?

Should not this latter variable be a bool?

I'm not a fan of packed bitfields like this, I prefer if you just
OR | and AND & the bits together in the driver.

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

> +       pdev->dev.platform_data = chip;
> +       chip->label = "octeon-gpio";
> +       chip->dev = &pdev->dev;
> +       chip->owner = THIS_MODULE;
> +       chip->base = 0;
> +       chip->can_sleep = 0;
> +       chip->ngpio = 20;
> +       chip->direction_input = octeon_gpio_dir_in;
> +       chip->get = octeon_gpio_get;
> +       chip->direction_output = octeon_gpio_dir_out;
> +       chip->set = octeon_gpio_set;
> +       err = gpiochip_add(chip);
> +       if (err)
> +               goto out;
> +
> +       dev_info(&pdev->dev, "OCTEON GPIO\n");

This is like shouting "REAL MADRID!" in the bootlog, be a bit more
precise: "octeon GPIO driver probed\n" or something so we know what
is happening.

Yours,
Linus Walleij

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

[David Daney]

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:
> On Sat, Jun 15, 2013 at 1:18 AM, David Daney <ddaney.cavm@gmail.com> wrote:
>
>> From: David Daney <david.daney@cavium.com>
>>
>> The SOCs in the OCTEON family have 16 (or in some cases 20) on-chip
>> GPIO pins, this driver handles them all.  Configuring the pins as
>> interrupt sources is handled elsewhere (OCTEON's irq handling code).
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>
>> This patch depends somewhat on patches in Ralf's MIPS/Linux -next tree
>> where we have patches that enable the Kconfig CAVIUM_OCTEON_SOC and
>> ARCH_REQUIRE_GPIOLIB symbols.  Apart from that it is stand-alone and
>> is probably suitable for merging via the GPIO tree.
>
> Really? You're using this:
>
> +#include <asm/octeon/octeon.h>
> +#include <asm/octeon/cvmx-gpio-defs.h>
>
> I cannot find this in my tree.

Weird, I see them here:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/include/asm/octeon/cvmx-gpio-defs.h
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/include/asm/octeon/octeon.h

Do you not have these?

>
> Further I ask why that second file is not part of *this* patch?
> It surely seems GPIO-related, and would probably need to
> go into include/linux/platform_data/gpio-octeon.h or something
> rather than such platform dirs.
>
> (...)
>> +config GPIO_OCTEON
>> +       tristate "Cavium OCTEON GPIO"
>> +       depends on GPIOLIB && CAVIUM_OCTEON_SOC
>
> depend on OF as well right? Or does the CAVIUM_OCTEON_SOC already
> imply that?

We already have 'select USE_OF', so I think adding OF here would be 
redundant.


>
> (...)
>> +++ b/drivers/gpio/gpio-octeon.c
>
>
>> +#define RX_DAT 0x80
>> +#define TX_SET 0x88
>> +#define TX_CLEAR 0x90
>
>
>> +/*
>> + * The address offset of the GPIO configuration register for a given
>> + * line.
>> + */
>> +static unsigned int bit_cfg_reg(unsigned int gpio)

>
> Maybe the passed variable shall be named "offset" here, as it is named
> offset on all call sites, and it surely local for this instance?

Well it is the gpio line, so perhaps it should universally be change to 
"line" or "pin"


>
>> +{
>> +       if (gpio < 16)
>> +               return 8 * gpio;
>> +       else
>> +               return 8 * (gpio - 16) + 0x100;
>
> Put this 0x100 in the #defines above with the name something like
> STRIDE.

But it is not a 'STRIDE', it is a discontinuity compensation and used in 
exactly one place.


>
>> +struct octeon_gpio {
>> +       struct gpio_chip chip;
>> +       u64 register_base;
>> +};
>
> OMG everything is 64 bit. Well has to come to this I guess.

Not everything.  This is custom logic in an SoC with 64-bit wide 
internal address buses, what would you suggest?


>
>> +static void octeon_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>> +{
>> +       struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio, chip);
>> +       u64 mask = 1ull << offset;
>
> And now BIT(offset) does not work anymore because it is defined as
> #define BIT(nr)                 (1UL << (nr))
> OK we will have to live with this FTM I guess.
>
>> +static int octeon_gpio_dir_out(struct gpio_chip *chip, unsigned offset,
>> +                              int value)
>> +{
>> +       struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio, chip);
>> +       union cvmx_gpio_bit_cfgx cfgx;
>> +
>> +       octeon_gpio_set(chip, offset, value);
>> +
>> +       cfgx.u64 = 0;
>> +       cfgx.s.tx_oe = 1;
>
> This makes me want to review that magic header file of yours,
> I guess this comes from <asm/octeon/cvmx-gpio-defs.h>?

Not really magic, but yes that is where it comes from.

>
> Should not this latter variable be a bool?

I don't think so, it is not the result of a comparison operator.

>
> I'm not a fan of packed bitfields like this, I prefer if you just
> OR | and AND & the bits together in the driver.
>
>> +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.

>
>> +       pdev->dev.platform_data = chip;
>> +       chip->label = "octeon-gpio";
>> +       chip->dev = &pdev->dev;
>> +       chip->owner = THIS_MODULE;
>> +       chip->base = 0;
>> +       chip->can_sleep = 0;
>> +       chip->ngpio = 20;
>> +       chip->direction_input = octeon_gpio_dir_in;
>> +       chip->get = octeon_gpio_get;
>> +       chip->direction_output = octeon_gpio_dir_out;
>> +       chip->set = octeon_gpio_set;
>> +       err = gpiochip_add(chip);
>> +       if (err)
>> +               goto out;
>> +
>> +       dev_info(&pdev->dev, "OCTEON GPIO\n");
>
> This is like shouting "REAL MADRID!" in the bootlog, be a bit more
> precise: "octeon GPIO driver probed\n" or something so we know what
> is happening.

No, more akin to 'Real Madrid', as 'OCTEON' is the correct spelling of 
its given name.

I will happily add "driver probed", and grudgingly switch to lower case 
if it is a necessary condition of patch acceptance.

David Daney

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

[Joe Perches]

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.

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

[David Daney]

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.

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.

David Daney

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

[Joe Perches]

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.

> 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.

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

[David Daney]

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.
>
>
>

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

[Linus Walleij]

On Thu, Jun 20, 2013 at 8:10 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 06/17/2013 01:51 AM, Linus Walleij wrote:

>> +#include <asm/octeon/octeon.h>
>> +#include <asm/octeon/cvmx-gpio-defs.h>
>>
>> I cannot find this in my tree.
>
> Weird, I see them here:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/include/asm/octeon/cvmx-gpio-defs.h
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/include/asm/octeon/octeon.h
>
> Do you not have these?

Yeah no problem, I must have misgrepped.
Sorry for the fuzz...

>> depend on OF as well right? Or does the CAVIUM_OCTEON_SOC already
>> imply that?
>
> We already have 'select USE_OF', so I think adding OF here would be
> redundant.

OK.

>>> +/*
>>> + * The address offset of the GPIO configuration register for a given
>>> + * line.
>>> + */
>>> +static unsigned int bit_cfg_reg(unsigned int gpio)
>>
>> Maybe the passed variable shall be named "offset" here, as it is named
>> offset on all call sites, and it surely local for this instance?
>
> Well it is the gpio line, so perhaps it should universally be change to
> "line" or "pin"

We use "offset" to signify line enumerators in drivers/gpio/*
well atleaste if they are local to a piece of hardware.
(Check the GPIO siblings.)

>>> +{
>>> +       if (gpio < 16)
>>> +               return 8 * gpio;
>>> +       else
>>> +               return 8 * (gpio - 16) + 0x100;
>>
>>
>> Put this 0x100 in the #defines above with the name something like
>> STRIDE.
>
> But it is not a 'STRIDE', it is a discontinuity compensation and used in
> exactly one place.

OK what about a comment or something, because it isn't
exactly intuitive right?

>>> +struct octeon_gpio {
>>> +       struct gpio_chip chip;
>>> +       u64 register_base;
>>> +};
>>
>> OMG everything is 64 bit. Well has to come to this I guess.
>
> Not everything.  This is custom logic in an SoC with 64-bit wide internal
> address buses, what would you suggest?

Yep that's what I meant, no big deal. Just first time
I really see it in driver bases.

>> I'm not a fan of packed bitfields like this, I prefer if you just
>> OR | and AND & the bits together in the driver.

I see you disregarded this comment, and looking at the header
files it seems the MIPS arch is a big fan if packed bitfields so
will live with it for this arch...

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

Nah. If a good rational reason like "hate" is given for not using a coding
idiom I will accept it as it stands ;-)

>>> +       dev_info(&pdev->dev, "OCTEON GPIO\n");
>>
>>
>> This is like shouting "REAL MADRID!" in the bootlog, be a bit more
>> precise: "octeon GPIO driver probed\n" or something so we know what
>> is happening.
>
> No, more akin to 'Real Madrid', as 'OCTEON' is the correct spelling of its
> given name.
>
> I will happily add "driver probed", and grudgingly switch to lower case if
> it is a necessary condition of patch acceptance.

I don't know, does this rest of the MIPS drivers emit similar messages
such that the bootlog will say

OCTEON clocks
OCTEON irqchip
OCTEON I2C
OCTEON GPIO

then I guess it's convention and it can stay like this.

Yours,
Linus Walleij

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

[David Daney]

Thanks for looking at this again.

I will be away from my office until the middle of July, so I will not be 
able to generate and test a revised patch until then.

David Daney



On 06/24/2013 03:06 PM, Linus Walleij wrote:
> On Thu, Jun 20, 2013 at 8:10 PM, David Daney <ddaney.cavm@gmail.com> wrote:
>> On 06/17/2013 01:51 AM, Linus Walleij wrote:
>
>>> +#include <asm/octeon/octeon.h>
>>> +#include <asm/octeon/cvmx-gpio-defs.h>
>>>
>>> I cannot find this in my tree.
>>
>> Weird, I see them here:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/include/asm/octeon/cvmx-gpio-defs.h
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/include/asm/octeon/octeon.h
>>
>> Do you not have these?
>
> Yeah no problem, I must have misgrepped.
> Sorry for the fuzz...
>
>>> depend on OF as well right? Or does the CAVIUM_OCTEON_SOC already
>>> imply that?
>>
>> We already have 'select USE_OF', so I think adding OF here would be
>> redundant.
>
> OK.
>
>>>> +/*
>>>> + * The address offset of the GPIO configuration register for a given
>>>> + * line.
>>>> + */
>>>> +static unsigned int bit_cfg_reg(unsigned int gpio)
>>>
>>> Maybe the passed variable shall be named "offset" here, as it is named
>>> offset on all call sites, and it surely local for this instance?
>>
>> Well it is the gpio line, so perhaps it should universally be change to
>> "line" or "pin"
>
> We use "offset" to signify line enumerators in drivers/gpio/*
> well atleaste if they are local to a piece of hardware.
> (Check the GPIO siblings.)
>
>>>> +{
>>>> +       if (gpio < 16)
>>>> +               return 8 * gpio;
>>>> +       else
>>>> +               return 8 * (gpio - 16) + 0x100;
>>>
>>>
>>> Put this 0x100 in the #defines above with the name something like
>>> STRIDE.
>>
>> But it is not a 'STRIDE', it is a discontinuity compensation and used in
>> exactly one place.
>
> OK what about a comment or something, because it isn't
> exactly intuitive right?
>
>>>> +struct octeon_gpio {
>>>> +       struct gpio_chip chip;
>>>> +       u64 register_base;
>>>> +};
>>>
>>> OMG everything is 64 bit. Well has to come to this I guess.
>>
>> Not everything.  This is custom logic in an SoC with 64-bit wide internal
>> address buses, what would you suggest?
>
> Yep that's what I meant, no big deal. Just first time
> I really see it in driver bases.
>
>>> I'm not a fan of packed bitfields like this, I prefer if you just
>>> OR | and AND & the bits together in the driver.
>
> I see you disregarded this comment, and looking at the header
> files it seems the MIPS arch is a big fan if packed bitfields so
> will live with it for this arch...
>
>>>> +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.
>
> Nah. If a good rational reason like "hate" is given for not using a coding
> idiom I will accept it as it stands ;-)
>
>>>> +       dev_info(&pdev->dev, "OCTEON GPIO\n");
>>>
>>>
>>> This is like shouting "REAL MADRID!" in the bootlog, be a bit more
>>> precise: "octeon GPIO driver probed\n" or something so we know what
>>> is happening.
>>
>> No, more akin to 'Real Madrid', as 'OCTEON' is the correct spelling of its
>> given name.
>>
>> I will happily add "driver probed", and grudgingly switch to lower case if
>> it is a necessary condition of patch acceptance.
>
> I don't know, does this rest of the MIPS drivers emit similar messages
> such that the bootlog will say
>
> OCTEON clocks
> OCTEON irqchip
> OCTEON I2C
> OCTEON GPIO
>
> then I guess it's convention and it can stay like this.
>
> Yours,
> Linus Walleij
>
>