diff mbox series

[RFC,net-next,v2,03/18] net: pse-pd: tps23881: Use helpers to calculate bit offset for a channel

Message ID 20241030-feature_poe_port_prio-v2-3-9559622ee47a@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Add support for PSE port priority | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; GEN HAS DIFF 2 files changed, 142 insertions(+);
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch warning CHECK: From:/Signed-off-by: email comments mismatch: 'From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>' != 'Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kory Maincent Oct. 30, 2024, 4:53 p.m. UTC 
From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

This driver frequently follows a pattern where two registers are read or
written in a single operation, followed by calculating the bit offset for
a specific channel.

Introduce helpers to streamline this process and reduce code redundancy,
making the codebase cleaner and more maintainable.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Thanks to Oleksij for the design of the helpers functions.

Change in v2:
- New patch
---
 drivers/net/pse-pd/tps23881.c | 107 +++++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 38 deletions(-)

Comments

Oleksij Rempel Oct. 31, 2024, 6:18 a.m. UTC | #1 
On Wed, Oct 30, 2024 at 05:53:05PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> This driver frequently follows a pattern where two registers are read or
> written in a single operation, followed by calculating the bit offset for
> a specific channel.
> 
> Introduce helpers to streamline this process and reduce code redundancy,
> making the codebase cleaner and more maintainable.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Andrew Lunn Oct. 31, 2024, 9:11 p.m. UTC | #2 
> +	val = tps23881_set_val(ret, chan, 0, BIT(chan % 4), BIT(chan % 4));
> +		val = tps23881_set_val(val, chan, 0, BIT(chan % 4),
> +				       BIT(chan % 4));
> +	val = tps23881_set_val(ret, chan, 4, BIT(chan % 4), BIT(chan % 4));
> +		val = tps23881_set_val(val, chan, 4, BIT(chan % 4),
> +				       BIT(chan % 4));
> +	val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
> +		val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
> +	val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
> +	val = tps23881_calc_val(ret, chan, 4, BIT(chan % 4));
> +		val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
> +		val = tps23881_calc_val(ret, chan, 4, BIT(chan % 4));

It looks like all the callers of this helper pass BIT(chan % 4) as the
last parameter. Maybe move that into the helper as well?

     Andrew
Kory Maincent Nov. 7, 2024, 10:47 a.m. UTC | #3 
On Thu, 31 Oct 2024 22:11:18 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +	val = tps23881_set_val(ret, chan, 0, BIT(chan % 4), BIT(chan % 4));
> > +		val = tps23881_set_val(val, chan, 0, BIT(chan % 4),
> > +				       BIT(chan % 4));
> > +	val = tps23881_set_val(ret, chan, 4, BIT(chan % 4), BIT(chan % 4));
> > +		val = tps23881_set_val(val, chan, 4, BIT(chan % 4),
> > +				       BIT(chan % 4));
> > +	val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
> > +		val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
> > +	val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
> > +	val = tps23881_calc_val(ret, chan, 4, BIT(chan % 4));
> > +		val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
> > +		val = tps23881_calc_val(ret, chan, 4, BIT(chan % 4));  
> 
> It looks like all the callers of this helper pass BIT(chan % 4) as the
> last parameter. Maybe move that into the helper as well?

There is different cases in the patch 4 of the series.

Regards,
diff mbox series

Patch

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index 20eab9857817..391b8964f687 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -53,6 +53,55 @@  static struct tps23881_priv *to_tps23881_priv(struct pse_controller_dev *pcdev)
 	return container_of(pcdev, struct tps23881_priv, pcdev);
 }
 
