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

[Florian Fainelli <florian@openwrt.org>]

Le 13/04/2012 18:17, David Daney a écrit :
> 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.

Since it is exposed by the sysfs entry ngpio, you can consider it being 
part of the ABI, and it can be used by an user to query the number of 
gpios available, and will also serve as the maximum number of gpios for 
gpiolib internally, it is certainly best to make it match what the 
hardware can actually do.

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