[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ce7912c3-ee63-4f3c-a15a-cf32cb8be44e@gmail.com>
Date: Mon, 29 Jan 2024 10:05:24 +0800
From: Jacky Huang <ychuang570808@...il.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
linus.walleij@...aro.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
p.zabel@...gutronix.de, j.neuschaefer@....net
Cc: linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
ychuang3@...oton.com, schung@...oton.com
Subject: Re: [PATCH v3 4/4] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO
driver
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@...oton.com>
>>
>> Add common pinctrl and GPIO driver for Nuvoton MA35 series SoC, and
>> add support for ma35d1 pinctrl.
>>
>> Signed-off-by: Jacky Huang <ychuang3@...oton.com>
>> ---
>
> 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;
>> +}
>
Powered by blists - more mailing lists