Re: [RESEND PATCH 2/3] pinctrl: canaan: Add support for k230 SoC

From: Ze Huang
Date: Wed Sep 18 2024 - 04:19:34 EST

Next message: Shen Jianping (ME-SE/EAD2): "RE: [PATCH v7 0/2] iio: imu: smi240: add bosch smi240 driver"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2 4/4] MAINTAINERS: add MAINTAINER for S32G2 SIUL2 GPIO driver"
In reply to: Krzysztof Kozlowski: "Re: [RESEND PATCH 2/3] pinctrl: canaan: Add support for k230 SoC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 9/16/24 11:58 PM, Krzysztof Kozlowski wrote:

On 16/09/2024 08:47, Ze Huang wrote:

Configuration of the K230 is similar to that of the K210. However, in K210,
the 256 functions for each pin are shared, whereas in K230, multiplex
functions are different for every pin.

Signed-off-by: Ze Huang <18771902331@xxxxxxx>
---
drivers/pinctrl/Kconfig | 10 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-k230.c | 674 +++++++++++++++++++++++++++++++++
3 files changed, 685 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-k230.c

...

+
+struct k230_pinctrl {
+ struct pinctrl_desc pctl;
+ struct pinctrl_dev *pctl_dev;
+ struct regmap *regmap_base;
+ void __iomem *base;
+ struct k230_pin_group *groups;
+ unsigned int ngroups;
+ struct k230_pmx_func *functions;
+ unsigned int nfunctions;
+};
+
+static struct regmap_config k230_regmap_config = {

Why is this not a const?

Will move definition of 'name' here and set to const.

+ .reg_bits = 32,
+ .val_bits = 32,
+ .max_register = 0x100,
+ .reg_stride = 4,
+};
+
+static int k230_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ return info->ngroups;
+}
+
+static const char *k230_get_group_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ return info->groups[selector].name;
+}
+
+static int k230_get_group_pins(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const unsigned int **pins,
+ unsigned int *num_pins)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ if (selector >= info->ngroups)
+ return -EINVAL;
+
+ *pins = info->groups[selector].pins;
+ *num_pins = info->groups[selector].num_pins;
+
+ return 0;
+}
+
+static inline const struct k230_pmx_func *k230_name_to_funtion(
+ const struct k230_pinctrl *info, const char *name)
+{
+ unsigned int i;
+
+ for (i = 0; i < info->nfunctions; i++) {
+ if (!strcmp(info->functions[i].name, name))
+ return &info->functions[i];
+ }
+
+ return NULL;
+}
+
+static int k230_pinctrl_parse_groups(struct device_node *np,
+ struct k230_pin_group *grp,
+ struct k230_pinctrl *info,
+ unsigned int index)
+{
+ struct device *dev = info->pctl_dev->dev;
+ const __be32 *list;
+ int size, i, ret;
+
+ grp->name = np->name;
+
+ list = of_get_property(np, "pinmux", &size);
+ size /= sizeof(*list);
+
+ grp->num_pins = size;
+ grp->pins = devm_kcalloc(dev, grp->num_pins, sizeof(*grp->pins),
+ GFP_KERNEL);
+ grp->data = devm_kcalloc(dev, grp->num_pins, sizeof(*grp->data),
+ GFP_KERNEL);
+ if (!grp->pins || !grp->data)
+ return -ENOMEM;
+
+ for (i = 0; i < size; i++) {
+ unsigned int mux_data = be32_to_cpu(*list++);
+
+ grp->pins[i] = (mux_data >> 8);
+ grp->data[i].func = (mux_data & 0xff);
+
+ ret = pinconf_generic_parse_dt_config(np, NULL,
+ &grp->data[i].configs,
+ &grp->data[i].nconfigs);
+ if (ret)
+ return ret;
+ }
+ of_node_put(np);
+

This looks like double free. There is no get in this scope.

Thanks for pointing that out. 'np' would be released by the caller. Will remove
`of_node_put()` here.

+ return 0;
+}
+
+static void k230_pinctrl_child_count(struct k230_pinctrl *info,
+ struct device_node *np)
+{
+ struct device_node *child;
+
+ for_each_child_of_node(np, child) {
+ info->nfunctions++;
+ info->ngroups += of_get_child_count(child);
+ }
+}
+
+static int k230_pinctrl_parse_functions(struct device_node *np,
+ struct k230_pinctrl *info,
+ unsigned int index)
+{
+ struct device *dev = info->pctl_dev->dev;
+ struct k230_pmx_func *func;
+ struct k230_pin_group *grp;
+ struct device_node *child;
+ static unsigned int idx, i;
+ int ret;
+
+ func = &info->functions[index];
+
+ func->name = np->name;
+ func->ngroups = of_get_child_count(np);
+ if (func->ngroups <= 0)
+ return 0;
+
+ func->groups = devm_kcalloc(dev, func->ngroups,
+ sizeof(*func->groups), GFP_KERNEL);
+ func->group_idx = devm_kcalloc(dev, func->ngroups,
+ sizeof(*func->group_idx), GFP_KERNEL);
+ if (!func->groups || !func->group_idx)
+ return -ENOMEM;
+
+ i = 0;
+
+ for_each_child_of_node(np, child) {
+ func->groups[i] = child->name;
+ func->group_idx[i] = idx;
+ grp = &info->groups[idx];
+ idx++;
+ ret = k230_pinctrl_parse_groups(child, grp, info, i++);
+ if (ret) {
+ of_node_put(child);

Use scoped loop instead.

OK, will use `for_each_child_of_node_scoped` instead.

+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int k230_pinctrl_parse_dt(struct platform_device *pdev,
+ struct k230_pinctrl *info)

Please keep all probe related code next to each other. That's quite
confusing to find probe code far away from the probe().

Noted, will do.

+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct device_node *child;
+ unsigned int i;
+ int ret;
+
+ k230_pinctrl_child_count(info, np);
+
+ info->functions = devm_kcalloc(dev, info->nfunctions,
+ sizeof(*info->functions), GFP_KERNEL);
+ info->groups = devm_kcalloc(dev, info->ngroups,
+ sizeof(*info->groups), GFP_KERNEL);
+ if (!info->functions || !info->groups)
+ return -ENOMEM;
+
+ i = 0;
+
+ for_each_child_of_node(np, child) {
+ ret = k230_pinctrl_parse_functions(child, info, i++);
+ if (ret) {
+ dev_err(dev, "failed to parse function\n");
+ of_node_put(child);

Use scoped loop instead.

Will use `for_each_child_of_node_scoped` instead.

+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static struct pinctrl_pin_desc k230_pins[] = {

Why is this not a const?

Field `drv_data` was used to point to currently activated group, which would
be updated in `set_mux()`. It was used to print the name of the current
function of pin in `k230_pinctrl_pin_dbg_show()`.

+ PINCTRL_PIN(0, "IO0"), PINCTRL_PIN(1, "IO1"), PINCTRL_PIN(2, "IO2"),
+ PINCTRL_PIN(3, "IO3"), PINCTRL_PIN(4, "IO4"), PINCTRL_PIN(5, "IO5"),
+ PINCTRL_PIN(6, "IO6"), PINCTRL_PIN(7, "IO7"), PINCTRL_PIN(8, "IO8"),
+ PINCTRL_PIN(9, "IO9"), PINCTRL_PIN(10, "IO10"), PINCTRL_PIN(11, "IO11"),
+ PINCTRL_PIN(12, "IO12"), PINCTRL_PIN(13, "IO13"), PINCTRL_PIN(14, "IO14"),
+ PINCTRL_PIN(15, "IO15"), PINCTRL_PIN(16, "IO16"), PINCTRL_PIN(17, "IO17"),
+ PINCTRL_PIN(18, "IO18"), PINCTRL_PIN(19, "IO19"), PINCTRL_PIN(20, "IO20"),
+ PINCTRL_PIN(21, "IO21"), PINCTRL_PIN(22, "IO22"), PINCTRL_PIN(23, "IO23"),
+ PINCTRL_PIN(24, "IO24"), PINCTRL_PIN(25, "IO25"), PINCTRL_PIN(26, "IO26"),
+ PINCTRL_PIN(27, "IO27"), PINCTRL_PIN(28, "IO28"), PINCTRL_PIN(29, "IO29"),
+ PINCTRL_PIN(30, "IO30"), PINCTRL_PIN(31, "IO31"), PINCTRL_PIN(32, "IO32"),
+ PINCTRL_PIN(33, "IO33"), PINCTRL_PIN(34, "IO34"), PINCTRL_PIN(35, "IO35"),
+ PINCTRL_PIN(36, "IO36"), PINCTRL_PIN(37, "IO37"), PINCTRL_PIN(38, "IO38"),
+ PINCTRL_PIN(39, "IO39"), PINCTRL_PIN(40, "IO40"), PINCTRL_PIN(41, "IO41"),
+ PINCTRL_PIN(42, "IO42"), PINCTRL_PIN(43, "IO43"), PINCTRL_PIN(44, "IO44"),
+ PINCTRL_PIN(45, "IO45"), PINCTRL_PIN(46, "IO46"), PINCTRL_PIN(47, "IO47"),
+ PINCTRL_PIN(48, "IO48"), PINCTRL_PIN(49, "IO49"), PINCTRL_PIN(50, "IO50"),
+ PINCTRL_PIN(51, "IO51"), PINCTRL_PIN(52, "IO52"), PINCTRL_PIN(53, "IO53"),
+ PINCTRL_PIN(54, "IO54"), PINCTRL_PIN(55, "IO55"), PINCTRL_PIN(56, "IO56"),
+ PINCTRL_PIN(57, "IO57"), PINCTRL_PIN(58, "IO58"), PINCTRL_PIN(59, "IO59"),
+ PINCTRL_PIN(60, "IO60"), PINCTRL_PIN(61, "IO61"), PINCTRL_PIN(62, "IO62"),
+ PINCTRL_PIN(63, "IO63")
+};
+
+static void k230_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
+ struct seq_file *s, unsigned int offset)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+ u32 val, mode, bias, drive, input, output, slew, schmitt, power;
+ struct k230_pin_group *grp = k230_pins[offset].drv_data;
+ static const char * const biasing[] = {
+ "pull none", "pull down", "pull up", "" };
+ static const char * const enable[] = {
+ "disable", "enable" };
+ static const char * const power_source[] = {
+ "3V3", "1V8" };
+ int ret;
+
+ ret = regmap_read(info->regmap_base, offset * 4, &val);
+ if (ret) {
+ dev_err(info->pctl_dev->dev,
+ "failed to read offset 0x%x\n", offset * 4);
+ return;
+ }
+
+ mode = (val & K230_PC_SEL) >> K230_SHIFT_SEL;
+ drive = (val & K230_PC_DS) >> K230_SHIFT_DS;
+ bias = (val & K230_PC_BIAS) >> K230_SHIFT_BIAS;
+ input = (val & K230_PC_IE) >> K230_SHIFT_IE;
+ output = (val & K230_PC_OE) >> K230_SHIFT_OE;
+ slew = (val & K230_PC_SL) >> K230_SHIFT_SL;
+ schmitt = (val & K230_PC_ST) >> K230_SHIFT_ST;
+ power = (val & K230_PC_MSC) >> K230_SHIFT_MSC;
+
+ seq_printf(s, "%s - strength %d - %s - %s - slewrate %s - schmitt %s - %s",
+ grp ? grp->name : "unknown",
+ drive,
+ biasing[bias],
+ input ? "input" : "output",
+ enable[slew],
+ enable[schmitt],
+ power_source[power]);
+}
+
+static int k230_dt_node_to_map(struct pinctrl_dev *pctldev,
+ struct device_node *np_config,
+ struct pinctrl_map **map,
+ unsigned int *num_maps)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+ struct device *dev = info->pctl_dev->dev;
+ const struct k230_pmx_func *func;
+ const struct k230_pin_group *grp;
+ struct pinctrl_map *new_map;
+ int map_num, i, j, idx;
+ unsigned int grp_id;
+
+ func = k230_name_to_funtion(info, np_config->name);
+ if (!func) {
+ dev_err(dev, "function %s not found\n", np_config->name);
+ return -EINVAL;
+ }
+
+ map_num = 0;
+ for (i = 0; i < func->ngroups; ++i) {
+ grp_id = func->group_idx[i];
+ /* npins of config map plus a mux map */
+ map_num += info->groups[grp_id].num_pins + 1;
+ }
+
+ new_map = kcalloc(map_num, sizeof(*new_map), GFP_KERNEL);
+ if (!new_map)
+ return -ENOMEM;
+ *map = new_map;
+ *num_maps = map_num;
+
+ idx = 0;
+ for (i = 0; i < func->ngroups; ++i) {
+ grp_id = func->group_idx[i];
+ grp = &info->groups[grp_id];
+ new_map[idx].type = PIN_MAP_TYPE_MUX_GROUP;
+ new_map[idx].data.mux.group = grp->name;
+ new_map[idx].data.mux.function = np_config->name;
+ idx++;
+
+ for (j = 0; j < grp->num_pins; ++j) {
+ new_map[idx].type = PIN_MAP_TYPE_CONFIGS_PIN;
+ new_map[idx].data.configs.group_or_pin =
+ pin_get_name(pctldev, grp->pins[j]);
+ new_map[idx].data.configs.configs =
+ grp->data[j].configs;
+ new_map[idx].data.configs.num_configs =
+ grp->data[j].nconfigs;
+ idx++;
+ }
+ }
+
+ return 0;
+}

...

+
+ ret = regmap_write(info->regmap_base, pin * 4, val);
+ if (ret) {
+ dev_err(dev, "failed to write offset 0x%x\n", pin * 4);

Isn't regmap an MMIO? If so, drop all of such messages. This just makes
unlikely error paths too big.

Acknowledged. Will drop them.

+ return ret;
+ }
+
+ return 0;
+}
+
+static int k230_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+ unsigned long *configs, unsigned int num_configs)
+{
+ enum pin_config_param param;
+ unsigned int arg, i;
+ int ret;
+
+ if (WARN_ON(pin >= K230_NPINS))

Drop WARN_ON. No need to panic kernel. Instead, handle correctly the error.

Acknowledged. Will use dev_err instead.

+ return -EINVAL;
+
+ for (i = 0; i < num_configs; i++) {
+ param = pinconf_to_config_param(configs[i]);
+ arg = pinconf_to_config_argument(configs[i]);
+ ret = k230_pinconf_set_param(pctldev, pin, param, arg);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static void k230_pconf_dbg_show(struct pinctrl_dev *pctldev,
+ struct seq_file *s, unsigned int pin)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(info->regmap_base, pin * 4, &val);
+ if (ret) {
+ dev_err(info->pctl_dev->dev, "failed to read offset 0x%x\n", pin * 4);
+ return;
+ }
+
+ seq_printf(s, " 0x%08x", val);
+}
+
+static const struct pinconf_ops k230_pinconf_ops = {
+ .is_generic = true,
+ .pin_config_get = k230_pinconf_get,
+ .pin_config_set = k230_pinconf_set,
+ .pin_config_dbg_show = k230_pconf_dbg_show,
+};
+
+static int k230_get_functions_count(struct pinctrl_dev *pctldev)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ return info->nfunctions;
+}
+
+static const char *k230_get_fname(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ return info->functions[selector].name;
+}
+
+static int k230_get_groups(struct pinctrl_dev *pctldev, unsigned int selector,
+ const char * const **groups, unsigned int *num_groups)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ *groups = info->functions[selector].groups;
+ *num_groups = info->functions[selector].ngroups;
+
+ return 0;
+}
+
+static int k230_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+ unsigned int group)
+{
+ struct k230_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+ const struct k230_pin_conf *data = info->groups[group].data;
+ struct k230_pin_group *grp = &info->groups[group];
+ const unsigned int *pins = grp->pins;
+ struct regmap *regmap;
+ unsigned int value, mask;
+ int cnt, reg;
+
+ regmap = info->regmap_base;
+
+ for (cnt = 0; cnt < grp->num_pins; cnt++) {
+ reg = pins[cnt] * 4;
+ value = data[cnt].func << K230_SHIFT_SEL;
+ mask = K230_PC_SEL;
+ regmap_update_bits(regmap, reg, mask, value);
+ k230_pins[pins[cnt]].drv_data = grp;
+ }
+
+ return 0;
+}
+
+static const struct pinmux_ops k230_pmxops = {
+ .get_functions_count = k230_get_functions_count,
+ .get_function_name = k230_get_fname,
+ .get_function_groups = k230_get_groups,
+ .set_mux = k230_set_mux,
+ .strict = true,
+};
+
+static int k230_pinctrl_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct k230_pinctrl *info;
+ struct pinctrl_desc *pctl;
+
+ info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ pctl = &info->pctl;
+
+ pctl->name = "k230-pinctrl";
+ pctl->owner = THIS_MODULE;
+ pctl->pins = k230_pins;
+ pctl->npins = ARRAY_SIZE(k230_pins);
+ pctl->pctlops = &k230_pctrl_ops;
+ pctl->pmxops = &k230_pmxops;
+ pctl->confops = &k230_pinconf_ops;
+
+ info->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(info->base))
+ return PTR_ERR(info->base);
+
+ k230_regmap_config.name = "canaan,pinctrl";

