Re: [PATCH net-next 04/12] net: pse-pd: tps23881: Add support for power limit and measurement features
flat view
From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: 2024-10-09 15:16:34
Also in:
linux-doc, lkml
On Wed, Oct 09, 2024 at 11:05:01AM +0200, Kory Maincent wrote:
On Wed, 9 Oct 2024 07:02:38 +0200 Oleksij Rempel [off-list ref] wrote:quoted
On Wed, Oct 02, 2024 at 06:28:00PM +0200, Kory Maincent wrote:quoted
From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> Expand PSE callbacks to support the newly introduced pi_get/set_current_limit() and pi_get_voltage() functions. These callbacks allow for power limit configuration in the TPS23881 controller. Additionally, the patch includes the detected class, the current power delivered and the power limit ranges in the status returned, providing more comprehensive PoE status reporting. Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>quoted
+static int tps23881_pi_get_class(struct tps23881_priv *priv, int id) +{....quoted
+ if (chan < 4) + class = ret >> 4; + else + class = ret >> 12;....quoted
+tps23881_pi_set_2p_pw_limit(struct tps23881_priv *priv, u8 chan, u8 pol) +{....quoted
+ reg = TPS23881_REG_2PAIR_POL1 + (chan % 4); + ret = i2c_smbus_read_word_data(client, reg); + if (ret < 0) + return ret; + + if (chan < 4) + val = (ret & 0xff00) | pol; + else + val = (ret & 0xff) | (pol << 8);This is a common pattern in this driver, we read and write two registers in one run and then calculate bit offset for the channel, can you please move it in to separate function. This can be done in a separate patch if you like.The pattern is common but the operations are always different so I didn't found a clean way of doing it. Here is a listing of it: if (chan < 4) class = ret >> 4; else class = ret >> 12; if (chan < 4) val = (ret & 0xff00) | pol; else val = (ret & 0xff) | (pol << 8); if (chan < 4) val = (u16)(ret | BIT(chan)); else val = (u16)(ret | BIT(chan + 4)); if (chan < 4) mW = (ret & 0xff) * TPS23881_MW_STEP; else mW = (ret >> 8) * TPS23881_MW_STEP; Any idea?
something like this:
/*
* 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)
{
u8 bit_offset;
if (chan < 4) {
bit_offset = field_offset;
} else {
bit_offset = field_offset;
reg_val >>= 8;
}
return (reg_val >> bit_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)
{
u8 bit_offset;
field_val &= field_mask;
if (chan < 4) {
bit_offset = field_offset;
reg_val &= ~(field_mask << bit_offset);
reg_val |= (field_val << bit_offset);
} else {
bit_offset = field_offset;
reg_val &= ~(field_mask << (bit_offset + 8));
reg_val |= (field_val << (bit_offset + 8));
}
return reg_val;
}
--
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 |