Re: [PATCH v5 2/3] gpio: loongson: add gpio driver support

[Linus Walleij <linus.walleij@linaro.org>]

On Mon, Nov 21, 2022 at 1:38 PM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:

> The Loongson platforms GPIO controller contains 60 GPIO pins in total,
> 4 of which are dedicated GPIO pins, and the remaining 56 are reused
> with other functions. Each GPIO can set input/output and has the
> interrupt capability.
>
> This driver added support for Loongson GPIO controller and support to
> use DTS or ACPI to descibe GPIO device resources.
>
> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
> Change in v5:

This is starting to look really good! We are getting to the final polish.

> +config GPIO_LOONGSON
> +       tristate "Loongson GPIO support"
> +       depends on LOONGARCH || COMPILE_TEST

select GPIO_GENERIC

You should use this in the "bit mode".

>  obj-$(CONFIG_GPIO_LOONGSON1)           += gpio-loongson1.o
> +obj-$(CONFIG_GPIO_LOONGSON)            += gpio-loongson.o

Isn't this a bit confusing? What about naming it
gpio-loongson2.c?

> +enum loongson_gpio_mode {
> +       BIT_CTRL_MODE,
> +       BYTE_CTRL_MODE,
> +};

I don't think you will need to track this, jus assume BYTE_CTRL_MODE
in your callbacks because we will replace the bit mode with assigned
accessors from GPIO_GENERIC.

> +
> +struct loongson_gpio_platform_data {
> +       const char              *label;
> +       enum loongson_gpio_mode mode;

So drop this.

> +static int loongson_gpio_request(
> +                       struct gpio_chip *chip, unsigned int pin)
> +{
> +       if (pin >= chip->ngpio)
> +               return -EINVAL;

This is not needed, the gpiolib core already checks this. Drop it.

> +static inline void __set_direction(struct loongson_gpio_chip *lgpio,
> +                       unsigned int pin, int input)
> +{
> +       u64 qval;
> +       u8  bval;
> +
> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
> +               qval = readq(LOONGSON_GPIO_OEN(lgpio));
> +               if (input)
> +                       qval |= 1ULL << pin;
> +               else
> +                       qval &= ~(1ULL << pin);
> +               writeq(qval, LOONGSON_GPIO_OEN(lgpio));
> +       } else {
> +               bval = input ? 1 : 0;
> +               writeb(bval, LOONGSON_GPIO_OEN_BYTE(lgpio, pin));
> +       }

Drop bit mode keep only byte mode.

> +static void __set_level(struct loongson_gpio_chip *lgpio, unsigned int pin,
> +                       int high)
> +{
> +       u64 qval;
> +       u8 bval;
> +
> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
> +               qval = readq(LOONGSON_GPIO_OUT(lgpio));
> +               if (high)
> +                       qval |= 1ULL << pin;
> +               else
> +                       qval &= ~(1ULL << pin);
> +               writeq(qval, LOONGSON_GPIO_OUT(lgpio));
> +       } else {
> +               bval = high ? 1 : 0;
> +               writeb(bval, LOONGSON_GPIO_OUT_BYTE(lgpio, pin));
> +       }

Dito.

> +static int loongson_gpio_get(struct gpio_chip *chip, unsigned int pin)
> +{
> +       u64 qval;
> +       u8  bval;
> +       int val;
> +
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
> +
> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
> +               qval = readq(LOONGSON_GPIO_IN(lgpio));
> +               val = (qval & (1ULL << pin)) != 0;
> +       } else {
> +               bval = readb(LOONGSON_GPIO_IN_BYTE(lgpio, pin));
> +               val = bval & 1;
> +       }

Dito.

> +static int loongson_gpio_to_irq(
> +                       struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct platform_device *pdev =
> +               container_of(chip->parent, struct platform_device, dev);
> +       struct loongson_gpio_chip *lgpio =
> +               container_of(chip, struct loongson_gpio_chip, chip);
> +
> +       if (offset >= chip->ngpio)
> +               return -EINVAL;
> +
> +       if ((lgpio->gsi_idx_map != NULL) && (offset < lgpio->mapsize))
> +               offset = lgpio->gsi_idx_map[offset];
> +       else
> +               return -EINVAL;
> +
> +       return platform_get_irq(pdev, offset);
> +}

I'm a bit suspicious about this. See the following in
Documentation/driver-api/gpio/driver.rst:

------------------
It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
irq_chip are orthogonal, and offering their services independent of each
other.

gpiod_to_irq() is just a convenience function to figure out the IRQ for a
certain GPIO line and should not be relied upon to have been called before
the IRQ is used.

Always prepare the hardware and make it ready for action in respective
callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having
been called first.
------------------

I am bit suspicious that your IRQchip implementation expects consumers
to call gpiod_to_irq() first and this is not legal.

> +static int loongson_gpio_init(
> +                       struct device *dev, struct loongson_gpio_chip *lgpio,
> +                       struct device_node *np, void __iomem *base)
> +{

Do something like this:

#define LOONGSON_GPIO_IN(x)            (x->base + x->p_data->in_offset)
+#define LOONGSON_GPIO_OUT(x)           (x->base + x->p_data->out_offset)
+#define LOONGSON_GPIO_OEN(x)           (x->base + x->p_data->conf_offset)

if (lgpio->p_data->mode == BIT_CTRL_MODE) {
       ret = bgpio_init(&g->gc, dev, 8,
                         lgpio->base + lgpio->p_data->in_offset,
                         lgpio->base + lgpio->p_data->out_offset,
                         0,
                         lgpio->base + lgpio->p_data->conf_offset,
                         NULL,
                         0);
        if (ret) {
                dev_err(dev, "unable to init generic GPIO\n");
                goto dis_clk;
        }

If you actually have a special purpose clear register in your hardware
which is not included here, then add it in the line with just 0 for that
function.

See the kerneldoc in bgpio_init() in drivers/gpio/gpio-mmio.c.

Then:

}  else {

> +       lgpio->chip.request = loongson_gpio_request;
> +       lgpio->chip.direction_input = loongson_gpio_direction_input;
> +       lgpio->chip.get = loongson_gpio_get;
> +       lgpio->chip.direction_output = loongson_gpio_direction_output;
> +       lgpio->chip.set = loongson_gpio_set;

Note also: implement loongson_gpio_get_direction(). To read the setting
of the conf register on startup. You now only need to implement it for
byte mode.

}

After this you should set ngpios, because bgpio_init() will overwrite it
with 64, so it cannot be done directly when parsing platform data,
cache it somewhere and write it here.

(...)

Yours,
Linus Walleij