| Date | Wed, 22 Nov 2023 13:50:47 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v5 04/39] pinctrl: add a Cirrus ep93xx SoC pin controller |
| |
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
|