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