Messages in this thread | | | From | Linus Walleij <> | Date | Wed, 23 Nov 2022 23:14:31 +0100 | Subject | Re: [PATCH v6 1/2] gpio: loongson: add gpio driver support |
| |
On Wed, Nov 23, 2022 at 9:04 AM 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 v6:
This is way better :)
I guess you notice how the driver gets smaller and smaller. This is a good sign!
> +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.
> +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.
> +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)
> + 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...
> +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()?
Yours, Linus Walleij
| |