Message-ID: <7203c3da-965c-483f-b10e-6b6e8f7495a4@adtran.com>
Date: Thu, 5 Jun 2025 17:04:56 +0000
From: Piotr Kubik <piotr.kubik@...ran.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
CC: Kory Maincent <kory.maincent@...tlin.com>, Andrew Lunn
	<andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [EXTERNAL]Re: [PATCH net-next v3 2/2] net: pse-pd: Add Si3474 PSE
 controller driver

Hi Oleksij, 

Thanks for  a review.  

On 5/22/25 11:29, Oleksij Rempel wrote:
> Hi Piotr,
> 
> here are some comments.
> 
> On Fri, May 16, 2025 at 01:07:18PM +0000, Piotr Kubik wrote:
>> From: Piotr Kubik <piotr.kubik@...ran.com>
>>
>> Add a driver for the Skyworks Si3474 I2C Power Sourcing Equipment
>> controller.
>>
>> Based on the TPS23881 driver code.
>>
>> Driver supports basic features of Si3474 IC:
>> - get port status,
>> - get port power,
>> - get port voltage,
>> - enable/disable port power.
>>
>> Only 4p configurations are supported at this moment.
>>
>> Signed-off-by: Piotr Kubik <piotr.kubik@...ran.com>
>> ---
>>  drivers/net/pse-pd/Kconfig  |  10 +
>>  drivers/net/pse-pd/Makefile |   1 +
>>  drivers/net/pse-pd/si3474.c | 649 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 660 insertions(+)
>>  create mode 100644 drivers/net/pse-pd/si3474.c
>>
>> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
>> index 7fab916a7f46..d1b100eb8c52 100644
>> --- a/drivers/net/pse-pd/Kconfig
>> +++ b/drivers/net/pse-pd/Kconfig
>> @@ -32,6 +32,16 @@ config PSE_PD692X0
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called pd692x0.
>>  
>> +config PSE_SI3474
>> +	tristate "Si3474 PSE controller"
>> +	depends on I2C
>> +	help
>> +	  This module provides support for Si3474 regulator based Ethernet
>> +	  Power Sourcing Equipment.
> 
> Will be good to add here current limitation, that is supports only
> 4-pair mode.
> 

Well, at the moment it is only in commitmsg, so yes, I think it is a good place to
notify potential user.

