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

[Piotr Kubik <piotr.kubik@adtran.com>]

On 5/16/25 23:38, ALOK TIWARI wrote:
> 
> On 16-05-2025 18:37, 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 | 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.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called si3474.
>> +
>>   config PSE_TPS23881
>>       tristate "TPS23881 PSE controller"
>>       depends on I2C
>> diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
>> index 9d2898b36737..cc78f7ea7f5f 100644
>> --- a/drivers/net/pse-pd/Makefile
>> +++ b/drivers/net/pse-pd/Makefile
>> @@ -5,4 +5,5 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
>>
>>   obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
>>   obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
>> +obj-$(CONFIG_PSE_SI3474) += si3474.o
>>   obj-$(CONFIG_PSE_TPS23881) += tps23881.o
>> diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
>> new file mode 100644
>> index 000000000000..7c21b475ca1a
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/si3474.c
>> @@ -0,0 +1,649 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Driver for the Skyworks Si3474 PoE PSE Controller
>> + *
>> + * Chip Architecture & Terminology:
>> + *
>> + * The Si3474 is a single-chip PoE PSE controller managing 8 physical power
>> + * delivery channels. Internally, it's structured into two logical "Quads".
>> + *
>> + * Quad 0: Manages physical channels ('ports' in datasheet) 0, 1, 2, 3
>> + * Quad 1: Manages physical channels ('ports' in datasheet) 4, 5, 6, 7
>> + *
>> + * Each Quad is accessed via a separate I2C address. The base address range is
>> + * set by hardware pins A1-A4, and the specific address selects Quad 0 (usually
>> + * the lower/even address) or Quad 1 (usually the higher/odd address).
>> + * See datasheet Table 2.2 for the address mapping.
>> + *
>> + * While the Quads manage channel-specific operations, the Si3474 package has
>> + * several resources shared across the entire chip:
>> + * - Single RESETb input pin.
>> + * - Single INTb output pin (signals interrupts from *either* Quad).
>> + * - Single OSS input pin (Emergency Shutdown).
>> + * - Global I2C Address (0x7F) used for firmware updates.
>> + * - Global status monitoring (Temperature, VDD/VPWR Undervoltage Lockout).
>> + *
>> + * Driver Architecture:
>> + *
>> + * To handle the mix of per-Quad access and shared resources correctly, this
>> + * driver treats the entire Si3474 package as one logical device. The driver
>> + * instance associated with the primary I2C address (Quad 0) takes ownership.
>> + * It discovers and manages the I2C client for the secondary address (Quad 1).
>> + * This primary instance handles shared resources like IRQ management and
>> + * registers a single PSE controller device representing all logical PIs.
>> + * Internal functions route I2C commands to the appropriate Quad's i2c_client
>> + * based on the target channel or PI.
>> + *
>> + * Terminology Mapping:
>> + *
>> + * - "PI" (Power Interface): Refers to the logical PSE port as defined by
>> + * IEEE 802.3 (typically corresponds to an RJ45 connector). This is the
>> + * `id` (0-7) used in the pse_controller_ops.
>> + * - "Channel": Refers to one of the 8 physical power control paths within
>> + * the Si3474 chip itself (hardware channels 0-7). This terminology is
>> + * used internally within the driver to avoid confusion with 'ports'.
>> + * - "Quad": One of the two internal 4-channel management units within the
>> + * Si3474, each accessed via its own I2C address.
>> + *
>> + * Relationship:
>> + * - A 2-Pair PoE PI uses 1 Channel.
>> + * - A 4-Pair PoE PI uses 2 Channels.
>> + *
>> + * ASCII Schematic:
>> + *
>> + * +-----------------------------------------------------+
>> + * |                    Si3474 Chip                      |
>> + * |                                                     |
>> + * | +---------------------+     +---------------------+ |
>> + * | |      Quad 0         |     |      Quad 1         | |
>> + * | | Channels 0, 1, 2, 3 |     | Channels 4, 5, 6, 7 | |
>> + * | +----------^----------+     +-------^-------------+ |
>> + * | I2C Addr 0 |                        | I2C Addr 1    |
>> + * |            +------------------------+               |
>> + * | (Primary Driver Instance) (Managed by Primary)      |
>> + * |                                                     |
>> + * | Shared Resources (affect whole chip):               |
>> + * |  - Single INTb Output -> Handled by Primary         |
>> + * |  - Single RESETb Input                              |
>> + * |  - Single OSS Input   -> Handled by Primary         |
>> + * |  - Global I2C Addr (0x7F) for Firmware Update       |
>> + * |  - Global Status (Temp, VDD/VPWR UVLO)              |
>> + * +-----------------------------------------------------+
>> + *        |   |   |   |        |   |   |   |
>> + *        Ch0 Ch1 Ch2 Ch3      Ch4 Ch5 Ch6 Ch7  (Physical Channels)
>> + *
>> + * Example Mapping (Logical PI to Physical Channel(s)):
>> + * * 2-Pair Mode (8 PIs):
>> + * PI 0 -> Ch 0
>> + * PI 1 -> Ch 1
>> + * ...
>> + * PI 7 -> Ch 7
>> + * * 4-Pair Mode (4 PIs):
>> + * PI 0 -> Ch 0 + Ch 1  (Managed via Quad 0 Addr)
>> + * PI 1 -> Ch 2 + Ch 3  (Managed via Quad 0 Addr)
>> + * PI 2 -> Ch 4 + Ch 5  (Managed via Quad 1 Addr)
>> + * PI 3 -> Ch 6 + Ch 7  (Managed via Quad 1 Addr)
>> + * (Note: Actual mapping depends on Device Tree and PORT_REMAP config)
>> + */
>> +
>> +#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
> 
> SI3474_MAX_CHANS, this could be sound like changes or chance
> 

Well, for me it sounds good. 
Moreover, this shortcut is widely used in kernel code for many different types of channels.

>> +
>> +#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 PORT_MODE_REG 0x12
>> +#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_pi_desc {
>> +     u8 chan[2];
> 
> here chan does not sound smooth -> channels[] ?
> especially for an array or list
> 

Same as above. 

>> +     bool is_4p;
>> +};
>> +
>> +struct si3474_priv {
>> +     struct i2c_client *client[2];
>> +     struct pse_controller_dev pcdev;
>> +     struct device_node *np;
>> +     struct si3474_pi_desc pi[SI3474_MAX_CHANS];
>> +};
> 
> Thanks,
> Alok

Thanks
/Piotr