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 |
