[RESEND PATCH 2/3] pinctrl: canaan: Add support for k230 SoC
Ze Huang
18771902331 at 163.com
Wed Sep 18 01:10:40 PDT 2024
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 at 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 at 163.com>");
>> +MODULE_DESCRIPTION("Canaan K230 pinctrl driver");
> Best regards,
> Krzysztof
More information about the linux-riscv
mailing list