+/*
+ * Helper to extract a value from a u16 register value, which is made of two
+ * u8 registers. The function calculates the bit offset based on the channel
+ * and extracts the relevant bits using a provided field mask.
+ *
+ * @param reg_val: The u16 register value (composed of two u8 registers).
+ * @param chan: The channel number (0-7).
+ * @param field_offset: The base bit offset to apply (e.g., 0 or 4).
+ * @param field_mask: The mask to apply to extract the required bits.
+ * @return: The extracted value for the specific channel.
+ */
+static u16 tps23881_calc_val(u16 reg_val, u8 chan, u8 field_offset,
+			     u16 field_mask)
+{
+	if (chan > 4)
+		reg_val >>= 8;
+
+	return (reg_val >> field_offset) & field_mask;
+}
+
+/*
+ * Helper to combine individual channel values into a u16 register value.
+ * The function sets the value for a specific channel in the appropriate
+ * position.
+ *
+ * @param reg_val: The current u16 register value.
+ * @param chan: The channel number (0-7).
+ * @param field_offset: The base bit offset to apply (e.g., 0 or 4).
+ * @param field_mask: The mask to apply for the field (e.g., 0x0F).
+ * @param field_val: The value to set for the specific channel (masked by
+ *                   field_mask).
+ * @return: The updated u16 register value with the channel value set.
+ */
+static u16 tps23881_set_val(u16 reg_val, u8 chan, u8 field_offset,
+			    u16 field_mask, u16 field_val)
+{
+	field_val &= field_mask;
+
+	if (chan < 4) {
+		reg_val &= ~(field_mask << field_offset);
+		reg_val |= (field_val << field_offset);
+	} else {
+		reg_val &= ~(field_mask << (field_offset + 8));
+		reg_val |= (field_val << (field_offset + 8));
+	}
+
+	return reg_val;
+}
+
 static int tps23881_pi_enable(struct pse_controller_dev *pcdev, int id)
 {
 	struct tps23881_priv *priv = to_tps23881_priv(pcdev);
@@ -69,17 +118,12 @@  static int tps23881_pi_enable(struct pse_controller_dev *pcdev, int id)
 		return ret;
 
 	chan = priv->port[id].chan[0];
-	if (chan < 4)
-		val = (u16)(ret | BIT(chan));
-	else
-		val = (u16)(ret | BIT(chan + 4));
+	val = tps23881_set_val(ret, chan, 0, BIT(chan % 4), BIT(chan % 4));
 
 	if (priv->port[id].is_4p) {
 		chan = priv->port[id].chan[1];
-		if (chan < 4)
-			val |= BIT(chan);
-		else
-			val |= BIT(chan + 4);
+		val = tps23881_set_val(val, chan, 0, BIT(chan % 4),
+				       BIT(chan % 4));
 	}
 
 	ret = i2c_smbus_write_word_data(client, TPS23881_REG_PW_EN, val);
@@ -105,17 +149,12 @@  static int tps23881_pi_disable(struct pse_controller_dev *pcdev, int id)
 		return ret;
 
 	chan = priv->port[id].chan[0];
-	if (chan < 4)
-		val = (u16)(ret | BIT(chan + 4));
-	else
-		val = (u16)(ret | BIT(chan + 8));
+	val = tps23881_set_val(ret, chan, 4, BIT(chan % 4), BIT(chan % 4));
 
 	if (priv->port[id].is_4p) {
 		chan = priv->port[id].chan[1];
-		if (chan < 4)
-			val |= BIT(chan + 4);
-		else
-			val |= BIT(chan + 8);
+		val = tps23881_set_val(val, chan, 4, BIT(chan % 4),
+				       BIT(chan % 4));
 	}
 
 	return i2c_smbus_write_word_data(client, TPS23881_REG_PW_EN, val);
@@ -127,6 +166,7 @@  static int tps23881_pi_is_enabled(struct pse_controller_dev *pcdev, int id)
 	struct i2c_client *client = priv->client;
 	bool enabled;
 	u8 chan;
+	u16 val;
 	int ret;
 
 	ret = i2c_smbus_read_word_data(client, TPS23881_REG_PW_STATUS);
@@ -134,17 +174,13 @@  static int tps23881_pi_is_enabled(struct pse_controller_dev *pcdev, int id)
 		return ret;
 
 	chan = priv->port[id].chan[0];
-	if (chan < 4)
-		enabled = ret & BIT(chan);
-	else
-		enabled = ret & BIT(chan + 4);
+	val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
+	enabled = !!(val);
 
 	if (priv->port[id].is_4p) {
 		chan = priv->port[id].chan[1];
-		if (chan < 4)
-			enabled &= !!(ret & BIT(chan));
-		else
-			enabled &= !!(ret & BIT(chan + 4));
+		val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
+		enabled &= !!(val);
 	}
 
 	/* Return enabled status only if both channel are on this state */
@@ -160,6 +196,7 @@  static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev,
 	struct i2c_client *client = priv->client;
 	bool enabled, delivering;
 	u8 chan;
+	u16 val;
 	int ret;
 
 	ret = i2c_smbus_read_word_data(client, TPS23881_REG_PW_STATUS);
@@ -167,23 +204,17 @@  static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev,
 		return ret;
 
 	chan = priv->port[id].chan[0];
-	if (chan < 4) {
-		enabled = ret & BIT(chan);
-		delivering = ret & BIT(chan + 4);
-	} else {
-		enabled = ret & BIT(chan + 4);
-		delivering = ret & BIT(chan + 8);
-	}
+	val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
+	enabled = !!(val);
+	val = tps23881_calc_val(ret, chan, 4, BIT(chan % 4));
+	delivering = !!(val);
 
 	if (priv->port[id].is_4p) {
 		chan = priv->port[id].chan[1];
-		if (chan < 4) {
-			enabled &= !!(ret & BIT(chan));
-			delivering &= !!(ret & BIT(chan + 4));
-		} else {
-			enabled &= !!(ret & BIT(chan + 4));
-			delivering &= !!(ret & BIT(chan + 8));
-		}
+		val = tps23881_calc_val(ret, chan, 0, BIT(chan % 4));
+		enabled &= !!(val);
+		val = tps23881_calc_val(ret, chan, 4, BIT(chan % 4));
+		delivering &= !!(val);
 	}
 
 	/* Return delivering status only if both channel are on this state */