Re: [PATCH v3 05/42] pinctrl: add a Cirrus ep93xx SoC pin controller

[Andy Shevchenko <andy@xxxxxxxxxx>]

On Thu, Jul 20, 2023 at 02:29:05PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> 
> This adds 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.

...

> +config PINCTRL_EP93XX
> +	bool
> +	depends on OF && (ARCH_EP93XX || COMPILE_TEST)

The OF seems to be functional dependency and not compile time one.
Thus

	depends on (OF && ARCH_EP93XX) || COMPILE_TEST

> +	select PINMUX
> +	select GENERIC_PINCONF
> +	select MFD_SYSCON

...

> +#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 */

> +#define PADS_MASK		(GENMASK(30, 25) | BIT(22) | BIT(21) | GENMASK(11, 6) | BIT(4))

Seems better to spell each bit as by definition given above.

...

> +/* Ordered by bit index */
> +static const char * const ep93xx_padgroups[] = {
> +	NULL, NULL, NULL, NULL,
> +	"RasOnP3",
> +	NULL,
> +	"I2SonAC97",
> +	"I2SonSSP",
> +	"EonIDE",
> +	"PonG",
> +	"GonIDE",
> +	"HonIDE",
> +	NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +	"A1onG",
> +	"A2onG",
> +	NULL, NULL,
> +	"MonG",
> +	"TonG",
> +	"GonK",
> +	"IonU2",
> +	"D0onG",
> +	"D1onG",

Instead of tons of NULLs, use

	[NN] = "...",

which is much less error prone.

> +};

...

> +/** ep9301, ep9302*/

Is it really kernel doc (besides missing space)?
Please, run

	scripts/kernel-doc -v -Wall -none $YOUR_FILE

for each file you contributed in the entire series and fix all warnings.

...

> +static const char *ep93xx_get_group_name(struct pinctrl_dev *pctldev,
> +					 unsigned int selector)
> +{
> +	struct ep93xx_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	switch (pmx->model) {
> +	case EP93XX_9301_PINCTRL:
> +		return ep9301_pin_groups[selector].grp.name;
> +	case EP93XX_9307_PINCTRL:
> +		return ep9307_pin_groups[selector].grp.name;
> +	case EP93XX_9312_PINCTRL:
> +		return ep9312_pin_groups[selector].grp.name;
> +	}

> +	return NULL;

Use default case instead.

> +}

...

> +	/* Which bits changed */
> +	before &= PADS_MASK;
> +	after &= PADS_MASK;
> +	expected = before & ~grp->mask;
> +	expected |= grp->value;
> +	expected &= PADS_MASK;

Instead of above:

	expected = (before & ~grp->mask) | grp->value;

> +	/* Print changed states */
> +	tmp = expected ^ after;

	tmp = (expected ^ after) & PADS_MASK;

> +	for_each_set_bit(i, &tmp, PADS_MAXBIT) {
> +		bool enabled = expected & BIT(i);
> +
> +		dev_err(pmx->dev,
> +			    "pin group %s could not be %s: probably a hardware limitation\n",
> +			    ep93xx_padgroups[i], str_enabled_disabled(enabled));

Wrong indentation.

> +		dev_err(pmx->dev,
> +				"DeviceCfg before: %08x, after %08x, expected %08x\n",
> +				before, after, expected);

Wrong indentation.

I believe this one can go to debug level.

> +	}

...

> +	pmx->model = (int)of_device_get_match_data(dev);

(uintptr_t) is more appropriate here.

...

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

Why not dev_err_probe() as elsewhere?

> +	}

...

> +static int __init ep93xx_pmx_init(void)
> +{
> +	return platform_driver_register(&ep93xx_pmx_driver);
> +}
> +arch_initcall(ep93xx_pmx_init);

+ blank line.

Also add everywhere MODULE_DESCRIPTION() as modpost recently started to
complain (probably with `make W=1` which you should execute anyway for
your new code).

> +MODULE_IMPORT_NS(EP93XX_SOC);

-- 
With Best Regards,
Andy Shevchenko