Re: [PATCH v5 04/39] pinctrl: add a Cirrus ep93xx SoC pin controller
From: Andy Shevchenko
Date: Wed Nov 22 2023 - 06:50:57 EST
On Wed, Nov 22, 2023 at 11:59:42AM +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.
...
> +config PINCTRL_EP93XX
> + bool
> + depends on OF && (ARCH_EP93XX || COMPILE_TEST)
How OF is here?
...
> +#include <linux/of.h>
Ditto.
...
> +#include <linux/slab.h>
+ blank line?
> +#include <linux/soc/cirrus/ep93xx.h>
...
> +/* ep9301, ep9302*/
Missing space.
...
> +static const unsigned int ssp_ep9301_pins[] = {
> + 93, 94, 95, 96
In multi-line definitions like this it makes sense to leave trailing comma.
> +};
> +
> +static const unsigned int ac97_ep9301_pins[] = {
> + 89, 92, 107, 154, 156
Ditto.
And in some other places.
> +};
...
> + /* Row C*/
Missing space. I noticed in more comments like this, please grep and fix
all of them.
...
> +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;
Make it default case.
> +}
...
> + dev_dbg(pmx->dev,
> + "before=0x%x, after=0x%x, mask=0x%lx\n",
> + before, after, PADS_MASK);
At least two first can be on a single line.
...
> + /* Which bits changed */
> + before &= PADS_MASK;
> + after &= PADS_MASK;
> + expected = before & ~grp->mask;
> + expected |= grp->value;
Usually we use this pattern:
expected = (before & ~grp->mask) | (grp->value & grp->mask);
but I don't know the full spectre of the meanings of these pieces, so just
consider it once more.
> + expected &= PADS_MASK;
...
> + pmx->model = (int)(id->driver_data);
Wouldn't it warn? Maybe not (it's 32-bit code, right?), but better to use
pmx->model = (int)(uintptr_t)id->driver_data;
...
> + /* using parent of_node to match in get_pinctrl_dev_from_of_node() */
> + dev->of_node = adev->dev.parent->of_node;
device_set_node()
...
> + 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() here?
> + }
--
With Best Regards,
Andy Shevchenko