Received-SPF: pass (google.com: domain of linux-kernel+bounces-161411-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 28 Apr 2024 15:11:05 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	Cristian Marussi <cristian.marussi@arm.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>, Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Dong Aisheng <aisheng.dong@nxp.com>, Jacky Bai <ping.bai@nxp.com>,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, imx@lists.linux.dev,
	linux-gpio@vger.kernel.org, Peng Fan <peng.fan@nxp.com>
Subject: Re: [PATCH v3 6/6] pinctrl: imx: support SCMI pinctrl protocol for
 i.MX95
Message-ID: <1bc4d494-f4c0-4bec-a2de-e9971ef4be49@moroto.mountain>
References: <20240428-pinctrl-scmi-oem-v3-v3-0-eda341eb47ed@nxp.com>
 <20240428-pinctrl-scmi-oem-v3-v3-6-eda341eb47ed@nxp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240428-pinctrl-scmi-oem-v3-v3-6-eda341eb47ed@nxp.com>

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