Received-SPF: pass (google.com: domain of linux-kernel+bounces-123228-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 28 Mar 2024 17:17:43 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Kory Maincent <kory.maincent@bootlin.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Russ Weight <russ.weight@linux.dev>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Oleksij Rempel <o.rempel@pengutronix.de>,
	Mark Brown <broonie@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org, devicetree@vger.kernel.org,
	Dent Project <dentproject@linuxfoundation.org>
Subject: Re: [PATCH net-next v6 17/17] net: pse-pd: Add TI TPS23881 PSE
 controller driver
Message-ID: <0cc9923f-2907-471e-81b0-2ca01a6e8c79@lunn.ch>
References: <20240326-feature_poe-v6-0-c1011b6ea1cb@bootlin.com>
 <20240326-feature_poe-v6-17-c1011b6ea1cb@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240326-feature_poe-v6-17-c1011b6ea1cb@bootlin.com>

> +static int
> +tps23881_get_unused_chan(struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS],
> +			 int port_cnt)
> +{
> +	bool used;
> +	int i, j;
> +
> +	for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +		used = false;
> +
> +		for (j = 0; j < port_cnt; j++) {
> +			if (port_matrix[j].hw_chan[0] == i) {
> +				used = true;
> +				break;
> +			}
> +
> +			if (port_matrix[j].is_4p &&
> +			    port_matrix[j].hw_chan[1] == i) {
> +				used = true;
> +				break;
> +			}
> +		}
> +
> +		if (!used)
> +			return i;
> +	}
> +
> +	return -1;

nitpick: Return -ENODEV.

> +	while (cnt_4ch_grp1 < 4) {
> +		ret = tps23881_get_unused_chan(tmp_port_matrix, port_cnt);
> +		if (ret < 0) {
> +			pr_err("tps23881: port matrix issue, no chan available\n");
> +			return -ENODEV;

and then just returns ret.

> +static int
> +tps23881_set_ports_conf(struct tps23881_priv *priv,
> +			struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS])
> +{
> +	struct i2c_client *client = priv->client;
> +	int i, ret;
> +	u16 val;
> +
> +	/* Set operating mode */
> +	ret = i2c_smbus_write_word_data(client, TPS23881_REG_OP_MODE, 0xaaaa);

Could you add some #defines here? This is semiauto i think?

> +	if (ret)
> +		return ret;
> +
> +	/* Disable DC disconnect */
> +	ret = i2c_smbus_write_word_data(client, TPS23881_REG_DIS_EN, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Set port power allocation */
> +	val = 0;
> +	for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +		if (!port_matrix[i].exist)
> +			continue;
> +
> +		if (port_matrix[i].is_4p)
> +			val |= 0xf << ((port_matrix[i].lgcl_chan[0] / 2) * 4);
> +		else
> +			val |= 0x3 << ((port_matrix[i].lgcl_chan[0] / 2) * 4);
> +	}
> +	ret = i2c_smbus_write_word_data(client, TPS23881_REG_PORT_POWER, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable detection and classification */
> +	val = 0;
> +	for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +		if (!port_matrix[i].exist)
> +			continue;
> +
> +		val |= BIT(port_matrix[i].lgcl_chan[0]) |
> +		       BIT(port_matrix[i].lgcl_chan[0] + 4);
> +		if (port_matrix[i].is_4p)
> +			val |= BIT(port_matrix[i].lgcl_chan[1]) |
> +			       BIT(port_matrix[i].lgcl_chan[1] + 4);
> +	}
> +	ret = i2c_smbus_write_word_data(client, TPS23881_REG_DET_CLA_EN, 0xffff);

This looks odd. You calculate val, and then don't use it?

     Andrew