lkml.org 
[lkml]   [2022]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 2/3] gpio: loongson: add gpio driver support
From
Date


在 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
>

\
 
 \ /
  Last update: 2022-11-23 09:03    [W:0.112 / U:0.748 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site