Hi Xianwei,
thanks for your patch!
On Wed, Dec 11, 2024 at 7:48 AM Xianwei Zhao via B4 Relay
<devnull+xianwei.zhao.amlogic.com@xxxxxxxxxx> wrote:
> From: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
>
> Add a new pinctrl driver for Amlogic SoCs. All future Amlogic
> SoCs pinctrl drives use this, such A4, A5, S6, S7 etc. To support
> new Amlogic SoCs, only need to add the corresponding dts file.
>
> Signed-off-by: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
First: are we sure these new SoCs have nothing in common
with sunxi? Because then the sunxi code should be reused.
In any way I recommend:
- Renaming drivers/pinctrl/sunxi to drivers/pinctrl/amlogic
so we keep this sorted by actual vendor, sunxi is apparently
yours (AMlogic:s) isn't it?
- Also fix MAINTAINERS accordingly.
- Add new driver under drivers/pinctrl/amlogic
- Do not change the Kconfig symbols for sunxi and
we should be fine.
> +static int aml_dt_node_to_map(struct pinctrl_dev *pctldev,
> + struct device_node *np,
> + struct pinctrl_map **map,
> + unsigned int *num_maps)
> +{
> + struct aml_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> + const struct aml_pctl_group *grp;
> + struct device *dev = info->dev;
> + struct pinctrl_map *new_map;
> + struct device_node *parent;
> + int map_num, i;
> +
> + grp = aml_pctl_find_group_by_name(info, np->name);
> + if (!grp) {
> + dev_err(dev, "unable to find group for node %pOFn\n", np);
> + return -EINVAL;
> + }
> +
> + if (grp->num_configs)
> + map_num = grp->npins + 1;
> + else
> + map_num = 1;
> + new_map = devm_kcalloc(dev, map_num, sizeof(*new_map), GFP_KERNEL);
> + if (!new_map)
> + return -ENOMEM;
> +
> + parent = of_get_parent(np);
> + if (!parent) {
> + devm_kfree(dev, new_map);
> + return -EINVAL;
> + }
> +
> + *map = new_map;
> + *num_maps = map_num;
> + new_map[0].type = PIN_MAP_TYPE_MUX_GROUP;
> + new_map[0].data.mux.function = parent->name;
> + new_map[0].data.mux.group = np->name;
> + of_node_put(parent);
> +
> + if (grp->num_configs) {
> + new_map++;
> + for (i = 0; i < grp->npins; i++) {
> + new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> + new_map[i].data.configs.group_or_pin =
> + pin_get_name(pctldev, grp->pins[i]);
> + new_map[i].data.configs.configs = grp->configs;
> + new_map[i].data.configs.num_configs = grp->num_configs;
> + }
> + }
> +
> + dev_info(dev, "maps: function %s group %s num %d\n",
> + (*map)->data.mux.function, grp->name, map_num);
> +
> + return 0;
> +}
> +
> +static void aml_dt_free_map(struct pinctrl_dev *pctldev,
> + struct pinctrl_map *map, unsigned int num_maps)
> +{
> +}
> +
> +static void aml_pin_dbg_show(struct pinctrl_dev *pcdev, struct seq_file *s,
> + unsigned int offset)
> +{
> + seq_printf(s, " %s", dev_name(pcdev->dev));
> +}
> +
> +static const struct pinctrl_ops aml_pctrl_ops = {
> + .get_groups_count = aml_get_groups_count,
> + .get_group_name = aml_get_group_name,
> + .get_group_pins = aml_get_group_pins,
> + .dt_node_to_map = aml_dt_node_to_map,
> + .dt_free_map = aml_dt_free_map,
> + .pin_dbg_show = aml_pin_dbg_show,
> +};
> +
> +static int aml_pctl_dt_calculate_pin(struct aml_pinctrl *info,
> + unsigned int bank_idx, unsigned int offset)
> +{
> + struct aml_gpio_bank *bank;
> + int retval = -EINVAL;
> + int i;
> +
> + for (i = 0; i < info->nbanks; i++) {
> + bank = &info->banks[i];
> + if (bank->bank_idx == bank_idx) {
> + if (offset < bank->gpio_chip.ngpio)
> + retval = bank->pin_base + offset;
> + break;
> + }
> + }
> + if (retval == -EINVAL)
> + dev_err(info->dev, "pin [bank:%d, offset:%d] is not present\n", bank_idx, offset);
> +
> + return retval;
> +}
> +
> +static int aml_pctl_dt_parse_groups(struct device_node *np,
> + struct aml_pctl_group *grp,
> + struct aml_pinctrl *info, int idx)
> +{
> + struct device *dev = info->dev;
> + struct aml_pinconf *conf;
> + struct property *of_pins;
> + unsigned int bank_idx;
> + unsigned int offset, npins;
> + int i = 0;
> + int ret;
> +
> + of_pins = of_find_property(np, "pinmux", NULL);
> + if (!of_pins) {
> + dev_info(dev, "Missing pinmux property\n");
> + return -ENOENT;
> + }
> +
> + npins = of_pins->length / sizeof(u32);
> + grp->npins = npins;
> + grp->name = np->name;
> + grp->pins = devm_kcalloc(dev, npins, sizeof(*grp->pins), GFP_KERNEL);
> + grp->pin_conf = devm_kcalloc(dev, npins, sizeof(*grp->pin_conf), GFP_KERNEL);
> +
> + if (!grp->pins || !grp->pin_conf)
> + return -ENOMEM;
> +
> + ret = pinconf_generic_parse_dt_config(np, info->pctl, &grp->configs,
> + &grp->num_configs);
But can't you just move this code around? grp->num_configs give the
number of configs, so why do you have to go and look up pinmux
above, can't you just use grp->num_configs instead of of_pins
and npins above?
> +static int aml_pctl_parse_functions(struct device_node *np,
> + struct aml_pinctrl *info, u32 index,
> + int *grp_index)
> +{
> + struct device *dev = info->dev;
> + struct aml_pmx_func *func;
> + struct aml_pctl_group *grp;
> + int ret, i;
> +
> + func = &info->functions[index];
> + func->name = np->name;
> + func->ngroups = of_get_child_count(np);
> + if (func->ngroups == 0)
> + return dev_err_probe(dev, -EINVAL, "No groups defined\n");
> +
> + func->groups = devm_kcalloc(dev, func->ngroups, sizeof(*func->groups), GFP_KERNEL);
> + if (!func->groups)
> + return -ENOMEM;
> +
> + i = 0;
> + for_each_child_of_node_scoped(np, child) {
> + func->groups[i] = child->name;
> + grp = &info->groups[*grp_index];
> + *grp_index += 1;
> + ret = aml_pctl_dt_parse_groups(child, grp, info, i++);
> + if (ret)
> + return ret;
> + }
> + dev_info(dev, "Function[%d\t name:%s,\tgroups:%d]\n", index, func->name, func->ngroups);
> +
> + return 0;
> +}
> +
> +static u32 aml_bank_pins(struct device_node *np)
> +{
> + u32 value;
> +
> + if (of_property_read_u32(np, "npins", &value) < 0)
> + return 0;
> + else
> + return value;
> +}
> +
> +static u32 aml_bank_reg_gpio_offset(struct device_node *np)
> +{
> + u32 value;
> +
> + if (of_property_read_u32(np, "reg-gpio-offset", &value) < 0)
> + return 0;
> + else
> + return value;
> +}
> +
> +static u32 aml_bank_reg_mux_offset(struct device_node *np)
> +{
> + u32 value;
> +
> + if (of_property_read_u32(np, "reg-mux-offset", &value) < 0)
> + return 0;
> + else
> + return value;
> +}
> +
> +static u32 aml_bank_bit_mux_offset(struct device_node *np)
> +{
> + u32 value;
> +
> + if (of_property_read_u32(np, "bit-mux-offset", &value) < 0)
> + return 0;
> + else
> + return value;
> +}
> +
> +static u32 aml_bank_index(struct device_node *np)
> +{
> + u32 value;
> +
> + if (of_property_read_u32(np, "bank-index", &value) < 0)
> + return 0;
> + else
> + return value;
> +}
Do we really need helpers for all of this? Can't you just
open code it, at least if it's just used in one place?
> +static unsigned int aml_count_pins(struct device_node *np)
> +{
> + struct device_node *child;
> + unsigned int pins = 0;
> +
> + for_each_child_of_node(np, child) {
> + if (of_property_read_bool(child, "gpio-controller"))
> + pins += aml_bank_pins(child);
> + }
> +
> + return pins;
> +}
> +
> +static void aml_pctl_dt_child_count(struct aml_pinctrl *info,
> + struct device_node *np)
> +{
> + struct device_node *child;
> +
> + for_each_child_of_node(np, child) {
> + if (of_property_read_bool(child, "gpio-controller")) {
> + info->nbanks++;
> + } else {
> + info->nfunctions++;
> + info->ngroups += of_get_child_count(child);
> + }
> + }
> +}
This looks like a weird dependency between gpio chips and
pins that I don't quite understand. Some comments and
references to the bindings will be needed so it is clear
what is going on.
> +static struct regmap *aml_map_resource(struct aml_pinctrl *info,
> + struct device_node *node, char *name)
> +{
> + struct resource res;
> + void __iomem *base;
> + int i;
> +
> + i = of_property_match_string(node, "reg-names", name);
> + if (of_address_to_resource(node, i, &res))
> + return NULL;
> +
> + base = devm_ioremap_resource(info->dev, &res);
> + if (IS_ERR(base))
> + return ERR_CAST(base);
This looks like reimplementation of
devm_platform_ioremap_resource_byname(), can't you just
pass your platform device here?
> +static int aml_pctl_probe_dt(struct platform_device *pdev,
> + struct pinctrl_desc *pctl_desc,
> + struct aml_pinctrl *info)
Because there is clearly a platform device involved.
I guess I will have more comments as the series progress, but this
is a good starting point!
Yours,
Linus Walleij
