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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F885185.3070005@gmail.com>
Date:	Fri, 13 Apr 2012 09:17:09 -0700
From:	David Daney <ddaney.cavm@...il.com>
To:	Florian Fainelli <florian@...nwrt.org>
CC:	David Daney <ddaney.cavm@...il.com>,
	Grant Likely <grant.likely@...retlab.ca>, ralf@...ux-mips.org,
	linux-mips@...ux-mips.org,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Rob Herring <rob.herring@...xeda.com>,
	devicetree-discuss@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] gpio/MIPS/OCTEON: Add a driver for OCTEON's on-chip
 GPIO pins.

On 04/13/2012 02:56 AM, Florian Fainelli wrote:
> Hi David,
>
[...]
>> +/*
>> + * 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;
>> +}
>
> You could explicitely inline this one, though the compiler will
> certainly do it by itself.
>

I always let the compiler decide.


[...]
>> +
>> + if (OCTEON_IS_MODEL(OCTEON_CN66XX) ||
>> + OCTEON_IS_MODEL(OCTEON_CN61XX) ||
>> + OCTEON_IS_MODEL(OCTEON_CNF71XX))
>> + chip->ngpio = 20;
>> + else
>> + chip->ngpio = 16;
>
> What about getting the number of gpios from platform_data and/or device
> tree?
>

Actually I am thinking about just setting it to 20 unconditionally. 
Anything requesting a non-present GPIO pin is buggy to begin with.

>> +
>> + 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, "version: " DRV_VERSION "\n");
>> +out:
>> + return err;
>> +}
>> +
>> +static int __exit 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_mgmt_match);
>
> You are using linux/of.h definitions here but you did not include it.
> Also, there is a typo, you want octeon_gpio_match instead.
>

Good catch.  I will fix that.  There is also a section mismatch I need 
to fix.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/