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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 16/04/2025 12:47, Piotr Kubik wrote:
> From: Piotr Kubik <piotr.kubik@adtran.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@adtran.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

> +                                    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.

> +
> +       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