[PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol for i.MX95

[Peng Fan]

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