| 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: <20250718192638.681ef327@kmaincent-XPS-13-7390>
Date: Fri, 18 Jul 2025 19:26:38 +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 v5 2/2] net: pse-pd: Add Si3474 PSE controller
driver
Le Fri, 11 Jul 2025 11:25:02 +0000,
Piotr Kubik <piotr.kubik@...ran.com> a écrit :
> From: Piotr Kubik <piotr.kubik@...ran.com>
>
> Add a driver for the Skyworks Si3474 I2C Power Sourcing Equipment
> controller.
>
> 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>
> ---
It would be nice to have the patch changes description here.
...
> +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;
I think you forgot to fix the xmas style here.
> + 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, 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));
> +
> + 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;
And here.
> +
> + 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));
> +
> + 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;
And here.
> +
> + 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;
> + }
> +
> + 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");
> + }
> + return ret;
> +}
> +
> +static int si3474_pi_enable(struct pse_controller_dev *pcdev, int id)
> +{
> + struct si3474_priv *priv = to_si3474_priv(pcdev);
> + struct i2c_client *client;
> + s32 ret;
> + u8 chan0, chan1;
> + u8 val = 0;
And here.
> +
> + if (id >= SI3474_MAX_CHANS)
> + return -ERANGE;
> +
> + si3474_get_channels(priv, id, &chan0, &chan1);
> + client = si3474_get_chan_client(priv, chan0);
> +
> + /* Release PI from shutdown */
> + ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> + if (ret < 0)
> + return ret;
> +
> + val = (u8)ret;
> + val |= CHAN_MASK(chan0);
> + val |= CHAN_MASK(chan1);
> +
> + ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> + if (ret)
> + return ret;
> +
> + /* DETECT_CLASS_ENABLE must be set when using AUTO mode,
> + * otherwise PI does not power up - datasheet section 2.10.2
> + */
> + val = CHAN_BIT(chan0) | CHAN_UPPER_BIT(chan0) |
> + CHAN_BIT(chan1) | CHAN_UPPER_BIT(chan1);
> +
> + ret = i2c_smbus_write_byte_data(client, DETECT_CLASS_ENABLE_REG,
> val);
> + if (ret)
> + return ret;
> +
> + 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;
> + s32 ret;
> + u8 chan0, chan1;
> + u8 val = 0;
And here, and other places in the patch. Are you sure you did it as you
described in your cover letter?
The code seems ok otherwise.
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Powered by blists - more mailing lists