Re: [PATCH v6 05/40] pinctrl: add a Cirrus ep93xx SoC pin controller

From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Nikita Shubin <nikita.shubin@maquefel.me>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
	Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH v6 05/40] pinctrl: add a Cirrus ep93xx SoC pin controller
Date: Wed, 13 Dec 2023 19:51:25 +0200	[thread overview]
Message-ID: <ZXnvHYjgnc3VsXnX@smile.fi.intel.com> (raw)
In-Reply-To: <20231212-ep93xx-v6-5-c307b8ac9aa8@maquefel.me>

On Tue, Dec 12, 2023 at 11:20:22AM +0300, Nikita Shubin wrote:
> Add a pin control (only multiplexing) driver for ep93xx SoC so
> we can fully convert ep93xx to device tree.
> 
> This driver is capable of muxing ep9301/ep9302/ep9307/ep9312/ep9315
> variants, this is chosen based on "compatible" in device tree.

Mostly nit-picks below, with the exception to setting device node.
See below.

...

> +/*
> + * There are several system configuration options selectable by the DeviceCfg and SysCfg
> + * registers. These registers provide the selection of several pin multiplexing options and also
> + * provide software access to the system reset configuration options. Please refer to the
> + * descriptions of the registers, “DeviceCfg” on page 5-25 and “SysCfg” on page 5-34, for a
> + * detailed explanation.
> + */
> +#define EP93XX_SYSCON_DEVCFG_D1ONG	BIT(30) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_D0ONG	BIT(29) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_IONU2	BIT(28) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_GONK	BIT(27) /* done */
> +#define EP93XX_SYSCON_DEVCFG_TONG	BIT(26) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_MONG	BIT(25) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_A2ONG	BIT(22) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_A1ONG	BIT(21) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_HONIDE	BIT(11) /* done */
> +#define EP93XX_SYSCON_DEVCFG_GONIDE	BIT(10) /* done */
> +#define EP93XX_SYSCON_DEVCFG_PONG	BIT(9) /* done */
> +#define EP93XX_SYSCON_DEVCFG_EONIDE	BIT(8) /* done */
> +#define EP93XX_SYSCON_DEVCFG_I2SONSSP	BIT(7) /* done */
> +#define EP93XX_SYSCON_DEVCFG_I2SONAC97	BIT(6) /* done */
> +#define EP93XX_SYSCON_DEVCFG_RASONP3	BIT(4) /* done */

What are these comments supposed to mean?

...

> +static const struct pinctrl_ops ep93xx_pctrl_ops = {
> +	.get_groups_count = ep93xx_get_groups_count,
> +	.get_group_name = ep93xx_get_group_name,
> +	.get_group_pins = ep93xx_get_group_pins,

> +	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> +	.dt_free_map = pinconf_generic_dt_free_map,

Hmm... Don you need to ifdef these fields?

> +};

...

> +static const struct pinfunction ep93xx_pmx_functions[] = {
> +	PINCTRL_PINFUNCTION("spi", spigrps, ARRAY_SIZE(spigrps)),

Is array_size.h being included?

> +	PINCTRL_PINFUNCTION("ac97", ac97grps, ARRAY_SIZE(ac97grps)),
> +	PINCTRL_PINFUNCTION("i2s", i2sgrps, ARRAY_SIZE(i2sgrps)),
> +	PINCTRL_PINFUNCTION("pwm", pwm1grps, ARRAY_SIZE(pwm1grps)),
> +	PINCTRL_PINFUNCTION("keypad", keypadgrps, ARRAY_SIZE(keypadgrps)),
> +	PINCTRL_PINFUNCTION("pata", idegrps, ARRAY_SIZE(idegrps)),
> +	PINCTRL_PINFUNCTION("lcd", rastergrps, ARRAY_SIZE(rastergrps)),
> +	PINCTRL_PINFUNCTION("gpio", gpiogrps, ARRAY_SIZE(gpiogrps)),
> +};

...

> +	switch (pmx->model) {
> +	case EP93XX_9301_PINCTRL:
> +		grp = &ep9301_pin_groups[group];
> +		break;
> +	case EP93XX_9307_PINCTRL:
> +		grp = &ep9307_pin_groups[group];
> +		break;
> +	case EP93XX_9312_PINCTRL:
> +		grp = &ep9312_pin_groups[group];
> +		break;

default?

> +	}

...

> +	pmx->model = (int)(uintptr_t)id->driver_data;

Is the model defined as int (signed)?

Otherwise can we use proper type?

...

> +	/* using parent of_node to match in get_pinctrl_dev_from_of_node() */
> +	device_set_of_node_from_dev(dev, adev->dev.parent);

Hmm... This takes references in comparison to device_set_node(). Is it intended?

...

> +	pmx->pctl = devm_pinctrl_register(dev, &ep93xx_pmx_desc, pmx);
> +	if (IS_ERR(pmx->pctl))
> +		return dev_err_probe(dev, PTR_ERR(pmx->pctl), "could not register pinmux driver\n");

It can be written as

	pmx->pctl = devm_pinctrl_register(dev, &ep93xx_pmx_desc, pmx);
	ret = PTR_ERR_OR_ZERO(...);
	if (ret)
		return dev_err_probe(dev, ret, "could not register pinmux driver\n");

(makes line shorter). But it's up to you.

-- 
With Best Regards,
Andy Shevchenko