Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570780 > unrolled thread

Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver Linus Walleij <linus.walleij@linaro.org> - 2017-01-31 15:30 +0100
    Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver Paul Cercueil <paul@crapouillou.net> - 2017-02-09 18:20 +0100
      Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver Linus Walleij <linus.walleij@linaro.org> - 2017-02-12 21:50 +0100

#1570780 — Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver

On Wed, Jan 25, 2017 at 7:51 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> This driver handles the GPIOs of all the Ingenic JZ47xx SoCs
> currently supported by the upsteam Linux kernel.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Looking nice.

> +#define JZ4740_GPIO_DATA       0x10
> +#define JZ4740_GPIO_SELECT     0x50
> +#define JZ4740_GPIO_DIR                0x60
> +#define JZ4740_GPIO_TRIG       0x70
> +#define JZ4740_GPIO_FLAG       0x80
> +
> +#define JZ4780_GPIO_INT                0x10
> +#define JZ4780_GPIO_PAT1       0x30
> +#define JZ4780_GPIO_PAT0       0x40
> +#define JZ4780_GPIO_FLAG       0x50
> +
> +#define REG_SET(x) ((x) + 0x4)
> +#define REG_CLEAR(x) ((x) + 0x8)
(...)
> +enum jz_version {
> +       ID_JZ4740,
> +       ID_JZ4780,
> +};
(...)
> +static inline bool gpio_get_value(struct ingenic_gpio_chip *jzgc, u8 offset)
> +{
> +       if (jzgc->version >= ID_JZ4780)
> +               return readl(jzgc->base + GPIO_PIN) & BIT(offset);
> +       else
> +               return readl(jzgc->base + JZ4740_GPIO_DATA) & BIT(offset);
> +}

This works for me, for sure.

What some people do, is to put the right virtual address in to the state
container.

So it would be just:

return !!readl(jzgc->datareg) & BIT(offset));

Notice also the double-bang that clamps the value to a bool. I know
the core does it too but I like to see it in drivers just to be sure.

> +static void gpio_set_value(struct ingenic_gpio_chip *jzgc, u8 offset, int value)
> +{
> +       u8 reg;
> +
> +       if (jzgc->version >= ID_JZ4780)
> +               reg = JZ4780_GPIO_PAT0;
> +       else
> +               reg = JZ4740_GPIO_DATA;
> +
> +       if (value)
> +               writel(BIT(offset), jzgc->base + REG_SET(reg));
> +       else
> +               writel(BIT(offset), jzgc->base + REG_CLEAR(reg));
> +}

Same comment.

What some drivers do when they just get/set a bit in a register
to get/set or set the direction of a GPIO, is to select GPIO_GENERIC
and just bgpio_init() with the right iomem pointers, then the core
will register handlers for get, set, set_direcition callback and
get_direction and your driver can just focus on the remainders.

> +static void ingenic_gpio_set(struct gpio_chip *gc,
> +               unsigned int offset, int value)
> +{
> +       struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
> +
> +       gpio_set_value(jzgc, offset, value);
> +}
> +
> +static int ingenic_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
> +
> +       return (int) gpio_get_value(jzgc, offset);
> +}
> +
> +static int ingenic_gpio_direction_input(struct gpio_chip *gc,
> +               unsigned int offset)
> +{
> +       return pinctrl_gpio_direction_input(gc->base + offset);
> +}
> +
> +static int ingenic_gpio_direction_output(struct gpio_chip *gc,
> +               unsigned int offset, int value)
> +{
> +       ingenic_gpio_set(gc, offset, value);
> +       return pinctrl_gpio_direction_output(gc->base + offset);
> +}

If you're not just replacing these with GPIO_GENERIC, please also
include a .get_direction() callback.

It's especially nice as it reads out the state at probe and "lsgpio"
lists if pins are inputs or outputs.

Yours,
Linus Walleij

[toc] | [next] | [standalone]

#1577800

Hi,

> What some drivers do when they just get/set a bit in a register
> to get/set or set the direction of a GPIO, is to select GPIO_GENERIC
> and just bgpio_init() with the right iomem pointers, then the core
> will register handlers for get, set, set_direcition callback and
> get_direction and your driver can just focus on the remainders.

GPIO_GENERIC and bgpio_init() would work for my .set() / .get() 
callbacks,
not for my .direction_input() / .direction_output() callbacks which need
to set more than one register.

> If you're not just replacing these with GPIO_GENERIC, please also
> include a .get_direction() callback.

My .direction_input() and .direction_output() callbacks just call into
the pinctrl driver, using pinctrl_gpio_direction_[in,out]put().
I didn't find a way to get the direction info from the pinctrl driver,
is that something that the core should provide?

Thanks,
-Paul

[toc] | [prev] | [next] | [standalone]

#1579276

On Thu, Feb 9, 2017 at 6:14 PM, Paul Cercueil <paul@crapouillou.net> wrote:

>> If you're not just replacing these with GPIO_GENERIC, please also
>> include a .get_direction() callback.
>
> My .direction_input() and .direction_output() callbacks just call into
> the pinctrl driver, using pinctrl_gpio_direction_[in,out]put().
> I didn't find a way to get the direction info from the pinctrl driver,
> is that something that the core should provide?

Hm OK you have a clear point there, there is no such callback.

OK I do not require you to fix that at this time.

I am hesitant about providing ever more callbacks from GPIO
to pin control, I might need some help for consolidation here.

With Mika's patches we have a .set_config() call to
pinctrl_gpio_set_config() so essentially
we *could* actually refactor all pin control drivers providing
a GPIO back-end to use:

pinctrl_gpio_set_config(gpio, PIN_CONF_PACKED(PIN_CONFIG_INPUT_ENABLE, 0));
pinctrl_gpio_set_config(gpio, PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val));

And replace the calls to pinctrl_gpio_direction_input()
and pinctrl_gpio_direction_output() with this throughout.

It makes things a bit simpler. If we need to figure things
out the reverse direction then pinctrl_gpio_get_config()
should be implemented and used as back-end for
figuring out direction.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web