Re: [PATCH RFC net-next v3 14/27] net: pse-pd: tps23881: Add support for PSE events and interrupts

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Thu, Nov 21, 2024 at 03:42:40PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Add support for PSE event reporting through interrupts. Set up the newly
> introduced devm_pse_irq_helper helper to register the interrupt. Events are
> reported for over-current and over-temperature conditions.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Change in v3:
> - Loop over interruption register to be sure the interruption pin is
>   freed before exiting the interrupt handler function.
> - Add exist variable to not report event for undescribed PIs.
> - Used helpers to convert the chan number to the PI port number.
> 
> Change in v2:
> - Remove support for OSS pin and TPC23881 specific port priority management
> ---
>  drivers/net/pse-pd/tps23881.c | 178 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 177 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> index b25561f95774..6fe8f150231f 100644
> --- a/drivers/net/pse-pd/tps23881.c
> +++ b/drivers/net/pse-pd/tps23881.c
> @@ -17,6 +17,13 @@
>  
>  #define TPS23881_MAX_CHANS 8
>  
> +#define TPS23881_REG_IT		0x0
> +#define TPS23881_REG_IT_MASK	0x1
> +#define TPS23881_REG_IT_IFAULT	BIT(5)
> +#define TPS23881_REG_IT_SUPF	BIT(7)
> +#define TPS23881_REG_FAULT	0x7
> +#define TPS23881_REG_SUPF_EVENT	0xb
> +#define TPS23881_REG_TSD	BIT(7)
>  #define TPS23881_REG_PW_STATUS	0x10
>  #define TPS23881_REG_OP_MODE	0x12
>  #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
> @@ -24,6 +31,7 @@
>  #define TPS23881_REG_DET_CLA_EN	0x14
>  #define TPS23881_REG_GEN_MASK	0x17
>  #define TPS23881_REG_NBITACC	BIT(5)
> +#define TPS23881_REG_INTEN	BIT(7)
>  #define TPS23881_REG_PW_EN	0x19
>  #define TPS23881_REG_2PAIR_POL1	0x1e
>  #define TPS23881_REG_PORT_MAP	0x26
> @@ -53,6 +61,7 @@ struct tps23881_port_desc {
>  	u8 chan[2];
>  	bool is_4p;
>  	int pw_pol;
> +	bool exist;
>  };
>  
>  struct tps23881_priv {
> @@ -791,8 +800,10 @@ tps23881_write_port_matrix(struct tps23881_priv *priv,
>  		hw_chan = port_matrix[i].hw_chan[0] % 4;
>  
>  		/* Set software port matrix for existing ports */
> -		if (port_matrix[i].exist)
> +		if (port_matrix[i].exist) {
>  			priv->port[pi_id].chan[0] = lgcl_chan;
> +			priv->port[pi_id].exist = true;
> +		}
>  
>  		/* Initialize power policy internal value */
>  		priv->port[pi_id].pw_pol = -1;
> @@ -1098,6 +1109,165 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
>  	return 0;
>  }
>  
> +/* Convert interrupt events to 0xff to be aligned with the chan
> + * number.
> + */
> +static u8 tps23881_it_export_chans_helper(u16 reg_val, u8 field_offset)

What is the meaning of _it_?

> +{
> +	u8 val;
> +
> +	val = (reg_val >> (4 + field_offset) & 0xf0) |
> +	      (reg_val >> field_offset & 0x0f);
> +
> +	return val;
> +}
> +
> +/* Convert chan number to port number */
> +static void tps23881_set_notifs_helper(struct tps23881_priv *priv,
> +				       u8 chans,
> +				       unsigned long *notifs,
> +				       unsigned long *notifs_mask,
> +				       enum ethtool_pse_events event)
> +{
> +	u8 chan;
> +	int i;
> +
> +	if (!chans)
> +		return;
> +
> +	for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +		if (!priv->port[i].exist)
> +			continue;
> +		/* No need to look at the 2nd channel in case of PoE4 as
> +		 * both registers are set.
> +		 */
> +		chan = priv->port[i].chan[0];
> +
> +		if (BIT(chan) & chans) {
> +			*notifs_mask |= BIT(i);
> +			notifs[i] |= event;
> +		}
> +	}
> +}
> +
> +static void tps23881_irq_event_over_temp(struct tps23881_priv *priv,
> +					 u16 reg_val,
> +					 unsigned long *notifs,
> +					 unsigned long *notifs_mask)
> +{
> +	int i;
> +
> +	if (reg_val & TPS23881_REG_TSD) {
> +		for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +			if (!priv->port[i].exist)
> +				continue;
> +
> +			*notifs_mask |= BIT(i);
> +			notifs[i] |= ETHTOOL_PSE_EVENT_OVER_TEMP;

Hm, should it be bound to bound to therman zone frame work and start
cooling or something? I guess this can be done in a separate step..

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |