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

[Dan Carpenter]

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;

> +	} 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.

> +		}
> +
> +		val = pinconf_to_config_argument(cfgs[i]);
> +
> +		pin_conf |= (val << shift) & mask;
> +
> +	}
> +
> +	reserve = n_pins * (1 + n_cfgs);
> +
> +	ret = pinctrl_utils_reserve_map(pctldev, map, reserved_maps, num_maps,
> +					reserve);
> +	if (ret < 0)
> +		goto free_cfgs;
> +
> +	of_property_for_each_u32(np, "pinmux", prop, p, pinmux_group) {
> +		u32 pin_id, pin_func, daisy_id, daisy_val, daisy_valid;
> +		const char *pin_name;
> +
> +		i = 0;
> +		ncfg = IMX_SCMI_NUM_CFG;
> +		pin_id = get_pin_no(pinmux_group);
> +		pin_func = get_pin_func(pinmux_group);
> +		daisy_id = get_pin_daisy_no(pinmux_group);
> +		daisy_val = get_pin_daisy_val(pinmux_group);
> +		cfg[i++] = pinconf_to_config_packed(IMX_SCMI_PIN_MUX, pin_func);
> +		cfg[i++] = pinconf_to_config_packed(IMX_SCMI_PIN_CONFIG, pin_conf);
> +
> +		daisy_valid = get_pin_daisy_valid(pinmux_group);
> +		if (daisy_valid) {
> +			cfg[i++] = pinconf_to_config_packed(IMX_SCMI_PIN_DAISY_ID,
> +							    daisy_id);
> +			cfg[i++] = pinconf_to_config_packed(IMX_SCMI_PIN_DAISY_CFG,
> +							    daisy_val);
> +		} else {
> +			ncfg -= 2;
> +		}
> +
> +		pin_name = pin_get_name(pctldev, pin_id);
> +
> +		dev_dbg(dev, "pin: %s, pin_conf: 0x%x, daisy_id: %u, daisy_val: 0x%x\n",
> +			pin_name, pin_conf, daisy_id, daisy_val);
> +
> +		ret = pinctrl_utils_add_map_configs(pctldev, map, reserved_maps,
> +						    num_maps, pin_name,
> +						    cfg, ncfg,
> +						    PIN_MAP_TYPE_CONFIGS_PIN);
> +		if (ret < 0)
> +			goto free_cfgs;
> +	};
> +
> +
> +free_cfgs:
> +	kfree(cfgs);
> +	return ret;
> +}
> +
> +static int pinctrl_scmi_imx_dt_node_to_map(struct pinctrl_dev *pctldev,
> +					   struct device_node *np_config,
> +					   struct pinctrl_map **map,
> +					   unsigned int *num_maps)
> +
> +{
> +	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

> +		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.

		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;

> +}

regards,
dan carpenter