| Message ID | 20240916063021.311721-1-18771902331@163.com |
|---|---|
| Headers | show |
| Series | Add initial support for Canaan Kendryte K230 pinctrl | expand |
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@163.com> >> --- >> 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@163.com>"); >> +MODULE_DESCRIPTION("Canaan K230 pinctrl driver"); > Best regards, > Krzysztof