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

[Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>]

Hi Andy,

I had add v9 that following your advice, but there are some explain 
about your doubt as follows:

在 2022/11/9 下午11:40, Andy Shevchenko 写道:
> 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@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 fuzzy reuse relationships on loongson2, e.g., lio can be
reused as gpio, which is one of the reserved gpios but the manual not
indicates this gpio index.
> ...
>
> > +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?!
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
> ...
>
> > +	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!
I will keep writel/readl.
> ...
>
> > +	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.
>
> > +};