Re: [PATCH v4 5/6] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[Linus Walleij]

On Thu, Feb 16, 2017 at 2:01 AM, David Daney <david.daney@xxxxxxxxxx> wrote:

> Cavium ThunderX and OCTEON-TX are arm64 based SoCs.  Add driver for
> the on-chip GPIO pins.
>
> Signed-off-by: David Daney <david.daney@xxxxxxxxxx>

Hm due to an API change in the upstream kernel you will still have
to respin this driver, but I'm sure you will fix it in 5 minutes.

> +#define  GPIO_BIT_CFG_FIL_CNT_SHIFT    4
> +#define  GPIO_BIT_CFG_FIL_SEL_SHIFT    8
(...)
> +#define GLITCH_FILTER_400NS ((4ull << GPIO_BIT_CFG_FIL_SEL_SHIFT) | \
> +                            (9ull << GPIO_BIT_CFG_FIL_CNT_SHIFT))

This is what we call debounce in the API I think.

You can consider implementing it below in the .set_config()
function:

> +/*
> + * Weird, setting open-drain mode causes signal inversion.  Note this
> + * so we can compensate in the dir_out function.
> + */
> +static int thunderx_gpio_set_single_ended(struct gpio_chip *chip,
> +                                         unsigned int line,
> +                                         enum single_ended_mode mode)


This function is renamed .set_config() in v4.11.

This can also be used for setting debounce for the line, and
you proabably at some point want to add support for using the
more elaborate settings in the glitch filter for that.

> +       chip->label = KBUILD_MODNAME;
> +       chip->parent = dev;
> +       chip->owner = THIS_MODULE;
> +       chip->base = -1; /* System allocated */
> +       chip->can_sleep = false;
> +       chip->ngpio = ngpio;
> +       chip->direction_input = thunderx_gpio_dir_in;
> +       chip->get = thunderx_gpio_get;
> +       chip->direction_output = thunderx_gpio_dir_out;
> +       chip->set = thunderx_gpio_set;
> +       chip->set_multiple = thunderx_gpio_set_multiple;
> +       chip->set_single_ended = thunderx_gpio_set_single_ended;

So .set_config() is what we call this now. See converted drivers like
gpio-wcove.c etc for examples.

Hm no .get_direction() callback? I suggest to add that. It is useful
for the userspace ABI and more.

You could also add a ->names array with names for all the
lines if you like. This is also nice for userspace and for PCI-based
drivers that is the only way to insert line names for the chip.

Yours,
Linus Walleij