gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
PATCH 1/1 gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
From: David Daney <[email protected]>
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).
---
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
drivers/gpio/Kconfig +8 -0
@@ -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
drivers/gpio/Makefile +1 -0
@@ -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
drivers/gpio/gpio-octeon.c new file +153 -0
153 lines not on this page · raw message
Signature · 1 line
1.7.11.7
Quoted 6 lines · David Daney
From: David Daney <[email protected]>
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).
Quoted 6 lines
Signed-off-by David Daney <[email protected]>This patch depends somewhat on patches in Ralf's MIPS/Linux -next tree where we have patches that enable the
KconfigCAVIUM_OCTEON_SOCandARCH_REQUIRE_GPIOLIBsymbols. 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.
(...)
drivers/gpio/Kconfig config GPIO_MXS · line 195
Quoted 3 lines
drivers/gpio/Kconfig +3
-0+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?
(...)
drivers/gpio/gpio-octeon.c line 25
Quoted 13 lines
+++
b/drivers/gpio/gpio-octeon.cdrivers/gpio/gpio-octeon.c new file +8
-0+#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?
drivers/gpio/gpio-octeon.c line 30
Quoted 5 lines
drivers/gpio/gpio-octeon.c new file +5
-0+{ +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.
drivers/gpio/gpio-octeon.c line 36
Quoted 4 lines
drivers/gpio/gpio-octeon.c new file +4
-0+struct octeon_gpio { +struct gpio_chip chip; +u64 register_base; +};
OMG everything is 64 bit. Well has to come to this I guess.
drivers/gpio/gpio-octeon.c line 49
Quoted 4 lines
drivers/gpio/gpio-octeon.c new file +4
-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;
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.
drivers/gpio/gpio-octeon.c line 63
Quoted 10 lines
drivers/gpio/gpio-octeon.c new file +10
-0+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.
drivers/gpio/gpio-octeon.c line 74
Quoted 6 lines
drivers/gpio/gpio-octeon.c new file +6
-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 ;
A common idiom we use for this is:
return !!(read_bits & (1ull << offset));
Quoted 6 lines
drivers/gpio/gpio-octeon.c new file +6
-0+ pdev->dev.platform_data = chip; + chip->label ="octeon-gpio" ; + chip->dev = &pdev->dev; + chip->owner =THIS_MODULE ; + chip->base =0 ; + chip->can_sleep =0 ;
drivers/gpio/gpio-octeon.c line 120
Quoted 10 lines
drivers/gpio/gpio-octeon.c new file +10
-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
Sorry for not responding earlier, but my e-mail system seems to have malfunctioned with respect to this message...
Quoted 20 lines · Linus Walleij
Quoted 6 lines · David Daney
From: David Daney <[email protected]>
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).
Quoted 6 lines
Signed-off-by David Daney <[email protected]>This patch depends somewhat on patches in Ralf's MIPS/Linux -next tree where we have patches that enable the
KconfigCAVIUM_OCTEON_SOCandARCH_REQUIRE_GPIOLIBsymbols. Apart from that it is stand-alone and is probably suitable for merging via the GPIO tree.Really? You're using this:
drivers/gpio/gpio-octeon.c new file +2
-0+#include <asm/octeon/octeon.h> +#include <asm/octeon/cvmx-gpio-defs.h>
Quoted 1 line
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?
Quoted 6 lines
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.hor something rather than such platform dirs.
Quoted 7 lines
(...)
drivers/gpio/Kconfig config GPIO_MXS · line 195
Quoted 3 lines
drivers/gpio/Kconfig +3
-0+config GPIO_OCTEON + tristate "Cavium OCTEON GPIO" + depends on GPIOLIB && CAVIUM_OCTEON_SOCdepend on OF as well right? Or does the
CAVIUM_OCTEON_SOCalready imply that?
We already have 'select USE_OF', so I think adding OF here would be redundant.
Quoted 10 lines
(...)
Quoted 6 lines
+++
b/drivers/gpio/gpio-octeon.cdrivers/gpio/gpio-octeon.c new file +3
-0+#define RX_DAT 0x80 +#define TX_SET 0x88 +#define TX_CLEAR 0x90
Quoted 9 lines
drivers/gpio/gpio-octeon.c line 25
Quoted 5 lines
drivers/gpio/gpio-octeon.c new file +5
-0+/* +* 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"
Quoted 9 lines
drivers/gpio/gpio-octeon.c line 30
Quoted 6 lines
drivers/gpio/gpio-octeon.c new file +5
-0+{ +if (gpio <16 ) +return 8 * gpio; +else +return 8 * (gpio -16 ) +0x100 ;Put this
0x100in 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.
Quoted 7 lines
drivers/gpio/gpio-octeon.c line 36
Quoted 5 lines
drivers/gpio/gpio-octeon.c new file +4
-0+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?
Quoted 10 lines
drivers/gpio/gpio-octeon.c line 49
Quoted 5 lines
drivers/gpio/gpio-octeon.c new file +4
-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;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.
Quoted 13 lines
drivers/gpio/gpio-octeon.c line 63
Quoted 10 lines
drivers/gpio/gpio-octeon.c new file +10
-0+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.
Quoted 2 lines
Should not this latter variable be a bool?
I don't think so, it is not the result of a comparison operator.
Quoted 13 lines
I'm not a fan of packed bitfields like this, I prefer if you just OR | and AND & the bits together in the driver.
drivers/gpio/gpio-octeon.c line 74
Quoted 6 lines
drivers/gpio/gpio-octeon.c new file +6
-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 ;A common idiom we use for this is:
Quoted 1 line
return !!(read_bits & (1ull << offset));
I hate that idiom, but if its use is a condition of accepting the patch, I will change it.
Quoted 18 lines
Quoted 7 lines
drivers/gpio/gpio-octeon.c new file +6
-0+ pdev->dev.platform_data = chip; + chip->label ="octeon-gpio" ; + chip->dev = &pdev->dev; + chip->owner =THIS_MODULE ; + chip->base =0 ; + chip->can_sleep =0 ;drivers/gpio/gpio-octeon.c line 120
Quoted 10 lines
drivers/gpio/gpio-octeon.c new file +10
-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" );
Quoted 3 lines
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
Quoted 2 lines · David Daney
Sorry for not responding earlier, but my e-mail system seems to have malfunctioned with respect to this message...
[]
Quoted 10 lines · Linus Walleij
Quoted 9 lines
drivers/gpio/gpio-octeon.c line 74
Quoted 6 lines
drivers/gpio/gpio-octeon.c new file +6
-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 ;A common idiom we use for this is:
Quoted 4 lines
Quoted 1 line
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.
Quoted 14 lines · Joe Perches
Quoted 2 lines · David Daney
Sorry for not responding earlier, but my e-mail system seems to have malfunctioned with respect to this message...
[]
Quoted 10 lines · Linus Walleij
Quoted 9 lines
drivers/gpio/gpio-octeon.c line 74
Quoted 6 lines
drivers/gpio/gpio-octeon.c new file +6
-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 ;A common idiom we use for this is:
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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
Quoted 20 lines · David Daney
Quoted 19 lines · Joe Perches
Quoted 2 lines · David Daney
Sorry for not responding earlier, but my e-mail system seems to have malfunctioned with respect to this message...
[]
Quoted 14 lines · Linus Walleij
Quoted 10 lines
drivers/gpio/gpio-octeon.c line 74
Quoted 6 lines
drivers/gpio/gpio-octeon.c new file +6
-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 ;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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 21 lines · Joe Perches
Quoted 20 lines · David Daney
Quoted 19 lines · Joe Perches
Quoted 2 lines · David Daney
Sorry for not responding earlier, but my e-mail system seems to have malfunctioned with respect to this message...
[]
Quoted 14 lines · Linus Walleij
Quoted 10 lines
drivers/gpio/gpio-octeon.c line 74
Quoted 6 lines
drivers/gpio/gpio-octeon.c new file +6
-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 ;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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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.odrivers/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]
Quoted 11 lines
Quoted 6 lines
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.
Quoted 5 lines · David Daney
Quoted 3 lines · Linus Walleij
drivers/gpio/gpio-octeon.c new file +2
-0+#include <asm/octeon/octeon.h> +#include <asm/octeon/cvmx-gpio-defs.h>
Quoted 8 lines
Quoted 1 line
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.hDo you not have these?
Yeah no problem, I must have misgrepped.
Sorry for the fuzz...
Quoted 5 lines
Quoted 2 lines
depend on OF as well right? Or does the
CAVIUM_OCTEON_SOCalready imply that?We already have 'select
USE_OF', so I think adding OF here would be redundant.
OK.
Quoted 11 lines
Quoted 8 lines
drivers/gpio/gpio-octeon.c line 25
Quoted 5 lines
drivers/gpio/gpio-octeon.c new file +5
-0+/* +* 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.)
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
drivers/gpio/gpio-octeon.c new file +5
-0+{ +if (gpio <16 ) +return 8 * gpio; +else +return 8 * (gpio -16 ) +0x100 ;
Quoted 5 lines
Quoted 2 lines
Put this
0x100in 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?
Quoted 9 lines
Quoted 6 lines
drivers/gpio/gpio-octeon.c line 36
Quoted 4 lines
drivers/gpio/gpio-octeon.c new file +4
-0+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.
Quoted 2 lines
Quoted 2 lines
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...
Quoted 10 lines
Quoted 10 lines
drivers/gpio/gpio-octeon.c line 74
Quoted 7 lines
drivers/gpio/gpio-octeon.c new file +3
-0+static int octeon_gpio_get (struct gpio_chip *chip,unsigned offset) +{ + struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio,chip);
drivers/gpio/gpio-octeon.c new file +3
-0+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:
Quoted 4 lines
Quoted 1 line
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 ;-)
Quoted 7 lines
Quoted 6 lines
Quoted 1 line
+
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.
Quoted 5 lines
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
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
Quoted 123 lines · Linus Walleij
Quoted 5 lines · David Daney
Quoted 3 lines · Linus Walleij
drivers/gpio/gpio-octeon.c new file +2
-0+#include <asm/octeon/octeon.h> +#include <asm/octeon/cvmx-gpio-defs.h> Quoted 8 lines
Quoted 1 line
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.hDo you not have these?
Yeah no problem, I must have misgrepped.
Sorry for the fuzz...Quoted 5 lines
Quoted 2 lines
depend on OF as well right? Or does the
CAVIUM_OCTEON_SOCalready imply that?We already have 'select
USE_OF', so I think adding OF here would be redundant.OK.
Quoted 11 lines
Quoted 8 lines
drivers/gpio/gpio-octeon.c line 25
Quoted 5 lines
drivers/gpio/gpio-octeon.c new file +5
-0+/* +* 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.)Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
drivers/gpio/gpio-octeon.c new file +5
-0+{ +if (gpio <16 ) +return 8 * gpio; +else +return 8 * (gpio -16 ) +0x100 ;Quoted 5 lines
Quoted 2 lines
Put this
0x100in 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?
Quoted 9 lines
Quoted 6 lines
drivers/gpio/gpio-octeon.c line 36
Quoted 4 lines
drivers/gpio/gpio-octeon.c new file +4
-0+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.
Quoted 2 lines
Quoted 2 lines
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...
Quoted 10 lines
Quoted 10 lines
drivers/gpio/gpio-octeon.c line 74
Quoted 7 lines
drivers/gpio/gpio-octeon.c new file +3
-0+static int octeon_gpio_get (struct gpio_chip *chip,unsigned offset) +{ + struct octeon_gpio *gpio = container_of(chip, struct octeon_gpio,chip);
drivers/gpio/gpio-octeon.c new file +3
-0+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:
Quoted 4 lines
Quoted 1 line
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 ;-)
Quoted 7 lines
Quoted 6 lines
Quoted 1 line
+
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.Quoted 5 lines
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 GPIOthen I guess it's convention and it can stay like this.
Yours,
Linus Walleij