>> +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;
>> +	bool is_enabled = false;
>> +	u8 chan0, chan1;
>> +	s32 ret;
>> +
>> +	if (id >= SI3474_MAX_CHANS)
>> +		return -ERANGE;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> 
> There are repeating patterns in client and channel calculation.
> Some of them can be uput in a separate function.
> 
>> +	if (ret < 0) {
>> +		admin_state->c33_admin_state =
>> +			ETHTOOL_C33_PSE_ADMIN_STATE_UNKNOWN;
>> +		return ret;
>> +	}
>> +
>> +	is_enabled = ((ret & (0x03 << (2 * (chan0 % 4)))) |
>> +		      (ret & (0x03 << (2 * (chan1 % 4))))) != 0;
> 
> Please replace magic numbers with defines.
> 
>> +
>> +	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;
>> +	bool delivering = false;
>> +	u8 chan0, chan1;
>> +	s32 ret;
>> +
>> +	if (id >= SI3474_MAX_CHANS)
>> +		return -ERANGE;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	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 & (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4))) != 0;
>> +
>> +	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;
>> +}
>> +
>> +/* Parse pse-pis subnode into chan array of si3474_priv */
>> +static int si3474_get_of_channels(struct si3474_priv *priv)
>> +{
>> +	struct device_node *pse_node;
>> +	struct pse_pi *pi;
>> +	u32 pi_no, chan_id;
>> +	s8 pairset_cnt;
>> +	s32 ret = 0;
>> +
>> +	pse_node = of_get_child_by_name(priv->np, "pse-pis");
>> +	if (!pse_node) {
>> +		dev_warn(&priv->client[0]->dev,
>> +			 "Unable to parse DT PSE power interface matrix, no pse-pis node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for_each_child_of_node_scoped(pse_node, node) {
>> +		if (!of_node_name_eq(node, "pse-pi"))
>> +			continue;
>> +
>> +		ret = of_property_read_u32(node, "reg", &pi_no);
>> +		if (ret) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Failed to read pse-pi reg property\n");
>> +			goto out;
>> +		}
>> +		if (pi_no >= SI3474_MAX_CHANS) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Invalid power interface number %u\n", pi_no);
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		pairset_cnt = of_property_count_elems_of_size(node, "pairsets",
>> +							      sizeof(u32));
>> +		if (!pairset_cnt) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Failed to get pairsets property\n");
>> +			goto out;
>> +		}
>> +
>> +		pi = &priv->pcdev.pi[pi_no];
>> +		if (!pi->pairset[0].np) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Missing pairset reference, power interface: %u\n",
>> +				pi_no);
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		ret = of_property_read_u32(pi->pairset[0].np, "reg", &chan_id);
>> +		if (ret) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Failed to read channel reg property, ret:%d\n",
>> +				ret);
>> +			goto out;
>> +		}
>> +		priv->pi[pi_no].chan[0] = chan_id;
> 
> should we validated chan_id?
> 
> 
OK, I will.
>> +		priv->pi[pi_no].is_4p = FALSE;
> 
> Please use lower case variant (false/true).
> 
>> +
>> +		if (pairset_cnt == 2) {
>> +			if (!pi->pairset[1].np) {
>> +				dev_err(&priv->client[0]->dev,
>> +					"Missing pairset reference, power interface: %u\n",
>> +					pi_no);
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +
>> +			ret = of_property_read_u32(pi->pairset[1].np, "reg",
>> +						   &chan_id);
>> +			if (ret) {
>> +				dev_err(&priv->client[0]->dev,
>> +					"Failed to read channel reg property\n");
>> +				goto out;
>> +			}
>> +			priv->pi[pi_no].chan[1] = chan_id;
> 
> same here
> 
>> +			priv->pi[pi_no].is_4p = TRUE;
>> +		} else {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Number of pairsets incorrect - only 4p configurations supported\n");
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +	}
>> +
>> +out:
>> +	of_node_put(pse_node);
>> +	return ret;
>> +}
>> +
> ...
> 
>> +static int si3474_pi_get_chan_current(struct si3474_priv *priv, u8 chan)
>> +{
>> +	struct i2c_client *client;
>> +	s32 ret;
>> +	u8 reg;
>> +	u64 tmp_64;
>> +
>> +	if (chan < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Registers 0x30 to 0x3d */
>> +	reg = PORT1_CURRENT_LSB_REG + (chan % 4) * 4;
> 
> Do this values are valid channels is not enabled and/or not delivering? 
> 

get_chan_current() is used in get_actual_pw only()
and there multiplied with actual channel voltage, that is zero if channel is disabled. 
I will optimize si3474_pi_get_actual_pw() to not read currents if voltage is zero.


>> +	ret = i2c_smbus_read_word_data(client, reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	tmp_64 = ret * SI3474_NA_STEP;
>> +
>> +	/* uA = nA / 1000 */
>> +	tmp_64 = DIV_ROUND_CLOSEST_ULL(tmp_64, 1000);
>> +	return (int)tmp_64;
>> +}
>> +
>> +static int si3474_pi_get_chan_voltage(struct si3474_priv *priv, u8 chan)
>> +{
>> +	struct i2c_client *client;
>> +	s32 ret;
>> +	u8 reg;
>> +	u32 val;
>> +
>> +	if (chan < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Registers 0x32 to 0x3f */
>> +	reg = PORT1_VOLTAGE_LSB_REG + (chan % 4) * 4;
>> +
>> +	ret = i2c_smbus_read_word_data(client, reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = ret * SI3474_UV_STEP;
>> +
>> +	return (int)val;
>> +}
>> +
>> +static int si3474_pi_get_voltage(struct pse_controller_dev *pcdev, int id)
>> +{
>> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
>> +	struct i2c_client *client;
>> +	u8 chan0, chan1;
>> +	s32 ret;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Check which channels are enabled*/
>> +	ret = i2c_smbus_read_byte_data(client, POWER_STATUS_REG);
>> +	if (ret < 0)
>> +		return ret;
> 
> Do voltage values are valide if channel is enabled by not delivering?
> 

If I understand your question correctly, then - yes.  
POWER_STATUS_REG returns enabled status if power is actually being delivered, 
so voltages must be valid in this case.

>> +	/* Take voltage from the first enabled channel */
>> +	if (ret & BIT(chan0 % 4))
>> +		ret = si3474_pi_get_chan_voltage(priv, chan0);
>> +	else if (ret & BIT(chan1))
> 
> should it be (chan1 % 4) ?
> 

yea, good catch. 

>> +		ret = si3474_pi_get_chan_voltage(priv, chan1);
>> +	else
>> +		/* 'should' be no voltage in this case */
>> +		return 0;
>> +
>> +	return ret;
>> +}
>> +
>> +static int si3474_pi_get_actual_pw(struct pse_controller_dev *pcdev, int id)
>> +{
>> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
>> +	s32 ret;
>> +	u32 uV, uA;
>> +	u64 tmp_64;
>> +	u8 chan0, chan1;
>> +
>> +	if (id >= SI3474_MAX_CHANS)
>> +		return -ERANGE;
>> +
>> +	ret = si3474_pi_get_voltage(&priv->pcdev, id);
>> +	if (ret < 0)
>> +		return ret;
>> +	uV = ret;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	ret = si3474_pi_get_chan_current(priv, chan0);
>> +	if (ret < 0)
>> +		return ret;
>> +	uA = ret;
>> +
>> +	ret = si3474_pi_get_chan_current(priv, chan1);
>> +	if (ret < 0)
>> +		return ret;
>> +	uA += ret;
>> +
>> +	tmp_64 = uV;
>> +	tmp_64 *= uA;
>> +	/* mW = uV * uA / 1000000000 */
>> +	return DIV_ROUND_CLOSEST_ULL(tmp_64, 1000000000);
>> +}
>> +
>> +static const struct pse_controller_ops si3474_ops = {
>> +	.setup_pi_matrix = si3474_setup_pi_matrix,
>> +	.pi_enable = si3474_pi_enable,
>> +	.pi_disable = si3474_pi_disable,
>> +	.pi_get_actual_pw = si3474_pi_get_actual_pw,
>> +	.pi_get_voltage = si3474_pi_get_voltage,
>> +	.pi_get_admin_state = si3474_pi_get_admin_state,
>> +	.pi_get_pw_status = si3474_pi_get_pw_status,
>> +};
>> +
>> +static void si3474_ancillary_i2c_remove(void *data)
>> +{
>> +	struct i2c_client *client = data;
>> +
>> +	i2c_unregister_device(client);
>> +}
>> +
>> +static int si3474_i2c_probe(struct i2c_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct si3474_priv *priv;
>> +	s32 ret;
>> +	u8 fw_version;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> +		dev_err(dev, "i2c check functionality failed\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, VENDOR_IC_ID_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret != SI3474_DEVICE_ID) {
>> +		dev_err(dev, "Wrong device ID: 0x%x\n", ret);
>> +		return -ENXIO;
>> +	}
>> +
>> +	ret = i2c_smbus_read_byte_data(client, FIRMWARE_REVISION_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +	fw_version = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, CHIP_REVISION_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	dev_dbg(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>> +		ret, fw_version);
>> +
>> +	priv->client[0] = client;
>> +	i2c_set_clientdata(client, priv);
>> +
>> +	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "secondary",
>> +						   priv->client[0]->addr + 1);
>> +	if (IS_ERR(priv->client[1]))
>> +		return PTR_ERR(priv->client[1]);
>> +
>> +	ret = devm_add_action_or_reset(dev, si3474_ancillary_i2c_remove, priv->client[1]);
>> +	if (ret < 0) {
>> +		dev_err(&priv->client[1]->dev, "Cannot register remove callback\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
>> +	if (ret < 0) {
>> +		dev_err(&priv->client[1]->dev, "Cannot access secondary PSE controller\n");
>> +		return ret;
>> +	}
>> +
>> +	if (ret != SI3474_DEVICE_ID) {
>> +		dev_err(&priv->client[1]->dev,
>> +			"Wrong device ID for secondary PSE controller: 0x%x\n", ret);
>> +		return -ENXIO;
>> +	}
>> +
>> +	priv->np = dev->of_node;
>> +	priv->pcdev.owner = THIS_MODULE;
>> +	priv->pcdev.ops = &si3474_ops;
>> +	priv->pcdev.dev = dev;
>> +	priv->pcdev.types = ETHTOOL_PSE_C33;
>> +	priv->pcdev.nr_lines = SI3474_MAX_CHANS;
> 
> Do we actually have SI3474_MAX_CHANS (8 channels) in 4p mode? I guess it
> will be 4.
> 

yes, in places where SI3474_MAX_CHANS is used we refer to physical channels which is 8 (4 PIs), 
but need to check what pcdev.nr_lines refers to.

>> +
>> +	ret = devm_pse_controller_register(dev, &priv->pcdev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register PSE controller: 0x%x\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id si3474_id[] = {
>> +	{ "si3474" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, si3474_id);
>> +
>> +static const struct of_device_id si3474_of_match[] = {
>> +	{
>> +		.compatible = "skyworks,si3474",
>> +	},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, si3474_of_match);
>> +
>> +static struct i2c_driver si3474_driver = {
>> +	.probe = si3474_i2c_probe,
>> +	.id_table = si3474_id,
>> +	.driver = {
>> +		.name = "si3474",
>> +		.of_match_table = si3474_of_match,
>> +	},
>> +};
>> +module_i2c_driver(si3474_driver);
>> +
>> +MODULE_AUTHOR("Piotr Kubik <piotr.kubik@...ran.com>");
>> +MODULE_DESCRIPTION("Skyworks Si3474 PoE PSE Controller driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.43.0
>>
> 

Thanks
/Piotr

Powered by blists - more mailing lists