| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 3 Jun 2023 22:58:38 +0300
From: andy.shevchenko@...il.com
To: Nikita Shubin <nikita.shubin@...uefel.me>
Cc: Alexander Sverdlin <alexander.sverdlin@...il.com>,
Arnd Bergmann <arnd@...db.de>,
Linus Walleij <linus.walleij@...aro.org>,
Michael Peters <mpeters@...eddedts.com>,
Kris Bahnsen <kris@...eddedts.com>,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v1 07/43] pinctrl: add a Cirrus ep93xx SoC pin controller
Thu, Jun 01, 2023 at 08:33:58AM +0300, Nikita Shubin kirjoitti:
> 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.
I have a déjà vu that I commented on this already...
...
> +enum ep93xx_pinctrl_model {
> + EP93XX_9301_PINCTRL = 0,
It's not needed, guaranteed by the C standard.
> + EP93XX_9307_PINCTRL,
> + EP93XX_9312_PINCTRL
Keep trailing comma, might help in case of this being extended.
> +};
> +/**
> + * struct ep93xx_pin_group - describes a ep93xx pin group
> + * @name: the name of this specific pin group
> + * @pins: an array of discrete physical pins used in this group, taken
> + * from the driver-local pin enumeration space
> + * @num_pins: the number of pins in this group array, i.e. the number of
> + * elements in .pins so we can iterate over that array
> + * @mask: bits to clear to enable this when doing pin muxing
> + * @value: bits to set to enable this when doing pin muxing
> + */
> +struct ep93xx_pin_group {
> + const char *name;
> + const unsigned int *pins;
> + const unsigned int num_pins;
Please, use struct pingroup.
> + u32 mask;
> + u32 value;
> +};
...
> +static const struct ep93xx_pin_group ep9301_pin_groups[] = {
> + {
> + .name = "ssp",
> + .pins = ssp_ep9301_pins,
> + .num_pins = ARRAY_SIZE(ssp_ep9301_pins),
Use PINCTRL_PINGROUP() respectively.
> + .mask = EP93XX_SYSCON_DEVCFG_I2SONSSP,
> + },
> +};
...
> +static const struct ep93xx_pin_group ep9307_pin_groups[] = {
Ditto.
> +};
...
> +static const struct ep93xx_pin_group ep9312_pin_groups[] = {
Ditto.
> +};
...
> + switch (pmx->model) {
> + case EP93XX_9301_PINCTRL:
> + return ARRAY_SIZE(ep9301_pin_groups);
> + case EP93XX_9307_PINCTRL:
> + return ARRAY_SIZE(ep9307_pin_groups);
> + case EP93XX_9312_PINCTRL:
> + return ARRAY_SIZE(ep9312_pin_groups);
> + }
> +
> + return 0;
Simply make it default: case.
...
> + switch (pmx->model) {
> + case EP93XX_9301_PINCTRL:
> + return ep9301_pin_groups[selector].name;
> + case EP93XX_9307_PINCTRL:
> + return ep9307_pin_groups[selector].name;
> + case EP93XX_9312_PINCTRL:
> + return ep9312_pin_groups[selector].name;
> + }
> +
> + return NULL;
Ditto.
...
> +static void ep93xx_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
> + unsigned int offset)
> +{
> + seq_printf(s, " " DRIVER_NAME);
How this is useful?
> +}
...
> +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,
> + .pin_dbg_show = ep93xx_pin_dbg_show,
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> + .dt_free_map = pinconf_generic_dt_free_map,
Missing ifdeffery?
> +};
...
> +/**
> + * struct ep93xx_pmx_func - describes ep93xx pinmux functions
> + * @name: the name of this specific function
> + * @groups: corresponding pin groups
> + */
> +struct ep93xx_pmx_func {
> + const char *name;
> + const char * const *groups;
> + const unsigned int num_groups;
> +};
Use struct pinfunction instead.
...
> +static const struct ep93xx_pmx_func ep93xx_pmx_functions[] = {
> + {
> + .name = "spi",
> + .groups = spigrps,
> + .num_groups = ARRAY_SIZE(spigrps),
> + },
And PINCTRL_PINFUNCTION() respectively.
> +};
...
> + 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?
> + }
...
> + 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], enabled ? "enabled" : "disabled");
str_enabled_disabled()
> + dev_err(pmx->dev,
> + "DeviceCfg before: %08x, after %08x, expected %08x\n",
> + before, after, expected);
> + }
...
> +static const struct of_device_id ep93xx_pinctrl_of_ids[] = {
> + { .compatible = "cirrus,ep9301-pinctrl", .data = (void *)EP93XX_9301_PINCTRL},
> + { .compatible = "cirrus,ep9302-pinctrl", .data = (void *)EP93XX_9301_PINCTRL},
> + { .compatible = "cirrus,ep9307-pinctrl", .data = (void *)EP93XX_9307_PINCTRL},
> + { .compatible = "cirrus,ep9312-pinctrl", .data = (void *)EP93XX_9312_PINCTRL},
> + { .compatible = "cirrus,ep9315-pinctrl", .data = (void *)EP93XX_9312_PINCTRL},
> + {},
Comma is not needed in the terminator entry.
> +};
...
> + const struct of_device_id *match = of_match_node(ep93xx_pinctrl_of_ids, pdev->dev.of_node);
Why? This is an old API, simply use of_device_get_match_data().
With it you don't need to locate OF ID table too early in the code.
> + struct ep93xx_pmx *pmx;
> + struct regmap *map;
> + struct device *dev = &pdev->dev;
> + struct device *parent;
Longer lines first?
...
> + pmx->dev = &pdev->dev;
... = dev; ?
...
> + parent = dev->parent;
> + if (!parent) {
> + dev_err(dev, "no parent to pin controller\n");
> + return -ENODEV;
return dev_err_probe(...);
> + }
...
> + map = syscon_node_to_regmap(parent->of_node);
> + if (IS_ERR(map)) {
> + dev_err(dev, "no syscon regmap\n");
> + return PTR_ERR(map);
Ditto.
> + }
...
> + switch (pmx->model) {
> + case EP93XX_9301_PINCTRL:
> + ep93xx_pmx_desc.pins = ep9301_pins;
> + ep93xx_pmx_desc.npins = ARRAY_SIZE(ep9301_pins);
> + dev_info(dev, "detected 9301/9302 chip variant\n");
> + break;
> + case EP93XX_9307_PINCTRL:
> + ep93xx_pmx_desc.pins = ep9307_pins;
> + ep93xx_pmx_desc.npins = ARRAY_SIZE(ep9307_pins);
> + dev_info(dev, "detected 9307 chip variant\n");
> + break;
> + case EP93XX_9312_PINCTRL:
> + ep93xx_pmx_desc.pins = ep9312_pins;
> + ep93xx_pmx_desc.npins = ARRAY_SIZE(ep9312_pins);
> + dev_info(dev, "detected 9312/9315 chip variant\n");
> + break;
default?
> + }
...
> + 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);
return dev_err_probe(...);
> + }
> + dev_info(dev, "initialized ep93xx pin control driver\n");
Noise. Please drop it.
> + return 0;
> +};
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists