[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a749cb75-eea8-c1af-5a07-5d85c7fc17ff@loongson.cn>
Date: Mon, 28 Nov 2022 14:25:25 +0800
From: Yinbo Zhu <zhuyinbo@...ngson.cn>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
WANG Xuerui <kernel@...0n.name>,
Jiaxun Yang <jiaxun.yang@...goat.com>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Juxin Gao <gaojuxin@...ngson.cn>,
Bibo Mao <maobibo@...ngson.cn>,
Yanteng Si <siyanteng@...ngson.cn>, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
loongarch@...ts.linux.dev, linux-mips@...r.kernel.org,
Arnaud Patard <apatard@...driva.com>,
Huacai Chen <chenhuacai@...nel.org>,
Jianmin Lv <lvjianmin@...ngson.cn>,
Hongchen Zhang <zhanghongchen@...ngson.cn>,
Liu Peibao <liupeibao@...ngson.cn>
Subject: Re: [PATCH v6 1/2] gpio: loongson: add gpio driver support
在 2022/11/24 6:14, Linus Walleij 写道:
> On Wed, Nov 23, 2022 at 9:04 AM Yinbo Zhu <zhuyinbo@...ngson.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@...ngson.cn>
>> Signed-off-by: Hongchen Zhang <zhanghongchen@...ngson.cn>
>> Signed-off-by: Liu Peibao <liupeibao@...ngson.cn>
>> Signed-off-by: Juxin Gao <gaojuxin@...ngson.cn>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@...ngson.cn>
>> ---
>> Change in v6:
> This is way better :)
>
> I guess you notice how the driver gets smaller and smaller.
> This is a good sign!
It's thanks to your advice!
>> +static int loongson_gpio_request(
>> + struct gpio_chip *chip, unsigned int pin)
>> +{
>> + if (pin >= chip->ngpio)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
> Drop this altogether as discussed in my other reply.
okay , I got it.
>
>> +static inline void __set_direction(struct loongson_gpio_chip *lgpio,
>> + unsigned int pin, int input)
>> +static void __set_level(struct loongson_gpio_chip *lgpio, unsigned int pin,
>> + int high)
> I missed this before. Also the use of __underscore for inner functions
> is a bad habit IMO (because __underscore is also used for compiler
> primitives such as __init which is confusing) The signature of these
> functions is too generic. Name them loongson_commit_direction() or
> loongson_commit_level() or something.
okay , I got it.
>
>> +static int loongson_gpio_get_direction(
>> + struct gpio_chip *chip, unsigned int pin)
> thanks for implementing this!
>
>> + if (lgpio->p_data->mode == BIT_CTRL_MODE) {
>> + ret = bgpio_init(&lgpio->chip, dev, 8,
>> + LOONGSON_GPIO_IN(lgpio),
>> + LOONGSON_GPIO_OUT(lgpio), 0,
>> + LOONGSON_GPIO_OEN(lgpio), NULL, 0);
>> + if (ret) {
>> + dev_err(dev, "unable to init generic GPIO\n");
>> + return ret;
>> + }
>> + lgpio->chip.ngpio = ngpios;
> Neat!
>
>> + lgpio->chip.base = 0;
> Drop this. It is good that the base is unpredictable so
> people don't start to rely on it. (drivers/gpio/TODO)
okay, I got it.
>
>> + rval = device_property_read_u16_array(dev, "gsi_idx_map", NULL, 0);
> But this gsi_idx_map is missing from your device tree bindings,
> is it not?
>
> Or what am I missing here? Sorry I might overlook something...
gsi_idx_map is for acpi, the dts doesn't use it, and acpi should be
follow dts,
so remove it.
>
>> +static int loongson_gpio_probe(struct platform_device *pdev)
>> +{
>> + void __iomem *reg_base;
>> + struct loongson_gpio_chip *lgpio;
>> + struct device_node *np = pdev->dev.of_node;
>> + struct device *dev = &pdev->dev;
>> +
>> + lgpio = devm_kzalloc(dev, sizeof(*lgpio), GFP_KERNEL);
>> + if (!lgpio)
>> + return -ENOMEM;
>> +
>> + loongson_gpio_get_props(pdev, lgpio);
>> +
>> + lgpio->p_data = device_get_match_data(&pdev->dev);
> lgpio->p_data = device_get_match_data(dev);
>
>
>> +static int __init loongson_gpio_setup(void)
>> +{
>> + return platform_driver_register(&loongson_gpio_driver);
>> +}
>> +postcore_initcall(loongson_gpio_setup);
> Why does this have to be postcore_initcall()?
it was refer other gpio driver. other perpherial driver will use gpio,
the gpio driver
should be loaded as early as possible,so use postcore_initcall();
>
> Yours,
> Linus Walleij
Powered by blists - more mailing lists