Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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 wrote:
On 06/17/2013 01:51 AM, Linus Walleij wrote:
+#include
+#include
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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
On Thu, Jun 20, 2013 at 8:10 PM, David Daney wrote:
> On 06/17/2013 01:51 AM, Linus Walleij wrote:
>> +#include
>> +#include
>>
>> 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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
On Thu, Jun 20, 2013 at 8:10 PM, David Daney ddaney.c...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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.c...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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 wrote:
From: David Daney
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
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
+#include
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 ?
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 = >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(>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
Re: Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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.c...@gmail.com wrote:
From: David Daney david.da...@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.da...@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'
Re: Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
On Sat, Jun 15, 2013 at 1:18 AM, David Daney wrote:
> From: David Daney
>
> 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
> 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
+#include
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 ?
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 = >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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
On Sat, Jun 15, 2013 at 1:18 AM, David Daney ddaney.c...@gmail.com wrote:
From: David Daney david.da...@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.da...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
From: David Daney
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
---
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 000..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
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#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(>dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+ chip = >chip;
+
+ res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res_mem == NULL) {
+ dev_err(>dev, "found no memory resource\n");
+ err = -ENXIO;
+ goto out;
+ }
+ if (!devm_request_mem_region(>dev, res_mem->start,
+ resource_size(res_mem),
+res_mem->name)) {
+ dev_err(>dev, "request_mem_region failed\n");
+
[PATCH] gpio MIPS/OCTEON: Add a driver for OCTEON's on-chip GPIO pins.
From: David Daney david.da...@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.da...@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 000..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,
+
