| From | andy.shevchenko@gmail ... | Date | Sat, 3 Jun 2023 22:58:38 +0300 | 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
|