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

[Yinbo Zhu <zhuyinbo@loongson.cn>]

在 2022/11/21 下午9:24, Linus Walleij 写道:
> 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?
This driver was plan to cover all loongson platform, not only Loongson-2
SoC but also other loongson platform. and I adop your advice in another
mail. I named this driver was gpio-loongson-64bit.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.
  yes, but in loongson_gpio_init need a distinction, so I kept this.
> 
>> +
>> +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.
I check gpio_request in gpilib, I notice gpio_is_valid is not equal to
this condition, so I still kept it for byte mode.
> 
>> +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.
okay, I will drop it.
> 
>> +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.
okay, I will drop bit mode.
> 
>> +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.
okay, I will drop bit mode.
> 
>> +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.
okay, I got it, and other driver use gpio interrupt doesn't rely on 
gpiod_to_irq, but can use gpiod_to_irq.

The reason is that gpio interrupt wasn't an independent module,  The
loongson interrupt controller liointc include lots of interrupt was
route to perpherial, such as i2c/spi .. gpio, so gpio interrupt as
normal perpherial interrupt, It is unnecessary and redundant to
implement a gpio irq chip. The liointc controller driver had cover all
interrupt.
> 
>> +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.
okay, I had do it.
in addition,  I don't like to use the dynamically allocated gpio base, 
so I set the gpio base after call bgpio_init.

> 
> 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.
okay, I had implement it.
> 
> }
> 
> 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.
okay, I will do it.
> 
> (...)
> 
> Yours,
> Linus Walleij
>