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).
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 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
On Sat, Jun 15, 2013 at 1:18 AM, David Daney <[email protected]> wrote:
> 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).
>
> 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 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
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 <[email protected]> wrote:
>
>> 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).
>>
>> 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 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
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.
>
>
>
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
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.
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.
On Thu, Jun 20, 2013 at 8:10 PM, David Daney <[email protected]> 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
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 <[email protected]> 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
>
>