Re: [PATCH v3 4/4] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO driver

[Jacky Huang]

Dear Christophe,

Thanks for your review.

In the next version, I will follow your suggestion and replace all instances
of devm_kzalloc() mentioned here with the use of devm_kcalloc().


Best Regards,
Jacky Huang


On 2024/1/28 下午 03:52, Christophe JAILLET wrote:
> Le 23/01/2024 à 09:06, Jacky Huang a écrit :
> > From: Jacky Huang <ychuang3@xxxxxxxxxxx>
> >
> > Add common pinctrl and GPIO driver for Nuvoton MA35 series SoC, and
> > add support for ma35d1 pinctrl.
> >
> > Signed-off-by: Jacky Huang <ychuang3@xxxxxxxxxxx>
> > ---
>
> Hi,
>
> Should there be a v4, a few nits below.
>
> CJ
>
> > +static int ma35_pinctrl_dt_node_to_map_func(struct pinctrl_dev 
> > *pctldev,
> > +                        struct device_node *np,
> > +                        struct pinctrl_map **map,
> > +                        unsigned int *num_maps)
> > +{
> > +    struct ma35_pinctrl *npctl = pinctrl_dev_get_drvdata(pctldev);
> > +    struct ma35_pin_group *grp;
> > +    struct pinctrl_map *new_map;
> > +    struct device_node *parent;
> > +    int map_num = 1;
> > +    int i;
> > +
> > +    /*
> > +     * first find the group of this node and check if we need create
> > +     * config maps for pins
> > +     */
> > +    grp = ma35_pinctrl_find_group_by_name(npctl, np->name);
> > +    if (!grp) {
> > +        dev_err(npctl->dev, "unable to find group for node %s\n", 
> > np->name);
> > +        return -EINVAL;
> > +    }
> > +
> > +    map_num += grp->npins;
> > +    new_map = devm_kzalloc(pctldev->dev, sizeof(*new_map) * map_num, 
> > GFP_KERNEL);
>
> devm_kcalloc()?
>
> > +    if (!new_map)
> > +        return -ENOMEM;
> > +
> > +    *map = new_map;
> > +    *num_maps = map_num;
> > +    /* create mux map */
> > +    parent = of_get_parent(np);
> > +    if (!parent) {
> > +        devm_kfree(pctldev->dev, new_map);
> > +        return -EINVAL;
> > +    }
> > +
> > +    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);
> > +
> > +    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->settings[i].configs;
> > +        new_map[i].data.configs.num_configs = 
> > grp->settings[i].nconfigs;
> > +    }
> > +    dev_dbg(pctldev->dev, "maps: function %s group %s num %d\n",
> > +        (*map)->data.mux.function, (*map)->data.mux.group, map_num);
> > +
> > +    return 0;
> > +}
>
> ...
>
> > +static int ma35_pinctrl_parse_groups(struct device_node *np, struct 
> > ma35_pin_group *grp,
> > +                     struct ma35_pinctrl *npctl, u32 index)
> > +{
> > +    unsigned long *configs;
> > +    unsigned int nconfigs;
> > +    struct ma35_pin_setting *pin;
> > +    const __be32 *list;
> > +    int i, j, size, ret;
> > +
> > +    dev_dbg(npctl->dev, "group(%d): %s\n", index, np->name);
> > +
> > +    grp->name = np->name;
> > +
> > +    ret = pinconf_generic_parse_dt_config(np, NULL, &configs, 
> > &nconfigs);
> > +    if (ret)
> > +        return ret;
> > +
> > +    /*
> > +     * the binding format is nuvoton,pins = <bank pin-mfp 
> > pin-function>,
> > +     * do sanity check and calculate pins number
> > +     */
> > +    list = of_get_property(np, "nuvoton,pins", &size);
> > +    size /= sizeof(*list);
> > +    if (!size || size % 3) {
> > +        dev_err(npctl->dev, "wrong setting!\n");
> > +        return -EINVAL;
> > +    }
> > +    grp->npins = size / 3;
> > +
> > +    grp->pins = devm_kzalloc(npctl->dev, grp->npins * 
> > sizeof(*grp->pins), GFP_KERNEL);
>
> devm_kcalloc()?
>
> > +    if (!grp->pins)
> > +        return -ENOMEM;
> > +
> > +    grp->settings = devm_kzalloc(npctl->dev, grp->npins * 
> > sizeof(*grp->settings), GFP_KERNEL);
>
> devm_kcalloc()?
>
> > +    if (!grp->settings)
> > +        return -ENOMEM;
> > +
> > +    pin = grp->settings;
> > +
> > +    for (i = 0, j = 0; i < size; i += 3, j++) {
> > +        pin->offset = be32_to_cpu(*list++) * 
> > MA35_MFP_REG_SZ_PER_BANK + MA35_MFP_REG_BASE;
> > +        pin->shift = (be32_to_cpu(*list++) * MA35_MFP_BITS_PER_PORT) 
> > % 32;
> > +        pin->muxval = be32_to_cpu(*list++);
> > +        pin->configs = configs;
> > +        pin->nconfigs = nconfigs;
> > +        grp->pins[j] = npctl->info->get_pin_num(pin->offset, 
> > pin->shift);
> > +        pin++;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int ma35_pinctrl_parse_functions(struct device_node *np, 
> > struct ma35_pinctrl *npctl,
> > +                    u32 index)
> > +{
> > +    struct device_node *child;
> > +    struct ma35_pin_func *func;
> > +    struct ma35_pin_group *grp;
> > +    static u32 grp_index;
> > +    u32 ret, i = 0;
> > +
> > +    dev_dbg(npctl->dev, "parse function(%d): %s\n", index, np->name);
> > +
> > +    func = &npctl->functions[index];
> > +    func->name = np->name;
> > +    func->ngroups = of_get_child_count(np);
> > +
> > +    if (func->ngroups <= 0)
> > +        return 0;
> > +
> > +    func->groups = devm_kzalloc(npctl->dev, func->ngroups * 
> > sizeof(char *), GFP_KERNEL);
>
> devm_kcalloc()?
>
> > +    if (!func->groups)
> > +        return -ENOMEM;
> > +
> > +    for_each_child_of_node(np, child) {
> > +        func->groups[i] = child->name;
> > +        grp = &npctl->groups[grp_index++];
> > +        ret = ma35_pinctrl_parse_groups(child, grp, npctl, i++);
> > +        if (ret) {
> > +            of_node_put(child);
> > +            return ret;
> > +        }
> > +    }
> > +    return 0;
> > +}