Re: [PATCH net-next 04/12] net: pse-pd: tps23881: Add support for power limit and measurement features

[Oleksij Rempel]

On Wed, Oct 09, 2024 at 11:05:01AM +0200, Kory Maincent wrote:
> On Wed, 9 Oct 2024 07:02:38 +0200
> Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> 
> > On Wed, Oct 02, 2024 at 06:28:00PM +0200, Kory Maincent wrote:
> > > From: Kory Maincent (Dent Project) <kory.maincent@xxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxx>  
> > 
> > > +static int tps23881_pi_get_class(struct tps23881_priv *priv, int id)
> > > +{  
> > ....
> > > +	if (chan < 4)
> > > +		class = ret >> 4;
> > > +	else
> > > +		class = ret >> 12;  
> > 
> > ....
> > > +tps23881_pi_set_2p_pw_limit(struct tps23881_priv *priv, u8 chan, u8 pol)
> > > +{  
> > ....
> > > +	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 |