linux-kernel - Re: [PATCH 2/2] drivers: i3c: Add driver for NXP P3H2x4x i3c-hub device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <76c991dd-f99b-497c-9d68-9e08541be4f0@kernel.org>
Date: Wed, 12 Feb 2025 17:53:34 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Aman Kumar Pandey <aman.kumarpandey@....com>,
 linux-kernel@...r.kernel.org, linux-i3c@...ts.infradead.org,
 alexandre.belloni@...tlin.com, krzk+dt@...nel.org, robh@...nel.org,
 conor+dt@...nel.org, devicetree@...r.kernel.org
Cc: vikash.bansal@....com, priyanka.jain@....com,
 shashank.rebbapragada@....com, Frank.Li@....com
Subject: Re: [PATCH 2/2] drivers: i3c: Add driver for NXP P3H2x4x i3c-hub
 device

On 12/02/2025 14:22, Aman Kumar Pandey wrote:
> +
> +static void p3h2x4x_of_get_tp_dt_conf(struct device *dev,
> +					const struct device_node *node)
> +{
> +	struct p3h2x4x *priv = dev_get_drvdata(dev);
> +	struct device_node *dev_node;
> +	int tp_port;
> +
> +	for_each_available_child_of_node(node, dev_node) {
> +		if (!dev_node->full_name ||
> +			(sscanf(dev_node->full_name, "target-port@%d", &tp_port) != 1))

NAK, undocumented ABI. Also weird code, why are you parsing DT structure
manually? Use proper functions to get the reg.



> +			continue;
> +
> +		if (tp_port < P3H2x4x_TP_MAX_COUNT) {
> +			priv->tp_bus[tp_port].dt_available = true;
> +			priv->tp_bus[tp_port].of_node = dev_node;
> +			priv->tp_bus[tp_port].tp_mask = BIT(tp_port);
> +			priv->tp_bus[tp_port].priv = priv;
> +			priv->tp_bus[tp_port].tp_port = tp_port;
> +		}
> +	}
> +}
> +
> +/* return true when backend node exist */
> +static bool p3h2x4x_is_backend_node_exist(int port, struct p3h2x4x *priv, u32 addr)
> +{
> +	struct smbus_device *backend = NULL;
> +
> +	list_for_each_entry(backend,
> +			&priv->tp_bus[port].tp_device_entry, list) {
> +		if (backend->addr == addr)
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static int p3h2x4x_read_backend_from_p3h2x4x_dts(struct device_node *i3c_node_target,
> +					struct p3h2x4x *priv)
> +{
> +	struct device_node *i3c_node_tp;
> +	const char *compatible;
> +	int tp_port, ret;
> +	u32 addr_dts;
> +	struct smbus_device *backend;
> +
> +	if (sscanf(i3c_node_target->full_name, "target-port@%d", &tp_port) == 0)
> +		return -EINVAL;
> +
> +	if (tp_port > P3H2x4x_TP_MAX_COUNT)
> +		return -ERANGE;
> +
> +	if (tp_port < 0)
> +		return -EINVAL;
> +
> +	INIT_LIST_HEAD(&priv->tp_bus[tp_port].tp_device_entry);
> +
> +	if (priv->settings.tp[tp_port].mode == P3H2x4x_TP_MODE_I3C)
> +		return 0;
> +
> +	for_each_available_child_of_node(i3c_node_target, i3c_node_tp) {
> +
> +		ret = of_property_read_u32(i3c_node_tp, "reg", &addr_dts);
> +		if (ret)
> +			return ret;
> +
> +		if (p3h2x4x_is_backend_node_exist(tp_port, priv, addr_dts))
> +			continue;
> +
> +		ret = of_property_read_string(i3c_node_tp, "compatible", &compatible);
> +		if (ret)
> +			return ret;
> +
> +		backend = kzalloc(sizeof(*backend), GFP_KERNEL);
> +		if (!backend)
> +			return -ENOMEM;
> +
> +		backend->addr = addr_dts;
> +		backend->compatible = compatible;
> +		backend->tp_device_dt_node = i3c_node_tp;
> +		backend->client = NULL;
> +
> +		list_add(&backend->list,
> +			&priv->tp_bus[tp_port].tp_device_entry);
> +	}
> +
> +	return 0;
> +}
> +
> +static void p3h2x4x_parse_dt_tp(struct device *dev,
> +				const struct device_node *i3c_node_hub,
> +				struct p3h2x4x *priv)
> +{
> +	struct device_node *i3c_node_target;
> +	int ret;
> +
> +	for_each_available_child_of_node(i3c_node_hub, i3c_node_target) {
> +		if (!strcmp(i3c_node_target->name, "target-port")) {
> +			ret = p3h2x4x_read_backend_from_p3h2x4x_dts(i3c_node_target, priv);
> +			if (ret)
> +				dev_err(dev, "DTS entry invalid - error %d", ret);
> +		}
> +	}
> +}
> +
> +static int p3h2x4x_get_tp_local_device_dt_setting(struct device *dev,
> +					const struct device_node *node, u32 id)
> +{
> +	u8 i;
> +	u32 local_dev_count, local_dev;
> +	struct p3h2x4x *priv = dev_get_drvdata(dev);
> +
> +	if (!of_get_property(node, "local_dev", &local_dev_count))

Oh no, read DTS conding style before upstreaming such code.


Best regards,
Krzysztof