Why this is not part of definition?

Will move to definition and set regmap to const.

+ info->regmap_base = devm_regmap_init_mmio(dev, info->base,
+ &k230_regmap_config);
+ if (IS_ERR(info->regmap_base))
+ return dev_err_probe(dev, PTR_ERR(info->regmap_base),
+ "failed to init regmap\n");
+
+ info->pctl_dev = devm_pinctrl_register(dev, pctl, info);
+ if (IS_ERR(info->pctl_dev))
+ return dev_err_probe(dev, PTR_ERR(info->pctl_dev),
+ "devm_pinctrl_register failed\n");
+
+ k230_pinctrl_parse_dt(pdev, info);
+
+ return 0;
+}
+
+static const struct of_device_id k230_dt_ids[] = {
+ { .compatible = "canaan,k230-pinctrl", },
+ { /* sintenel */ }
+};
+MODULE_DEVICE_TABLE(of, k230_dt_ids);
+
+static struct platform_driver k230_pinctrl_driver = {
+ .probe = k230_pinctrl_probe,
+ .driver = {
+ .name = "k230-pinctrl",
+ .of_match_table = k230_dt_ids,
+ },
+};
+module_platform_driver(k230_pinctrl_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Ze Huang <18771902331@xxxxxxx>");
+MODULE_DESCRIPTION("Canaan K230 pinctrl driver");

Best regards,
Krzysztof

Next message: Shen Jianping (ME-SE/EAD2): "RE: [PATCH v7 0/2] iio: imu: smi240: add bosch smi240 driver"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2 4/4] MAINTAINERS: add MAINTAINER for S32G2 SIUL2 GPIO driver"
In reply to: Krzysztof Kozlowski: "Re: [RESEND PATCH 2/3] pinctrl: canaan: Add support for k230 SoC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]