Re: [PATCH net-next v5 2/2] net: pse-pd: Add Si3474 PSE controller driver

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 11 Jul 2025 11:25:02 +0000 Piotr Kubik wrote:
> +static int si3474_pi_get_admin_state(struct pse_controller_dev *pcdev, int id,
> +				     struct pse_admin_state *admin_state)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	s32 ret;
> +	u8 chan0, chan1;
> +	bool is_enabled = false;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;

Avoid defensive programming in the kernel. Since you set nr_lines
to MAX_CHANS the core should not be calling you with invalid IDs.

> +	si3474_get_channels(priv, id, &chan0, &chan1);
> +	client = si3474_get_chan_client(priv, chan0);
> +
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> +	if (ret < 0) {
> +		admin_state->c33_admin_state =
> +			ETHTOOL_C33_PSE_ADMIN_STATE_UNKNOWN;
> +		return ret;
> +	}
> +
> +	is_enabled = (ret & CHAN_MASK(chan0)) |
> +		     (ret & CHAN_MASK(chan1));

nit: here you do (ret & MASK1) | (ret & MASK2) ...

> +	if (is_enabled)
> +		admin_state->c33_admin_state =
> +			ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED;
> +	else
> +		admin_state->c33_admin_state =
> +			ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED;
> +
> +	return 0;
> +}
> +
> +static int si3474_pi_get_pw_status(struct pse_controller_dev *pcdev, int id,
> +				   struct pse_pw_status *pw_status)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	s32 ret;
> +	u8 chan0, chan1;
> +	bool delivering = false;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;
> +
> +	si3474_get_channels(priv, id, &chan0, &chan1);
> +	client = si3474_get_chan_client(priv, chan0);
> +
> +	ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
> +	if (ret < 0) {
> +		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_UNKNOWN;
> +		return ret;
> +	}
> +
> +	delivering = ret & (CHAN_UPPER_BIT(chan0) | CHAN_UPPER_BIT(chan1));

here for similar logic you do: ret & (MASK1 | MASK2) ...

> +	if (delivering)
> +		pw_status->c33_pw_status =
> +			ETHTOOL_C33_PSE_PW_D_STATUS_DELIVERING;
> +	else
> +		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_DISABLED;
> +
> +	return 0;
> +}
> +
> +static int si3474_get_of_channels(struct si3474_priv *priv)
> +{
> +	struct pse_pi *pi;
> +	u32 chan_id;
> +	s32 ret;
> +	u8 pi_no;
> +
> +	for (pi_no = 0; pi_no < SI3474_MAX_CHANS; pi_no++) {
> +		pi = &priv->pcdev.pi[pi_no];
> +		u8 pairset_no;
> +
> +		for (pairset_no = 0; pairset_no < 2; pairset_no++) {
> +			if (!pi->pairset[pairset_no].np)
> +				continue;
> +
> +			ret = of_property_read_u32(pi->pairset[pairset_no].np,
> +						   "reg", &chan_id);
> +			if (ret) {
> +				dev_err(&priv->client[0]->dev,
> +					"Failed to read channel reg property\n");
> +				return ret;
> +			}
> +			if (chan_id > SI3474_MAX_CHANS) {
> +				dev_err(&priv->client[0]->dev,
> +					"Incorrect channel number: %d\n", chan_id);
> +				return ret;

ret is not set here (it will be zero because of previous call)

> +			}
> +
> +			priv->pi[pi_no].chan[pairset_no] = chan_id;
> +			/* Mark as 4-pair if second pairset is present */
> +			priv->pi[pi_no].is_4p = (pairset_no == 1);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int si3474_setup_pi_matrix(struct pse_controller_dev *pcdev)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	s32 ret;
> +
> +	ret = si3474_get_of_channels(priv);
> +	if (ret < 0) {
> +		dev_warn(&priv->client[0]->dev,
> +			 "Unable to parse DT PSE power interface matrix\n");
> +	}

nit: unnecessary brackets around single-line statement

> +	return ret;
> +}
-- 
pw-bot: cr