Hi Dan,
> Subject: Re: [PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol for
> i.MX95
>
> On Sun, Apr 28, 2024 at 01:07:52PM +0800, Peng Fan (OSS) wrote:
> > +static int pinctrl_scmi_imx_dt_group_node_to_map(struct pinctrl_dev
> *pctldev,
> > + struct device_node *np,
> > + struct pinctrl_map **map,
> > + unsigned int
> *reserved_maps,
> > + unsigned int *num_maps,
> > + const char *func_name)
> > +{
> > + struct device *dev = pctldev->dev;
> > + unsigned long *cfgs = NULL;
> > + unsigned int n_cfgs, reserve = 1;
> > + int i, n_pins, ret;
> > + u32 ncfg, val, mask, shift, pin_conf, pinmux_group;
> > + unsigned long cfg[IMX_SCMI_NUM_CFG];
> > + enum pin_config_param param;
> > + struct property *prop;
> > + const __be32 *p;
> > +
> > + n_pins = of_property_count_u32_elems(np, "pinmux");
> > + if (n_pins < 0) {
> > + dev_warn(dev, "Can't find 'pinmux' property in
> node %pOFn\n", np);
> > + return -EINVAL;
>
> return n_pins;
Yeah. Fix in v4.
>
> > + } else if (!n_pins) {
> > + return -EINVAL;
> > + }
> > +
> > + ret = pinconf_generic_parse_dt_config(np, pctldev, &cfgs, &n_cfgs);
> > + if (ret) {
> > + dev_err(dev, "%pOF: could not parse node property\n", np);
> > + return ret;
> > + }
> > +
> > + pin_conf = 0;
> > + for (i = 0; i < n_cfgs; i++) {
> > + param = pinconf_to_config_param(cfgs[i]);
> > + ret = pinctrl_scmi_imx_map_pinconf_type(param, &mask,
> &shift);
> > + if (ret) {
> > + dev_err(dev, "Error map pinconf_type %d\n", ret);
> > + return ret;
>
> This should be goto free_cfgs.
Good catch. Fix in v4.
>
> > + }
> > +
> > + val = pinconf_to_config_argument(cfgs[i]);
> > +
...
> > +
> > +{
> > + unsigned int reserved_maps;
> > + struct device_node *np;
> > + int ret = 0;
> > +
> > + reserved_maps = 0;
> > + *map = NULL;
> > + *num_maps = 0;
> > +
> > + for_each_available_child_of_node(np_config, np) {
>
> Btw, if you used the scoped version of these loops such as
> for_each_available_child_of_node_scoped(), then
Yeah, it will simply code.
>
> > + ret = pinctrl_scmi_imx_dt_group_node_to_map(pctldev, np,
> map,
> > +
> &reserved_maps,
> > + num_maps,
> > + np_config-
> >name);
> > + if (ret < 0) {
> > + of_node_put(np);
>
> You could get rid of the calls to of_node_put(). I would move the call to
> pinctrl_utils_free_map() here.
ok. Update in v4.
>
> if (ret) {
> pinctrl_utils_free_map(pctldev, *map, *num_maps);
> return ret;
> }
>
> > + break;
> > + }
> > + }
> > +
> > + if (ret)
> > + pinctrl_utils_free_map(pctldev, *map, *num_maps);
> > +
> > + return ret;
>
> return 0;
Thanks for your quick response, I will wait for more reviews, then post v4.
Thanks,
Peng.
>
> > +}
>
> regards,
> dan carpenter
