| 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: <38b02e2d-7935-4a23-b351-d23941e781b0@adtran.com>
Date: Thu, 17 Apr 2025 11:30:36 +0000
From: Piotr Kubik <piotr.kubik@...ran.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, Oleksij Rempel
<o.rempel@...gutronix.de>, 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 1/2] net: pse-pd: Add Si3474 PSE controller
driver
Thanks Krzysztof for the review, I agree with most of the issues stated, will fix.
On 4/16/25 12:54, Krzysztof Kozlowski wrote:
> [Nie otrzymujesz często wiadomości e-mail z krzk@...nel.org. Dowiedz się, dlaczego jest to ważne, na stronie https://aka.ms/LearnAboutSenderIdentification ]
>
> On 16/04/2025 12:47, 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 | 477 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 488 insertions(+)
>> create mode 100644 drivers/net/pse-pd/si3474.c
>
> Please put bindings before their user (see DT submitting patches)
>
>>
>> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
>> index 7fab916a7f46..6d2fef6c2602 100644
>> --- a/drivers/net/pse-pd/Kconfig
>> +++ b/drivers/net/pse-pd/Kconfig
>> @@ -41,4 +41,14 @@ config PSE_TPS23881
>>
>> To compile this driver as a module, choose M here: the
>> module will be called tps23881.
>> +
>> +config PSE_SI3474
>> + tristate "Si3474 PSE controller"
>> + depends on I2C
>> + help
>> + This module provides support for Si3474 regulator based Ethernet
>> + Power Sourcing Equipment.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called si3474.
>> endif
>> diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
>> index 9d2898b36737..b33b4d905cd5 100644
>> --- a/drivers/net/pse-pd/Makefile
>> +++ b/drivers/net/pse-pd/Makefile
>> @@ -6,3 +6,4 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
>> obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
>> obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
>> obj-$(CONFIG_PSE_TPS23881) += tps23881.o
>> +obj-$(CONFIG_PSE_SI3474) += si3474.o
>> \ No newline at end of file
>
> 1. Warnin ghere
> 2. Don't add your entries to the end but in more-or-less alphabetically
> sorted place.
>
>> diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
>> new file mode 100644
>> index 000000000000..a2b4b8bff393
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/si3474.c
>> @@ -0,0 +1,477 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Driver for the Skyworks Si3474 PoE PSE Controller
>> + *
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pse-pd/pse.h>
>> +
>> +#define SI3474_MAX_CHANS 8
>> +
>> +#define MANUFACTURER_ID 0x08
>> +#define IC_ID 0x05
>> +#define SI3474_DEVICE_ID (MANUFACTURER_ID << 3 | IC_ID)
>> +
>> +/* Misc registers */
>> +#define VENDOR_IC_ID_REG 0x1B
>> +#define TEMPERATURE_REG 0x2C
>> +#define FIRMWARE_REVISION_REG 0x41
>> +#define CHIP_REVISION_REG 0x43
>> +
>> +/* Main status registers */
>> +#define POWER_STATUS_REG 0x10
>> +#define PB_POWER_ENABLE_REG 0x19
>> +
>> +/* PORTn Current */
>> +#define PORT1_CURRENT_LSB_REG 0x30
>> +
>> +/* PORTn Current [mA], return in [nA] */
>> +/* 1000 * ((PORTn_CURRENT_MSB << 8) + PORTn_CURRENT_LSB) / 16384 */
>> +#define SI3474_NA_STEP (1000 * 1000 * 1000 / 16384)
>> +
>> +/* VPWR Voltage */
>> +#define VPWR_LSB_REG 0x2E
>> +#define VPWR_MSB_REG 0x2F
>> +
>> +/* PORTn Voltage */
>> +#define PORT1_VOLTAGE_LSB_REG 0x32
>> +
>> +/* VPWR Voltage [V], return in [uV] */
>> +/* 60 * (( VPWR_MSB << 8) + VPWR_LSB) / 16384 */
>> +#define SI3474_UV_STEP (1000 * 1000 * 60 / 16384)
>> +
>> +struct si3474_port_desc {
>> + u8 chan[2];
>> + bool is_4p;
>> +};
>> +
>> +struct si3474_priv {
>> + struct i2c_client *client;
>> + struct pse_controller_dev pcdev;
>> + struct device_node *np;
>> + struct si3474_port_desc port[SI3474_MAX_CHANS];
>> +};
>> +
>> +static struct si3474_priv *to_si3474_priv(struct pse_controller_dev *pcdev)
>> +{
>> + return container_of(pcdev, struct si3474_priv, pcdev);
>> +}
>> +
>> +static int si3474_pi_get_admin_state(struct pse_controller_dev *pcdev,
>> int id,
>
> Your patchset is corrupted
yeah, my mail client silently folded long lines.
>
>> + struct pse_admin_state *admin_state)
>> +{
>> + struct si3474_priv *priv = to_si3474_priv(pcdev);
>> + struct i2c_client *client = priv->client;
>> + bool enabled = FALSE;
>
> I believe it is "false", not FALSE.
>
>
> ...
>
>> +
>> + 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_info(&client->dev, "Chip revision: 0x%x, firmware version: 0x%x\n",
>> + ret, fw_version);
>
> dev_dbg, don't pollute dmesg on success.
I disagree here, as I'd like to know that device is present and what versions it runs just by looking into dmesg.
This approach is similar to other drivers, all current PSE drivers log it this way.
>
>> +
>> + priv->client = client;
>> + i2c_set_clientdata(client, priv);
>> + 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;
>> + ret = devm_pse_controller_register(dev, &priv->pcdev);
>> + if (ret) {
>> + return dev_err_probe(dev, ret,
>> + "Failed to register PSE controller\n");
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static const struct i2c_device_id si3474_id[] = {{"si3474"}, {}};
>
> Use existing kernel style for such arrays.
>
>
>
> Best regards,
> Krzysztof
Regards,
Piotr
Powered by blists - more mailing lists