Re: [PATCH v8 1/2] pinctrl: pinctrl-loongson2: add pinctrl driver support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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@loongson.cn>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.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@loongson.cn>
> + *         Yinbo Zhu <zhuyinbo@loongson.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