| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 9 Nov 2022 17:40:39 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Yinbo Zhu <zhuyinbo@...ngson.cn>
Cc: Stephen Rothwell <sfr@...b.auug.org.au>,
Linus Walleij <linus.walleij@...aro.org>,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
zhanghongchen <zhanghongchen@...ngson.cn>
Subject: Re: [PATCH v8 1/2] pinctrl: pinctrl-loongson2: add pinctrl driver
support
On Wed, Nov 09, 2022 at 02:11:21PM +0800, Yinbo Zhu wrote:
> The Loongson-2 SoC has a few pins that can be used as GPIOs or take
> multiple other functions. Add a driver for the pinmuxing.
>
> There is currently no support for GPIO pin pull-up and pull-down.
> Signed-off-by: zhanghongchen <zhanghongchen@...ngson.cn>
> Signed-off-by: Yinbo Zhu <zhuyinbo@...ngson.cn>
Why two SoBs? Who is(are) the author(s)?
...
> + help
> + This selects pin control driver for the Loongson-2 SoC. It
One space is enough.
> + provides pin config functions multiplexing. GPIO pin pull-up,
> + pull-down functions are not supported. Say yes to enable
> + pinctrl for Loongson-2 SoC.
Perhaps keep your entry in order?
> source "drivers/pinctrl/actions/Kconfig"
> source "drivers/pinctrl/aspeed/Kconfig"
> source "drivers/pinctrl/bcm/Kconfig"
...
> @@ -29,6 +29,7 @@ obj-$(CONFIG_PINCTRL_KEEMBAY) += pinctrl-keembay.o
> obj-$(CONFIG_PINCTRL_LANTIQ) += pinctrl-lantiq.o
> obj-$(CONFIG_PINCTRL_FALCON) += pinctrl-falcon.o
> obj-$(CONFIG_PINCTRL_XWAY) += pinctrl-xway.o
> +obj-$(CONFIG_PINCTRL_LOONGSON2) += pinctrl-loongson2.o
I would expect more order here...
> obj-$(CONFIG_PINCTRL_LPC18XX) += pinctrl-lpc18xx.o
> obj-$(CONFIG_PINCTRL_MAX77620) += pinctrl-max77620.o
> obj-$(CONFIG_PINCTRL_MCP23S08_I2C) += pinctrl-mcp23s08_i2c.o
...
> + * Author: zhanghongchen <zhanghongchen@...ngson.cn>
> + * Yinbo Zhu <zhuyinbo@...ngson.cn>
Missed Co-developed-by tag above?
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
I found no user of this header.
But you missed mod_devicetable.h.
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
Can we keep it as a separate group after generic linux/* ones?
> +#include <linux/seq_file.h>
+ Blank line.
> +#include <asm-generic/io.h>
No, use linux/io.h.
...
> +#define PMX_GROUP(grp, offset, bitv) \
> + { \
> + .name = #grp, \
> + .pins = grp ## _pins, \
> + .num_pins = ARRAY_SIZE(grp ## _pins), \
> + .reg = offset, \
> + .bit = bitv, \
> + }
Use PINCTRL_PINGROUP() and associated data structure.
...
> +static const unsigned int lio_pins[] = {};
> +static const unsigned int uart2_pins[] = {};
> +static const unsigned int uart1_pins[] = {};
> +static const unsigned int camera_pins[] = {};
> +static const unsigned int dvo1_pins[] = {};
> +static const unsigned int dvo0_pins[] = {};
No sure what this means.
...
> +static struct loongson2_pmx_group loongson2_pmx_groups[] = {
> + PMX_GROUP(gpio, 0x0, 64),
> + PMX_GROUP(sdio, 0x0, 20),
> + PMX_GROUP(can1, 0x0, 17),
> + PMX_GROUP(can0, 0x0, 16),
> + PMX_GROUP(pwm3, 0x0, 15),
> + PMX_GROUP(pwm2, 0x0, 14),
> + PMX_GROUP(pwm1, 0x0, 13),
> + PMX_GROUP(pwm0, 0x0, 12),
> + PMX_GROUP(i2c1, 0x0, 11),
> + PMX_GROUP(i2c0, 0x0, 10),
> + PMX_GROUP(nand, 0x0, 9),
> + PMX_GROUP(sata_led, 0x0, 8),
> + PMX_GROUP(lio, 0x0, 7),
> + PMX_GROUP(i2s, 0x0, 6),
> + PMX_GROUP(hda, 0x0, 4),
> + PMX_GROUP(uart2, 0x8, 13),
> + PMX_GROUP(uart1, 0x8, 12),
> + PMX_GROUP(camera, 0x10, 5),
> + PMX_GROUP(dvo1, 0x10, 4),
> + PMX_GROUP(dvo0, 0x10, 1),
> +
Redundant blank line.
> +};
...
> +static const char * const gpio_groups[] = {
> + "sdio", "can1", "can0", "pwm3", "pwm2", "pwm1", "pwm0", "i2c1",
> + "i2c0", "nand", "sata_led", "lio", "i2s", "hda", "uart2", "uart1",
> + "camera", "dvo1", "dvo0"
Leave trailing comma.
Also it would be nice to have that grouped like
"sdio",
"can1", "can0",
"pwm3", "pwm2", "pwm1", "pwm0",
"i2c1", "i2c0",
"nand",
"sata_led",
"lio",
"i2s", "hda",
"uart2", "uart1",
"camera",
"dvo1", "dvo0",
> +};
...
> + unsigned long reg = (unsigned long)pctrl->reg_base +
> + loongson2_pmx_groups[group_num].reg;
Why casting?!
...
> + val = readl((void *)reg);
Ouch.
> + if (func_num == 0)
> + val &= ~(1<<mux_bit);
> + else
> + val |= (1<<mux_bit);
Why not using __assign_bit() or similar? Or at least BIT() ?
...
> + writel(val, (void *)reg);
Ouch!
...
> + pctrl->reg_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(pctrl->reg_base))
> + return dev_err_probe(pctrl->dev, PTR_ERR(pctrl->reg_base),
> + "unable to map I/O memory");
Message duplicates what core does.
...
> + pctrl->desc.confops = NULL;
Redundant.
...
> +static const struct of_device_id loongson2_pinctrl_dt_match[] = {
> + {
> + .compatible = "loongson,ls2k-pinctrl",
> + },
> + { },
No comma for the terminator line.
> +};
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists