| 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
| ||
|
Message-ID: <20250519115439.35382771@kmaincent-XPS-13-7390>
Date: Mon, 19 May 2025 11:54:39 +0200
From: Kory Maincent <kory.maincent@...tlin.com>
To: Piotr Kubik <piotr.kubik@...ran.com>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>, 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: [PATCH net-next v3 2/2] net: pse-pd: Add Si3474 PSE controller
driver
On Fri, 16 May 2025 13:07:18 +0000
Piotr Kubik <piotr.kubik@...ran.com> 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.
By curiosity, I suppose your hardware have only PoE4 PIs. Could you support and
test 2p configuration by only configuring 2 of the 4 pairs on one PI?
Maybe it could be done on a second stage if you prefer.
>
> Signed-off-by: Piotr Kubik <piotr.kubik@...ran.com>
...
> + is_enabled = ((ret & (0x03 << (2 * (chan0 % 4)))) |
> + (ret & (0x03 << (2 * (chan1 % 4))))) != 0;
There are precedence in the operators. I don't think you need that much of
parenthesis.
This should do the work:
is_enabled = (ret & 0x03 << chan0 % 4 * 2) |
(ret & 0x03 << chan1 % 4 * 2) != 0;
Also I saw that this kind of calculation is made several times. Maybe you could
add a helper with documentation like I did:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/net/pse-pd/tps23881.c#L79
> +/* 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;
> + }
You should not parse the pse-pis node and subnodes, it is already done before
the setup_pi_matrix ops call in the core framework.
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/net/pse-pd/pse_core.c#L131
You should rather use directly the pcdev->pi[x] table. You have to match the
the phandle save in pcdev->pi[x].pairset[0/1].np to your channel device node
and set up the hardware matrix accordingly.
That's good to have another development on PSE, this shows me that documentation
are missing or not enough in PSE core like on this ops. I will update it when I
have time.
> +
> + 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;
> + }
> +
...
> +static int si3474_pi_enable(struct pse_controller_dev *pcdev, int id)
> +{
> + struct si3474_priv *priv = to_si3474_priv(pcdev);
> + struct i2c_client *client;
> + u8 chan0, chan1;
> + u8 val = 0;
> + 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];
> +
> + /* Release pi from shutdown */
> + ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> + if (ret < 0)
> + return ret;
> +
> + val = (u8)ret;
> + val |= (0x03 << (2 * (chan0 % 4)));
> + val |= (0x03 << (2 * (chan1 % 4)));
Same calculation as before, use a helper as said before.
> +
> + ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> + if (ret)
> + return ret;
> +
> + /* Give time for transition to complete */
> + ssleep(1);
1s sleep?! It is a lot. Why do you need this? Does it comes from the datasheet?
> +
> + /* Trigger pi to power up */
> + val = (BIT(chan0 % 4) | BIT(chan1 % 4));
> + ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
> +
> + return 0;
> +}
> +
> +static int si3474_pi_disable(struct pse_controller_dev *pcdev, int id)
> +{
> + struct si3474_priv *priv = to_si3474_priv(pcdev);
> + struct i2c_client *client;
> + u8 chan0, chan1;
> + u8 val = 0;
> + 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];
> +
> + /* Trigger pi to power down */
> + val = (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4));
This calculation is also used several times. Please use a helper with
documentation.
> + ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
> +
> + /* Shutdown pi */
> + ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> + if (ret < 0)
> + return ret;
> +
> + val = (u8)ret;
> + val &= ~(0x03 << (2 * (chan0 % 4)));
> + val &= ~(0x03 << (2 * (chan1 % 4)));
Use a helper.
> +
> + ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +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;
Idem
> +
> + 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;
Idem
> +
> + ret = i2c_smbus_read_word_data(client, reg);
> + if (ret < 0)
> + return ret;
> +
> + val = ret * SI3474_UV_STEP;
> +
> + return (int)val;
> +}
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Powered by blists - more mailing lists