On Wed, Nov 09, 2022 at 02:11:21PM +0800, Yinbo Zhu wrote:There are some fuzzy reuse relationships on loongson2, e.g., lio can be
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@xxxxxxxxxxx>
Signed-off-by: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
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@xxxxxxxxxxx>
+ * Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
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.
There are some registers that determine the pin reuse relationship. For example, register A determines the sdio reuse relationship, and register A+offset determines the uart reuse relationship
...
+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?!
I will keep writel/readl.
...
+ 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.
+};